linux-kernel - Re: [PATCH v5 1/3] dt-bindings: interrupt-controller: aspeed,ast2700: Add support for INTC hierarchy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251024231154.GA2962687-robh@kernel.org>
Date: Fri, 24 Oct 2025 18:11:54 -0500
From: Rob Herring <robh@...nel.org>
To: Ryan Chen <ryan_chen@...eedtech.com>
Cc: Thomas Gleixner <tglx@...utronix.de>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Joel Stanley <joel@....id.au>,
	Andrew Jeffery <andrew@...econstruct.com.au>,
	"jk@...econstruct.com.au" <jk@...econstruct.com.au>,
	Kevin Chen <kevin_chen@...eedtech.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>,
	"linux-aspeed@...ts.ozlabs.org" <linux-aspeed@...ts.ozlabs.org>
Subject: Re: [PATCH v5 1/3] dt-bindings: interrupt-controller:
 aspeed,ast2700: Add support for INTC hierarchy

On Thu, Oct 23, 2025 at 06:57:01AM +0000, Ryan Chen wrote:
> Hello Rob.
> 	Thank you for your detailed review and comments.
> 
> > Subject: Re: [PATCH v5 1/3] dt-bindings: interrupt-controller: aspeed,ast2700:
> > Add support for INTC hierarchy
> > 
> > On Wed, Oct 22, 2025 at 02:55:05PM +0800, Ryan Chen wrote:
> > > AST2700 contains two-level interrupt controllers (INTC0 and INTC1),
> > > each with its own register space and handling different sets of
> > > peripherals.
> > 
> > This is a mess!
> > 
> > How does this relate to the existing "aspeed,ast2700-intc-ic"? Its schema has a
> > block diagram of connections which I can understand. This does not.
> > 
> > The use of child nodes here is questionable. A variable number of interrupt
> > banks is not a reason to have child nodes. I'm only guessing that's what's
> > happening here because you haven't explained it.
> 
> Let me clarify the hardware structure and the purpose of these bindings.
> 
> The AST2700 SoC includes two top-level interrupt controller modules,
> INTC0 and INTC1. (aspeed,ast2700-intc0, aspeed,ast2700-intc1)
> Each of them provides routing selection and register protection
> features.
> Within each INTCx block, there are multiple sub-blocks called
> intc-ic, each handling multi-interrupt sources.
> ("aspeed,ast2700-intc0-ic", "aspeed,ast2700-intc1-ic")
> 
> Cascading occurs between the child banks:
> Level 1 : intc0-ic have multi-interrupts connect to GIC (root)
> Level 2 : multi Intc1-ic# connect to intc0-ic
> The parent intc0/1 nodes expose register regions for routing and
> protection control, serving as containers for their intc-ic children.

Being a 2nd vs. 3rd level interrupt controller is not a reason for 
different compatibles. The programming model is obviously the same for 
both as you essentially have 0 driver changes. Having N banks of 32 
interrupts vs. 1 bank of 32 interrupts is not a reason to have multiple 
intcN-ic nodes. That is a very common difference between instances of 
the same interrupt controller such as the GIC.

What you need to do is simply extend your driver to support N banks of 
32 interrupts. That's what almost every other irqchip driver with more 
than 32 interrupts does. If you are lucky, then the offset to each 
bank's registers is just hwirq/32 * <bank stride> and the number of 
banks can be calculated from the length of 'reg'. If you are not 
lucky, then you could put 1 'reg' entry for each bank.

AFAICT, the existing binding in aspeed,ast2700-intc.yaml should work for 
you.

> 
> The following simplified diagram shows the hierarchy:
> 
> 
>                                  +----------+       +----------+
>                                  |  intc0   |       |  intc1   |
> - - - - - - - - - - - - - - - - -+---- -----+- -  - +------ - -+
>   +-----------------------+      |          |       |          |
>   | +-------+ +---------+ |      |          |       |          |
>   | |       | |         | |      |          |       |          |
>   | |  PSP  +-+ GIC     | |      |          |       |          |
>   | |       | |         | |      |          |       |          |
>   | +-------+ |         | |      |          |       |          |
>   |           |         | |      +----------+       |          |
>   |           | 192~201 <-|------+          <-------+ intc1-ic |
>   |           +---------+ |      |          |       |          |
>   +-----------------------+      | intc0-ic <-------+ intc1-ic |
>                                  |          |       |          |
>                                  | 			<-------+ intc1-ic |
>                                  +----------+		  .....

You already match on intc0 and handle 32 interrupts. Now you are adding 
intc0-ic to match on and handling the same 32 interrupts?

Rob