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

[Ryan Chen <ryan_chen@aspeedtech.com>]

> Subject: Re: [PATCH v4 1/2] dt-bindings: interrupt-controller: aspeed: Add
> parent compatibles and refine documentation
> 
> On Tue, Aug 12, 2025 at 06:08:29PM +0800, Ryan Chen wrote:
> > AST2700 contains two independent top-level interrupt controllers
> > (INTC0, INTC1). Each occupies its own register space and handles
> > different sets of peripherals. Above them, the PSP (CA35) GIC is the
> > root interrupt aggregator. In hardware, INTC1 outputs are routed into
> > INTC0, and INTC0 outputs are routed into the GIC.
> >
> > Introduce distinct compatibles for these parent blocks so the DT can
> > model the hierarchy and register space layout accurately:
> >
> >   - aspeed,ast2700-intc0  (parent node at 0x12100000)
> >   - aspeed,ast2700-intc1  (parent node at 0x14c18000)
> >
> > The existing child compatible:
> >
> >   - aspeed,ast2700-intc-ic
> >
> > continues to describe the interrupt-controller instances within each
> > INTC block (e.g. INTC0_0..INTC0_11 and INTC1_0..INTC1_5).
> >
> > 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..81304b53c112 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  |
> > +  +-----+   +---------+
> 
> Same problem as last time. This tells me intc0 has not children...
> 
> > +            +---------+
> > +            |         |---module0
> > +            | INTC0_0 |---module1
> > +            |         |---...
> > +            +---------+---module31
> > +            |---....  |
> > +            +---------+
> > +            |         |     +---------+
> > +            | INTC0_11| +---| INTC1   |
> > +            |         |     +---------+
> 
> ...This tells that inc1 has no children (only intc0_11, which you said is
> aspeed,ast2700-intc-ic !!!)....
> (keep scrolling)
> 
> > +            +---------+     +---------+---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@":
> 
> ... but this tells me that intc0 and intc1 has children.
> 
> > +    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.
> > +
> > +      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.
> 
> No, you do not define two. Only interrupts.

Thanks, I did my homework, it is no need. 

> 
> > +
> > +    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>;
> 
> 
> ... and that's quite wrong unit address. Also no resources in the parent, so this
> entire split seems superficial and incorrect.
Sorry, it is my mistake.

> 
> This binding is not improving. You are not responding to REAL problems
> described to you. What's more, you send it in a way making our life difficult,
> look:

Sorry, My intent was to describe the SoC register space more precisely with dtsi,
by having intc0/intc1 as parent nodes and the multiple intc-ic instances as children.
But if this approach is not acceptable, I will drop the parent node split and keep
only the multi-instance aspeed,ast2700-intc-ic nodes, as before.

Please let me know if you think there is still a way to keep the hierarchy useful.
Thanks your guidance. 
> 
> b4 diff '20250812100830.145578-2-ryan_chen@aspeedtech.com'
> Using cached copy of the lookup
> ---
> Analyzing 3 messages in the thread
> Could not find lower series to compare against.
> 
> Best regards,
> Krzysztof