[U-Boot] [PATCH 1/4] gpio: UniPhier: add driver for UniPhier GPIO controller
Masahiro Yamada
yamada.masahiro at socionext.com
Wed Jul 29 06:10:23 CEST 2015
2015-07-29 2:36 GMT+09:00 Simon Glass <sjg at chromium.org>:
> 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.
OK, bindings should be the same beyond projects.
So, we can copy bindings docs if they are already available in Linux.
>>
>>
>>
>>
>>
>>
>>>>
>>>>>> + *
>>>>>> + * 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.
Right, code is neater with C structures.
>>
>>
>>
>>>>
>>>> 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.
No, almost.
Some IPs (such as MMC card) are connected to 16bit bus in our old SoCs,
but 32bit bus in new ones. But, it is rare.
We are developing some aarch64 SoC.
Some of IPs that were originally developed for 32bit SoC might be
reused for ARM64 SoC.
If the register view is consistent, that is OK..
>>
>>
>> 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.
We should rework ns16550 driver without C struct.
It is a widely-used IP, so it is the case where macro + reg_shift
should be supported.
UniPhier SoCs have 32-bit bus for its internal bus,
and 16-bit bus for external connection.
Some UART devices are connected on both buses.
(It is often useful to have multiple UART devices for normal console, GDB, etc.)
C structure, which fixes the reg stride at compile time,
does not fix with such a case.
>>
>>
>>
>> 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.
OK, move struct gpio_ctlr_bank will be moved to tegra_gpio.c
and arch-tegra*/gpio.h will be removed
>>
>>
>>>>
>>>> - 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.
This hardware has only two registers in each bank. (DIR and DATA)
struct uniphier_gpio_bank {
}
>>
>>
>>
>>
>>
>>>>
>>>>
>>>>
>>>>>> +
>>>>>> + 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
> _______________________________________________
> 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