[RFC PATCH 0/3] gpio: rcar: Add support for GPIO alternative interrupt

[RFC PATCH 0/3] gpio: rcar: Add support for GPIO alternative interrupt

[Eugeniu Rosca]

The motivation behind this series is described in commit
("gpio: rcar: Add support for GPIO alternative interrupt").

The reasons for which RFC tag is attached are:
 - Have Renesas/linux-gpio communities been facing GPIO interrupt
   partitioning questions in the past (specifically AP vs RT)?
 - Is it a no-go to have mandatory DTS dependencies like updating
   'interrupts' on specifying the new 'use-alternative-interrupt'?

Many thanks for your comments!

Eugeniu Rosca (1):
  [LOCAL] arm64: dts: renesas: ulcb: Showcase 'use-alternative-interrupt'

Torii Kenichi (2):
  dt-bindings: gpio-rcar: Add optional use-alternative-interrupt property
  gpio: rcar: Add support for GPIO alternative interrupt

 Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt |  3 +++
 arch/arm64/boot/dts/renesas/ulcb.dtsi                        | 40 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpio/gpio-rcar.c                                     | 22 +++++++++++++++++-----
 3 files changed, 60 insertions(+), 5 deletions(-)

-- 
2.26.0

[RFC PATCH 1/3] dt-bindings: gpio-rcar: Add optional use-alternative-interrupt property

[Eugeniu Rosca]

From: Torii Kenichi <torii.ken1@jp.fujitsu.com>

When setting this property, you must set alternate interrupt number in
the 'interrupts' property.

Signed-off-by: Torii Kenichi <torii.ken1@jp.fujitsu.com>
[erosca: tidy up the descriptions/comments]
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt b/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt
index 10dce84b1545..aa5aea90431a 100644
--- a/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt
+++ b/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt
@@ -50,6 +50,9 @@ Optional properties:
   - clocks: Must contain a reference to the functional clock.  The property is
     mandatory if the hardware implements a controllable functional clock for
     the GPIO instance.
+  - use-alternative-interrupt: Use 'alternative' GPIO interrupt instead
+    of 'normal' GPIO interrupt. When you specify this property, you must
+    also change the 'interrupts' DT property.
 
   - gpio-reserved-ranges: See gpio.txt.
 
-- 
2.26.0

[RFC PATCH 2/3] gpio: rcar: Add support for GPIO alternative interrupt

[Eugeniu Rosca]

From: Torii Kenichi <torii.ken1@jp.fujitsu.com>

INTC-AP accepts both GPIO interrupt and GPIO alternative interrupt,
but INTC-RT can only handle GPIO interrupt, as depicted in below excerpt
from 'Figure 7.1 GPIO Block Configuration (1)' of
'R-Car3 HW User's Manual Rev.2.00, Jul 2019':

  +------------------------------+
  | Interrupt  display register  +----> GPIO.ch*  (to INTC-AP, INTC-RT)
  |           (INTDTn)           +----> GPIO.ch*A (to INTC-AP)
  +------------------------------+

It seems to be also the case for earlier Renesas SoCs like RZ/G,
as per 'Figure 6.1 GPIO Block Configuration' in
'RZ/G Series User’s Manual: Hardware Rev.1.00 Sep 2016' [1].

To reduce the interference between RT domain (CR7/SH) and the AP domain
(Cortex A5x) and to independently control GPIO interrupts in these two
domains, add support for processing GPIO alternative interrupts in AP.

This allows handling normal GPIO interrupts exclusively by INTC-RT.
The change is DT-driven and depends on the enablement of the
'use-alternative-interrupt' DTS property.

One caveat is that the 'interrupts' property update must go hand in hand
with the newly added 'use-alternative-interrupt' property.

[1] https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rzg/rzg1m.html#documents

Signed-off-by: Torii Kenichi <torii.ken1@jp.fujitsu.com>
[erosca: enrich commit description]
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 drivers/gpio/gpio-rcar.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c
index 7284473c9fe3..6bbab447bba8 100644
--- a/drivers/gpio/gpio-rcar.c
+++ b/drivers/gpio/gpio-rcar.c
@@ -43,6 +43,8 @@ struct gpio_rcar_priv {
 	bool has_outdtsel;
 	bool has_both_edge_trigger;
 	struct gpio_rcar_bank_info bank_info;
+	int intmsk;
+	int mskclr;
 };
 
 #define IOINTSEL 0x00	/* General IO/Interrupt Switching Register */
@@ -56,6 +58,8 @@ struct gpio_rcar_priv {
 #define POSNEG 0x20	/* Positive/Negative Logic Select Register */
 #define EDGLEVEL 0x24	/* Edge/level Select Register */
 #define FILONOFF 0x28	/* Chattering Prevention On/Off Register */
+#define INTMSKS 0x38	/* Interrupt Sub Mask Register */
+#define MSKCLRS 0x3c	/* Interrupt Sub Mask Clear Register */
 #define OUTDTSEL 0x40	/* Output Data Select Register */
 #define BOTHEDGE 0x4c	/* One Edge/Both Edge Select Register */
 
@@ -90,7 +94,7 @@ static void gpio_rcar_irq_disable(struct irq_data *d)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct gpio_rcar_priv *p = gpiochip_get_data(gc);
 
-	gpio_rcar_write(p, INTMSK, ~BIT(irqd_to_hwirq(d)));
+	gpio_rcar_write(p, p->intmsk, ~BIT(irqd_to_hwirq(d)));
 }
 
 static void gpio_rcar_irq_enable(struct irq_data *d)
@@ -98,7 +102,7 @@ static void gpio_rcar_irq_enable(struct irq_data *d)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct gpio_rcar_priv *p = gpiochip_get_data(gc);
 
-	gpio_rcar_write(p, MSKCLR, BIT(irqd_to_hwirq(d)));
+	gpio_rcar_write(p, p->mskclr, BIT(irqd_to_hwirq(d)));
 }
 
 static void gpio_rcar_config_interrupt_input_mode(struct gpio_rcar_priv *p,
@@ -203,7 +207,7 @@ static irqreturn_t gpio_rcar_irq_handler(int irq, void *dev_id)
 	unsigned int offset, irqs_handled = 0;
 
 	while ((pending = gpio_rcar_read(p, INTDT) &
-			  gpio_rcar_read(p, INTMSK))) {
+			  gpio_rcar_read(p, p->intmsk))) {
 		offset = __ffs(pending);
 		gpio_rcar_write(p, INTCLR, BIT(offset));
 		generic_handle_irq(irq_find_mapping(p->gpio_chip.irq.domain,
@@ -427,6 +431,14 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p, unsigned int *npins)
 		*npins = RCAR_MAX_GPIO_PER_BANK;
 	}
 
+	if (of_get_property(np, "use-alternative-interrupt", NULL)) {
+		p->intmsk = INTMSKS;
+		p->mskclr = MSKCLRS;
+	} else {
+		p->intmsk = INTMSK;
+		p->mskclr = MSKCLR;
+	}
+
 	return 0;
 }
 
@@ -544,7 +556,7 @@ static int gpio_rcar_suspend(struct device *dev)
 	p->bank_info.iointsel = gpio_rcar_read(p, IOINTSEL);
 	p->bank_info.inoutsel = gpio_rcar_read(p, INOUTSEL);
 	p->bank_info.outdt = gpio_rcar_read(p, OUTDT);
-	p->bank_info.intmsk = gpio_rcar_read(p, INTMSK);
+	p->bank_info.intmsk = gpio_rcar_read(p, p->intmsk);
 	p->bank_info.posneg = gpio_rcar_read(p, POSNEG);
 	p->bank_info.edglevel = gpio_rcar_read(p, EDGLEVEL);
 	if (p->has_both_edge_trigger)
@@ -586,7 +598,7 @@ static int gpio_rcar_resume(struct device *dev)
 				!!(p->bank_info.bothedge & mask));
 
 			if (p->bank_info.intmsk & mask)
-				gpio_rcar_write(p, MSKCLR, mask);
+				gpio_rcar_write(p, p->mskclr, mask);
 		}
 	}
 
-- 
2.26.0

[RFC PATCH 3/3] [LOCAL] arm64: dts: renesas: ulcb: Showcase 'use-alternative-interrupt'

[Eugeniu Rosca]

One caveat is that the 'interrupts' property update _must_ go hand
in hand with the newly added 'use-alternative-interrupt' property.
Is there an easy way to avoid it?

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 arch/arm64/boot/dts/renesas/ulcb.dtsi | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/ulcb.dtsi b/arch/arm64/boot/dts/renesas/ulcb.dtsi
index d2ba9598cd1e..168dadbd1185 100644
--- a/arch/arm64/boot/dts/renesas/ulcb.dtsi
+++ b/arch/arm64/boot/dts/renesas/ulcb.dtsi
@@ -174,6 +174,46 @@ &extalr_clk {
 	clock-frequency = <32768>;
 };
 
+&gpio0 {
+	use-alternative-interrupt;
+	interrupts = <GIC_SPI 248 IRQ_TYPE_LEVEL_HIGH>;
+};
+
+&gpio1 {
+	use-alternative-interrupt;
+	interrupts = <GIC_SPI 249 IRQ_TYPE_LEVEL_HIGH>;
+};
+
+&gpio2 {
+	use-alternative-interrupt;
+	interrupts = <GIC_SPI 250 IRQ_TYPE_LEVEL_HIGH>;
+};
+
+&gpio3 {
+	use-alternative-interrupt;
+	interrupts = <GIC_SPI 251 IRQ_TYPE_LEVEL_HIGH>;
+};
+
+&gpio4 {
+	use-alternative-interrupt;
+	interrupts = <GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH>;
+};
+
+&gpio5 {
+	use-alternative-interrupt;
+	interrupts = <GIC_SPI 253 IRQ_TYPE_LEVEL_HIGH>;
+};
+
+&gpio6 {
+	use-alternative-interrupt;
+	interrupts = <GIC_SPI 254 IRQ_TYPE_LEVEL_HIGH>;
+};
+
+&gpio7 {
+	use-alternative-interrupt;
+	interrupts = <GIC_SPI 255 IRQ_TYPE_LEVEL_HIGH>;
+};
+
 &hdmi0 {
 	status = "okay";
 
-- 
2.26.0

Re: [RFC PATCH 1/3] dt-bindings: gpio-rcar: Add optional use-alternative-interrupt property

[Geert Uytterhoeven]

Hi Eugeniu,

CC devicetree

On Wed, Apr 22, 2020 at 12:11 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> From: Torii Kenichi <torii.ken1@jp.fujitsu.com>
>
> When setting this property, you must set alternate interrupt number in
> the 'interrupts' property.
>
> Signed-off-by: Torii Kenichi <torii.ken1@jp.fujitsu.com>
> [erosca: tidy up the descriptions/comments]
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>

Thanks for your/forwarding this patch!

> --- a/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt
> +++ b/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt
> @@ -50,6 +50,9 @@ Optional properties:
>    - clocks: Must contain a reference to the functional clock.  The property is
>      mandatory if the hardware implements a controllable functional clock for
>      the GPIO instance.
> +  - use-alternative-interrupt: Use 'alternative' GPIO interrupt instead

renesas,use-alternative-interrupt?
(unless you can convince Rob this becomes a generic property).

> +    of 'normal' GPIO interrupt. When you specify this property, you must
> +    also change the 'interrupts' DT property.

In light of "DT describes the hardware", I think you should instead list
both interrupts in DT.  I.e. first/single interrupt is the normal one,
second optional interrupt is the alternative one.

On R-Car Gen2, the oldest SoC family with this feature I could find,
they are called "ext_int" resp. "ext_alt_int", so introducing the
optional "clock-names" property would be a good idea, too.

>    - gpio-reserved-ranges: See gpio.txt.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH 2/3] gpio: rcar: Add support for GPIO alternative interrupt

[Geert Uytterhoeven]

Hi Eugeniu,

CC devicetree
CC Stefano

Thanks for forwarding this patch!

On Wed, Apr 22, 2020 at 12:11 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> From: Torii Kenichi <torii.ken1@jp.fujitsu.com>
>
> INTC-AP accepts both GPIO interrupt and GPIO alternative interrupt,
> but INTC-RT can only handle GPIO interrupt, as depicted in below excerpt
> from 'Figure 7.1 GPIO Block Configuration (1)' of
> 'R-Car3 HW User's Manual Rev.2.00, Jul 2019':
>
>   +------------------------------+
>   | Interrupt  display register  +----> GPIO.ch*  (to INTC-AP, INTC-RT)
>   |           (INTDTn)           +----> GPIO.ch*A (to INTC-AP)
>   +------------------------------+

Note that GPIO.ch[67]A seem to be available on INTC-RT, too. But for
the other channels, you're right in that the "A" variants are connected
to INTC-AP only.

> It seems to be also the case for earlier Renesas SoCs like RZ/G,
> as per 'Figure 6.1 GPIO Block Configuration' in
> 'RZ/G Series User’s Manual: Hardware Rev.1.00 Sep 2016' [1].

On R-Car Gen2, they're called "EXT_INT" resp. "EXT_ALT_INT" instead of
"GPIO.ch*" and "GPIO.ch*A".

> To reduce the interference between RT domain (CR7/SH) and the AP domain
> (Cortex A5x) and to independently control GPIO interrupts in these two
> domains, add support for processing GPIO alternative interrupts in AP.
>
> This allows handling normal GPIO interrupts exclusively by INTC-RT.
> The change is DT-driven and depends on the enablement of the
> 'use-alternative-interrupt' DTS property.
>
> One caveat is that the 'interrupts' property update must go hand in hand
> with the newly added 'use-alternative-interrupt' property.

As I replied to the DT bindings patch, I think the "interrupts" property
should contain both, so "use-alternative-interrupt" can pick the one it
needs.

> Signed-off-by: Torii Kenichi <torii.ken1@jp.fujitsu.com>
> [erosca: enrich commit description]
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>

[actual patch[1] deleted, as it doesn't really matter for the discussion].

You may want to look at "LTD20-205 System Device Tree Project"[2],
where Stefano talks about using DT to describe the full system
(I've read the slides, but haven't watched the video yet).

Your patch shows that the assumption "All devices have interrupts routed
to both interrupt controllers" isn't always true.

[1] https://lore.kernel.org/linux-gpio/20200422101026.9220-3-erosca@de.adit-jv.com/
[2] https://connect.linaro.org/resources/ltd20/ltd20-205/

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds