diff mbox series

[PATCH/RFC,5/6] gpio: rcar: Add support for R-Car V3U

Message ID 20201019120614.22149-6-geert+renesas@glider.be
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series R-Car V3U GPIO support | expand

Commit Message

Geert Uytterhoeven Oct. 19, 2020, 12:06 p.m. UTC
From: Phong Hoang <phong.hoang.wz@renesas.com>

Add support for the R-Car V3U (r8a779a0) SoC.
This includes support for the new "General Input Enable" register to
control input enable.

Signed-off-by: Phong Hoang <phong.hoang.wz@renesas.com>
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Untested on actual hardware.

Should input be enabled unconditionally, as recommended by the Hardware
Manual for backwards compatibility with existing software?
As per (errata?) commit ae9550f635533b1c ("gpio-rcar: Use OUTDT when
reading GPIOs configured as output"), the gpio-rcar driver does not use
the INDT register to read the status of a GPIO line when configured for
output.
---
 drivers/gpio/gpio-rcar.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Comments

Geert Uytterhoeven Oct. 20, 2020, 8:22 a.m. UTC | #1
On Mon, Oct 19, 2020 at 2:06 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> From: Phong Hoang <phong.hoang.wz@renesas.com>
>
> Add support for the R-Car V3U (r8a779a0) SoC.
> This includes support for the new "General Input Enable" register to
> control input enable.
>
> Signed-off-by: Phong Hoang <phong.hoang.wz@renesas.com>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Untested on actual hardware.
>
> Should input be enabled unconditionally, as recommended by the Hardware
> Manual for backwards compatibility with existing software?
> As per (errata?) commit ae9550f635533b1c ("gpio-rcar: Use OUTDT when
> reading GPIOs configured as output"), the gpio-rcar driver does not use
> the INDT register to read the status of a GPIO line when configured for
> output.

More investigation:
  1. On SH7734, R-Car Gen1, R-Car Gen2, and RZ/G1, INDT only reflects
     the state of the input pin if the GPIO is configured for input
     (documented, verified on R-Car M2-W),
  2. On R-Car Gen3 and RZ/G2, INDT always reflects the state of the
     input pin (documented, verified on R-Car H3/M3-W/M3-N),
  3. On R-Car V3U, input enable can be configured explicitly through the
      new INEN register (documented, unverified).

Given register accesses are slow, I think it makes sense to enable input
explicitly and unconditionally on R-Car V3U, and always use INDT to read
pin state on R-Car Gen3 and V3U.

Thanks for your comments!

Gr{oetje,eeting}s,

                        Geert
Ulrich Hecht Oct. 21, 2020, 4:15 p.m. UTC | #2
Thank you for your patch!

> On 10/19/2020 2:06 PM Geert Uytterhoeven <geert+renesas@glider.be> wrote:
> 
>  
> From: Phong Hoang <phong.hoang.wz@renesas.com>
> 
> Add support for the R-Car V3U (r8a779a0) SoC.
> This includes support for the new "General Input Enable" register to
> control input enable.
> 
> Signed-off-by: Phong Hoang <phong.hoang.wz@renesas.com>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Untested on actual hardware.
> 
> Should input be enabled unconditionally, as recommended by the Hardware
> Manual for backwards compatibility with existing software?
> As per (errata?) commit ae9550f635533b1c ("gpio-rcar: Use OUTDT when
> reading GPIOs configured as output"), the gpio-rcar driver does not use
> the INDT register to read the status of a GPIO line when configured for
> output.
> ---
>  drivers/gpio/gpio-rcar.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c
> index a924cf8ac8df7f8f..44d09a0028db49e9 100644
> --- a/drivers/gpio/gpio-rcar.c
> +++ b/drivers/gpio/gpio-rcar.c
> @@ -42,6 +42,7 @@ struct gpio_rcar_priv {
>  	atomic_t wakeup_path;
>  	bool has_outdtsel;
>  	bool has_both_edge_trigger;
> +	bool has_inen;
>  	struct gpio_rcar_bank_info bank_info;
>  };
>  
> @@ -58,6 +59,7 @@ struct gpio_rcar_priv {
>  #define FILONOFF 0x28	/* Chattering Prevention On/Off Register */
>  #define OUTDTSEL 0x40	/* Output Data Select Register */
>  #define BOTHEDGE 0x4c	/* One Edge/Both Edge Select Register */
> +#define INEN 0x50	/* General Input Enable Register */
>  
>  #define RCAR_MAX_GPIO_PER_BANK		32
>  
> @@ -126,6 +128,10 @@ static void gpio_rcar_config_interrupt_input_mode(struct gpio_rcar_priv *p,
>  	if (p->has_both_edge_trigger)
>  		gpio_rcar_modify_bit(p, BOTHEDGE, hwirq, both);
>  
> +	/* Select "Input Enable" in INEN */
> +	if (p->has_inen)
> +		gpio_rcar_modify_bit(p, INEN, hwirq, true);
> +
>  	/* Select "Interrupt Input Mode" in IOINTSEL */
>  	gpio_rcar_modify_bit(p, IOINTSEL, hwirq, true);
>  
> @@ -231,6 +237,10 @@ static void gpio_rcar_config_general_input_output_mode(struct gpio_chip *chip,
>  	/* Configure positive logic in POSNEG */
>  	gpio_rcar_modify_bit(p, POSNEG, gpio, false);
>  
> +	/* Select "Input Enable/Disable" in INEN */

Nit: It's always called "Input Enable" in the DS.

> +	if (p->has_inen)
> +		gpio_rcar_modify_bit(p, INEN, gpio, !output);
> +
>  	/* Select "General Input/Output Mode" in IOINTSEL */
>  	gpio_rcar_modify_bit(p, IOINTSEL, gpio, false);
>  
> @@ -349,16 +359,25 @@ static int gpio_rcar_direction_output(struct gpio_chip *chip, unsigned offset,
>  struct gpio_rcar_info {
>  	bool has_outdtsel;
>  	bool has_both_edge_trigger;
> +	bool has_inen;
>  };
>  
>  static const struct gpio_rcar_info gpio_rcar_info_gen1 = {
>  	.has_outdtsel = false,
>  	.has_both_edge_trigger = false,
> +	.has_inen = false,
>  };
>  
>  static const struct gpio_rcar_info gpio_rcar_info_gen2 = {
>  	.has_outdtsel = true,
>  	.has_both_edge_trigger = true,
> +	.has_inen = false,
> +};
> +
> +static const struct gpio_rcar_info gpio_rcar_info_v3u = {
> +	.has_outdtsel = true,
> +	.has_both_edge_trigger = true,
> +	.has_inen = true,
>  };
>  
>  static const struct of_device_id gpio_rcar_of_table[] = {
> @@ -389,6 +408,9 @@ static const struct of_device_id gpio_rcar_of_table[] = {
>  		.compatible = "renesas,gpio-r8a7796",
>  		/* Gen3 GPIO is identical to Gen2. */
>  		.data = &gpio_rcar_info_gen2,
> +	}, {
> +		.compatible = "renesas,gpio-r8a779a0",
> +		.data = &gpio_rcar_info_v3u,
>  	}, {
>  		.compatible = "renesas,rcar-gen1-gpio",
>  		.data = &gpio_rcar_info_gen1,
> @@ -419,6 +441,7 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p, unsigned int *npins)
>  	info = of_device_get_match_data(p->dev);
>  	p->has_outdtsel = info->has_outdtsel;
>  	p->has_both_edge_trigger = info->has_both_edge_trigger;
> +	p->has_inen = info->has_inen;
>  
>  	ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &args);
>  	*npins = ret == 0 ? args.args[2] : RCAR_MAX_GPIO_PER_BANK;
> -- 
> 2.17.1

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli
Ulrich Hecht Oct. 21, 2020, 4:16 p.m. UTC | #3
> On 10/20/2020 10:22 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > Should input be enabled unconditionally, as recommended by the Hardware
> > Manual for backwards compatibility with existing software?
> > As per (errata?) commit ae9550f635533b1c ("gpio-rcar: Use OUTDT when
> > reading GPIOs configured as output"), the gpio-rcar driver does not use
> > the INDT register to read the status of a GPIO line when configured for
> > output.
> 
> More investigation:
>   1. On SH7734, R-Car Gen1, R-Car Gen2, and RZ/G1, INDT only reflects
>      the state of the input pin if the GPIO is configured for input
>      (documented, verified on R-Car M2-W),
>   2. On R-Car Gen3 and RZ/G2, INDT always reflects the state of the
>      input pin (documented, verified on R-Car H3/M3-W/M3-N),
>   3. On R-Car V3U, input enable can be configured explicitly through the
>       new INEN register (documented, unverified).
> 
> Given register accesses are slow, I think it makes sense to enable input
> explicitly and unconditionally on R-Car V3U, and always use INDT to read
> pin state on R-Car Gen3 and V3U.

That sounds like a good solution to me.

CU
Uli
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c
index a924cf8ac8df7f8f..44d09a0028db49e9 100644
--- a/drivers/gpio/gpio-rcar.c
+++ b/drivers/gpio/gpio-rcar.c
@@ -42,6 +42,7 @@  struct gpio_rcar_priv {
 	atomic_t wakeup_path;
 	bool has_outdtsel;
 	bool has_both_edge_trigger;
+	bool has_inen;
 	struct gpio_rcar_bank_info bank_info;
 };
 
@@ -58,6 +59,7 @@  struct gpio_rcar_priv {
 #define FILONOFF 0x28	/* Chattering Prevention On/Off Register */
 #define OUTDTSEL 0x40	/* Output Data Select Register */
 #define BOTHEDGE 0x4c	/* One Edge/Both Edge Select Register */
+#define INEN 0x50	/* General Input Enable Register */
 
 #define RCAR_MAX_GPIO_PER_BANK		32
 
@@ -126,6 +128,10 @@  static void gpio_rcar_config_interrupt_input_mode(struct gpio_rcar_priv *p,
 	if (p->has_both_edge_trigger)
 		gpio_rcar_modify_bit(p, BOTHEDGE, hwirq, both);
 
+	/* Select "Input Enable" in INEN */
+	if (p->has_inen)
+		gpio_rcar_modify_bit(p, INEN, hwirq, true);
+
 	/* Select "Interrupt Input Mode" in IOINTSEL */
 	gpio_rcar_modify_bit(p, IOINTSEL, hwirq, true);
 
@@ -231,6 +237,10 @@  static void gpio_rcar_config_general_input_output_mode(struct gpio_chip *chip,
 	/* Configure positive logic in POSNEG */
 	gpio_rcar_modify_bit(p, POSNEG, gpio, false);
 
+	/* Select "Input Enable/Disable" in INEN */
+	if (p->has_inen)
+		gpio_rcar_modify_bit(p, INEN, gpio, !output);
+
 	/* Select "General Input/Output Mode" in IOINTSEL */
 	gpio_rcar_modify_bit(p, IOINTSEL, gpio, false);
 
@@ -349,16 +359,25 @@  static int gpio_rcar_direction_output(struct gpio_chip *chip, unsigned offset,
 struct gpio_rcar_info {
 	bool has_outdtsel;
 	bool has_both_edge_trigger;
+	bool has_inen;
 };
 
 static const struct gpio_rcar_info gpio_rcar_info_gen1 = {
 	.has_outdtsel = false,
 	.has_both_edge_trigger = false,
+	.has_inen = false,
 };
 
 static const struct gpio_rcar_info gpio_rcar_info_gen2 = {
 	.has_outdtsel = true,
 	.has_both_edge_trigger = true,
+	.has_inen = false,
+};
+
+static const struct gpio_rcar_info gpio_rcar_info_v3u = {
+	.has_outdtsel = true,
+	.has_both_edge_trigger = true,
+	.has_inen = true,
 };
 
 static const struct of_device_id gpio_rcar_of_table[] = {
@@ -389,6 +408,9 @@  static const struct of_device_id gpio_rcar_of_table[] = {
 		.compatible = "renesas,gpio-r8a7796",
 		/* Gen3 GPIO is identical to Gen2. */
 		.data = &gpio_rcar_info_gen2,
+	}, {
+		.compatible = "renesas,gpio-r8a779a0",
+		.data = &gpio_rcar_info_v3u,
 	}, {
 		.compatible = "renesas,rcar-gen1-gpio",
 		.data = &gpio_rcar_info_gen1,
@@ -419,6 +441,7 @@  static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p, unsigned int *npins)
 	info = of_device_get_match_data(p->dev);
 	p->has_outdtsel = info->has_outdtsel;
 	p->has_both_edge_trigger = info->has_both_edge_trigger;
+	p->has_inen = info->has_inen;
 
 	ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &args);
 	*npins = ret == 0 ? args.args[2] : RCAR_MAX_GPIO_PER_BANK;