[PATCH v3 0/2] irqchip: aspeed: Add AST2700 INTC debugfs support and yaml update

[PATCH v3 0/2] irqchip: aspeed: Add AST2700 INTC debugfs support and yaml update

[Ryan Chen]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="UTF-8", Size: 860 bytes --]

This patch series adds device tree bindings and driver support for the
AST2700 SoC¡¦s two interrupt controllers (INTC0 and INTC1), along with
debugfs entries for runtime inspection of routing and register protection
status, and bindings example refine.

v3:
- aspeed,ast2700-intc.yaml
  - improve commit message description.
- irq-aspeed-intc.c
  - add platform driver for "aspeed,ast2700-intc0/1" compatible nodes.

v2:
- fix dt bindingcheck

Ryan Chen (2):
  dt-bindings: interrupt-controller: aspeed: Add parent node compatibles
    and refine documentation
  irqchip: aspeed: add debugfs support and AST2700 INTC0/INTC1
    routing/protection display

 .../aspeed,ast2700-intc.yaml                  | 158 ++++++++----
 drivers/irqchip/irq-aspeed-intc.c             | 238 ++++++++++++++++++
 2 files changed, 353 insertions(+), 43 deletions(-)

-- 
2.34.1

[PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation

[Ryan Chen]

The AST2700 SoC contains two independent top-level interrupt controllers
(INTC0 and INTC1), each responsible for handling different peripheral
groups and occupying separate register spaces. Above them, PSP(CA35) GIC
controller acts as the root interrupt aggregator. Accurately describing
this hierarchical hardware structure in the device tree requires distinct
compatible strings for the parent nodes of INTC0 and INTC1.

- Adds 'aspeed,ast2700-intc0' and 'aspeed,ast2700-intc1' compatible
 strings for parent interrupt controller nodes. (in addition to the
 existing 'aspeed,ast2700-intc-ic' for child nodes)
- Clarifies the relationship and function of INTC0 parent
 (intc0_0~x: child), INTC1 parent (intc1_0~x: child), and the GIC
 in the documentation.
- Updates block diagrams and device tree examples to illustrate
 the hierarchy and compatible usage.
- Refines documentation and example formatting.

This change allows the device tree and driver to distinguish between
parent (top-level) and child (group) interrupt controller nodes,
enabling more precise driver matching SOC register space allocation.

Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
---
 .../aspeed,ast2700-intc.yaml                  | 158 +++++++++++++-----
 1 file changed, 115 insertions(+), 43 deletions(-)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml
index 55636d06a674..bdc4d8835843 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml
@@ -10,6 +10,33 @@ description:
   This interrupt controller hardware is second level interrupt controller that
   is hooked to a parent interrupt controller. It's useful to combine multiple
   interrupt sources into 1 interrupt to parent interrupt controller.
+  Depend to which INTC0 or INTC1 used.
+  INTC0 and INTC1 are two kinds of interrupt controller with enable and raw
+  status registers for use.
+  INTC0 is used to assert GIC if interrupt in INTC1 asserted.
+  INTC1 is used to assert INTC0 if interrupt of modules asserted.
+  +-----+   +---------+
+  | GIC |---|  INTC0  |
+  +-----+   +---------+
+            +---------+
+            |         |---module0
+            | INTC0_0 |---module1
+            |         |---...
+            +---------+---module31
+            |---....  |
+            +---------+
+            |         |     +---------+
+            | INTC0_11| +---| INTC1   |
+            |         |     +---------+
+            +---------+     +---------+---module0
+                            | INTC1_0 |---module1
+                            |         |---...
+                            +---------+---module31
+                            ...
+                            +---------+---module0
+                            | INTC1_5 |---module1
+                            |         |---...
+                            +---------+---module31
 
 maintainers:
   - Kevin Chen <kevin_chen@aspeedtech.com>
@@ -17,49 +44,70 @@ maintainers:
 properties:
   compatible:
     enum:
-      - aspeed,ast2700-intc-ic
+      - aspeed,ast2700-intc0
+      - aspeed,ast2700-intc1
 
   reg:
     maxItems: 1
 
-  interrupt-controller: true
+  '#address-cells':
+    const: 2
 
-  '#interrupt-cells':
+  '#size-cells':
     const: 2
-    description:
-      The first cell is the IRQ number, the second cell is the trigger
-      type as defined in interrupt.txt in this directory.
-
-  interrupts:
-    maxItems: 6
-    description: |
-      Depend to which INTC0 or INTC1 used.
-      INTC0 and INTC1 are two kinds of interrupt controller with enable and raw
-      status registers for use.
-      INTC0 is used to assert GIC if interrupt in INTC1 asserted.
-      INTC1 is used to assert INTC0 if interrupt of modules asserted.
-      +-----+   +-------+     +---------+---module0
-      | GIC |---| INTC0 |--+--| INTC1_0 |---module2
-      |     |   |       |  |  |         |---...
-      +-----+   +-------+  |  +---------+---module31
-                           |
-                           |   +---------+---module0
-                           +---| INTC1_1 |---module2
-                           |   |         |---...
-                           |   +---------+---module31
-                          ...
-                           |   +---------+---module0
-                           +---| INTC1_5 |---module2
-                               |         |---...
-                               +---------+---module31
 
+  ranges: true
+
+patternProperties:
+  "^interrupt-controller@":
+    type: object
+    description: Interrupt group child nodes
+    additionalProperties: false
+
+    properties:
+      compatible:
+        enum:
+          - aspeed,ast2700-intc-ic
+
+      reg:
+        maxItems: 1
+
+      interrupt-controller: true
+
+      '#interrupt-cells':
+        const: 2
+        description: |
+          The first cell is the IRQ number, the second cell is the trigger
+          type as defined in interrupt.txt in this directory.
+
+      interrupts:
+        minItems: 1
+        maxItems: 6
+        description: |
+          The interrupts provided by this interrupt controller.
+
+      interrupts-extended:
+        minItems: 1
+        maxItems: 6
+        description: |
+          This property is required when defining a cascaded interrupt controller
+          that is connected under another interrupt controller. It specifies the
+          parent interrupt(s) in the upstream controller to which this controller
+          is connected.
+
+    oneOf:
+      - required: [interrupts]
+      - required: [interrupts-extended]
+
+    required:
+      - compatible
+      - reg
+      - interrupt-controller
+      - '#interrupt-cells'
 
 required:
   - compatible
   - reg
-  - interrupt-controller
-  - '#interrupt-cells'
-  - interrupts
 
 additionalProperties: false
 
@@ -68,19 +116,43 @@ examples:
     #include <dt-bindings/interrupt-controller/arm-gic.h>
 
     bus {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      intc0: interrupt-controller@12100000 {
+        compatible = "aspeed,ast2700-intc0";
+        reg = <0 0x12100000 0 0x4000>;
+        ranges = <0x0 0x0 0x0 0x12100000 0x0 0x4000>;
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        intc0_11: interrupt-controller@1b00 {
+          compatible = "aspeed,ast2700-intc-ic";
+          reg = <0 0x12101b00 0 0x10>;
+          #interrupt-cells = <2>;
+          interrupt-controller;
+          interrupts = <GIC_SPI 192 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
+                       <GIC_SPI 193 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
+                       <GIC_SPI 194 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
+                       <GIC_SPI 195 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
+                       <GIC_SPI 196 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
+                       <GIC_SPI 197 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
+        };
+      };
+
+      intc1: interrupt-controller@14c18000 {
+        compatible = "aspeed,ast2700-intc1";
+        reg = <0 0x14c18000 0 0x400>;
+        ranges = <0x0 0x0 0x0 0x14c18000 0x0 0x400>;
         #address-cells = <2>;
         #size-cells = <2>;
 
-        interrupt-controller@12101b00 {
-            compatible = "aspeed,ast2700-intc-ic";
-            reg = <0 0x12101b00 0 0x10>;
-            #interrupt-cells = <2>;
-            interrupt-controller;
-            interrupts = <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 193 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 194 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 195 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 196 IRQ_TYPE_LEVEL_HIGH>,
-                         <GIC_SPI 197 IRQ_TYPE_LEVEL_HIGH>;
+        intc1_0: interrupt-controller@100 {
+          compatible = "aspeed,ast2700-intc-ic";
+          reg = <0x0 0x100 0x0 0x10>;
+          #interrupt-cells = <2>;
+          interrupt-controller;
+          interrupts-extended = <&intc0_11 0 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
         };
+      };
     };
-- 
2.34.1

Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation

[Thomas Gleixner]

On Tue, Jul 22 2025 at 17:51, Ryan Chen wrote:
> -        interrupt-controller@12101b00 {
> -            compatible = "aspeed,ast2700-intc-ic";
> -            reg = <0 0x12101b00 0 0x10>;
> -            #interrupt-cells = <2>;
> -            interrupt-controller;
> -            interrupts = <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 193 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 194 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 195 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 196 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 197 IRQ_TYPE_LEVEL_HIGH>;
> +        intc1_0: interrupt-controller@100 {
> +          compatible = "aspeed,ast2700-intc-ic";
> +          reg = <0x0 0x100 0x0 0x10>;

I doubt that the controller base address is at 0x100 ...

RE: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation

[Ryan Chen]

> Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add
> parent node compatibles and refine documentation
> 
> On Tue, Jul 22 2025 at 17:51, Ryan Chen wrote:
> > -        interrupt-controller@12101b00 {
> > -            compatible = "aspeed,ast2700-intc-ic";
> > -            reg = <0 0x12101b00 0 0x10>;
> > -            #interrupt-cells = <2>;
> > -            interrupt-controller;
> > -            interrupts = <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>,
> > -                         <GIC_SPI 193 IRQ_TYPE_LEVEL_HIGH>,
> > -                         <GIC_SPI 194 IRQ_TYPE_LEVEL_HIGH>,
> > -                         <GIC_SPI 195 IRQ_TYPE_LEVEL_HIGH>,
> > -                         <GIC_SPI 196 IRQ_TYPE_LEVEL_HIGH>,
> > -                         <GIC_SPI 197 IRQ_TYPE_LEVEL_HIGH>;
> > +        intc1_0: interrupt-controller@100 {
> > +          compatible = "aspeed,ast2700-intc-ic";
> > +          reg = <0x0 0x100 0x0 0x10>;
> 
> I doubt that the controller base address is at 0x100 ...
Sorry, besides the interrupt cascade, our interrupt architecture is most like this one.
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/interrupt-controller/marvell%2Ccp110-icu.yaml#L74-L98
and also others
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/mmc/aspeed%2Csdhci.yaml#L83-L107

I don't understand you doubt it, and also we have proven in our internal Linux release.
https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi#L1676-L1730

Could you point out more information what you doubt? And I can provide more information.

Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation

[Rob Herring]

On Tue, Jul 22, 2025 at 05:51:55PM +0800, Ryan Chen wrote:
> The AST2700 SoC contains two independent top-level interrupt controllers
> (INTC0 and INTC1), each responsible for handling different peripheral
> groups and occupying separate register spaces. Above them, PSP(CA35) GIC
> controller acts as the root interrupt aggregator. Accurately describing
> this hierarchical hardware structure in the device tree requires distinct
> compatible strings for the parent nodes of INTC0 and INTC1.
> 
> - Adds 'aspeed,ast2700-intc0' and 'aspeed,ast2700-intc1' compatible
>  strings for parent interrupt controller nodes. (in addition to the
>  existing 'aspeed,ast2700-intc-ic' for child nodes)
> - Clarifies the relationship and function of INTC0 parent
>  (intc0_0~x: child), INTC1 parent (intc1_0~x: child), and the GIC
>  in the documentation.
> - Updates block diagrams and device tree examples to illustrate
>  the hierarchy and compatible usage.
> - Refines documentation and example formatting.
> 
> This change allows the device tree and driver to distinguish between
> parent (top-level) and child (group) interrupt controller nodes,
> enabling more precise driver matching SOC register space allocation.

That's nice, but is an ABI break.

> 
> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> ---
>  .../aspeed,ast2700-intc.yaml                  | 158 +++++++++++++-----
>  1 file changed, 115 insertions(+), 43 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml
> index 55636d06a674..bdc4d8835843 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml
> +++ b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml
> @@ -10,6 +10,33 @@ description:
>    This interrupt controller hardware is second level interrupt controller that
>    is hooked to a parent interrupt controller. It's useful to combine multiple
>    interrupt sources into 1 interrupt to parent interrupt controller.
> +  Depend to which INTC0 or INTC1 used.
> +  INTC0 and INTC1 are two kinds of interrupt controller with enable and raw
> +  status registers for use.
> +  INTC0 is used to assert GIC if interrupt in INTC1 asserted.
> +  INTC1 is used to assert INTC0 if interrupt of modules asserted.
> +  +-----+   +---------+
> +  | GIC |---|  INTC0  |
> +  +-----+   +---------+
> +            +---------+
> +            |         |---module0
> +            | INTC0_0 |---module1
> +            |         |---...
> +            +---------+---module31
> +            |---....  |
> +            +---------+
> +            |         |     +---------+
> +            | INTC0_11| +---| INTC1   |
> +            |         |     +---------+
> +            +---------+     +---------+---module0
> +                            | INTC1_0 |---module1
> +                            |         |---...
> +                            +---------+---module31
> +                            ...
> +                            +---------+---module0
> +                            | INTC1_5 |---module1
> +                            |         |---...
> +                            +---------+---module31
>  
>  maintainers:
>    - Kevin Chen <kevin_chen@aspeedtech.com>
> @@ -17,49 +44,70 @@ maintainers:
>  properties:
>    compatible:
>      enum:
> -      - aspeed,ast2700-intc-ic
> +      - aspeed,ast2700-intc0
> +      - aspeed,ast2700-intc1
>  
>    reg:
>      maxItems: 1
>  
> -  interrupt-controller: true
> +  '#address-cells':
> +    const: 2
>  
> -  '#interrupt-cells':
> +  '#size-cells':
>      const: 2
> -    description:
> -      The first cell is the IRQ number, the second cell is the trigger
> -      type as defined in interrupt.txt in this directory.
> -
> -  interrupts:
> -    maxItems: 6
> -    description: |
> -      Depend to which INTC0 or INTC1 used.
> -      INTC0 and INTC1 are two kinds of interrupt controller with enable and raw
> -      status registers for use.
> -      INTC0 is used to assert GIC if interrupt in INTC1 asserted.
> -      INTC1 is used to assert INTC0 if interrupt of modules asserted.
> -      +-----+   +-------+     +---------+---module0
> -      | GIC |---| INTC0 |--+--| INTC1_0 |---module2
> -      |     |   |       |  |  |         |---...
> -      +-----+   +-------+  |  +---------+---module31
> -                           |
> -                           |   +---------+---module0
> -                           +---| INTC1_1 |---module2
> -                           |   |         |---...
> -                           |   +---------+---module31
> -                          ...
> -                           |   +---------+---module0
> -                           +---| INTC1_5 |---module2
> -                               |         |---...
> -                               +---------+---module31
>  
> +  ranges: true
> +
> +patternProperties:
> +  "^interrupt-controller@":
> +    type: object
> +    description: Interrupt group child nodes
> +    additionalProperties: false
> +
> +    properties:
> +      compatible:
> +        enum:
> +          - aspeed,ast2700-intc-ic
> +
> +      reg:
> +        maxItems: 1
> +
> +      interrupt-controller: true
> +
> +      '#interrupt-cells':
> +        const: 2
> +        description: |

Don't need '|'.

> +          The first cell is the IRQ number, the second cell is the trigger
> +          type as defined in interrupt.txt in this directory.

Don't add links to legacy documents.

> +
> +      interrupts:
> +        minItems: 1
> +        maxItems: 6
> +        description: |
> +          The interrupts provided by this interrupt controller.
> +
> +      interrupts-extended:

Why do you have both interrupts and interrupts-extended? They are 
mutually exclusive and both are auto-magically supported. The schemas 
only have to document 'interrupts'.

> +        minItems: 1
> +        maxItems: 6
> +        description: |
> +          This property is required when defining a cascaded interrupt controller
> +          that is connected under another interrupt controller. It specifies the
> +          parent interrupt(s) in the upstream controller to which this controller
> +          is connected.
> +
> +    oneOf:
> +      - required: [interrupts]
> +      - required: [interrupts-extended]
> +
> +    required:
> +      - compatible
> +      - reg
> +      - interrupt-controller
> +      - '#interrupt-cells'
>  
>  required:
>    - compatible
>    - reg
> -  - interrupt-controller
> -  - '#interrupt-cells'
> -  - interrupts
>  
>  additionalProperties: false
>  
> @@ -68,19 +116,43 @@ examples:
>      #include <dt-bindings/interrupt-controller/arm-gic.h>
>  
>      bus {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      intc0: interrupt-controller@12100000 {

This node isn't an interrupt-controller.

> +        compatible = "aspeed,ast2700-intc0";
> +        reg = <0 0x12100000 0 0x4000>;
> +        ranges = <0x0 0x0 0x0 0x12100000 0x0 0x4000>;
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        intc0_11: interrupt-controller@1b00 {
> +          compatible = "aspeed,ast2700-intc-ic";
> +          reg = <0 0x12101b00 0 0x10>;
> +          #interrupt-cells = <2>;
> +          interrupt-controller;
> +          interrupts = <GIC_SPI 192 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
> +                       <GIC_SPI 193 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
> +                       <GIC_SPI 194 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
> +                       <GIC_SPI 195 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
> +                       <GIC_SPI 196 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
> +                       <GIC_SPI 197 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
> +        };
> +      };
> +
> +      intc1: interrupt-controller@14c18000 {
> +        compatible = "aspeed,ast2700-intc1";
> +        reg = <0 0x14c18000 0 0x400>;
> +        ranges = <0x0 0x0 0x0 0x14c18000 0x0 0x400>;
>          #address-cells = <2>;
>          #size-cells = <2>;
>  
> -        interrupt-controller@12101b00 {
> -            compatible = "aspeed,ast2700-intc-ic";
> -            reg = <0 0x12101b00 0 0x10>;
> -            #interrupt-cells = <2>;
> -            interrupt-controller;
> -            interrupts = <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 193 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 194 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 195 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 196 IRQ_TYPE_LEVEL_HIGH>,
> -                         <GIC_SPI 197 IRQ_TYPE_LEVEL_HIGH>;
> +        intc1_0: interrupt-controller@100 {
> +          compatible = "aspeed,ast2700-intc-ic";
> +          reg = <0x0 0x100 0x0 0x10>;
> +          #interrupt-cells = <2>;
> +          interrupt-controller;
> +          interrupts-extended = <&intc0_11 0 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
>          };
> +      };
>      };
> -- 
> 2.34.1
> 

RE: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation

[Ryan Chen]

> Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add
> parent node compatibles and refine documentation
> 
> On Tue, Jul 22, 2025 at 05:51:55PM +0800, Ryan Chen wrote:
> > The AST2700 SoC contains two independent top-level interrupt
> > controllers
> > (INTC0 and INTC1), each responsible for handling different peripheral
> > groups and occupying separate register spaces. Above them, PSP(CA35)
> > GIC controller acts as the root interrupt aggregator. Accurately
> > describing this hierarchical hardware structure in the device tree
> > requires distinct compatible strings for the parent nodes of INTC0 and INTC1.
> >
> > - Adds 'aspeed,ast2700-intc0' and 'aspeed,ast2700-intc1' compatible
> > strings for parent interrupt controller nodes. (in addition to the
> > existing 'aspeed,ast2700-intc-ic' for child nodes)
> > - Clarifies the relationship and function of INTC0 parent
> >  (intc0_0~x: child), INTC1 parent (intc1_0~x: child), and the GIC  in
> > the documentation.
> > - Updates block diagrams and device tree examples to illustrate  the
> > hierarchy and compatible usage.
> > - Refines documentation and example formatting.
> >
> > This change allows the device tree and driver to distinguish between
> > parent (top-level) and child (group) interrupt controller nodes,
> > enabling more precise driver matching SOC register space allocation.
> 
> That's nice, but is an ABI break.

Sorry, Is it an ABI break?
I keep the compatible aspeed,ast2700-intc-ic, not change it.
Just move it to be child, and make aspeed,ast2700-intc0/1 to be parent for aspeed,ast2700-intc-ic.

Older : https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/interrupt-controller/aspeed%2Cast2700-intc.yaml#L70C2-L86C7
     bus {
        #address-cells = <2>;
        #size-cells = <2>;

        interrupt-controller@12101b00 {
            compatible = "aspeed,ast2700-intc-ic";
            reg = <0 0x12101b00 0 0x10>;
            #interrupt-cells = <2>;
            interrupt-controller;
            interrupts = <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>,
                         <GIC_SPI 193 IRQ_TYPE_LEVEL_HIGH>,
                         <GIC_SPI 194 IRQ_TYPE_LEVEL_HIGH>,
                         <GIC_SPI 195 IRQ_TYPE_LEVEL_HIGH>,
                         <GIC_SPI 196 IRQ_TYPE_LEVEL_HIGH>,
                         <GIC_SPI 197 IRQ_TYPE_LEVEL_HIGH>;
        };
    }; 

New parent and child
    bus {
      #address-cells = <2>;
      #size-cells = <2>;

      intc0: interrupt-controller@12100000 {
        compatible = "aspeed,ast2700-intc0";
        reg = <0 0x12100000 0 0x4000>;
        ranges = <0x0 0x0 0x0 0x12100000 0x0 0x4000>;
        #address-cells = <2>;
        #size-cells = <2>;

        intc0_11: interrupt-controller@1b00 {
          compatible = "aspeed,ast2700-intc-ic";
          reg = <0 0x12101b00 0 0x10>;
          #interrupt-cells = <2>;
          interrupt-controller;
          interrupts = <GIC_SPI 192 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
                       <GIC_SPI 193 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
                       <GIC_SPI 194 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
                       <GIC_SPI 195 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
                       <GIC_SPI 196 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>,
                       <GIC_SPI 197 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
        };
      };

      intc1: interrupt-controller@14c18000 {
        compatible = "aspeed,ast2700-intc1";
        reg = <0 0x14c18000 0 0x400>;
        ranges = <0x0 0x0 0x0 0x14c18000 0x0 0x400>;
        #address-cells = <2>;
        #size-cells = <2>;

        intc1_0: interrupt-controller@100 {
          compatible = "aspeed,ast2700-intc-ic";
          reg = <0x0 0x100 0x0 0x10>;
          #interrupt-cells = <2>;
          interrupt-controller;
          interrupts-extended = <&intc0_11 0 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
        };
      };
    };

> 
> >
> > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> > ---
> >  .../aspeed,ast2700-intc.yaml                  | 158
> +++++++++++++-----
> >  1 file changed, 115 insertions(+), 43 deletions(-)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast270
> > 0-intc.yaml
> > b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast270
> > 0-intc.yaml index 55636d06a674..bdc4d8835843 100644
> > ---
> > a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast270
> > 0-intc.yaml
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/aspeed,as
> > +++ t2700-intc.yaml
> > @@ -10,6 +10,33 @@ description:
> >    This interrupt controller hardware is second level interrupt controller
> that
> >    is hooked to a parent interrupt controller. It's useful to combine multiple
> >    interrupt sources into 1 interrupt to parent interrupt controller.
> > +  Depend to which INTC0 or INTC1 used.
> > +  INTC0 and INTC1 are two kinds of interrupt controller with enable
> > + and raw  status registers for use.
> > +  INTC0 is used to assert GIC if interrupt in INTC1 asserted.
> > +  INTC1 is used to assert INTC0 if interrupt of modules asserted.
> > +  +-----+   +---------+
> > +  | GIC |---|  INTC0  |
> > +  +-----+   +---------+
> > +            +---------+
> > +            |         |---module0
> > +            | INTC0_0 |---module1
> > +            |         |---...
> > +            +---------+---module31
> > +            |---....  |
> > +            +---------+
> > +            |         |     +---------+
> > +            | INTC0_11| +---| INTC1   |
> > +            |         |     +---------+
> > +            +---------+     +---------+---module0
> > +                            | INTC1_0 |---module1
> > +                            |         |---...
> > +                            +---------+---module31
> > +                            ...
> > +                            +---------+---module0
> > +                            | INTC1_5 |---module1
> > +                            |         |---...
> > +                            +---------+---module31
> >
> >  maintainers:
> >    - Kevin Chen <kevin_chen@aspeedtech.com> @@ -17,49 +44,70 @@
> > maintainers:
> >  properties:
> >    compatible:
> >      enum:
> > -      - aspeed,ast2700-intc-ic
> > +      - aspeed,ast2700-intc0
> > +      - aspeed,ast2700-intc1
> >
> >    reg:
> >      maxItems: 1
> >
> > -  interrupt-controller: true
> > +  '#address-cells':
> > +    const: 2
> >
> > -  '#interrupt-cells':
> > +  '#size-cells':
> >      const: 2
> > -    description:
> > -      The first cell is the IRQ number, the second cell is the trigger
> > -      type as defined in interrupt.txt in this directory.
> > -
> > -  interrupts:
> > -    maxItems: 6
> > -    description: |
> > -      Depend to which INTC0 or INTC1 used.
> > -      INTC0 and INTC1 are two kinds of interrupt controller with enable
> and raw
> > -      status registers for use.
> > -      INTC0 is used to assert GIC if interrupt in INTC1 asserted.
> > -      INTC1 is used to assert INTC0 if interrupt of modules asserted.
> > -      +-----+   +-------+     +---------+---module0
> > -      | GIC |---| INTC0 |--+--| INTC1_0 |---module2
> > -      |     |   |       |  |  |         |---...
> > -      +-----+   +-------+  |  +---------+---module31
> > -                           |
> > -                           |   +---------+---module0
> > -                           +---| INTC1_1 |---module2
> > -                           |   |         |---...
> > -                           |   +---------+---module31
> > -                          ...
> > -                           |   +---------+---module0
> > -                           +---| INTC1_5 |---module2
> > -                               |         |---...
> > -                               +---------+---module31
> >
> > +  ranges: true
> > +
> > +patternProperties:
> > +  "^interrupt-controller@":
> > +    type: object
> > +    description: Interrupt group child nodes
> > +    additionalProperties: false
> > +
> > +    properties:
> > +      compatible:
> > +        enum:
> > +          - aspeed,ast2700-intc-ic
> > +
> > +      reg:
> > +        maxItems: 1
> > +
> > +      interrupt-controller: true
> > +
> > +      '#interrupt-cells':
> > +        const: 2
> > +        description: |
> 
> Don't need '|'.
> 
> > +          The first cell is the IRQ number, the second cell is the trigger
> > +          type as defined in interrupt.txt in this directory.
> 
> Don't add links to legacy documents.
> 
> > +
> > +      interrupts:
> > +        minItems: 1
> > +        maxItems: 6
> > +        description: |
> > +          The interrupts provided by this interrupt controller.
> > +
> > +      interrupts-extended:
> 
> Why do you have both interrupts and interrupts-extended? They are mutually
> exclusive and both are auto-magically supported. The schemas only have to
> document 'interrupts'.
> 
> > +        minItems: 1
> > +        maxItems: 6
> > +        description: |
> > +          This property is required when defining a cascaded interrupt
> controller
> > +          that is connected under another interrupt controller. It specifies
> the
> > +          parent interrupt(s) in the upstream controller to which this
> controller
> > +          is connected.
> > +
> > +    oneOf:
> > +      - required: [interrupts]
> > +      - required: [interrupts-extended]
> > +
> > +    required:
> > +      - compatible
> > +      - reg
> > +      - interrupt-controller
> > +      - '#interrupt-cells'
> >
> >  required:
> >    - compatible
> >    - reg
> > -  - interrupt-controller
> > -  - '#interrupt-cells'
> > -  - interrupts
> >
> >  additionalProperties: false
> >
> > @@ -68,19 +116,43 @@ examples:
> >      #include <dt-bindings/interrupt-controller/arm-gic.h>
> >
> >      bus {
> > +      #address-cells = <2>;
> > +      #size-cells = <2>;
> > +
> > +      intc0: interrupt-controller@12100000 {
> 
> This node isn't an interrupt-controller.
> 
> > +        compatible = "aspeed,ast2700-intc0";
> > +        reg = <0 0x12100000 0 0x4000>;
> > +        ranges = <0x0 0x0 0x0 0x12100000 0x0 0x4000>;
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        intc0_11: interrupt-controller@1b00 {
> > +          compatible = "aspeed,ast2700-intc-ic";
> > +          reg = <0 0x12101b00 0 0x10>;
> > +          #interrupt-cells = <2>;
> > +          interrupt-controller;
> > +          interrupts = <GIC_SPI 192 (GIC_CPU_MASK_SIMPLE(4) |
> IRQ_TYPE_LEVEL_HIGH)>,
> > +                       <GIC_SPI 193 (GIC_CPU_MASK_SIMPLE(4) |
> IRQ_TYPE_LEVEL_HIGH)>,
> > +                       <GIC_SPI 194 (GIC_CPU_MASK_SIMPLE(4) |
> IRQ_TYPE_LEVEL_HIGH)>,
> > +                       <GIC_SPI 195 (GIC_CPU_MASK_SIMPLE(4) |
> IRQ_TYPE_LEVEL_HIGH)>,
> > +                       <GIC_SPI 196 (GIC_CPU_MASK_SIMPLE(4) |
> IRQ_TYPE_LEVEL_HIGH)>,
> > +                       <GIC_SPI 197 (GIC_CPU_MASK_SIMPLE(4) |
> IRQ_TYPE_LEVEL_HIGH)>;
> > +        };
> > +      };
> > +
> > +      intc1: interrupt-controller@14c18000 {
> > +        compatible = "aspeed,ast2700-intc1";
> > +        reg = <0 0x14c18000 0 0x400>;
> > +        ranges = <0x0 0x0 0x0 0x14c18000 0x0 0x400>;
> >          #address-cells = <2>;
> >          #size-cells = <2>;
> >
> > -        interrupt-controller@12101b00 {
> > -            compatible = "aspeed,ast2700-intc-ic";
> > -            reg = <0 0x12101b00 0 0x10>;
> > -            #interrupt-cells = <2>;
> > -            interrupt-controller;
> > -            interrupts = <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>,
> > -                         <GIC_SPI 193 IRQ_TYPE_LEVEL_HIGH>,
> > -                         <GIC_SPI 194 IRQ_TYPE_LEVEL_HIGH>,
> > -                         <GIC_SPI 195 IRQ_TYPE_LEVEL_HIGH>,
> > -                         <GIC_SPI 196 IRQ_TYPE_LEVEL_HIGH>,
> > -                         <GIC_SPI 197 IRQ_TYPE_LEVEL_HIGH>;
> > +        intc1_0: interrupt-controller@100 {
> > +          compatible = "aspeed,ast2700-intc-ic";
> > +          reg = <0x0 0x100 0x0 0x10>;
> > +          #interrupt-cells = <2>;
> > +          interrupt-controller;
> > +          interrupts-extended = <&intc0_11 0
> (GIC_CPU_MASK_SIMPLE(4)
> > + | IRQ_TYPE_LEVEL_HIGH)>;
> >          };
> > +      };
> >      };
> > --
> > 2.34.1
> >

Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation

[Krzysztof Kozlowski]

On 22/07/2025 11:51, Ryan Chen wrote:
> The AST2700 SoC contains two independent top-level interrupt controllers
> (INTC0 and INTC1), each responsible for handling different peripheral
> groups and occupying separate register spaces. Above them, PSP(CA35) GIC
> controller acts as the root interrupt aggregator. Accurately describing
> this hierarchical hardware structure in the device tree requires distinct
> compatible strings for the parent nodes of INTC0 and INTC1.
> 
> - Adds 'aspeed,ast2700-intc0' and 'aspeed,ast2700-intc1' compatible
>  strings for parent interrupt controller nodes. (in addition to the
>  existing 'aspeed,ast2700-intc-ic' for child nodes)

I don't understand how this solves your problem at all. Look at old
diagram - is it correct? If not, what makes you think that new diagram
is correct?

What is the meaning of existing binding and existing intc-ic compatible?


> - Clarifies the relationship and function of INTC0 parent
>  (intc0_0~x: child), INTC1 parent (intc1_0~x: child), and the GIC
>  in the documentation.
> - Updates block diagrams and device tree examples to illustrate
>  the hierarchy and compatible usage.
> - Refines documentation and example formatting.
> 
> This change allows the device tree and driver to distinguish between
> parent (top-level) and child (group) interrupt controller nodes,
> enabling more precise driver matching SOC register space allocation.

And how it was not possible before? That's poor argument especially that
DT does not have to ever distinguish that.


Best regards,
Krzysztof

RE: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation

[Ryan Chen]

> Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add
> parent node compatibles and refine documentation
> 
> On 22/07/2025 11:51, Ryan Chen wrote:
> > The AST2700 SoC contains two independent top-level interrupt
> > controllers
> > (INTC0 and INTC1), each responsible for handling different peripheral
> > groups and occupying separate register spaces. Above them, PSP(CA35)
> > GIC controller acts as the root interrupt aggregator. Accurately
> > describing this hierarchical hardware structure in the device tree
> > requires distinct compatible strings for the parent nodes of INTC0 and INTC1.
> >
> > - Adds 'aspeed,ast2700-intc0' and 'aspeed,ast2700-intc1' compatible
> > strings for parent interrupt controller nodes. (in addition to the
> > existing 'aspeed,ast2700-intc-ic' for child nodes)
> 
> I don't understand how this solves your problem at all. Look at old diagram - is
> it correct? If not, what makes you think that new diagram is correct?
> 
> What is the meaning of existing binding and existing intc-ic compatible?
> 
The new parent nodes (aspeed,ast2700-intc0/intc1) make the device tree layout match the
actual hardware separation shown in the SoC datasheet.
This allows us to register the full resource region, allocate platform resources properly,
and cleanly extend/debug in the future.

The previous "aspeed,ast2700-intc-ic" compatible only describes the interrupt controller instance,
not the full register block. In practice, with only a single child node, there is no way to:
map and manage the entire address space for each INTC block (0x12100000 and 0x14c18000),
or cleanly expose debug features that must access routing/protection registers outside the intc-ic range.

The old diagram was incomplete, since it implied that the interrupt controller block had only the intc-ic instance,
but in hardware each INTC region contains multiple functions and register ranges.

This binding change is mainly for clarity and correctness, aligning DT and driver with the real SoC register map
and future-proofing for debug/maintenance.
> 
> > - Clarifies the relationship and function of INTC0 parent
> >  (intc0_0~x: child), INTC1 parent (intc1_0~x: child), and the GIC  in
> > the documentation.
> > - Updates block diagrams and device tree examples to illustrate  the
> > hierarchy and compatible usage.
> > - Refines documentation and example formatting.
> >
> > This change allows the device tree and driver to distinguish between
> > parent (top-level) and child (group) interrupt controller nodes,
> > enabling more precise driver matching SOC register space allocation.
> 
> And how it was not possible before? That's poor argument especially that DT
> does not have to ever distinguish that.
> 
> 
> Best regards,
> Krzysztof

RE: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation

[Ryan Chen]

> Subject: RE: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent
> node compatibles and refine documentation
> 
> > Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed:
> > Add parent node compatibles and refine documentation
> >
> > On 22/07/2025 11:51, Ryan Chen wrote:
> > > The AST2700 SoC contains two independent top-level interrupt
> > > controllers
> > > (INTC0 and INTC1), each responsible for handling different
> > > peripheral groups and occupying separate register spaces. Above
> > > them, PSP(CA35) GIC controller acts as the root interrupt
> > > aggregator. Accurately describing this hierarchical hardware
> > > structure in the device tree requires distinct compatible strings for the parent
> nodes of INTC0 and INTC1.
> > >
> > > - Adds 'aspeed,ast2700-intc0' and 'aspeed,ast2700-intc1' compatible
> > > strings for parent interrupt controller nodes. (in addition to the
> > > existing 'aspeed,ast2700-intc-ic' for child nodes)
> >
> > I don't understand how this solves your problem at all. Look at old
> > diagram - is it correct? If not, what makes you think that new diagram is
> correct?
> >
> > What is the meaning of existing binding and existing intc-ic compatible?
> >
> The new parent nodes (aspeed,ast2700-intc0/intc1) make the device tree layout
> match the actual hardware separation shown in the SoC datasheet.
> This allows us to register the full resource region, allocate platform resources
> properly, and cleanly extend/debug in the future.
> 
> The previous "aspeed,ast2700-intc-ic" compatible only describes the interrupt
> controller instance, not the full register block. In practice, with only a single child
> node, there is no way to:
> map and manage the entire address space for each INTC block (0x12100000 and
> 0x14c18000), or cleanly expose debug features that must access
> routing/protection registers outside the intc-ic range.
> 
> The old diagram was incomplete, since it implied that the interrupt controller
> block had only the intc-ic instance, but in hardware each INTC region contains
> multiple functions and register ranges.
> 
> This binding change is mainly for clarity and correctness, aligning DT and driver
> with the real SoC register map and future-proofing for debug/maintenance.
> >
> > > - Clarifies the relationship and function of INTC0 parent
> > >  (intc0_0~x: child), INTC1 parent (intc1_0~x: child), and the GIC
> > > in the documentation.
> > > - Updates block diagrams and device tree examples to illustrate  the
> > > hierarchy and compatible usage.
> > > - Refines documentation and example formatting.
> > >
> > > This change allows the device tree and driver to distinguish between
> > > parent (top-level) and child (group) interrupt controller nodes,
> > > enabling more precise driver matching SOC register space allocation.
> >
> > And how it was not possible before? That's poor argument especially
> > that DT does not have to ever distinguish that.
> >

Hi Krzysztof,

I wanted to follow up on my previous explanation about separating parent and child nodes for AST2700 INTC in the device tree.
There is other SoCs, such as Marvell’s CP110 ICU, also use a similar approach to separate parent controller and functional child nodes in the device tree, as shown here:
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/interrupt-controller/marvell%2Ccp110-icu.yaml#L74-L98
Do you need me to provide further details or additional about our SOC design information?
Or is there anything specific you’d like clarified regarding the motivation or the binding structure?

Thanks for your feedback and guidance.

Best regards,
Ryan
> >
> > Best regards,
> > Krzysztof

Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation

[Krzysztof Kozlowski]

On 25/07/2025 09:18, Ryan Chen wrote:
> 
>> Subject: RE: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent
>> node compatibles and refine documentation
>>
>>> Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed:
>>> Add parent node compatibles and refine documentation
>>>
>>> On 22/07/2025 11:51, Ryan Chen wrote:
>>>> The AST2700 SoC contains two independent top-level interrupt
>>>> controllers
>>>> (INTC0 and INTC1), each responsible for handling different
>>>> peripheral groups and occupying separate register spaces. Above
>>>> them, PSP(CA35) GIC controller acts as the root interrupt
>>>> aggregator. Accurately describing this hierarchical hardware
>>>> structure in the device tree requires distinct compatible strings for the parent
>> nodes of INTC0 and INTC1.
>>>>
>>>> - Adds 'aspeed,ast2700-intc0' and 'aspeed,ast2700-intc1' compatible
>>>> strings for parent interrupt controller nodes. (in addition to the
>>>> existing 'aspeed,ast2700-intc-ic' for child nodes)
>>>
>>> I don't understand how this solves your problem at all. Look at old
>>> diagram - is it correct? If not, what makes you think that new diagram is
>> correct?
>>>
>>> What is the meaning of existing binding and existing intc-ic compatible?
>>>
>> The new parent nodes (aspeed,ast2700-intc0/intc1) make the device tree layout
>> match the actual hardware separation shown in the SoC datasheet.
>> This allows us to register the full resource region, allocate platform resources
>> properly, and cleanly extend/debug in the future.
>>
>> The previous "aspeed,ast2700-intc-ic" compatible only describes the interrupt
>> controller instance, not the full register block. In practice, with only a single child
>> node, there is no way to:
>> map and manage the entire address space for each INTC block (0x12100000 and
>> 0x14c18000), or cleanly expose debug features that must access
>> routing/protection registers outside the intc-ic range.
>>
>> The old diagram was incomplete, since it implied that the interrupt controller
>> block had only the intc-ic instance, but in hardware each INTC region contains
>> multiple functions and register ranges.
>>
>> This binding change is mainly for clarity and correctness, aligning DT and driver
>> with the real SoC register map and future-proofing for debug/maintenance.
>>>
>>>> - Clarifies the relationship and function of INTC0 parent
>>>>  (intc0_0~x: child), INTC1 parent (intc1_0~x: child), and the GIC
>>>> in the documentation.
>>>> - Updates block diagrams and device tree examples to illustrate  the
>>>> hierarchy and compatible usage.
>>>> - Refines documentation and example formatting.
>>>>
>>>> This change allows the device tree and driver to distinguish between
>>>> parent (top-level) and child (group) interrupt controller nodes,
>>>> enabling more precise driver matching SOC register space allocation.
>>>
>>> And how it was not possible before? That's poor argument especially
>>> that DT does not have to ever distinguish that.
>>>
> 
> Hi Krzysztof,
> 
> I wanted to follow up on my previous explanation about separating parent and child nodes for AST2700 INTC in the device tree.
> There is other SoCs, such as Marvell’s CP110 ICU, also use a similar approach to separate parent controller and functional child nodes in the device tree, as shown here:
> https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/interrupt-controller/marvell%2Ccp110-icu.yaml#L74-L98
> Do you need me to provide further details or additional about our SOC design information?
> Or is there anything specific you’d like clarified regarding the motivation or the binding structure?

Start properly wrapping your email responses. All of them are
misformatted, all the time!

You got replies from two DT binding maintainers. Work with that.

Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation

[Krzysztof Kozlowski]

On 22/07/2025 11:51, Ryan Chen wrote:
> +  INTC0 is used to assert GIC if interrupt in INTC1 asserted.
> +  INTC1 is used to assert INTC0 if interrupt of modules asserted.
> +  +-----+   +---------+
> +  | GIC |---|  INTC0  |
> +  +-----+   +---------+
> +            +---------+
> +            |         |---module0
> +            | INTC0_0 |---module1
> +            |         |---...
> +            +---------+---module31
> +            |---....  |
> +            +---------+
> +            |         |     +---------+
> +            | INTC0_11| +---| INTC1   |
> +            |         |     +---------+
> +            +---------+     +---------+---module0
> +                            | INTC1_0 |---module1
> +                            |         |---...
> +                            +---------+---module31
> +                            ...
> +                            +---------+---module0
> +                            | INTC1_5 |---module1
> +                            |         |---...
> +                            +---------+---module31

You binding also said intc1 is the parent of intc-ic, so where is here
intc-ic?

This diagram and new binding do not match at all.


Best regards,
Krzysztof

RE: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation

[Ryan Chen]

> Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add
> parent node compatibles and refine documentation
> 
> On 22/07/2025 11:51, Ryan Chen wrote:
> > +  INTC0 is used to assert GIC if interrupt in INTC1 asserted.
> > +  INTC1 is used to assert INTC0 if interrupt of modules asserted.
> > +  +-----+   +---------+
> > +  | GIC |---|  INTC0  |
> > +  +-----+   +---------+
> > +            +---------+
> > +            |         |---module0
> > +            | INTC0_0 |---module1
> > +            |         |---...
> > +            +---------+---module31
> > +            |---....  |
> > +            +---------+
> > +            |         |     +---------+
> > +            | INTC0_11| +---| INTC1   |
> > +            |         |     +---------+
> > +            +---------+     +---------+---module0
> > +                            | INTC1_0 |---module1
> > +                            |         |---...
> > +                            +---------+---module31
> > +                            ...
> > +                            +---------+---module0
> > +                            | INTC1_5 |---module1
> > +                            |         |---...
> > +                            +---------+---module31
> 
> You binding also said intc1 is the parent of intc-ic, so where is here intc-ic?
> 
> This diagram and new binding do not match at all.

The corresponded compatible is following.

  +-----+   +---------+
  | GIC |---|  INTC0  | -> (parent : aspeed,ast2700-intc0)
  +-----+   +---------+
            +---------+
            |        |---module0
            | INTC0_0 |---module1
			(child : aspeed,ast2700-intc-ic)
            |        |---...
            +---------+---module31
            |---....  |
            +---------+
            |         |    					 +---------+
            | INTC0_11 | +----------------------------	| INTC1   |  -> -> (parent : aspeed,ast2700-intc1)
			(child : aspeed,ast2700-intc-ic)
            |         |     					+---------+
            +---------+     					+--------  -+---module0
                            				| INTC1_0 | --> (child : aspeed,ast2700-intc-ic)
                            				|        |---...
                            				+---------  +---module31
                            				...
                            				+---------  +---module0
                            				| INTC1_5 | --> ((child : aspeed,ast2700-intc-ic))
                            				|        |---...
                            				+---------  +---module31

> 
> 
> Best regards,
> Krzysztof

Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation

[Krzysztof Kozlowski]

On 23/07/2025 10:08, Ryan Chen wrote:
>> Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add
>> parent node compatibles and refine documentation
>>
>> On 22/07/2025 11:51, Ryan Chen wrote:
>>> +  INTC0 is used to assert GIC if interrupt in INTC1 asserted.
>>> +  INTC1 is used to assert INTC0 if interrupt of modules asserted.
>>> +  +-----+   +---------+
>>> +  | GIC |---|  INTC0  |
>>> +  +-----+   +---------+
>>> +            +---------+
>>> +            |         |---module0
>>> +            | INTC0_0 |---module1
>>> +            |         |---...
>>> +            +---------+---module31
>>> +            |---....  |
>>> +            +---------+
>>> +            |         |     +---------+
>>> +            | INTC0_11| +---| INTC1   |
>>> +            |         |     +---------+
>>> +            +---------+     +---------+---module0
>>> +                            | INTC1_0 |---module1
>>> +                            |         |---...
>>> +                            +---------+---module31
>>> +                            ...
>>> +                            +---------+---module0
>>> +                            | INTC1_5 |---module1
>>> +                            |         |---...
>>> +                            +---------+---module31
>>
>> You binding also said intc1 is the parent of intc-ic, so where is here intc-ic?
>>
>> This diagram and new binding do not match at all.
> 
> The corresponded compatible is following.
> 
>   +-----+   +---------+
>   | GIC |---|  INTC0  | -> (parent : aspeed,ast2700-intc0)
>   +-----+   +---------+
>             +---------+
>             |        |---module0
>             | INTC0_0 |---module1
> 			(child : aspeed,ast2700-intc-ic)
>             |        |---...
>             +---------+---module31
>             |---....  |
>             +---------+
>             |         |    					 +---------+
>             | INTC0_11 | +----------------------------	| INTC1   |  -> -> (parent : aspeed,ast2700-intc1)

AGAIN (second time): that's not what your binding said.

Your binding is explicit here, which is what we want in general. It says
that inct1 is one of the parents of intc-ic.

Let me be clear, because you will be dragging this talk with irrelevant
arguments forever - changing this binding is close to no. If you come
with correct arguments, maybe would work. But the main point is that you
probably do not have to even change the binding to achieve proper
hardware description. Work on that.

Best regards,
Krzysztof

RE: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation

[Ryan Chen]

> Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent
> node compatibles and refine documentation
> 
> On 23/07/2025 10:08, Ryan Chen wrote:
> >> Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller:
> >> aspeed: Add parent node compatibles and refine documentation
> >>
> >> On 22/07/2025 11:51, Ryan Chen wrote:
> >>> +  INTC0 is used to assert GIC if interrupt in INTC1 asserted.
> >>> +  INTC1 is used to assert INTC0 if interrupt of modules asserted.
> >>> +  +-----+   +---------+
> >>> +  | GIC |---|  INTC0  |
> >>> +  +-----+   +---------+
> >>> +            +---------+
> >>> +            |         |---module0
> >>> +            | INTC0_0 |---module1
> >>> +            |         |---...
> >>> +            +---------+---module31
> >>> +            |---....  |
> >>> +            +---------+
> >>> +            |         |     +---------+
> >>> +            | INTC0_11| +---| INTC1   |
> >>> +            |         |     +---------+
> >>> +            +---------+     +---------+---module0
> >>> +                            | INTC1_0 |---module1
> >>> +                            |         |---...
> >>> +                            +---------+---module31
> >>> +                            ...
> >>> +                            +---------+---module0
> >>> +                            | INTC1_5 |---module1
> >>> +                            |         |---...
> >>> +                            +---------+---module31
> >>
> >> You binding also said intc1 is the parent of intc-ic, so where is here intc-ic?
> >>
> >> This diagram and new binding do not match at all.
> >
> > The corresponded compatible is following.
> >
> >   +-----+   +---------+
> >   | GIC |---|  INTC0  | -> (parent : aspeed,ast2700-intc0)
> >   +-----+   +---------+
> >             +---------+
> >             |        |---module0
> >             | INTC0_0 |---module1
> > 			(child : aspeed,ast2700-intc-ic)
> >             |        |---...
> >             +---------+---module31
> >             |---....  |
> >             +---------+
> >             |         |    					 +---------+
> >             | INTC0_11 | +----------------------------	| INTC1   |  -> ->
> (parent : aspeed,ast2700-intc1)
> 
> AGAIN (second time): that's not what your binding said.
> 
> Your binding is explicit here, which is what we want in general. It says that inct1 is
> one of the parents of intc-ic.
> 
> Let me be clear, because you will be dragging this talk with irrelevant arguments
> forever - changing this binding is close to no. If you come with correct arguments,
> maybe would work. But the main point is that you probably do not have to even
> change the binding to achieve proper hardware description. Work on that.
> 

If I do not change the binding, I think the yaml and dts can still fit the interrupt
nesting architecture by using both interrupts and interrupts-extended.

For first-level controllers, use the standard interrupts property
(e.g. with the GIC as the parent).

For second-level INTC-IC instances, use interrupts-extended to refer to the
first-level INTC-IC, following common Linux practice for stacked interrupt controllers.
For example:
dts
// First level
intc0_11: interrupt-controller@12101b00 {
    compatible = "aspeed,ast2700-intc-ic";
    reg = <...>;
    interrupt-controller;
    #interrupt-cells = <2>;
    interrupts = <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>, ...;
};

// Second level, cascaded
intc1_0: interrupt-controller@14c18100 {
    compatible = "aspeed,ast2700-intc-ic";
    reg = <...>;
    interrupt-controller;
    #interrupt-cells = <2>;
    interrupts-extended = <&intc0_11 0 IRQ_TYPE_LEVEL_HIGH>;
};
In yaml, I can use:
oneOf:
  - required: [interrupts]
  - required: [interrupts-extended]
This allows both cases to be valid.

Please let me know if this is the recommended approach,
or if further changes are needed.

> Best regards,
> Krzysztof

Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation

[Krzysztof Kozlowski]

On 27/07/2025 03:47, Ryan Chen wrote:
>> Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent
>> node compatibles and refine documentation
>>
>> On 23/07/2025 10:08, Ryan Chen wrote:
>>>> Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller:
>>>> aspeed: Add parent node compatibles and refine documentation
>>>>
>>>> On 22/07/2025 11:51, Ryan Chen wrote:
>>>>> +  INTC0 is used to assert GIC if interrupt in INTC1 asserted.
>>>>> +  INTC1 is used to assert INTC0 if interrupt of modules asserted.
>>>>> +  +-----+   +---------+
>>>>> +  | GIC |---|  INTC0  |
>>>>> +  +-----+   +---------+
>>>>> +            +---------+
>>>>> +            |         |---module0
>>>>> +            | INTC0_0 |---module1
>>>>> +            |         |---...
>>>>> +            +---------+---module31
>>>>> +            |---....  |
>>>>> +            +---------+
>>>>> +            |         |     +---------+
>>>>> +            | INTC0_11| +---| INTC1   |
>>>>> +            |         |     +---------+
>>>>> +            +---------+     +---------+---module0
>>>>> +                            | INTC1_0 |---module1
>>>>> +                            |         |---...
>>>>> +                            +---------+---module31
>>>>> +                            ...
>>>>> +                            +---------+---module0
>>>>> +                            | INTC1_5 |---module1
>>>>> +                            |         |---...
>>>>> +                            +---------+---module31
>>>>
>>>> You binding also said intc1 is the parent of intc-ic, so where is here intc-ic?
>>>>
>>>> This diagram and new binding do not match at all.
>>>
>>> The corresponded compatible is following.
>>>
>>>   +-----+   +---------+
>>>   | GIC |---|  INTC0  | -> (parent : aspeed,ast2700-intc0)
>>>   +-----+   +---------+
>>>             +---------+
>>>             |        |---module0
>>>             | INTC0_0 |---module1
>>> 			(child : aspeed,ast2700-intc-ic)
>>>             |        |---...
>>>             +---------+---module31
>>>             |---....  |
>>>             +---------+
>>>             |         |    					 +---------+
>>>             | INTC0_11 | +----------------------------	| INTC1   |  -> ->
>> (parent : aspeed,ast2700-intc1)
>>
>> AGAIN (second time): that's not what your binding said.
>>
>> Your binding is explicit here, which is what we want in general. It says that inct1 is
>> one of the parents of intc-ic.

... and you never addressed that. :/

>>
>> Let me be clear, because you will be dragging this talk with irrelevant arguments
>> forever - changing this binding is close to no. If you come with correct arguments,
>> maybe would work. But the main point is that you probably do not have to even
>> change the binding to achieve proper hardware description. Work on that.
>>
> 
> If I do not change the binding, I think the yaml and dts can still fit the interrupt
> nesting architecture by using both interrupts and interrupts-extended.
> 
> For first-level controllers, use the standard interrupts property
> (e.g. with the GIC as the parent).
> 
> For second-level INTC-IC instances, use interrupts-extended to refer to the
> first-level INTC-IC, following common Linux practice for stacked interrupt controllers.
> For example:
> dts
> // First level
> intc0_11: interrupt-controller@12101b00 {
>     compatible = "aspeed,ast2700-intc-ic";
>     reg = <...>;
>     interrupt-controller;
>     #interrupt-cells = <2>;
>     interrupts = <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>, ...;
> };
> 
> // Second level, cascaded
> intc1_0: interrupt-controller@14c18100 {
>     compatible = "aspeed,ast2700-intc-ic";
>     reg = <...>;
>     interrupt-controller;
>     #interrupt-cells = <2>;
>     interrupts-extended = <&intc0_11 0 IRQ_TYPE_LEVEL_HIGH>;

This looks like changing the meaning of the interrupt. What was the
interrupt here before? What interrupt is here now?

> };
> In yaml, I can use:
> oneOf:
>   - required: [interrupts]
>   - required: [interrupts-extended]
> This allows both cases to be valid.


Hm? Since when you need both cases?


Best regards,
Krzysztof

RE: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent node compatibles and refine documentation

[Ryan Chen]

> Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller: aspeed: Add parent
> node compatibles and refine documentation
> 
> On 27/07/2025 03:47, Ryan Chen wrote:
> >> Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller:
> >> aspeed: Add parent node compatibles and refine documentation
> >>
> >> On 23/07/2025 10:08, Ryan Chen wrote:
> >>>> Subject: Re: [PATCH v3 1/2] dt-bindings: interrupt-controller:
> >>>> aspeed: Add parent node compatibles and refine documentation
> >>>>
> >>>> On 22/07/2025 11:51, Ryan Chen wrote:
> >>>>> +  INTC0 is used to assert GIC if interrupt in INTC1 asserted.
> >>>>> +  INTC1 is used to assert INTC0 if interrupt of modules asserted.
> >>>>> +  +-----+   +---------+
> >>>>> +  | GIC |---|  INTC0  |
> >>>>> +  +-----+   +---------+
> >>>>> +            +---------+
> >>>>> +            |         |---module0
> >>>>> +            | INTC0_0 |---module1
> >>>>> +            |         |---...
> >>>>> +            +---------+---module31
> >>>>> +            |---....  |
> >>>>> +            +---------+
> >>>>> +            |         |     +---------+
> >>>>> +            | INTC0_11| +---| INTC1   |
> >>>>> +            |         |     +---------+
> >>>>> +            +---------+     +---------+---module0
> >>>>> +                            | INTC1_0 |---module1
> >>>>> +                            |         |---...
> >>>>> +                            +---------+---module31
> >>>>> +                            ...
> >>>>> +                            +---------+---module0
> >>>>> +                            | INTC1_5 |---module1
> >>>>> +                            |         |---...
> >>>>> +                            +---------+---module31
> >>>>
> >>>> You binding also said intc1 is the parent of intc-ic, so where is here intc-ic?
> >>>>
> >>>> This diagram and new binding do not match at all.
> >>>
> >>> The corresponded compatible is following.
> >>>
> >>>   +-----+   +---------+
> >>>   | GIC |---|  INTC0  | -> (parent : aspeed,ast2700-intc0)
> >>>   +-----+   +---------+
> >>>             +---------+
> >>>             |        |---module0
> >>>             | INTC0_0 |---module1
> >>> 			(child : aspeed,ast2700-intc-ic)
> >>>             |        |---...
> >>>             +---------+---module31
> >>>             |---....  |
> >>>             +---------+
> >>>             |         |    					 +---------+
> >>>             | INTC0_11 | +----------------------------	| INTC1   |  -> ->
> >> (parent : aspeed,ast2700-intc1)
> >>
> >> AGAIN (second time): that's not what your binding said.
> >>
> >> Your binding is explicit here, which is what we want in general. It
> >> says that inct1 is one of the parents of intc-ic.
> 
> ... and you never addressed that. :/

The following is datasheet description. 

AST2700 Interrupt Controller Hierarchy (from datasheet):

INTC0 and INTC1 are AMBA slave devices on the AHB bus,
each with their own register space. 480 interrupt sources: INTn (n=0~479)
INT0~127 can be routed directly to PSP, SSP, or TSP.
INT128~319 are handled by INTC1, which have multiple instances
(INTC1_0, INTC1_1, ...)
INTC1 outputs are routed into INTC0; INTC0 outputs go to the GIC.

This structure means:
INTC0 receives INT0~127 and also all outputs from INTC1.
INTC1 handles a subset of interrupt sources, and its output 
is routed as an input to INTC0.

Block Diagram / Interrupt Chain:
GIC
 |
 v
INTC0 (parent, aspeed,ast2700-intc0)
 |
 +-- INTC0_0 (aspeed,ast2700-intc-ic) --> [module0, module1, ...]
 |
 +-- ...
 |
 +-- INTC0_11 (aspeed,ast2700-intc-ic)
      |
      v
   INTC1 (parent, aspeed,ast2700-intc1)
    |
    +-- INTC1_0 (aspeed,ast2700-intc-ic) --> [moduleA, moduleB, ...]
    |
    +-- ...
    |
    +-- INTC1_5 (aspeed,ast2700-intc-ic) --> [moduleY, moduleZ, ...]

| Device Tree Node | Hardware Block  | Output Routed To    |
| -------          | ---------------      | ----------------------     |
| intc0          | INTC0 @12100000 | GIC               |
| intc1          | INTC1 @14c18000 | INTC0 input (cascaded)|

intc0 uses interrupts to connect to the GIC (top-level parent)
intc1 uses interrupts-extended to connect to an input on INTC0
(second-level, cascaded)

This approach ensures the software and device tree reflect the
actual hardware interrupt paths.
It allows the kernel to correctly map register space and handle interrupt
delivery, and makes future debug/maintenance straightforward.

If there are any details you'd like clarified or if you recommend a 
different device tree structure, please let me know!

> 
> >>
> >> Let me be clear, because you will be dragging this talk with
> >> irrelevant arguments forever - changing this binding is close to no.
> >> If you come with correct arguments, maybe would work. But the main
> >> point is that you probably do not have to even change the binding to achieve
> proper hardware description. Work on that.
> >>
> >
> > If I do not change the binding, I think the yaml and dts can still fit
> > the interrupt nesting architecture by using both interrupts and
> interrupts-extended.
> >
> > For first-level controllers, use the standard interrupts property
> > (e.g. with the GIC as the parent).
> >
> > For second-level INTC-IC instances, use interrupts-extended to refer
> > to the first-level INTC-IC, following common Linux practice for stacked interrupt
> controllers.
> > For example:
> > dts
> > // First level
> > intc0_11: interrupt-controller@12101b00 {
> >     compatible = "aspeed,ast2700-intc-ic";
> >     reg = <...>;
> >     interrupt-controller;
> >     #interrupt-cells = <2>;
> >     interrupts = <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>, ...; };
> >
> > // Second level, cascaded
> > intc1_0: interrupt-controller@14c18100 {
> >     compatible = "aspeed,ast2700-intc-ic";
> >     reg = <...>;
> >     interrupt-controller;
> >     #interrupt-cells = <2>;
> >     interrupts-extended = <&intc0_11 0 IRQ_TYPE_LEVEL_HIGH>;
> 
> This looks like changing the meaning of the interrupt. What was the interrupt
> here before? What interrupt is here now?
> 

The change from interrupts to interrupts-extended does not change the 
source or meaning of the interrupt itself.
For first-level INTC-IC nodes, the parent is the GIC, so we use interrupts.

For second-level (cascaded) INTC-IC nodes, the parent is an input on INTC0, 
so interrupts-extended is required to correctly reflect the hardware chain as
described in the datasheet.
This ensures the DT matches the hardware hierarchy—the actual interrupt source 
and routing path are not changed, only described more precisely.

> > };
> > In yaml, I can use:
> > oneOf:
> >   - required: [interrupts]
> >   - required: [interrupts-extended]
> > This allows both cases to be valid.
> 
> 
> Hm? Since when you need both cases?

The oneOf schema allows the binding to support both scenarios,
matching the hardware and software requirements.

first-level INTC-IC nodes required [interrupts]
second-level (cascaded) required [interrupts-extended]

> 
> 
> Best regards,
> Krzysztof

[PATCH v3 2/2] irqchip: aspeed: add debugfs support and AST2700 INTC0/INTC1 routing/protection display

[Ryan Chen]

AST2700 INTC0/INTC1 nodes ("aspeed,ast2700-intc0/1") not only
include the interrupt controller child node ("aspeed,ast2700-intc-ic"),
but also provide interrupt routing and register protection features.
This patch adds debugfs entries for interrupt routing and protection
status for AST2700 INTC0/INTC1.

- Register platform driver for "aspeed,ast2700-intc0" and
 "aspeed,ast2700-intc1" compatible nodes.
- Add show_routing/show_prot callbacks for both intc0 and intc1,
 displaying current interrupt routing and protection register status.
- Expose routing/protection information via debugfs for debugging
 and validation.

Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
---
 drivers/irqchip/irq-aspeed-intc.c | 238 ++++++++++++++++++++++++++++++
 1 file changed, 238 insertions(+)

diff --git a/drivers/irqchip/irq-aspeed-intc.c b/drivers/irqchip/irq-aspeed-intc.c
index 8330221799a0..8385f3d5f901 100644
--- a/drivers/irqchip/irq-aspeed-intc.c
+++ b/drivers/irqchip/irq-aspeed-intc.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/debugfs.h>
 #include <linux/irq.h>
 #include <linux/irqchip.h>
 #include <linux/irqchip/chained_irq.h>
@@ -15,6 +16,13 @@
 #include <linux/io.h>
 #include <linux/spinlock.h>
 
+#define INTC0_ROUTING0_SEL0	0x200
+#define INTC0_ROUTING0_SEL1	0x300
+#define INTC0_ROUTING0_SEL2	0x400
+#define INTC1_ROUTING0_SEL0	0x80
+#define INTC1_ROUTING0_SEL1	0xa0
+#define INTC1_ROUTING0_SEL2	0xc0
+
 #define INTC_INT_ENABLE_REG	0x00
 #define INTC_INT_STATUS_REG	0x04
 #define INTC_IRQS_PER_WORD	32
@@ -137,3 +145,233 @@ static int __init aspeed_intc_ic_of_init(struct device_node *node,
 }
 
 IRQCHIP_DECLARE(ast2700_intc_ic, "aspeed,ast2700-intc-ic", aspeed_intc_ic_of_init);
+
+struct aspeed_intc {
+	void __iomem *base;
+	struct device *dev;
+	struct dentry *dbg_root;
+	int (*show_routing)(struct seq_file *s, void *unused);
+	int (*show_prot)(struct seq_file *s, void *unused);
+};
+
+/*
+ * 000: Route interrupt INTn to PSP GICINT0-31
+ * 001: Route interrupt INTn to SSPINT0-31
+ * 010: Route interrupt INTn to TSPINT0-31
+ */
+static int aspeed_intc0_show_routing(struct seq_file *s, void *unused)
+{
+	struct aspeed_intc *intc = s->private;
+	int group, bit;
+
+	seq_puts(s, "int | PSP | SSP | TSP\n");
+	seq_puts(s, "----+-----+-----+----\n");
+
+	for (group = 0; group < 4; group++) {
+		u32 reg0 = readl(intc->base + INTC0_ROUTING0_SEL0 + group * 4);
+		u32 reg1 = readl(intc->base + INTC0_ROUTING0_SEL1 + group * 4);
+		u32 reg2 = readl(intc->base + INTC0_ROUTING0_SEL2 + group * 4);
+
+		for (bit = 0; bit < 32; bit++) {
+			int idx = group * 32 + bit;
+			u8 routing = (((reg2 >> bit) & 0x1) << 2) |
+				     (((reg1 >> bit) & 0x1) << 1) |
+				     (((reg0 >> bit) & 0x1) << 0);
+
+			const char *ca35 = (routing == 0) ? " O " : " - ";
+			const char *ssp  = (routing == 1) ? " O " : " - ";
+			const char *tsp  = (routing == 2) ? " O " : " - ";
+
+			seq_printf(s, "%-4d| %s | %s | %s\n", idx, ca35, ssp, tsp);
+		}
+	}
+	return 0;
+}
+
+static int aspeed_intc0_show_prot(struct seq_file *s, void *unused)
+{
+	struct aspeed_intc *intc = s->private;
+	u32 prot = readl(intc->base + 0x40);
+
+	seq_printf(s, "INTC040 : 0x%08x\n", prot);
+
+	static const char * const prot_bits[] = {
+		"hprot_ca35: Protect INTC010~018,1xxx accessed by PSP only",
+		"hprot_ssp: Protect INTC020~028,2xxx accessed by SSP only",
+		"hprot_tsp: Protect INTC030~038,3xxx accessed by TSP only",
+		"hprot_sirqs: Protect INTC0C0~0D4 to be read only",
+		"hprot_sirqs_1700: Protect INTC0D8~0DC to be read only",
+		"hprot_sirqs_ext: Protect INTC0E0 to be read only",
+		"hprot_reg_prot: Protect INTC044,2xx~3xx to be read only",
+		"hprot_rd1_prot: Read protect for INTC044,200-438",
+		"hprot_rd2_prot: Read protect for INTC0C0~164",
+		"hprot_rd3_prot: Read protect for INTC02x,1xxx to be read by PSP only",
+		"hprot_rd4_prot: Read protect for INTC03x,2xxx to be read by SSP only",
+		"hprot_rd5_prot: Read protect for INTC04x,3xxx to be read by TSP only",
+		"hprot_mcu0: Protect INTC050~054,028 accessed by MCU0 only",
+		"hprot_ca35p: Protect INTC010~018 accessed by PSP secure only"
+	};
+
+	for (int i = 0; i < 14; i++)
+		seq_printf(s, "  [%2d] %s: %s\n", i, prot_bits[i],
+			   (prot & BIT(i)) ? "Enable" : "Disable");
+	return 0;
+}
+
+/*
+ * 000: Route interrupt INTi to PSP(default)
+ * 001: Route interrupt INTi to INTC controller
+ * 010: Route interrupt INTi to SSP
+ * 011: Route interrupt INTi to TSP
+ * 100: Route interrupt INTi to PSP S1
+ * 101: Route interrupt INTi to PSP S2
+ * 110: Route interrupt INTi to MCU0
+ */
+static int aspeed_intc1_show_routing(struct seq_file *s, void *unused)
+{
+	struct aspeed_intc *intc = s->private;
+	int group, bit;
+
+	seq_puts(s, "index      | PSP | INTC| SSP | TSP | S1  | S2  | MCU0\n");
+	seq_puts(s, "-----------+-----+-----+-----+-----+-----+-----+-----\n");
+
+	for (group = 0; group < 6; group++) {
+		u32 reg0 = readl(intc->base + INTC1_ROUTING0_SEL0 + group * 4);
+		u32 reg1 = readl(intc->base + INTC1_ROUTING0_SEL1 + group * 4);
+		u32 reg2 = readl(intc->base + INTC1_ROUTING0_SEL2 + group * 4);
+
+		for (bit = 0; bit < 32; bit++) {
+			u8 routing = (((reg2 >> bit) & 0x1) << 2) |
+				     (((reg1 >> bit) & 0x1) << 1) |
+				     (((reg0 >> bit) & 0x1) << 0);
+
+			const char *psp  = (routing == 0) ? " O " : " - ";
+			const char *intc = (routing == 1) ? " O " : " - ";
+			const char *ssp  = (routing == 2) ? " O " : " - ";
+			const char *tsp  = (routing == 3) ? " O " : " - ";
+			const char *s1   = (routing == 4) ? " O " : " - ";
+			const char *s2   = (routing == 5) ? " O " : " - ";
+			const char *mcu0 = (routing == 6) ? " O " : " - ";
+
+			seq_printf(s, "intc1_%d_%02d | %s | %s | %s | %s | %s | %s | %s\n",
+				   group, bit, psp, intc, ssp, tsp, s1, s2, mcu0);
+		}
+	}
+	return 0;
+}
+
+static int aspeed_intc1_show_prot(struct seq_file *s, void *unused)
+{
+	struct aspeed_intc *intc = s->private;
+	u32 prot = readl(intc->base);
+
+	seq_printf(s, "INTC1: 0x%08x\n", prot);
+
+	static const char * const prot_bits[] = {
+		"pprot_ca35: Protect INTC100~150,280~2D0,300~350 write by PSP only",
+		"pprot_ssp: Protect INTC180~1D0 write by SSP only",
+		"pprot_tsp: Protect INTC200~250 write by TSP only",
+		"pprot_reg_prot: Protect INTC080~0D4 to be read only",
+		"pprot_regrd: Protect INTC080~0D4 to be read protected",
+		"pprot_regrd2: Protect INTC100~150,280~2D0,300~350 read by PSP only",
+		"pprot_regrd3: Protect INTC180~1D0 read by SSP only",
+		"pprot_regrd4: Protect INTC200~250 read by TSP only",
+		"pprot_mcu0: Protect INTC010,014 write by MCU0 only",
+		"pprot_regrd5: Protect INTC010,014 read by MCU0 only",
+		"pprot_treg: Protect INTC040~054 to be read protected"
+	};
+
+	for (int i = 0; i < 11; i++)
+		seq_printf(s, "  [%2d] %s: %s\n", i, prot_bits[i],
+			   (prot & BIT(i)) ? "Enable" : "Disable");
+	return 0;
+}
+
+static int aspeed_intc_open_routing(struct inode *inode, struct file *file)
+{
+	struct aspeed_intc *intc = inode->i_private;
+
+	if (!intc->show_routing)
+		return -ENODEV;
+	return single_open(file, intc->show_routing, intc);
+}
+
+static int aspeed_intc_open_prot(struct inode *inode, struct file *file)
+{
+	struct aspeed_intc *intc = inode->i_private;
+
+	if (!intc->show_prot)
+		return -ENODEV;
+	return single_open(file, intc->show_prot, intc);
+}
+
+static const struct file_operations aspeed_intc_routing_fops = {
+	.owner   = THIS_MODULE,
+	.open    = aspeed_intc_open_routing,
+	.read    = seq_read,
+	.llseek  = seq_lseek,
+	.release = single_release,
+};
+
+static const struct file_operations aspeed_intc_prot_fops = {
+	.owner   = THIS_MODULE,
+	.open    = aspeed_intc_open_prot,
+	.read    = seq_read,
+	.llseek  = seq_lseek,
+	.release = single_release,
+};
+
+static int aspeed_intc_probe(struct platform_device *pdev)
+{
+	struct aspeed_intc *intc;
+	struct resource *res;
+
+	intc = devm_kzalloc(&pdev->dev, sizeof(*intc), GFP_KERNEL);
+	if (!intc)
+		return -ENOMEM;
+	intc->dev = &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	intc->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(intc->base))
+		return PTR_ERR(intc->base);
+
+	if (of_device_is_compatible(pdev->dev.of_node, "aspeed,ast2700-intc0")) {
+		intc->show_routing = aspeed_intc0_show_routing;
+		intc->show_prot    = aspeed_intc0_show_prot;
+	} else if (of_device_is_compatible(pdev->dev.of_node, "aspeed,ast2700-intc1")) {
+		intc->show_routing = aspeed_intc1_show_routing;
+		intc->show_prot    = aspeed_intc1_show_prot;
+	} else {
+		intc->show_routing = NULL;
+		intc->show_prot = NULL;
+	}
+
+	platform_set_drvdata(pdev, intc);
+
+	intc->dbg_root = debugfs_create_dir(dev_name(&pdev->dev), NULL);
+	if (intc->dbg_root) {
+		debugfs_create_file("routing", 0400, intc->dbg_root, intc,
+				    &aspeed_intc_routing_fops);
+		debugfs_create_file("protection", 0400, intc->dbg_root, intc,
+				    &aspeed_intc_prot_fops);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id aspeed_intc_of_match[] = {
+	{ .compatible = "aspeed,ast2700-intc0", },
+	{ .compatible = "aspeed,ast2700-intc1", },
+	{},
+};
+
+static struct platform_driver aspeed_intc_driver = {
+	.probe  = aspeed_intc_probe,
+	.driver = {
+		.name = "ast2700-intc",
+		.of_match_table = aspeed_intc_of_match,
+	},
+};
+builtin_platform_driver(aspeed_intc_driver);
+
-- 
2.34.1

Re: [PATCH v3 2/2] irqchip: aspeed: add debugfs support and AST2700 INTC0/INTC1 routing/protection display

[Thomas Gleixner]

On Tue, Jul 22 2025 at 17:51, Ryan Chen wrote:

The subsystem prefix is made up. See:

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch-submission-notes

> AST2700 INTC0/INTC1 nodes ("aspeed,ast2700-intc0/1") not only
> include the interrupt controller child node ("aspeed,ast2700-intc-ic"),
> but also provide interrupt routing and register protection features.

> This patch adds debugfs entries for interrupt routing and protection

# git grep 'This patch' Documentation/process

> status for AST2700 INTC0/INTC1.
>
> - Register platform driver for "aspeed,ast2700-intc0" and
>  "aspeed,ast2700-intc1" compatible nodes.
> - Add show_routing/show_prot callbacks for both intc0 and intc1,
>  displaying current interrupt routing and protection register status.
> - Expose routing/protection information via debugfs for debugging
>  and validation.
> +
> +struct aspeed_intc {
> +	void __iomem *base;
> +	struct device *dev;
> +	struct dentry *dbg_root;
> +	int (*show_routing)(struct seq_file *s, void *unused);
> +	int (*show_prot)(struct seq_file *s, void *unused);
> +};

See the chapter about struct declarations and initializers in the
documentation I linked to above.

> +static int aspeed_intc1_show_prot(struct seq_file *s, void *unused)
> +{
> +	struct aspeed_intc *intc = s->private;
> +	u32 prot = readl(intc->base);
> +
> +	seq_printf(s, "INTC1: 0x%08x\n", prot);
> +
> +	static const char * const prot_bits[] = {
> +		"pprot_ca35: Protect INTC100~150,280~2D0,300~350 write by PSP only",
> +		"pprot_ssp: Protect INTC180~1D0 write by SSP only",
> +		"pprot_tsp: Protect INTC200~250 write by TSP only",
> +		"pprot_reg_prot: Protect INTC080~0D4 to be read only",
> +		"pprot_regrd: Protect INTC080~0D4 to be read protected",
> +		"pprot_regrd2: Protect INTC100~150,280~2D0,300~350 read by PSP only",
> +		"pprot_regrd3: Protect INTC180~1D0 read by SSP only",
> +		"pprot_regrd4: Protect INTC200~250 read by TSP only",
> +		"pprot_mcu0: Protect INTC010,014 write by MCU0 only",
> +		"pprot_regrd5: Protect INTC010,014 read by MCU0 only",
> +		"pprot_treg: Protect INTC040~054 to be read protected"
> +	};
> +
> +	for (int i = 0; i < 11; i++)
> +		seq_printf(s, "  [%2d] %s: %s\n", i, prot_bits[i],
> +			   (prot & BIT(i)) ? "Enable" : "Disable");
> +	return 0;
> +}

I really have to ask, what the value of this metric ton of string
constants and decoding is. This is a debug interface, which is intended
for developers and experts. As these are hardware bits, which are
immutable, it's completely sufficient to print out the raw values here
and let the engineer decode it, no?

> +static int aspeed_intc_probe(struct platform_device *pdev)
> +{
> +	struct aspeed_intc *intc;
> +	struct resource *res;
> +
> +	intc = devm_kzalloc(&pdev->dev, sizeof(*intc), GFP_KERNEL);
> +	if (!intc)
> +		return -ENOMEM;
> +	intc->dev = &pdev->dev;

intc->dev is not used anywhere.

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	intc->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(intc->base))
> +		return PTR_ERR(intc->base);
> +
> +	if (of_device_is_compatible(pdev->dev.of_node, "aspeed,ast2700-intc0")) {
> +		intc->show_routing = aspeed_intc0_show_routing;
> +		intc->show_prot    = aspeed_intc0_show_prot;
> +	} else if (of_device_is_compatible(pdev->dev.of_node, "aspeed,ast2700-intc1")) {
> +		intc->show_routing = aspeed_intc1_show_routing;
> +		intc->show_prot    = aspeed_intc1_show_prot;
> +	} else {
> +		intc->show_routing = NULL;
> +		intc->show_prot = NULL;

What's the point of creating the debugfs entry instead of bailing out?

> +	}
> +
> +	platform_set_drvdata(pdev, intc);
> +
> +	intc->dbg_root = debugfs_create_dir(dev_name(&pdev->dev), NULL);

Why storing this? It's just used for setting up the debugfs entry, no?

> +	if (intc->dbg_root) {
> +		debugfs_create_file("routing", 0400, intc->dbg_root, intc,
> +				    &aspeed_intc_routing_fops);
> +		debugfs_create_file("protection", 0400, intc->dbg_root, intc,
> +				    &aspeed_intc_prot_fops);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id aspeed_intc_of_match[] = {
> +	{ .compatible = "aspeed,ast2700-intc0", },
> +	{ .compatible = "aspeed,ast2700-intc1", },
> +	{},
> +};
> +
> +static struct platform_driver aspeed_intc_driver = {
> +	.probe  = aspeed_intc_probe,
> +	.driver = {
> +		.name = "ast2700-intc",
> +		.of_match_table = aspeed_intc_of_match,
> +	},
> +};
> +builtin_platform_driver(aspeed_intc_driver);

Why has this to be builtin and not a module? It has zero dependencies on
the existing code in this file, right?

Just stick it into a seperate source file and make it modular with a
seperate Kconfig switch. No point in carrying this code as built-in in
multi-platform builds.

This whole platform driver muck is just there to expose the routing and
protection registers in debugfs even if debugfs is disabled. Seriously?

It's completely disconnected from the interrupt delivery chain as far as
the kernel is concerned, i.e. it does not provide a interrupt
domain/chip. So that interface dumps just some register values with a
lot of effort and leaves it to the user to decode which actual linux
interrupt in the real intc-ic interrupt domains is affected, right?

I'm still failing to see the value of all of this. As the kernel does
not even modify these registers, you are basically implementing a dump
facility for decoding what the firmware put into those registers, right?

I don't have a strong opinion about it, but if it has a value, then all
of this can be done with way smaller code by just dumping the raw
register values all in one go. No need for two files and string
encoding. That's what user space is for.

Something like the completely uncompiled below, which I cobbled together
quickly for illustration. You get the idea.

Thanks,

        tglx
---

#define INTC_TYPE_C0	0
#define INTC_TYPE_C1	1

struct aspeed_intc {
	void __iomem	*base;
	unsigned int	type;
};

const struct aspeed_intc_data {
	char		*name;
	unsigned int	groups;
	unsigned int	prot_offs;
	unsigned int	rout_offs;
	unsigned int	rout_gap;
} aspeed_intc_data[2] = {
	{
		.name		= "INTC0",
		.groups		= 4,
		.prot_offs	= 0x40,
		.rout_offs	= 0x200,
		.rout_gap	= 0x100,
	},
	{
		.name		= "INTC1",
		.groups		= 6,
		.prot_offs	= 0x0,
		.rout_offs	= 0x80,
		.rout_gap	= 0x20,
	},
};

static int aspeed_intc_show(struct seq_file *m, void *unused)
{
	struct aspeed_intc *intc = m->private;
	const struct aspeed_intc_data *d = &aspeed_intc_data[intc->type];
	void __iomem *base = intc->base;

	seq_printf(m, "%s\n", d->name)
	seq_printf(m, "P: 0x%08x\n", readl(base + d->prot_offs));

	base += d->rout_offs;
	for (unsigned int i = 0; i < d->groups; i++, base += 4) {
		seq_printf(m, "R%d: 0x%08x 0x%08x 0x%08x\n", i, readl(base),
			   readl(base + d->rout_gap), readl(base + 2 * d->rout_gap));
	}
	return 0;
}
DEFINE_SHOW_ATTRIBUTE(aspeed_intc);

static int aspeed_intc_probe(struct platform_device *pdev)
{
	struct aspeed_intc *intc;
	struct resource *res;
	struct dentry *dir;

	intc = devm_kzalloc(&pdev->dev, sizeof(*intc), GFP_KERNEL);
	if (!intc)
		return -ENOMEM;

	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
	intc->base = devm_ioremap_resource(&pdev->dev, res);
	if (IS_ERR(intc->base))
		return PTR_ERR(intc->base);

	if (of_device_is_compatible(pdev->dev.of_node, "aspeed,ast2700-intc0"))
		intc->type = INTC_TYPE_C0;
	else if (of_device_is_compatible(pdev->dev.of_node, "aspeed,ast2700-intc1"))
		intc->type = INTC_TYPE_C1;
	else
		return -ENOTSUPP;

	platform_set_drvdata(pdev, intc);

	dir = debugfs_create_dir(dev_name(&pdev->dev), NULL);
	debugfs_create_file("intc_regs", 0400, dir, intc, &aspeed_intc_fops);
	return 0;
}

static const struct of_device_id aspeed_intc_of_match[] = {
	{ .compatible	= "aspeed,ast2700-intc0", },
	{ .compatible	= "aspeed,ast2700-intc1", },
	{ },
};
MODULE_DEVICE_TABLE(of, aspeed_intc_of_match);

static struct platform_driver aspeed_intc_driver = {
	.probe  = aspeed_intc_probe,
	.driver = {
		.name		= "ast2700-intc",
		.of_match_table	= aspeed_intc_of_match,
	},
};
module_platform_driver(aspeed_intc_driver);

RE: [PATCH v3 2/2] irqchip: aspeed: add debugfs support and AST2700 INTC0/INTC1 routing/protection display

[Ryan Chen]

> Subject: Re: [PATCH v3 2/2] irqchip: aspeed: add debugfs support and AST2700
> INTC0/INTC1 routing/protection display
> 
> On Tue, Jul 22 2025 at 17:51, Ryan Chen wrote:
> 
> The subsystem prefix is made up. See:
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch-su
> bmission-notes

Will prefix update to
Irqchip/aspeed-intc: add debugfs support and AST2700 INTC0/INTC1 routing/protection display

> 
> > AST2700 INTC0/INTC1 nodes ("aspeed,ast2700-intc0/1") not only include
> > the interrupt controller child node ("aspeed,ast2700-intc-ic"), but
> > also provide interrupt routing and register protection features.
> 
> > This patch adds debugfs entries for interrupt routing and protection
> 
> # git grep 'This patch' Documentation/process

Modify to
Adds debugfs entries for interrupt routing and protection status for AST2700 INTC0/INTC1.
> 
> > status for AST2700 INTC0/INTC1.
> >
> > - Register platform driver for "aspeed,ast2700-intc0" and
> > "aspeed,ast2700-intc1" compatible nodes.
> > - Add show_routing/show_prot callbacks for both intc0 and intc1,
> > displaying current interrupt routing and protection register status.
> > - Expose routing/protection information via debugfs for debugging  and
> > validation.
> > +
> > +struct aspeed_intc {
> > +	void __iomem *base;
> > +	struct device *dev;
> > +	struct dentry *dbg_root;
> > +	int (*show_routing)(struct seq_file *s, void *unused);
> > +	int (*show_prot)(struct seq_file *s, void *unused); };
> 
> See the chapter about struct declarations and initializers in the documentation
> I linked to above.

Sorry, I don't see the struct "> > +	int (*show_prot)(struct seq_file *s, void *unused); };"
My original submit is following, it should ok. Am I right?

+struct aspeed_intc {
+	void __iomem *base;
+	struct device *dev;
+	struct dentry *dbg_root;
+	int (*show_routing)(struct seq_file *s, void *unused);
+	int (*show_prot)(struct seq_file *s, void *unused);
+};
https://www.spinics.net/lists/kernel/msg5776957.html
> 
> > +static int aspeed_intc1_show_prot(struct seq_file *s, void *unused) {
> > +	struct aspeed_intc *intc = s->private;
> > +	u32 prot = readl(intc->base);
> > +
> > +	seq_printf(s, "INTC1: 0x%08x\n", prot);
> > +
> > +	static const char * const prot_bits[] = {
> > +		"pprot_ca35: Protect INTC100~150,280~2D0,300~350 write by PSP
> only",
> > +		"pprot_ssp: Protect INTC180~1D0 write by SSP only",
> > +		"pprot_tsp: Protect INTC200~250 write by TSP only",
> > +		"pprot_reg_prot: Protect INTC080~0D4 to be read only",
> > +		"pprot_regrd: Protect INTC080~0D4 to be read protected",
> > +		"pprot_regrd2: Protect INTC100~150,280~2D0,300~350 read by PSP
> only",
> > +		"pprot_regrd3: Protect INTC180~1D0 read by SSP only",
> > +		"pprot_regrd4: Protect INTC200~250 read by TSP only",
> > +		"pprot_mcu0: Protect INTC010,014 write by MCU0 only",
> > +		"pprot_regrd5: Protect INTC010,014 read by MCU0 only",
> > +		"pprot_treg: Protect INTC040~054 to be read protected"
> > +	};
> > +
> > +	for (int i = 0; i < 11; i++)
> > +		seq_printf(s, "  [%2d] %s: %s\n", i, prot_bits[i],
> > +			   (prot & BIT(i)) ? "Enable" : "Disable");
> > +	return 0;
> > +}
> 
> I really have to ask, what the value of this metric ton of string constants and
> decoding is. This is a debug interface, which is intended for developers and
> experts. As these are hardware bits, which are immutable, it's completely
> sufficient to print out the raw values here and let the engineer decode it, no?

The reason for the string decoding was to make debug output more friendly for quick diagnosis during bring-up,
If it is not accepted, I will revise the patch to remove the extra string decoding and only output the raw values.

> 
> > +static int aspeed_intc_probe(struct platform_device *pdev) {
> > +	struct aspeed_intc *intc;
> > +	struct resource *res;
> > +
> > +	intc = devm_kzalloc(&pdev->dev, sizeof(*intc), GFP_KERNEL);
> > +	if (!intc)
> > +		return -ENOMEM;
> > +	intc->dev = &pdev->dev;
> 
> intc->dev is not used anywhere.

Will remove it. 

> 
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	intc->base = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(intc->base))
> > +		return PTR_ERR(intc->base);
> > +
> > +	if (of_device_is_compatible(pdev->dev.of_node, "aspeed,ast2700-intc0"))
> {
> > +		intc->show_routing = aspeed_intc0_show_routing;
> > +		intc->show_prot    = aspeed_intc0_show_prot;
> > +	} else if (of_device_is_compatible(pdev->dev.of_node,
> "aspeed,ast2700-intc1")) {
> > +		intc->show_routing = aspeed_intc1_show_routing;
> > +		intc->show_prot    = aspeed_intc1_show_prot;
> > +	} else {
> > +		intc->show_routing = NULL;
> > +		intc->show_prot = NULL;
> 
> What's the point of creating the debugfs entry instead of bailing out?

Yes, will update to
return -ENODEV;
> 
> > +	}
> > +
> > +	platform_set_drvdata(pdev, intc);
> > +
> > +	intc->dbg_root = debugfs_create_dir(dev_name(&pdev->dev), NULL);
> 
> Why storing this? It's just used for setting up the debugfs entry, no?

I'll update the code to use a local variable, by following.

struct dentry *dbg_root;
dbg_root = debugfs_create_dir(dev_name(&pdev->dev), NULL);
if (dbg_root) {
    debugfs_create_file("routing", 0400, dbg_root, intc, &aspeed_intc_routing_fops);
    debugfs_create_file("protection", 0400, dbg_root, intc, &aspeed_intc_prot_fops);
}
> 
> > +	if (intc->dbg_root) {
> > +		debugfs_create_file("routing", 0400, intc->dbg_root, intc,
> > +				    &aspeed_intc_routing_fops);
> > +		debugfs_create_file("protection", 0400, intc->dbg_root, intc,
> > +				    &aspeed_intc_prot_fops);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id aspeed_intc_of_match[] = {
> > +	{ .compatible = "aspeed,ast2700-intc0", },
> > +	{ .compatible = "aspeed,ast2700-intc1", },
> > +	{},
> > +};
> > +
> > +static struct platform_driver aspeed_intc_driver = {
> > +	.probe  = aspeed_intc_probe,
> > +	.driver = {
> > +		.name = "ast2700-intc",
> > +		.of_match_table = aspeed_intc_of_match,
> > +	},
> > +};
> > +builtin_platform_driver(aspeed_intc_driver);
> 
> Why has this to be builtin and not a module? It has zero dependencies on the
> existing code in this file, right?
> Just stick it into a seperate source file and make it modular with a seperate
> Kconfig switch. No point in carrying this code as built-in in multi-platform
> builds.
> 
> This whole platform driver muck is just there to expose the routing and
> protection registers in debugfs even if debugfs is disabled. Seriously?
> 
OK. I will do it in separate source c file and make it modular with Kconfig.
And also depends on DEBUG_FS
Thanks for guidance.


> It's completely disconnected from the interrupt delivery chain as far as the
> kernel is concerned, i.e. it does not provide a interrupt domain/chip. So that
> interface dumps just some register values with a lot of effort and leaves it to
> the user to decode which actual linux interrupt in the real intc-ic interrupt
> domains is affected, right?
> 
> I'm still failing to see the value of all of this. As the kernel does not even
> modify these registers, you are basically implementing a dump facility for
> decoding what the firmware put into those registers, right?
> 
> I don't have a strong opinion about it, but if it has a value, then all of this can
> be done with way smaller code by just dumping the raw register values all in
> one go. No need for two files and string encoding. That's what user space is
> for.
> 
Thanks, I will send a new version with a modular driver and a simplified debugfs interface as you suggested.

> Something like the completely uncompiled below, which I cobbled together
> quickly for illustration. You get the idea.
> 
> Thanks,
> 
>         tglx
> ---
> 
> #define INTC_TYPE_C0	0
> #define INTC_TYPE_C1	1
> 
> struct aspeed_intc {
> 	void __iomem	*base;
> 	unsigned int	type;
> };
> 
> const struct aspeed_intc_data {
> 	char		*name;
> 	unsigned int	groups;
> 	unsigned int	prot_offs;
> 	unsigned int	rout_offs;
> 	unsigned int	rout_gap;
> } aspeed_intc_data[2] = {
> 	{
> 		.name		= "INTC0",
> 		.groups		= 4,
> 		.prot_offs	= 0x40,
> 		.rout_offs	= 0x200,
> 		.rout_gap	= 0x100,
> 	},
> 	{
> 		.name		= "INTC1",
> 		.groups		= 6,
> 		.prot_offs	= 0x0,
> 		.rout_offs	= 0x80,
> 		.rout_gap	= 0x20,
> 	},
> };
> 
> static int aspeed_intc_show(struct seq_file *m, void *unused) {
> 	struct aspeed_intc *intc = m->private;
> 	const struct aspeed_intc_data *d = &aspeed_intc_data[intc->type];
> 	void __iomem *base = intc->base;
> 
> 	seq_printf(m, "%s\n", d->name)
> 	seq_printf(m, "P: 0x%08x\n", readl(base + d->prot_offs));
> 
> 	base += d->rout_offs;
> 	for (unsigned int i = 0; i < d->groups; i++, base += 4) {
> 		seq_printf(m, "R%d: 0x%08x 0x%08x 0x%08x\n", i, readl(base),
> 			   readl(base + d->rout_gap), readl(base + 2 * d->rout_gap));
> 	}
> 	return 0;
> }
> DEFINE_SHOW_ATTRIBUTE(aspeed_intc);
> 
> static int aspeed_intc_probe(struct platform_device *pdev) {
> 	struct aspeed_intc *intc;
> 	struct resource *res;
> 	struct dentry *dir;
> 
> 	intc = devm_kzalloc(&pdev->dev, sizeof(*intc), GFP_KERNEL);
> 	if (!intc)
> 		return -ENOMEM;
> 
> 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> 	intc->base = devm_ioremap_resource(&pdev->dev, res);
> 	if (IS_ERR(intc->base))
> 		return PTR_ERR(intc->base);
> 
> 	if (of_device_is_compatible(pdev->dev.of_node, "aspeed,ast2700-intc0"))
> 		intc->type = INTC_TYPE_C0;
> 	else if (of_device_is_compatible(pdev->dev.of_node,
> "aspeed,ast2700-intc1"))
> 		intc->type = INTC_TYPE_C1;
> 	else
> 		return -ENOTSUPP;
> 
> 	platform_set_drvdata(pdev, intc);
> 
> 	dir = debugfs_create_dir(dev_name(&pdev->dev), NULL);
> 	debugfs_create_file("intc_regs", 0400, dir, intc, &aspeed_intc_fops);
> 	return 0;
> }
> 
> static const struct of_device_id aspeed_intc_of_match[] = {
> 	{ .compatible	= "aspeed,ast2700-intc0", },
> 	{ .compatible	= "aspeed,ast2700-intc1", },
> 	{ },
> };
> MODULE_DEVICE_TABLE(of, aspeed_intc_of_match);
> 
> static struct platform_driver aspeed_intc_driver = {
> 	.probe  = aspeed_intc_probe,
> 	.driver = {
> 		.name		= "ast2700-intc",
> 		.of_match_table	= aspeed_intc_of_match,
> 	},
> };
> module_platform_driver(aspeed_intc_driver);

RE: [PATCH v3 2/2] irqchip: aspeed: add debugfs support and AST2700 INTC0/INTC1 routing/protection display

[Thomas Gleixner]

On Wed, Jul 23 2025 at 06:02, Ryan Chen wrote:
>> > +struct aspeed_intc {
>> > +	void __iomem *base;
>> > +	struct device *dev;
>> > +	struct dentry *dbg_root;
>> > +	int (*show_routing)(struct seq_file *s, void *unused);
>> > +	int (*show_prot)(struct seq_file *s, void *unused); };
>> 
>> See the chapter about struct declarations and initializers in the documentation
>> I linked to above.
>
> Sorry, I don't see the struct "> > +	int (*show_prot)(struct seq_file *s, void *unused); };"

I fatfingered that, but that's not the problem.

> My original submit is following, it should ok. Am I right?

No. Read the chapter I pointed you to.

> https://www.spinics.net/lists/kernel/msg5776957.html

I have replied to this very mail. No need to paste me this and the pointer
to some random mail archive

RE: [PATCH v3 2/2] irqchip: aspeed: add debugfs support and AST2700 INTC0/INTC1 routing/protection display

[Ryan Chen]

> Subject: RE: [PATCH v3 2/2] irqchip: aspeed: add debugfs support and AST2700
> INTC0/INTC1 routing/protection display
> 
> On Wed, Jul 23 2025 at 06:02, Ryan Chen wrote:
> >> > +struct aspeed_intc {
> >> > +	void __iomem *base;
> >> > +	struct device *dev;
> >> > +	struct dentry *dbg_root;
> >> > +	int (*show_routing)(struct seq_file *s, void *unused);
> >> > +	int (*show_prot)(struct seq_file *s, void *unused); };
> >>
> >> See the chapter about struct declarations and initializers in the
> >> documentation I linked to above.
> >
> > Sorry, I don't see the struct "> > +	int (*show_prot)(struct seq_file *s, void
> *unused); };"
> 
> I fatfingered that, but that's not the problem.
> 
> > My original submit is following, it should ok. Am I right?
> 
> No. Read the chapter I pointed you to.
> 
> > https://www.spinics.net/lists/kernel/msg5776957.html
Thanks, I think your point is align the struct member names.
Will update.
struct aspeed_intc {
        void __iomem		*base;
        struct device		*dev;
        struct dentry		*dbg_root;
        int				(*show_routing)(struct seq_file *s, void *unused);
        int				(*show_prot)(struct seq_file *s, void *unused);
};
> 
> I have replied to this very mail. No need to paste me this and the pointer to
> some random mail archive
>