Hi Masahiro, On 21 July 2015 at 12:19, Masahiro Yamada <yamada.masah...@socionext.com> wrote: > Hi Simon, > > > 2015-07-18 23:36 GMT+09:00 Simon Glass <s...@chromium.org>: >> Hi Masahiro, >> >> On 13 July 2015 at 02:29, Masahiro Yamada <yamada.masah...@socionext.com> >> wrote: >>> This GPIO controller device is used on UniPhier SoCs. >>> >>> Signed-off-by: Masahiro Yamada <yamada.masah...@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.masah...@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. > >>> + * >>> + * 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. > > 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. > > - 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. > > > >>> + >>> + 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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
