[PATCH] dt-bindings: interrupt-controller: aspeed: Refine AST2700 binding description and example

[PATCH] dt-bindings: interrupt-controller: aspeed: Refine AST2700 binding description and example

[Ryan Chen]

- Update block diagram for better readability and accuracy.
- Clarify the relationship and function of INTC0, INTC1, and the GIC.
- Documentation and example refine.

This enhances the documentation quality and helps developers understand
the interrupt controller hierarchy and usage.

Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
---
 .../aspeed,ast2700-intc.yaml                  | 155 +++++++++++++-----
 1 file changed, 112 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..751a07d49c90 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,67 @@ 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.
+
+    required:
+      - compatible
+      - reg
+      - interrupt-controller
+      - '#interrupt-cells'
+      - interrupts
 
 required:
   - compatible
   - reg
-  - interrupt-controller
-  - '#interrupt-cells'
-  - interrupts
 
 additionalProperties: false
 
@@ -68,19 +113,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 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] dt-bindings: interrupt-controller: aspeed: Refine AST2700 binding description and example

[Krzysztof Kozlowski]

On 14/07/2025 09:17, Ryan Chen wrote:
> - Update block diagram for better readability and accuracy.
> - Clarify the relationship and function of INTC0, INTC1, and the GIC.
> - Documentation and example refine.
> 
> This enhances the documentation quality and helps developers understand
> the interrupt controller hierarchy and usage.

Changing ABI (compatibles) is not enhancing quality and is not explained
here.

> 
> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> ---
>  .../aspeed,ast2700-intc.yaml                  | 155 +++++++++++++-----
>  1 file changed, 112 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..751a07d49c90 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,67 @@ maintainers:
>  properties:
>    compatible:
>      enum:
> -      - aspeed,ast2700-intc-ic
> +      - aspeed,ast2700-intc0
> +      - aspeed,ast2700-intc1

No, you cannot change compatibles.

You just rewrite entire bindings just because of wish to "refine"?
Hardware changed? What happened here?

You need to clearly describe ABI impact and reasons, like possible bugs
you address. You cannot just rewrite existing binding into something
entirely else.

Best regards,
Krzysztof

RE: [PATCH] dt-bindings: interrupt-controller: aspeed: Refine AST2700 binding description and example

[Ryan Chen]

> Subject: Re: [PATCH] dt-bindings: interrupt-controller: aspeed: Refine AST2700
> binding description and example
> 
> On 14/07/2025 09:17, Ryan Chen wrote:
> > - Update block diagram for better readability and accuracy.
> > - Clarify the relationship and function of INTC0, INTC1, and the GIC.
> > - Documentation and example refine.
> >
> > This enhances the documentation quality and helps developers
> > understand the interrupt controller hierarchy and usage.
> 
> Changing ABI (compatibles) is not enhancing quality and is not explained here.
> 
Sorry, I would add following in description.
- add '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.

> >
> > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> > ---
> >  .../aspeed,ast2700-intc.yaml                  | 155
> +++++++++++++-----
> >  1 file changed, 112 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..751a07d49c90 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,67 @@
> > maintainers:
> >  properties:
> >    compatible:
> >      enum:
> > -      - aspeed,ast2700-intc-ic
> > +      - aspeed,ast2700-intc0
> > +      - aspeed,ast2700-intc1
> 
> No, you cannot change compatibles.
> 
> You just rewrite entire bindings just because of wish to "refine"?
> Hardware changed? What happened here?
> 

Thank you for your feedback.
There is no hardware change.
My intention was to clarify the interrupt controller hierarchy
by introducing separate compatible strings for the parent nodes.

Sorry, I don't change original compatibles, I add parent compatibles aspeed,ast2700-intc0, aspeed,ast2700-intc1.
Modify original to be child nodes, and still keep the same compatible aspeed,ast2700-intc-ic

> You need to clearly describe ABI impact and reasons, like possible bugs you
> address. You cannot just rewrite existing binding into something entirely else.
> 
> Best regards,
> Krzysztof

Re: [PATCH] dt-bindings: interrupt-controller: aspeed: Refine AST2700 binding description and example

[Rob Herring (Arm)]

On Mon, 14 Jul 2025 15:17:53 +0800, Ryan Chen wrote:
> - Update block diagram for better readability and accuracy.
> - Clarify the relationship and function of INTC0, INTC1, and the GIC.
> - Documentation and example refine.
> 
> This enhances the documentation quality and helps developers understand
> the interrupt controller hierarchy and usage.
> 
> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> ---
>  .../aspeed,ast2700-intc.yaml                  | 155 +++++++++++++-----
>  1 file changed, 112 insertions(+), 43 deletions(-)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml: address-cells: missing type definition
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml: size-cells: missing type definition
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.example.dts:39.15-41: Warning (reg_format): /example-0/bus/interrupt-controller@12100000/interrupt-controller@1b00:reg: property has invalid length (12 bytes) (#address-cells == 2, #size-cells == 2)
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.example.dtb: Warning (pci_device_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.example.dtb: Warning (pci_device_bus_num): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.example.dtb: Warning (simple_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.example.dtb: Warning (i2c_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.example.dtb: Warning (spi_bus_reg): Failed prerequisite 'reg_format'
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.example.dtb: interrupt-controller@12100000 (aspeed,ast2700-intc0): '#address-cells', '#size-cells' do not match any of the regexes: '^interrupt-controller@', '^pinctrl-[0-9]+$'
	from schema $id: http://devicetree.org/schemas/interrupt-controller/aspeed,ast2700-intc.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.example.dtb: interrupt-controller@12100000 (aspeed,ast2700-intc0): interrupt-controller@1b00:reg:0: [0, 303045376, 16] is too short
	from schema $id: http://devicetree.org/schemas/reg.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.example.dtb: interrupt-controller@14c18000 (aspeed,ast2700-intc1): '#address-cells', '#size-cells' do not match any of the regexes: '^interrupt-controller@', '^pinctrl-[0-9]+$'
	from schema $id: http://devicetree.org/schemas/interrupt-controller/aspeed,ast2700-intc.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.example.dtb: interrupt-controller@14c18000 (aspeed,ast2700-intc1): interrupt-controller@100: 'interrupts' is a required property
	from schema $id: http://devicetree.org/schemas/interrupt-controller/aspeed,ast2700-intc.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250714071753.2653620-1-ryan_chen@aspeedtech.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH] dt-bindings: interrupt-controller: aspeed: Refine AST2700 binding description and example

[Krzysztof Kozlowski]

On 14/07/2025 09:36, Ryan Chen wrote:
>> Subject: Re: [PATCH] dt-bindings: interrupt-controller: aspeed: Refine AST2700
>> binding description and example
>>
>> On 14/07/2025 09:17, Ryan Chen wrote:
>>> - Update block diagram for better readability and accuracy.
>>> - Clarify the relationship and function of INTC0, INTC1, and the GIC.
>>> - Documentation and example refine.
>>>
>>> This enhances the documentation quality and helps developers
>>> understand the interrupt controller hierarchy and usage.
>>
>> Changing ABI (compatibles) is not enhancing quality and is not explained here.
>>
> Sorry, I would add following in description.
> - add '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.
> 

It does not say why is this needed in the first place...

Best regards,
Krzysztof

Re: [PATCH] dt-bindings: interrupt-controller: aspeed: Refine AST2700 binding description and example

[Krzysztof Kozlowski]

On 14/07/2025 10:23, Krzysztof Kozlowski wrote:
> On 14/07/2025 09:36, Ryan Chen wrote:
>>> Subject: Re: [PATCH] dt-bindings: interrupt-controller: aspeed: Refine AST2700
>>> binding description and example
>>>
>>> On 14/07/2025 09:17, Ryan Chen wrote:
>>>> - Update block diagram for better readability and accuracy.
>>>> - Clarify the relationship and function of INTC0, INTC1, and the GIC.
>>>> - Documentation and example refine.
>>>>
>>>> This enhances the documentation quality and helps developers
>>>> understand the interrupt controller hierarchy and usage.
>>>
>>> Changing ABI (compatibles) is not enhancing quality and is not explained here.
>>>
>> Sorry, I would add following in description.
>> - add '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.
>>
> 
> It does not say why is this needed in the first place...
> 
And also never tested :/. I won't be reviewing it.


Best regards,
Krzysztof

RE: [PATCH] dt-bindings: interrupt-controller: aspeed: Refine AST2700 binding description and example

[Ryan Chen]

> Subject: Re: [PATCH] dt-bindings: interrupt-controller: aspeed: Refine AST2700
> binding description and example
> 
> On 14/07/2025 10:23, Krzysztof Kozlowski wrote:
> > On 14/07/2025 09:36, Ryan Chen wrote:
> >>> Subject: Re: [PATCH] dt-bindings: interrupt-controller: aspeed:
> >>> Refine AST2700 binding description and example
> >>>
> >>> On 14/07/2025 09:17, Ryan Chen wrote:
> >>>> - Update block diagram for better readability and accuracy.
> >>>> - Clarify the relationship and function of INTC0, INTC1, and the GIC.
> >>>> - Documentation and example refine.
> >>>>
> >>>> This enhances the documentation quality and helps developers
> >>>> understand the interrupt controller hierarchy and usage.
> >>>
> >>> Changing ABI (compatibles) is not enhancing quality and is not explained
> here.
> >>>
> >> Sorry, I would add following in description.
> >> - add '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.
> >>
> >
> > It does not say why is this needed in the first place...
> >
> And also never tested :/. I won't be reviewing it.

Thanks, will update example and test it, and send v2
> 
> 
> Best regards,
> Krzysztof