From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rayagonda Kokatanur Date: Thu, 30 Apr 2020 15:24:48 +0530 Subject: [PATCH v2 1/2] drivers: gpio: add broadcom iproc gpio driver support In-Reply-To: References: <20200429062050.24536-1-rayagonda.kokatanur@broadcom.com> <20200429062050.24536-2-rayagonda.kokatanur@broadcom.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Wed, Apr 29, 2020 at 11:34 PM Simon Glass wrote: > > Hi Rayagonda, > > On Wed, 29 Apr 2020 at 00:21, Rayagonda Kokatanur > wrote: > > > > Add gpio driver support for Broadcom iproc-based socs. > > > > Signed-off-by: Rayagonda Kokatanur > > Signed-off-by: Sheetal Tigadoli > > --- > > 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 > > +#include > > +#include > > common.h always goes first > > +#include > > put asm ones here > > > +#include > > +#include > > +#include > > +#include > > 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