[U-Boot] [PATCH V2 1/3] gpio: Add DW APB GPIO driver

[U-Boot] [PATCH V2 1/3] gpio: Add DW APB GPIO driver

[Marek Vasut]

Add driver for the DesignWare APB GPIO IP block.
This driver is DM capable and probes from DT.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Simon Glass <sjg@chromium.org>
---
 drivers/gpio/Kconfig      |   7 ++
 drivers/gpio/Makefile     |   1 +
 drivers/gpio/dwapb_gpio.c | 167 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 175 insertions(+)
 create mode 100644 drivers/gpio/dwapb_gpio.c

V2: Obtain the bank name from the "bank-name" property instead.

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 0c43777..c04388d 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -8,6 +8,13 @@ config DM_GPIO
 	  particular GPIOs that they provide. The uclass interface
 	  is defined in include/asm-generic/gpio.h.
 
+config DWAPB_GPIO
+	bool "DWAPB GPIO driver"
+	depends on DM && DM_GPIO
+	default n
+	help
+	  Support for the Designware APB GPIO driver.
+
 config LPC32XX_GPIO
 	bool "LPC32XX GPIO driver"
 	depends on DM
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 67c6374..603c96b 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -6,6 +6,7 @@
 #
 
 ifndef CONFIG_SPL_BUILD
+obj-$(CONFIG_DWAPB_GPIO)	+= dwapb_gpio.o
 obj-$(CONFIG_AXP_GPIO)		+= axp_gpio.o
 endif
 obj-$(CONFIG_DM_GPIO)		+= gpio-uclass.o
diff --git a/drivers/gpio/dwapb_gpio.c b/drivers/gpio/dwapb_gpio.c
new file mode 100644
index 0000000..72cec48
--- /dev/null
+++ b/drivers/gpio/dwapb_gpio.c
@@ -0,0 +1,167 @@
+/*
+ * (C) Copyright 2015 Marek Vasut <marex@denx.de>
+ *
+ * DesignWare APB GPIO driver
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <malloc.h>
+#include <asm/arch/gpio.h>
+#include <asm/gpio.h>
+#include <asm/io.h>
+#include <dm.h>
+#include <dm/device-internal.h>
+#include <dm/lists.h>
+#include <dm/root.h>
+#include <errno.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+#define GPIO_SWPORTA_DR		0x00
+#define GPIO_SWPORTA_DDR	0x04
+#define GPIO_INTEN		0x30
+#define GPIO_INTMASK		0x34
+#define GPIO_INTTYPE_LEVEL	0x38
+#define GPIO_INT_POLARITY	0x3c
+#define GPIO_INTSTATUS		0x40
+#define GPIO_PORTA_DEBOUNCE	0x48
+#define GPIO_PORTA_EOI		0x4c
+#define GPIO_EXT_PORTA		0x50
+
+struct gpio_dwapb_platdata {
+	const char	*name;
+	int		bank;
+	int		pins;
+	fdt_addr_t	base;
+};
+
+static int dwapb_gpio_direction_input(struct udevice *dev, unsigned pin)
+{
+	struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
+
+	clrbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);
+	return 0;
+}
+
+static int dwapb_gpio_direction_output(struct udevice *dev, unsigned pin,
+				     int val)
+{
+	struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
+
+	setbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);
+
+	if (val)
+		setbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
+	else
+		clrbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
+
+	return 0;
+}
+
+static int dwapb_gpio_get_value(struct udevice *dev, unsigned pin)
+{
+	struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
+	return !!(readl(plat->base + GPIO_EXT_PORTA) & (1 << pin));
+}
+
+
+static int dwapb_gpio_set_value(struct udevice *dev, unsigned pin, int val)
+{
+	struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
+
+	if (val)
+		setbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
+	else
+		clrbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
+
+	return 0;
+}
+
+static const struct dm_gpio_ops gpio_dwapb_ops = {
+	.direction_input	= dwapb_gpio_direction_input,
+	.direction_output	= dwapb_gpio_direction_output,
+	.get_value		= dwapb_gpio_get_value,
+	.set_value		= dwapb_gpio_set_value,
+};
+
+static int gpio_dwapb_probe(struct udevice *dev)
+{
+	struct gpio_dev_priv *priv = dev_get_uclass_priv(dev);
+	struct gpio_dwapb_platdata *plat = dev->platdata;
+
+	if (!plat)
+		return 0;
+
+	priv->gpio_count = plat->pins;
+	priv->bank_name = plat->name;
+
+	return 0;
+}
+
+static int gpio_dwapb_bind(struct udevice *dev)
+{
+	struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
+	const void *blob = gd->fdt_blob;
+	struct udevice *subdev;
+	fdt_addr_t base;
+	int ret, node, bank = 0;
+
+	/* If this is a child device, there is nothing to do here */
+	if (plat)
+		return 0;
+
+	base = fdtdec_get_addr(blob, dev->of_offset, "reg");
+	if (base == FDT_ADDR_T_NONE) {
+		debug("Can't get the GPIO register base address\n");
+		return -ENXIO;
+	}
+
+	for (node = fdt_first_subnode(blob, dev->of_offset);
+	     node > 0;
+	     node = fdt_next_subnode(blob, node)) {
+		if (!fdtdec_get_bool(blob, node, "gpio-controller"))
+			continue;
+
+		plat = NULL;
+		plat = calloc(1, sizeof(*plat));
+		if (!plat)
+			return -ENOMEM;
+
+		plat->base = base;
+		plat->bank = bank;
+		plat->pins = fdtdec_get_int(blob, node, "snps,nr-gpios", 0);
+		ret = fdt_get_string(blob, node, "bank-name", &plat->name);
+		if (ret)
+			goto err;
+
+		ret = device_bind(dev, dev->driver, plat->name,
+				  plat, -1, &subdev);
+		if (ret)
+			goto err;
+
+		subdev->of_offset = node;
+		bank++;
+	}
+
+	return 0;
+
+err:
+	free(plat);
+	return ret;
+}
+
+static const struct udevice_id gpio_dwapb_ids[] = {
+	{ .compatible = "snps,dw-apb-gpio" },
+	{ }
+};
+
+U_BOOT_DRIVER(gpio_dwapb) = {
+	.name		= "gpio-dwapb",
+	.id		= UCLASS_GPIO,
+	.of_match	= gpio_dwapb_ids,
+	.ops		= &gpio_dwapb_ops,
+	.bind		= gpio_dwapb_bind,
+	.probe		= gpio_dwapb_probe,
+};
-- 
2.1.4

[U-Boot] [PATCH 2/3] arm: socfpga: dts: Add bank-name property to each GPIO bank

[Marek Vasut]

Add "bank-name" property to each GPIO bank to give it unique name.
The approach here is exactly the same as with the "regulator-name"
property for regulators.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Simon Glass <sjg@chromium.org>
---
 arch/arm/dts/socfpga.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/dts/socfpga.dtsi b/arch/arm/dts/socfpga.dtsi
index e17e9f4..125034b 100644
--- a/arch/arm/dts/socfpga.dtsi
+++ b/arch/arm/dts/socfpga.dtsi
@@ -550,6 +550,7 @@
 
 			porta: gpio-controller at 0 {
 				compatible = "snps,dw-apb-gpio-port";
+				bank-name = "porta";
 				gpio-controller;
 				#gpio-cells = <2>;
 				snps,nr-gpios = <29>;
@@ -570,6 +571,7 @@
 
 			portb: gpio-controller at 0 {
 				compatible = "snps,dw-apb-gpio-port";
+				bank-name = "portb";
 				gpio-controller;
 				#gpio-cells = <2>;
 				snps,nr-gpios = <29>;
@@ -590,6 +592,7 @@
 
 			portc: gpio-controller at 0 {
 				compatible = "snps,dw-apb-gpio-port";
+				bank-name = "portc";
 				gpio-controller;
 				#gpio-cells = <2>;
 				snps,nr-gpios = <27>;
-- 
2.1.4

[U-Boot] [PATCH 3/3] arm: socfpga: Enable DWAPB GPIO driver

[Marek Vasut]

Enable the DWAPB GPIO driver for SoCFPGA Cyclone V and Arria V.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
---
 configs/socfpga_arria5_defconfig   | 2 ++
 configs/socfpga_cyclone5_defconfig | 2 ++
 configs/socfpga_socrates_defconfig | 2 ++
 include/configs/socfpga_arria5.h   | 2 +-
 include/configs/socfpga_cyclone5.h | 2 +-
 5 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/configs/socfpga_arria5_defconfig b/configs/socfpga_arria5_defconfig
index 4d1cd21..b90686e 100644
--- a/configs/socfpga_arria5_defconfig
+++ b/configs/socfpga_arria5_defconfig
@@ -7,6 +7,8 @@ CONFIG_SPL=y
 # CONFIG_CMD_FLASH is not set
 CONFIG_OF_CONTROL=y
 CONFIG_SPI_FLASH=y
+CONFIG_DM_GPIO=y
+CONFIG_DWAPB_GPIO=y
 CONFIG_SPL_DM=y
 CONFIG_SPL_MMC_SUPPORT=y
 CONFIG_DM_SEQ_ALIAS=y
diff --git a/configs/socfpga_cyclone5_defconfig b/configs/socfpga_cyclone5_defconfig
index ae3a1de..5d7362b 100644
--- a/configs/socfpga_cyclone5_defconfig
+++ b/configs/socfpga_cyclone5_defconfig
@@ -10,6 +10,8 @@ CONFIG_SPI_FLASH=y
 CONFIG_DM_ETH=y
 CONFIG_NETDEVICES=y
 CONFIG_ETH_DESIGNWARE=y
+CONFIG_DM_GPIO=y
+CONFIG_DWAPB_GPIO=y
 CONFIG_SPL_DM=y
 CONFIG_SPL_MMC_SUPPORT=y
 CONFIG_DM_SEQ_ALIAS=y
diff --git a/configs/socfpga_socrates_defconfig b/configs/socfpga_socrates_defconfig
index 71d4711..6ef2f97 100644
--- a/configs/socfpga_socrates_defconfig
+++ b/configs/socfpga_socrates_defconfig
@@ -10,6 +10,8 @@ CONFIG_SPI_FLASH=y
 CONFIG_DM_ETH=y
 CONFIG_NETDEVICES=y
 CONFIG_ETH_DESIGNWARE=y
+CONFIG_DM_GPIO=y
+CONFIG_DWAPB_GPIO=y
 CONFIG_SPL_DM=y
 CONFIG_SPL_MMC_SUPPORT=y
 CONFIG_DM_SEQ_ALIAS=y
diff --git a/include/configs/socfpga_arria5.h b/include/configs/socfpga_arria5.h
index e1cd9cc..3193684 100644
--- a/include/configs/socfpga_arria5.h
+++ b/include/configs/socfpga_arria5.h
@@ -23,6 +23,7 @@
 #define CONFIG_CMD_EXT4_WRITE
 #define CONFIG_CMD_FAT
 #define CONFIG_CMD_FS_GENERIC
+#define CONFIG_CMD_GPIO
 #define CONFIG_CMD_GREPENV
 #define CONFIG_CMD_MII
 #define CONFIG_CMD_MMC
@@ -30,7 +31,6 @@
 #define CONFIG_CMD_USB
 #define CONFIG_CMD_USB_MASS_STORAGE
 
-
 /* Memory configurations */
 #define PHYS_SDRAM_1_SIZE		0x40000000	/* 1GiB on SoCDK */
 
diff --git a/include/configs/socfpga_cyclone5.h b/include/configs/socfpga_cyclone5.h
index 9b31741..9e733e5 100644
--- a/include/configs/socfpga_cyclone5.h
+++ b/include/configs/socfpga_cyclone5.h
@@ -23,6 +23,7 @@
 #define CONFIG_CMD_EXT4_WRITE
 #define CONFIG_CMD_FAT
 #define CONFIG_CMD_FS_GENERIC
+#define CONFIG_CMD_GPIO
 #define CONFIG_CMD_GREPENV
 #define CONFIG_CMD_MII
 #define CONFIG_CMD_MMC
@@ -30,7 +31,6 @@
 #define CONFIG_CMD_USB
 #define CONFIG_CMD_USB_MASS_STORAGE
 
-
 /* Memory configurations */
 #define PHYS_SDRAM_1_SIZE		0x40000000	/* 1GiB on SoCDK */
 
-- 
2.1.4

[U-Boot] [PATCH V2 1/3] gpio: Add DW APB GPIO driver

[Simon Glass]

Hi Marek,

On 10 August 2015 at 09:30, Marek Vasut <marex@denx.de> wrote:
> Add driver for the DesignWare APB GPIO IP block.
> This driver is DM capable and probes from DT.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> ---
>  drivers/gpio/Kconfig      |   7 ++
>  drivers/gpio/Makefile     |   1 +
>  drivers/gpio/dwapb_gpio.c | 167 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 175 insertions(+)
>  create mode 100644 drivers/gpio/dwapb_gpio.c

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

Please see nits/suggestions below.

Are you going to submit a GPIO binding change for this?

>
> V2: Obtain the bank name from the "bank-name" property instead.
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 0c43777..c04388d 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -8,6 +8,13 @@ config DM_GPIO
>           particular GPIOs that they provide. The uclass interface
>           is defined in include/asm-generic/gpio.h.
>
> +config DWAPB_GPIO
> +       bool "DWAPB GPIO driver"
> +       depends on DM && DM_GPIO
> +       default n
> +       help
> +         Support for the Designware APB GPIO driver.

You could expand this to talk about bank naming, features supported,
chips which use it.

> +
>  config LPC32XX_GPIO
>         bool "LPC32XX GPIO driver"
>         depends on DM
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 67c6374..603c96b 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -6,6 +6,7 @@
>  #
>
>  ifndef CONFIG_SPL_BUILD
> +obj-$(CONFIG_DWAPB_GPIO)       += dwapb_gpio.o
>  obj-$(CONFIG_AXP_GPIO)         += axp_gpio.o
>  endif
>  obj-$(CONFIG_DM_GPIO)          += gpio-uclass.o
> diff --git a/drivers/gpio/dwapb_gpio.c b/drivers/gpio/dwapb_gpio.c
> new file mode 100644
> index 0000000..72cec48
> --- /dev/null
> +++ b/drivers/gpio/dwapb_gpio.c
> @@ -0,0 +1,167 @@
> +/*
> + * (C) Copyright 2015 Marek Vasut <marex@denx.de>
> + *
> + * DesignWare APB GPIO driver
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <malloc.h>
> +#include <asm/arch/gpio.h>
> +#include <asm/gpio.h>
> +#include <asm/io.h>
> +#include <dm.h>
> +#include <dm/device-internal.h>
> +#include <dm/lists.h>
> +#include <dm/root.h>
> +#include <errno.h>

should go just below common.h

> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +#define GPIO_SWPORTA_DR                0x00
> +#define GPIO_SWPORTA_DDR       0x04
> +#define GPIO_INTEN             0x30
> +#define GPIO_INTMASK           0x34
> +#define GPIO_INTTYPE_LEVEL     0x38
> +#define GPIO_INT_POLARITY      0x3c
> +#define GPIO_INTSTATUS         0x40
> +#define GPIO_PORTA_DEBOUNCE    0x48
> +#define GPIO_PORTA_EOI         0x4c
> +#define GPIO_EXT_PORTA         0x50

What's the deal with C structures? Has the policy on this changed? I
can't help thinking that your GPIO_ prefix is unnecessary.

> +
> +struct gpio_dwapb_platdata {
> +       const char      *name;
> +       int             bank;
> +       int             pins;
> +       fdt_addr_t      base;
> +};
> +
> +static int dwapb_gpio_direction_input(struct udevice *dev, unsigned pin)
> +{
> +       struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
> +
> +       clrbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);
> +       return 0;
> +}
> +
> +static int dwapb_gpio_direction_output(struct udevice *dev, unsigned pin,
> +                                    int val)
> +{
> +       struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
> +
> +       setbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);
> +
> +       if (val)
> +               setbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
> +       else
> +               clrbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
> +
> +       return 0;
> +}
> +
> +static int dwapb_gpio_get_value(struct udevice *dev, unsigned pin)
> +{
> +       struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
> +       return !!(readl(plat->base + GPIO_EXT_PORTA) & (1 << pin));
> +}
> +
> +
> +static int dwapb_gpio_set_value(struct udevice *dev, unsigned pin, int val)
> +{
> +       struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
> +
> +       if (val)
> +               setbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
> +       else
> +               clrbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
> +
> +       return 0;
> +}
> +
> +static const struct dm_gpio_ops gpio_dwapb_ops = {
> +       .direction_input        = dwapb_gpio_direction_input,
> +       .direction_output       = dwapb_gpio_direction_output,
> +       .get_value              = dwapb_gpio_get_value,
> +       .set_value              = dwapb_gpio_set_value,
> +};
> +
> +static int gpio_dwapb_probe(struct udevice *dev)
> +{
> +       struct gpio_dev_priv *priv = dev_get_uclass_priv(dev);
> +       struct gpio_dwapb_platdata *plat = dev->platdata;
> +
> +       if (!plat)
> +               return 0;
> +
> +       priv->gpio_count = plat->pins;
> +       priv->bank_name = plat->name;
> +
> +       return 0;
> +}
> +
> +static int gpio_dwapb_bind(struct udevice *dev)
> +{
> +       struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
> +       const void *blob = gd->fdt_blob;
> +       struct udevice *subdev;
> +       fdt_addr_t base;
> +       int ret, node, bank = 0;
> +
> +       /* If this is a child device, there is nothing to do here */
> +       if (plat)
> +               return 0;
> +
> +       base = fdtdec_get_addr(blob, dev->of_offset, "reg");
> +       if (base == FDT_ADDR_T_NONE) {
> +               debug("Can't get the GPIO register base address\n");
> +               return -ENXIO;

-EINVAL I think, since this is an invalid parameter

> +       }
> +
> +       for (node = fdt_first_subnode(blob, dev->of_offset);
> +            node > 0;
> +            node = fdt_next_subnode(blob, node)) {
> +               if (!fdtdec_get_bool(blob, node, "gpio-controller"))
> +                       continue;
> +
> +               plat = NULL;
> +               plat = calloc(1, sizeof(*plat));
> +               if (!plat)
> +                       return -ENOMEM;
> +
> +               plat->base = base;
> +               plat->bank = bank;
> +               plat->pins = fdtdec_get_int(blob, node, "snps,nr-gpios", 0);
> +               ret = fdt_get_string(blob, node, "bank-name", &plat->name);
> +               if (ret)
> +                       goto err;
> +
> +               ret = device_bind(dev, dev->driver, plat->name,
> +                                 plat, -1, &subdev);
> +               if (ret)
> +                       goto err;
> +
> +               subdev->of_offset = node;
> +               bank++;
> +       }
> +
> +       return 0;
> +
> +err:
> +       free(plat);
> +       return ret;
> +}
> +
> +static const struct udevice_id gpio_dwapb_ids[] = {
> +       { .compatible = "snps,dw-apb-gpio" },
> +       { }
> +};
> +
> +U_BOOT_DRIVER(gpio_dwapb) = {
> +       .name           = "gpio-dwapb",
> +       .id             = UCLASS_GPIO,
> +       .of_match       = gpio_dwapb_ids,
> +       .ops            = &gpio_dwapb_ops,
> +       .bind           = gpio_dwapb_bind,
> +       .probe          = gpio_dwapb_probe,
> +};
> --
> 2.1.4
>

Regards,
Simon

[U-Boot] [PATCH 2/3] arm: socfpga: dts: Add bank-name property to each GPIO bank

[Simon Glass]

On 10 August 2015 at 09:30, Marek Vasut <marex@denx.de> wrote:
> Add "bank-name" property to each GPIO bank to give it unique name.
> The approach here is exactly the same as with the "regulator-name"
> property for regulators.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> ---
>  arch/arm/dts/socfpga.dtsi | 3 +++
>  1 file changed, 3 insertions(+)

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

[U-Boot] [PATCH 3/3] arm: socfpga: Enable DWAPB GPIO driver

[Simon Glass]

On 10 August 2015 at 09:30, Marek Vasut <marex@denx.de> wrote:
> Enable the DWAPB GPIO driver for SoCFPGA Cyclone V and Arria V.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> ---
>  configs/socfpga_arria5_defconfig   | 2 ++
>  configs/socfpga_cyclone5_defconfig | 2 ++
>  configs/socfpga_socrates_defconfig | 2 ++
>  include/configs/socfpga_arria5.h   | 2 +-
>  include/configs/socfpga_cyclone5.h | 2 +-
>  5 files changed, 8 insertions(+), 2 deletions(-)

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

[U-Boot] [PATCH V2 1/3] gpio: Add DW APB GPIO driver

[Fabio Estevam]

On Wed, Aug 12, 2015 at 11:15 AM, Simon Glass <sjg@chromium.org> wrote:

>> +#define GPIO_SWPORTA_DR                0x00
>> +#define GPIO_SWPORTA_DDR       0x04
>> +#define GPIO_INTEN             0x30
>> +#define GPIO_INTMASK           0x34
>> +#define GPIO_INTTYPE_LEVEL     0x38
>> +#define GPIO_INT_POLARITY      0x3c
>> +#define GPIO_INTSTATUS         0x40
>> +#define GPIO_PORTA_DEBOUNCE    0x48
>> +#define GPIO_PORTA_EOI         0x4c
>> +#define GPIO_EXT_PORTA         0x50
>
> What's the deal with C structures? Has the policy on this changed? I

I thought we no longer need to access registers via structs, and
accessing them via base + offset, like the kernel does is OK in
U-boot:
https://www.marc.info/?l=u-boot&m=142609602127309&w=2

Regards,

Fabio Estevam

[U-Boot] [PATCH V2 1/3] gpio: Add DW APB GPIO driver

[Simon Glass]

+Tom

Hi Fabio,

On 12 August 2015 at 08:24, Fabio Estevam <festevam@gmail.com> wrote:
>
> On Wed, Aug 12, 2015 at 11:15 AM, Simon Glass <sjg@chromium.org> wrote:
>
> >> +#define GPIO_SWPORTA_DR                0x00
> >> +#define GPIO_SWPORTA_DDR       0x04
> >> +#define GPIO_INTEN             0x30
> >> +#define GPIO_INTMASK           0x34
> >> +#define GPIO_INTTYPE_LEVEL     0x38
> >> +#define GPIO_INT_POLARITY      0x3c
> >> +#define GPIO_INTSTATUS         0x40
> >> +#define GPIO_PORTA_DEBOUNCE    0x48
> >> +#define GPIO_PORTA_EOI         0x4c
> >> +#define GPIO_EXT_PORTA         0x50
> >
> > What's the deal with C structures? Has the policy on this changed? I
>
> I thought we no longer need to access registers via structs, and
> accessing them via base + offset, like the kernel does is OK in
> U-boot:
> https://www.marc.info/?l=u-boot&m=142609602127309&w=2

Thanks for the link. No I did not know that.

Perhaps we should add the type-checking referred to in that thread?
What is it and who is doing it?

Regards,
Simon

[U-Boot] [PATCH V2 1/3] gpio: Add DW APB GPIO driver

[Marek Vasut]

On Wednesday, August 12, 2015 at 04:15:00 PM, Simon Glass wrote:
> Hi Marek,
> 
> On 10 August 2015 at 09:30, Marek Vasut <marex@denx.de> wrote:
> > Add driver for the DesignWare APB GPIO IP block.
> > This driver is DM capable and probes from DT.
> > 
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Simon Glass <sjg@chromium.org>
> > ---
> > 
> >  drivers/gpio/Kconfig      |   7 ++
> >  drivers/gpio/Makefile     |   1 +
> >  drivers/gpio/dwapb_gpio.c | 167
> >  ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 175
> >  insertions(+)
> >  create mode 100644 drivers/gpio/dwapb_gpio.c
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Please see nits/suggestions below.
> 
> Are you going to submit a GPIO binding change for this?

You mean for the bank-name ? Yes, I think it only makes sense to do it.

> > V2: Obtain the bank name from the "bank-name" property instead.
> > 
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index 0c43777..c04388d 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -8,6 +8,13 @@ config DM_GPIO
> > 
> >           particular GPIOs that they provide. The uclass interface
> >           is defined in include/asm-generic/gpio.h.
> > 
> > +config DWAPB_GPIO
> > +       bool "DWAPB GPIO driver"
> > +       depends on DM && DM_GPIO
> > +       default n
> > +       help
> > +         Support for the Designware APB GPIO driver.
> 
> You could expand this to talk about bank naming, features supported,
> chips which use it.

Done

> > +
> > 
> >  config LPC32XX_GPIO
> >  
> >         bool "LPC32XX GPIO driver"
> >         depends on DM
> > 
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > index 67c6374..603c96b 100644
> > --- a/drivers/gpio/Makefile
> > +++ b/drivers/gpio/Makefile
> > @@ -6,6 +6,7 @@
> > 
> >  #
> >  
> >  ifndef CONFIG_SPL_BUILD
> > 
> > +obj-$(CONFIG_DWAPB_GPIO)       += dwapb_gpio.o
> > 
> >  obj-$(CONFIG_AXP_GPIO)         += axp_gpio.o
> >  endif
> >  obj-$(CONFIG_DM_GPIO)          += gpio-uclass.o
> > 
> > diff --git a/drivers/gpio/dwapb_gpio.c b/drivers/gpio/dwapb_gpio.c
> > new file mode 100644
> > index 0000000..72cec48
> > --- /dev/null
> > +++ b/drivers/gpio/dwapb_gpio.c
> > @@ -0,0 +1,167 @@
> > +/*
> > + * (C) Copyright 2015 Marek Vasut <marex@denx.de>
> > + *
> > + * DesignWare APB GPIO driver
> > + *
> > + * SPDX-License-Identifier:    GPL-2.0+
> > + */
> > +
> > +#include <common.h>
> > +#include <malloc.h>
> > +#include <asm/arch/gpio.h>
> > +#include <asm/gpio.h>
> > +#include <asm/io.h>
> > +#include <dm.h>
> > +#include <dm/device-internal.h>
> > +#include <dm/lists.h>
> > +#include <dm/root.h>
> > +#include <errno.h>
> 
> should go just below common.h

Yup

> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +#define GPIO_SWPORTA_DR                0x00
> > +#define GPIO_SWPORTA_DDR       0x04
> > +#define GPIO_INTEN             0x30
> > +#define GPIO_INTMASK           0x34
> > +#define GPIO_INTTYPE_LEVEL     0x38
> > +#define GPIO_INT_POLARITY      0x3c
> > +#define GPIO_INTSTATUS         0x40
> > +#define GPIO_PORTA_DEBOUNCE    0x48
> > +#define GPIO_PORTA_EOI         0x4c
> > +#define GPIO_EXT_PORTA         0x50
> 
> What's the deal with C structures? Has the policy on this changed? I
> can't help thinking that your GPIO_ prefix is unnecessary.

I think we don't do that anymore. The GPIO_ stuff is copied from Linux.

> > +
> > +struct gpio_dwapb_platdata {
> > +       const char      *name;
> > +       int             bank;
> > +       int             pins;
> > +       fdt_addr_t      base;
> > +};
> > +
> > +static int dwapb_gpio_direction_input(struct udevice *dev, unsigned pin)
> > +{
> > +       struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
> > +
> > +       clrbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);
> > +       return 0;
> > +}
> > +
> > +static int dwapb_gpio_direction_output(struct udevice *dev, unsigned
> > pin, +                                    int val)
> > +{
> > +       struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
> > +
> > +       setbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);
> > +
> > +       if (val)
> > +               setbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
> > +       else
> > +               clrbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
> > +
> > +       return 0;
> > +}
> > +
> > +static int dwapb_gpio_get_value(struct udevice *dev, unsigned pin)
> > +{
> > +       struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
> > +       return !!(readl(plat->base + GPIO_EXT_PORTA) & (1 << pin));
> > +}
> > +
> > +
> > +static int dwapb_gpio_set_value(struct udevice *dev, unsigned pin, int
> > val) +{
> > +       struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
> > +
> > +       if (val)
> > +               setbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
> > +       else
> > +               clrbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct dm_gpio_ops gpio_dwapb_ops = {
> > +       .direction_input        = dwapb_gpio_direction_input,
> > +       .direction_output       = dwapb_gpio_direction_output,
> > +       .get_value              = dwapb_gpio_get_value,
> > +       .set_value              = dwapb_gpio_set_value,
> > +};
> > +
> > +static int gpio_dwapb_probe(struct udevice *dev)
> > +{
> > +       struct gpio_dev_priv *priv = dev_get_uclass_priv(dev);
> > +       struct gpio_dwapb_platdata *plat = dev->platdata;
> > +
> > +       if (!plat)
> > +               return 0;
> > +
> > +       priv->gpio_count = plat->pins;
> > +       priv->bank_name = plat->name;
> > +
> > +       return 0;
> > +}
> > +
> > +static int gpio_dwapb_bind(struct udevice *dev)
> > +{
> > +       struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
> > +       const void *blob = gd->fdt_blob;
> > +       struct udevice *subdev;
> > +       fdt_addr_t base;
> > +       int ret, node, bank = 0;
> > +
> > +       /* If this is a child device, there is nothing to do here */
> > +       if (plat)
> > +               return 0;
> > +
> > +       base = fdtdec_get_addr(blob, dev->of_offset, "reg");
> > +       if (base == FDT_ADDR_T_NONE) {
> > +               debug("Can't get the GPIO register base address\n");
> > +               return -ENXIO;
> 
> -EINVAL I think, since this is an invalid parameter

OK

[...]

[U-Boot] [PATCH V3 1/3] gpio: Add DW APB GPIO driver

[Marek Vasut]

Add driver for the DesignWare APB GPIO IP block.
This driver is DM capable and probes from DT.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Simon Glass <sjg@chromium.org>
---
 drivers/gpio/Kconfig      |  10 +++
 drivers/gpio/Makefile     |   1 +
 drivers/gpio/dwapb_gpio.c | 167 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 178 insertions(+)
 create mode 100644 drivers/gpio/dwapb_gpio.c

V2: Obtain the bank name from the "bank-name" property instead.
V3: Tweak the description, replace ENXIO with EINVAL, fix Simon's comments.

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 5934597..8db48d1 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -14,6 +14,16 @@ config DM_GPIO
 	  particular GPIOs that they provide. The uclass interface
 	  is defined in include/asm-generic/gpio.h.
 
+config DWAPB_GPIO
+	bool "DWAPB GPIO driver"
+	depends on DM && DM_GPIO
+	default n
+	help
+	  Support for the Designware APB GPIO driver. This driver can
+	  be probed only from device tree. Each DW APB GPIO controller
+	  can contain multiple GPIO banks, use the "bank-name" property
+	  in the OF to select reasonable name for each bank.
+
 config LPC32XX_GPIO
 	bool "LPC32XX GPIO driver"
 	depends on DM
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 26f2574..ba18594 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -6,6 +6,7 @@
 #
 
 ifndef CONFIG_SPL_BUILD
+obj-$(CONFIG_DWAPB_GPIO)	+= dwapb_gpio.o
 obj-$(CONFIG_AXP_GPIO)		+= axp_gpio.o
 endif
 obj-$(CONFIG_DM_GPIO)		+= gpio-uclass.o
diff --git a/drivers/gpio/dwapb_gpio.c b/drivers/gpio/dwapb_gpio.c
new file mode 100644
index 0000000..cee93e8
--- /dev/null
+++ b/drivers/gpio/dwapb_gpio.c
@@ -0,0 +1,167 @@
+/*
+ * (C) Copyright 2015 Marek Vasut <marex@denx.de>
+ *
+ * DesignWare APB GPIO driver
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <errno.h>
+#include <malloc.h>
+#include <asm/arch/gpio.h>
+#include <asm/gpio.h>
+#include <asm/io.h>
+#include <dm.h>
+#include <dm/device-internal.h>
+#include <dm/lists.h>
+#include <dm/root.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+#define GPIO_SWPORTA_DR		0x00
+#define GPIO_SWPORTA_DDR	0x04
+#define GPIO_INTEN		0x30
+#define GPIO_INTMASK		0x34
+#define GPIO_INTTYPE_LEVEL	0x38
+#define GPIO_INT_POLARITY	0x3c
+#define GPIO_INTSTATUS		0x40
+#define GPIO_PORTA_DEBOUNCE	0x48
+#define GPIO_PORTA_EOI		0x4c
+#define GPIO_EXT_PORTA		0x50
+
+struct gpio_dwapb_platdata {
+	const char	*name;
+	int		bank;
+	int		pins;
+	fdt_addr_t	base;
+};
+
+static int dwapb_gpio_direction_input(struct udevice *dev, unsigned pin)
+{
+	struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
+
+	clrbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);
+	return 0;
+}
+
+static int dwapb_gpio_direction_output(struct udevice *dev, unsigned pin,
+				     int val)
+{
+	struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
+
+	setbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);
+
+	if (val)
+		setbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
+	else
+		clrbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
+
+	return 0;
+}
+
+static int dwapb_gpio_get_value(struct udevice *dev, unsigned pin)
+{
+	struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
+	return !!(readl(plat->base + GPIO_EXT_PORTA) & (1 << pin));
+}
+
+
+static int dwapb_gpio_set_value(struct udevice *dev, unsigned pin, int val)
+{
+	struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
+
+	if (val)
+		setbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
+	else
+		clrbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
+
+	return 0;
+}
+
+static const struct dm_gpio_ops gpio_dwapb_ops = {
+	.direction_input	= dwapb_gpio_direction_input,
+	.direction_output	= dwapb_gpio_direction_output,
+	.get_value		= dwapb_gpio_get_value,
+	.set_value		= dwapb_gpio_set_value,
+};
+
+static int gpio_dwapb_probe(struct udevice *dev)
+{
+	struct gpio_dev_priv *priv = dev_get_uclass_priv(dev);
+	struct gpio_dwapb_platdata *plat = dev->platdata;
+
+	if (!plat)
+		return 0;
+
+	priv->gpio_count = plat->pins;
+	priv->bank_name = plat->name;
+
+	return 0;
+}
+
+static int gpio_dwapb_bind(struct udevice *dev)
+{
+	struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
+	const void *blob = gd->fdt_blob;
+	struct udevice *subdev;
+	fdt_addr_t base;
+	int ret, node, bank = 0;
+
+	/* If this is a child device, there is nothing to do here */
+	if (plat)
+		return 0;
+
+	base = fdtdec_get_addr(blob, dev->of_offset, "reg");
+	if (base == FDT_ADDR_T_NONE) {
+		debug("Can't get the GPIO register base address\n");
+		return -EINVAL;
+	}
+
+	for (node = fdt_first_subnode(blob, dev->of_offset);
+	     node > 0;
+	     node = fdt_next_subnode(blob, node)) {
+		if (!fdtdec_get_bool(blob, node, "gpio-controller"))
+			continue;
+
+		plat = NULL;
+		plat = calloc(1, sizeof(*plat));
+		if (!plat)
+			return -ENOMEM;
+
+		plat->base = base;
+		plat->bank = bank;
+		plat->pins = fdtdec_get_int(blob, node, "snps,nr-gpios", 0);
+		ret = fdt_get_string(blob, node, "bank-name", &plat->name);
+		if (ret)
+			goto err;
+
+		ret = device_bind(dev, dev->driver, plat->name,
+				  plat, -1, &subdev);
+		if (ret)
+			goto err;
+
+		subdev->of_offset = node;
+		bank++;
+	}
+
+	return 0;
+
+err:
+	free(plat);
+	return ret;
+}
+
+static const struct udevice_id gpio_dwapb_ids[] = {
+	{ .compatible = "snps,dw-apb-gpio" },
+	{ }
+};
+
+U_BOOT_DRIVER(gpio_dwapb) = {
+	.name		= "gpio-dwapb",
+	.id		= UCLASS_GPIO,
+	.of_match	= gpio_dwapb_ids,
+	.ops		= &gpio_dwapb_ops,
+	.bind		= gpio_dwapb_bind,
+	.probe		= gpio_dwapb_probe,
+};
-- 
2.1.4