| Message ID | 1439220630-5994-1-git-send-email-marex@denx.de |
|---|---|
| State | Accepted |
| Delegated to: | Marek Vasut |
| Headers | show |
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
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
+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
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 [...]
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, +};
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.