RE: [PATCH 2/3] gpio: gpio-rz: GPIO driver for Renesas RZ series
Hi Jacopo, On Thursday, January 12, 2017, jacopo mondi wrote: > Oh! That's weird I don't have that statement in my manual (public version > R01UH0403EJ0060 Rev.0.60) If you go to www.renesas.com and search for "RZ/A1H User Manual", you will see that a new version 3.0 manual was recently posted. This new version contains many new descriptions and precautions. It also now includes the SDHI chapter which previously was always omitted because of legal reasons. There is a separate manual for the RZ/A1L. The peripherals are all the same, but the function pinouts are different because the packages are smaller. Hence, all the existing upstream drivers for RZ/A1H work fine for RZ/A1M and RZ/A1L without modification...except the pins comes out on different ports...and the sh-pfc driver won't work well in this situation. Chris
Re: [PATCH 2/3] gpio: gpio-rz: GPIO driver for Renesas RZ series
Hi Chris, On 12/01/2017 15:39, Chris Brandt wrote: Hi Jacopo, On Thursday, January 12, 2017, jacopo mondi wrote: To read the pin direction I would need to add one more register to the "reg" property range provided in the DTS. This is of course doable, but I would have two questions here: - why did you chose to use PMSR register instead of reading/writing directly to PM? Am I missing something? I guess you can still use the PMSR register to get the current direction of the I/O pin. According to the HW manual: "When reading, the higher 16 bits are read as H. The lower 16 bits are read as the value of the PMn register." Oh! That's weird I don't have that statement in my manual (public version R01UH0403EJ0060 Rev.0.60) The only description for PMSR I have is: "This register provides an alternative method for writing data to the PMn register. The higher 16 bits of the PMSRn register specify whether data can be written to the PMn.PMnm bit specified by the lower 16 bits of the PMSRn register." So I assumed it was one direction only While the PMSR register is a cool idea of having a special register that can change a single port pin direction without doing a read-modify-write by the CPU, it doesn't seem to be a standard option in Renesas SoCs (well, except for the older NEC V850 parts where this PFC HW came from). So a more traditional method of just using the Pmn register seems to be more compatible across Renesas SoCs. Chris
RE: [PATCH 2/3] gpio: gpio-rz: GPIO driver for Renesas RZ series
Hi Jacopo, On Thursday, January 12, 2017, jacopo mondi wrote: > To read the pin direction I would need to add one more register to the > "reg" property range provided in the DTS. > This is of course doable, but I would have two questions here: > - why did you chose to use PMSR register instead of reading/writing > directly to PM? Am I missing something? I guess you can still use the PMSR register to get the current direction of the I/O pin. According to the HW manual: "When reading, the higher 16 bits are read as H. The lower 16 bits are read as the value of the PMn register." While the PMSR register is a cool idea of having a special register that can change a single port pin direction without doing a read-modify-write by the CPU, it doesn't seem to be a standard option in Renesas SoCs (well, except for the older NEC V850 parts where this PFC HW came from). So a more traditional method of just using the Pmn register seems to be more compatible across Renesas SoCs. Chris
Re: [PATCH 2/3] gpio: gpio-rz: GPIO driver for Renesas RZ series
Hi Linus,
thanks for review
On 11/01/2017 15:55, Linus Walleij wrote:
On Mon, Jan 9, 2017 at 8:31 PM, Jacopo Mondi wrote:
+static void rz_gpio_free(struct gpio_chip *chip, unsigned offset)
+{
+ pinctrl_free_gpio(chip->base + offset);
+
+ /* Set the GPIO as an input to ensure that the next GPIO request won't
+ * drive the GPIO pin as an output.
+ */
+ rz_gpio_direction_input(chip, offset);
+}
Very close to gpiochip_generic_free()
Maybe we should actually set lines as input in the
generic routine!
Is it ok then to simply substitute "pinctrl_free_gpio" with
"gpiochip_generic_free" and keep rz_gpio_direction_input here?
+ gpio_chip = &p->gpio_chip;
+ gpio_chip->direction_input = rz_gpio_direction_input;
+ gpio_chip->get = rz_gpio_get;
+ gpio_chip->direction_output = rz_gpio_direction_output;
Please implement .get_direction() as well.
That's funny how a simple addition like this one would require several
changes (possibly even in the dts ABI defined by this driver).
This is more a question for the original driver author I hope is
listening here:
To read the pin direction I would need to add one more register to the
"reg" property range provided in the DTS.
This is of course doable, but I would have two questions here:
- why did you chose to use PMSR register instead of reading/writing
directly to PM? Am I missing something?
- wouldn't it be better to just receive the port base address from the
"reg" property and offset from that instead of having the 3 register
addresses specified in the dts?
See, the dts now looks like this:
reg = <0xfcfe3100 0x4>, /* PSR */
<0xfcfe3200 0x2>, /* PPR */
<0xfcfe3800 0x4>; /* PMSR */
Shouldn't we just receive the gpiochip base address and the offset as we
like?
Thanks
j
Re: [PATCH 2/3] gpio: gpio-rz: GPIO driver for Renesas RZ series
On Mon, Jan 9, 2017 at 8:31 PM, Jacopo Mondi wrote:
> From: Magnus Damm
>
> This commit combines Magnus' original driver and minor fixes to
> forward-port it to a more recent kernel version (v4.10)
>
> Signed-off-by: Jacopo Mondi
>
> ---
> gpio: Renesas RZ GPIO driver V3
>
> This patch adds a GPIO driver for the RZ series of SoCs from
> Renesas. The V3 of the driver requires DT to be used.
>
> The hardware allows control of GPIOs in blocks of up to 16 pins.
> Interrupts are not included in this hardware block, if interrupts
> are needed then the PFC needs to be configured to a IRQ pin function
> which is handled by the GIC hardware.
>
> Tested with yet-to-be-posted DT devices on r7s72100 and Genmai using
> LEDs, DIP switches and I2C bitbang.
>
> Signed-off-by: Magnus Damm
>
> gpio: gpio-rz: Port to v4.10-rc1
>
> Fix invalid return value in gpio remove function and change gpiochip's
> "dev" field name to "parent" to compile driver against v4.10
>
> Signed-off-by: Jacopo Mondi
This commit message looks seriously mangled, please make it look like a regular
one with all signed offs at the end.
> +config GPIO_RZ
> + tristate "Renesas RZ GPIO"
> + depends on ARM
ARM really? Not some Renesas or so?
Include || COMPILE_TEST?
> +#include
> +#include
> +#include
No. Use only
#include
> +static inline struct rz_gpio_priv *gpio_to_priv(struct gpio_chip *chip)
> +{
> + return container_of(chip, struct rz_gpio_priv, gpio_chip);
> +}
Please get rid of this and use gpiochip_get_data()
after adding the chip with devm_gpiochip_add_data()
supplying the data you need.
Look at other GPIO drivers for examples.
> +static int rz_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> + /* Get bit from PPR register to determine pin state */
> + return rz_gpio_read_ppr(gpio_to_priv(chip), offset);
return !!rz_gpio_read_ppr(gpio_to_priv(chip), offset);
To clamp to bool.
> +static void rz_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> + /* Set bit in P register via PSR to control output */
> + rz_gpio_write(gpio_to_priv(chip), REG_PSR, offset, !!value);
> +}
Ironically you can trust the value to be 0/1 here. Check the
gpiolib.
> +static int rz_gpio_request(struct gpio_chip *chip, unsigned offset)
> +{
> + return pinctrl_request_gpio(chip->base + offset);
> +}
What about just using gpiochip_generic_request instead?
> +static void rz_gpio_free(struct gpio_chip *chip, unsigned offset)
> +{
> + pinctrl_free_gpio(chip->base + offset);
> +
> + /* Set the GPIO as an input to ensure that the next GPIO request won't
> + * drive the GPIO pin as an output.
> + */
> + rz_gpio_direction_input(chip, offset);
> +}
Very close to gpiochip_generic_free()
Maybe we should actually set lines as input in the
generic routine!
> + gpio_chip = &p->gpio_chip;
> + gpio_chip->direction_input = rz_gpio_direction_input;
> + gpio_chip->get = rz_gpio_get;
> + gpio_chip->direction_output = rz_gpio_direction_output;
Please implement .get_direction() as well.
> + gpio_chip->set = rz_gpio_set;
> + gpio_chip->request = rz_gpio_request;
> + gpio_chip->free = rz_gpio_free;
> + gpio_chip->label = dev_name(&pdev->dev);
> + gpio_chip->parent = &pdev->dev;
> + gpio_chip->owner = THIS_MODULE;
> + gpio_chip->base = -1;
> + gpio_chip->ngpio = ret == 0 ? args.args[2] : RZ_GPIOS_PER_PORT;
> +
> + ret = gpiochip_add(gpio_chip);
Use devm_gpiochip_add_data() providing struct rz_gpio_priv * as
data.
> +static int rz_gpio_remove(struct platform_device *pdev)
> +{
> + struct rz_gpio_priv *p = platform_get_drvdata(pdev);
> +
> + gpiochip_remove(&p->gpio_chip);
> + return 0;
> +}
And with the devm_gpiochip_add_data() you don't even need
this remove().
Yours,
Linus Walleij
[PATCH 2/3] gpio: gpio-rz: GPIO driver for Renesas RZ series
From: Magnus Damm
This commit combines Magnus' original driver and minor fixes to
forward-port it to a more recent kernel version (v4.10)
Signed-off-by: Jacopo Mondi
---
gpio: Renesas RZ GPIO driver V3
This patch adds a GPIO driver for the RZ series of SoCs from
Renesas. The V3 of the driver requires DT to be used.
The hardware allows control of GPIOs in blocks of up to 16 pins.
Interrupts are not included in this hardware block, if interrupts
are needed then the PFC needs to be configured to a IRQ pin function
which is handled by the GIC hardware.
Tested with yet-to-be-posted DT devices on r7s72100 and Genmai using
LEDs, DIP switches and I2C bitbang.
Signed-off-by: Magnus Damm
gpio: gpio-rz: Port to v4.10-rc1
Fix invalid return value in gpio remove function and change gpiochip's
"dev" field name to "parent" to compile driver against v4.10
Signed-off-by: Jacopo Mondi
---
drivers/gpio/Kconfig | 6 ++
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-rz.c | 212 +
3 files changed, 219 insertions(+)
create mode 100644 drivers/gpio/gpio-rz.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index d5d3654..e9ad7b4 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -369,6 +369,12 @@ config GPIO_RCAR
help
Say yes here to support GPIO on Renesas R-Car SoCs.
+config GPIO_RZ
+ tristate "Renesas RZ GPIO"
+ depends on ARM
+ help
+ Say yes here to support GPIO on Renesas RZ SoCs.
+
config GPIO_SPEAR_SPICS
bool "ST SPEAr13xx SPI Chip Select as GPIO support"
depends on PLAT_SPEAR
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index a7676b8..f0b2713 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -96,6 +96,7 @@ obj-$(CONFIG_GPIO_PXA)+= gpio-pxa.o
obj-$(CONFIG_GPIO_RC5T583) += gpio-rc5t583.o
obj-$(CONFIG_GPIO_RDC321X) += gpio-rdc321x.o
obj-$(CONFIG_GPIO_RCAR)+= gpio-rcar.o
+obj-$(CONFIG_GPIO_RZ) += gpio-rz.o
obj-$(CONFIG_ARCH_SA1100) += gpio-sa1100.o
obj-$(CONFIG_GPIO_SCH) += gpio-sch.o
obj-$(CONFIG_GPIO_SCH311X) += gpio-sch311x.o
diff --git a/drivers/gpio/gpio-rz.c b/drivers/gpio/gpio-rz.c
new file mode 100644
index 000..b48fa05
--- /dev/null
+++ b/drivers/gpio/gpio-rz.c
@@ -0,0 +1,212 @@
+/*
+ * RZ GPIO Support - Ports
+ *
+ * Copyright (C) 2013 Magnus Damm
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; version 2 of the
+ * License.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define RZ_GPIOS_PER_PORT 16
+
+enum { REG_PSR, REG_PPR, REG_PMSR, REG_NR };
+
+struct rz_gpio_priv {
+ void __iomem *io[REG_NR];
+ struct gpio_chip gpio_chip;
+};
+
+static inline u32 rz_gpio_read_ppr(struct rz_gpio_priv *p, int offs)
+{
+ unsigned long msk = BIT(offs % RZ_GPIOS_PER_PORT);
+ unsigned int offset = (offs / RZ_GPIOS_PER_PORT) * 4;
+
+ return ioread32(p->io[REG_PPR] + offset) & msk;
+}
+
+static inline void rz_gpio_write(struct rz_gpio_priv *p, int reg, int offs,
+bool value)
+{
+ unsigned long msk = BIT(offs % RZ_GPIOS_PER_PORT);
+ unsigned int offset = (offs / RZ_GPIOS_PER_PORT) * 4;
+
+ /* upper 16 bits contain mask and lower 16 actual value */
+ iowrite32(value ? (msk | (msk << 16)) : (msk << 16),
+ p->io[reg] + offset);
+}
+
+static inline struct rz_gpio_priv *gpio_to_priv(struct gpio_chip *chip)
+{
+ return container_of(chip, struct rz_gpio_priv, gpio_chip);
+}
+
+static int rz_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
+{
+ /* Set bit in PM register via PMSR to disable output */
+ rz_gpio_write(gpio_to_priv(chip), REG_PMSR, offset, true);
+ return 0;
+}
+
+static int rz_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+ /* Get bit from PPR register to determine pin state */
+ return rz_gpio_read_ppr(gpio_to_priv(chip), offset);
+}
+
+static void rz_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+ /* Set bit in P register via PSR to control output */
+ rz_gpio_write(gpio_to_priv(chip), REG_PSR, offset, !!value);
+}
+
+static int rz_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
+ int value)
+{
+ /* Write GPIO value to output before selecting output mode of pin */
+ rz_gpio_set(chip, offset, value);
+
+ /* Clear bit in PM register via PMSR to enable output */
+ rz_gpio_write(gpio_to_priv(chip), REG_PMSR, offset, false);
+ return 0;
+}
+
+static int rz_gpio_request(struct gpio_chip *chip, unsigned offset)
+{
+ return p
