| Message ID | 1438029890-9901-1-git-send-email-marex@denx.de |
|---|---|
| State | Accepted |
| Delegated to: | Marek Vasut |
| Headers | show |
Hi Marek, On 27 July 2015 at 14:44, 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/Makefile | 1 + > drivers/gpio/dwapb_gpio.c | 167 ++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 168 insertions(+) > create mode 100644 drivers/gpio/dwapb_gpio.c > > 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..cdb6e78 > --- /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 Should use C structure, right? > + > +struct gpio_dwapb_platdata { > + char name[24]; > + 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); blank line after declarations > + 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, Do you want to implement .function? > +}; > + > +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 node, bank = 0; > + const char *name; > + > + /* 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; > + } > + > + name = fdt_get_name(blob, dev->of_offset, NULL); > + > + for (node = fdt_first_subnode(blob, dev->of_offset); > + node > 0; > + node = fdt_next_subnode(blob, node)) { > + int ret; > + > + if (!fdtdec_get_bool(blob, node, "gpio-controller")) > + continue; > + > + plat = NULL; > + plat = calloc(1, sizeof(*plat)); I suppose this should use devm_alloc() now. > + if (!plat) > + return -ENOMEM; > + > + plat->base = base; > + plat->bank = bank; > + plat->pins = fdtdec_get_int(blob, node, "snps,nr-gpios", 0); > + snprintf(plat->name, sizeof(plat->name) - 1, "%s-bank%i-", > + name, bank); Why such a long name? That's going to be a pain to type in the 'gpio' command. > + > + ret = device_bind(dev, dev->driver, plat->name, > + plat, -1, &subdev); > + if (ret) > + return ret; > + > + subdev->of_offset = node; > + bank++; > + } > + > + > + return 0; > +} > + > +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
On Sunday, August 02, 2015 at 11:28:13 PM, Simon Glass wrote: > Hi Marek, Hi, > On 27 July 2015 at 14:44, 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> [...] > > +#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 > > Should use C structure, right? My understanding is that we no longer want C structs . Tom ? [...] > > +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, > > Do you want to implement .function? No, the pinmuxing on SoCFPGA is still in a weird state. > > +}; > > + > > +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 node, bank = 0; > > + const char *name; > > + > > + /* 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; > > + } > > + > > + name = fdt_get_name(blob, dev->of_offset, NULL); > > + > > + for (node = fdt_first_subnode(blob, dev->of_offset); > > + node > 0; > > + node = fdt_next_subnode(blob, node)) { > > + int ret; > > + > > + if (!fdtdec_get_bool(blob, node, "gpio-controller")) > > + continue; > > + > > + plat = NULL; > > + plat = calloc(1, sizeof(*plat)); > > I suppose this should use devm_alloc() now. Is that even in u-boot/master ? Also, I'm not sure it's a good idea to put this in if I use this driver in SPL. > > + if (!plat) > > + return -ENOMEM; > > + > > + plat->base = base; > > + plat->bank = bank; > > + plat->pins = fdtdec_get_int(blob, node, "snps,nr-gpios", > > 0); + snprintf(plat->name, sizeof(plat->name) - 1, > > "%s-bank%i-", + name, bank); > > Why such a long name? That's going to be a pain to type in the 'gpio' > command. Do you have a suggestion please ? Also, I can as well use "gpio <operation> N" , where N is a number. > > + > > + ret = device_bind(dev, dev->driver, plat->name, > > + plat, -1, &subdev); > > + if (ret) > > + return ret; > > + > > + subdev->of_offset = node; > > + bank++; > > + } > > + > > + > > + return 0; > > +} > > + > > +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
Hi Marek, On 2 August 2015 at 16:19, Marek Vasut <marex@denx.de> wrote: > On Sunday, August 02, 2015 at 11:28:13 PM, Simon Glass wrote: >> Hi Marek, > > Hi, > >> On 27 July 2015 at 14:44, 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> > > [...] > >> > +#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 >> >> Should use C structure, right? > > My understanding is that we no longer want C structs . Tom ? > > [...] > >> > +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, >> >> Do you want to implement .function? > > No, the pinmuxing on SoCFPGA is still in a weird state. > >> > +}; >> > + >> > +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 node, bank = 0; >> > + const char *name; >> > + >> > + /* 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; >> > + } >> > + >> > + name = fdt_get_name(blob, dev->of_offset, NULL); >> > + >> > + for (node = fdt_first_subnode(blob, dev->of_offset); >> > + node > 0; >> > + node = fdt_next_subnode(blob, node)) { >> > + int ret; >> > + >> > + if (!fdtdec_get_bool(blob, node, "gpio-controller")) >> > + continue; >> > + >> > + plat = NULL; >> > + plat = calloc(1, sizeof(*plat)); >> >> I suppose this should use devm_alloc() now. > > Is that even in u-boot/master ? Also, I'm not sure it's a good idea to put > this in if I use this driver in SPL. Yes it's very new. Only in dm/master. It's only compiled in when enabled, so you can still use it in SPL. Up to you. At some point I think we should convert all driver allocs to use devm so that we know what allocation is going on. > >> > + if (!plat) >> > + return -ENOMEM; >> > + >> > + plat->base = base; >> > + plat->bank = bank; >> > + plat->pins = fdtdec_get_int(blob, node, "snps,nr-gpios", >> > 0); + snprintf(plat->name, sizeof(plat->name) - 1, >> > "%s-bank%i-", + name, bank); >> >> Why such a long name? That's going to be a pain to type in the 'gpio' >> command. > > Do you have a suggestion please ? A, B, C is good if you have <=26 banks. Tegra does that. Exynos does PA0, PA1, PB0, PC0, PC1, etc. > > Also, I can as well use "gpio <operation> N" , where N is a number. > >> > + >> > + ret = device_bind(dev, dev->driver, plat->name, >> > + plat, -1, &subdev); >> > + if (ret) >> > + return ret; >> > + >> > + subdev->of_offset = node; >> > + bank++; >> > + } >> > + >> > + >> > + return 0; >> > +} >> > + >> > +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 Regards, Simon
On Monday, August 03, 2015 at 01:38:28 AM, Simon Glass wrote: > Hi Marek, Hi Simon, [...] > >> > + if (!fdtdec_get_bool(blob, node, "gpio-controller")) > >> > + continue; > >> > + > >> > + plat = NULL; > >> > + plat = calloc(1, sizeof(*plat)); > >> > >> I suppose this should use devm_alloc() now. > > > > Is that even in u-boot/master ? Also, I'm not sure it's a good idea to > > put this in if I use this driver in SPL. > > Yes it's very new. Only in dm/master. > > It's only compiled in when enabled, so you can still use it in SPL. > > Up to you. At some point I think we should convert all driver allocs > to use devm so that we know what allocation is going on. When is this landing in master ? I don't mind either way, but I can do a linting pass on the socfpga when things settle down too. > >> > + if (!plat) > >> > + return -ENOMEM; > >> > + > >> > + plat->base = base; > >> > + plat->bank = bank; > >> > + plat->pins = fdtdec_get_int(blob, node, > >> > "snps,nr-gpios", 0); + snprintf(plat->name, > >> > sizeof(plat->name) - 1, "%s-bank%i-", + name, > >> > bank); > >> > >> Why such a long name? That's going to be a pain to type in the 'gpio' > >> command. > > > > Do you have a suggestion please ? > > A, B, C is good if you have <=26 banks. Tegra does that. > > Exynos does PA0, PA1, PB0, PC0, PC1, etc. How do I identify which one is PA0/PA1/PA2 , shall I perform some transformation on the register address of the GPIO block or example ? But how can I assure that if the next SoCFPGA has these addresses completely different, these GPIO numbers will be stable ? Isn't it better to be explicit about the GPIO block ID then ? Also, remember that I have an FPGA in the same package, which has a lot of I/O. > > Also, I can as well use "gpio <operation> N" , where N is a number. What about this ? How does indexing the GPIOs with plain number fit into the picture please ? [...]
Hi Marek, On 2 August 2015 at 18:16, Marek Vasut <marex@denx.de> wrote: > On Monday, August 03, 2015 at 01:38:28 AM, Simon Glass wrote: >> Hi Marek, > > Hi Simon, > > [...] > >> >> > + if (!fdtdec_get_bool(blob, node, "gpio-controller")) >> >> > + continue; >> >> > + >> >> > + plat = NULL; >> >> > + plat = calloc(1, sizeof(*plat)); >> >> >> >> I suppose this should use devm_alloc() now. >> > >> > Is that even in u-boot/master ? Also, I'm not sure it's a good idea to >> > put this in if I use this driver in SPL. >> >> Yes it's very new. Only in dm/master. >> >> It's only compiled in when enabled, so you can still use it in SPL. >> >> Up to you. At some point I think we should convert all driver allocs >> to use devm so that we know what allocation is going on. > > When is this landing in master ? I don't mind either way, but I can do > a linting pass on the socfpga when things settle down too. Hopefully I'll have a pull request out on Friday. > >> >> > + if (!plat) >> >> > + return -ENOMEM; >> >> > + >> >> > + plat->base = base; >> >> > + plat->bank = bank; >> >> > + plat->pins = fdtdec_get_int(blob, node, >> >> > "snps,nr-gpios", 0); + snprintf(plat->name, >> >> > sizeof(plat->name) - 1, "%s-bank%i-", + name, >> >> > bank); >> >> >> >> Why such a long name? That's going to be a pain to type in the 'gpio' >> >> command. >> > >> > Do you have a suggestion please ? >> >> A, B, C is good if you have <=26 banks. Tegra does that. >> >> Exynos does PA0, PA1, PB0, PC0, PC1, etc. > > How do I identify which one is PA0/PA1/PA2 , shall I perform some transformation > on the register address of the GPIO block or example ? But how can I assure that > if the next SoCFPGA has these addresses completely different, these GPIO numbers > will be stable ? Isn't it better to be explicit about the GPIO block ID then ? It's up to you. Normally each bank has a name and the datasheet specifies it. In your case if not you could think about a naming scheme. > > Also, remember that I have an FPGA in the same package, which has a lot of I/O. > >> > Also, I can as well use "gpio <operation> N" , where N is a number. > > What about this ? How does indexing the GPIOs with plain number fit into the > picture please ? Driver model GPIO handles this automatically. If you can add different numbers of GPIO blocks you might consider adding them as different GPIO banks with their own names. The global number depends on the probe order which depends on the bind order (i.e. device tree node order). But names are safer than numbers - a small change can change all the numbers. There is a function to get the global number given a GPIO device and offset. Regards, Simon
On Wednesday, August 05, 2015 at 04:39:33 PM, Simon Glass wrote: > Hi Marek, Hi Simon, > On 2 August 2015 at 18:16, Marek Vasut <marex@denx.de> wrote: > > On Monday, August 03, 2015 at 01:38:28 AM, Simon Glass wrote: > >> Hi Marek, > > > > Hi Simon, > > > > [...] > > > >> >> > + if (!fdtdec_get_bool(blob, node, > >> >> > "gpio-controller")) + continue; > >> >> > + > >> >> > + plat = NULL; > >> >> > + plat = calloc(1, sizeof(*plat)); > >> >> > >> >> I suppose this should use devm_alloc() now. > >> > > >> > Is that even in u-boot/master ? Also, I'm not sure it's a good idea to > >> > put this in if I use this driver in SPL. > >> > >> Yes it's very new. Only in dm/master. > >> > >> It's only compiled in when enabled, so you can still use it in SPL. > >> > >> Up to you. At some point I think we should convert all driver allocs > >> to use devm so that we know what allocation is going on. > > > > When is this landing in master ? I don't mind either way, but I can do > > a linting pass on the socfpga when things settle down too. > > Hopefully I'll have a pull request out on Friday. Cool! > >> >> > + if (!plat) > >> >> > + return -ENOMEM; > >> >> > + > >> >> > + plat->base = base; > >> >> > + plat->bank = bank; > >> >> > + plat->pins = fdtdec_get_int(blob, node, > >> >> > "snps,nr-gpios", 0); + snprintf(plat->name, > >> >> > sizeof(plat->name) - 1, "%s-bank%i-", + > >> >> > name, bank); > >> >> > >> >> Why such a long name? That's going to be a pain to type in the 'gpio' > >> >> command. > >> > > >> > Do you have a suggestion please ? > >> > >> A, B, C is good if you have <=26 banks. Tegra does that. > >> > >> Exynos does PA0, PA1, PB0, PC0, PC1, etc. > > > > How do I identify which one is PA0/PA1/PA2 , shall I perform some > > transformation on the register address of the GPIO block or example ? > > But how can I assure that if the next SoCFPGA has these addresses > > completely different, these GPIO numbers will be stable ? Isn't it > > better to be explicit about the GPIO block ID then ? > > It's up to you. Normally each bank has a name and the datasheet > specifies it. In your case if not you could think about a naming > scheme. Can you please take a look into arch/arm/dts/socfpga.dtsi ? The system has three GPIO controllers (look for gpio0, gpio1, gpio2) and each of these controllers has one bank (porta, portb, portc) . I can name my gpios portxN , where x is either of a,b,c and N is the GPIO number. The problem is, I cannot determine in dwapb_gpio_bind() which one is "porta", "portb" and "portc" because all I have is the physical addess of the GPIO controller and the index of the bank in the namespace of that controller. Sure, I can do some sort of global counting in the driver, but I would like to avoid that sort of thing. I can also add some kind of ad-hoc DT prop, but that's also not a good idea I think. Do you have any suggestion for me please ? > > Also, remember that I have an FPGA in the same package, which has a lot > > of I/O. > > > >> > Also, I can as well use "gpio <operation> N" , where N is a number. > > > > What about this ? How does indexing the GPIOs with plain number fit into > > the picture please ? > > Driver model GPIO handles this automatically. If you can add different > numbers of GPIO blocks you might consider adding them as different > GPIO banks with their own names. The global number depends on the > probe order which depends on the bind order (i.e. device tree node > order). But names are safer than numbers - a small change can change > all the numbers. There is a function to get the global number given a > GPIO device and offset. I see, thanks for clarifying!
Hi Marek, On 5 August 2015 at 19:49, Marek Vasut <marex@denx.de> wrote: > On Wednesday, August 05, 2015 at 04:39:33 PM, Simon Glass wrote: >> Hi Marek, > > Hi Simon, > >> On 2 August 2015 at 18:16, Marek Vasut <marex@denx.de> wrote: >> > On Monday, August 03, 2015 at 01:38:28 AM, Simon Glass wrote: >> >> Hi Marek, >> > >> > Hi Simon, >> > >> > [...] >> > >> >> >> > + if (!fdtdec_get_bool(blob, node, >> >> >> > "gpio-controller")) + continue; >> >> >> > + >> >> >> > + plat = NULL; >> >> >> > + plat = calloc(1, sizeof(*plat)); >> >> >> >> >> >> I suppose this should use devm_alloc() now. >> >> > >> >> > Is that even in u-boot/master ? Also, I'm not sure it's a good idea to >> >> > put this in if I use this driver in SPL. >> >> >> >> Yes it's very new. Only in dm/master. >> >> >> >> It's only compiled in when enabled, so you can still use it in SPL. >> >> >> >> Up to you. At some point I think we should convert all driver allocs >> >> to use devm so that we know what allocation is going on. >> > >> > When is this landing in master ? I don't mind either way, but I can do >> > a linting pass on the socfpga when things settle down too. >> >> Hopefully I'll have a pull request out on Friday. > > Cool! > >> >> >> > + if (!plat) >> >> >> > + return -ENOMEM; >> >> >> > + >> >> >> > + plat->base = base; >> >> >> > + plat->bank = bank; >> >> >> > + plat->pins = fdtdec_get_int(blob, node, >> >> >> > "snps,nr-gpios", 0); + snprintf(plat->name, >> >> >> > sizeof(plat->name) - 1, "%s-bank%i-", + >> >> >> > name, bank); >> >> >> >> >> >> Why such a long name? That's going to be a pain to type in the 'gpio' >> >> >> command. >> >> > >> >> > Do you have a suggestion please ? >> >> >> >> A, B, C is good if you have <=26 banks. Tegra does that. >> >> >> >> Exynos does PA0, PA1, PB0, PC0, PC1, etc. >> > >> > How do I identify which one is PA0/PA1/PA2 , shall I perform some >> > transformation on the register address of the GPIO block or example ? >> > But how can I assure that if the next SoCFPGA has these addresses >> > completely different, these GPIO numbers will be stable ? Isn't it >> > better to be explicit about the GPIO block ID then ? >> >> It's up to you. Normally each bank has a name and the datasheet >> specifies it. In your case if not you could think about a naming >> scheme. > > Can you please take a look into arch/arm/dts/socfpga.dtsi ? > The system has three GPIO controllers (look for gpio0, gpio1, gpio2) > and each of these controllers has one bank (porta, portb, portc) . > > I can name my gpios portxN , where x is either of a,b,c and N is the > GPIO number. The problem is, I cannot determine in dwapb_gpio_bind() > which one is "porta", "portb" and "portc" because all I have is the > physical addess of the GPIO controller and the index of the bank in > the namespace of that controller. > > Sure, I can do some sort of global counting in the driver, but I would > like to avoid that sort of thing. I can also add some kind of ad-hoc DT > prop, but that's also not a good idea I think. Do you have any suggestion > for me please ? One option is to use the device tree node name but it isn't very friendly - gpio0@xxxxx. You could perhaps add a new property like 'bank-name'? > >> > Also, remember that I have an FPGA in the same package, which has a lot >> > of I/O. >> > >> >> > Also, I can as well use "gpio <operation> N" , where N is a number. >> > >> > What about this ? How does indexing the GPIOs with plain number fit into >> > the picture please ? >> >> Driver model GPIO handles this automatically. If you can add different >> numbers of GPIO blocks you might consider adding them as different >> GPIO banks with their own names. The global number depends on the >> probe order which depends on the bind order (i.e. device tree node >> order). But names are safer than numbers - a small change can change >> all the numbers. There is a function to get the global number given a >> GPIO device and offset. > > I see, thanks for clarifying! Regards, Simon
On Friday, August 07, 2015 at 09:13:45 PM, Simon Glass wrote: > Hi Marek, Hi! > On 5 August 2015 at 19:49, Marek Vasut <marex@denx.de> wrote: > > On Wednesday, August 05, 2015 at 04:39:33 PM, Simon Glass wrote: > >> Hi Marek, > > > > Hi Simon, [...] > >> It's up to you. Normally each bank has a name and the datasheet > >> specifies it. In your case if not you could think about a naming > >> scheme. > > > > Can you please take a look into arch/arm/dts/socfpga.dtsi ? > > The system has three GPIO controllers (look for gpio0, gpio1, gpio2) > > and each of these controllers has one bank (porta, portb, portc) . > > > > I can name my gpios portxN , where x is either of a,b,c and N is the > > GPIO number. The problem is, I cannot determine in dwapb_gpio_bind() > > which one is "porta", "portb" and "portc" because all I have is the > > physical addess of the GPIO controller and the index of the bank in > > the namespace of that controller. > > > > Sure, I can do some sort of global counting in the driver, but I would > > like to avoid that sort of thing. I can also add some kind of ad-hoc DT > > prop, but that's also not a good idea I think. Do you have any suggestion > > for me please ? > > One option is to use the device tree node name but it isn't very > friendly - gpio0@xxxxx. That's what I do now pretty much. > You could perhaps add a new property like 'bank-name'? Do we want to add ad-hoc DT nodes which are a) Not describing hardware b) Not part of the official DT bindings for that platform ? Is that really a way to go ? [...] Best regards, Marek Vasut
Hi Marek, On 7 August 2015 at 14:35, Marek Vasut <marex@denx.de> wrote: > On Friday, August 07, 2015 at 09:13:45 PM, Simon Glass wrote: >> Hi Marek, > > Hi! > >> On 5 August 2015 at 19:49, Marek Vasut <marex@denx.de> wrote: >> > On Wednesday, August 05, 2015 at 04:39:33 PM, Simon Glass wrote: >> >> Hi Marek, >> > >> > Hi Simon, > > [...] > >> >> It's up to you. Normally each bank has a name and the datasheet >> >> specifies it. In your case if not you could think about a naming >> >> scheme. >> > >> > Can you please take a look into arch/arm/dts/socfpga.dtsi ? >> > The system has three GPIO controllers (look for gpio0, gpio1, gpio2) >> > and each of these controllers has one bank (porta, portb, portc) . >> > >> > I can name my gpios portxN , where x is either of a,b,c and N is the >> > GPIO number. The problem is, I cannot determine in dwapb_gpio_bind() >> > which one is "porta", "portb" and "portc" because all I have is the >> > physical addess of the GPIO controller and the index of the bank in >> > the namespace of that controller. >> > >> > Sure, I can do some sort of global counting in the driver, but I would >> > like to avoid that sort of thing. I can also add some kind of ad-hoc DT >> > prop, but that's also not a good idea I think. Do you have any suggestion >> > for me please ? >> >> One option is to use the device tree node name but it isn't very >> friendly - gpio0@xxxxx. > > That's what I do now pretty much. > >> You could perhaps add a new property like 'bank-name'? > > Do we want to add ad-hoc DT nodes which are > a) Not describing hardware > b) Not part of the official DT bindings for that platform > ? > > Is that really a way to go ? > > [...] > It needs to be part of the official binding. Naming the hardware is part of the hardware definition - see for example the regulator-name property for regulators. Another option is to use an alias: aliases { gpio0 = &gpio_0; gpio1 = &gpio_1; gpio2 = &gpio_2; } Then you can turn gpio0 into bank A, gpio1 into bank B, etc. Regards, Simon
On Friday, August 07, 2015 at 10:37:54 PM, Simon Glass wrote: > Hi Marek, Hi Simon, > On 7 August 2015 at 14:35, Marek Vasut <marex@denx.de> wrote: > > On Friday, August 07, 2015 at 09:13:45 PM, Simon Glass wrote: > >> Hi Marek, > > > > Hi! > > > >> On 5 August 2015 at 19:49, Marek Vasut <marex@denx.de> wrote: > >> > On Wednesday, August 05, 2015 at 04:39:33 PM, Simon Glass wrote: > >> >> Hi Marek, > >> > > >> > Hi Simon, > > > > [...] > > > >> >> It's up to you. Normally each bank has a name and the datasheet > >> >> specifies it. In your case if not you could think about a naming > >> >> scheme. > >> > > >> > Can you please take a look into arch/arm/dts/socfpga.dtsi ? > >> > The system has three GPIO controllers (look for gpio0, gpio1, gpio2) > >> > and each of these controllers has one bank (porta, portb, portc) . > >> > > >> > I can name my gpios portxN , where x is either of a,b,c and N is the > >> > GPIO number. The problem is, I cannot determine in dwapb_gpio_bind() > >> > which one is "porta", "portb" and "portc" because all I have is the > >> > physical addess of the GPIO controller and the index of the bank in > >> > the namespace of that controller. > >> > > >> > Sure, I can do some sort of global counting in the driver, but I would > >> > like to avoid that sort of thing. I can also add some kind of ad-hoc > >> > DT prop, but that's also not a good idea I think. Do you have any > >> > suggestion for me please ? > >> > >> One option is to use the device tree node name but it isn't very > >> friendly - gpio0@xxxxx. > > > > That's what I do now pretty much. > > > >> You could perhaps add a new property like 'bank-name'? > > > > Do we want to add ad-hoc DT nodes which are > > a) Not describing hardware > > b) Not part of the official DT bindings for that platform > > ? > > > > Is that really a way to go ? > > > > [...] > > It needs to be part of the official binding. Naming the hardware is > part of the hardware definition - see for example the regulator-name > property for regulators. So what do you think about introducing a 'bank-name' property then ? I think this might work just fine ? > Another option is to use an alias: > > aliases { > gpio0 = &gpio_0; > gpio1 = &gpio_1; > gpio2 = &gpio_2; > } > > Then you can turn gpio0 into bank A, gpio1 into bank B, etc. Is there a function which maps the udevice->dev->of_offset into an alias's seq ID ? Best regards, Marek Vasut
Hi Marek, On 10 August 2015 at 09:01, Marek Vasut <marex@denx.de> wrote: > > On Friday, August 07, 2015 at 10:37:54 PM, Simon Glass wrote: > > Hi Marek, > > Hi Simon, > > > On 7 August 2015 at 14:35, Marek Vasut <marex@denx.de> wrote: > > > On Friday, August 07, 2015 at 09:13:45 PM, Simon Glass wrote: > > >> Hi Marek, > > > > > > Hi! > > > > > >> On 5 August 2015 at 19:49, Marek Vasut <marex@denx.de> wrote: > > >> > On Wednesday, August 05, 2015 at 04:39:33 PM, Simon Glass wrote: > > >> >> Hi Marek, > > >> > > > >> > Hi Simon, > > > > > > [...] > > > > > >> >> It's up to you. Normally each bank has a name and the datasheet > > >> >> specifies it. In your case if not you could think about a naming > > >> >> scheme. > > >> > > > >> > Can you please take a look into arch/arm/dts/socfpga.dtsi ? > > >> > The system has three GPIO controllers (look for gpio0, gpio1, gpio2) > > >> > and each of these controllers has one bank (porta, portb, portc) . > > >> > > > >> > I can name my gpios portxN , where x is either of a,b,c and N is the > > >> > GPIO number. The problem is, I cannot determine in dwapb_gpio_bind() > > >> > which one is "porta", "portb" and "portc" because all I have is the > > >> > physical addess of the GPIO controller and the index of the bank in > > >> > the namespace of that controller. > > >> > > > >> > Sure, I can do some sort of global counting in the driver, but I would > > >> > like to avoid that sort of thing. I can also add some kind of ad-hoc > > >> > DT prop, but that's also not a good idea I think. Do you have any > > >> > suggestion for me please ? > > >> > > >> One option is to use the device tree node name but it isn't very > > >> friendly - gpio0@xxxxx. > > > > > > That's what I do now pretty much. > > > > > >> You could perhaps add a new property like 'bank-name'? > > > > > > Do we want to add ad-hoc DT nodes which are > > > a) Not describing hardware > > > b) Not part of the official DT bindings for that platform > > > ? > > > > > > Is that really a way to go ? > > > > > > [...] > > > > It needs to be part of the official binding. Naming the hardware is > > part of the hardware definition - see for example the regulator-name > > property for regulators. > > So what do you think about introducing a 'bank-name' property then ? > I think this might work just fine ? I think it's OK - just make sure you send the GPIO binding change to Linux too. > > > Another option is to use an alias: > > > > aliases { > > gpio0 = &gpio_0; > > gpio1 = &gpio_1; > > gpio2 = &gpio_2; > > } > > > > Then you can turn gpio0 into bank A, gpio1 into bank B, etc. > > Is there a function which maps the udevice->dev->of_offset into an alias's > seq ID ? dev->seq, once it is probed. Until it is probed it doesn't have a sequence number (by definition). You can use fdtdec_get_alias_seq() but I don't recommend it. We're trying to drop fdtdec eventually. Regards, Simon
On Monday, August 10, 2015 at 05:35:25 PM, Simon Glass wrote: > Hi Marek, > > On 10 August 2015 at 09:01, Marek Vasut <marex@denx.de> wrote: > > On Friday, August 07, 2015 at 10:37:54 PM, Simon Glass wrote: > > > Hi Marek, > > > > Hi Simon, > > > > > On 7 August 2015 at 14:35, Marek Vasut <marex@denx.de> wrote: > > > > On Friday, August 07, 2015 at 09:13:45 PM, Simon Glass wrote: > > > >> Hi Marek, > > > > > > > > Hi! > > > > > > > >> On 5 August 2015 at 19:49, Marek Vasut <marex@denx.de> wrote: > > > >> > On Wednesday, August 05, 2015 at 04:39:33 PM, Simon Glass wrote: > > > >> >> Hi Marek, > > > >> > > > > >> > Hi Simon, > > > > > > > > [...] > > > > > > > >> >> It's up to you. Normally each bank has a name and the datasheet > > > >> >> specifies it. In your case if not you could think about a naming > > > >> >> scheme. > > > >> > > > > >> > Can you please take a look into arch/arm/dts/socfpga.dtsi ? > > > >> > The system has three GPIO controllers (look for gpio0, gpio1, > > > >> > gpio2) and each of these controllers has one bank (porta, portb, > > > >> > portc) . > > > >> > > > > >> > I can name my gpios portxN , where x is either of a,b,c and N is > > > >> > the GPIO number. The problem is, I cannot determine in > > > >> > dwapb_gpio_bind() which one is "porta", "portb" and "portc" > > > >> > because all I have is the physical addess of the GPIO controller > > > >> > and the index of the bank in the namespace of that controller. > > > >> > > > > >> > Sure, I can do some sort of global counting in the driver, but I > > > >> > would like to avoid that sort of thing. I can also add some kind > > > >> > of ad-hoc DT prop, but that's also not a good idea I think. Do > > > >> > you have any suggestion for me please ? > > > >> > > > >> One option is to use the device tree node name but it isn't very > > > >> friendly - gpio0@xxxxx. > > > > > > > > That's what I do now pretty much. > > > > > > > >> You could perhaps add a new property like 'bank-name'? > > > > > > > > Do we want to add ad-hoc DT nodes which are > > > > a) Not describing hardware > > > > b) Not part of the official DT bindings for that platform > > > > ? > > > > > > > > Is that really a way to go ? > > > > > > > > [...] > > > > > > It needs to be part of the official binding. Naming the hardware is > > > part of the hardware definition - see for example the regulator-name > > > property for regulators. > > > > So what do you think about introducing a 'bank-name' property then ? > > I think this might work just fine ? > > I think it's OK - just make sure you send the GPIO binding change to Linux > too. > > > > Another option is to use an alias: > > > > > > aliases { > > > > > > gpio0 = &gpio_0; > > > gpio1 = &gpio_1; > > > gpio2 = &gpio_2; > > > > > > } > > > > > > Then you can turn gpio0 into bank A, gpio1 into bank B, etc. > > > > Is there a function which maps the udevice->dev->of_offset into an > > alias's seq ID ? > > dev->seq, once it is probed. Until it is probed it doesn't have a > sequence number (by definition). > > You can use fdtdec_get_alias_seq() but I don't recommend it. We're > trying to drop fdtdec eventually. OK. I sent a V2, so that should be OK now.
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..cdb6e78 --- /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 { + char name[24]; + 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 node, bank = 0; + const char *name; + + /* 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; + } + + name = fdt_get_name(blob, dev->of_offset, NULL); + + for (node = fdt_first_subnode(blob, dev->of_offset); + node > 0; + node = fdt_next_subnode(blob, node)) { + int ret; + + 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); + snprintf(plat->name, sizeof(plat->name) - 1, "%s-bank%i-", + name, bank); + + ret = device_bind(dev, dev->driver, plat->name, + plat, -1, &subdev); + if (ret) + return ret; + + subdev->of_offset = node; + bank++; + } + + + return 0; +} + +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, +};
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/Makefile | 1 + drivers/gpio/dwapb_gpio.c | 167 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 168 insertions(+) create mode 100644 drivers/gpio/dwapb_gpio.c