Hi Simon,
2015-07-18 23:36 GMT+09:00 Simon Glass <[email protected]>: > Hi Masahiro, > > On 13 July 2015 at 02:29, Masahiro Yamada <[email protected]> > wrote: >> This GPIO controller device is used on UniPhier SoCs. >> >> Signed-off-by: Masahiro Yamada <[email protected]> >> --- >> >> 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 <[email protected]> >> + * >> + * 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 _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
