[U-Boot] [PATCH 1/4] gpio: UniPhier: add driver for UniPhier GPIO controller
Masahiro Yamada
yamada.masahiro at socionext.com
Sun Jul 26 17:56:52 CEST 2015
Hi Simon,
2015-07-22 12:46 GMT+09:00 Simon Glass <sjg at chromium.org>:
> 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.
I still think it is not important, though.
What is more important is to document it in bindings text.
- How many cells this driver takes
- The meaning of each argument
Consumers are responsible to follow the spec written in the bindings doc.
Adding binding document for each driver is pretty strict in Linux,
but it is not U-boot. At least, I do not see often u-boot custodians
point it out.
>>
>>>> + *
>>>> + * 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.
Because someone decided so?
>>
>> 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.
Indeed, it is necessary and useful.
You should read Documentation/devicetree/bindings/serial/8250.txt in
Linux, for example.
I use 8250-compat driver with reg-shift = <1>
because the UART device is connected to 16-bit bus.
I would not set the reg-shift property if the device was connected to 8-bit bus.
Likewise, I would set reg-shift = <2> if the device was connected to 32-bit bus.
reg_base + (UART_LCR << reg_shift) works quite nicely.
On the other hand, include/ns16550.h in U-boot is horrible.
#if !defined(CONFIG_SYS_NS16550_REG_SIZE) || (CONFIG_SYS_NS16550_REG_SIZE == 0)
#error "Please define NS16550 registers size."
#elif defined(CONFIG_SYS_NS16550_MEM32) && !defined(CONFIG_DM_SERIAL)
#define UART_REG(x) u32 x
#elif (CONFIG_SYS_NS16550_REG_SIZE > 0)
#define UART_REG(x) \
unsigned char prepad_##x[CONFIG_SYS_NS16550_REG_SIZE - 1]; \
unsigned char x;
#elif (CONFIG_SYS_NS16550_REG_SIZE < 0)
#define UART_REG(x) \
unsigned char x; \
unsigned char postpad_##x[-CONFIG_SYS_NS16550_REG_SIZE - 1];
#endif
This is already too crappy.
Besides, it cannot be run-time configurable that is a requirement of
Device Tree.
If we have one device on 16-bit bus and another on 32-bus, this driver cannot
support them at the same time.
C structure for hardware register does not fit with Device Tree.
Tegra GPIO also demonstrates the problem well.
Strangely, drivers/gpio/tegra_gpio.c depends on <asm/arch/gpio.h>
for the definition of struct gpio_ctlr_bank.
What is worse,
arch/arm/include/asm/arch-tegra20/gpio.h
arch/arm/include/asm/arch-tegra30/gpio.h
arch/arm/include/asm/arch-tegra124/gpio.h
define different struct gpio_ctlr_bank.
This is a good example of bad driver implementation.
>>
>> - 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.
__packed is often used to not insert padding, and it is sometimes
needed for C structures in case unaligned registers exist.
>>
>>
>>
>>>> +
>>>> + 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
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
--
Best Regards
Masahiro Yamada
More information about the U-Boot
mailing list