Re: [U-Boot] [PATCH 1/3] dm: gpio: Add driver for MPC85XX GPIO controller
Mario Six wrote: The problem is that in 36-bit mode the physical addresses are 64-bit, which means that you get 64-bit integers when you read something from the device tree with fdtdec_get_addr. But the device tree addresses themselves seem to be 32-bit, because if I read a property like 'reg = <0xf000 0x100>', I get a 64-bit value that contains two 32-bit values, so I have to do 'addr = reg >> 32; size = reg & 0x;' to extract them (see the patch). Doing that poses a problem if you use the 32-bit mode, though, since then the physical addresses are 32-bit. After reading your comment (and a bit of digging), I found the fdtdec_get_addr_size_auto_noparent function, which seems to fix that problem (by taking the parent's address-size values into account). I'll respin the patches with that function and Simon's concerns addressed. Addresses from the reg properties should be read with functions like platform_get_resource(). You're not supposed to be reading the device tree properties directly. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/3] dm: gpio: Add driver for MPC85XX GPIO controller
On Sun, May 8, 2016 at 3:28 PM, Timur Tabi wrote: > On Mon, May 2, 2016 at 6:43 AM, Mario Six wrote: > > Regarding the address width discrepancy: The system I'm working on is a > > P1022 Qoriq, which has 36 bit width, which implies that phys_addr_t needs > > to be 64 bit. But the everything else (including the GPIO controller) > uses > > 32 bits, thus the device tree addresses are 32 bit wide. I'm not quite > sure > > how to handle this difference; DM support for this platform is brand-new, > > so there are no drivers to look to for guidance. > > I did primary development on the P1022DS, so maybe I can help. It's > been a while since I've worked on it, though. > > First, note that there are two versions of the P1022DS DTS: one is > 32-bit, and the other is 36-bit. This was back when Freescale's SOCs > were transitioning to 36-bit physical addresses. So if you're never > going to have more than 2GB of RAM in your system, you can use 32-bit > physical addresses in the DTS. > > Can you explain the problem again? Like I said, it's been a while, > but I have a hard time believing that you've discovered a new problem > on the P1022 that hasn't already been solved. > The problem is that in 36-bit mode the physical addresses are 64-bit, which means that you get 64-bit integers when you read something from the device tree with fdtdec_get_addr. But the device tree addresses themselves seem to be 32-bit, because if I read a property like 'reg = <0xf000 0x100>', I get a 64-bit value that contains two 32-bit values, so I have to do 'addr = reg >> 32; size = reg & 0x;' to extract them (see the patch). Doing that poses a problem if you use the 32-bit mode, though, since then the physical addresses are 32-bit. After reading your comment (and a bit of digging), I found the fdtdec_get_addr_size_auto_noparent function, which seems to fix that problem (by taking the parent's address-size values into account). I'll respin the patches with that function and Simon's concerns addressed. Thanks for the help! Best regards, Mario ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/3] dm: gpio: Add driver for MPC85XX GPIO controller
On Mon, May 2, 2016 at 6:43 AM, Mario Six wrote: > Regarding the address width discrepancy: The system I'm working on is a > P1022 Qoriq, which has 36 bit width, which implies that phys_addr_t needs > to be 64 bit. But the everything else (including the GPIO controller) uses > 32 bits, thus the device tree addresses are 32 bit wide. I'm not quite sure > how to handle this difference; DM support for this platform is brand-new, > so there are no drivers to look to for guidance. I did primary development on the P1022DS, so maybe I can help. It's been a while since I've worked on it, though. First, note that there are two versions of the P1022DS DTS: one is 32-bit, and the other is 36-bit. This was back when Freescale's SOCs were transitioning to 36-bit physical addresses. So if you're never going to have more than 2GB of RAM in your system, you can use 32-bit physical addresses in the DTS. Can you explain the problem again? Like I said, it's been a while, but I have a hard time believing that you've discovered a new problem on the P1022 that hasn't already been solved. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/3] dm: gpio: Add driver for MPC85XX GPIO controller
Hi Simon,
On Sun, May 1, 2016 at 7:46 PM, Simon Glass wrote:
> Hi Mario,
>
> On 26 April 2016 at 08:08, Mario Six wrote:
> >
> > Signed-off-by: Mario Six
> > ---
> > arch/powerpc/include/asm/arch-mpc85xx/gpio.h | 2 +
> > arch/powerpc/include/asm/immap_85xx.h| 2 +
> > drivers/gpio/Kconfig | 6 +
> > drivers/gpio/Makefile| 1 +
> > drivers/gpio/mpc85xx_gpio.c | 182
> +++
> > 5 files changed, 193 insertions(+)
> > create mode 100644 drivers/gpio/mpc85xx_gpio.c
> >
>
> Seems OK but I have some comments below.
>
> > diff --git a/arch/powerpc/include/asm/arch-mpc85xx/gpio.h
> b/arch/powerpc/include/asm/arch-mpc85xx/gpio.h
> > index da7352a..41b6677 100644
> > --- a/arch/powerpc/include/asm/arch-mpc85xx/gpio.h
> > +++ b/arch/powerpc/include/asm/arch-mpc85xx/gpio.h
> > @@ -14,6 +14,8 @@
> > #ifndef __ASM_ARCH_MX85XX_GPIO_H
> > #define __ASM_ARCH_MX85XX_GPIO_H
> >
> > +#ifndef CONFIG_MPC85XX_GPIO
> > #include
> > +#endif
> >
> > #endif
> > diff --git a/arch/powerpc/include/asm/immap_85xx.h
> b/arch/powerpc/include/asm/immap_85xx.h
> > index 53ca6d9..dcc50b2 100644
> > --- a/arch/powerpc/include/asm/immap_85xx.h
> > +++ b/arch/powerpc/include/asm/immap_85xx.h
> > @@ -265,6 +265,7 @@ typedef struct ccsr_pcix {
> > #define PIWAR_WRITE_SNOOP 0x5000
> > #define PIWAR_MEM_2G 0x001e
> >
> > +#ifndef CONFIG_MPC85XX_GPIO
> > typedef struct ccsr_gpio {
> > u32 gpdir;
> > u32 gpodr;
> > @@ -273,6 +274,7 @@ typedef struct ccsr_gpio {
> > u32 gpimr;
> > u32 gpicr;
> > } ccsr_gpio_t;
> > +#endif
> >
> > /* L2 Cache Registers */
> > typedef struct ccsr_l2cache {
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index 2b4624d..72a5bdc 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -143,4 +143,10 @@ config ZYNQ_GPIO
> > help
> > Supports GPIO access on Zynq SoC.
> >
> > +config MPC85XX_GPIO
> > + bool "MPC85xx GPIO driver"
> > + depends on DM_GPIO && MPC85xx
> > + help
> > + This driver supports the built-in GPIO controller of MPC85XX
> CPUs.
>
> Can you please add a bit more info - how many GPIOs and banks, what
> features are supported (input/output/etc.)
>
> > +
> > endmenu
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > index 4f071c4..1e4f16b 100644
> > --- a/drivers/gpio/Makefile
> > +++ b/drivers/gpio/Makefile
> > @@ -32,6 +32,7 @@ obj-$(CONFIG_DA8XX_GPIO) += da8xx_gpio.o
> > obj-$(CONFIG_DM644X_GPIO) += da8xx_gpio.o
> > obj-$(CONFIG_ALTERA_PIO) += altera_pio.o
> > obj-$(CONFIG_MPC83XX_GPIO) += mpc83xx_gpio.o
> > +obj-$(CONFIG_MPC85XX_GPIO) += mpc85xx_gpio.o
> > obj-$(CONFIG_SH_GPIO_PFC) += sh_pfc.o
> > obj-$(CONFIG_OMAP_GPIO)+= omap_gpio.o
> > obj-$(CONFIG_DB8500_GPIO) += db8500_gpio.o
> > diff --git a/drivers/gpio/mpc85xx_gpio.c b/drivers/gpio/mpc85xx_gpio.c
> > new file mode 100644
> > index 000..2289eb7
> > --- /dev/null
> > +++ b/drivers/gpio/mpc85xx_gpio.c
> > @@ -0,0 +1,182 @@
> > +/*
> > + * (C) Copyright 2016
> > + * Mario Six, Guntermann & Drunck GmbH, s...@gdsys.de
> > + *
> > + * based on arch/powerpc/include/asm/mpc85xx_gpio.h, which is
> > + *
> > + * Copyright 2010 eXMeritus, A Boeing Company
> > + *
> > + * SPDX-License-Identifier:GPL-2.0+
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +struct ccsr_gpio {
> > + u32 gpdir;
> > + u32 gpodr;
> > + u32 gpdat;
> > + u32 gpier;
> > + u32 gpimr;
> > + u32 gpicr;
> > +};
> > +
> > +struct mpc85xx_gpio_data {
> > + struct ccsr_gpio __iomem *base;
> > + u32 addr;
>
> ulong?
>
> > + u8 gpio_count;
>
> uint is better
>
> Also please add comments on these 3 members.
>
> > +};
> > +
> > +static inline unsigned int
>
> please put on same line
>
> > +mpc85xx_gpio_get_val(struct ccsr_gpio *base, unsigned int mask)
> > +{
> > + /* Read the requested values */
> > + return in_be32(&base->gpdat) & mask;
> > +}
> > +
> > +static inline unsigned int
> > +mpc85xx_gpio_get_dir(struct ccsr_gpio *base, unsigned int mask)
> > +{
> > + /* Read the requested values */
> > + return in_be32(&base->gpdir) & mask;
> > +}
> > +
> > +static inline void
> > +mpc85xx_gpio_set_in(struct ccsr_gpio *base, unsigned int gpios)
> > +{
> > + clrbits_be32(&base->gpdat, gpios);
> > + clrbits_be32(&base->gpdir, gpios);
> > +}
> > +
> > +static inline void
> > +mpc85xx_gpio_set_low(struct ccsr_gpio *base, unsigned int gpios)
> > +{
> > + clrbits_be32(&base->gpdat, gpios);
> > + setbits_be32(&base->gpdir, gpios);
> > +}
> > +
> > +static inline void
> > +mpc85xx_gpio_set_high(struct ccsr_gpio *base, unsigned int gpios)
> > +{
> > + setbits_be32(&base->
Re: [U-Boot] [PATCH 1/3] dm: gpio: Add driver for MPC85XX GPIO controller
Hi Mario,
On 26 April 2016 at 08:08, Mario Six wrote:
>
> Signed-off-by: Mario Six
> ---
> arch/powerpc/include/asm/arch-mpc85xx/gpio.h | 2 +
> arch/powerpc/include/asm/immap_85xx.h| 2 +
> drivers/gpio/Kconfig | 6 +
> drivers/gpio/Makefile| 1 +
> drivers/gpio/mpc85xx_gpio.c | 182
> +++
> 5 files changed, 193 insertions(+)
> create mode 100644 drivers/gpio/mpc85xx_gpio.c
>
Seems OK but I have some comments below.
> diff --git a/arch/powerpc/include/asm/arch-mpc85xx/gpio.h
> b/arch/powerpc/include/asm/arch-mpc85xx/gpio.h
> index da7352a..41b6677 100644
> --- a/arch/powerpc/include/asm/arch-mpc85xx/gpio.h
> +++ b/arch/powerpc/include/asm/arch-mpc85xx/gpio.h
> @@ -14,6 +14,8 @@
> #ifndef __ASM_ARCH_MX85XX_GPIO_H
> #define __ASM_ARCH_MX85XX_GPIO_H
>
> +#ifndef CONFIG_MPC85XX_GPIO
> #include
> +#endif
>
> #endif
> diff --git a/arch/powerpc/include/asm/immap_85xx.h
> b/arch/powerpc/include/asm/immap_85xx.h
> index 53ca6d9..dcc50b2 100644
> --- a/arch/powerpc/include/asm/immap_85xx.h
> +++ b/arch/powerpc/include/asm/immap_85xx.h
> @@ -265,6 +265,7 @@ typedef struct ccsr_pcix {
> #define PIWAR_WRITE_SNOOP 0x5000
> #define PIWAR_MEM_2G 0x001e
>
> +#ifndef CONFIG_MPC85XX_GPIO
> typedef struct ccsr_gpio {
> u32 gpdir;
> u32 gpodr;
> @@ -273,6 +274,7 @@ typedef struct ccsr_gpio {
> u32 gpimr;
> u32 gpicr;
> } ccsr_gpio_t;
> +#endif
>
> /* L2 Cache Registers */
> typedef struct ccsr_l2cache {
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 2b4624d..72a5bdc 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -143,4 +143,10 @@ config ZYNQ_GPIO
> help
> Supports GPIO access on Zynq SoC.
>
> +config MPC85XX_GPIO
> + bool "MPC85xx GPIO driver"
> + depends on DM_GPIO && MPC85xx
> + help
> + This driver supports the built-in GPIO controller of MPC85XX CPUs.
Can you please add a bit more info - how many GPIOs and banks, what
features are supported (input/output/etc.)
> +
> endmenu
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 4f071c4..1e4f16b 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_DA8XX_GPIO) += da8xx_gpio.o
> obj-$(CONFIG_DM644X_GPIO) += da8xx_gpio.o
> obj-$(CONFIG_ALTERA_PIO) += altera_pio.o
> obj-$(CONFIG_MPC83XX_GPIO) += mpc83xx_gpio.o
> +obj-$(CONFIG_MPC85XX_GPIO) += mpc85xx_gpio.o
> obj-$(CONFIG_SH_GPIO_PFC) += sh_pfc.o
> obj-$(CONFIG_OMAP_GPIO)+= omap_gpio.o
> obj-$(CONFIG_DB8500_GPIO) += db8500_gpio.o
> diff --git a/drivers/gpio/mpc85xx_gpio.c b/drivers/gpio/mpc85xx_gpio.c
> new file mode 100644
> index 000..2289eb7
> --- /dev/null
> +++ b/drivers/gpio/mpc85xx_gpio.c
> @@ -0,0 +1,182 @@
> +/*
> + * (C) Copyright 2016
> + * Mario Six, Guntermann & Drunck GmbH, s...@gdsys.de
> + *
> + * based on arch/powerpc/include/asm/mpc85xx_gpio.h, which is
> + *
> + * Copyright 2010 eXMeritus, A Boeing Company
> + *
> + * SPDX-License-Identifier:GPL-2.0+
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +struct ccsr_gpio {
> + u32 gpdir;
> + u32 gpodr;
> + u32 gpdat;
> + u32 gpier;
> + u32 gpimr;
> + u32 gpicr;
> +};
> +
> +struct mpc85xx_gpio_data {
> + struct ccsr_gpio __iomem *base;
> + u32 addr;
ulong?
> + u8 gpio_count;
uint is better
Also please add comments on these 3 members.
> +};
> +
> +static inline unsigned int
please put on same line
> +mpc85xx_gpio_get_val(struct ccsr_gpio *base, unsigned int mask)
> +{
> + /* Read the requested values */
> + return in_be32(&base->gpdat) & mask;
> +}
> +
> +static inline unsigned int
> +mpc85xx_gpio_get_dir(struct ccsr_gpio *base, unsigned int mask)
> +{
> + /* Read the requested values */
> + return in_be32(&base->gpdir) & mask;
> +}
> +
> +static inline void
> +mpc85xx_gpio_set_in(struct ccsr_gpio *base, unsigned int gpios)
> +{
> + clrbits_be32(&base->gpdat, gpios);
> + clrbits_be32(&base->gpdir, gpios);
> +}
> +
> +static inline void
> +mpc85xx_gpio_set_low(struct ccsr_gpio *base, unsigned int gpios)
> +{
> + clrbits_be32(&base->gpdat, gpios);
> + setbits_be32(&base->gpdir, gpios);
> +}
> +
> +static inline void
> +mpc85xx_gpio_set_high(struct ccsr_gpio *base, unsigned int gpios)
> +{
> + setbits_be32(&base->gpdat, gpios);
> + setbits_be32(&base->gpdir, gpios);
> +}
> +
> +static int mpc85xx_gpio_direction_input(struct udevice *dev, unsigned int
> gpio)
> +{
> + struct mpc85xx_gpio_data *data = dev_get_priv(dev);
> +
> + mpc85xx_gpio_set_in(data->base, 1U << (31 - gpio));
How about defining something at the top and using that throughout:
#define GPIO_MASK(gp
[U-Boot] [PATCH 1/3] dm: gpio: Add driver for MPC85XX GPIO controller
Signed-off-by: Mario Six
---
arch/powerpc/include/asm/arch-mpc85xx/gpio.h | 2 +
arch/powerpc/include/asm/immap_85xx.h| 2 +
drivers/gpio/Kconfig | 6 +
drivers/gpio/Makefile| 1 +
drivers/gpio/mpc85xx_gpio.c | 182 +++
5 files changed, 193 insertions(+)
create mode 100644 drivers/gpio/mpc85xx_gpio.c
diff --git a/arch/powerpc/include/asm/arch-mpc85xx/gpio.h
b/arch/powerpc/include/asm/arch-mpc85xx/gpio.h
index da7352a..41b6677 100644
--- a/arch/powerpc/include/asm/arch-mpc85xx/gpio.h
+++ b/arch/powerpc/include/asm/arch-mpc85xx/gpio.h
@@ -14,6 +14,8 @@
#ifndef __ASM_ARCH_MX85XX_GPIO_H
#define __ASM_ARCH_MX85XX_GPIO_H
+#ifndef CONFIG_MPC85XX_GPIO
#include
+#endif
#endif
diff --git a/arch/powerpc/include/asm/immap_85xx.h
b/arch/powerpc/include/asm/immap_85xx.h
index 53ca6d9..dcc50b2 100644
--- a/arch/powerpc/include/asm/immap_85xx.h
+++ b/arch/powerpc/include/asm/immap_85xx.h
@@ -265,6 +265,7 @@ typedef struct ccsr_pcix {
#define PIWAR_WRITE_SNOOP 0x5000
#define PIWAR_MEM_2G 0x001e
+#ifndef CONFIG_MPC85XX_GPIO
typedef struct ccsr_gpio {
u32 gpdir;
u32 gpodr;
@@ -273,6 +274,7 @@ typedef struct ccsr_gpio {
u32 gpimr;
u32 gpicr;
} ccsr_gpio_t;
+#endif
/* L2 Cache Registers */
typedef struct ccsr_l2cache {
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 2b4624d..72a5bdc 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -143,4 +143,10 @@ config ZYNQ_GPIO
help
Supports GPIO access on Zynq SoC.
+config MPC85XX_GPIO
+ bool "MPC85xx GPIO driver"
+ depends on DM_GPIO && MPC85xx
+ help
+ This driver supports the built-in GPIO controller of MPC85XX CPUs.
+
endmenu
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 4f071c4..1e4f16b 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_DA8XX_GPIO) += da8xx_gpio.o
obj-$(CONFIG_DM644X_GPIO) += da8xx_gpio.o
obj-$(CONFIG_ALTERA_PIO) += altera_pio.o
obj-$(CONFIG_MPC83XX_GPIO) += mpc83xx_gpio.o
+obj-$(CONFIG_MPC85XX_GPIO) += mpc85xx_gpio.o
obj-$(CONFIG_SH_GPIO_PFC) += sh_pfc.o
obj-$(CONFIG_OMAP_GPIO)+= omap_gpio.o
obj-$(CONFIG_DB8500_GPIO) += db8500_gpio.o
diff --git a/drivers/gpio/mpc85xx_gpio.c b/drivers/gpio/mpc85xx_gpio.c
new file mode 100644
index 000..2289eb7
--- /dev/null
+++ b/drivers/gpio/mpc85xx_gpio.c
@@ -0,0 +1,182 @@
+/*
+ * (C) Copyright 2016
+ * Mario Six, Guntermann & Drunck GmbH, s...@gdsys.de
+ *
+ * based on arch/powerpc/include/asm/mpc85xx_gpio.h, which is
+ *
+ * Copyright 2010 eXMeritus, A Boeing Company
+ *
+ * SPDX-License-Identifier:GPL-2.0+
+ */
+
+#include
+#include
+#include
+#include
+
+DECLARE_GLOBAL_DATA_PTR;
+
+struct ccsr_gpio {
+ u32 gpdir;
+ u32 gpodr;
+ u32 gpdat;
+ u32 gpier;
+ u32 gpimr;
+ u32 gpicr;
+};
+
+struct mpc85xx_gpio_data {
+ struct ccsr_gpio __iomem *base;
+ u32 addr;
+ u8 gpio_count;
+};
+
+static inline unsigned int
+mpc85xx_gpio_get_val(struct ccsr_gpio *base, unsigned int mask)
+{
+ /* Read the requested values */
+ return in_be32(&base->gpdat) & mask;
+}
+
+static inline unsigned int
+mpc85xx_gpio_get_dir(struct ccsr_gpio *base, unsigned int mask)
+{
+ /* Read the requested values */
+ return in_be32(&base->gpdir) & mask;
+}
+
+static inline void
+mpc85xx_gpio_set_in(struct ccsr_gpio *base, unsigned int gpios)
+{
+ clrbits_be32(&base->gpdat, gpios);
+ clrbits_be32(&base->gpdir, gpios);
+}
+
+static inline void
+mpc85xx_gpio_set_low(struct ccsr_gpio *base, unsigned int gpios)
+{
+ clrbits_be32(&base->gpdat, gpios);
+ setbits_be32(&base->gpdir, gpios);
+}
+
+static inline void
+mpc85xx_gpio_set_high(struct ccsr_gpio *base, unsigned int gpios)
+{
+ setbits_be32(&base->gpdat, gpios);
+ setbits_be32(&base->gpdir, gpios);
+}
+
+static int mpc85xx_gpio_direction_input(struct udevice *dev, unsigned int gpio)
+{
+ struct mpc85xx_gpio_data *data = dev_get_priv(dev);
+
+ mpc85xx_gpio_set_in(data->base, 1U << (31 - gpio));
+ return 0;
+}
+
+static int
+mpc85xx_gpio_set_value(struct udevice *dev, unsigned gpio, int value)
+{
+ struct mpc85xx_gpio_data *data = dev_get_priv(dev);
+
+ if (value)
+ mpc85xx_gpio_set_high(data->base, 1U << (31 - gpio));
+ else
+ mpc85xx_gpio_set_low(data->base, 1U << (31 - gpio));
+ return 0;
+}
+
+static int
+mpc85xx_gpio_direction_output(struct udevice *dev, unsigned gpio, int value)
+{
+ struct mpc85xx_gpio_data *data = dev_get_priv(dev);
+
+ if (value)
+ mpc85xx_gpio_set_high(data->base, 1U << (31 - gpio));
+ else
+ mpc85xx_gpio_set_low(data->base, 1U << (31 - gpio));
+
