Hi Masahiro,
On 21 July 2015 at 12:19, Masahiro Yamada yamada.masahiro@socionext.com wrote:
Hi Simon,
2015-07-18 23:36 GMT+09:00 Simon Glass sjg@chromium.org:
Hi Masahiro,
On 13 July 2015 at 02:29, Masahiro Yamada yamada.masahiro@socionext.com wrote:
This GPIO controller device is used on UniPhier SoCs.
Signed-off-by: Masahiro Yamada yamada.masahiro@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_UNIPHIERhelpSay 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@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);elsetmp &= ~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