dm: gpio: Add DM compatibilty to GPIO driver for Davinci

PATCH Superseded Adam Ford 2 participants 4 messages Sep 13, 2017 – Sep 17, 2017

Subsystems (4)

Arm (32-bit) Arm sub-architectures GPIO TI DaVinci machine support

lore

Share & tools

lore

Adam Ford

author

2017-09-13 03:28 UTC #0 lore raw

PATCH v2 1/1 dm: gpio: Add DM compatibilty to GPIO driver for Davinci

This adds DM compatibility for the davinici GPIO driver. Tested on da850-evm.

Signed-off-by Adam Ford <[email protected]>

---
V2: The bank calculation needs to take into account the size of the struct
Whitespace fixes

 arch/arm/mach-davinci/include/mach/gpio.h |  14 +-
 board/davinci/da8xxevm/da850evm.c         |   2 +
 configs/da850evm_defconfig                |   3 +-
 drivers/gpio/da8xx_gpio.c                 | 208 +++++++++++++++++++++++++++---
 include/configs/da850evm.h                |   1 +
 5 files changed, 205 insertions(+), 23 deletions(-)

arch/arm/mach-davinci/include/mach/gpio.h +13 -1

index 7da0060..32cae3a 100644
@@ -40,7 +40,7 @@ struct davinci_gpio_bank {
 	unsigned int irq_num;
 	unsigned int irq_mask;
 	unsigned long *in_use;
-	unsigned long base;
+	struct davinci_gpio *base;
 };
 
 #define davinci_gpio_bank01 ((struct davinci_gpio *)DAVINCI_GPIO_BANK01)

@@ -49,7 +49,9 @@ struct davinci_gpio_bank {
 #define davinci_gpio_bank67 ((struct davinci_gpio *)DAVINCI_GPIO_BANK67)
 #define davinci_gpio_bank8 ((struct davinci_gpio *)DAVINCI_GPIO_BANK8)
 
+#ifndef CONFIG_DM_GPIO
 #define gpio_status()		gpio_info()
+#endif
 #define GPIO_NAME_SIZE		20
 #if defined(CONFIG_SOC_DM644X)
 /* GPIO0 to GPIO53, omit the V3.3 volts one */

@@ -64,4 +66,14 @@ struct davinci_gpio_bank {
 
 void gpio_info(void);
 
+#ifdef CONFIG_DM_GPIO
+
+/* Information about a GPIO bank */
+struct davinci_gpio_platdata {
+	int bank_index;
+	ulong base;	/* address of registers in physical memory */
+	const char *port_name;
+};
+#endif
+
 #endif

board/davinci/da8xxevm/da850evm.c +2 -0

index 42bc0f4..2a54d0f 100644
@@ -10,6 +10,7 @@
  */
 
 #include <common.h>
+#include <dm.h>
 #include <i2c.h>
 #include <net.h>
 #include <netdev.h>

@@ -24,6 +25,7 @@
 #include <linux/errno.h>
 #include <hwconfig.h>
 #include <asm/mach-types.h>
+#include <asm/gpio.h>
 
 #ifdef CONFIG_MMC_DAVINCI
 #include <mmc.h>

configs/da850evm_defconfig +2 -1

index b966fa7..24507da 100644
@@ -20,14 +20,15 @@ CONFIG_HUSH_PARSER=y
 CONFIG_SYS_PROMPT="U-Boot > "
 # CONFIG_CMD_IMLS is not set
 CONFIG_CRC32_VERIFY=y
+# CONFIG_CMD_EEPROM is not set
 # CONFIG_CMD_FLASH is not set
-# CONFIG_CMD_GPIO is not set
 # CONFIG_CMD_SETEXPR is not set
 CONFIG_CMD_MTDPARTS=y
 CONFIG_CMD_DIAG=y
 CONFIG_OF_CONTROL=y
 CONFIG_ENV_IS_IN_SPI_FLASH=y
 CONFIG_DM=y
+CONFIG_DM_GPIO=y
 CONFIG_DM_I2C=y
 CONFIG_DM_I2C_COMPAT=y
 CONFIG_I2C_SET_DEFAULT_BUS_NUM=y

drivers/gpio/da8xx_gpio.c +187 ~~-21~~

261 lines not on this page · raw message

include/configs/da850evm.h +1 -0

index ab2e6ae..ea8c441 100644
@@ -262,6 +262,7 @@
 #define CONFIG_ENV_SECT_SIZE		(64 << 10)
 #endif
 
+#define CONFIG_DA8XX_GPIO
 /*
  * U-Boot general configuration
  */

Signature · 1 line

2.7.4

Adam Ford

author

2017-09-17 10:29 UTC #2 lore raw

PATCH v2 1/1 dm: gpio: Add DM compatibilty to GPIO driver for Davinci

Quoted 336 lines · Simon Glass

Hi Adam,

Quoted 320 lines · Adam Ford

This adds DM compatibility for the davinici GPIO driver. Tested on da850-evm.

Signed-off-by Adam Ford <[email protected]>

---
V2: The bank calculation needs to take into account the size of the struct
Whitespace fixes

 arch/arm/mach-davinci/include/mach/gpio.h |  14 +-
 board/davinci/da8xxevm/da850evm.c         |   2 +
 configs/da850evm_defconfig                |   3 +-
 drivers/gpio/da8xx_gpio.c                 | 208 +++++++++++++++++++++++++++---
 include/configs/da850evm.h                |   1 +
 5 files changed, 205 insertions(+), 23 deletions(-)

arch/arm/mach-davinci/include/mach/gpio.h +13 -1

index 7da0060..32cae3a 100644
@@ -40,7 +40,7 @@ struct davinci_gpio_bank {
        unsigned int irq_num;
        unsigned int irq_mask;
        unsigned long *in_use;
-       unsigned long base;
+       struct davinci_gpio *base;
 };
 
 #define davinci_gpio_bank01 ((struct davinci_gpio *)DAVINCI_GPIO_BANK01)

@@ -49,7 +49,9 @@ struct davinci_gpio_bank {
 #define davinci_gpio_bank67 ((struct davinci_gpio *)DAVINCI_GPIO_BANK67)
 #define davinci_gpio_bank8 ((struct davinci_gpio *)DAVINCI_GPIO_BANK8)
 
+#ifndef CONFIG_DM_GPIO
 #define gpio_status()          gpio_info()
+#endif
 #define GPIO_NAME_SIZE         20
 #if defined(CONFIG_SOC_DM644X)
 /* GPIO0 to GPIO53, omit the V3.3 volts one */

@@ -64,4 +66,14 @@ struct davinci_gpio_bank {
 
 void gpio_info(void);
 
+#ifdef CONFIG_DM_GPIO
+
+/* Information about a GPIO bank */
+struct davinci_gpio_platdata {
+       int bank_index;
+       ulong base;     /* address of registers in physical memory */
+       const char *port_name;
+};
+#endif
+
 #endif

board/davinci/da8xxevm/da850evm.c +2 -0

index 42bc0f4..2a54d0f 100644
@@ -10,6 +10,7 @@
  */
 
 #include <common.h>
+#include <dm.h>
 #include <i2c.h>
 #include <net.h>
 #include <netdev.h>

@@ -24,6 +25,7 @@
 #include <linux/errno.h>
 #include <hwconfig.h>
 #include <asm/mach-types.h>
+#include <asm/gpio.h>
 
 #ifdef CONFIG_MMC_DAVINCI
 #include <mmc.h>

configs/da850evm_defconfig +2 -1

index b966fa7..24507da 100644
@@ -20,14 +20,15 @@ CONFIG_HUSH_PARSER=y
 CONFIG_SYS_PROMPT="U-Boot > "
 # CONFIG_CMD_IMLS is not set
 CONFIG_CRC32_VERIFY=y
+# CONFIG_CMD_EEPROM is not set
 # CONFIG_CMD_FLASH is not set
-# CONFIG_CMD_GPIO is not set
 # CONFIG_CMD_SETEXPR is not set
 CONFIG_CMD_MTDPARTS=y
 CONFIG_CMD_DIAG=y
 CONFIG_OF_CONTROL=y
 CONFIG_ENV_IS_IN_SPI_FLASH=y
 CONFIG_DM=y
+CONFIG_DM_GPIO=y
 CONFIG_DM_I2C=y
 CONFIG_DM_I2C_COMPAT=y
 CONFIG_I2C_SET_DEFAULT_BUS_NUM=y

drivers/gpio/da8xx_gpio.c +143 ~~-21~~

index fa3a394..b64f936 100644
@@ -8,16 +8,20 @@
  */
 
 #include <common.h>
+#include <dm.h>
+#include <fdtdec.h>
 #include <asm/io.h>
 #include <asm/gpio.h>
 #include <asm/arch/hardware.h>
 #include <asm/arch/davinci_misc.h>
 
+#ifndef CONFIG_DM_GPIO
 static struct gpio_registry {
        int is_registered;
        char name[GPIO_NAME_SIZE];
 } gpio_registry[MAX_NUM_GPIOS];
 
+
 #if defined(CONFIG_SOC_DA8XX)
 #define pinmux(x)       (&davinci_syscfg_regs->pinmux[x])

@@ -334,42 +338,30 @@ int gpio_free(unsigned gpio)
        /* Do not configure as input or change pin mux here */
        return 0;
 }
+#endif
 
-int gpio_direction_input(unsigned gpio)
+static int _gpio_direction_output(struct davinci_gpio *bank, unsigned gpio, int value)
 {
-       struct davinci_gpio *bank;
-
-       bank = GPIO_BANK(gpio);
-       setbits_le32(&bank->dir, 1U << GPIO_BIT(gpio));
+       clrbits_le32(&bank->dir, 1U << GPIO_BIT(gpio));
+       gpio_set_value(gpio, value);
        return 0;
 }
 
-int gpio_direction_output(unsigned gpio, int value)
+static int _gpio_direction_input(struct davinci_gpio *bank, unsigned gpio)
 {
-       struct davinci_gpio *bank;
-
-       bank = GPIO_BANK(gpio);
-       clrbits_le32(&bank->dir, 1U << GPIO_BIT(gpio));
-       gpio_set_value(gpio, value);
+       setbits_le32(&bank->dir, 1U << GPIO_BIT(gpio));
        return 0;
 }
 
-int gpio_get_value(unsigned gpio)
+static int _gpio_get_value(struct davinci_gpio *bank, unsigned gpio)
 {
-       struct davinci_gpio *bank;
        unsigned int ip;
-
-       bank = GPIO_BANK(gpio);
        ip = in_le32(&bank->in_data) & (1U << GPIO_BIT(gpio));
        return ip ? 1 : 0;
 }
 
-int gpio_set_value(unsigned gpio, int value)
+static int _gpio_set_value(struct davinci_gpio *bank, unsigned gpio, int value)
 {
-       struct davinci_gpio *bank;
-
-       bank = GPIO_BANK(gpio);
-
        if (value)
                bank->set_data = 1U << GPIO_BIT(gpio);
        else

@@ -378,6 +370,13 @@ int gpio_set_value(unsigned gpio, int value)
        return 0;
 }
 
+static int _gpio_get_dir(struct davinci_gpio *bank, unsigned gpio)
+{
+       return in_le32(&bank->dir) & (1U << GPIO_BIT(gpio));
+}
+
+#ifndef CONFIG_DM_GPIO
+
 void gpio_info(void)
 {
        unsigned gpio, dir, val;

@@ -385,7 +384,8 @@ void gpio_info(void)
 
        for (gpio = 0; gpio < MAX_NUM_GPIOS; ++gpio) {
                bank = GPIO_BANK(gpio);
-               dir = in_le32(&bank->dir) & (1U << GPIO_BIT(gpio));
+               printf("gpio_info: Bank=%x\n", (int) bank);
+               dir = _gpio_get_dir(bank, gpio);
                val = gpio_get_value(gpio);
 
                printf("% 4d: %s: %d [%c] %s\n",

@@ -394,3 +394,169 @@ void gpio_info(void)
                        gpio_registry[gpio].name);
        }
 }
+
+int gpio_direction_input(unsigned gpio)
+{
+       struct davinci_gpio *bank;
+
+       bank = GPIO_BANK(gpio);
+       return _gpio_direction_input(bank, gpio);
+}
+
+int gpio_direction_output(unsigned gpio, int value)
+{
+       struct davinci_gpio *bank;
+
+       bank = GPIO_BANK(gpio);
+       return _gpio_direction_output(bank, gpio, value);
+}
+
+int gpio_get_value(unsigned gpio)
+{
+       struct davinci_gpio *bank;
+
+       bank = GPIO_BANK(gpio);
+       return _gpio_get_value(bank, gpio);
+}
+
+int gpio_set_value(unsigned gpio, int value)
+{
+       struct davinci_gpio *bank;
+
+       bank = GPIO_BANK(gpio);
+       return _gpio_set_value(bank, gpio, value);
+}
+
+#else /* CONFIG_DM_GPIO */
+/* set GPIO pin 'gpio' as an input */
+
+static struct davinci_gpio *davinci_get_gpio_bank(struct udevice *dev, unsigned offset)
+{
+       struct davinci_gpio_bank *bank = dev_get_priv(dev);
+
+       /* The device tree is not broken into banks but the infrastructure is
+        * expecting it this way, so we'll first include the 0x10 offset, then
+        * calculate the bank manually based on the offset.
+        */
+
+       return ((struct davinci_gpio *)bank->base) + 0x10 +
+                       ((offset >> 5) * sizeof(struct davinci_gpio));
+}
+
+static int davinci_gpio_direction_input(struct udevice *dev, unsigned offset)
+{
+       struct davinci_gpio *base = davinci_get_gpio_bank(dev, offset);
+
+       _gpio_direction_input(base, offset);
+       return 0;
+}
+
+/* set GPIO pin 'gpio' as an output, with polarity 'value' */
+static int davinci_gpio_direction_output(struct udevice *dev, unsigned offset,
+                                      int value)
+{
+       struct davinci_gpio *base = davinci_get_gpio_bank(dev, offset);
+
+       _gpio_direction_output(base, offset, value);
+       return 0;
+}
+
+/* read GPIO IN value of pin 'gpio' */
+static int davinci_gpio_get_value(struct udevice *dev, unsigned offset)
+{
+       struct davinci_gpio *base = davinci_get_gpio_bank(dev, offset);
+
+       return _gpio_get_value(base, offset);
+}
+
+/* write GPIO OUT value to pin 'gpio' */
+static int davinci_gpio_set_value(struct udevice *dev, unsigned offset,
+                                int value)
+{
+       struct davinci_gpio *base = davinci_get_gpio_bank(dev, offset);
+
+       _gpio_set_value(base, offset, value);
+
+       return 0;
+}
+
+static int davinci_gpio_get_function(struct udevice *dev, unsigned offset)
+{
+       unsigned int dir;
+       struct davinci_gpio *base = davinci_get_gpio_bank(dev, offset);
+
+       dir = _gpio_get_dir(base, offset);
+
+       if (dir)
+               return GPIOF_INPUT;
+
+       return GPIOF_OUTPUT;
+}
+
+static const struct dm_gpio_ops gpio_davinci_ops = {
+       .direction_input        = davinci_gpio_direction_input,
+       .direction_output       = davinci_gpio_direction_output,
+       .get_value              = davinci_gpio_get_value,
+       .set_value              = davinci_gpio_set_value,
+       .get_function           = davinci_gpio_get_function,
+};
+
+static int davinci_gpio_probe(struct udevice *dev)
+{
+       struct davinci_gpio_bank *bank = dev_get_priv(dev);
+       struct davinci_gpio_platdata *plat = dev_get_platdata(dev);
+       struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
+       const void *fdt = gd->fdt_blob;
+       int node = dev_of_offset(dev);
+
+       uc_priv->bank_name = plat->port_name;
+       uc_priv->gpio_count = fdtdec_get_int(fdt, node, "ti,ngpio", -1);
+       bank->base = (struct davinci_gpio *)plat->base;
+       return 0;
+}
+
+static int davinci_gpio_bind(struct udevice *dev)

drivers/gpio/da8xx_gpio.c void gpio_info(void) · line 528

Quoted 10 lines

drivers/gpio/da8xx_gpio.c +10 -0

+{
+       struct davinci_gpio_platdata *plat = dev->platdata;
+       fdt_addr_t base_addr;
+
+       if (plat)
+               return 0;
+
+       base_addr = devfdt_get_addr(dev);
+       if (base_addr == FDT_ADDR_T_NONE)
+               return -ENODEV;

-EINVAL. There is definitely a device.

Quoted 2 lines

Also we should not be reading the DT in the bind() method. This should happen in ofdata_to_platdata()

Can you point me to an example board you want me to use? Several boards do it this way including omap_gpio.c, mxc_gpio.c, imx_rgpio2.c, and others. I used the omap_gpio.c file as a model for this since they are similar.

Quoted 11 lines

drivers/gpio/da8xx_gpio.c void gpio_info(void) · line 534
Quoted 7 lines
drivers/gpio/da8xx_gpio.c +6 -0
+
+       /*
+       * TODO:
+       * When every board is converted to driver model and DT is
+       * supported, this can be done by auto-alloc feature, but
+       * not using calloc to alloc memory for platdata.
I don't really get this because we are in a driver-model method here. Can we not use the plat data here?

See the above comment. Some boards have a bind function while others use ofdata_to_platdata(). The readme shows two possible ways. I did it this way because the examples I followed did it this way. If it's good enough for them, why can't it be good enough this?

Quoted 45 lines

Quoted 41 lines

drivers/gpio/da8xx_gpio.c +28 -0

+       */
+       plat = calloc(1, sizeof(*plat));
+       if (!plat)
+               return -ENOMEM;
+
+       plat->base = base_addr;
+       plat->port_name = fdt_get_name(gd->fdt_blob, dev_of_offset(dev), NULL);
+       dev->platdata = plat;
+
+       return 0;
+}
+
+static const struct udevice_id davinci_gpio_ids[] = {
+       { .compatible = "ti,dm6441-gpio" },
+       { }
+};
+
+U_BOOT_DRIVER(gpio_davinci) = {
+       .name   = "gpio_davinci",
+       .id     = UCLASS_GPIO,
+       .ops    = &gpio_davinci_ops,
+       .of_match = davinci_gpio_ids,
+       .bind   = davinci_gpio_bind,
+       .probe  = davinci_gpio_probe,
+       .priv_auto_alloc_size = sizeof(struct davinci_gpio_bank),
+};
+
+#endif

include/configs/da850evm.h +1 -1

index ab2e6ae..ea8c441 100644
@@ -262,6 +262,7 @@
 #define CONFIG_ENV_SECT_SIZE           (64 << 10)
 #endif
 
+#define CONFIG_DA8XX_GPIO
 /*
  * U-Boot general configuration
  */
--

Quoted 1 line

2.7.4

Regards,
Simon

Simon Glass

2017-09-17 17:55 UTC #3 lore raw

PATCH v2 1/1 dm: gpio: Add DM compatibilty to GPIO driver for Davinci

Hi Adam,

Quoted 18 lines · Adam Ford

Quoted 17 lines · Simon Glass
Hi Adam,
Quoted 14 lines · Adam Ford
This adds DM compatibility for the davinici GPIO driver. Tested on da850-evm.
Signed-off-by Adam Ford <[email protected]>
---
V2: The bank calculation needs to take into account the size of the struct
Whitespace fixes
 arch/arm/mach-davinci/include/mach/gpio.h |  14 +-
 board/davinci/da8xxevm/da850evm.c         |   2 +
 configs/da850evm_defconfig                |   3 +-
 drivers/gpio/da8xx_gpio.c                 | 208 +++++++++++++++++++++++++++---
 include/configs/da850evm.h                |   1 +
 5 files changed, 205 insertions(+), 23 deletions(-)

[...]

Quoted 14 lines

drivers/gpio/da8xx_gpio.c void gpio_info(void) · line 528

Quoted 11 lines

drivers/gpio/da8xx_gpio.c +11 -0

+static int davinci_gpio_bind(struct udevice *dev)
+{
+       struct davinci_gpio_platdata *plat = dev->platdata;
+       fdt_addr_t base_addr;
+
+       if (plat)
+               return 0;
+
+       base_addr = devfdt_get_addr(dev);
+       if (base_addr == FDT_ADDR_T_NONE)
+               return -ENODEV;

-EINVAL. There is definitely a device.

Quoted 7 lines

Quoted 2 lines
Also we should not be reading the DT in the bind() method. This should happen in ofdata_to_platdata()
Can you point me to an example board you want me to use? Several boards do it this way including omap_gpio.c, mxc_gpio.c, imx_rgpio2.c, and others. I used the omap_gpio.c file as a model for this since they are similar.

See for example rk_gpio which is a simple driver.

One mode is tegra_gpio - that driver has an 'empty' parent device and then creates child GPIO devices in the bind() method.

Another is omap_gpio - that uses platform data and U_BOOT_DEVICE() to create drivers (for some boards) rather than device tree.

But here you don't see to be doing that. You just have a single GPIO device, right? If so, you should be able to put everything in ofdata_to_platdata().

If you set .platdata_auto_alloc_size it will automatically allocate the platform data.

Quoted 8 lines

Quoted 7 lines

drivers/gpio/da8xx_gpio.c +6 -0

+
+       /*
+       * TODO:
+       * When every board is converted to driver model and DT is
+       * supported, this can be done by auto-alloc feature, but
+       * not using calloc to alloc memory for platdata.

Quoted 8 lines

Quoted 3 lines
I don't really get this because we are in a driver-model method here. Can we not use the plat data here?
See the above comment. Some boards have a bind function while others use ofdata_to_platdata(). The readme shows two possible ways. I did it this way because the examples I followed did it this way. If it's good enough for them, why can't it be good enough this?

It is frustrating when you follow examples and they are not quite right. I will do a little series to tidy this stuff up a bit and cc you.

Which readme are you referring to?

Regards,
Simon