Re: [U-Boot] [RFC PATCH 19/29] drivers: gpio: add GPIO controller driver for OcteonTX

Hi Suneel,

On Tue, 29 Oct 2019 at 14:08, Suneel Garapati suneelglinux@gmail.com wrote:

From: Suneel Garapati sgarapati@marvell.com

Adds support for GPIO controllers found on OcteonTX or OcteonTX2 SoC platforms.

Signed-off-by: Aaron Williams awilliams@marvell.com Signed-off-by: Suneel Garapati sgarapati@marvell.com

---

drivers/gpio/Kconfig | 7 ++ drivers/gpio/Makefile | 1 + drivers/gpio/octeontx_gpio.c | 218 +++++++++++++++++++++++++++++++++++ 3 files changed, 226 insertions(+) create mode 100644 drivers/gpio/octeontx_gpio.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index c1ad5d64a3..91265180d3 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -276,6 +276,13 @@ config PIC32_GPIO help Say yes here to support Microchip PIC32 GPIOs.

+config OCTEONTX_GPIO

•   bool "OcteonTX GPIO driver"
  

•   depends on DM_GPIO && (ARCH_OCTEONTX || ARCH_OCTEONTX2)
  

•   default y
  

•   help
  

•     Say yes here to support OcteonTX GPIOs.
  

Please add a bit more detail here - how many GPIOs, what features are supported?

config STM32_GPIO bool "ST STM32 GPIO driver" depends on DM_GPIO && (STM32 || ARCH_STM32MP) diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index ccc49e2eb0..d7651bff4b 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -55,6 +55,7 @@ obj-$(CONFIG_HIKEY_GPIO) += hi6220_gpio.o obj-$(CONFIG_HSDK_CREG_GPIO) += hsdk-creg-gpio.o obj-$(CONFIG_IMX_RGPIO2P) += imx_rgpio2p.o obj-$(CONFIG_PIC32_GPIO) += pic32_gpio.o +obj-$(CONFIG_OCTEONTX_GPIO) += octeontx_gpio.o obj-$(CONFIG_MVEBU_GPIO) += mvebu_gpio.o obj-$(CONFIG_MSM_GPIO) += msm_gpio.o obj-$(CONFIG_$(SPL_)PCF8575_GPIO) += pcf8575_gpio.o diff --git a/drivers/gpio/octeontx_gpio.c b/drivers/gpio/octeontx_gpio.c new file mode 100644 index 0000000000..d012044c47 --- /dev/null +++ b/drivers/gpio/octeontx_gpio.c @@ -0,0 +1,218 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2018 Marvell International Ltd.
• • https://spdx.org/licenses
• • (C) Copyright 2011
• • eInfochips Ltd. <www.einfochips.com>
• • Written-by: Ajay Bhargav ajay.bhargav@einfochips.com
• • (C) Copyright 2010
• • Marvell Semiconductor <www.marvell.com>
• */

+#include <common.h> +#include <dm.h> +#include <malloc.h> +#include <errno.h> +#include <fdtdec.h> +#include <asm/io.h> +#include <asm/gpio.h> +#include <asm/bitops.h> +#include <dt-bindings/gpio/gpio.h>

+DECLARE_GLOBAL_DATA_PTR;

+/** Returns the bit value to write or read based on the offset */ +#define GPIO_BIT(x) (1ULL << ((x) & 0x3f))

Maybe BIT_ULL(x & 03f)

Again I think patman would probably find these problems.

Also suggest a bit more internal review if someone has time.

+#define GPIO_RX_DAT (0x0) +#define GPIO_TX_SET (0x8) +#define GPIO_TX_CLR (0x10) +#define GPIO_CONST (0x90) +#define GPIO_RX1_DAT (0x1400) +#define GPIO_TX1_SET (0x1408) +#define GPIO_TX1_CLR (0x1410)

Drop rackets

+/** Returns the offset to the output register based on the offset and value */ +#define GPIO_TX_REG(offset, value) \

•   ((offset) >= 64 ? ((value) ? GPIO_TX1_SET : GPIO_TX1_CLR) :     \
  

•                     ((value) ? GPIO_TX_SET : GPIO_TX_CLR))
  

+/** Returns the offset to the input data register based on the offset */ +#define GPIO_RX_DAT_REG(offset) (((offset) >= 64) ? GPIO_RX1_DAT : GPIO_RX_DAT)

+/** Returns the bit configuration register based on the offset */ +#define GPIO_BIT_CFG(x) (0x400 + 8 * (x)) +#define GPIO_BIT_CFG_FN(x) (((x) >> 16) & 0x3ff) +#define GPIO_BIT_CFG_TX_OE(x) ((x) & 0x1) +#define GPIO_BIT_CFG_RX_DAT(x) ((x) & 0x1)

Should use _SHIFT and _MASK registers and open-code things rather than these sort of macros.

+/** PCI ID on NCB bus */ +#define PCI_DEVICE_ID_OCTEONTX_GPIO 0xa00a

What does this mean? If it is a PCI device ID it should go in pci_ids.h

+union gpio_const {

comment for union

•   u64 u;
  

•   struct {
  

•           u64 gpios:8;    /** Number of GPIOs implemented */
  

/* Number

But maybe use the 'struct' comment style above the struct?

•           u64 pp:8;       /** Number of PP vectors */
  

What is a PP vector? Please add comments

•           u64:48; /* Reserved */
  

•   } s;
  

+};

+struct octeontx_gpio {

•   void __iomem *baseaddr;
  

+};

+static int octeontx_gpio_dir_input(struct udevice *dev, unsigned int offset) +{

•   struct octeontx_gpio *gpio = dev_get_priv(dev);
  

•   debug("%s(%s, %u)\n", __func__, dev->name, offset);
  

•   clrbits_le64(gpio->baseaddr + GPIO_BIT_CFG(offset),
  

•                (0x3ffUL << 16) | 4UL | 2UL | 1UL);
  

You don't need the ULs here. Please fix below also

Again, please use blank line before return globally

•   return 0;
  

+}

+static int octeontx_gpio_dir_output(struct udevice *dev, unsigned int offset,

•                               int value)
  

+{

•   struct octeontx_gpio *gpio = dev_get_priv(dev);
  

•   debug("%s(%s, %u, %d)\n", __func__, dev->name, offset, value);
  

•   writeq(GPIO_BIT(offset), gpio->baseaddr + GPIO_TX_REG(offset, value));
  

•   clrsetbits_le64(gpio->baseaddr + GPIO_BIT_CFG(offset),
  

•                   ((0x3ffUL << 16) | 4UL), 1UL);
  

•   return 0;
  

+}

+static int octeontx_gpio_get_value(struct udevice *dev,

•                              unsigned int offset)
  

+{

•   struct octeontx_gpio *gpio = dev_get_priv(dev);
  

•   u64 reg = readq(gpio->baseaddr + GPIO_RX_DAT_REG(offset));
  

•   debug("%s(%s, %u): value: %d\n", __func__, dev->name, offset,
  

•         !!(reg & GPIO_BIT(offset)));
  

•   return !!(reg & GPIO_BIT(offset));
  

+}

+static int octeontx_gpio_set_value(struct udevice *dev,

•                              unsigned int offset, int value)
  

+{

•   struct octeontx_gpio *gpio = dev_get_priv(dev);
  

•   debug("%s(%s, %u, %d)\n", __func__, dev->name, offset, value);
  

•   writeq(GPIO_BIT(offset), gpio->baseaddr + GPIO_TX_REG(offset, value));
  

•   return 0;
  

+}

+static int octeontx_gpio_get_function(struct udevice *dev,

•                                 unsigned int offset)
  

+{

•   struct octeontx_gpio *gpio = dev_get_priv(dev);
  

•   u64 pinsel = readl(gpio->baseaddr + GPIO_BIT_CFG(offset));
  

•   debug("%s(%s, %u): pinsel: 0x%llx\n", __func__, dev->name, offset,
  

•         pinsel);
  

•   if (GPIO_BIT_CFG_FN(pinsel))
  

•           return GPIOF_FUNC;
  

•   else if (GPIO_BIT_CFG_TX_OE(pinsel))
  

•           return GPIOF_OUTPUT;
  

•   else
  

•           return GPIOF_INPUT;
  

+}

+static int octeontx_gpio_xlate(struct udevice *dev, struct gpio_desc *desc,

•                          struct ofnode_phandle_args *args)
  

+{

•   if (args->args_count < 1)
  

•           return -EINVAL;
  

Could use log_msg_ret("arg_count", -EINVAL) if you want some logging

•   desc->offset = args->args[0];
  

•   desc->flags = 0;
  

•   if (args->args_count > 1) {
  

•           if (args->args[1] & GPIO_ACTIVE_LOW)
  

•                   desc->flags |= GPIOD_ACTIVE_LOW;
  

•           /* In the future add tri-state flag support */
  

•   }
  

•   return 0;
  

+}

+static const struct dm_gpio_ops octeontx_gpio_ops = {

•   .direction_input        = octeontx_gpio_dir_input,
  

•   .direction_output       = octeontx_gpio_dir_output,
  

•   .get_value              = octeontx_gpio_get_value,
  

•   .set_value              = octeontx_gpio_set_value,
  

•   .get_function           = octeontx_gpio_get_function,
  

•   .xlate                  = octeontx_gpio_xlate,
  

+};

+static int octeontx_gpio_probe(struct udevice *dev) +{

•   pci_dev_t bdf = dm_pci_get_bdf(dev);
  

•   struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
  

•   struct octeontx_gpio *priv = dev_get_priv(dev);
  

•   union gpio_const gpio_const;
  

•   char *end;
  

•   const char *status;
  

•   status = ofnode_read_string(dev->node, "status");
  

•   if (status && !strncmp(status, "ok", 2)) {
  

•           debug("%s(%s): GPIO device disabled in device tree\n",
  

•                 __func__, dev->name);
  

•           return -1;
  

•   }
  

•   dev->req_seq = PCI_FUNC(bdf);
  

What is that for?

•   priv->baseaddr = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0,
  

•                                   PCI_REGION_MEM);
  

•   if (!priv->baseaddr) {
  

•           debug("%s(%s): Could not get base address\n",
  

•                 __func__, dev->name);
  

•           return -1;
  

•   }
  

•   gpio_const.u = readq(priv->baseaddr + GPIO_CONST);
  

•   debug("%s(%s): base address: %p, of_offset: %ld, pin count: %d\n",
  

•         __func__, dev->name, priv->baseaddr, dev->node.of_offset,
  

•         gpio_const.s.gpios);
  

•   uc_priv->gpio_count = gpio_const.s.gpios;
  

•   uc_priv->bank_name  = strdup(dev->name);
  

•   end = strchr(uc_priv->bank_name, '@');
  

•   end[0] = 'A' + dev->seq;
  

•   end[1] = '\0';
  

•   return 0;
  

+}

+static const struct udevice_id octeontx_gpio_ids[] = {

•   { .compatible = "cavium,thunder-8890-gpio" },
  

•   { }
  

+};

+U_BOOT_DRIVER(octeontx_gpio) = {

•   .name   = "octeontx_gpio",
  

•   .id     = UCLASS_GPIO,
  

•   .of_match = of_match_ptr(octeontx_gpio_ids),
  

•   .probe = octeontx_gpio_probe,
  

•   .priv_auto_alloc_size = sizeof(struct octeontx_gpio),
  

•   .ops    = &octeontx_gpio_ops,
  

•   .flags  = DM_FLAG_PRE_RELOC,
  

+};

+static struct pci_device_id octeontx_gpio_supported[] = {

•   { PCI_VDEVICE(CAVIUM, PCI_DEVICE_ID_OCTEONTX_GPIO) },
  

•   { },
  

+};

+U_BOOT_PCI_DEVICE(octeontx_gpio, octeontx_gpio_supported);

2.23.0

Regards, Simon