On Tue, Sep 12, 2017 at 11:27 PM, Simon Glass <s...@chromium.org> wrote:
> Hi Adam,
>
> On 12 September 2017 at 21:28, Adam Ford <aford...@gmail.com> wrote:
>> This adds DM compatibility for the davinici GPIO driver.
>> Tested on da850-evm.
>>
>> Signed-off-by: Adam Ford <aford...@gmail.com>
>> ---
>> 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(-)
>>
>> diff --git a/arch/arm/mach-davinci/include/mach/gpio.h
>> b/arch/arm/mach-davinci/include/mach/gpio.h
>> index 7da0060..32cae3a 100644
>> --- a/arch/arm/mach-davinci/include/mach/gpio.h
>> +++ b/arch/arm/mach-davinci/include/mach/gpio.h
>> @@ -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
>> diff --git a/board/davinci/da8xxevm/da850evm.c
>> b/board/davinci/da8xxevm/da850evm.c
>> index 42bc0f4..2a54d0f 100644
>> --- a/board/davinci/da8xxevm/da850evm.c
>> +++ b/board/davinci/da8xxevm/da850evm.c
>> @@ -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>
>> diff --git a/configs/da850evm_defconfig b/configs/da850evm_defconfig
>> index b966fa7..24507da 100644
>> --- a/configs/da850evm_defconfig
>> +++ b/configs/da850evm_defconfig
>> @@ -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
>> diff --git a/drivers/gpio/da8xx_gpio.c b/drivers/gpio/da8xx_gpio.c
>> index fa3a394..b64f936 100644
>> --- a/drivers/gpio/da8xx_gpio.c
>> +++ b/drivers/gpio/da8xx_gpio.c
>> @@ -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)
>> +{
>> + 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.
>
> 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.
>
>> +
>> + /*
>> + * 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?
>> + */
>> + 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
>> diff --git a/include/configs/da850evm.h b/include/configs/da850evm.h
>> index ab2e6ae..ea8c441 100644
>> --- a/include/configs/da850evm.h
>> +++ b/include/configs/da850evm.h
>> @@ -262,6 +262,7 @@
>> #define CONFIG_ENV_SECT_SIZE (64 << 10)
>> #endif
>>
>> +#define CONFIG_DA8XX_GPIO
>> /*
>> * U-Boot general configuration
>> */
>> --
>> 2.7.4
>
> Regards,
> Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
