[U-Boot] [PATCH 0/3] dm: gpio: Add driver for MPC85xx GPIO controller

[U-Boot] [PATCH 0/3] dm: gpio: Add driver for MPC85xx GPIO controller

[Mario Six]

The functions for accessing GPIOs on MPC85xx are hardcoded in
arch/powerpc/include/asm/mpc85xx_gpio.h This leads to problems if another GPIO
controller supporting the driver model is to be used simultaneously.

Therefore, this patch moves the "static" functions into a DM-compatible driver,
and also introduces a set of functions into the GPIO uclass that expose the
controller's capability to switch individual GPIOs into open-drain-mode.

Mario Six (3):
  dm: gpio: Add driver for MPC85XX GPIO controller
  dm: gpio: Add methods for open drain setting
  dm: gpio: Implement open drain for MPC85XX GPIO

 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/gpio-uclass.c                   |  30 ++++
 drivers/gpio/mpc85xx_gpio.c                  | 223 +++++++++++++++++++++++++++
 include/asm-generic/gpio.h                   |  29 ++++
 7 files changed, 293 insertions(+)
 create mode 100644 drivers/gpio/mpc85xx_gpio.c

--
2.7.0.GIT

[U-Boot] [PATCH 1/3] dm: gpio: Add driver for MPC85XX GPIO controller

[Mario Six]

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---
 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 <asm/mpc85xx_gpio.h>
+#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	0x00005000
 #define PIWAR_MEM_2G		0x0000001e
 
+#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 0000000..2289eb7
--- /dev/null
+++ b/drivers/gpio/mpc85xx_gpio.c
@@ -0,0 +1,182 @@
+/*
+ * (C) Copyright 2016
+ * Mario Six, Guntermann & Drunck GmbH, six at 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 <common.h>
+#include <dm.h>
+#include <asm/gpio.h>
+#include <mapmem.h>
+
+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));
+	return 0;
+}
+
+static int
+mpc85xx_gpio_get_value(struct udevice *dev, unsigned gpio)
+{
+	struct mpc85xx_gpio_data *data = dev_get_priv(dev);
+
+	return !!mpc85xx_gpio_get_val(data->base, 1U << (31 - gpio));
+}
+
+static int
+mpc85xx_gpio_get_function(struct udevice *dev, unsigned gpio)
+{
+	struct mpc85xx_gpio_data *data = dev_get_priv(dev);
+	int dir;
+
+	dir = !!mpc85xx_gpio_get_dir(data->base, 1U << (31 - gpio));
+	return dir ? GPIOF_OUTPUT : GPIOF_INPUT;
+}
+
+static int
+mpc85xx_gpio_ofdata_to_platdata(struct udevice *dev) {
+	struct mpc85xx_gpio_data *data = dev_get_priv(dev);
+	u64 reg;
+	u32 addr, size;
+
+	reg = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
+	addr = reg >> 32;
+	size = reg & 0xFFFFFFFF;
+
+	data->addr = addr;
+	data->base = map_sysmem(CONFIG_SYS_IMMR + addr, size);
+
+	if (!data->base)
+		return -ENOMEM;
+
+	data->gpio_count = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
+					  "ngpios", 32);
+
+	return 0;
+}
+
+static int
+mpc85xx_gpio_probe(struct udevice *dev)
+{
+	struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
+	struct mpc85xx_gpio_data *data = dev_get_priv(dev);
+	char name[32], *str;
+
+	snprintf(name, sizeof(name), "MPC@%x_", data->addr);
+	str = strdup(name);
+
+	if (!str)
+		return -ENOMEM;
+
+	uc_priv->bank_name = str;
+
+	uc_priv->gpio_count = data->gpio_count;
+
+	return 0;
+}
+
+static const struct dm_gpio_ops gpio_mpc85xx_ops = {
+	.direction_input	= mpc85xx_gpio_direction_input,
+	.direction_output	= mpc85xx_gpio_direction_output,
+	.get_value		= mpc85xx_gpio_get_value,
+	.set_value		= mpc85xx_gpio_set_value,
+	.get_function		= mpc85xx_gpio_get_function,
+};
+
+static const struct udevice_id mpc85xx_gpio_ids[] = {
+	{ .compatible = "fsl,pq3-gpio" },
+	{ /* sentinel */ }
+};
+
+U_BOOT_DRIVER(gpio_mpc85xx) = {
+	.name	= "gpio_mpc85xx",
+	.id	= UCLASS_GPIO,
+	.ops	= &gpio_mpc85xx_ops,
+	.ofdata_to_platdata = mpc85xx_gpio_ofdata_to_platdata,
+	.of_match = mpc85xx_gpio_ids,
+	.probe	= mpc85xx_gpio_probe,
+	.priv_auto_alloc_size = sizeof(struct mpc85xx_gpio_data),
+};
-- 
2.7.0.GIT

[U-Boot] [PATCH 2/3] dm: gpio: Add methods for open drain setting

[Mario Six]

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---
 drivers/gpio/gpio-uclass.c | 30 ++++++++++++++++++++++++++++++
 include/asm-generic/gpio.h | 29 +++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
index b58d4e6..16b9648 100644
--- a/drivers/gpio/gpio-uclass.c
+++ b/drivers/gpio/gpio-uclass.c
@@ -352,6 +352,36 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int value)
 	return 0;
 }
 
+int dm_gpio_get_open_drain(struct gpio_desc *desc)
+{
+	struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
+	int ret;
+
+	ret = check_reserved(desc, "get_open_drain");
+	if (ret)
+		return ret;
+
+	if (ops->set_open_drain)
+		return ops->get_open_drain(desc->dev, desc->offset);
+	else
+		return 0;
+}
+
+int dm_gpio_set_open_drain(struct gpio_desc *desc, int value)
+{
+	struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
+	int ret;
+
+	ret = check_reserved(desc, "set_open_drain");
+	if (ret)
+		return ret;
+
+	if (ops->set_open_drain)
+		ops->set_open_drain(desc->dev, desc->offset, value);
+
+	return 0;
+}
+
 int dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags)
 {
 	struct udevice *dev = desc->dev;
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index 68b5f0b..ba65080 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -241,6 +241,8 @@ struct dm_gpio_ops {
 				int value);
 	int (*get_value)(struct udevice *dev, unsigned offset);
 	int (*set_value)(struct udevice *dev, unsigned offset, int value);
+	int (*get_open_drain)(struct udevice *dev, unsigned offset);
+	int (*set_open_drain)(struct udevice *dev, unsigned offset, int value);
 	/**
 	 * get_function() Get the GPIO function
 	 *
@@ -541,6 +543,33 @@ int dm_gpio_get_value(const struct gpio_desc *desc);
 int dm_gpio_set_value(const struct gpio_desc *desc, int value);
 
 /**
+ * dm_gpio_get_open_drain() - Check if open-drain-mode of a GPIO is active
+ *
+ * This checks if open-drain-mode for a GPIO is enabled or not. This method is
+ * optional; if the driver does not support it, nothing happens when the method
+ * is called.
+ *
+ * @desc:	GPIO description containing device, offset and flags,
+ *		previously returned by gpio_request_by_name()
+ * @return Value of open drain mode for GPIO (0 for inactive, 1 for active) or
+ *	   -ve on error
+ */
+int dm_gpio_get_open_drain(struct gpio_desc *desc);
+
+/**
+ * dm_gpio_set_open_drain() - Switch open-drain-mode of a GPIO on or off
+ *
+ * This enables or disables open-drain-mode for a GPIO. This method is
+ * optional; if the driver does not support it, nothing happens when the method
+ * is called.
+ *
+ * @desc:	GPIO description containing device, offset and flags,
+ *		previously returned by gpio_request_by_name()
+ * @return 0 if OK, -ve on error
+ */
+int dm_gpio_set_open_drain(struct gpio_desc *desc, int value);
+
+/**
  * dm_gpio_set_dir() - Set the direction for a GPIO
  *
  * This sets up the direction according tot the provided flags. It will do
-- 
2.7.0.GIT

[U-Boot] [PATCH 3/3] dm: gpio: Implement open drain for MPC85XX GPIO

[Mario Six]

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---
 drivers/gpio/mpc85xx_gpio.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/gpio/mpc85xx_gpio.c b/drivers/gpio/mpc85xx_gpio.c
index 2289eb7..6e920e6 100644
--- a/drivers/gpio/mpc85xx_gpio.c
+++ b/drivers/gpio/mpc85xx_gpio.c
@@ -66,6 +66,25 @@ mpc85xx_gpio_set_high(struct ccsr_gpio *base, unsigned int gpios)
 	setbits_be32(&base->gpdir, gpios);
 }
 
+static inline int
+mpc85xx_gpio_open_drain_val(struct ccsr_gpio *base, unsigned int mask)
+{
+	/* Read the requested values */
+	return in_be32(&base->gpodr) & mask;
+}
+
+static inline void
+mpc85xx_gpio_open_drain_on(struct ccsr_gpio *base, unsigned int gpios)
+{
+	setbits_be32(&base->gpodr, gpios);
+}
+
+static inline void
+mpc85xx_gpio_open_drain_off(struct ccsr_gpio *base, unsigned int gpios)
+{
+	clrbits_be32(&base->gpodr, gpios);
+}
+
 static int mpc85xx_gpio_direction_input(struct udevice *dev, unsigned int gpio)
 {
 	struct mpc85xx_gpio_data *data = dev_get_priv(dev);
@@ -75,6 +94,26 @@ static int mpc85xx_gpio_direction_input(struct udevice *dev, unsigned int gpio)
 }
 
 static int
+mpc85xx_gpio_get_open_drain(struct udevice *dev, unsigned gpio)
+{
+	struct mpc85xx_gpio_data *data = dev_get_priv(dev);
+
+	return !!mpc85xx_gpio_open_drain_val(data->base, 1U << (31 - gpio));
+}
+
+static int
+mpc85xx_gpio_set_open_drain(struct udevice *dev, unsigned gpio, int value)
+{
+	struct mpc85xx_gpio_data *data = dev_get_priv(dev);
+
+	if (value)
+		mpc85xx_gpio_open_drain_on(data->base, 1U << (31 - gpio));
+	else
+		mpc85xx_gpio_open_drain_off(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);
@@ -163,6 +202,8 @@ static const struct dm_gpio_ops gpio_mpc85xx_ops = {
 	.direction_output	= mpc85xx_gpio_direction_output,
 	.get_value		= mpc85xx_gpio_get_value,
 	.set_value		= mpc85xx_gpio_set_value,
+	.get_open_drain		= mpc85xx_gpio_get_open_drain,
+	.set_open_drain		= mpc85xx_gpio_set_open_drain,
 	.get_function		= mpc85xx_gpio_get_function,
 };
 
-- 
2.7.0.GIT

[U-Boot] [PATCH 1/3] dm: gpio: Add driver for MPC85XX GPIO controller

[Simon Glass]

Hi Mario,

On 26 April 2016 at 08:08, Mario Six <mario.six@gdsys.cc> wrote:
>
> Signed-off-by: Mario Six <mario.six@gdsys.cc>
> ---
>  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 <asm/mpc85xx_gpio.h>
> +#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      0x00005000
>  #define PIWAR_MEM_2G           0x0000001e
>
> +#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 0000000..2289eb7
> --- /dev/null
> +++ b/drivers/gpio/mpc85xx_gpio.c
> @@ -0,0 +1,182 @@
> +/*
> + * (C) Copyright 2016
> + * Mario Six, Guntermann & Drunck GmbH, six at 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 <common.h>
> +#include <dm.h>
> +#include <asm/gpio.h>
> +#include <mapmem.h>
> +
> +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@the top and using that throughout:

#define GPIO_MASK(gpio)  (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));
> +       return 0;
> +}
> +
> +static int
> +mpc85xx_gpio_get_value(struct udevice *dev, unsigned gpio)
> +{
> +       struct mpc85xx_gpio_data *data = dev_get_priv(dev);
> +
> +       return !!mpc85xx_gpio_get_val(data->base, 1U << (31 - gpio));
> +}
> +
> +static int
> +mpc85xx_gpio_get_function(struct udevice *dev, unsigned gpio)
> +{
> +       struct mpc85xx_gpio_data *data = dev_get_priv(dev);
> +       int dir;
> +
> +       dir = !!mpc85xx_gpio_get_dir(data->base, 1U << (31 - gpio));
> +       return dir ? GPIOF_OUTPUT : GPIOF_INPUT;
> +}
> +
> +static int
> +mpc85xx_gpio_ofdata_to_platdata(struct udevice *dev) {
> +       struct mpc85xx_gpio_data *data = dev_get_priv(dev);
> +       u64 reg;
> +       u32 addr, size;
> +
> +       reg = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");

This seem really odd. It seems like it returns a 64-bit value but you
turn it into two 32-bit values. My suggestions:

- create dev_map_sysmem() ius
- figure out why apparently phys_addr_t is 64 bits but your device
tree has 32-bit addresses

> +       addr = reg >> 32;
> +       size = reg & 0xFFFFFFFF;
> +
> +       data->addr = addr;
> +       data->base = map_sysmem(CONFIG_SYS_IMMR + addr, size);
> +
> +       if (!data->base)
> +               return -ENOMEM;
> +
> +       data->gpio_count = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
> +                                         "ngpios", 32);
> +
> +       return 0;
> +}
> +
> +static int
> +mpc85xx_gpio_probe(struct udevice *dev)
> +{
> +       struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> +       struct mpc85xx_gpio_data *data = dev_get_priv(dev);
> +       char name[32], *str;
> +
> +       snprintf(name, sizeof(name), "MPC@%x_", data->addr);
> +       str = strdup(name);
> +
> +       if (!str)
> +               return -ENOMEM;
> +
> +       uc_priv->bank_name = str;
> +

Drop blank line.

> +       uc_priv->gpio_count = data->gpio_count;
> +
> +       return 0;
> +}
> +
> +static const struct dm_gpio_ops gpio_mpc85xx_ops = {
> +       .direction_input        = mpc85xx_gpio_direction_input,
> +       .direction_output       = mpc85xx_gpio_direction_output,
> +       .get_value              = mpc85xx_gpio_get_value,
> +       .set_value              = mpc85xx_gpio_set_value,
> +       .get_function           = mpc85xx_gpio_get_function,
> +};
> +
> +static const struct udevice_id mpc85xx_gpio_ids[] = {
> +       { .compatible = "fsl,pq3-gpio" },
> +       { /* sentinel */ }
> +};
> +
> +U_BOOT_DRIVER(gpio_mpc85xx) = {
> +       .name   = "gpio_mpc85xx",
> +       .id     = UCLASS_GPIO,
> +       .ops    = &gpio_mpc85xx_ops,
> +       .ofdata_to_platdata = mpc85xx_gpio_ofdata_to_platdata,
> +       .of_match = mpc85xx_gpio_ids,
> +       .probe  = mpc85xx_gpio_probe,
> +       .priv_auto_alloc_size = sizeof(struct mpc85xx_gpio_data),
> +};
> --
> 2.7.0.GIT
>

Regards,
Simon

[U-Boot] [PATCH 2/3] dm: gpio: Add methods for open drain setting

[Simon Glass]

Hi Mario,

On 26 April 2016 at 08:08, Mario Six <mario.six@gdsys.cc> wrote:
> Signed-off-by: Mario Six <mario.six@gdsys.cc>

Please always add a commit message.

> ---
>  drivers/gpio/gpio-uclass.c | 30 ++++++++++++++++++++++++++++++
>  include/asm-generic/gpio.h | 29 +++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+)
>
> diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
> index b58d4e6..16b9648 100644
> --- a/drivers/gpio/gpio-uclass.c
> +++ b/drivers/gpio/gpio-uclass.c
> @@ -352,6 +352,36 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int value)
>         return 0;
>  }
>
> +int dm_gpio_get_open_drain(struct gpio_desc *desc)
> +{
> +       struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
> +       int ret;
> +
> +       ret = check_reserved(desc, "get_open_drain");
> +       if (ret)
> +               return ret;
> +
> +       if (ops->set_open_drain)
> +               return ops->get_open_drain(desc->dev, desc->offset);
> +       else
> +               return 0;

-ENOSYS seems better here

> +}
> +
> +int dm_gpio_set_open_drain(struct gpio_desc *desc, int value)
> +{
> +       struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
> +       int ret;
> +
> +       ret = check_reserved(desc, "set_open_drain");
> +       if (ret)
> +               return ret;
> +
> +       if (ops->set_open_drain)
> +               ops->set_open_drain(desc->dev, desc->offset, value);

Check return value

> +
> +       return 0;

-ENOSYS here I think, or are you trying to suppress it?

> +}
> +
>  int dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags)
>  {
>         struct udevice *dev = desc->dev;
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index 68b5f0b..ba65080 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -241,6 +241,8 @@ struct dm_gpio_ops {
>                                 int value);
>         int (*get_value)(struct udevice *dev, unsigned offset);
>         int (*set_value)(struct udevice *dev, unsigned offset, int value);
> +       int (*get_open_drain)(struct udevice *dev, unsigned offset);
> +       int (*set_open_drain)(struct udevice *dev, unsigned offset, int value);
>         /**
>          * get_function() Get the GPIO function
>          *
> @@ -541,6 +543,33 @@ int dm_gpio_get_value(const struct gpio_desc *desc);
>  int dm_gpio_set_value(const struct gpio_desc *desc, int value);
>
>  /**
> + * dm_gpio_get_open_drain() - Check if open-drain-mode of a GPIO is active
> + *
> + * This checks if open-drain-mode for a GPIO is enabled or not. This method is
> + * optional; if the driver does not support it, nothing happens when the method
> + * is called.

Can you include a brief description of what open drain is?

> + *
> + * @desc:      GPIO description containing device, offset and flags,
> + *             previously returned by gpio_request_by_name()
> + * @return Value of open drain mode for GPIO (0 for inactive, 1 for active) or
> + *        -ve on error
> + */
> +int dm_gpio_get_open_drain(struct gpio_desc *desc);
> +
> +/**
> + * dm_gpio_set_open_drain() - Switch open-drain-mode of a GPIO on or off
> + *
> + * This enables or disables open-drain-mode for a GPIO. This method is
> + * optional; if the driver does not support it, nothing happens when the method
> + * is called.
> + *
> + * @desc:      GPIO description containing device, offset and flags,
> + *             previously returned by gpio_request_by_name()
> + * @return 0 if OK, -ve on error
> + */
> +int dm_gpio_set_open_drain(struct gpio_desc *desc, int value);
> +
> +/**
>   * dm_gpio_set_dir() - Set the direction for a GPIO
>   *
>   * This sets up the direction according tot the provided flags. It will do
> --
> 2.7.0.GIT
>

Regards,
Simon

[U-Boot] [PATCH 3/3] dm: gpio: Implement open drain for MPC85XX GPIO

[Simon Glass]

Hi Mario,

On 26 April 2016 at 08:08, Mario Six <mario.six@gdsys.cc> wrote:
> Signed-off-by: Mario Six <mario.six@gdsys.cc>

Commit message please.

> ---
>  drivers/gpio/mpc85xx_gpio.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

>
> diff --git a/drivers/gpio/mpc85xx_gpio.c b/drivers/gpio/mpc85xx_gpio.c
> index 2289eb7..6e920e6 100644
> --- a/drivers/gpio/mpc85xx_gpio.c
> +++ b/drivers/gpio/mpc85xx_gpio.c
> @@ -66,6 +66,25 @@ mpc85xx_gpio_set_high(struct ccsr_gpio *base, unsigned int gpios)
>         setbits_be32(&base->gpdir, gpios);
>  }
>
> +static inline int

same line

> +mpc85xx_gpio_open_drain_val(struct ccsr_gpio *base, unsigned int mask)
> +{
> +       /* Read the requested values */
> +       return in_be32(&base->gpodr) & mask;
> +}
> +
> +static inline void
> +mpc85xx_gpio_open_drain_on(struct ccsr_gpio *base, unsigned int gpios)
> +{
> +       setbits_be32(&base->gpodr, gpios);
> +}
> +
> +static inline void
> +mpc85xx_gpio_open_drain_off(struct ccsr_gpio *base, unsigned int gpios)
> +{
> +       clrbits_be32(&base->gpodr, gpios);
> +}
> +
>  static int mpc85xx_gpio_direction_input(struct udevice *dev, unsigned int gpio)
>  {
>         struct mpc85xx_gpio_data *data = dev_get_priv(dev);
> @@ -75,6 +94,26 @@ static int mpc85xx_gpio_direction_input(struct udevice *dev, unsigned int gpio)
>  }
>
>  static int
> +mpc85xx_gpio_get_open_drain(struct udevice *dev, unsigned gpio)
> +{
> +       struct mpc85xx_gpio_data *data = dev_get_priv(dev);
> +
> +       return !!mpc85xx_gpio_open_drain_val(data->base, 1U << (31 - gpio));
> +}
> +
> +static int
> +mpc85xx_gpio_set_open_drain(struct udevice *dev, unsigned gpio, int value)
> +{
> +       struct mpc85xx_gpio_data *data = dev_get_priv(dev);
> +
> +       if (value)
> +               mpc85xx_gpio_open_drain_on(data->base, 1U << (31 - gpio));
> +       else
> +               mpc85xx_gpio_open_drain_off(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);
> @@ -163,6 +202,8 @@ static const struct dm_gpio_ops gpio_mpc85xx_ops = {
>         .direction_output       = mpc85xx_gpio_direction_output,
>         .get_value              = mpc85xx_gpio_get_value,
>         .set_value              = mpc85xx_gpio_set_value,
> +       .get_open_drain         = mpc85xx_gpio_get_open_drain,
> +       .set_open_drain         = mpc85xx_gpio_set_open_drain,
>         .get_function           = mpc85xx_gpio_get_function,
>  };
>
> --
> 2.7.0.GIT
>

Regards,
Simon

[U-Boot] [PATCH 1/3] dm: gpio: Add driver for MPC85XX GPIO controller

[Mario Six]

Hi Simon,

On Sun, May 1, 2016 at 7:46 PM, Simon Glass <sjg@chromium.org> wrote:

> Hi Mario,
>
> On 26 April 2016 at 08:08, Mario Six <mario.six@gdsys.cc> wrote:
> >
> > Signed-off-by: Mario Six <mario.six@gdsys.cc>
> > ---
> >  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 <asm/mpc85xx_gpio.h>
> > +#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      0x00005000
> >  #define PIWAR_MEM_2G           0x0000001e
> >
> > +#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 0000000..2289eb7
> > --- /dev/null
> > +++ b/drivers/gpio/mpc85xx_gpio.c
> > @@ -0,0 +1,182 @@
> > +/*
> > + * (C) Copyright 2016
> > + * Mario Six, Guntermann & Drunck GmbH, six at 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 <common.h>
> > +#include <dm.h>
> > +#include <asm/gpio.h>
> > +#include <mapmem.h>
> > +
> > +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(gpio)  (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));
> > +       return 0;
> > +}
> > +
> > +static int
> > +mpc85xx_gpio_get_value(struct udevice *dev, unsigned gpio)
> > +{
> > +       struct mpc85xx_gpio_data *data = dev_get_priv(dev);
> > +
> > +       return !!mpc85xx_gpio_get_val(data->base, 1U << (31 - gpio));
> > +}
> > +
> > +static int
> > +mpc85xx_gpio_get_function(struct udevice *dev, unsigned gpio)
> > +{
> > +       struct mpc85xx_gpio_data *data = dev_get_priv(dev);
> > +       int dir;
> > +
> > +       dir = !!mpc85xx_gpio_get_dir(data->base, 1U << (31 - gpio));
> > +       return dir ? GPIOF_OUTPUT : GPIOF_INPUT;
> > +}
> > +
> > +static int
> > +mpc85xx_gpio_ofdata_to_platdata(struct udevice *dev) {
> > +       struct mpc85xx_gpio_data *data = dev_get_priv(dev);
> > +       u64 reg;
> > +       u32 addr, size;
> > +
> > +       reg = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
>
> This seem really odd. It seems like it returns a 64-bit value but you
> turn it into two 32-bit values. My suggestions:
>
> - create dev_map_sysmem() ius
> - figure out why apparently phys_addr_t is 64 bits but your device
> tree has 32-bit addresses
>
> > +       addr = reg >> 32;
> > +       size = reg & 0xFFFFFFFF;
> > +
> > +       data->addr = addr;
> > +       data->base = map_sysmem(CONFIG_SYS_IMMR + addr, size);
> > +
> > +       if (!data->base)
> > +               return -ENOMEM;
> > +
> > +       data->gpio_count = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
> > +                                         "ngpios", 32);
> > +
> > +       return 0;
> > +}
> > +
> > +static int
> > +mpc85xx_gpio_probe(struct udevice *dev)
> > +{
> > +       struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> > +       struct mpc85xx_gpio_data *data = dev_get_priv(dev);
> > +       char name[32], *str;
> > +
> > +       snprintf(name, sizeof(name), "MPC@%x_", data->addr);
> > +       str = strdup(name);
> > +
> > +       if (!str)
> > +               return -ENOMEM;
> > +
> > +       uc_priv->bank_name = str;
> > +
>
> Drop blank line.
>
> > +       uc_priv->gpio_count = data->gpio_count;
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct dm_gpio_ops gpio_mpc85xx_ops = {
> > +       .direction_input        = mpc85xx_gpio_direction_input,
> > +       .direction_output       = mpc85xx_gpio_direction_output,
> > +       .get_value              = mpc85xx_gpio_get_value,
> > +       .set_value              = mpc85xx_gpio_set_value,
> > +       .get_function           = mpc85xx_gpio_get_function,
> > +};
> > +
> > +static const struct udevice_id mpc85xx_gpio_ids[] = {
> > +       { .compatible = "fsl,pq3-gpio" },
> > +       { /* sentinel */ }
> > +};
> > +
> > +U_BOOT_DRIVER(gpio_mpc85xx) = {
> > +       .name   = "gpio_mpc85xx",
> > +       .id     = UCLASS_GPIO,
> > +       .ops    = &gpio_mpc85xx_ops,
> > +       .ofdata_to_platdata = mpc85xx_gpio_ofdata_to_platdata,
> > +       .of_match = mpc85xx_gpio_ids,
> > +       .probe  = mpc85xx_gpio_probe,
> > +       .priv_auto_alloc_size = sizeof(struct mpc85xx_gpio_data),
> > +};
> > --
> > 2.7.0.GIT
> >
>
> Regards,
> Simon
>

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.

With "dev_map_sysmem" you mean a function that takes the value of the "reg"
property and returns the mapped memory region, I presume?

Everything else will be addressed in v2.

Thanks for reviewing!

Best regards,
Mario

[U-Boot] [PATCH 1/3] dm: gpio: Add driver for MPC85XX GPIO controller

[Timur Tabi]

On Mon, May 2, 2016 at 6:43 AM, Mario Six <mario.six@gdsys.cc> 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] [PATCH 1/3] dm: gpio: Add driver for MPC85XX GPIO controller

[Mario Six]

On Sun, May 8, 2016 at 3:28 PM, Timur Tabi <timur@tabi.org> wrote:

> On Mon, May 2, 2016 at 6:43 AM, Mario Six <mario.six@gdsys.cc> 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 & 0xFFFFFFFF;' 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] [PATCH v2 0/3] dm: gpio: Add driver for MPC85xx GPIO controller

[Mario Six]

The functions for accessing GPIOs on MPC85xx are hardcoded in
arch/powerpc/include/asm/mpc85xx_gpio.h This leads to problems if another GPIO
controller supporting the driver model is to be used simultaneously.

Therefore, this patch moves the "static" functions into a DM-compatible driver,
and also introduces a set of functions into the GPIO uclass that expose the
controller's capability to switch individual GPIOs into open-drain-mode.

Mario Six (3):
  dm: gpio: Add driver for MPC85XX GPIO controller
  dm: gpio: Add methods for open drain setting
  dm: gpio: Implement open drain for MPC85XX GPIO

 arch/powerpc/include/asm/arch-mpc85xx/gpio.h |   2 +
 arch/powerpc/include/asm/immap_85xx.h        |   2 +
 drivers/gpio/Kconfig                         |  25 ++++
 drivers/gpio/Makefile                        |   1 +
 drivers/gpio/gpio-uclass.c                   |  32 +++++
 drivers/gpio/mpc85xx_gpio.c                  | 202 +++++++++++++++++++++++++++
 include/asm-generic/gpio.h                   |  34 +++++
 7 files changed, 298 insertions(+)
 create mode 100644 drivers/gpio/mpc85xx_gpio.c

--
2.7.0.GIT

[U-Boot] [PATCH v2 1/3] dm: gpio: Add driver for MPC85XX GPIO controller

[Mario Six]

This patch adds a driver for the built-in GPIO controller of the MPC85XX
SoC (probably supporting other PowerQUICC III SoCs as well).

Each GPIO bank is identified by its own entry in the device tree, i.e.

gpio-controller at fc00 {
      #gpio-cells = <2>;
      compatible = "fsl,pq3-gpio";
      reg = <0xfc00 0x100>
}

By default, each bank is assumed to have 32 GPIOs, but the ngpios
setting is honored, so the number of GPIOs for each bank in configurable
to match the actual GPIO count of the SoC (e.g. the 32/32/23 banks of
the P1022 SoC).

The usual functions of GPIO drivers (setting input/output mode and output
value setting) are supported.

The driver has been tested on MPC85XX, but it is likely that other
PowerQUICC III devices will work as well.

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---

v2:
- Added missing commit message
- Improved the Kconfig description
- Fixed and documented the mpc85xx_gpio_data members
- Introduced GPIO_MASK macro to simplify the code
- Fixed white space issues in function headers
- Removed unnecessary empty line
- Use fdtdec_get_addr_size_auto_noparent to read the register base data

---
 arch/powerpc/include/asm/arch-mpc85xx/gpio.h |   2 +
 arch/powerpc/include/asm/immap_85xx.h        |   2 +
 drivers/gpio/Kconfig                         |  25 ++++
 drivers/gpio/Makefile                        |   1 +
 drivers/gpio/mpc85xx_gpio.c                  | 183 +++++++++++++++++++++++++++
 5 files changed, 213 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 <asm/mpc85xx_gpio.h>
+#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	0x00005000
 #define PIWAR_MEM_2G		0x0000001e

+#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..068ee63 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -143,4 +143,29 @@ config ZYNQ_GPIO
 	help
 	  Supports GPIO access on Zynq SoC.

+config MPC85XX_GPIO
+	bool "Freescale MPC85XX GPIO driver"
+	depends on DM_GPIO
+	help
+	  This driver supports the built-in GPIO controller of MPC85XX CPUs.
+	  Each GPIO bank is identified by its own entry in the device tree,
+	  i.e.
+
+	  gpio-controller at fc00 {
+		#gpio-cells = <2>;
+		compatible = "fsl,pq3-gpio";
+		reg = <0xfc00 0x100>
+	  }
+
+	  By default, each bank is assumed to have 32 GPIOs, but the ngpios
+	  setting is honored, so the number of GPIOs for each bank is
+	  configurable to match the actual GPIO count of the SoC (e.g. the
+	  32/32/23 banks of the P1022 SoC).
+
+	  The standard functions of input/output mode, and output value setting
+	  are supported; the open-drain capability of the controller is not
+	  supported yet.
+
+	  The driver has been tested on MPC85XX, but it is likely that other
+	  PowerQUICC III devices will work as well.
 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 0000000..acf0414
--- /dev/null
+++ b/drivers/gpio/mpc85xx_gpio.c
@@ -0,0 +1,183 @@
+/*
+ * (C) Copyright 2016
+ * Mario Six, Guntermann & Drunck GmbH, six at 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 <common.h>
+#include <dm.h>
+#include <asm/gpio.h>
+#include <mapmem.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+struct ccsr_gpio {
+	u32	gpdir;
+	u32	gpodr;
+	u32	gpdat;
+	u32	gpier;
+	u32	gpimr;
+	u32	gpicr;
+};
+
+struct mpc85xx_gpio_data {
+	/* The bank's register base in memory */
+	struct ccsr_gpio __iomem *base;
+	/* The address of the registers; used to identify the bank */
+	ulong addr;
+	/* The GPIO count of the bank */
+	uint gpio_count;
+};
+
+inline unsigned int gpio_mask(unsigned int gpio) {
+	return (1U << (31 - (gpio)));
+}
+
+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, gpio_mask(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, gpio_mask(gpio));
+	else
+		mpc85xx_gpio_set_low(data->base, gpio_mask(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, gpio_mask(gpio));
+	else
+		mpc85xx_gpio_set_low(data->base, gpio_mask(gpio));
+	return 0;
+}
+
+static int mpc85xx_gpio_get_value(struct udevice *dev, unsigned gpio)
+{
+	struct mpc85xx_gpio_data *data = dev_get_priv(dev);
+
+	return !!mpc85xx_gpio_get_val(data->base, gpio_mask(gpio));
+}
+
+static int mpc85xx_gpio_get_function(struct udevice *dev, unsigned gpio)
+{
+	struct mpc85xx_gpio_data *data = dev_get_priv(dev);
+	int dir;
+
+	dir = !!mpc85xx_gpio_get_dir(data->base, gpio_mask(gpio));
+	return dir ? GPIOF_OUTPUT : GPIOF_INPUT;
+}
+
+static int mpc85xx_gpio_ofdata_to_platdata(struct udevice *dev) {
+	struct mpc85xx_gpio_data *data = dev_get_priv(dev);
+	fdt_addr_t addr;
+	fdt_size_t size;
+
+	addr = fdtdec_get_addr_size_auto_noparent(gd->fdt_blob, dev->of_offset,
+						  "reg", 0, &size);
+
+	data->addr = addr;
+	data->base = map_sysmem(CONFIG_SYS_IMMR + addr, size);
+
+	if (!data->base)
+		return -ENOMEM;
+
+	data->gpio_count = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
+					  "ngpios", 32);
+
+	return 0;
+}
+
+static int mpc85xx_gpio_probe(struct udevice *dev)
+{
+	struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
+	struct mpc85xx_gpio_data *data = dev_get_priv(dev);
+	char name[32], *str;
+
+	snprintf(name, sizeof(name), "MPC@%lx_", data->addr);
+	str = strdup(name);
+
+	if (!str)
+		return -ENOMEM;
+
+	uc_priv->bank_name = str;
+	uc_priv->gpio_count = data->gpio_count;
+
+	return 0;
+}
+
+static const struct dm_gpio_ops gpio_mpc85xx_ops = {
+	.direction_input	= mpc85xx_gpio_direction_input,
+	.direction_output	= mpc85xx_gpio_direction_output,
+	.get_value		= mpc85xx_gpio_get_value,
+	.set_value		= mpc85xx_gpio_set_value,
+	.get_function 		= mpc85xx_gpio_get_function,
+};
+
+static const struct udevice_id mpc85xx_gpio_ids[] = {
+	{ .compatible = "fsl,pq3-gpio" },
+	{ /* sentinel */ }
+};
+
+U_BOOT_DRIVER(gpio_mpc85xx) = {
+	.name	= "gpio_mpc85xx",
+	.id	= UCLASS_GPIO,
+	.ops	= &gpio_mpc85xx_ops,
+	.ofdata_to_platdata = mpc85xx_gpio_ofdata_to_platdata,
+	.of_match = mpc85xx_gpio_ids,
+	.probe	= mpc85xx_gpio_probe,
+	.priv_auto_alloc_size = sizeof(struct mpc85xx_gpio_data),
+};
--
2.7.0.GIT

[U-Boot] [PATCH v2 2/3] dm: gpio: Add methods for open drain setting

[Mario Six]

Certain GPIO devices have the capability to switch their GPIOs into
open-drain mode, that is, instead of actively driving the output
(Push-pull output), the pin is connected to the collector (for a NPN
transistor) or the drain (for a MOSFET) of a transistor, respectively.
The pin then either forms an open circuit or a connection to ground,
depending on the state of the transistor.

This patch adds functions to the GPIO uclass to switch GPIOs to
open-drain mode on devices that support it.

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---

v2:
- Added missing commit message
- Fixed error return value of dm_gpio_get_open_drain
- Fixed return value passing in dm_gpio_set_open_drain and added comment
- Added description of open-drain mode

---
 drivers/gpio/gpio-uclass.c | 32 ++++++++++++++++++++++++++++++++
 include/asm-generic/gpio.h | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
index b58d4e6..1be9988 100644
--- a/drivers/gpio/gpio-uclass.c
+++ b/drivers/gpio/gpio-uclass.c
@@ -352,6 +352,38 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int value)
 	return 0;
 }

+int dm_gpio_get_open_drain(struct gpio_desc *desc)
+{
+	struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
+	int ret;
+
+	ret = check_reserved(desc, "get_open_drain");
+	if (ret)
+		return ret;
+
+	if (ops->set_open_drain)
+		return ops->get_open_drain(desc->dev, desc->offset);
+	else
+		return -ENOSYS;
+}
+
+int dm_gpio_set_open_drain(struct gpio_desc *desc, int value)
+{
+	struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
+	int ret;
+
+	ret = check_reserved(desc, "set_open_drain");
+	if (ret)
+		return ret;
+
+	if (ops->set_open_drain)
+		ret = ops->set_open_drain(desc->dev, desc->offset, value);
+	else
+		return 0; /* feature not supported -> ignore setting */
+
+	return ret;
+}
+
 int dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags)
 {
 	struct udevice *dev = desc->dev;
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index 68b5f0b..ece7552 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -241,6 +241,8 @@ struct dm_gpio_ops {
 				int value);
 	int (*get_value)(struct udevice *dev, unsigned offset);
 	int (*set_value)(struct udevice *dev, unsigned offset, int value);
+	int (*get_open_drain)(struct udevice *dev, unsigned offset);
+	int (*set_open_drain)(struct udevice *dev, unsigned offset, int value);
 	/**
 	 * get_function() Get the GPIO function
 	 *
@@ -541,6 +543,38 @@ int dm_gpio_get_value(const struct gpio_desc *desc);
 int dm_gpio_set_value(const struct gpio_desc *desc, int value);

 /**
+ * dm_gpio_get_open_drain() - Check if open-drain-mode of a GPIO is active
+ *
+ * This checks if open-drain-mode for a GPIO is enabled or not. This method is
+ * optional.
+ *
+ * @desc:	GPIO description containing device, offset and flags,
+ *		previously returned by gpio_request_by_name()
+ * @return Value of open drain mode for GPIO (0 for inactive, 1 for active) or
+ *	   -ve on error
+ */
+int dm_gpio_get_open_drain(struct gpio_desc *desc);
+
+/**
+ * dm_gpio_set_open_drain() - Switch open-drain-mode of a GPIO on or off
+ *
+ * This enables or disables open-drain mode for a GPIO. This method is
+ * optional; if the driver does not support it, nothing happens when the method
+ * is called.
+ *
+ * In open-drain mode, instead of actively driving the output (Push-pull
+ * output), the GPIO's pin is connected to the collector (for a NPN transistor)
+ * or the drain (for a MOSFET) of a transistor, respectively. The pin then
+ * either forms an open circuit or a connection to ground, depending on the
+ * state of the transistor.
+ *
+ * @desc:	GPIO description containing device, offset and flags,
+ *		previously returned by gpio_request_by_name()
+ * @return 0 if OK, -ve on error
+ */
+int dm_gpio_set_open_drain(struct gpio_desc *desc, int value);
+
+/**
  * dm_gpio_set_dir() - Set the direction for a GPIO
  *
  * This sets up the direction according tot the provided flags. It will do
--
2.7.0.GIT

[U-Boot] [PATCH v2 3/3] dm: gpio: Implement open drain for MPC85XX GPIO

[Mario Six]

This patch implements the open-drain setting feature for the MPC85XX
GPIO controller.

Signed-off-by: Mario Six <mario.six@gdsys.cc>
---

v3:
- Added missing commit message
- Fixed white space issues in function headers

---
 drivers/gpio/Kconfig        |  6 +++---
 drivers/gpio/mpc85xx_gpio.c | 19 +++++++++++++++++++
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 068ee63..b250622 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -162,9 +162,9 @@ config MPC85XX_GPIO
 	  configurable to match the actual GPIO count of the SoC (e.g. the
 	  32/32/23 banks of the P1022 SoC).

-	  The standard functions of input/output mode, and output value setting
-	  are supported; the open-drain capability of the controller is not
-	  supported yet.
+	  Aside from the standard functions of input/output mode, and output
+	  value setting, the open-drain feature, which can configure individual
+	  GPIOs to work as open-drain outputs, is supported.

 	  The driver has been tested on MPC85XX, but it is likely that other
 	  PowerQUICC III devices will work as well.
diff --git a/drivers/gpio/mpc85xx_gpio.c b/drivers/gpio/mpc85xx_gpio.c
index acf0414..dc6193c 100644
--- a/drivers/gpio/mpc85xx_gpio.c
+++ b/drivers/gpio/mpc85xx_gpio.c
@@ -73,6 +73,25 @@ static inline void mpc85xx_gpio_set_high(struct ccsr_gpio *base,
 	setbits_be32(&base->gpdir, gpios);
 }

+static inline int mpc85xx_gpio_open_drain_val(struct ccsr_gpio *base,
+					      unsigned int mask)
+{
+	/* Read the requested values */
+	return in_be32(&base->gpodr) & mask;
+}
+
+static inline void mpc85xx_gpio_open_drain_on(struct ccsr_gpio *base,
+					      unsigned int gpios)
+{
+	setbits_be32(&base->gpodr, gpios);
+}
+
+static inline void mpc85xx_gpio_open_drain_off(struct ccsr_gpio *base,
+					       unsigned int gpios)
+{
+	clrbits_be32(&base->gpodr, gpios);
+}
+
 static int mpc85xx_gpio_direction_input(struct udevice *dev, unsigned int gpio)
 {
 	struct mpc85xx_gpio_data *data = dev_get_priv(dev);
--
2.7.0.GIT

[U-Boot] [PATCH 1/3] dm: gpio: Add driver for MPC85XX GPIO controller

[Timur Tabi]

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 & 0xFFFFFFFF;' 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] [PATCH v2 1/3] dm: gpio: Add driver for MPC85XX GPIO controller

[Timur Tabi]

Mario Six wrote:
> +	addr = fdtdec_get_addr_size_auto_noparent(gd->fdt_blob, dev->of_offset,
> +						  "reg", 0, &size);

Why can't you use platform_get_resource()?

[U-Boot] [PATCH v2 1/3] dm: gpio: Add driver for MPC85XX GPIO controller

[Mario Six]

On Tue, May 10, 2016 at 2:30 PM, Timur Tabi <timur@tabi.org> wrote:

> Mario Six wrote:
>
>> +       addr = fdtdec_get_addr_size_auto_noparent(gd->fdt_blob,
>> dev->of_offset,
>> +                                                 "reg", 0, &size);
>>
>
> Why can't you use platform_get_resource()?
>
>
platform_get_resource is from the kernel, not U-Boot ;-) (Unless I've missed
something and it has been implemented without me noticing).

I suppose I could use dev_get_addr and just use sizeof(...) for the iomem
allocation, but then I would ignore the size from the device tree, which
doesn't sit well with me. But that'd be fine, too (it does look a bit
neater, I
admit). If Simon wants me to do it that way, I will.

Best regards,
Mario

[U-Boot] [PATCH v2 1/3] dm: gpio: Add driver for MPC85XX GPIO controller

[Timur Tabi]

Mario Six wrote:
> On Tue, May 10, 2016 at 2:30 PM, Timur Tabi <timur@tabi.org
> <mailto:timur@tabi.org>> wrote:
>
>     Mario Six wrote:
>
>         +       addr = fdtdec_get_addr_size_auto_noparent(gd->fdt_blob,
>         dev->of_offset,
>         +                                                 "reg", 0, &size);
>
>
>     Why can't you use platform_get_resource()?
>
>
> platform_get_resource is from the kernel, not U-Boot ;-) (Unless I've missed
> something and it has been implemented without me noticing).

Ugh, sorry.  I thought this was kernel code.  I make that mistake a lot.

Now everything makes more sense.  I think 
fdtdec_get_addr_size_auto_noparent() is correct.

[U-Boot] [PATCH v2 1/3] dm: gpio: Add driver for MPC85XX GPIO controller

[Mario Six]

On Tue, May 10, 2016 at 3:26 PM, Timur Tabi <timur@tabi.org> wrote:

> Mario Six wrote:
>
>> On Tue, May 10, 2016 at 2:30 PM, Timur Tabi <timur@tabi.org
>> <mailto:timur@tabi.org>> wrote:
>>
>>     Mario Six wrote:
>>
>>         +       addr = fdtdec_get_addr_size_auto_noparent(gd->fdt_blob,
>>         dev->of_offset,
>>         +                                                 "reg", 0,
>> &size);
>>
>>
>>     Why can't you use platform_get_resource()?
>>
>>
>> platform_get_resource is from the kernel, not U-Boot ;-) (Unless I've
>> missed
>> something and it has been implemented without me noticing).
>>
>
> Ugh, sorry.  I thought this was kernel code.  I make that mistake a lot.
>
> Now everything makes more sense.  I think
> fdtdec_get_addr_size_auto_noparent() is correct.
>
> Yeah, I make that mistake all the time, too :-)

We'll see what Simon has to say about v2.

Thanks for all the help!

Best regards,
Mario

[U-Boot] [PATCH v2 2/3] dm: gpio: Add methods for open drain setting

[Simon Glass]

Hi Mario,

On 10 May 2016 at 01:51, Mario Six <mario.six@gdsys.cc> wrote:
> Certain GPIO devices have the capability to switch their GPIOs into
> open-drain mode, that is, instead of actively driving the output
> (Push-pull output), the pin is connected to the collector (for a NPN
> transistor) or the drain (for a MOSFET) of a transistor, respectively.
> The pin then either forms an open circuit or a connection to ground,
> depending on the state of the transistor.
>
> This patch adds functions to the GPIO uclass to switch GPIOs to
> open-drain mode on devices that support it.
>
> Signed-off-by: Mario Six <mario.six@gdsys.cc>
> ---
>
> v2:
> - Added missing commit message
> - Fixed error return value of dm_gpio_get_open_drain
> - Fixed return value passing in dm_gpio_set_open_drain and added comment
> - Added description of open-drain mode
>
> ---
>  drivers/gpio/gpio-uclass.c | 32 ++++++++++++++++++++++++++++++++
>  include/asm-generic/gpio.h | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

Can you please do a follow-up patch to update the test/dm/gpio.c file
to call these two methods? You'll need to update
drivers/gpio/sandbox.c a little also.

Regards,
Simon

[U-Boot] [PATCH v2 3/3] dm: gpio: Implement open drain for MPC85XX GPIO

[Simon Glass]

Hi Mario,

On 10 May 2016 at 01:51, Mario Six <mario.six@gdsys.cc> wrote:
> This patch implements the open-drain setting feature for the MPC85XX
> GPIO controller.
>
> Signed-off-by: Mario Six <mario.six@gdsys.cc>
> ---
>
> v3:
> - Added missing commit message
> - Fixed white space issues in function headers
>
> ---
>  drivers/gpio/Kconfig        |  6 +++---
>  drivers/gpio/mpc85xx_gpio.c | 19 +++++++++++++++++++
>  2 files changed, 22 insertions(+), 3 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

But please see below.

>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 068ee63..b250622 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -162,9 +162,9 @@ config MPC85XX_GPIO
>           configurable to match the actual GPIO count of the SoC (e.g. the
>           32/32/23 banks of the P1022 SoC).
>
> -         The standard functions of input/output mode, and output value setting
> -         are supported; the open-drain capability of the controller is not
> -         supported yet.
> +         Aside from the standard functions of input/output mode, and output
> +         value setting, the open-drain feature, which can configure individual
> +         GPIOs to work as open-drain outputs, is supported.
>
>           The driver has been tested on MPC85XX, but it is likely that other
>           PowerQUICC III devices will work as well.
> diff --git a/drivers/gpio/mpc85xx_gpio.c b/drivers/gpio/mpc85xx_gpio.c
> index acf0414..dc6193c 100644
> --- a/drivers/gpio/mpc85xx_gpio.c
> +++ b/drivers/gpio/mpc85xx_gpio.c
> @@ -73,6 +73,25 @@ static inline void mpc85xx_gpio_set_high(struct ccsr_gpio *base,
>         setbits_be32(&base->gpdir, gpios);
>  }
>
> +static inline int mpc85xx_gpio_open_drain_val(struct ccsr_gpio *base,
> +                                             unsigned int mask)
> +{
> +       /* Read the requested values */
> +       return in_be32(&base->gpodr) & mask;
> +}
> +
> +static inline void mpc85xx_gpio_open_drain_on(struct ccsr_gpio *base,
> +                                             unsigned int gpios)
> +{
> +       setbits_be32(&base->gpodr, gpios);

Why gpios? This would normally be 'offset', indicating that it is the
GPIO offset within the bank.

Also the code seems odd - don't you need to convert the value into a mask?

> +}
> +
> +static inline void mpc85xx_gpio_open_drain_off(struct ccsr_gpio *base,
> +                                              unsigned int gpios)
> +{
> +       clrbits_be32(&base->gpodr, gpios);
> +}
> +
>  static int mpc85xx_gpio_direction_input(struct udevice *dev, unsigned int gpio)
>  {
>         struct mpc85xx_gpio_data *data = dev_get_priv(dev);
> --
> 2.7.0.GIT
>

Regards,
Simon

[U-Boot] [PATCH v2 3/3] dm: gpio: Implement open drain for MPC85XX GPIO

[Mario Six]

On Thu, May 19, 2016 at 5:59 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Mario,
>
> On 10 May 2016 at 01:51, Mario Six <mario.six@gdsys.cc> wrote:
>> This patch implements the open-drain setting feature for the MPC85XX
>> GPIO controller.
>>
>> Signed-off-by: Mario Six <mario.six@gdsys.cc>
>> ---
>>
>> v3:
>> - Added missing commit message
>> - Fixed white space issues in function headers
>>
>> ---
>>  drivers/gpio/Kconfig        |  6 +++---
>>  drivers/gpio/mpc85xx_gpio.c | 19 +++++++++++++++++++
>>  2 files changed, 22 insertions(+), 3 deletions(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> But please see below.
>
>>
>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>> index 068ee63..b250622 100644
>> --- a/drivers/gpio/Kconfig
>> +++ b/drivers/gpio/Kconfig
>> @@ -162,9 +162,9 @@ config MPC85XX_GPIO
>>           configurable to match the actual GPIO count of the SoC (e.g. the
>>           32/32/23 banks of the P1022 SoC).
>>
>> -         The standard functions of input/output mode, and output value setting
>> -         are supported; the open-drain capability of the controller is not
>> -         supported yet.
>> +         Aside from the standard functions of input/output mode, and output
>> +         value setting, the open-drain feature, which can configure individual
>> +         GPIOs to work as open-drain outputs, is supported.
>>
>>           The driver has been tested on MPC85XX, but it is likely that other
>>           PowerQUICC III devices will work as well.
>> diff --git a/drivers/gpio/mpc85xx_gpio.c b/drivers/gpio/mpc85xx_gpio.c
>> index acf0414..dc6193c 100644
>> --- a/drivers/gpio/mpc85xx_gpio.c
>> +++ b/drivers/gpio/mpc85xx_gpio.c
>> @@ -73,6 +73,25 @@ static inline void mpc85xx_gpio_set_high(struct ccsr_gpio *base,
>>         setbits_be32(&base->gpdir, gpios);
>>  }
>>
>> +static inline int mpc85xx_gpio_open_drain_val(struct ccsr_gpio *base,
>> +                                             unsigned int mask)
>> +{
>> +       /* Read the requested values */
>> +       return in_be32(&base->gpodr) & mask;
>> +}
>> +
>> +static inline void mpc85xx_gpio_open_drain_on(struct ccsr_gpio *base,
>> +                                             unsigned int gpios)
>> +{
>> +       setbits_be32(&base->gpodr, gpios);
>
> Why gpios? This would normally be 'offset', indicating that it is the
> GPIO offset within the bank.
>
> Also the code seems odd - don't you need to convert the value into a mask?
>

Ah, yes. Sorry, just noticed that I missed the actual
mpc85xx_gpio_{set,get}_open_drain functions when merging the patches. You might
notice that nothing is actually added to the dm_gpio_ops structure as well
(that's what I get for trying to follow the original code style too closely).
Will add those in v4, and the test/dm/gpio.c / sandbox additions as well.

>> +}
>> +
>> +static inline void mpc85xx_gpio_open_drain_off(struct ccsr_gpio *base,
>> +                                              unsigned int gpios)
>> +{
>> +       clrbits_be32(&base->gpodr, gpios);
>> +}
>> +
>>  static int mpc85xx_gpio_direction_input(struct udevice *dev, unsigned int gpio)
>>  {
>>         struct mpc85xx_gpio_data *data = dev_get_priv(dev);
>> --
>> 2.7.0.GIT
>>
>
> Regards,
> Simon

Thanks for reviewing!

Best regards,
Mario