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

Hi Rayagonda,

On Mon, 4 May 2020 at 10:00, Rayagonda Kokatanur rayagonda.kokatanur@broadcom.com wrote:

Add gpio driver support for Broadcom iproc-based socs.

Signed-off-by: Rayagonda Kokatanur rayagonda.kokatanur@broadcom.com Signed-off-by: Sheetal Tigadoli sheetal.tigadoli@broadcom.com

---

drivers/gpio/Kconfig | 11 ++ drivers/gpio/Makefile | 1 + drivers/gpio/iproc_gpio.c | 275 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 287 insertions(+) create mode 100644 drivers/gpio/iproc_gpio.c

Reviewed-by: Simon Glass sjg@chromium.org

nits and suggestions below

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 2081520f42..57d2cd2e75 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -135,6 +135,17 @@ 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 the same GPIO Controller IP hence this driver could be used
  

•     for all.
  

•     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..f75831d6e7 --- /dev/null +++ b/drivers/gpio/iproc_gpio.c @@ -0,0 +1,275 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright (C) 2020 Broadcom
• */

+#include <common.h> +#include <errno.h> +#include <asm/gpio.h> +#include <asm/io.h> +#include <dm.h> +#include <dm/device_compat.h> +#include <dm/devres.h> +#include <dm/pinctrl.h>

Do you need that one?

+/*

• • 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 NGPIO_PER_BANK 32 +#define GPIO_BANK_SIZE 0x200 +#define GPIO_BANK(pin) ((pin) / NGPIO_PER_BANK) +#define IPROC_GPIO_SHIFT(pin) ((pin) % NGPIO_PER_BANK) +#define IPROC_GPIO_REG(pin, reg) (GPIO_BANK_SIZE * GPIO_BANK(pin) + (reg))

+/* device register offset */ +#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 - mapping between gpio and pinctrl specfied

• •                          using gpio-ranges parameter in dt.
    

• • @gpio_pin: start of gpio number in gpio-ranges
• • @pctrl_pin: start of pinctrl number in gpio-ranges
• • @npins: total number of pins in gpio-ranges
• • @node: list node
• */

+struct iproc_gpio_pctrl_map {

•   u32 gpio_pin;
  

•   u32 pctrl_pin;
  

•   u32 npins;
  

•   struct list_head node;
  

+};

+/**

• • struct iproc_gpio_pctrl_map - gpio device instance
• • @pinctrl_dev:pointer to pinctrl device
• • @gpiomap: list node having mapping between gpio and pinctrl
• • @base: I/O register base address of gpio device
• • @name: gpio device name, ex GPIO0, GPIO1
• • @ngpios: total number of gpios
• */

+struct iproc_gpio_platdata {

•   struct udevice *pinctrl_dev;
  

•   struct list_head gpiomap;
  

•   void __iomem *base;
  

•   char *name;
  

•   u32 ngpios;
  

+};

+/**

• • iproc_gpio_set_bit - set or clear one bit (corresponding to the GPIO pin)

Please fit the title on a single line. You can add more detail later. E.g.

iproc_gpio_set_bit() - set or clear one bit in an iproc GPIO register

The bit relates to a GPIO pin

• • in a iproc GPIO register
• • @iproc_gpio: Iproc GPIO device

But here you are actually passing plat

• • @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);
  

•   clrsetbits_le32(plat->base + offset, BIT(shift),
  

•                   (set ? BIT(shift) : 0));
  

+}

+static inline bool iproc_gpio_get_bit(struct iproc_gpio_platdata *plat,

•                                 u32 reg, u32 gpio)
  

+{

•   u32 offset = IPROC_GPIO_REG(gpio, reg);
  

•   u32 shift = IPROC_GPIO_SHIFT(gpio);
  

•   return readl(plat->base + offset) & BIT(shift);
  

+}

+static u32 iproc_get_pctrl_from_gpio(struct iproc_gpio_platdata *plat, u32 gpio)

function comment

+{

•   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)) {
  

•                   ret = range->pctrl_pin + (gpio - range->gpio_pin);
  

•                   break;
  

•           }
  

•   }
  

•   return ret;
  

+}

+static int iproc_get_gpio_pctrl_mapping(struct udevice *dev)

function comment

+{

•   struct iproc_gpio_platdata *plat = dev_get_platdata(dev);
  

•   struct iproc_gpio_pctrl_map *range = NULL;
  

•   struct ofnode_phandle_args args;
  

•   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)
  

comment why you are doing this return. I think it is because it is a top-level device?

•           return 0;
  

•   pctrl = iproc_get_pctrl_from_gpio(plat, gpio);
  

•   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);
  

You might consider dropping the IPROC_GPIO prefix on these, or at least the IPROC prefix. They are close to this file so there is no need for all the boilerplate.

•   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;
  

+}

+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) {
  

•           debug("%s: Failed to get base address\n", __func__);
  

•           return -EINVAL;
  

•   }
  

•   ret = dev_read_u32(dev, "ngpios", &plat->ngpios);
  

•   if (ret < 0) {
  

•           dev_err(dev, "%s: Failed to get ngpios\n", __func__);
  

Note that this adds size to the driver. If you don't care about that it is fine, otherwise consider using log_debug()

•           return ret;
  

•   }
  

•   uclass_get_device_by_phandle(UCLASS_PINCTRL, dev, "gpio-ranges",
  

•                                &plat->pinctrl_dev);
  

•   if (ret < 0) {
  

•           dev_err(dev, "%s: Failed to get pinctrl phandle\n", __func__);
  

•           return ret;
  

•   }
  

•   INIT_LIST_HEAD(&plat->gpiomap);
  

•   ret = iproc_get_gpio_pctrl_mapping(dev);
  

•   if (ret < 0) {
  

•           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);
  

•   if (!plat->name)
  

•           return -ENOMEM;
  

•   uc_priv->gpio_count = plat->ngpios;
  

•   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