diff mbox series

[02/12] dt-bindings: interrupt-controller: stm32-exti: Add irq nexus child node

Message ID 20240216094758.916722-3-antonio.borneo@foss.st.com (mailing list archive)
State New, archived
Headers show
Series irqchip/stm32-exti: add irq-map and STM32MP25 support | expand

Commit Message

Antonio Borneo Feb. 16, 2024, 9:47 a.m. UTC 
The mapping of EXTI interrupts to its parent interrupt controller
is both SoC and instance dependent.
The current implementation requires adding a new table to the
driver's code and a new compatible for each new EXTI instance.

Add to the binding an interrupt nexus child node that will be
used on the new EXTI instances and can be optionally used on the
existing instances.
The property 'interrupt-map' in the nexus node maps each EXTI
interrupt to the parent interrupt.
Align #address-cells and #interrupt-cells between the EXTI node
and its nexus node.

Signed-off-by: Antonio Borneo <antonio.borneo@foss.st.com>
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 .../interrupt-controller/st,stm32-exti.yaml   | 42 ++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

Comments

Thomas Gleixner Feb. 19, 2024, 2:19 p.m. UTC | #1 
On Fri, Feb 16 2024 at 10:47, Antonio Borneo wrote:
> The mapping of EXTI interrupts to its parent interrupt controller
> is both SoC and instance dependent.
> The current implementation requires adding a new table to the
> driver's code and a new compatible for each new EXTI instance.
>
> Add to the binding an interrupt nexus child node that will be
> used on the new EXTI instances and can be optionally used on the
> existing instances.
> The property 'interrupt-map' in the nexus node maps each EXTI
> interrupt to the parent interrupt.
> Align #address-cells and #interrupt-cells between the EXTI node
> and its nexus node.
>
> Signed-off-by: Antonio Borneo <antonio.borneo@foss.st.com>
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

This S-O-B chain is broken as it suggests that you wrote the patch and
Fabrice relayed it.

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

But that's not the case. If you co-developed this with Fabrice then
please use Co-developed-by. See:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

Thanks,

        tglx
Rob Herring (Arm) Feb. 22, 2024, 11:43 p.m. UTC | #2 
On Fri, Feb 16, 2024 at 10:47:47AM +0100, Antonio Borneo wrote:
> The mapping of EXTI interrupts to its parent interrupt controller
> is both SoC and instance dependent.
> The current implementation requires adding a new table to the
> driver's code and a new compatible for each new EXTI instance.
> 
> Add to the binding an interrupt nexus child node that will be
> used on the new EXTI instances and can be optionally used on the
> existing instances.
> The property 'interrupt-map' in the nexus node maps each EXTI
> interrupt to the parent interrupt.
> Align #address-cells and #interrupt-cells between the EXTI node
> and its nexus node.

Looks like an abuse of interrupt-map. You avoid adding yourself to the 
abuser list by putting it in a child node. Clever. (See list in 
drivers/of/irq.c if you don't know what I'm talking about)

I assume the EXTI has 0..N interrupts. Just define 'interrupts' with N 
entries with each entry mapping EXTI interrupt N to 'interrupts' entry 
N.

Rob
Antonio Borneo Feb. 23, 2024, 1:39 p.m. UTC | #3 
On Thu, 2024-02-22 at 16:43 -0700, Rob Herring wrote:
> On Fri, Feb 16, 2024 at 10:47:47AM +0100, Antonio Borneo wrote:
> > The mapping of EXTI interrupts to its parent interrupt controller
> > is both SoC and instance dependent.
> > The current implementation requires adding a new table to the
> > driver's code and a new compatible for each new EXTI instance.
> > 
> > Add to the binding an interrupt nexus child node that will be
> > used on the new EXTI instances and can be optionally used on the
> > existing instances.
> > The property 'interrupt-map' in the nexus node maps each EXTI
> > interrupt to the parent interrupt.
> > Align #address-cells and #interrupt-cells between the EXTI node
> > and its nexus node.
> 
> Looks like an abuse of interrupt-map. You avoid adding yourself to the 
> abuser list by putting it in a child node. Clever. (See list in 
> drivers/of/irq.c if you don't know what I'm talking about)

Hi Rob,
thanks for the review.

Yes, I know already about the abuser list but, from the commit
message and the associated comment, I interpret it as an incorrect
use of the property interrupt-map with custom syntax thus relying 
on custom parsing code.
The child nexus node in this series allows using the default parser
in kernel.

From your reply, looks like my interpretation is incorrect and I
missed the real concern about the abuser list.
Could you please explain why this use of interrupt-map is incorrect
and/or which are the correct use cases?

> I assume the EXTI has 0..N interrupts. Just define 'interrupts' with N 
> entries with each entry mapping EXTI interrupt N to 'interrupts' entry 
> N.

Yes, EXTI has 0..N interrupts that can be mapped to multiple
parent interrupt controllers and the mapping table has holes.
While the DT in this series only use one interrupt parent, a second
parent will follow.
So 'interrupts-extended' property would be a better matching than
'interrupts' to handle the multiple parents.

But how to code the missing entries in an 'interrupts-extended' list?
As in the example in Documentation/devicetree/bindings/dma/apple,admac.yaml ?

The 'interrupt-map' contains the matching EXTI index, thus allowing
a 'sparse' map where holes are simply ignored.

Best Regards,
Antonio
Antonio Borneo April 15, 2024, 1:37 p.m. UTC | #4 
On Mon, 2024-02-19 at 15:19 +0100, Thomas Gleixner wrote:
> On Fri, Feb 16 2024 at 10:47, Antonio Borneo wrote:
> > The mapping of EXTI interrupts to its parent interrupt controller
> > is both SoC and instance dependent.
> > The current implementation requires adding a new table to the
> > driver's code and a new compatible for each new EXTI instance.
> > 
> > Add to the binding an interrupt nexus child node that will be
> > used on the new EXTI instances and can be optionally used on the
> > existing instances.
> > The property 'interrupt-map' in the nexus node maps each EXTI
> > interrupt to the parent interrupt.
> > Align #address-cells and #interrupt-cells between the EXTI node
> > and its nexus node.
> > 
> > Signed-off-by: Antonio Borneo <antonio.borneo@foss.st.com>
> > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> 
> This S-O-B chain is broken as it suggests that you wrote the patch and
> Fabrice relayed it.
> 
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
> 
> But that's not the case. If you co-developed this with Fabrice then
> please use Co-developed-by. See:
> 
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
> 
> Thanks,
> 
>         tglx
> 

Thanks for the review.
I'm sending a V2 addressing all the remarks.

Regards,
Antonio
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/interrupt-controller/st,stm32-exti.yaml b/Documentation/devicetree/bindings/interrupt-controller/st,stm32-exti.yaml
index 00c10a8258f1..1a4cf9537b9e 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/st,stm32-exti.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/st,stm32-exti.yaml
@@ -26,6 +26,9 @@  properties:
   "#interrupt-cells":
     const: 2
 
+  "#address-cells":
+    const: 0
+
   reg:
     maxItems: 1
 
@@ -42,6 +45,24 @@  properties:
     description:
       Interrupts references to primary interrupt controller
 
+  exti-interrupt-map:
+    type: object
+    properties:
+      interrupt-map: true
+
+      interrupt-map-mask: true
+
+      "#interrupt-cells":
+        const: 2
+
+      "#address-cells":
+        const: 0
+
+    required:
+      - interrupt-map
+      - "#interrupt-cells"
+      - "#address-cells"
+
 required:
   - "#interrupt-cells"
   - compatible
@@ -89,8 +110,27 @@  examples:
         reg = <0x5000d000 0x400>;
     };
 
+  - |
     //Example 2
-    exti2: interrupt-controller@40013c00 {
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    exti2: interrupt-controller@5000d000 {
+        compatible = "st,stm32mp1-exti", "syscon";
+        interrupt-controller;
+        #interrupt-cells = <2>;
+        reg = <0x5000d000 0x400>;
+        exti-interrupt-map {
+            #address-cells = <0>;
+            #interrupt-cells = <2>;
+            interrupt-map-mask = <0xffffffff 0>;
+            interrupt-map =
+                <0 0 &intc GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
+                <3 0 &intc GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
+        };
+    };
+
+  - |
+    //Example 3
+    exti3: interrupt-controller@40013c00 {
         compatible = "st,stm32-exti";
         interrupt-controller;
         #interrupt-cells = <2>;