[U-Boot] [PATCH 1/4] gpio: UniPhier: add driver for UniPhier GPIO controller

Simon Glass sjg at chromium.org
Wed Jul 22 05:46:58 CEST 2015


Hi Masahiro,

On 21 July 2015 at 12:19, Masahiro Yamada <yamada.masahiro at socionext.com> wrote:
> Hi Simon,
>
>
> 2015-07-18 23:36 GMT+09:00 Simon Glass <sjg at chromium.org>:
>> Hi Masahiro,
>>
>> On 13 July 2015 at 02:29, Masahiro Yamada <yamada.masahiro at socionext.com> wrote:
>>> This GPIO controller device is used on UniPhier SoCs.
>>>
>>> Signed-off-by: Masahiro Yamada <yamada.masahiro at socionext.com>
>>> ---
>>>
>>>  drivers/gpio/Kconfig         |   6 ++
>>>  drivers/gpio/Makefile        |   1 +
>>>  drivers/gpio/gpio-uniphier.c | 186 +++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 193 insertions(+)
>>>  create mode 100644 drivers/gpio/gpio-uniphier.c
>>>
>>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>>> index 0c43777..1176e3f 100644
>>> --- a/drivers/gpio/Kconfig
>>> +++ b/drivers/gpio/Kconfig
>>> @@ -36,6 +36,12 @@ config SANDBOX_GPIO_COUNT
>>>           of 'anonymous' GPIOs that do not belong to any device or bank.
>>>           Select a suitable value depending on your needs.
>>>
>>> +config GPIO_UNIPHIER
>>> +       bool "UniPhier GPIO"
>>> +       depends on ARCH_UNIPHIER
>>> +       help
>>> +         Say yes here to support UniPhier GPIOs.
>>> +
>>>  config VYBRID_GPIO
>>>         bool "Vybrid GPIO driver"
>>>         depends on DM
>>> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
>>> index 5864850..5ec4ad7 100644
>>> --- a/drivers/gpio/Makefile
>>> +++ b/drivers/gpio/Makefile
>>> @@ -44,5 +44,6 @@ oby-$(CONFIG_SX151X)          += sx151x.o
>>>  obj-$(CONFIG_SUNXI_GPIO)       += sunxi_gpio.o
>>>  obj-$(CONFIG_LPC32XX_GPIO)     += lpc32xx_gpio.o
>>>  obj-$(CONFIG_STM32_GPIO)       += stm32_gpio.o
>>> +obj-$(CONFIG_GPIO_UNIPHIER)    += gpio-uniphier.o
>>>  obj-$(CONFIG_ZYNQ_GPIO)                += zynq_gpio.o
>>>  obj-$(CONFIG_VYBRID_GPIO)      += vybrid_gpio.o
>>> diff --git a/drivers/gpio/gpio-uniphier.c b/drivers/gpio/gpio-uniphier.c
>>> new file mode 100644
>>> index 0000000..8f8ea38
>>> --- /dev/null
>>> +++ b/drivers/gpio/gpio-uniphier.c
>>> @@ -0,0 +1,186 @@
>>> +/*
>>> + * Copyright (C) 2015 Masahiro Yamada <yamada.masahiro at socionext.com>
>>> + *
>>> + * SPDX-License-Identifier:    GPL-2.0+
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <mapmem.h>
>>> +#include <linux/io.h>
>>> +#include <asm/errno.h>
>>> +#include <asm/gpio.h>
>>> +#include <dm/device.h>
>>> +
>>> +/*
>>> + * Unfortunately, the hardware specification adopts weird GPIO pin labeling.
>>> + * The ports are named as
>>> + *   PORT00,  PORT01,  PORT02,  ..., PORT07,
>>> + *   PORT10,  PORT11,  PORT12,  ..., PORT17,
>>> + *   PORT20,  PORT21,  PORT22,  ..., PORT27,
>>> + *    ...
>>> + *   PORT90,  PORT91,  PORT92,  ..., PORT97,
>>> + *   PORT100, PORT101, PORT102, ..., PORT107,
>>> + *    ...
>>> + *
>>> + * The PORTs with 8 or 9 in the one's place are missing, i.e. the one's place
>>> + * is octal, while the other places are decimal.  If we handle the port numbers
>>> + * as seen in the hardware documents, the GPIO offsets must be non-contiguous.
>>> + * It is possible to have sparse GPIO pins, but not handy for GPIO range
>>> + * mappings, register accessing, etc.
>>
>> This is because they are separate banks: PORT0, PORT1, PORT2, each
>> with 8 GPIOs. Exynos and Tegra have the same thing.
>>
>> Both of these use a separate device for each bank - this works out
>> nicely with device tree since you can say:
>>
>>    <&port1 2 0>
>>
>> and it will do the right thing.
>>
>> With exynos they are described in the device tree. With tegra the
>> top-level GPIO device is in the device tree and we manually bind its
>> children, one for each bank.
>>
>> Maybe you should do a similar thing for uniphier?
>
>
> As far as I understood, drivers/gpio/tegra_gpio.c in U-boot binds its children,
> while drivers/gpio/gpio-tegra.c in Linux adds only one GPIO chip for
> the whole banks.
>
> Because the Tegra device tree has only one GPIO node, so you cannot do
> like <&port0 3 0>, <&port1 2 0>, ...

Well instead we do things like <&gpio TEGRA_GPIO(H, 3) GPIO_ACTIVE_HIGH>

It's not quite as nice but it works.

>
>
> For Exynos, yes, there are multiple GPIO nodes under the pinctrl OF node.
> So, it is possible to do like <&port0 3 0>, <&port1 2 0>, ...
>
>
> Unfortunately, UniPhier SoCs have much more GPIO banks.
>
> I had already consulted linux-gpio ML:
> https://lkml.org/lkml/2015/6/18/933
>
> First, I tried to add 30 separate GPIO nodes in my device tree,
> but finally, I thought it was ridiculous.
>
> So, I turned around into the single OF node.
>

OK.

>
>
> My design priority is to use an identical device tree for both Linux and U-Boot.
> This is quite natural because device tree is a hardware description language.

Yes.

>
> I implemented a GPIO driver for Linux first in order to decide the
> device tree interface and basic design policy.
> Then, I backported it to U-boot.
> This is because the GPIO subsystem in Linux has more factors that
> should be taken into account:
>    - Interaction between GPIO and Pinctrl:  "gpio-ranges",
> "gpio-ranges-group-names"
>    - gpioirq_chip
>    - better support for OF  (drivers/gpio/gpiolib-of.c).
>      I want to match the device and the OF node to take advantage of it.
>       This is why I do not want to do like Tegra.
>
> I posted the GPIO driver for Linux, and v3 is now under review.
> https://lkml.org/lkml/2015/7/13/856
>
>
> I still not 100% sure what is the best solution. Perhaps, I may be
> missing something.
> If you think I am doing wrong, you can delay this series.
> I am not in a hurry about this series.
>
>
>
> BTW, Tegra has TEGRA_GPIO macro to be friendly to GPIO consumers.
>
> For ex.
> gpio = <&gpio TEGRA_GPIO(L, 6) GPIO_ACTIVE_HIGH>
>
>
> I can implement a similar one if necessary, but it is a minor issue.
>

I think you should, yes.

>
>>> + *
>>> + * To make things simpler (for driver and device tree implementation), this
>>> + * driver takes contiguously-numbered GPIO offsets.  GPIO consumers should make
>>> + * sure to convert the PORT number into the one that fits in this driver.
>>> + * The conversion logic is very easy math, for example,
>>> + *   PORT15  -->  GPIO offset 13   (8 * 1 + 5)
>>> + *   PORT123 -->  GPIO offset 99   (8 * 12 + 3)
>>> + */
>>> +#define UNIPHIER_GPIO_PORTS_PER_BANK   8
>>> +
>>> +#define UNIPHIER_GPIO_REG_DATA         0       /* data */
>>> +#define UNIPHIER_GPIO_REG_DIR          4       /* direction (1:in, 0:out) */
>>> +
>>> +/* delete the following when BIT(nr) is added to include/linux/bitops.h */
>>> +#define BIT(nr)                        (1UL << (nr))
>>> +
>>> +struct uniphier_gpio_priv {
>>> +       void __iomem *base;
>>> +};
>>> +
>>> +static unsigned uniphier_gpio_bank_to_reg(unsigned bank, unsigned reg_type)
>>> +{
>>> +       unsigned reg;
>>> +
>>> +       reg = (bank + 1) * 8 + reg_type;
>>> +
>>> +       /*
>>> +        * Unfortunately, there is a register hole at offset 0x90-0x9f.
>>> +        * Add 0x10 when crossing the hole.
>>> +        */
>>> +       if (reg >= 0x90)
>>> +               reg += 0x10;
>>> +
>>> +       return reg;
>>> +}
>>> +
>>> +static void uniphier_gpio_offset_write(struct udevice *dev, unsigned offset,
>>> +                                      unsigned reg_type, int value)
>>> +{
>>> +       struct uniphier_gpio_priv *priv = dev_get_priv(dev);
>>> +       unsigned bank = offset / UNIPHIER_GPIO_PORTS_PER_BANK;
>>> +       unsigned bit = offset % UNIPHIER_GPIO_PORTS_PER_BANK;
>>> +       unsigned reg;
>>> +       u32 tmp;
>>> +
>>> +       reg = uniphier_gpio_bank_to_reg(bank, reg_type);
>>
>> I think what you want is a struct gpio_bank or similar, and then store
>> the bank pointer in the device. We should use C struct access for I/O.
>
> Why?  Rationale please?
>
> When I see drivers/gpio/gpio-tegra.c in Linux,
> I only see macros
>
> #define GPIO_CNF(x) (GPIO_REG(x) + 0x00)
> #define GPIO_OE(x) (GPIO_REG(x) + 0x10)
> #define GPIO_OUT(x) (GPIO_REG(x) + 0X20)
> #define GPIO_IN(x) (GPIO_REG(x) + 0x30)
> #define GPIO_INT_STA(x) (GPIO_REG(x) + 0x40)
> #define GPIO_INT_ENB(x) (GPIO_REG(x) + 0x50)
> #define GPIO_INT_LVL(x) (GPIO_REG(x) + 0x60)
> #define GPIO_INT_CLR(x) (GPIO_REG(x) + 0x70)
>
>
> Actually, looks like macros are more often used in Linux for accessing I/O.

Right, but U-Boot uses structures normally.

>
> I am negative about using C struct for this purpose because:
>
>  - C struct is not flexible because we can not change the register stride
>    with the "reg-shift" property.

Do you need to? To me that would be unusual hardware.

>
>  - You may notice a nasty problem I hit by commit d6bc30af52446
>    ("ARM: UniPhier: remove __packed that causes a problem on GCC 4.9").
>    There is a pit-fall with  C struct + __packed.
>

Yes but there is no need to use __packed when everything is a 32-bit
register. I try to avoid __packed. I don't think that matters here.

>
>
>
>>> +
>>> +       tmp = readl(priv->base + reg);
>>> +       if (value)
>>> +               tmp |= BIT(bit);
>>> +       else
>>> +               tmp &= ~BIT(bit);
>>> +       writel(tmp, priv->base + reg);
>>> +}
>>> +
>>> +static int uniphier_gpio_offset_read(struct udevice *dev, unsigned offset,
>>> +                                    unsigned reg_type)
>>> +{
>>> +       struct uniphier_gpio_priv *priv = dev_get_priv(dev);
>>> +       unsigned bank = offset / UNIPHIER_GPIO_PORTS_PER_BANK;
>>> +       unsigned bit = offset % UNIPHIER_GPIO_PORTS_PER_BANK;
>>> +       unsigned reg;
>>> +
>>> +       reg = uniphier_gpio_bank_to_reg(bank, reg_type);
>>> +
>>> +       return readl(priv->base + reg) & BIT(bit) ? 1 : 0;
>>> +}
>>> +
>>> +static int uniphier_gpio_direction_input(struct udevice *dev, unsigned offset)
>>> +{
>>> +       uniphier_gpio_offset_write(dev, offset, UNIPHIER_GPIO_REG_DIR, 1);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int uniphier_gpio_direction_output(struct udevice *dev, unsigned offset,
>>> +                                         int value)
>>> +{
>>> +       uniphier_gpio_offset_write(dev, offset, UNIPHIER_GPIO_REG_DATA, value);
>>> +       uniphier_gpio_offset_write(dev, offset, UNIPHIER_GPIO_REG_DIR, 0);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int uniphier_gpio_get_value(struct udevice *dev, unsigned offset)
>>> +{
>>> +       return uniphier_gpio_offset_read(dev, offset, UNIPHIER_GPIO_REG_DATA);
>>> +}
>>> +
>>> +static int uniphier_gpio_set_value(struct udevice *dev, unsigned offset,
>>> +                                  int value)
>>> +{
>>> +       uniphier_gpio_offset_write(dev, offset, UNIPHIER_GPIO_REG_DATA, value);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int uniphier_gpio_get_function(struct udevice *dev, unsigned offset)
>>> +{
>>> +       return uniphier_gpio_offset_read(dev, offset, UNIPHIER_GPIO_REG_DIR) ?
>>> +                                               GPIOF_INPUT : GPIOF_OUTPUT;
>>> +}
>>> +
>>> +static const struct dm_gpio_ops uniphier_gpio_ops = {
>>> +       .direction_input        = uniphier_gpio_direction_input,
>>> +       .direction_output       = uniphier_gpio_direction_output,
>>> +       .get_value              = uniphier_gpio_get_value,
>>> +       .set_value              = uniphier_gpio_set_value,
>>> +       .get_function           = uniphier_gpio_get_function,
>>> +};
>>> +
>>> +static int uniphier_gpio_probe(struct udevice *dev)
>>> +{
>>> +       struct uniphier_gpio_priv *priv = dev_get_priv(dev);
>>> +       struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
>>> +       DECLARE_GLOBAL_DATA_PTR;
>>> +       fdt_addr_t addr;
>>> +       fdt_size_t size;
>>> +
>>> +       addr = fdtdec_get_addr_size(gd->fdt_blob, dev->of_offset, "reg",
>>> +                                   &size);
>>> +       if (addr == FDT_ADDR_T_NONE)
>>> +               return -EINVAL;
>>> +
>>> +       priv->base = map_sysmem(addr, size);
>>> +       if (!priv->base)
>>> +               return -ENOMEM;
>>
>> Most drivers don't bother with this but it is more correct. We have
>> dev_get_addr(). How about adding dev_get_addr_size() and using that?
>>
>
> Or, do we need struct resource to handle addr and size together?  I am
> not sure...
>

If you like, either is good with me. I'm quite happen with just an
addr and size for now. At some point I hope that regmap will provide
this functionality.

Regards.
Simon


More information about the U-Boot mailing list