[PATCH v3 0/6] Rework realtek-rtl IRQ driver

[PATCH v3 0/6] Rework realtek-rtl IRQ driver

[Sander Vanheule]

After seeing some use, and with more devices tested, the current implementation
for the Realtek SoC interrupt controller was found to contain a few flaws.

The driver requires the following fixes:
- irq_domain_ops::map should map the virq, not the hwirq
- routing has an off-by-one error. Routing values (1..6) correspond to MIPS CAUSEF(2..7)

The following improvements should also be made:
- Use N real cascaded interrupts with an interrupt-specific mask of child irq lines.
  Otherwise a high-priority interrupt may cause a low-priority interrupt to be
  handled first.
- Get rid of assumed routing to parent interrupts of the original implementation.

Changes since v2 (RFC):
Link: https://lore.kernel.org/all/cover.1640548009.git.sander@svanheule.net/

- Define new, two-part compatibles for devicetree bindings. The existing format
  is kept for the old one-part compatible, but deprecated. New compatibles will
  require a different way of specifying parent interrupts and interrupt routing.
- Add change to handle all pending SoC interrupts in one go.

Changes since v1 (RFC):
Link: https://lore.kernel.org/all/cover.1640261161.git.sander@svanheule.net/

- Split some of the changes to limit the patch scope to one issue.
- Dropped some small (spurious or unneeded) changes
- Instead of dropping/replacing interrupt-map, the last patches now provide an
  implementation that amends the current situtation.

Sander Vanheule (6):
  irqchip/realtek-rtl: map control data to virq
  irqchip/realtek-rtl: fix off-by-one in routing
  irqchip/realtek-rtl: clear all pending interrupts
  dt-bindings: interrupt-controller: realtek,rtl-intc: require parents
  irqchip/realtek-rtl: use parent interrupts
  irqchip/realtek-rtl: use per-parent domains

 .../realtek,rtl-intc.yaml                     |  78 ++++--
 drivers/irqchip/irq-realtek-rtl.c             | 235 ++++++++++++------
 2 files changed, 222 insertions(+), 91 deletions(-)

-- 
2.33.1

[PATCH v3 1/6] irqchip/realtek-rtl: map control data to virq

[Sander Vanheule]

The driver assigned the irqchip and irq handler to the hardware irq,
instead of the virq. This is incorrect, and only worked because these
irq numbers happened to be the same on the devices used for testing the
original driver.

Fixes: 9f3a0f34b84a ("irqchip: Add support for Realtek RTL838x/RTL839x interrupt controller")
Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 drivers/irqchip/irq-realtek-rtl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c
index fd9f275592d2..d6788dd93c7b 100644
--- a/drivers/irqchip/irq-realtek-rtl.c
+++ b/drivers/irqchip/irq-realtek-rtl.c
@@ -62,7 +62,7 @@ static struct irq_chip realtek_ictl_irq = {
 
 static int intc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
 {
-	irq_set_chip_and_handler(hw, &realtek_ictl_irq, handle_level_irq);
+	irq_set_chip_and_handler(irq, &realtek_ictl_irq, handle_level_irq);
 
 	return 0;
 }
-- 
2.33.1

[PATCH v3 2/6] irqchip/realtek-rtl: fix off-by-one in routing

[Sander Vanheule]

There is an offset between routing values (1..6) and the connected MIPS
CPU interrupts (2..7), but no distinction was made between these two
values.

This issue was previously hidden during testing, because an interrupt
mapping was used where for each required interrupt another (unused)
routing was configured, with an offset of +1.

Offset the CPU IRQ numbers by -1 to retrieve the correct routing value.

Fixes: 9f3a0f34b84a ("irqchip: Add support for Realtek RTL838x/RTL839x interrupt controller")
Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 drivers/irqchip/irq-realtek-rtl.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c
index d6788dd93c7b..568614edd88f 100644
--- a/drivers/irqchip/irq-realtek-rtl.c
+++ b/drivers/irqchip/irq-realtek-rtl.c
@@ -95,7 +95,8 @@ static void realtek_irq_dispatch(struct irq_desc *desc)
  * SoC interrupts are cascaded to MIPS CPU interrupts according to the
  * interrupt-map in the device tree. Each SoC interrupt gets 4 bits for
  * the CPU interrupt in an Interrupt Routing Register. Max 32 SoC interrupts
- * thus go into 4 IRRs.
+ * thus go into 4 IRRs. A routing value of '0' means the interrupt is left
+ * disconnected. Routing values {1..15} connect to output lines {0..14}.
  */
 static int __init map_interrupts(struct device_node *node, struct irq_domain *domain)
 {
@@ -134,7 +135,7 @@ static int __init map_interrupts(struct device_node *node, struct irq_domain *do
 		of_node_put(cpu_ictl);
 
 		cpu_int = be32_to_cpup(imap + 2);
-		if (cpu_int > 7)
+		if (cpu_int > 7 || cpu_int < 2)
 			return -EINVAL;
 
 		if (!(mips_irqs_set & BIT(cpu_int))) {
@@ -143,7 +144,8 @@ static int __init map_interrupts(struct device_node *node, struct irq_domain *do
 			mips_irqs_set |= BIT(cpu_int);
 		}
 
-		regs[(soc_int * 4) / 32] |= cpu_int << (soc_int * 4) % 32;
+		/* Use routing values (1..6) for CPU interrupts (2..7) */
+		regs[(soc_int * 4) / 32] |= (cpu_int - 1) << (soc_int * 4) % 32;
 		imap += 3;
 	}
 
-- 
2.33.1

[PATCH v3 3/6] irqchip/realtek-rtl: clear all pending interrupts

[Sander Vanheule]

Instead of only servicing the lowest pending interrupt line, make sure
all pending SoC interrupts are serviced before exiting the chained
handler. This adds a small overhead if only one interrupt is pending,
but should prevent rapid re-triggering of the handler.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 drivers/irqchip/irq-realtek-rtl.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c
index 568614edd88f..50a56820c99b 100644
--- a/drivers/irqchip/irq-realtek-rtl.c
+++ b/drivers/irqchip/irq-realtek-rtl.c
@@ -76,16 +76,20 @@ static void realtek_irq_dispatch(struct irq_desc *desc)
 {
 	struct irq_chip *chip = irq_desc_get_chip(desc);
 	struct irq_domain *domain;
-	unsigned int pending;
+	unsigned long pending;
+	unsigned int soc_int;
 
 	chained_irq_enter(chip, desc);
 	pending = readl(REG(RTL_ICTL_GIMR)) & readl(REG(RTL_ICTL_GISR));
+
 	if (unlikely(!pending)) {
 		spurious_interrupt();
 		goto out;
 	}
+
 	domain = irq_desc_get_handler_data(desc);
-	generic_handle_domain_irq(domain, __ffs(pending));
+	for_each_set_bit(soc_int, &pending, 32)
+		generic_handle_domain_irq(domain, soc_int);
 
 out:
 	chained_irq_exit(chip, desc);
-- 
2.33.1

[PATCH v3 4/6] dt-bindings: interrupt-controller: realtek,rtl-intc: require parents

[Sander Vanheule]

The interrupt router has 32 inputs and up to 15 outputs, and the way
these are mapped to each other is runtime configurable. The outputs of
this interrupt router on the other hand, are connected to a fixed set of
parent interrupts. This means that "interrupt-map" is inappropriate, and
rather a list of parent interrupts should be specified.

Two-part compatibles are introduced to be able to require "interrupts"
for new devicetrees. The relevant descriptions are extended or added to
more clearly describe the inputs and outputs of this router.  The old
compatible, "interrupt-map" and "#address-cells", is deprecated.
Interrupt specifiers for new compatibles will require two cells, to
indicate the output selection.

To prevent spurious changes when more SoCs are added, "allOf" is used
with one "if", and the compatible enum only has one item.

The example is updated to provide a correct example for RTL8380 SoCs.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 .../realtek,rtl-intc.yaml                     | 78 ++++++++++++++-----
 1 file changed, 58 insertions(+), 20 deletions(-)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml
index 9e76fff20323..aab8d44010af 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml
@@ -6,6 +6,10 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
 
 title: Realtek RTL SoC interrupt controller devicetree bindings
 
+description:
+  Interrupt router for Realtek MIPS SoCs, allowing each SoC interrupt to be
+  routed to one parent interrupt, or left disconnected.
+
 maintainers:
   - Birger Koblitz <mail@birger-koblitz.de>
   - Bert Vermeulen <bert@biot.com>
@@ -13,45 +17,79 @@ maintainers:
 
 properties:
   compatible:
-    const: realtek,rtl-intc
+    oneOf:
+      - items:
+          - enum:
+              - realtek,rtl8380-intc
+          - const: realtek,rtl-intc
+      - const: realtek,rtl-intc
+        deprecated: true
 
-  "#interrupt-cells":
-    const: 1
+  "#interrupt-cells": true
 
   reg:
     maxItems: 1
 
   interrupts:
-    maxItems: 1
+    minItems: 1
+    maxItems: 15
+    description:
+      List of parent interrupts, in the order that they are connected to this
+      interrupt router's outputs.
 
   interrupt-controller: true
 
-  "#address-cells":
-    const: 0
-
-  interrupt-map:
-    description: Describes mapping from SoC interrupts to CPU interrupts
-
 required:
   - compatible
   - reg
   - "#interrupt-cells"
   - interrupt-controller
-  - "#address-cells"
-  - interrupt-map
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          const: realtek,rtl-intc
+    then:
+      properties:
+        "#interrupt-cells":
+          const: 1
+
+        "#address-cells":
+          const: 0
+
+        interrupt-map: true
+      required:
+        - "#address-cells"
+        - interrupt-map
+    else:
+      properties:
+        "#interrupt-cells":
+          description:
+            Two cells to specify which line to connect to, and which output it should
+            be routed to. Both cells use a zero-based index.
+          const: 2
+      required:
+        - interrupts
 
 additionalProperties: false
 
 examples:
   - |
     intc: interrupt-controller@3000 {
-      compatible = "realtek,rtl-intc";
-      #interrupt-cells = <1>;
+      compatible = "realtek,rtl8380-intc", "realtek,rtl-intc";
+      #interrupt-cells = <2>;
       interrupt-controller;
-      reg = <0x3000 0x20>;
-      #address-cells = <0>;
-      interrupt-map =
-              <31 &cpuintc 2>,
-              <30 &cpuintc 1>,
-              <29 &cpuintc 5>;
+      reg = <0x3000 0x18>;
+
+      interrupt-parent = <&cpuintc>;
+      interrupts = <2>, <3>, <4>, <5>, <6>;
+    };
+
+    irq-consumer@0 {
+      reg = <0 4>;
+      interrupt-parent = <&intc>;
+      interrupts =
+        <19 3>, /* IRQ 19, routed to output 3 (cpuintc 5) */
+        <18 4>; /* IRQ 18, routed to output 4 (cpuintc 6) */
     };
-- 
2.33.1

Re: [PATCH v3 4/6] dt-bindings: interrupt-controller: realtek,rtl-intc: require parents

[Rob Herring]

On Sun, Jan 09, 2022 at 03:54:35PM +0100, Sander Vanheule wrote:
> The interrupt router has 32 inputs and up to 15 outputs, and the way
> these are mapped to each other is runtime configurable. The outputs of
> this interrupt router on the other hand, are connected to a fixed set of
> parent interrupts. This means that "interrupt-map" is inappropriate, and
> rather a list of parent interrupts should be specified.

I'm not sure why interrupt-map is not appropriate. It is not appropriate 
if you have to touch the interrupt router h/w in servicing the 
interrupts. If you just need one time configuration of the mapping, then 
it should be fine to use I think.

> Two-part compatibles are introduced to be able to require "interrupts"
> for new devicetrees. The relevant descriptions are extended or added to
> more clearly describe the inputs and outputs of this router.  The old
> compatible, "interrupt-map" and "#address-cells", is deprecated.
> Interrupt specifiers for new compatibles will require two cells, to
> indicate the output selection.
> 
> To prevent spurious changes when more SoCs are added, "allOf" is used
> with one "if", and the compatible enum only has one item.
> 
> The example is updated to provide a correct example for RTL8380 SoCs.
> 
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> ---
>  .../realtek,rtl-intc.yaml                     | 78 ++++++++++++++-----
>  1 file changed, 58 insertions(+), 20 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml
> index 9e76fff20323..aab8d44010af 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml
> +++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml
> @@ -6,6 +6,10 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
>  title: Realtek RTL SoC interrupt controller devicetree bindings
>  
> +description:
> +  Interrupt router for Realtek MIPS SoCs, allowing each SoC interrupt to be
> +  routed to one parent interrupt, or left disconnected.
> +
>  maintainers:
>    - Birger Koblitz <mail@birger-koblitz.de>
>    - Bert Vermeulen <bert@biot.com>
> @@ -13,45 +17,79 @@ maintainers:
>  
>  properties:
>    compatible:
> -    const: realtek,rtl-intc
> +    oneOf:
> +      - items:
> +          - enum:
> +              - realtek,rtl8380-intc
> +          - const: realtek,rtl-intc
> +      - const: realtek,rtl-intc
> +        deprecated: true
>  
> -  "#interrupt-cells":
> -    const: 1
> +  "#interrupt-cells": true
>  
>    reg:
>      maxItems: 1
>  
>    interrupts:
> -    maxItems: 1
> +    minItems: 1
> +    maxItems: 15
> +    description:
> +      List of parent interrupts, in the order that they are connected to this
> +      interrupt router's outputs.
>  
>    interrupt-controller: true
>  
> -  "#address-cells":
> -    const: 0
> -
> -  interrupt-map:
> -    description: Describes mapping from SoC interrupts to CPU interrupts
> -
>  required:
>    - compatible
>    - reg
>    - "#interrupt-cells"
>    - interrupt-controller
> -  - "#address-cells"
> -  - interrupt-map
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          const: realtek,rtl-intc
> +    then:
> +      properties:
> +        "#interrupt-cells":
> +          const: 1
> +
> +        "#address-cells":
> +          const: 0
> +
> +        interrupt-map: true
> +      required:
> +        - "#address-cells"
> +        - interrupt-map
> +    else:
> +      properties:
> +        "#interrupt-cells":
> +          description:
> +            Two cells to specify which line to connect to, and which output it should
> +            be routed to. Both cells use a zero-based index.

Picking the index picks the priority? Which is higher priority?


> +          const: 2
> +      required:
> +        - interrupts
>  
>  additionalProperties: false
>  
>  examples:
>    - |
>      intc: interrupt-controller@3000 {
> -      compatible = "realtek,rtl-intc";
> -      #interrupt-cells = <1>;
> +      compatible = "realtek,rtl8380-intc", "realtek,rtl-intc";
> +      #interrupt-cells = <2>;
>        interrupt-controller;
> -      reg = <0x3000 0x20>;
> -      #address-cells = <0>;
> -      interrupt-map =
> -              <31 &cpuintc 2>,
> -              <30 &cpuintc 1>,
> -              <29 &cpuintc 5>;
> +      reg = <0x3000 0x18>;
> +
> +      interrupt-parent = <&cpuintc>;
> +      interrupts = <2>, <3>, <4>, <5>, <6>;
> +    };
> +
> +    irq-consumer@0 {
> +      reg = <0 4>;
> +      interrupt-parent = <&intc>;
> +      interrupts =
> +        <19 3>, /* IRQ 19, routed to output 3 (cpuintc 5) */
> +        <18 4>; /* IRQ 18, routed to output 4 (cpuintc 6) */
>      };
> -- 
> 2.33.1
> 
> 

Re: [PATCH v3 4/6] dt-bindings: interrupt-controller: realtek,rtl-intc: require parents

[Sander Vanheule]

Hi Rob,

On Fri, 2022-01-21 at 16:56 -0600, Rob Herring wrote:
> On Sun, Jan 09, 2022 at 03:54:35PM +0100, Sander Vanheule wrote:
> > The interrupt router has 32 inputs and up to 15 outputs, and the way
> > these are mapped to each other is runtime configurable. The outputs of
> > this interrupt router on the other hand, are connected to a fixed set of
> > parent interrupts. This means that "interrupt-map" is inappropriate, and
> > rather a list of parent interrupts should be specified.
> 
> I'm not sure why interrupt-map is not appropriate. It is not appropriate 
> if you have to touch the interrupt router h/w in servicing the 
> interrupts. If you just need one time configuration of the mapping, then 
> it should be fine to use I think.

If interrupt-map is used, then AFAICT there are no hooks to inform the driver that a
translation has occurred. How should the interrupt controller driver then know how to set
up the routing? Commit de4adddcbcc2 ("of/irq: Add a quirk for controllers with their own
definition of interrupt-map") added a quirk for the original binding/driver, but that
requires open-coding an interrupt-map parser in the driver.

What this binding doesn't mention (I can add it), is that there are also two IRQ status
registers to:
  - unmask/mask SoC interrupts
  - read the current status of the SoC interrupts

In theory, if the routing is set up correctly (and the IRQ permanently unmasked), I think
one could treat interrupt-map as intended, and connect SoC peripheral IRQ handlers
directly to the parent interrupts. But then the interrupt subsystem would need to check
all attached handlers. This interrupt router/controller allows to check which peripheral
is triggering the parent IRQ, which should be more efficient.

These interrupt controllers are also used on multi-threaded systems, where each hardware
thread has its own IRQ controller. I'm still experimenting with the implementation, but 
there the routing registers would be used to set the CPU affinity of SoC interrupts.

I have to say that I'm not very familiar with the kernel code that handles all this
though, so maybe I'm just missing something?

> > Two-part compatibles are introduced to be able to require "interrupts"
> > for new devicetrees. The relevant descriptions are extended or added to
> > more clearly describe the inputs and outputs of this router.  The old
> > compatible, "interrupt-map" and "#address-cells", is deprecated.
> > Interrupt specifiers for new compatibles will require two cells, to
> > indicate the output selection.
> > 
> > To prevent spurious changes when more SoCs are added, "allOf" is used
> > with one "if", and the compatible enum only has one item.
> > 
> > The example is updated to provide a correct example for RTL8380 SoCs.
> > 
> > Signed-off-by: Sander Vanheule <sander@svanheule.net>
> > ---
> >  .../realtek,rtl-intc.yaml                     | 78 ++++++++++++++-----
> >  1 file changed, 58 insertions(+), 20 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-
> > intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-
> > intc.yaml
> > index 9e76fff20323..aab8d44010af 100644
> > --- a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml
> > @@ -6,6 +6,10 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
> >  
> >  title: Realtek RTL SoC interrupt controller devicetree bindings
> >  
> > +description:
> > +  Interrupt router for Realtek MIPS SoCs, allowing each SoC interrupt to be
> > +  routed to one parent interrupt, or left disconnected.
> > +
> >  maintainers:
> >    - Birger Koblitz <mail@birger-koblitz.de>
> >    - Bert Vermeulen <bert@biot.com>
> > @@ -13,45 +17,79 @@ maintainers:
> >  
> >  properties:
> >    compatible:
> > -    const: realtek,rtl-intc
> > +    oneOf:
> > +      - items:
> > +          - enum:
> > +              - realtek,rtl8380-intc
> > +          - const: realtek,rtl-intc
> > +      - const: realtek,rtl-intc
> > +        deprecated: true
> >  
> > -  "#interrupt-cells":
> > -    const: 1
> > +  "#interrupt-cells": true
> >  
> >    reg:
> >      maxItems: 1
> >  
> >    interrupts:
> > -    maxItems: 1
> > +    minItems: 1
> > +    maxItems: 15
> > +    description:
> > +      List of parent interrupts, in the order that they are connected to this
> > +      interrupt router's outputs.
> >  
> >    interrupt-controller: true
> >  
> > -  "#address-cells":
> > -    const: 0
> > -
> > -  interrupt-map:
> > -    description: Describes mapping from SoC interrupts to CPU interrupts
> > -
> >  required:
> >    - compatible
> >    - reg
> >    - "#interrupt-cells"
> >    - interrupt-controller
> > -  - "#address-cells"
> > -  - interrupt-map
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          const: realtek,rtl-intc
> > +    then:
> > +      properties:
> > +        "#interrupt-cells":
> > +          const: 1
> > +
> > +        "#address-cells":
> > +          const: 0
> > +
> > +        interrupt-map: true
> > +      required:
> > +        - "#address-cells"
> > +        - interrupt-map
> > +    else:
> > +      properties:
> > +        "#interrupt-cells":
> > +          description:
> > +            Two cells to specify which line to connect to, and which output it should
> > +            be routed to. Both cells use a zero-based index.
> 
> Picking the index picks the priority? Which is higher priority?

Yes, picking an output will select the (implied) priority. If the parent interrupts are
the six MIPS CPU HW interrupts, then CPU IRQ7 has the highest priority, and IRQ2 has the
lowest priority. All known implementations connect output (0..5) to CPU IRQ(2..7), so 
lower output index then means lower priority.


Best,
Sander

> 
> 
> > +          const: 2
> > +      required:
> > +        - interrupts
> >  
> >  additionalProperties: false
> >  
> >  examples:
> >    - |
> >      intc: interrupt-controller@3000 {
> > -      compatible = "realtek,rtl-intc";
> > -      #interrupt-cells = <1>;
> > +      compatible = "realtek,rtl8380-intc", "realtek,rtl-intc";
> > +      #interrupt-cells = <2>;
> >        interrupt-controller;
> > -      reg = <0x3000 0x20>;
> > -      #address-cells = <0>;
> > -      interrupt-map =
> > -              <31 &cpuintc 2>,
> > -              <30 &cpuintc 1>,
> > -              <29 &cpuintc 5>;
> > +      reg = <0x3000 0x18>;
> > +
> > +      interrupt-parent = <&cpuintc>;
> > +      interrupts = <2>, <3>, <4>, <5>, <6>;
> > +    };
> > +
> > +    irq-consumer@0 {
> > +      reg = <0 4>;
> > +      interrupt-parent = <&intc>;
> > +      interrupts =
> > +        <19 3>, /* IRQ 19, routed to output 3 (cpuintc 5) */
> > +        <18 4>; /* IRQ 18, routed to output 4 (cpuintc 6) */
> >      };
> > -- 
> > 2.33.1
> > 
> > 

Re: [PATCH v3 4/6] dt-bindings: interrupt-controller: realtek,rtl-intc: require parents

[Rob Herring]

On Sat, Jan 22, 2022 at 01:49:44PM +0100, Sander Vanheule wrote:
> Hi Rob,
> 
> On Fri, 2022-01-21 at 16:56 -0600, Rob Herring wrote:
> > On Sun, Jan 09, 2022 at 03:54:35PM +0100, Sander Vanheule wrote:
> > > The interrupt router has 32 inputs and up to 15 outputs, and the way
> > > these are mapped to each other is runtime configurable. The outputs of
> > > this interrupt router on the other hand, are connected to a fixed set of
> > > parent interrupts. This means that "interrupt-map" is inappropriate, and
> > > rather a list of parent interrupts should be specified.
> > 
> > I'm not sure why interrupt-map is not appropriate. It is not appropriate 
> > if you have to touch the interrupt router h/w in servicing the 
> > interrupts. If you just need one time configuration of the mapping, then 
> > it should be fine to use I think.
> 
> If interrupt-map is used, then AFAICT there are no hooks to inform the driver that a
> translation has occurred. How should the interrupt controller driver then know how to set
> up the routing? Commit de4adddcbcc2 ("of/irq: Add a quirk for controllers with their own
> definition of interrupt-map") added a quirk for the original binding/driver, but that
> requires open-coding an interrupt-map parser in the driver.

The issue was not open-coding parsing, but was the need for something in 
the middle to service the interrupt. As 'interrupt-map' should be a 
transparent remapping or routing.

> 
> What this binding doesn't mention (I can add it), is that there are also two IRQ status
> registers to:
>   - unmask/mask SoC interrupts
>   - read the current status of the SoC interrupts

That would not be transparent.

> In theory, if the routing is set up correctly (and the IRQ permanently unmasked), I think
> one could treat interrupt-map as intended, and connect SoC peripheral IRQ handlers
> directly to the parent interrupts. But then the interrupt subsystem would need to check
> all attached handlers. This interrupt router/controller allows to check which peripheral
> is triggering the parent IRQ, which should be more efficient.
>
> These interrupt controllers are also used on multi-threaded systems, where each hardware
> thread has its own IRQ controller. I'm still experimenting with the implementation, but 
> there the routing registers would be used to set the CPU affinity of SoC interrupts.
> 
> I have to say that I'm not very familiar with the kernel code that handles all this
> though, so maybe I'm just missing something?

Okay, seems 'interrupt-map' is indeed not appropriate here.

Rob

[PATCH v3 5/6] irqchip/realtek-rtl: use parent interrupts

[Sander Vanheule]

The interrupt-map property for "realtek,rtl-intc" has been deprecated in
favor of a list of parent interrupts. Drop the open-coded parser for
interrupt-map, and use the first parent interrupt instead. If no parent
was provided, the driver will assume that this is the first hardware
interrupt of the SoC's MIPS CPU.

All SoC interrupts were treated equally, independent of which output
they were actually routed to. This means the driver might as well route
all interrupts to the first output, and achieve the same behaviour.

Without the interrupt-map property, interrupt routing information is no
longer available at initialisation. Routing setup will now happen later,
when a hardware interrupt is mapped by the subsystem.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 drivers/irqchip/irq-realtek-rtl.c | 133 ++++++++++++++----------------
 1 file changed, 61 insertions(+), 72 deletions(-)

diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c
index 50a56820c99b..388f4a7bfb80 100644
--- a/drivers/irqchip/irq-realtek-rtl.c
+++ b/drivers/irqchip/irq-realtek-rtl.c
@@ -21,11 +21,33 @@
 #define RTL_ICTL_IRR2		0x10
 #define RTL_ICTL_IRR3		0x14
 
+#define RTL_ICTL_NUM_INPUTS	32
+
 #define REG(x)		(realtek_ictl_base + x)
 
 static DEFINE_RAW_SPINLOCK(irq_lock);
 static void __iomem *realtek_ictl_base;
 
+/*
+ * IRR0-IRR3 store 4 bits per interrupt, but Realtek uses inverted numbering,
+ * placing IRQ 31 in the first four bits. A routing value of '0' means the
+ * interrupt is left disconnected. Routing values {1..15} connect to output
+ * lines {0..14}.
+ */
+#define IRR_OFFSET(idx)		(4 * (3 - (idx * 4) / 32))
+#define IRR_SHIFT(idx)		((idx * 4) % 32)
+
+static inline void write_irr(void __iomem *irr0, int idx, u32 value)
+{
+	unsigned int offset = IRR_OFFSET(idx);
+	unsigned int shift = IRR_SHIFT(idx);
+	u32 irr;
+
+	irr = readl(irr0 + offset) & ~(0xf << shift);
+	irr |= (value & 0xf) << shift;
+	writel(irr, irr0 + offset);
+}
+
 static void realtek_ictl_unmask_irq(struct irq_data *i)
 {
 	unsigned long flags;
@@ -62,8 +84,14 @@ static struct irq_chip realtek_ictl_irq = {
 
 static int intc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
 {
+	unsigned long flags;
+
 	irq_set_chip_and_handler(irq, &realtek_ictl_irq, handle_level_irq);
 
+	raw_spin_lock_irqsave(&irq_lock, flags);
+	write_irr(REG(RTL_ICTL_IRR0), hw, 1);
+	raw_spin_unlock_irqrestore(&irq_lock, flags);
+
 	return 0;
 }
 
@@ -95,90 +123,51 @@ static void realtek_irq_dispatch(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
-/*
- * SoC interrupts are cascaded to MIPS CPU interrupts according to the
- * interrupt-map in the device tree. Each SoC interrupt gets 4 bits for
- * the CPU interrupt in an Interrupt Routing Register. Max 32 SoC interrupts
- * thus go into 4 IRRs. A routing value of '0' means the interrupt is left
- * disconnected. Routing values {1..15} connect to output lines {0..14}.
- */
-static int __init map_interrupts(struct device_node *node, struct irq_domain *domain)
-{
-	struct device_node *cpu_ictl;
-	const __be32 *imap;
-	u32 imaplen, soc_int, cpu_int, tmp, regs[4];
-	int ret, i, irr_regs[] = {
-		RTL_ICTL_IRR3,
-		RTL_ICTL_IRR2,
-		RTL_ICTL_IRR1,
-		RTL_ICTL_IRR0,
-	};
-	u8 mips_irqs_set;
-
-	ret = of_property_read_u32(node, "#address-cells", &tmp);
-	if (ret || tmp)
-		return -EINVAL;
-
-	imap = of_get_property(node, "interrupt-map", &imaplen);
-	if (!imap || imaplen % 3)
-		return -EINVAL;
-
-	mips_irqs_set = 0;
-	memset(regs, 0, sizeof(regs));
-	for (i = 0; i < imaplen; i += 3 * sizeof(u32)) {
-		soc_int = be32_to_cpup(imap);
-		if (soc_int > 31)
-			return -EINVAL;
-
-		cpu_ictl = of_find_node_by_phandle(be32_to_cpup(imap + 1));
-		if (!cpu_ictl)
-			return -EINVAL;
-		ret = of_property_read_u32(cpu_ictl, "#interrupt-cells", &tmp);
-		if (ret || tmp != 1)
-			return -EINVAL;
-		of_node_put(cpu_ictl);
-
-		cpu_int = be32_to_cpup(imap + 2);
-		if (cpu_int > 7 || cpu_int < 2)
-			return -EINVAL;
-
-		if (!(mips_irqs_set & BIT(cpu_int))) {
-			irq_set_chained_handler_and_data(cpu_int, realtek_irq_dispatch,
-							 domain);
-			mips_irqs_set |= BIT(cpu_int);
-		}
-
-		/* Use routing values (1..6) for CPU interrupts (2..7) */
-		regs[(soc_int * 4) / 32] |= (cpu_int - 1) << (soc_int * 4) % 32;
-		imap += 3;
-	}
-
-	for (i = 0; i < 4; i++)
-		writel(regs[i], REG(irr_regs[i]));
-
-	return 0;
-}
-
 static int __init realtek_rtl_of_init(struct device_node *node, struct device_node *parent)
 {
+	struct of_phandle_args oirq;
 	struct irq_domain *domain;
-	int ret;
+	unsigned int soc_irq;
+	int parent_irq;
 
 	realtek_ictl_base = of_iomap(node, 0);
 	if (!realtek_ictl_base)
 		return -ENXIO;
 
-	/* Disable all cascaded interrupts */
+	/* Disable all cascaded interrupts and clear routing */
 	writel(0, REG(RTL_ICTL_GIMR));
+	for (soc_irq = 0; soc_irq < RTL_ICTL_NUM_INPUTS; soc_irq++)
+		write_irr(REG(RTL_ICTL_IRR0), soc_irq, 0);
+
+	if (WARN_ON(!of_irq_count(node))) {
+		/*
+		 * If DT contains no parent interrupts, assume MIPS CPU IRQ 2
+		 * (HW0) is connected to the first output. This is the case for
+		 * all known hardware anyway. "interrupt-map" is deprecated, so
+		 * don't bother trying to parse that.
+		 */
+		oirq.np = of_find_compatible_node(NULL, NULL, "mti,cpu-interrupt-controller");
+		oirq.args_count = 1;
+		oirq.args[0] = 2;
+
+		parent_irq = irq_create_of_mapping(&oirq);
+
+		of_node_put(oirq.np);
+	} else {
+		parent_irq = of_irq_get(node, 0);
+	}
+
+	if (parent_irq < 0)
+		return parent_irq;
+	else if (!parent_irq)
+		return -ENODEV;
 
-	domain = irq_domain_add_simple(node, 32, 0,
+	domain = irq_domain_add_simple(node, RTL_ICTL_NUM_INPUTS, 0,
 				       &irq_domain_ops, NULL);
+	if (!domain)
+		return -ENOMEM;
 
-	ret = map_interrupts(node, domain);
-	if (ret) {
-		pr_err("invalid interrupt map\n");
-		return ret;
-	}
+	irq_set_chained_handler_and_data(parent_irq, realtek_irq_dispatch, domain);
 
 	return 0;
 }
-- 
2.33.1

[PATCH v3 6/6] irqchip/realtek-rtl: use per-parent domains

[Sander Vanheule]

The interrupt router can connect each of its inputs to one of the parent
interrupts. These parent interrupts may be handled differently by their
interrupt controller. SoC interrupts should be treated per-parent, to
maintain this expected behaviour for routed child interrupts.

Rework the driver to use a separate domain for each output, using all
available parents interrupts (as specified in the devicetree).

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 drivers/irqchip/irq-realtek-rtl.c | 150 ++++++++++++++++++++++++------
 1 file changed, 124 insertions(+), 26 deletions(-)

diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c
index 388f4a7bfb80..868eb9b25e84 100644
--- a/drivers/irqchip/irq-realtek-rtl.c
+++ b/drivers/irqchip/irq-realtek-rtl.c
@@ -22,12 +22,22 @@
 #define RTL_ICTL_IRR3		0x14
 
 #define RTL_ICTL_NUM_INPUTS	32
+#define RTL_ICTL_NUM_OUTPUTS	15
 
 #define REG(x)		(realtek_ictl_base + x)
 
 static DEFINE_RAW_SPINLOCK(irq_lock);
 static void __iomem *realtek_ictl_base;
 
+struct realtek_ictl_output {
+	/* IRQ controller data */
+	struct fwnode_handle *fwnode;
+	/* Output specific data */
+	unsigned int output_index;
+	struct irq_domain *domain;
+	u32 child_mask;
+};
+
 /*
  * IRR0-IRR3 store 4 bits per interrupt, but Realtek uses inverted numbering,
  * placing IRQ 31 in the first four bits. A routing value of '0' means the
@@ -37,6 +47,11 @@ static void __iomem *realtek_ictl_base;
 #define IRR_OFFSET(idx)		(4 * (3 - (idx * 4) / 32))
 #define IRR_SHIFT(idx)		((idx * 4) % 32)
 
+static inline u32 read_irr(void __iomem *irr0, int idx)
+{
+	return (readl(irr0 + IRR_OFFSET(idx)) >> IRR_SHIFT(idx)) & 0xf;
+}
+
 static inline void write_irr(void __iomem *irr0, int idx, u32 value)
 {
 	unsigned int offset = IRR_OFFSET(idx);
@@ -84,51 +99,128 @@ static struct irq_chip realtek_ictl_irq = {
 
 static int intc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
 {
+	struct realtek_ictl_output *output = d->host_data;
 	unsigned long flags;
+	u32 routing_old;
+	int err = 0;
+
+	raw_spin_lock_irqsave(&irq_lock, flags);
+
+	/*
+	 * Inputs can only be routed to one output, so they shouldn't end up in
+	 * multiple domains. Perform this check in the same atomic context as
+	 * configuring the routing to prevent races.
+	 */
+	routing_old = read_irr(REG(RTL_ICTL_IRR0), hw);
+	if (routing_old && output->output_index != routing_old - 1) {
+		pr_err("int %ld already routed to output %d\n",
+			hw, routing_old - 1);
+		err = -EINVAL;
+		goto out;
+	}
+
+	output->child_mask |= BIT(hw);
+	write_irr(REG(RTL_ICTL_IRR0), hw, output->output_index + 1);
 
 	irq_set_chip_and_handler(irq, &realtek_ictl_irq, handle_level_irq);
 
-	raw_spin_lock_irqsave(&irq_lock, flags);
-	write_irr(REG(RTL_ICTL_IRR0), hw, 1);
+out:
 	raw_spin_unlock_irqrestore(&irq_lock, flags);
 
-	return 0;
+	return err;
+}
+
+static int intc_select(struct irq_domain *d, struct irq_fwspec *fwspec,
+	enum irq_domain_bus_token bus_token)
+{
+	struct realtek_ictl_output *output = d->host_data;
+
+	if (fwspec->fwnode != output->fwnode)
+		return false;
+
+	/* Original specifiers only had one parameter */
+	if (WARN_ON_ONCE(fwspec->param_count < 2))
+		return true;
+
+	return fwspec->param[1] == output->output_index;
 }
 
 static const struct irq_domain_ops irq_domain_ops = {
 	.map = intc_map,
+	.select = intc_select,
 	.xlate = irq_domain_xlate_onecell,
 };
 
 static void realtek_irq_dispatch(struct irq_desc *desc)
 {
+	struct realtek_ictl_output *output = irq_desc_get_handler_data(desc);
 	struct irq_chip *chip = irq_desc_get_chip(desc);
-	struct irq_domain *domain;
 	unsigned long pending;
 	unsigned int soc_int;
 
 	chained_irq_enter(chip, desc);
-	pending = readl(REG(RTL_ICTL_GIMR)) & readl(REG(RTL_ICTL_GISR));
+	pending = readl(REG(RTL_ICTL_GIMR)) & readl(REG(RTL_ICTL_GISR))
+		& output->child_mask;
 
 	if (unlikely(!pending)) {
 		spurious_interrupt();
 		goto out;
 	}
 
-	domain = irq_desc_get_handler_data(desc);
-	for_each_set_bit(soc_int, &pending, 32)
-		generic_handle_domain_irq(domain, soc_int);
+	for_each_set_bit(soc_int, &pending, RTL_ICTL_NUM_INPUTS)
+		generic_handle_domain_irq(output->domain, soc_int);
 
 out:
 	chained_irq_exit(chip, desc);
 }
 
+static int __init setup_parent_interrupts(struct device_node *node, int *parents,
+	unsigned int num_parents)
+{
+	struct realtek_ictl_output *outputs;
+	struct realtek_ictl_output *output;
+	struct irq_domain *domain;
+	unsigned int p;
+
+	outputs = kcalloc(num_parents, sizeof(*outputs), GFP_KERNEL);
+	if (!outputs)
+		return -ENOMEM;
+
+	for (p = 0; p < num_parents; p++) {
+		output = outputs + p;
+
+		domain = irq_domain_add_simple(node, RTL_ICTL_NUM_INPUTS, 0,
+					       &irq_domain_ops, output);
+		if (!domain)
+			goto domain_err;
+
+		output->fwnode = of_node_to_fwnode(node);
+		output->output_index = p;
+		output->domain = domain;
+
+		irq_set_chained_handler_and_data(parents[p], realtek_irq_dispatch, output);
+	}
+
+	return 0;
+
+domain_err:
+	while (p--) {
+		irq_set_chained_handler_and_data(parents[p], NULL, NULL);
+		irq_domain_remove(outputs[p].domain);
+	}
+
+	kfree(outputs);
+
+	return -ENOMEM;
+}
+
 static int __init realtek_rtl_of_init(struct device_node *node, struct device_node *parent)
 {
+	int parent_irqs[RTL_ICTL_NUM_OUTPUTS];
 	struct of_phandle_args oirq;
-	struct irq_domain *domain;
+	unsigned int num_parents;
 	unsigned int soc_irq;
-	int parent_irq;
+	unsigned int p;
 
 	realtek_ictl_base = of_iomap(node, 0);
 	if (!realtek_ictl_base)
@@ -139,37 +231,43 @@ static int __init realtek_rtl_of_init(struct device_node *node, struct device_no
 	for (soc_irq = 0; soc_irq < RTL_ICTL_NUM_INPUTS; soc_irq++)
 		write_irr(REG(RTL_ICTL_IRR0), soc_irq, 0);
 
-	if (WARN_ON(!of_irq_count(node))) {
+	num_parents = of_irq_count(node);
+	if (num_parents > RTL_ICTL_NUM_OUTPUTS) {
+		pr_err("too many parent interrupts\n");
+		return -EINVAL;
+	}
+
+	for (p = 0; p < num_parents; p++)
+		parent_irqs[p] = of_irq_get(node, p);
+
+	if (WARN_ON(!num_parents)) {
 		/*
 		 * If DT contains no parent interrupts, assume MIPS CPU IRQ 2
 		 * (HW0) is connected to the first output. This is the case for
 		 * all known hardware anyway. "interrupt-map" is deprecated, so
 		 * don't bother trying to parse that.
+		 * Since this is to account for old devicetrees with one-cell
+		 * interrupt specifiers, only one output domain is needed.
 		 */
 		oirq.np = of_find_compatible_node(NULL, NULL, "mti,cpu-interrupt-controller");
 		oirq.args_count = 1;
 		oirq.args[0] = 2;
 
-		parent_irq = irq_create_of_mapping(&oirq);
+		parent_irqs[0] = irq_create_of_mapping(&oirq);
+		num_parents = 1;
 
 		of_node_put(oirq.np);
-	} else {
-		parent_irq = of_irq_get(node, 0);
 	}
 
-	if (parent_irq < 0)
-		return parent_irq;
-	else if (!parent_irq)
-		return -ENODEV;
-
-	domain = irq_domain_add_simple(node, RTL_ICTL_NUM_INPUTS, 0,
-				       &irq_domain_ops, NULL);
-	if (!domain)
-		return -ENOMEM;
-
-	irq_set_chained_handler_and_data(parent_irq, realtek_irq_dispatch, domain);
+	/* Ensure we haven't collected any errors before proceeding */
+	for (p = 0; p < num_parents; p++) {
+		if (parent_irqs[p] < 0)
+			return parent_irqs[p];
+		if (!parent_irqs[p])
+			return -ENODEV;
+	}
 
-	return 0;
+	return setup_parent_interrupts(node, &parent_irqs[0], num_parents);
 }
 
 IRQCHIP_DECLARE(realtek_rtl_intc, "realtek,rtl-intc", realtek_rtl_of_init);
-- 
2.33.1

Re: [PATCH v3 0/6] Rework realtek-rtl IRQ driver

[Marc Zyngier]

On Sun, 09 Jan 2022 14:54:31 +0000,
Sander Vanheule <sander@svanheule.net> wrote:
> 
> After seeing some use, and with more devices tested, the current
> implementation for the Realtek SoC interrupt controller was found to
> contain a few flaws.

FWIW, I have queued the first 3 patches as fixes for 5.17. I'll look
at the rest after the merge window.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.