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

Simon Glass sjg at chromium.org
Wed Nov 20 03:00:52 UTC 2019


Hi Suneel,

On Tue, 29 Oct 2019 at 14:08, Suneel Garapati <suneelglinux at gmail.com> wrote:
>
> From: Suneel Garapati <sgarapati at marvell.com>
>
> Adds support for GPIO controllers found on OcteonTX or
> OcteonTX2 SoC platforms.
>
> Signed-off-by: Aaron Williams <awilliams at marvell.com>
> Signed-off-by: Suneel Garapati <sgarapati at 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 at 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


More information about the U-Boot mailing list