[PATCH v2 0/5] ARM: rskrza1: Add RZ/A1 IRQC and input switches

[PATCH v2 0/5] ARM: rskrza1: Add RZ/A1 IRQC and input switches

[Geert Uytterhoeven]

	Hi all,

Unlike on most other Renesas SoCs, the GPIO controller block on RZ/A1
and RZ/A2 SoCs lack interrupt functionality.  While the GPIOs can be
routed to the GIC as pin interrupts, this is of limited use, as the
PL390 or GIC-400 supports rising edge and high-level interrupts only.

Fortunately RZ/A1 and RZ/A2 SoCs contain a small front-end for the GIC,
allowing to use up to 8 external interrupts, with configurable sense
select.

Hence this patch series adds DT bindings and a driver for this
front-end, adds a device node for it in the RZ/A1H DTS, and uses it to
enable support for the 3 input switches on the Renesas RSK+RZA1
development board.

Changes compared to v1:
  - Add Reviewed-by,
  - Replace gic_spi_base in OF match data by renesas,gic-spi-base in DT,
  - Document RZ/A2M,
  - Use u16 for register values,
  - Use relaxed I/O accessors,
  - Use "rza1-irqc" as irq_chip class name,
  - Enable driver on RZ/A2M.

Dependencies:
  - Patch 3 depends on patch 2,
  - Patch 4 can be applied as soon as the DT bindings in patch 1 have
    been accepted,
  - Patch 5 depends on patch 4.

Upstream strategy:
  - Patches 1-2 are intended to be applied to the irqchip tree,
  - Patches 3-5 are meant for the Renesas tree.

This has been tested on RSK+RZA1 with evtest and s2ram wake-up.
I have verified proper operation of low-level and rising/falling sense
select, too.

Thanks!

Geert Uytterhoeven (5):
  dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt
    Controller
  irqchip: Add Renesas RZ/A1 Interrupt Controller driver
  soc: renesas: Enable RZ/A1 IRQC on RZ/A1H and RZ/A2M
  ARM: dts: r7s72100: Add IRQC device node
  ARM: dts: rskrza1: Add input switches

 .../renesas,rza1-irqc.txt                     |  30 +++
 arch/arm/boot/dts/r7s72100-rskrza1.dts        |  38 +++
 arch/arm/boot/dts/r7s72100.dtsi               |   9 +
 drivers/irqchip/Kconfig                       |   4 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-renesas-rza1.c            | 235 ++++++++++++++++++
 drivers/soc/renesas/Kconfig                   |   4 +-
 7 files changed, 320 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
 create mode 100644 drivers/irqchip/irq-renesas-rza1.c

-- 
2.17.1

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

[PATCH v2 1/5] dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt Controller

[Geert Uytterhoeven]

Add DT bindings for the Renesas RZ/A1 Interrupt Controller.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - Add "renesas,gic-spi-base",
  - Document RZ/A2M.
---
 .../renesas,rza1-irqc.txt                     | 30 +++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
new file mode 100644
index 0000000000000000..ea8ddb6955338ccd
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
@@ -0,0 +1,30 @@
+DT bindings for the Renesas RZ/A1 Interrupt Controller
+
+The RZ/A1 Interrupt Controller is a front-end for the GIC found on Renesas
+RZ/A1 and RZ/A2 SoCs:
+  - IRQ sense select for 8 external interrupts, 1:1-mapped to 8 GIC SPI
+    interrupts,
+  - NMI edge select.
+
+Required properties:
+  - compatible: Must be "renesas,<soctype>-irqc", and "renesas,rza1-irqc" as
+		fallback.
+		Examples with soctypes are:
+		  - "renesas,r7s72100-irqc" (RZ/A1H)
+		  - "renesas,r7s9210-irqc" (RZ/A2M)
+  - #interrupt-cells: Must be 2 (an interrupt index and flags, as defined
+				 in interrupts.txt in this directory)
+  - interrupt-controller: Marks the device as an interrupt controller
+  - reg: Base address and length of the memory resource used by the interrupt
+         controller
+  - renesas,gic-spi-base: Lowest GIC SPI interrupt number this block maps to.
+
+Example:
+
+	irqc: interrupt-controller@fcfef800 {
+		compatible = "renesas,r7s72100-irqc", "renesas,rza1-irqc";
+		#interrupt-cells = <2>;
+		interrupt-controller;
+		reg = <0xfcfef800 0x6>;
+		renesas,gic-spi-base = <0>;
+	};
-- 
2.17.1

Re: [PATCH v2 1/5] dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt Controller

[Rob Herring]

On Tue, Apr 30, 2019 at 7:13 AM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
>
> Add DT bindings for the Renesas RZ/A1 Interrupt Controller.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2:
>   - Add "renesas,gic-spi-base",
>   - Document RZ/A2M.
> ---
>  .../renesas,rza1-irqc.txt                     | 30 +++++++++++++++++++
>  1 file changed, 30 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> new file mode 100644
> index 0000000000000000..ea8ddb6955338ccd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> @@ -0,0 +1,30 @@
> +DT bindings for the Renesas RZ/A1 Interrupt Controller
> +
> +The RZ/A1 Interrupt Controller is a front-end for the GIC found on Renesas
> +RZ/A1 and RZ/A2 SoCs:
> +  - IRQ sense select for 8 external interrupts, 1:1-mapped to 8 GIC SPI
> +    interrupts,
> +  - NMI edge select.
> +
> +Required properties:
> +  - compatible: Must be "renesas,<soctype>-irqc", and "renesas,rza1-irqc" as
> +               fallback.
> +               Examples with soctypes are:
> +                 - "renesas,r7s72100-irqc" (RZ/A1H)
> +                 - "renesas,r7s9210-irqc" (RZ/A2M)
> +  - #interrupt-cells: Must be 2 (an interrupt index and flags, as defined
> +                                in interrupts.txt in this directory)
> +  - interrupt-controller: Marks the device as an interrupt controller
> +  - reg: Base address and length of the memory resource used by the interrupt
> +         controller
> +  - renesas,gic-spi-base: Lowest GIC SPI interrupt number this block maps to.

Why isn't this just an 'interrupts' property? Plus, without
'interrupts' walking the hierarchy is broken.

Rob

Re: [PATCH v2 1/5] dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt Controller

[Rob Herring]

On Tue, Apr 30, 2019 at 7:13 AM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
>
> Add DT bindings for the Renesas RZ/A1 Interrupt Controller.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2:
>   - Add "renesas,gic-spi-base",
>   - Document RZ/A2M.
> ---
>  .../renesas,rza1-irqc.txt                     | 30 +++++++++++++++++++
>  1 file changed, 30 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> new file mode 100644
> index 0000000000000000..ea8ddb6955338ccd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> @@ -0,0 +1,30 @@
> +DT bindings for the Renesas RZ/A1 Interrupt Controller
> +
> +The RZ/A1 Interrupt Controller is a front-end for the GIC found on Renesas
> +RZ/A1 and RZ/A2 SoCs:
> +  - IRQ sense select for 8 external interrupts, 1:1-mapped to 8 GIC SPI
> +    interrupts,
> +  - NMI edge select.
> +
> +Required properties:
> +  - compatible: Must be "renesas,<soctype>-irqc", and "renesas,rza1-irqc" as
> +               fallback.
> +               Examples with soctypes are:
> +                 - "renesas,r7s72100-irqc" (RZ/A1H)
> +                 - "renesas,r7s9210-irqc" (RZ/A2M)
> +  - #interrupt-cells: Must be 2 (an interrupt index and flags, as defined
> +                                in interrupts.txt in this directory)
> +  - interrupt-controller: Marks the device as an interrupt controller
> +  - reg: Base address and length of the memory resource used by the interrupt
> +         controller
> +  - renesas,gic-spi-base: Lowest GIC SPI interrupt number this block maps to.

Why isn't this just an 'interrupts' property? Plus, without
'interrupts' walking the hierarchy is broken.

Rob

Re: [PATCH v2 1/5] dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt Controller

[Geert Uytterhoeven]

Hi Rob,

On Tue, Apr 30, 2019 at 5:03 PM Rob Herring <robh+dt@kernel.org> wrote:
> On Tue, Apr 30, 2019 at 7:13 AM Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
> >
> > Add DT bindings for the Renesas RZ/A1 Interrupt Controller.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > v2:
> >   - Add "renesas,gic-spi-base",
> >   - Document RZ/A2M.

> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > @@ -0,0 +1,30 @@
> > +DT bindings for the Renesas RZ/A1 Interrupt Controller
> > +
> > +The RZ/A1 Interrupt Controller is a front-end for the GIC found on Renesas
> > +RZ/A1 and RZ/A2 SoCs:
> > +  - IRQ sense select for 8 external interrupts, 1:1-mapped to 8 GIC SPI
> > +    interrupts,
> > +  - NMI edge select.
> > +
> > +Required properties:
> > +  - compatible: Must be "renesas,<soctype>-irqc", and "renesas,rza1-irqc" as
> > +               fallback.
> > +               Examples with soctypes are:
> > +                 - "renesas,r7s72100-irqc" (RZ/A1H)
> > +                 - "renesas,r7s9210-irqc" (RZ/A2M)
> > +  - #interrupt-cells: Must be 2 (an interrupt index and flags, as defined
> > +                                in interrupts.txt in this directory)
> > +  - interrupt-controller: Marks the device as an interrupt controller
> > +  - reg: Base address and length of the memory resource used by the interrupt
> > +         controller
> > +  - renesas,gic-spi-base: Lowest GIC SPI interrupt number this block maps to.
>
> Why isn't this just an 'interrupts' property? Plus, without

Because Marc told me this is what everyone uses...

> 'interrupts' walking the hierarchy is broken.

What is "interrupts walking"? Can you please elaborate?

Thanks!

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: [PATCH v2 1/5] dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt Controller

[Geert Uytterhoeven]

Hi Rob,

On Tue, Apr 30, 2019 at 5:03 PM Rob Herring <robh+dt@kernel.org> wrote:
> On Tue, Apr 30, 2019 at 7:13 AM Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
> >
> > Add DT bindings for the Renesas RZ/A1 Interrupt Controller.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > v2:
> >   - Add "renesas,gic-spi-base",
> >   - Document RZ/A2M.

> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > @@ -0,0 +1,30 @@
> > +DT bindings for the Renesas RZ/A1 Interrupt Controller
> > +
> > +The RZ/A1 Interrupt Controller is a front-end for the GIC found on Renesas
> > +RZ/A1 and RZ/A2 SoCs:
> > +  - IRQ sense select for 8 external interrupts, 1:1-mapped to 8 GIC SPI
> > +    interrupts,
> > +  - NMI edge select.
> > +
> > +Required properties:
> > +  - compatible: Must be "renesas,<soctype>-irqc", and "renesas,rza1-irqc" as
> > +               fallback.
> > +               Examples with soctypes are:
> > +                 - "renesas,r7s72100-irqc" (RZ/A1H)
> > +                 - "renesas,r7s9210-irqc" (RZ/A2M)
> > +  - #interrupt-cells: Must be 2 (an interrupt index and flags, as defined
> > +                                in interrupts.txt in this directory)
> > +  - interrupt-controller: Marks the device as an interrupt controller
> > +  - reg: Base address and length of the memory resource used by the interrupt
> > +         controller
> > +  - renesas,gic-spi-base: Lowest GIC SPI interrupt number this block maps to.
>
> Why isn't this just an 'interrupts' property? Plus, without

Because Marc told me this is what everyone uses...

> 'interrupts' walking the hierarchy is broken.

What is "interrupts walking"? Can you please elaborate?

Thanks!

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: [PATCH v2 1/5] dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt Controller

[Marc Zyngier]

On 30/04/2019 16:02, Rob Herring wrote:
> On Tue, Apr 30, 2019 at 7:13 AM Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
>>
>> Add DT bindings for the Renesas RZ/A1 Interrupt Controller.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> v2:
>>   - Add "renesas,gic-spi-base",
>>   - Document RZ/A2M.
>> ---
>>  .../renesas,rza1-irqc.txt                     | 30 +++++++++++++++++++
>>  1 file changed, 30 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
>> new file mode 100644
>> index 0000000000000000..ea8ddb6955338ccd
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
>> @@ -0,0 +1,30 @@
>> +DT bindings for the Renesas RZ/A1 Interrupt Controller
>> +
>> +The RZ/A1 Interrupt Controller is a front-end for the GIC found on Renesas
>> +RZ/A1 and RZ/A2 SoCs:
>> +  - IRQ sense select for 8 external interrupts, 1:1-mapped to 8 GIC SPI
>> +    interrupts,
>> +  - NMI edge select.
>> +
>> +Required properties:
>> +  - compatible: Must be "renesas,<soctype>-irqc", and "renesas,rza1-irqc" as
>> +               fallback.
>> +               Examples with soctypes are:
>> +                 - "renesas,r7s72100-irqc" (RZ/A1H)
>> +                 - "renesas,r7s9210-irqc" (RZ/A2M)
>> +  - #interrupt-cells: Must be 2 (an interrupt index and flags, as defined
>> +                                in interrupts.txt in this directory)
>> +  - interrupt-controller: Marks the device as an interrupt controller
>> +  - reg: Base address and length of the memory resource used by the interrupt
>> +         controller
>> +  - renesas,gic-spi-base: Lowest GIC SPI interrupt number this block maps to.
> 
> Why isn't this just an 'interrupts' property?

That's likely because of kernel limitations. The DT code does an
of_populate() on any device that it finds, parse the "interrupts"
propertiy, resulting in the irq_descs being populated.

That creates havoc, as these interrupts are not for this device, but for
something that is connected to it. This is merely a bridge of some sort.

Furthermore, this is a rather long established practice: gic-v2m,
gic-v3-mbi, mediatek,sysirq, mediatek,cirq... All the bits of glue that
for one reason or another plug onto the GIC use the same method.

> Plus, without 'interrupts' walking the hierarchy is broken.

Erm... Which hierarchy?

Thanks,
	
	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 1/5] dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt Controller

[Marc Zyngier]

On 30/04/2019 16:02, Rob Herring wrote:
> On Tue, Apr 30, 2019 at 7:13 AM Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
>>
>> Add DT bindings for the Renesas RZ/A1 Interrupt Controller.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> v2:
>>   - Add "renesas,gic-spi-base",
>>   - Document RZ/A2M.
>> ---
>>  .../renesas,rza1-irqc.txt                     | 30 +++++++++++++++++++
>>  1 file changed, 30 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
>> new file mode 100644
>> index 0000000000000000..ea8ddb6955338ccd
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
>> @@ -0,0 +1,30 @@
>> +DT bindings for the Renesas RZ/A1 Interrupt Controller
>> +
>> +The RZ/A1 Interrupt Controller is a front-end for the GIC found on Renesas
>> +RZ/A1 and RZ/A2 SoCs:
>> +  - IRQ sense select for 8 external interrupts, 1:1-mapped to 8 GIC SPI
>> +    interrupts,
>> +  - NMI edge select.
>> +
>> +Required properties:
>> +  - compatible: Must be "renesas,<soctype>-irqc", and "renesas,rza1-irqc" as
>> +               fallback.
>> +               Examples with soctypes are:
>> +                 - "renesas,r7s72100-irqc" (RZ/A1H)
>> +                 - "renesas,r7s9210-irqc" (RZ/A2M)
>> +  - #interrupt-cells: Must be 2 (an interrupt index and flags, as defined
>> +                                in interrupts.txt in this directory)
>> +  - interrupt-controller: Marks the device as an interrupt controller
>> +  - reg: Base address and length of the memory resource used by the interrupt
>> +         controller
>> +  - renesas,gic-spi-base: Lowest GIC SPI interrupt number this block maps to.
> 
> Why isn't this just an 'interrupts' property?

That's likely because of kernel limitations. The DT code does an
of_populate() on any device that it finds, parse the "interrupts"
propertiy, resulting in the irq_descs being populated.

That creates havoc, as these interrupts are not for this device, but for
something that is connected to it. This is merely a bridge of some sort.

Furthermore, this is a rather long established practice: gic-v2m,
gic-v3-mbi, mediatek,sysirq, mediatek,cirq... All the bits of glue that
for one reason or another plug onto the GIC use the same method.

> Plus, without 'interrupts' walking the hierarchy is broken.

Erm... Which hierarchy?

Thanks,
	
	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 1/5] dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt Controller

[Rob Herring]

On Tue, Apr 30, 2019 at 10:34 AM Marc Zyngier <marc.zyngier@arm.com> wrote:
>
> On 30/04/2019 16:02, Rob Herring wrote:
> > On Tue, Apr 30, 2019 at 7:13 AM Geert Uytterhoeven
> > <geert+renesas@glider.be> wrote:
> >>
> >> Add DT bindings for the Renesas RZ/A1 Interrupt Controller.
> >>
> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >> ---
> >> v2:
> >>   - Add "renesas,gic-spi-base",
> >>   - Document RZ/A2M.
> >> ---
> >>  .../renesas,rza1-irqc.txt                     | 30 +++++++++++++++++++
> >>  1 file changed, 30 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> >> new file mode 100644
> >> index 0000000000000000..ea8ddb6955338ccd
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> >> @@ -0,0 +1,30 @@
> >> +DT bindings for the Renesas RZ/A1 Interrupt Controller
> >> +
> >> +The RZ/A1 Interrupt Controller is a front-end for the GIC found on Renesas
> >> +RZ/A1 and RZ/A2 SoCs:
> >> +  - IRQ sense select for 8 external interrupts, 1:1-mapped to 8 GIC SPI
> >> +    interrupts,
> >> +  - NMI edge select.
> >> +
> >> +Required properties:
> >> +  - compatible: Must be "renesas,<soctype>-irqc", and "renesas,rza1-irqc" as
> >> +               fallback.
> >> +               Examples with soctypes are:
> >> +                 - "renesas,r7s72100-irqc" (RZ/A1H)
> >> +                 - "renesas,r7s9210-irqc" (RZ/A2M)
> >> +  - #interrupt-cells: Must be 2 (an interrupt index and flags, as defined
> >> +                                in interrupts.txt in this directory)
> >> +  - interrupt-controller: Marks the device as an interrupt controller
> >> +  - reg: Base address and length of the memory resource used by the interrupt
> >> +         controller
> >> +  - renesas,gic-spi-base: Lowest GIC SPI interrupt number this block maps to.
> >
> > Why isn't this just an 'interrupts' property?
>
> That's likely because of kernel limitations. The DT code does an
> of_populate() on any device that it finds, parse the "interrupts"
> propertiy, resulting in the irq_descs being populated.
>
> That creates havoc, as these interrupts are not for this device, but for
> something that is connected to it. This is merely a bridge of some sort.

'interrupt-map' would avoid that problem I think.

> Furthermore, this is a rather long established practice: gic-v2m,
> gic-v3-mbi, mediatek,sysirq, mediatek,cirq... All the bits of glue that
> for one reason or another plug onto the GIC use the same method.

All handling the mapping to the parent in their own way...

> > Plus, without 'interrupts' walking the hierarchy is broken.
>
> Erm... Which hierarchy?

of_irq_init() expects that an interrupt-controller without an
interrupt-parent is the root controller. So you're right. We only need
to have an 'interrupt-parent', but not 'interrupts'.

Rob

Re: [PATCH v2 1/5] dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt Controller

[Rob Herring]

On Tue, Apr 30, 2019 at 10:34 AM Marc Zyngier <marc.zyngier@arm.com> wrote:
>
> On 30/04/2019 16:02, Rob Herring wrote:
> > On Tue, Apr 30, 2019 at 7:13 AM Geert Uytterhoeven
> > <geert+renesas@glider.be> wrote:
> >>
> >> Add DT bindings for the Renesas RZ/A1 Interrupt Controller.
> >>
> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >> ---
> >> v2:
> >>   - Add "renesas,gic-spi-base",
> >>   - Document RZ/A2M.
> >> ---
> >>  .../renesas,rza1-irqc.txt                     | 30 +++++++++++++++++++
> >>  1 file changed, 30 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> >> new file mode 100644
> >> index 0000000000000000..ea8ddb6955338ccd
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> >> @@ -0,0 +1,30 @@
> >> +DT bindings for the Renesas RZ/A1 Interrupt Controller
> >> +
> >> +The RZ/A1 Interrupt Controller is a front-end for the GIC found on Renesas
> >> +RZ/A1 and RZ/A2 SoCs:
> >> +  - IRQ sense select for 8 external interrupts, 1:1-mapped to 8 GIC SPI
> >> +    interrupts,
> >> +  - NMI edge select.
> >> +
> >> +Required properties:
> >> +  - compatible: Must be "renesas,<soctype>-irqc", and "renesas,rza1-irqc" as
> >> +               fallback.
> >> +               Examples with soctypes are:
> >> +                 - "renesas,r7s72100-irqc" (RZ/A1H)
> >> +                 - "renesas,r7s9210-irqc" (RZ/A2M)
> >> +  - #interrupt-cells: Must be 2 (an interrupt index and flags, as defined
> >> +                                in interrupts.txt in this directory)
> >> +  - interrupt-controller: Marks the device as an interrupt controller
> >> +  - reg: Base address and length of the memory resource used by the interrupt
> >> +         controller
> >> +  - renesas,gic-spi-base: Lowest GIC SPI interrupt number this block maps to.
> >
> > Why isn't this just an 'interrupts' property?
>
> That's likely because of kernel limitations. The DT code does an
> of_populate() on any device that it finds, parse the "interrupts"
> propertiy, resulting in the irq_descs being populated.
>
> That creates havoc, as these interrupts are not for this device, but for
> something that is connected to it. This is merely a bridge of some sort.

'interrupt-map' would avoid that problem I think.

> Furthermore, this is a rather long established practice: gic-v2m,
> gic-v3-mbi, mediatek,sysirq, mediatek,cirq... All the bits of glue that
> for one reason or another plug onto the GIC use the same method.

All handling the mapping to the parent in their own way...

> > Plus, without 'interrupts' walking the hierarchy is broken.
>
> Erm... Which hierarchy?

of_irq_init() expects that an interrupt-controller without an
interrupt-parent is the root controller. So you're right. We only need
to have an 'interrupt-parent', but not 'interrupts'.

Rob

Re: [PATCH v2 1/5] dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt Controller

[Geert Uytterhoeven]

Hi Rob,

On Tue, Apr 30, 2019 at 10:26 PM Rob Herring <robh+dt@kernel.org> wrote:
> On Tue, Apr 30, 2019 at 10:34 AM Marc Zyngier <marc.zyngier@arm.com> wrote:
> > On 30/04/2019 16:02, Rob Herring wrote:
> > > On Tue, Apr 30, 2019 at 7:13 AM Geert Uytterhoeven
> > > <geert+renesas@glider.be> wrote:
> > >>
> > >> Add DT bindings for the Renesas RZ/A1 Interrupt Controller.
> > >>
> > >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > >> ---
> > >> v2:
> > >>   - Add "renesas,gic-spi-base",
> > >>   - Document RZ/A2M.
> > >> ---
> > >>  .../renesas,rza1-irqc.txt                     | 30 +++++++++++++++++++
> > >>  1 file changed, 30 insertions(+)
> > >>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > >>
> > >> diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > >> new file mode 100644
> > >> index 0000000000000000..ea8ddb6955338ccd
> > >> --- /dev/null
> > >> +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > >> @@ -0,0 +1,30 @@
> > >> +DT bindings for the Renesas RZ/A1 Interrupt Controller
> > >> +
> > >> +The RZ/A1 Interrupt Controller is a front-end for the GIC found on Renesas
> > >> +RZ/A1 and RZ/A2 SoCs:
> > >> +  - IRQ sense select for 8 external interrupts, 1:1-mapped to 8 GIC SPI
> > >> +    interrupts,
> > >> +  - NMI edge select.
> > >> +
> > >> +Required properties:
> > >> +  - compatible: Must be "renesas,<soctype>-irqc", and "renesas,rza1-irqc" as
> > >> +               fallback.
> > >> +               Examples with soctypes are:
> > >> +                 - "renesas,r7s72100-irqc" (RZ/A1H)
> > >> +                 - "renesas,r7s9210-irqc" (RZ/A2M)
> > >> +  - #interrupt-cells: Must be 2 (an interrupt index and flags, as defined
> > >> +                                in interrupts.txt in this directory)
> > >> +  - interrupt-controller: Marks the device as an interrupt controller
> > >> +  - reg: Base address and length of the memory resource used by the interrupt
> > >> +         controller
> > >> +  - renesas,gic-spi-base: Lowest GIC SPI interrupt number this block maps to.
> > >
> > > Why isn't this just an 'interrupts' property?
> >
> > That's likely because of kernel limitations. The DT code does an
> > of_populate() on any device that it finds, parse the "interrupts"
> > propertiy, resulting in the irq_descs being populated.
> >
> > That creates havoc, as these interrupts are not for this device, but for
> > something that is connected to it. This is merely a bridge of some sort.
>
> 'interrupt-map' would avoid that problem I think.

"interrupt-map" seems to be meant for translation on a bus?
What to do with the child and parent unit addresses fields?
The parent unit address size depends on the #address-cells of the parent
interrupt-controller (i.e. GIC, so it's zero).
But the child unit address size depends on the #address-cells of the bus node
on which the child is located, so that's a (non-zero) bus #address-cells
(from the root node), not an interrupt-controller #address-cells.

Each line in an interrupt-map also contains a child interrupt specifier.
As the RZ/A1 IRQC supports 8 interrupt inputs with 4 sense types,
that would mean 32 lines? Or should I just ignore the senses here,
and specify 0?

i.e. interrupt-map = <0 0 0 &gic GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH,
                      0 1 0 &gic GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH,
                      0 2 0 &gic GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH,
                      0 3 0 &gic GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH,
                      0 4 0 &gic GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH,
                      0 5 0 &gic GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH,
                      0 6 0 &gic GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH,
                      0 7 0 &gic GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;

(using zero for the child unit addresses, too)?

> > Furthermore, this is a rather long established practice: gic-v2m,
> > gic-v3-mbi, mediatek,sysirq, mediatek,cirq... All the bits of glue that
> > for one reason or another plug onto the GIC use the same method.
>
> All handling the mapping to the parent in their own way...
>
> > > Plus, without 'interrupts' walking the hierarchy is broken.
> >
> > Erm... Which hierarchy?
>
> of_irq_init() expects that an interrupt-controller without an
> interrupt-parent is the root controller. So you're right. We only need

That applies to IRQCHIP_DECLARE() drivers only, not platform device
drivers, right?

> to have an 'interrupt-parent', but not 'interrupts'.

This is implied by "interrupt-parent = <&gic>;" on the soc node.

Thanks!

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: [PATCH v2 1/5] dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt Controller

[Geert Uytterhoeven]

Hi Rob,

On Tue, Apr 30, 2019 at 10:26 PM Rob Herring <robh+dt@kernel.org> wrote:
> On Tue, Apr 30, 2019 at 10:34 AM Marc Zyngier <marc.zyngier@arm.com> wrote:
> > On 30/04/2019 16:02, Rob Herring wrote:
> > > On Tue, Apr 30, 2019 at 7:13 AM Geert Uytterhoeven
> > > <geert+renesas@glider.be> wrote:
> > >>
> > >> Add DT bindings for the Renesas RZ/A1 Interrupt Controller.
> > >>
> > >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > >> ---
> > >> v2:
> > >>   - Add "renesas,gic-spi-base",
> > >>   - Document RZ/A2M.
> > >> ---
> > >>  .../renesas,rza1-irqc.txt                     | 30 +++++++++++++++++++
> > >>  1 file changed, 30 insertions(+)
> > >>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > >>
> > >> diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > >> new file mode 100644
> > >> index 0000000000000000..ea8ddb6955338ccd
> > >> --- /dev/null
> > >> +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > >> @@ -0,0 +1,30 @@
> > >> +DT bindings for the Renesas RZ/A1 Interrupt Controller
> > >> +
> > >> +The RZ/A1 Interrupt Controller is a front-end for the GIC found on Renesas
> > >> +RZ/A1 and RZ/A2 SoCs:
> > >> +  - IRQ sense select for 8 external interrupts, 1:1-mapped to 8 GIC SPI
> > >> +    interrupts,
> > >> +  - NMI edge select.
> > >> +
> > >> +Required properties:
> > >> +  - compatible: Must be "renesas,<soctype>-irqc", and "renesas,rza1-irqc" as
> > >> +               fallback.
> > >> +               Examples with soctypes are:
> > >> +                 - "renesas,r7s72100-irqc" (RZ/A1H)
> > >> +                 - "renesas,r7s9210-irqc" (RZ/A2M)
> > >> +  - #interrupt-cells: Must be 2 (an interrupt index and flags, as defined
> > >> +                                in interrupts.txt in this directory)
> > >> +  - interrupt-controller: Marks the device as an interrupt controller
> > >> +  - reg: Base address and length of the memory resource used by the interrupt
> > >> +         controller
> > >> +  - renesas,gic-spi-base: Lowest GIC SPI interrupt number this block maps to.
> > >
> > > Why isn't this just an 'interrupts' property?
> >
> > That's likely because of kernel limitations. The DT code does an
> > of_populate() on any device that it finds, parse the "interrupts"
> > propertiy, resulting in the irq_descs being populated.
> >
> > That creates havoc, as these interrupts are not for this device, but for
> > something that is connected to it. This is merely a bridge of some sort.
>
> 'interrupt-map' would avoid that problem I think.

"interrupt-map" seems to be meant for translation on a bus?
What to do with the child and parent unit addresses fields?
The parent unit address size depends on the #address-cells of the parent
interrupt-controller (i.e. GIC, so it's zero).
But the child unit address size depends on the #address-cells of the bus node
on which the child is located, so that's a (non-zero) bus #address-cells
(from the root node), not an interrupt-controller #address-cells.

Each line in an interrupt-map also contains a child interrupt specifier.
As the RZ/A1 IRQC supports 8 interrupt inputs with 4 sense types,
that would mean 32 lines? Or should I just ignore the senses here,
and specify 0?

i.e. interrupt-map = <0 0 0 &gic GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH,
                      0 1 0 &gic GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH,
                      0 2 0 &gic GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH,
                      0 3 0 &gic GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH,
                      0 4 0 &gic GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH,
                      0 5 0 &gic GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH,
                      0 6 0 &gic GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH,
                      0 7 0 &gic GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;

(using zero for the child unit addresses, too)?

> > Furthermore, this is a rather long established practice: gic-v2m,
> > gic-v3-mbi, mediatek,sysirq, mediatek,cirq... All the bits of glue that
> > for one reason or another plug onto the GIC use the same method.
>
> All handling the mapping to the parent in their own way...
>
> > > Plus, without 'interrupts' walking the hierarchy is broken.
> >
> > Erm... Which hierarchy?
>
> of_irq_init() expects that an interrupt-controller without an
> interrupt-parent is the root controller. So you're right. We only need

That applies to IRQCHIP_DECLARE() drivers only, not platform device
drivers, right?

> to have an 'interrupt-parent', but not 'interrupts'.

This is implied by "interrupt-parent = <&gic>;" on the soc node.

Thanks!

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: [PATCH v2 1/5] dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt Controller

[Rob Herring]

On Wed, May 1, 2019 at 2:16 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Rob,
>
> On Tue, Apr 30, 2019 at 10:26 PM Rob Herring <robh+dt@kernel.org> wrote:
> > On Tue, Apr 30, 2019 at 10:34 AM Marc Zyngier <marc.zyngier@arm.com> wrote:
> > > On 30/04/2019 16:02, Rob Herring wrote:
> > > > On Tue, Apr 30, 2019 at 7:13 AM Geert Uytterhoeven
> > > > <geert+renesas@glider.be> wrote:
> > > >>
> > > >> Add DT bindings for the Renesas RZ/A1 Interrupt Controller.
> > > >>
> > > >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > >> ---
> > > >> v2:
> > > >>   - Add "renesas,gic-spi-base",
> > > >>   - Document RZ/A2M.
> > > >> ---
> > > >>  .../renesas,rza1-irqc.txt                     | 30 +++++++++++++++++++
> > > >>  1 file changed, 30 insertions(+)
> > > >>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > > >>
> > > >> diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > > >> new file mode 100644
> > > >> index 0000000000000000..ea8ddb6955338ccd
> > > >> --- /dev/null
> > > >> +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > > >> @@ -0,0 +1,30 @@
> > > >> +DT bindings for the Renesas RZ/A1 Interrupt Controller
> > > >> +
> > > >> +The RZ/A1 Interrupt Controller is a front-end for the GIC found on Renesas
> > > >> +RZ/A1 and RZ/A2 SoCs:
> > > >> +  - IRQ sense select for 8 external interrupts, 1:1-mapped to 8 GIC SPI
> > > >> +    interrupts,
> > > >> +  - NMI edge select.
> > > >> +
> > > >> +Required properties:
> > > >> +  - compatible: Must be "renesas,<soctype>-irqc", and "renesas,rza1-irqc" as
> > > >> +               fallback.
> > > >> +               Examples with soctypes are:
> > > >> +                 - "renesas,r7s72100-irqc" (RZ/A1H)
> > > >> +                 - "renesas,r7s9210-irqc" (RZ/A2M)
> > > >> +  - #interrupt-cells: Must be 2 (an interrupt index and flags, as defined
> > > >> +                                in interrupts.txt in this directory)
> > > >> +  - interrupt-controller: Marks the device as an interrupt controller
> > > >> +  - reg: Base address and length of the memory resource used by the interrupt
> > > >> +         controller
> > > >> +  - renesas,gic-spi-base: Lowest GIC SPI interrupt number this block maps to.
> > > >
> > > > Why isn't this just an 'interrupts' property?
> > >
> > > That's likely because of kernel limitations. The DT code does an
> > > of_populate() on any device that it finds, parse the "interrupts"
> > > propertiy, resulting in the irq_descs being populated.
> > >
> > > That creates havoc, as these interrupts are not for this device, but for
> > > something that is connected to it. This is merely a bridge of some sort.
> >
> > 'interrupt-map' would avoid that problem I think.
>
> "interrupt-map" seems to be meant for translation on a bus?
> What to do with the child and parent unit addresses fields?
> The parent unit address size depends on the #address-cells of the parent
> interrupt-controller (i.e. GIC, so it's zero).
> But the child unit address size depends on the #address-cells of the bus node
> on which the child is located, so that's a (non-zero) bus #address-cells
> (from the root node), not an interrupt-controller #address-cells.

The #address-cells is always retrieved from the interrupt-parent node
(or its parent). The interrupt-parent can implicitly be the child's
parent, but that is rarely used in modern systems.

> Each line in an interrupt-map also contains a child interrupt specifier.
> As the RZ/A1 IRQC supports 8 interrupt inputs with 4 sense types,
> that would mean 32 lines? Or should I just ignore the senses here,
> and specify 0?

You can ignore parts of the child cells with interrupt-map-mask, so
you should just need 8 entries.

> i.e. interrupt-map = <0 0 0 &gic GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH,
>                       0 1 0 &gic GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH,
>                       0 2 0 &gic GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH,
>                       0 3 0 &gic GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH,
>                       0 4 0 &gic GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH,
>                       0 5 0 &gic GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH,
>                       0 6 0 &gic GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH,
>                       0 7 0 &gic GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
>
> (using zero for the child unit addresses, too)?
>
> > > Furthermore, this is a rather long established practice: gic-v2m,
> > > gic-v3-mbi, mediatek,sysirq, mediatek,cirq... All the bits of glue that
> > > for one reason or another plug onto the GIC use the same method.
> >
> > All handling the mapping to the parent in their own way...
> >
> > > > Plus, without 'interrupts' walking the hierarchy is broken.
> > >
> > > Erm... Which hierarchy?
> >
> > of_irq_init() expects that an interrupt-controller without an
> > interrupt-parent is the root controller. So you're right. We only need
>
> That applies to IRQCHIP_DECLARE() drivers only, not platform device
> drivers, right?

Right.

Rob

Re: [PATCH v2 1/5] dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt Controller

[Rob Herring]

On Wed, May 1, 2019 at 2:16 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Rob,
>
> On Tue, Apr 30, 2019 at 10:26 PM Rob Herring <robh+dt@kernel.org> wrote:
> > On Tue, Apr 30, 2019 at 10:34 AM Marc Zyngier <marc.zyngier@arm.com> wrote:
> > > On 30/04/2019 16:02, Rob Herring wrote:
> > > > On Tue, Apr 30, 2019 at 7:13 AM Geert Uytterhoeven
> > > > <geert+renesas@glider.be> wrote:
> > > >>
> > > >> Add DT bindings for the Renesas RZ/A1 Interrupt Controller.
> > > >>
> > > >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > >> ---
> > > >> v2:
> > > >>   - Add "renesas,gic-spi-base",
> > > >>   - Document RZ/A2M.
> > > >> ---
> > > >>  .../renesas,rza1-irqc.txt                     | 30 +++++++++++++++++++
> > > >>  1 file changed, 30 insertions(+)
> > > >>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > > >>
> > > >> diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > > >> new file mode 100644
> > > >> index 0000000000000000..ea8ddb6955338ccd
> > > >> --- /dev/null
> > > >> +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > > >> @@ -0,0 +1,30 @@
> > > >> +DT bindings for the Renesas RZ/A1 Interrupt Controller
> > > >> +
> > > >> +The RZ/A1 Interrupt Controller is a front-end for the GIC found on Renesas
> > > >> +RZ/A1 and RZ/A2 SoCs:
> > > >> +  - IRQ sense select for 8 external interrupts, 1:1-mapped to 8 GIC SPI
> > > >> +    interrupts,
> > > >> +  - NMI edge select.
> > > >> +
> > > >> +Required properties:
> > > >> +  - compatible: Must be "renesas,<soctype>-irqc", and "renesas,rza1-irqc" as
> > > >> +               fallback.
> > > >> +               Examples with soctypes are:
> > > >> +                 - "renesas,r7s72100-irqc" (RZ/A1H)
> > > >> +                 - "renesas,r7s9210-irqc" (RZ/A2M)
> > > >> +  - #interrupt-cells: Must be 2 (an interrupt index and flags, as defined
> > > >> +                                in interrupts.txt in this directory)
> > > >> +  - interrupt-controller: Marks the device as an interrupt controller
> > > >> +  - reg: Base address and length of the memory resource used by the interrupt
> > > >> +         controller
> > > >> +  - renesas,gic-spi-base: Lowest GIC SPI interrupt number this block maps to.
> > > >
> > > > Why isn't this just an 'interrupts' property?
> > >
> > > That's likely because of kernel limitations. The DT code does an
> > > of_populate() on any device that it finds, parse the "interrupts"
> > > propertiy, resulting in the irq_descs being populated.
> > >
> > > That creates havoc, as these interrupts are not for this device, but for
> > > something that is connected to it. This is merely a bridge of some sort.
> >
> > 'interrupt-map' would avoid that problem I think.
>
> "interrupt-map" seems to be meant for translation on a bus?
> What to do with the child and parent unit addresses fields?
> The parent unit address size depends on the #address-cells of the parent
> interrupt-controller (i.e. GIC, so it's zero).
> But the child unit address size depends on the #address-cells of the bus node
> on which the child is located, so that's a (non-zero) bus #address-cells
> (from the root node), not an interrupt-controller #address-cells.

The #address-cells is always retrieved from the interrupt-parent node
(or its parent). The interrupt-parent can implicitly be the child's
parent, but that is rarely used in modern systems.

> Each line in an interrupt-map also contains a child interrupt specifier.
> As the RZ/A1 IRQC supports 8 interrupt inputs with 4 sense types,
> that would mean 32 lines? Or should I just ignore the senses here,
> and specify 0?

You can ignore parts of the child cells with interrupt-map-mask, so
you should just need 8 entries.

> i.e. interrupt-map = <0 0 0 &gic GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH,
>                       0 1 0 &gic GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH,
>                       0 2 0 &gic GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH,
>                       0 3 0 &gic GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH,
>                       0 4 0 &gic GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH,
>                       0 5 0 &gic GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH,
>                       0 6 0 &gic GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH,
>                       0 7 0 &gic GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
>
> (using zero for the child unit addresses, too)?
>
> > > Furthermore, this is a rather long established practice: gic-v2m,
> > > gic-v3-mbi, mediatek,sysirq, mediatek,cirq... All the bits of glue that
> > > for one reason or another plug onto the GIC use the same method.
> >
> > All handling the mapping to the parent in their own way...
> >
> > > > Plus, without 'interrupts' walking the hierarchy is broken.
> > >
> > > Erm... Which hierarchy?
> >
> > of_irq_init() expects that an interrupt-controller without an
> > interrupt-parent is the root controller. So you're right. We only need
>
> That applies to IRQCHIP_DECLARE() drivers only, not platform device
> drivers, right?

Right.

Rob

Re: [PATCH v2 1/5] dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt Controller

[Geert Uytterhoeven]

Hi Rob,

On Wed, May 1, 2019 at 9:38 PM Rob Herring <robh+dt@kernel.org> wrote:
> On Wed, May 1, 2019 at 2:16 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Tue, Apr 30, 2019 at 10:26 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > On Tue, Apr 30, 2019 at 10:34 AM Marc Zyngier <marc.zyngier@arm.com> wrote:
> > > > On 30/04/2019 16:02, Rob Herring wrote:
> > > > > On Tue, Apr 30, 2019 at 7:13 AM Geert Uytterhoeven
> > > > > <geert+renesas@glider.be> wrote:
> > > > >>
> > > > >> Add DT bindings for the Renesas RZ/A1 Interrupt Controller.
> > > > >>
> > > > >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > >> ---
> > > > >> v2:
> > > > >>   - Add "renesas,gic-spi-base",
> > > > >>   - Document RZ/A2M.
> > > > >> ---
> > > > >>  .../renesas,rza1-irqc.txt                     | 30 +++++++++++++++++++
> > > > >>  1 file changed, 30 insertions(+)
> > > > >>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > > > >>
> > > > >> diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > > > >> new file mode 100644
> > > > >> index 0000000000000000..ea8ddb6955338ccd
> > > > >> --- /dev/null
> > > > >> +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > > > >> @@ -0,0 +1,30 @@
> > > > >> +DT bindings for the Renesas RZ/A1 Interrupt Controller
> > > > >> +
> > > > >> +The RZ/A1 Interrupt Controller is a front-end for the GIC found on Renesas
> > > > >> +RZ/A1 and RZ/A2 SoCs:
> > > > >> +  - IRQ sense select for 8 external interrupts, 1:1-mapped to 8 GIC SPI
> > > > >> +    interrupts,
> > > > >> +  - NMI edge select.
> > > > >> +
> > > > >> +Required properties:
> > > > >> +  - compatible: Must be "renesas,<soctype>-irqc", and "renesas,rza1-irqc" as
> > > > >> +               fallback.
> > > > >> +               Examples with soctypes are:
> > > > >> +                 - "renesas,r7s72100-irqc" (RZ/A1H)
> > > > >> +                 - "renesas,r7s9210-irqc" (RZ/A2M)
> > > > >> +  - #interrupt-cells: Must be 2 (an interrupt index and flags, as defined
> > > > >> +                                in interrupts.txt in this directory)
> > > > >> +  - interrupt-controller: Marks the device as an interrupt controller
> > > > >> +  - reg: Base address and length of the memory resource used by the interrupt
> > > > >> +         controller
> > > > >> +  - renesas,gic-spi-base: Lowest GIC SPI interrupt number this block maps to.
> > > > >
> > > > > Why isn't this just an 'interrupts' property?
> > > >
> > > > That's likely because of kernel limitations. The DT code does an
> > > > of_populate() on any device that it finds, parse the "interrupts"
> > > > propertiy, resulting in the irq_descs being populated.
> > > >
> > > > That creates havoc, as these interrupts are not for this device, but for
> > > > something that is connected to it. This is merely a bridge of some sort.
> > >
> > > 'interrupt-map' would avoid that problem I think.
> >
> > "interrupt-map" seems to be meant for translation on a bus?
> > What to do with the child and parent unit addresses fields?
> > The parent unit address size depends on the #address-cells of the parent
> > interrupt-controller (i.e. GIC, so it's zero).
> > But the child unit address size depends on the #address-cells of the bus node
> > on which the child is located, so that's a (non-zero) bus #address-cells
> > (from the root node), not an interrupt-controller #address-cells.
>
> The #address-cells is always retrieved from the interrupt-parent node
> (or its parent). The interrupt-parent can implicitly be the child's
> parent, but that is rarely used in modern systems.

That's not what Devicetree Specification, Release v0.2 says:

    child unit address The unit address of the child node being mapped.
    The number of 32-bit cells required to specify this is described by
    the #address-cells property of the bus node on which the child is
    located.

2.4.4 Interrupt Mapping Example (for PCI) says the bus node is the PCI
bridge, with #address-cells = <3>.

But in the RZ/A1 case the child unit address is irrelevant, as its an
external interrupt input not related to a specific bus.  It could be
used by a device without unit address (e.g. gpio-keys), or some device
on an external local bus (root #adress-cells is <1> on 32-bit without
LPAE, but this block could be reused in a future LPAE or arm64 SoCs),
or on e.g. an SPI or i2c bus, with its own #adress-cells value
(coincidentally <1>, too).

I see of_irq_parse_raw() does use the address-cells of the parent
interrupt controller (which is usually 0) when iterating its way up,
following interrupt-map.

So the child unit address does have two different meanings?

> > Each line in an interrupt-map also contains a child interrupt specifier.
> > As the RZ/A1 IRQC supports 8 interrupt inputs with 4 sense types,
> > that would mean 32 lines? Or should I just ignore the senses here,
> > and specify 0?
>
> You can ignore parts of the child cells with interrupt-map-mask, so
> you should just need 8 entries.

Right, thanks.



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: [PATCH v2 1/5] dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt Controller

[Geert Uytterhoeven]

Hi Rob,

On Wed, May 1, 2019 at 9:38 PM Rob Herring <robh+dt@kernel.org> wrote:
> On Wed, May 1, 2019 at 2:16 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Tue, Apr 30, 2019 at 10:26 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > On Tue, Apr 30, 2019 at 10:34 AM Marc Zyngier <marc.zyngier@arm.com> wrote:
> > > > On 30/04/2019 16:02, Rob Herring wrote:
> > > > > On Tue, Apr 30, 2019 at 7:13 AM Geert Uytterhoeven
> > > > > <geert+renesas@glider.be> wrote:
> > > > >>
> > > > >> Add DT bindings for the Renesas RZ/A1 Interrupt Controller.
> > > > >>
> > > > >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > >> ---
> > > > >> v2:
> > > > >>   - Add "renesas,gic-spi-base",
> > > > >>   - Document RZ/A2M.
> > > > >> ---
> > > > >>  .../renesas,rza1-irqc.txt                     | 30 +++++++++++++++++++
> > > > >>  1 file changed, 30 insertions(+)
> > > > >>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > > > >>
> > > > >> diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > > > >> new file mode 100644
> > > > >> index 0000000000000000..ea8ddb6955338ccd
> > > > >> --- /dev/null
> > > > >> +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > > > >> @@ -0,0 +1,30 @@
> > > > >> +DT bindings for the Renesas RZ/A1 Interrupt Controller
> > > > >> +
> > > > >> +The RZ/A1 Interrupt Controller is a front-end for the GIC found on Renesas
> > > > >> +RZ/A1 and RZ/A2 SoCs:
> > > > >> +  - IRQ sense select for 8 external interrupts, 1:1-mapped to 8 GIC SPI
> > > > >> +    interrupts,
> > > > >> +  - NMI edge select.
> > > > >> +
> > > > >> +Required properties:
> > > > >> +  - compatible: Must be "renesas,<soctype>-irqc", and "renesas,rza1-irqc" as
> > > > >> +               fallback.
> > > > >> +               Examples with soctypes are:
> > > > >> +                 - "renesas,r7s72100-irqc" (RZ/A1H)
> > > > >> +                 - "renesas,r7s9210-irqc" (RZ/A2M)
> > > > >> +  - #interrupt-cells: Must be 2 (an interrupt index and flags, as defined
> > > > >> +                                in interrupts.txt in this directory)
> > > > >> +  - interrupt-controller: Marks the device as an interrupt controller
> > > > >> +  - reg: Base address and length of the memory resource used by the interrupt
> > > > >> +         controller
> > > > >> +  - renesas,gic-spi-base: Lowest GIC SPI interrupt number this block maps to.
> > > > >
> > > > > Why isn't this just an 'interrupts' property?
> > > >
> > > > That's likely because of kernel limitations. The DT code does an
> > > > of_populate() on any device that it finds, parse the "interrupts"
> > > > propertiy, resulting in the irq_descs being populated.
> > > >
> > > > That creates havoc, as these interrupts are not for this device, but for
> > > > something that is connected to it. This is merely a bridge of some sort.
> > >
> > > 'interrupt-map' would avoid that problem I think.
> >
> > "interrupt-map" seems to be meant for translation on a bus?
> > What to do with the child and parent unit addresses fields?
> > The parent unit address size depends on the #address-cells of the parent
> > interrupt-controller (i.e. GIC, so it's zero).
> > But the child unit address size depends on the #address-cells of the bus node
> > on which the child is located, so that's a (non-zero) bus #address-cells
> > (from the root node), not an interrupt-controller #address-cells.
>
> The #address-cells is always retrieved from the interrupt-parent node
> (or its parent). The interrupt-parent can implicitly be the child's
> parent, but that is rarely used in modern systems.

That's not what Devicetree Specification, Release v0.2 says:

    child unit address The unit address of the child node being mapped.
    The number of 32-bit cells required to specify this is described by
    the #address-cells property of the bus node on which the child is
    located.

2.4.4 Interrupt Mapping Example (for PCI) says the bus node is the PCI
bridge, with #address-cells = <3>.

But in the RZ/A1 case the child unit address is irrelevant, as its an
external interrupt input not related to a specific bus.  It could be
used by a device without unit address (e.g. gpio-keys), or some device
on an external local bus (root #adress-cells is <1> on 32-bit without
LPAE, but this block could be reused in a future LPAE or arm64 SoCs),
or on e.g. an SPI or i2c bus, with its own #adress-cells value
(coincidentally <1>, too).

I see of_irq_parse_raw() does use the address-cells of the parent
interrupt controller (which is usually 0) when iterating its way up,
following interrupt-map.

So the child unit address does have two different meanings?

> > Each line in an interrupt-map also contains a child interrupt specifier.
> > As the RZ/A1 IRQC supports 8 interrupt inputs with 4 sense types,
> > that would mean 32 lines? Or should I just ignore the senses here,
> > and specify 0?
>
> You can ignore parts of the child cells with interrupt-map-mask, so
> you should just need 8 entries.

Right, thanks.



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: [PATCH v2 1/5] dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt Controller

[Rob Herring]

On Thu, May 2, 2019 at 5:01 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Rob,
>
> On Wed, May 1, 2019 at 9:38 PM Rob Herring <robh+dt@kernel.org> wrote:
> > On Wed, May 1, 2019 at 2:16 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Tue, Apr 30, 2019 at 10:26 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > > On Tue, Apr 30, 2019 at 10:34 AM Marc Zyngier <marc.zyngier@arm.com> wrote:
> > > > > On 30/04/2019 16:02, Rob Herring wrote:
> > > > > > On Tue, Apr 30, 2019 at 7:13 AM Geert Uytterhoeven
> > > > > > <geert+renesas@glider.be> wrote:
> > > > > >>
> > > > > >> Add DT bindings for the Renesas RZ/A1 Interrupt Controller.
> > > > > >>
> > > > > >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > > >> ---
> > > > > >> v2:
> > > > > >>   - Add "renesas,gic-spi-base",
> > > > > >>   - Document RZ/A2M.
> > > > > >> ---
> > > > > >>  .../renesas,rza1-irqc.txt                     | 30 +++++++++++++++++++
> > > > > >>  1 file changed, 30 insertions(+)
> > > > > >>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > > > > >>
> > > > > >> diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > > > > >> new file mode 100644
> > > > > >> index 0000000000000000..ea8ddb6955338ccd
> > > > > >> --- /dev/null
> > > > > >> +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > > > > >> @@ -0,0 +1,30 @@
> > > > > >> +DT bindings for the Renesas RZ/A1 Interrupt Controller
> > > > > >> +
> > > > > >> +The RZ/A1 Interrupt Controller is a front-end for the GIC found on Renesas
> > > > > >> +RZ/A1 and RZ/A2 SoCs:
> > > > > >> +  - IRQ sense select for 8 external interrupts, 1:1-mapped to 8 GIC SPI
> > > > > >> +    interrupts,
> > > > > >> +  - NMI edge select.
> > > > > >> +
> > > > > >> +Required properties:
> > > > > >> +  - compatible: Must be "renesas,<soctype>-irqc", and "renesas,rza1-irqc" as
> > > > > >> +               fallback.
> > > > > >> +               Examples with soctypes are:
> > > > > >> +                 - "renesas,r7s72100-irqc" (RZ/A1H)
> > > > > >> +                 - "renesas,r7s9210-irqc" (RZ/A2M)
> > > > > >> +  - #interrupt-cells: Must be 2 (an interrupt index and flags, as defined
> > > > > >> +                                in interrupts.txt in this directory)
> > > > > >> +  - interrupt-controller: Marks the device as an interrupt controller
> > > > > >> +  - reg: Base address and length of the memory resource used by the interrupt
> > > > > >> +         controller
> > > > > >> +  - renesas,gic-spi-base: Lowest GIC SPI interrupt number this block maps to.
> > > > > >
> > > > > > Why isn't this just an 'interrupts' property?
> > > > >
> > > > > That's likely because of kernel limitations. The DT code does an
> > > > > of_populate() on any device that it finds, parse the "interrupts"
> > > > > propertiy, resulting in the irq_descs being populated.
> > > > >
> > > > > That creates havoc, as these interrupts are not for this device, but for
> > > > > something that is connected to it. This is merely a bridge of some sort.
> > > >
> > > > 'interrupt-map' would avoid that problem I think.
> > >
> > > "interrupt-map" seems to be meant for translation on a bus?
> > > What to do with the child and parent unit addresses fields?
> > > The parent unit address size depends on the #address-cells of the parent
> > > interrupt-controller (i.e. GIC, so it's zero).
> > > But the child unit address size depends on the #address-cells of the bus node
> > > on which the child is located, so that's a (non-zero) bus #address-cells
> > > (from the root node), not an interrupt-controller #address-cells.
> >
> > The #address-cells is always retrieved from the interrupt-parent node
> > (or its parent). The interrupt-parent can implicitly be the child's
> > parent, but that is rarely used in modern systems.
>
> That's not what Devicetree Specification, Release v0.2 says:
>
>     child unit address The unit address of the child node being mapped.
>     The number of 32-bit cells required to specify this is described by
>     the #address-cells property of the bus node on which the child is
>     located.
>
> 2.4.4 Interrupt Mapping Example (for PCI) says the bus node is the PCI
> bridge, with #address-cells = <3>.

PCI is more inline with the spec wording, but systems evolved where
the interrupt hierarchy doesn't match the bus hierarchy.

> But in the RZ/A1 case the child unit address is irrelevant, as its an
> external interrupt input not related to a specific bus.  It could be
> used by a device without unit address (e.g. gpio-keys), or some device
> on an external local bus (root #adress-cells is <1> on 32-bit without
> LPAE, but this block could be reused in a future LPAE or arm64 SoCs),
> or on e.g. an SPI or i2c bus, with its own #adress-cells value
> (coincidentally <1>, too).
>
> I see of_irq_parse_raw() does use the address-cells of the parent
> interrupt controller (which is usually 0) when iterating its way up,
> following interrupt-map.
>
> So the child unit address does have two different meanings?

Indeed. That's why you'll see interrupt-controller nodes with the odd
'#address-cells = <0>;' in them. It's often omitted because it only
matters if there's an interrupt-map. We should clarify the spec.

Rob

Re: [PATCH v2 1/5] dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt Controller

[Rob Herring]

On Thu, May 2, 2019 at 5:01 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Rob,
>
> On Wed, May 1, 2019 at 9:38 PM Rob Herring <robh+dt@kernel.org> wrote:
> > On Wed, May 1, 2019 at 2:16 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Tue, Apr 30, 2019 at 10:26 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > > On Tue, Apr 30, 2019 at 10:34 AM Marc Zyngier <marc.zyngier@arm.com> wrote:
> > > > > On 30/04/2019 16:02, Rob Herring wrote:
> > > > > > On Tue, Apr 30, 2019 at 7:13 AM Geert Uytterhoeven
> > > > > > <geert+renesas@glider.be> wrote:
> > > > > >>
> > > > > >> Add DT bindings for the Renesas RZ/A1 Interrupt Controller.
> > > > > >>
> > > > > >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > > >> ---
> > > > > >> v2:
> > > > > >>   - Add "renesas,gic-spi-base",
> > > > > >>   - Document RZ/A2M.
> > > > > >> ---
> > > > > >>  .../renesas,rza1-irqc.txt                     | 30 +++++++++++++++++++
> > > > > >>  1 file changed, 30 insertions(+)
> > > > > >>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > > > > >>
> > > > > >> diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > > > > >> new file mode 100644
> > > > > >> index 0000000000000000..ea8ddb6955338ccd
> > > > > >> --- /dev/null
> > > > > >> +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> > > > > >> @@ -0,0 +1,30 @@
> > > > > >> +DT bindings for the Renesas RZ/A1 Interrupt Controller
> > > > > >> +
> > > > > >> +The RZ/A1 Interrupt Controller is a front-end for the GIC found on Renesas
> > > > > >> +RZ/A1 and RZ/A2 SoCs:
> > > > > >> +  - IRQ sense select for 8 external interrupts, 1:1-mapped to 8 GIC SPI
> > > > > >> +    interrupts,
> > > > > >> +  - NMI edge select.
> > > > > >> +
> > > > > >> +Required properties:
> > > > > >> +  - compatible: Must be "renesas,<soctype>-irqc", and "renesas,rza1-irqc" as
> > > > > >> +               fallback.
> > > > > >> +               Examples with soctypes are:
> > > > > >> +                 - "renesas,r7s72100-irqc" (RZ/A1H)
> > > > > >> +                 - "renesas,r7s9210-irqc" (RZ/A2M)
> > > > > >> +  - #interrupt-cells: Must be 2 (an interrupt index and flags, as defined
> > > > > >> +                                in interrupts.txt in this directory)
> > > > > >> +  - interrupt-controller: Marks the device as an interrupt controller
> > > > > >> +  - reg: Base address and length of the memory resource used by the interrupt
> > > > > >> +         controller
> > > > > >> +  - renesas,gic-spi-base: Lowest GIC SPI interrupt number this block maps to.
> > > > > >
> > > > > > Why isn't this just an 'interrupts' property?
> > > > >
> > > > > That's likely because of kernel limitations. The DT code does an
> > > > > of_populate() on any device that it finds, parse the "interrupts"
> > > > > propertiy, resulting in the irq_descs being populated.
> > > > >
> > > > > That creates havoc, as these interrupts are not for this device, but for
> > > > > something that is connected to it. This is merely a bridge of some sort.
> > > >
> > > > 'interrupt-map' would avoid that problem I think.
> > >
> > > "interrupt-map" seems to be meant for translation on a bus?
> > > What to do with the child and parent unit addresses fields?
> > > The parent unit address size depends on the #address-cells of the parent
> > > interrupt-controller (i.e. GIC, so it's zero).
> > > But the child unit address size depends on the #address-cells of the bus node
> > > on which the child is located, so that's a (non-zero) bus #address-cells
> > > (from the root node), not an interrupt-controller #address-cells.
> >
> > The #address-cells is always retrieved from the interrupt-parent node
> > (or its parent). The interrupt-parent can implicitly be the child's
> > parent, but that is rarely used in modern systems.
>
> That's not what Devicetree Specification, Release v0.2 says:
>
>     child unit address The unit address of the child node being mapped.
>     The number of 32-bit cells required to specify this is described by
>     the #address-cells property of the bus node on which the child is
>     located.
>
> 2.4.4 Interrupt Mapping Example (for PCI) says the bus node is the PCI
> bridge, with #address-cells = <3>.

PCI is more inline with the spec wording, but systems evolved where
the interrupt hierarchy doesn't match the bus hierarchy.

> But in the RZ/A1 case the child unit address is irrelevant, as its an
> external interrupt input not related to a specific bus.  It could be
> used by a device without unit address (e.g. gpio-keys), or some device
> on an external local bus (root #adress-cells is <1> on 32-bit without
> LPAE, but this block could be reused in a future LPAE or arm64 SoCs),
> or on e.g. an SPI or i2c bus, with its own #adress-cells value
> (coincidentally <1>, too).
>
> I see of_irq_parse_raw() does use the address-cells of the parent
> interrupt controller (which is usually 0) when iterating its way up,
> following interrupt-map.
>
> So the child unit address does have two different meanings?

Indeed. That's why you'll see interrupt-controller nodes with the odd
'#address-cells = <0>;' in them. It's often omitted because it only
matters if there's an interrupt-map. We should clarify the spec.

Rob

Re: [PATCH v2 1/5] dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt Controller

[Geert Uytterhoeven]

Hi Rob,

On Thu, May 2, 2019 at 6:51 PM Rob Herring <robh+dt@kernel.org> wrote:
> On Thu, May 2, 2019 at 5:01 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, May 1, 2019 at 9:38 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > On Wed, May 1, 2019 at 2:16 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Tue, Apr 30, 2019 at 10:26 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > > > 'interrupt-map' would avoid that problem I think.
> > > >
> > > > "interrupt-map" seems to be meant for translation on a bus?
> > > > What to do with the child and parent unit addresses fields?
> > > > The parent unit address size depends on the #address-cells of the parent
> > > > interrupt-controller (i.e. GIC, so it's zero).
> > > > But the child unit address size depends on the #address-cells of the bus node
> > > > on which the child is located, so that's a (non-zero) bus #address-cells
> > > > (from the root node), not an interrupt-controller #address-cells.
> > >
> > > The #address-cells is always retrieved from the interrupt-parent node
> > > (or its parent). The interrupt-parent can implicitly be the child's
> > > parent, but that is rarely used in modern systems.
> >
> > That's not what Devicetree Specification, Release v0.2 says:
> >
> >     child unit address The unit address of the child node being mapped.
> >     The number of 32-bit cells required to specify this is described by
> >     the #address-cells property of the bus node on which the child is
> >     located.
> >
> > 2.4.4 Interrupt Mapping Example (for PCI) says the bus node is the PCI
> > bridge, with #address-cells = <3>.
>
> PCI is more inline with the spec wording, but systems evolved where
> the interrupt hierarchy doesn't match the bus hierarchy.

OK.

> > But in the RZ/A1 case the child unit address is irrelevant, as its an
> > external interrupt input not related to a specific bus.  It could be
> > used by a device without unit address (e.g. gpio-keys), or some device
> > on an external local bus (root #adress-cells is <1> on 32-bit without
> > LPAE, but this block could be reused in a future LPAE or arm64 SoCs),
> > or on e.g. an SPI or i2c bus, with its own #adress-cells value
> > (coincidentally <1>, too).
> >
> > I see of_irq_parse_raw() does use the address-cells of the parent
> > interrupt controller (which is usually 0) when iterating its way up,
> > following interrupt-map.
> >
> > So the child unit address does have two different meanings?
>
> Indeed. That's why you'll see interrupt-controller nodes with the odd
> '#address-cells = <0>;' in them. It's often omitted because it only
> matters if there's an interrupt-map. We should clarify the spec.

Yeah, I had noticed that, but didn't want to dive too deep into that
(at that time).  I always assumed it was some silly mistake, combined
with dtsi cargo cult copying.  Thanks, now I know better....

BTW, the GIC bindings don't help that much: #address-cells can be
0, 1, or 2, #size-cells can be 1 or 2. No explanation why...

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: [PATCH v2 1/5] dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt Controller

[Geert Uytterhoeven]

Hi Rob,

On Thu, May 2, 2019 at 6:51 PM Rob Herring <robh+dt@kernel.org> wrote:
> On Thu, May 2, 2019 at 5:01 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, May 1, 2019 at 9:38 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > On Wed, May 1, 2019 at 2:16 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Tue, Apr 30, 2019 at 10:26 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > > > 'interrupt-map' would avoid that problem I think.
> > > >
> > > > "interrupt-map" seems to be meant for translation on a bus?
> > > > What to do with the child and parent unit addresses fields?
> > > > The parent unit address size depends on the #address-cells of the parent
> > > > interrupt-controller (i.e. GIC, so it's zero).
> > > > But the child unit address size depends on the #address-cells of the bus node
> > > > on which the child is located, so that's a (non-zero) bus #address-cells
> > > > (from the root node), not an interrupt-controller #address-cells.
> > >
> > > The #address-cells is always retrieved from the interrupt-parent node
> > > (or its parent). The interrupt-parent can implicitly be the child's
> > > parent, but that is rarely used in modern systems.
> >
> > That's not what Devicetree Specification, Release v0.2 says:
> >
> >     child unit address The unit address of the child node being mapped.
> >     The number of 32-bit cells required to specify this is described by
> >     the #address-cells property of the bus node on which the child is
> >     located.
> >
> > 2.4.4 Interrupt Mapping Example (for PCI) says the bus node is the PCI
> > bridge, with #address-cells = <3>.
>
> PCI is more inline with the spec wording, but systems evolved where
> the interrupt hierarchy doesn't match the bus hierarchy.

OK.

> > But in the RZ/A1 case the child unit address is irrelevant, as its an
> > external interrupt input not related to a specific bus.  It could be
> > used by a device without unit address (e.g. gpio-keys), or some device
> > on an external local bus (root #adress-cells is <1> on 32-bit without
> > LPAE, but this block could be reused in a future LPAE or arm64 SoCs),
> > or on e.g. an SPI or i2c bus, with its own #adress-cells value
> > (coincidentally <1>, too).
> >
> > I see of_irq_parse_raw() does use the address-cells of the parent
> > interrupt controller (which is usually 0) when iterating its way up,
> > following interrupt-map.
> >
> > So the child unit address does have two different meanings?
>
> Indeed. That's why you'll see interrupt-controller nodes with the odd
> '#address-cells = <0>;' in them. It's often omitted because it only
> matters if there's an interrupt-map. We should clarify the spec.

Yeah, I had noticed that, but didn't want to dive too deep into that
(at that time).  I always assumed it was some silly mistake, combined
with dtsi cargo cult copying.  Thanks, now I know better....

BTW, the GIC bindings don't help that much: #address-cells can be
0, 1, or 2, #size-cells can be 1 or 2. No explanation why...

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: [PATCH v2 1/5] dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt Controller

[Marc Zyngier]

On 30/04/2019 21:25, Rob Herring wrote:
> On Tue, Apr 30, 2019 at 10:34 AM Marc Zyngier <marc.zyngier@arm.com> wrote:
>>
>> On 30/04/2019 16:02, Rob Herring wrote:
>>> On Tue, Apr 30, 2019 at 7:13 AM Geert Uytterhoeven
>>> <geert+renesas@glider.be> wrote:
>>>>
>>>> Add DT bindings for the Renesas RZ/A1 Interrupt Controller.
>>>>
>>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>> ---
>>>> v2:
>>>>   - Add "renesas,gic-spi-base",
>>>>   - Document RZ/A2M.
>>>> ---
>>>>  .../renesas,rza1-irqc.txt                     | 30 +++++++++++++++++++
>>>>  1 file changed, 30 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
>>>> new file mode 100644
>>>> index 0000000000000000..ea8ddb6955338ccd
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
>>>> @@ -0,0 +1,30 @@
>>>> +DT bindings for the Renesas RZ/A1 Interrupt Controller
>>>> +
>>>> +The RZ/A1 Interrupt Controller is a front-end for the GIC found on Renesas
>>>> +RZ/A1 and RZ/A2 SoCs:
>>>> +  - IRQ sense select for 8 external interrupts, 1:1-mapped to 8 GIC SPI
>>>> +    interrupts,
>>>> +  - NMI edge select.
>>>> +
>>>> +Required properties:
>>>> +  - compatible: Must be "renesas,<soctype>-irqc", and "renesas,rza1-irqc" as
>>>> +               fallback.
>>>> +               Examples with soctypes are:
>>>> +                 - "renesas,r7s72100-irqc" (RZ/A1H)
>>>> +                 - "renesas,r7s9210-irqc" (RZ/A2M)
>>>> +  - #interrupt-cells: Must be 2 (an interrupt index and flags, as defined
>>>> +                                in interrupts.txt in this directory)
>>>> +  - interrupt-controller: Marks the device as an interrupt controller
>>>> +  - reg: Base address and length of the memory resource used by the interrupt
>>>> +         controller
>>>> +  - renesas,gic-spi-base: Lowest GIC SPI interrupt number this block maps to.
>>>
>>> Why isn't this just an 'interrupts' property?
>>
>> That's likely because of kernel limitations. The DT code does an
>> of_populate() on any device that it finds, parse the "interrupts"
>> propertiy, resulting in the irq_descs being populated.
>>
>> That creates havoc, as these interrupts are not for this device, but for
>> something that is connected to it. This is merely a bridge of some sort.
> 
> 'interrupt-map' would avoid that problem I think.

I'm afraid it doesn't scale at all. Case in point, the GICv3 ITS. Up to
32bit worth of IDs. How do you represent that using an interrupt-map?
Agreed, that's the extreme case, but representing more than a handful of
interrupts using an interrupt-map is a pain.

> 
>> Furthermore, this is a rather long established practice: gic-v2m,
>> gic-v3-mbi, mediatek,sysirq, mediatek,cirq... All the bits of glue that
>> for one reason or another plug onto the GIC use the same method.
> 
> All handling the mapping to the parent in their own way...

Yes, and that's the problem. We need a scalable way to describe ranges
of interrupts that are "forwarded" (for the lack of a better term), but
now "owned" by the the interrupt controller.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 1/5] dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt Controller

[Marc Zyngier]

On 30/04/2019 21:25, Rob Herring wrote:
> On Tue, Apr 30, 2019 at 10:34 AM Marc Zyngier <marc.zyngier@arm.com> wrote:
>>
>> On 30/04/2019 16:02, Rob Herring wrote:
>>> On Tue, Apr 30, 2019 at 7:13 AM Geert Uytterhoeven
>>> <geert+renesas@glider.be> wrote:
>>>>
>>>> Add DT bindings for the Renesas RZ/A1 Interrupt Controller.
>>>>
>>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>> ---
>>>> v2:
>>>>   - Add "renesas,gic-spi-base",
>>>>   - Document RZ/A2M.
>>>> ---
>>>>  .../renesas,rza1-irqc.txt                     | 30 +++++++++++++++++++
>>>>  1 file changed, 30 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
>>>> new file mode 100644
>>>> index 0000000000000000..ea8ddb6955338ccd
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
>>>> @@ -0,0 +1,30 @@
>>>> +DT bindings for the Renesas RZ/A1 Interrupt Controller
>>>> +
>>>> +The RZ/A1 Interrupt Controller is a front-end for the GIC found on Renesas
>>>> +RZ/A1 and RZ/A2 SoCs:
>>>> +  - IRQ sense select for 8 external interrupts, 1:1-mapped to 8 GIC SPI
>>>> +    interrupts,
>>>> +  - NMI edge select.
>>>> +
>>>> +Required properties:
>>>> +  - compatible: Must be "renesas,<soctype>-irqc", and "renesas,rza1-irqc" as
>>>> +               fallback.
>>>> +               Examples with soctypes are:
>>>> +                 - "renesas,r7s72100-irqc" (RZ/A1H)
>>>> +                 - "renesas,r7s9210-irqc" (RZ/A2M)
>>>> +  - #interrupt-cells: Must be 2 (an interrupt index and flags, as defined
>>>> +                                in interrupts.txt in this directory)
>>>> +  - interrupt-controller: Marks the device as an interrupt controller
>>>> +  - reg: Base address and length of the memory resource used by the interrupt
>>>> +         controller
>>>> +  - renesas,gic-spi-base: Lowest GIC SPI interrupt number this block maps to.
>>>
>>> Why isn't this just an 'interrupts' property?
>>
>> That's likely because of kernel limitations. The DT code does an
>> of_populate() on any device that it finds, parse the "interrupts"
>> propertiy, resulting in the irq_descs being populated.
>>
>> That creates havoc, as these interrupts are not for this device, but for
>> something that is connected to it. This is merely a bridge of some sort.
> 
> 'interrupt-map' would avoid that problem I think.

I'm afraid it doesn't scale at all. Case in point, the GICv3 ITS. Up to
32bit worth of IDs. How do you represent that using an interrupt-map?
Agreed, that's the extreme case, but representing more than a handful of
interrupts using an interrupt-map is a pain.

> 
>> Furthermore, this is a rather long established practice: gic-v2m,
>> gic-v3-mbi, mediatek,sysirq, mediatek,cirq... All the bits of glue that
>> for one reason or another plug onto the GIC use the same method.
> 
> All handling the mapping to the parent in their own way...

Yes, and that's the problem. We need a scalable way to describe ranges
of interrupts that are "forwarded" (for the lack of a better term), but
now "owned" by the the interrupt controller.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 1/5] dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt Controller

[Rob Herring]

On Thu, May 2, 2019 at 9:02 AM Marc Zyngier <marc.zyngier@arm.com> wrote:
>
> On 30/04/2019 21:25, Rob Herring wrote:
> > On Tue, Apr 30, 2019 at 10:34 AM Marc Zyngier <marc.zyngier@arm.com> wrote:
> >>
> >> On 30/04/2019 16:02, Rob Herring wrote:
> >>> On Tue, Apr 30, 2019 at 7:13 AM Geert Uytterhoeven
> >>> <geert+renesas@glider.be> wrote:
> >>>>
> >>>> Add DT bindings for the Renesas RZ/A1 Interrupt Controller.
> >>>>
> >>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >>>> ---
> >>>> v2:
> >>>>   - Add "renesas,gic-spi-base",
> >>>>   - Document RZ/A2M.
> >>>> ---
> >>>>  .../renesas,rza1-irqc.txt                     | 30 +++++++++++++++++++
> >>>>  1 file changed, 30 insertions(+)
> >>>>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> >>>> new file mode 100644
> >>>> index 0000000000000000..ea8ddb6955338ccd
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> >>>> @@ -0,0 +1,30 @@
> >>>> +DT bindings for the Renesas RZ/A1 Interrupt Controller
> >>>> +
> >>>> +The RZ/A1 Interrupt Controller is a front-end for the GIC found on Renesas
> >>>> +RZ/A1 and RZ/A2 SoCs:
> >>>> +  - IRQ sense select for 8 external interrupts, 1:1-mapped to 8 GIC SPI
> >>>> +    interrupts,
> >>>> +  - NMI edge select.
> >>>> +
> >>>> +Required properties:
> >>>> +  - compatible: Must be "renesas,<soctype>-irqc", and "renesas,rza1-irqc" as
> >>>> +               fallback.
> >>>> +               Examples with soctypes are:
> >>>> +                 - "renesas,r7s72100-irqc" (RZ/A1H)
> >>>> +                 - "renesas,r7s9210-irqc" (RZ/A2M)
> >>>> +  - #interrupt-cells: Must be 2 (an interrupt index and flags, as defined
> >>>> +                                in interrupts.txt in this directory)
> >>>> +  - interrupt-controller: Marks the device as an interrupt controller
> >>>> +  - reg: Base address and length of the memory resource used by the interrupt
> >>>> +         controller
> >>>> +  - renesas,gic-spi-base: Lowest GIC SPI interrupt number this block maps to.
> >>>
> >>> Why isn't this just an 'interrupts' property?
> >>
> >> That's likely because of kernel limitations. The DT code does an
> >> of_populate() on any device that it finds, parse the "interrupts"
> >> propertiy, resulting in the irq_descs being populated.
> >>
> >> That creates havoc, as these interrupts are not for this device, but for
> >> something that is connected to it. This is merely a bridge of some sort.
> >
> > 'interrupt-map' would avoid that problem I think.
>
> I'm afraid it doesn't scale at all. Case in point, the GICv3 ITS. Up to
> 32bit worth of IDs. How do you represent that using an interrupt-map?
> Agreed, that's the extreme case, but representing more than a handful of
> interrupts using an interrupt-map is a pain.

Neither does a "parent's irq base" property scale. It works well if
you have a single linear range, but not any other case. So there's can
something scale to any possible mapping and can we express simple
cases in a compact form. Essentially we need to express the mapping
for a range of irqs with the assumption that we can add the child irq
number to the parent (otherwise I don't think you can scale it). That
also requires assumptions about what the irq specifiers contain. All
the custom properties do that anyway, but the standard interrupt
properties do not.

Perhaps we just need some interrupt controller specific hook to handle
more complicated cases of interrupt-map. If we can't map the child to
the parent using the standard matching (masking), then punt it to the
driver to find a match however it wants. So you could have something
like this:

interrupt-map-mask = <0xffffffff 0>;
interrupt-map = <<child base 1> 0 &gic GIC_SPI <parent base 1>
IRQ_TYPE_LEVEL_HIGH>,
        <<child base 2> 0 &gic GIC_SPI <parent base 2> IRQ_TYPE_LEVEL_HIGH>;

Then it is up to the driver to decide how to map an entry and it
doesn't require an exact match in DT. I've of course given little
thought to how exactly we wire up that driver hook. :)

> >> Furthermore, this is a rather long established practice: gic-v2m,
> >> gic-v3-mbi, mediatek,sysirq, mediatek,cirq... All the bits of glue that
> >> for one reason or another plug onto the GIC use the same method.
> >
> > All handling the mapping to the parent in their own way...
>
> Yes, and that's the problem. We need a scalable way to describe ranges
> of interrupts that are "forwarded" (for the lack of a better term), but
> now "owned" by the the interrupt controller.

Do we need to solve that now and for this case? Yes, 8 entries of
interrupt-map is more verbose than just a property specifying a base
irq, but I'd prefer standard over non-standard.

OTOH, I guess what's one more custom property...

Rob

Re: [PATCH v2 1/5] dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt Controller

[Rob Herring]

On Thu, May 2, 2019 at 9:02 AM Marc Zyngier <marc.zyngier@arm.com> wrote:
>
> On 30/04/2019 21:25, Rob Herring wrote:
> > On Tue, Apr 30, 2019 at 10:34 AM Marc Zyngier <marc.zyngier@arm.com> wrote:
> >>
> >> On 30/04/2019 16:02, Rob Herring wrote:
> >>> On Tue, Apr 30, 2019 at 7:13 AM Geert Uytterhoeven
> >>> <geert+renesas@glider.be> wrote:
> >>>>
> >>>> Add DT bindings for the Renesas RZ/A1 Interrupt Controller.
> >>>>
> >>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >>>> ---
> >>>> v2:
> >>>>   - Add "renesas,gic-spi-base",
> >>>>   - Document RZ/A2M.
> >>>> ---
> >>>>  .../renesas,rza1-irqc.txt                     | 30 +++++++++++++++++++
> >>>>  1 file changed, 30 insertions(+)
> >>>>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> >>>> new file mode 100644
> >>>> index 0000000000000000..ea8ddb6955338ccd
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
> >>>> @@ -0,0 +1,30 @@
> >>>> +DT bindings for the Renesas RZ/A1 Interrupt Controller
> >>>> +
> >>>> +The RZ/A1 Interrupt Controller is a front-end for the GIC found on Renesas
> >>>> +RZ/A1 and RZ/A2 SoCs:
> >>>> +  - IRQ sense select for 8 external interrupts, 1:1-mapped to 8 GIC SPI
> >>>> +    interrupts,
> >>>> +  - NMI edge select.
> >>>> +
> >>>> +Required properties:
> >>>> +  - compatible: Must be "renesas,<soctype>-irqc", and "renesas,rza1-irqc" as
> >>>> +               fallback.
> >>>> +               Examples with soctypes are:
> >>>> +                 - "renesas,r7s72100-irqc" (RZ/A1H)
> >>>> +                 - "renesas,r7s9210-irqc" (RZ/A2M)
> >>>> +  - #interrupt-cells: Must be 2 (an interrupt index and flags, as defined
> >>>> +                                in interrupts.txt in this directory)
> >>>> +  - interrupt-controller: Marks the device as an interrupt controller
> >>>> +  - reg: Base address and length of the memory resource used by the interrupt
> >>>> +         controller
> >>>> +  - renesas,gic-spi-base: Lowest GIC SPI interrupt number this block maps to.
> >>>
> >>> Why isn't this just an 'interrupts' property?
> >>
> >> That's likely because of kernel limitations. The DT code does an
> >> of_populate() on any device that it finds, parse the "interrupts"
> >> propertiy, resulting in the irq_descs being populated.
> >>
> >> That creates havoc, as these interrupts are not for this device, but for
> >> something that is connected to it. This is merely a bridge of some sort.
> >
> > 'interrupt-map' would avoid that problem I think.
>
> I'm afraid it doesn't scale at all. Case in point, the GICv3 ITS. Up to
> 32bit worth of IDs. How do you represent that using an interrupt-map?
> Agreed, that's the extreme case, but representing more than a handful of
> interrupts using an interrupt-map is a pain.

Neither does a "parent's irq base" property scale. It works well if
you have a single linear range, but not any other case. So there's can
something scale to any possible mapping and can we express simple
cases in a compact form. Essentially we need to express the mapping
for a range of irqs with the assumption that we can add the child irq
number to the parent (otherwise I don't think you can scale it). That
also requires assumptions about what the irq specifiers contain. All
the custom properties do that anyway, but the standard interrupt
properties do not.

Perhaps we just need some interrupt controller specific hook to handle
more complicated cases of interrupt-map. If we can't map the child to
the parent using the standard matching (masking), then punt it to the
driver to find a match however it wants. So you could have something
like this:

interrupt-map-mask = <0xffffffff 0>;
interrupt-map = <<child base 1> 0 &gic GIC_SPI <parent base 1>
IRQ_TYPE_LEVEL_HIGH>,
        <<child base 2> 0 &gic GIC_SPI <parent base 2> IRQ_TYPE_LEVEL_HIGH>;

Then it is up to the driver to decide how to map an entry and it
doesn't require an exact match in DT. I've of course given little
thought to how exactly we wire up that driver hook. :)

> >> Furthermore, this is a rather long established practice: gic-v2m,
> >> gic-v3-mbi, mediatek,sysirq, mediatek,cirq... All the bits of glue that
> >> for one reason or another plug onto the GIC use the same method.
> >
> > All handling the mapping to the parent in their own way...
>
> Yes, and that's the problem. We need a scalable way to describe ranges
> of interrupts that are "forwarded" (for the lack of a better term), but
> now "owned" by the the interrupt controller.

Do we need to solve that now and for this case? Yes, 8 entries of
interrupt-map is more verbose than just a property specifying a base
irq, but I'd prefer standard over non-standard.

OTOH, I guess what's one more custom property...

Rob

[PATCH v2 2/5] irqchip: Add Renesas RZ/A1 Interrupt Controller driver

[Geert Uytterhoeven]

Add a driver for the Renesas RZ/A1 Interrupt Controller.

This supports using up to 8 external interrupts on RZ/A1, with
configurable sense select.

NMI edge select is not yet supported.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - Use u16 for register values,
  - Use relaxed I/O accessors,
  - Use "rza1-irqc" as irq_chip class name,
  - Replace gic_spi_base in OF match data by renesas,gic-spi-base in DT.
---
 drivers/irqchip/Kconfig            |   4 +
 drivers/irqchip/Makefile           |   1 +
 drivers/irqchip/irq-renesas-rza1.c | 235 +++++++++++++++++++++++++++++
 3 files changed, 240 insertions(+)
 create mode 100644 drivers/irqchip/irq-renesas-rza1.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 8c1c67f9a4bb5bf6..db07db4c48486649 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -224,6 +224,10 @@ config RENESAS_IRQC
 	select GENERIC_IRQ_CHIP
 	select IRQ_DOMAIN
 
+config RENESAS_RZA1_IRQC
+	bool
+	select IRQ_DOMAIN_HIERARCHY
+
 config ST_IRQCHIP
 	bool
 	select REGMAP
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index f8c66e958a64deaa..ce37fba74effad8c 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_JCORE_AIC)			+= irq-jcore-aic.o
 obj-$(CONFIG_RDA_INTC)			+= irq-rda-intc.o
 obj-$(CONFIG_RENESAS_INTC_IRQPIN)	+= irq-renesas-intc-irqpin.o
 obj-$(CONFIG_RENESAS_IRQC)		+= irq-renesas-irqc.o
+obj-$(CONFIG_RENESAS_RZA1_IRQC)		+= irq-renesas-rza1.o
 obj-$(CONFIG_VERSATILE_FPGA_IRQ)	+= irq-versatile-fpga.o
 obj-$(CONFIG_ARCH_NSPIRE)		+= irq-zevio.o
 obj-$(CONFIG_ARCH_VT8500)		+= irq-vt8500.o
diff --git a/drivers/irqchip/irq-renesas-rza1.c b/drivers/irqchip/irq-renesas-rza1.c
new file mode 100644
index 0000000000000000..f7a3c96cffb492f5
--- /dev/null
+++ b/drivers/irqchip/irq-renesas-rza1.c
@@ -0,0 +1,235 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/A1 IRQC Driver
+ *
+ * Copyright (C) 2019 Glider bvba
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irqdomain.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+
+#define IRQC_NUM_IRQ		8
+
+#define ICR0			0	/* Interrupt Control Register 0 */
+
+#define ICR0_NMIL		BIT(15)	/* NMI Input Level (0=low, 1=high) */
+#define ICR0_NMIE		BIT(8)	/* Edge Select (0=falling, 1=rising) */
+#define ICR0_NMIF		BIT(1)	/* NMI Interrupt Request */
+
+#define ICR1			2	/* Interrupt Control Register 1 */
+
+#define ICR1_IRQS(n, sense)	((sense) << ((n) * 2))	/* IRQ Sense Select */
+#define ICR1_IRQS_LEVEL_LOW	0
+#define ICR1_IRQS_EDGE_FALLING	1
+#define ICR1_IRQS_EDGE_RISING	2
+#define ICR1_IRQS_EDGE_BOTH	3
+#define ICR1_IRQS_MASK(n)	ICR1_IRQS((n), 3)
+
+#define IRQRR			4	/* IRQ Interrupt Request Register */
+
+
+struct rza1_irqc_priv {
+	struct device *dev;
+	void __iomem *base;
+	struct irq_chip chip;
+	struct irq_domain *irq_domain;
+	u32 gic_spi_base;
+};
+
+static struct rza1_irqc_priv *irq_data_to_priv(struct irq_data *data)
+{
+	return data->domain->host_data;
+}
+
+static void rza1_irqc_eoi(struct irq_data *d)
+{
+	struct rza1_irqc_priv *priv = irq_data_to_priv(d);
+	u16 bit = BIT(irqd_to_hwirq(d));
+	u16 tmp;
+
+	tmp = readw_relaxed(priv->base + IRQRR);
+	if (tmp & bit)
+		writew_relaxed(GENMASK(IRQC_NUM_IRQ - 1, 0) & ~bit,
+			       priv->base + IRQRR);
+
+	irq_chip_eoi_parent(d);
+}
+
+static int rza1_irqc_set_type(struct irq_data *d, unsigned int type)
+{
+	struct rza1_irqc_priv *priv = irq_data_to_priv(d);
+	unsigned int hw_irq = irqd_to_hwirq(d);
+	u16 sense, tmp;
+
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_LEVEL_LOW:
+		sense = ICR1_IRQS_LEVEL_LOW;
+		break;
+
+	case IRQ_TYPE_EDGE_FALLING:
+		sense = ICR1_IRQS_EDGE_FALLING;
+		break;
+
+	case IRQ_TYPE_EDGE_RISING:
+		sense = ICR1_IRQS_EDGE_RISING;
+		break;
+
+	case IRQ_TYPE_EDGE_BOTH:
+		sense = ICR1_IRQS_EDGE_BOTH;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	tmp = readw_relaxed(priv->base + ICR1);
+	tmp &= ~ICR1_IRQS_MASK(hw_irq);
+	tmp |= ICR1_IRQS(hw_irq, sense);
+	writew_relaxed(tmp, priv->base + ICR1);
+	return 0;
+}
+
+static int rza1_irqc_alloc(struct irq_domain *domain, unsigned int virq,
+			   unsigned int nr_irqs, void *arg)
+{
+	struct rza1_irqc_priv *priv = domain->host_data;
+	struct irq_fwspec *fwspec = arg;
+	struct irq_fwspec spec;
+	int ret;
+
+	ret = irq_domain_set_hwirq_and_chip(domain, virq, fwspec->param[0],
+					    &priv->chip, priv);
+	if (ret)
+		return ret;
+
+	spec.fwnode = &priv->dev->of_node->fwnode;
+	spec.param_count = 3;
+	spec.param[0] = GIC_SPI;
+	spec.param[1] = priv->gic_spi_base + fwspec->param[0];
+	spec.param[2] = IRQ_TYPE_LEVEL_HIGH;
+
+	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &spec);
+}
+
+static int rza1_irqc_translate(struct irq_domain *domain,
+			       struct irq_fwspec *fwspec, unsigned long *hwirq,
+			       unsigned int *type)
+{
+	if (fwspec->param_count != 2 || fwspec->param[0] >= IRQC_NUM_IRQ)
+		return -EINVAL;
+
+	*hwirq = fwspec->param[0];
+	*type = fwspec->param[1];
+	return 0;
+}
+
+static const struct irq_domain_ops rza1_irqc_domain_ops = {
+	.alloc = rza1_irqc_alloc,
+	.translate = rza1_irqc_translate,
+};
+
+static int rza1_irqc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct irq_domain *parent = NULL;
+	struct device_node *gic_node;
+	struct rza1_irqc_priv *priv;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	ret = of_property_read_u32(np, "renesas,gic-spi-base",
+				   &priv->gic_spi_base);
+	if (ret) {
+		dev_err(dev, "missing %s property\n", "renesas,gic-spi-base");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, priv);
+	priv->dev = dev;
+
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	gic_node = of_irq_find_parent(np);
+	if (gic_node) {
+		parent = irq_find_host(gic_node);
+		of_node_put(gic_node);
+	}
+
+	if (!parent) {
+		dev_err(dev, "cannot find parent domain\n");
+		return -ENODEV;
+	}
+
+	priv->chip.name = "rza1-irqc",
+	priv->chip.irq_mask = irq_chip_mask_parent,
+	priv->chip.irq_unmask = irq_chip_unmask_parent,
+	priv->chip.irq_eoi = rza1_irqc_eoi,
+	priv->chip.irq_retrigger = irq_chip_retrigger_hierarchy,
+	priv->chip.irq_set_type = rza1_irqc_set_type,
+	priv->chip.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE;
+
+	priv->irq_domain = irq_domain_add_hierarchy(parent, 0, IRQC_NUM_IRQ,
+						    np, &rza1_irqc_domain_ops,
+						    priv);
+	if (!priv->irq_domain) {
+		dev_err(dev, "cannot initialize irq domain\n");
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int rza1_irqc_remove(struct platform_device *pdev)
+{
+	struct rza1_irqc_priv *priv = platform_get_drvdata(pdev);
+
+	irq_domain_remove(priv->irq_domain);
+	return 0;
+}
+
+static const struct of_device_id rza1_irqc_dt_ids[] = {
+	{ .compatible = "renesas,rza1-irqc" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, rza1_irqc_dt_ids);
+
+static struct platform_driver rza1_irqc_device_driver = {
+	.probe		= rza1_irqc_probe,
+	.remove		= rza1_irqc_remove,
+	.driver		= {
+		.name	= "renesas_rza1_irqc",
+		.of_match_table	= rza1_irqc_dt_ids,
+	}
+};
+
+static int __init rza1_irqc_init(void)
+{
+	return platform_driver_register(&rza1_irqc_device_driver);
+}
+postcore_initcall(rza1_irqc_init);
+
+static void __exit rza1_irqc_exit(void)
+{
+	platform_driver_unregister(&rza1_irqc_device_driver);
+}
+module_exit(rza1_irqc_exit);
+
+MODULE_AUTHOR("Geert Uytterhoeven <geert+renesas@glider.be>");
+MODULE_DESCRIPTION("Renesas RZ/A1 IRQC Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

RE: [PATCH v2 2/5] irqchip: Add Renesas RZ/A1 Interrupt Controller driver

[Chris Brandt]

On Tue, Apr 30, 2019, Geert Uytterhoeven wrote:
> Add a driver for the Renesas RZ/A1 Interrupt Controller.
> 
> This supports using up to 8 external interrupts on RZ/A1, with
> configurable sense select.
> 
> NMI edge select is not yet supported.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2:
>   - Use u16 for register values,
>   - Use relaxed I/O accessors,
>   - Use "rza1-irqc" as irq_chip class name,
>   - Replace gic_spi_base in OF match data by renesas,gic-spi-base in DT.
> ---
>  drivers/irqchip/Kconfig            |   4 +
>  drivers/irqchip/Makefile           |   1 +
>  drivers/irqchip/irq-renesas-rza1.c | 235 +++++++++++++++++++++++++++++
>  3 files changed, 240 insertions(+)
>  create mode 100644 drivers/irqchip/irq-renesas-rza1.c

Tested on RZ/A1H RSK and RZ/A2M EVB (push buttons and LCD
touchscreen controller).

Thanks!

Tested-by: Chris Brandt <chris.brandt@renesas.com>

RE: [PATCH v2 2/5] irqchip: Add Renesas RZ/A1 Interrupt Controller driver

[Chris Brandt]

On Tue, Apr 30, 2019, Geert Uytterhoeven wrote:
> Add a driver for the Renesas RZ/A1 Interrupt Controller.
> 
> This supports using up to 8 external interrupts on RZ/A1, with
> configurable sense select.
> 
> NMI edge select is not yet supported.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2:
>   - Use u16 for register values,
>   - Use relaxed I/O accessors,
>   - Use "rza1-irqc" as irq_chip class name,
>   - Replace gic_spi_base in OF match data by renesas,gic-spi-base in DT.
> ---
>  drivers/irqchip/Kconfig            |   4 +
>  drivers/irqchip/Makefile           |   1 +
>  drivers/irqchip/irq-renesas-rza1.c | 235 +++++++++++++++++++++++++++++
>  3 files changed, 240 insertions(+)
>  create mode 100644 drivers/irqchip/irq-renesas-rza1.c

Tested on RZ/A1H RSK and RZ/A2M EVB (push buttons and LCD
touchscreen controller).

Thanks!

Tested-by: Chris Brandt <chris.brandt@renesas.com>

[PATCH v2 3/5] soc: renesas: Enable RZ/A1 IRQC on RZ/A1H and RZ/A2M

[Geert Uytterhoeven]

Auto-enable support for the RZ/A1 Interrupt Controller when configuring
a kernel which supports RZ/A1H or RZ/A2M SoCs.
Keep selects sorted while at it.

This is similar to how interrupt controllers for other Renesas SoCs are
enabled.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
---
v2:
  - Add Reviewed-by,
  - Add RZ/A2M,
  - Sort RZ/A1H selects.
---
 drivers/soc/renesas/Kconfig | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/renesas/Kconfig b/drivers/soc/renesas/Kconfig
index 68bfca6f20ddf8a7..2bbf49e5d441808b 100644
--- a/drivers/soc/renesas/Kconfig
+++ b/drivers/soc/renesas/Kconfig
@@ -57,14 +57,16 @@ config ARCH_R7S72100
 	bool "RZ/A1H (R7S72100)"
 	select PM
 	select PM_GENERIC_DOMAINS
-	select SYS_SUPPORTS_SH_MTU2
 	select RENESAS_OSTM
+	select RENESAS_RZA1_IRQC
+	select SYS_SUPPORTS_SH_MTU2
 
 config ARCH_R7S9210
 	bool "RZ/A2 (R7S9210)"
 	select PM
 	select PM_GENERIC_DOMAINS
 	select RENESAS_OSTM
+	select RENESAS_RZA1_IRQC
 
 config ARCH_R8A73A4
 	bool "R-Mobile APE6 (R8A73A40)"
-- 
2.17.1

[PATCH v2 4/5] ARM: dts: r7s72100: Add IRQC device node

[Geert Uytterhoeven]

Enable support for the IRQC on RZ/A1H, which is a small front-end to the
GIC.  This allows to use up to 8 external interrupts with configurable
sense select.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
---
v2:
  - Add Reviewed-by,
  - Add "renesas,gic-spi-base".
---
 arch/arm/boot/dts/r7s72100.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi
index 2211f88ede2ad351..52855db63a60580c 100644
--- a/arch/arm/boot/dts/r7s72100.dtsi
+++ b/arch/arm/boot/dts/r7s72100.dtsi
@@ -670,6 +670,15 @@
 			status = "disabled";
 		};
 
+		irqc: interrupt-controller@fcfef800 {
+			compatible = "renesas,r7s72100-irqc",
+				     "renesas,rza1-irqc";
+			#interrupt-cells = <2>;
+			interrupt-controller;
+			reg = <0xfcfef800 0x6>;
+			renesas,gic-spi-base = <0>;
+		};
+
 		mtu2: timer@fcff0000 {
 			compatible = "renesas,mtu2-r7s72100", "renesas,mtu2";
 			reg = <0xfcff0000 0x400>;
-- 
2.17.1

[PATCH v3 5/5] ARM: dts: rskrza1: Add input switches

[Geert Uytterhoeven]

Add support for input switches SW1-3 on the Renesas RZ/A1 RSK+RZA1
development board.

Note that this uses the IRQ interrupts, as the RZ/A1 GPIO controller
does not include interrupt support.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v3:
  - No changes,

v2:
  - Use rza1-irqc instead of gic.
---
 arch/arm/boot/dts/r7s72100-rskrza1.dts | 38 ++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100-rskrza1.dts b/arch/arm/boot/dts/r7s72100-rskrza1.dts
index ff24301dc1be54de..99acfe4fe11aaed9 100644
--- a/arch/arm/boot/dts/r7s72100-rskrza1.dts
+++ b/arch/arm/boot/dts/r7s72100-rskrza1.dts
@@ -8,6 +8,7 @@
 /dts-v1/;
 #include "r7s72100.dtsi"
 #include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/input/input.h>
 #include <dt-bindings/pinctrl/r7s72100-pinctrl.h>
 
 / {
@@ -28,6 +29,37 @@
 		reg = <0x08000000 0x02000000>;
 	};
 
+	keyboard {
+		compatible = "gpio-keys";
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&keyboard_pins>;
+
+		key-1 {
+			interrupt-parent = <&irqc>;
+			interrupts = <3 IRQ_TYPE_EDGE_BOTH>;
+			linux,code = <KEY_1>;
+			label = "SW1";
+			wakeup-source;
+		};
+
+		key-2 {
+			interrupt-parent = <&irqc>;
+			interrupts = <2 IRQ_TYPE_EDGE_BOTH>;
+			linux,code = <KEY_2>;
+			label = "SW2";
+			wakeup-source;
+		};
+
+		key-3 {
+			interrupt-parent = <&irqc>;
+			interrupts = <5 IRQ_TYPE_EDGE_BOTH>;
+			linux,code = <KEY_3>;
+			label = "SW3";
+			wakeup-source;
+		};
+	};
+
 	lbsc {
 		#address-cells = <1>;
 		#size-cells = <1>;
@@ -101,6 +133,12 @@
 			 <RZA1_PINMUX(1, 7, 1)>;	/* RIIC3SDA */
 	};
 
+	keyboard_pins: keyboard {
+		pinmux = <RZA1_PINMUX(1, 9, 3)>,	/* IRQ3 */
+			 <RZA1_PINMUX(1, 8, 3)>,	/* IRQ2 */
+			 <RZA1_PINMUX(1, 11, 3)>;	/* IRQ5 */
+	};
+
 	/* Serial Console */
 	scif2_pins: serial2 {
 		pinmux = <RZA1_PINMUX(3, 0, 6)>,	/* TxD2 */
-- 
2.17.1