[U-Boot] [PATCH 1/4] gpio: UniPhier: add driver for UniPhier GPIO controller
Simon Glass
sjg at chromium.org
Tue Jul 28 19:36:34 CEST 2015
Hi Masahiro,
On 26 July 2015 at 09:56, Masahiro Yamada <yamada.masahiro at socionext.com> wrote:
> 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.
We should be strict on this, it's really important.
>
>
>
>
>
>
>>>
>>>>> + *
>>>>> + * 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?
>
Yes it has been a long-standing convention.
Here's some example code using offsets:
u8 *pr_base = (u8 *)(ibase + 0x08);
u32 *actl = (u32 *)(ibase + ACTL);
u16 *ir_base = (u16 *)(ibase + 0x20);
int i;
writel(SCIS_IRQ9, ibase + ILB_ACTL);
for (i = 0; i < NUM_PIRQS; i++)
writeb(ir->pic[i], pr_base + i);
for (i = 0; i < NUM_OF_PCI_DEVS; i++)
writew(ir->pcidev[i], ir_base + i);
and again with structs:
struct ibase_regs *ibase...
int i;
writel(SCIS_IRQ9, &ibase->actl);
for (i = 0; i < NUM_PIRQS; i++)
writeb(ir->pic[i], &ibase->pirq[i]);
for (i = 0; i < NUM_OF_PCI_DEVS; i++)
writew(ir->pcidev[i], &ibase->ir[i]);
I much prefer the second one - it is easier to get right and looks
neater. We can pass the register struct around the place without
worrying about bad offsets, etc. There are fewer #defines for the
offsets.
It just seems odd to me not to use basic C features when we are writing C.
>
>
>
>>>
>>> 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.
Sure, but that is IP that is widely implemented differently by
different SoCs. In your case you have one SoC. My question is whether
the Unipher SoCs are inconsistent with their registers sizes.
>
>
>
> 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.
IMO device tree has nothing to do with this. Only this particular
driver has this requirement. Also if you look at the driver model
implementation of ns16550 it does use a C struct at least for the main
driver. It deals with the problem you mention in another way.
>
>
>
> 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.
Sure, but that should be fixed. They can all use the same structure
IMO. Tegra 114 and 124 are the same, and tegra20 is the same except
that it lacks some fields.
>
>
>>>
>>> - 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.
Right, but that's uncommon in drivers. Hardware registers tend to use
aligned access and in fact many demand it, as you found.
I am not sure how U-Boot came to have this convention as I've not been
around that long. My understanding is that Albert knows most about it.
If there is a reason why you need different size registers in your
GPIO driver then I think it's fine to implement it that way. But in
general we should use structures IMO.
>
>
>
>
>
>>>
>>>
>>>
>>>>> +
>>>>> + 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
Regards,
Simon
More information about the U-Boot
mailing list