[U-Boot] [PATCH 1/4] gpio: UniPhier: add driver for UniPhier GPIO controller
Masahiro Yamada
yamada.masahiro at socionext.com
Tue Jul 21 20:19:27 CEST 2015
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>, ...
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.
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.
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.
>> + *
>> + * 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.
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.
- 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.
>> +
>> + 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...
--
Best Regards
Masahiro Yamada
More information about the U-Boot
mailing list