[PATCH v2 1/2] drivers: gpio: add broadcom iproc gpio driver support

Rayagonda Kokatanur rayagonda.kokatanur at broadcom.com
Thu Apr 30 11:54:48 CEST 2020


On Wed, Apr 29, 2020 at 11:34 PM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Rayagonda,
>
> On Wed, 29 Apr 2020 at 00:21, Rayagonda Kokatanur
> <rayagonda.kokatanur at broadcom.com> wrote:
> >
> > Add gpio driver support for Broadcom iproc-based socs.
> >
> > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur at broadcom.com>
> > Signed-off-by: Sheetal Tigadoli <sheetal.tigadoli at broadcom.com>
> > ---
> >  drivers/gpio/Kconfig      |  10 ++
> >  drivers/gpio/Makefile     |   1 +
> >  drivers/gpio/iproc_gpio.c | 259 ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 270 insertions(+)
> >  create mode 100644 drivers/gpio/iproc_gpio.c
> >
>
> Minor nits below
>
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index 2081520f42..a04b4af1b5 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -135,6 +135,16 @@ config IMX_RGPIO2P
> >         help
> >           This driver supports i.MX7ULP Rapid GPIO2P controller.
> >
> > +config IPROC_GPIO
> > +       bool "Broadcom iProc GPIO driver(without pinconf)"
> > +       default n
> > +       help
> > +         The Broadcom iProc based SoCs- Cygnus, NS2, NS3, NSP and Stingray,
> > +         use same GPIO Controller IP hence this driver could be used for all.
>
> use the same
>
> > +
> > +         The Broadcom iProc based SoCs have multiple GPIO controllers and only
> > +         the always-ON GPIO controller (CRMU/AON) is supported by this driver.
> > +
> >  config HSDK_CREG_GPIO
> >         bool "HSDK CREG GPIO griver"
> >         depends on DM_GPIO
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > index 7638259007..5dc5849477 100644
> > --- a/drivers/gpio/Makefile
> > +++ b/drivers/gpio/Makefile
> > @@ -19,6 +19,7 @@ obj-$(CONFIG_CORTINA_GPIO)      += cortina_gpio.o
> >  obj-$(CONFIG_INTEL_GPIO)       += intel_gpio.o
> >  obj-$(CONFIG_INTEL_ICH6_GPIO)  += intel_ich6_gpio.o
> >  obj-$(CONFIG_INTEL_BROADWELL_GPIO)     += intel_broadwell_gpio.o
> > +obj-$(CONFIG_IPROC_GPIO)       += iproc_gpio.o
> >  obj-$(CONFIG_KIRKWOOD_GPIO)    += kw_gpio.o
> >  obj-$(CONFIG_KONA_GPIO)        += kona_gpio.o
> >  obj-$(CONFIG_MARVELL_GPIO)     += mvgpio.o
> > diff --git a/drivers/gpio/iproc_gpio.c b/drivers/gpio/iproc_gpio.c
> > new file mode 100644
> > index 0000000000..037f1a7de5
> > --- /dev/null
> > +++ b/drivers/gpio/iproc_gpio.c
> > @@ -0,0 +1,259 @@
> > +// SPDX-License-Identifier:      GPL-2.0+
> > +/*
> > + * Copyright (C) 2020 Broadcom
> > + */
> > +
> > +#include <asm/gpio.h>
> > +#include <asm/io.h>
> > +#include <common.h>
>
> common.h always goes first
> > +#include <dm.h>
>
> put asm ones here
>
> > +#include <dm/device_compat.h>
> > +#include <dm/devres.h>
> > +#include <dm/pinctrl.h>
> > +#include <errno.h>
>
> put that just below dm.h
>
> See Include files here: https://www.denx.de/wiki/U-Boot/CodingStyle
>
> > +
> > +/*
> > + * There are five GPIO bank register. Each bank can configure max of 32 gpios.
> > + * BANK0 - gpios 0 to 31
> > + * BANK1 - gpios 32 to 63
> > + * BANK2 - gpios 64 to 95
> > + * BANK3 - gpios 96 to 127
> > + * BANK4 - gpios 128 to 150
> > + *
> > + * Offset difference between consecutive bank register is 0x200
> > + */
> > +#define IPROC_GPIO_PER_BANK         32
> > +#define IPROC_GPIO_SHIFT(n)         ((n) % IPROC_GPIO_PER_BANK)
> > +#define IPROC_GPIO_BANK_OFFSET(n)   (0x200 * ((n) / IPROC_GPIO_PER_BANK))
>
> Move this one to a function?

I will rewritten the macro, please have a look and let me know.
>
> > +#define IPROC_GPIO_REG(pin, reg)    (IPROC_GPIO_BANK_OFFSET(pin) + (reg))
> > +
> > +#define IPROC_GPIO_DATA_IN_OFFSET   0x00
> > +#define IPROC_GPIO_DATA_OUT_OFFSET  0x04
> > +#define IPROC_GPIO_OUT_EN_OFFSET    0x08
> > +
> > +struct iproc_gpio_pctrl_map {
>
> Needs struct comment talking about members
>
> > +       u32 gpio_pin;
> > +       u32 pctrl_pin;
> > +       u32 npins;
> > +       struct list_head node;
> > +};
> > +
> > +struct iproc_gpio_platdata {
>
> Needs struct comment talking about members
>
> > +       struct udevice *pinctrl_dev;
> > +       struct list_head gpiomap;
> > +       /* register base for this bank */
> > +       void __iomem *base;
> > +       char *name;
> > +       u32 ngpios;
> > +};
> > +
> > +/**
> > + *  iproc_gpio_set_bit - set or clear one bit (corresponding to the GPIO pin)
> > + *  in a iproc GPIO register
> > + *
> > + *  @iproc_gpio: Iproc GPIO device
> > + *  @reg: register offset
> > + *  @gpio: GPIO pin
> > + *  @set: set or clear
> > + */
> > +static inline void iproc_gpio_set_bit(struct iproc_gpio_platdata *plat,
> > +                                     u32 reg,
> > +                                     u32 gpio, bool set)
> > +{
> > +       u32 offset = IPROC_GPIO_REG(gpio, reg);
> > +       u32 shift = IPROC_GPIO_SHIFT(gpio);
> > +       u32 val;
> > +
> > +       val = readl(plat->base + offset);
>
> You can use clrsetbits_le32()
>
> > +       if (set)
> > +               val |= BIT(shift);
> > +       else
> > +               val &= ~BIT(shift);
> > +       writel(val, plat->base + offset);
> > +}
> > +
> > +static inline bool iproc_gpio_get_bit(struct iproc_gpio_platdata *plat,
> > +                                     u32 reg,
> > +                                     u32 gpio)
>
> can fit on two lines? Do you need the inline?
>
> > +{
> > +       u32 offset = IPROC_GPIO_REG(gpio, reg);
> > +       u32 shift = IPROC_GPIO_SHIFT(gpio);
> > +
> > +       return !!(readl(plat->base + offset) & BIT(shift));
>
> Since you return bool I suspect you don't need the !! ?
>
> > +}
> > +
> > +static u32 iproc_get_pctrl_from_gpio(struct iproc_gpio_platdata *plat, u32 gpio)
> > +{
> > +       struct iproc_gpio_pctrl_map *range = NULL;
> > +       struct list_head *pos, *tmp;
> > +       u32 ret = 0;
> > +
> > +       list_for_each_safe(pos, tmp, &plat->gpiomap) {
> > +               range = list_entry(pos, struct iproc_gpio_pctrl_map, node);
> > +               if (gpio == range->gpio_pin ||
> > +                   (gpio < (range->gpio_pin + range->npins))) {
>
> drop extra ()
>
> > +                       ret = (range->pctrl_pin + (gpio - range->gpio_pin));
>
> drop extra ()
>
> > +                       break;
> > +               }
> > +       }
> > +
> > +       return ret;
> > +}
> > +
> > +static int iproc_get_gpio_pctrl_mapping(struct udevice *dev)
> > +{
> > +       struct iproc_gpio_platdata *plat = dev_get_platdata(dev);
> > +       struct iproc_gpio_pctrl_map *range = NULL;
> > +       struct ofnode_phandle_args args;
> > +       int node = dev_of_offset(dev);
> > +       int index = 0, ret;
> > +
> > +       for (;; index++) {
> > +               ret = dev_read_phandle_with_args(dev, "gpio-ranges",
> > +                                                NULL, 3, index, &args);
> > +               if (ret)
> > +                       break;
> > +
> > +               range = devm_kzalloc(dev, sizeof(*range), GFP_KERNEL);
> > +               if (!range)
> > +                       return -ENOMEM;
> > +
> > +               range->gpio_pin = args.args[0];
> > +               range->pctrl_pin = args.args[1];
> > +               range->npins = args.args[2];
> > +               list_add_tail(&range->node, &plat->gpiomap);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int iproc_gpio_request(struct udevice *dev, u32 gpio, const char *label)
> > +{
> > +       struct iproc_gpio_platdata *plat = dev_get_platdata(dev);
> > +       u32 pctrl;
> > +
> > +       if (!plat->pinctrl_dev)
> > +               return 0;
> > +
> > +       pctrl = iproc_get_pctrl_from_gpio(plat, gpio);
>
> newline before return (fix below too)
>
> > +       return pinctrl_request(plat->pinctrl_dev, pctrl, 0);
> > +}
> > +
> > +static int iproc_gpio_direction_input(struct udevice *dev, u32 gpio)
> > +{
> > +       struct iproc_gpio_platdata *plat = dev_get_platdata(dev);
> > +
> > +       iproc_gpio_set_bit(plat, IPROC_GPIO_OUT_EN_OFFSET, gpio, false);
> > +       dev_dbg(dev, "gpio:%u set input\n", gpio);
> > +       return 0;
> > +}
> > +
> > +static int iproc_gpio_direction_output(struct udevice *dev, u32 gpio,
> > +                                      int value)
> > +{
> > +       struct iproc_gpio_platdata *plat = dev_get_platdata(dev);
> > +
> > +       iproc_gpio_set_bit(plat, IPROC_GPIO_OUT_EN_OFFSET, gpio, true);
> > +       iproc_gpio_set_bit(plat, IPROC_GPIO_DATA_OUT_OFFSET, gpio, !!value);
> > +       dev_dbg(dev, "gpio:%u set output, value:%d\n", gpio, value);
> > +
> > +       return 0;
> > +}
> > +
> > +static int iproc_gpio_get_value(struct udevice *dev, u32 gpio)
> > +{
> > +       struct iproc_gpio_platdata *plat = dev_get_platdata(dev);
> > +       int value;
> > +
> > +       value = iproc_gpio_get_bit(plat, IPROC_GPIO_DATA_IN_OFFSET, gpio);
> > +       dev_dbg(dev, "gpio:%u get, value:%d\n", gpio, value);
> > +
> > +       return value;
> > +}
> > +
> > +static int iproc_gpio_set_value(struct udevice *dev, u32 gpio, int value)
> > +{
> > +       struct iproc_gpio_platdata *plat = dev_get_platdata(dev);
> > +
> > +       if (iproc_gpio_get_bit(plat, IPROC_GPIO_OUT_EN_OFFSET, gpio))
> > +               iproc_gpio_set_bit(plat, IPROC_GPIO_DATA_OUT_OFFSET, gpio,
> > +                                  !!value);
> > +
> > +       dev_dbg(dev, "gpio:%u set, value:%d\n", gpio, value);
> > +       return 0;
> > +}
> > +
> > +static int iproc_gpio_get_function(struct udevice *dev, u32 gpio)
> > +{
> > +       struct iproc_gpio_platdata *plat = dev_get_platdata(dev);
> > +
> > +       if (iproc_gpio_get_bit(plat, IPROC_GPIO_OUT_EN_OFFSET, gpio))
> > +               return GPIOF_OUTPUT;
> > +       else
> > +               return GPIOF_INPUT;
>
> What about pins that are not used as GPIOs?

Each bit in the IPROC_GPIO_OUT_EN_OFFSET register specifies one gpio line.
And each bit represents whether gpio is in INPUT or OUTPUT.
0 - means INPUT
1 - means OUTPUT.

It doesn't support other modes like GPIO UNUSED or any other function.

I will fix all review comments and push patch v3.
>
> > +}
> > +
> > +static int iproc_gpio_ofdata_to_platdata(struct udevice *dev)
> > +{
> > +       struct iproc_gpio_platdata *plat = dev_get_platdata(dev);
> > +       struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> > +       int ret;
> > +       char name[10];
> > +
> > +       plat->base = dev_read_addr_ptr(dev);
> > +       if (!plat->base) {
> > +               dev_err(dev, "%s: Failed to get base address\n", __func__);
>
> These should not happen, so could be debug()?
>
> > +               return -EINVAL;
> > +       }
> > +
> > +       ret = dev_read_u32(dev, "ngpios", &plat->ngpios);
> > +       if (ret < 0) {
> > +               dev_err(dev, "%s: Failed to get ngpios\n", __func__);
> > +               return ret;
> > +       }
> > +
> > +       uclass_get_device_by_phandle(UCLASS_PINCTRL, dev, "gpio-ranges",
> > +                                    &plat->pinctrl_dev);
> > +
> > +       INIT_LIST_HEAD(&plat->gpiomap);
> > +       ret = iproc_get_gpio_pctrl_mapping(dev);
> > +       if (ret) {
> > +               dev_err(dev, "%s: Failed to get gpio to pctrl map ret(%d)\n",
> > +                       __func__, ret);
> > +               return ret;
> > +       }
> > +
> > +       snprintf(name, sizeof(name), "GPIO%d", dev->req_seq);
> > +       plat->name = strdup(name);
> > +
> > +       uc_priv->gpio_count = plat->ngpio`s;
> > +       uc_priv->bank_name = plat->name;
> > +
> > +       dev_info(dev, ":bank name(%s) base %p, #gpios %d\n",
> > +                plat->name, plat->base, plat->ngpios);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct dm_gpio_ops iproc_gpio_ops = {
> > +       .request                = iproc_gpio_request,
> > +       .direction_input        = iproc_gpio_direction_input,
> > +       .direction_output       = iproc_gpio_direction_output,
> > +       .get_value              = iproc_gpio_get_value,
> > +       .set_value              = iproc_gpio_set_value,
> > +       .get_function           = iproc_gpio_get_function,
> > +};
> > +
> > +static const struct udevice_id iproc_gpio_ids[] = {
> > +       { .compatible = "brcm,iproc-gpio" },
> > +       { }
> > +};
> > +
> > +U_BOOT_DRIVER(iproc_gpio) = {
> > +       .name                   = "iproc_gpio",
> > +       .id                     = UCLASS_GPIO,
> > +       .of_match               = iproc_gpio_ids,
> > +       .ops                    = &iproc_gpio_ops,
> > +       .ofdata_to_platdata     = iproc_gpio_ofdata_to_platdata,
> > +       .platdata_auto_alloc_size       = sizeof(struct iproc_gpio_platdata),
> > +};
> > --
> > 2.17.1
> >
>
> Regards,
> Simon


More information about the U-Boot mailing list