Message-ID: <6de719dc84324166ed60bb8ec130cf2c9ef351f5.camel@codeconstruct.com.au>
Date: Fri, 06 Feb 2026 17:49:44 +1030
From: Andrew Jeffery <andrew@...econstruct.com.au>
To: Krzysztof Kozlowski <krzk@...nel.org>, Ryan Chen
 <ryan_chen@...eedtech.com>
Cc: Thomas Gleixner <tglx@...utronix.de>, Rob Herring <robh@...nel.org>, 
 Krzysztof Kozlowski	 <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Joel Stanley	 <joel@....id.au>, Paul Walmsley
 <pjw@...nel.org>, Palmer Dabbelt	 <palmer@...belt.com>, Albert Ou
 <aou@...s.berkeley.edu>, Alexandre Ghiti	 <alex@...ti.fr>,
 "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>, "linux-riscv@...ts.infradead.org"	
 <linux-riscv@...ts.infradead.org>, Jeremy Kerr <jk@...econstruct.com.au>
Subject: Re: [PATCH 0/4] Add AST2700 INTC0/INTC1 support

Hi Krzysztof,

I've been working with Ryan to address what I considered some
shortcomings of the original approach. For reference, I outlined my
concerns on v4 of the previous series[1].

[1]: https://lore.kernel.org/all/1a2ca78746e00c2ec4bfc2953a897c48376ed36f.camel@codeconstruct.com.au/

That reply proposed the new DT structure, and this series is a
refinement & implementation of those ideas

With that context:

> On Thu, 2026-02-05 at 10:56 +0100, Krzysztof Kozlowski wrote:
> On 05/02/2026 10:49, Ryan Chen wrote:
> 
> > Subject: Re: [PATCH 0/4] Add AST2700 INTC0/INTC1 support
> > 
> > On Thu, Feb 05, 2026 at 02:07:18PM +0800, Ryan Chen wrote:
> > > This series replaces the existing AST2700 interrupt controller binding
> > > and driver. The original implementation was focused on a narrow,
> > > PSP-centric view and could not fully describe the complexity of the
> > > AST2700 interrupt fabric:
> > > 
> > > * It was focused primarily on the perspective of the Primary Service
> > > > Processor (PSP).
> > > * It could not handle interrupt route configuration.
> > > * It could not handle interrupt register protection.
> > > 
> > > By contrast, the new bindings and drivers describe the interrupt
> > > controllers at the block-function level and provide a unified binding
> > > design that can be used from the perspective of any of the four
> > > integrated processors (the Primary, Secondary and Tertiary Service
> > > Processors, and the Boot MCU):
> > > 
> > > Where and how did you address last feedback given to you here:
> > > 
> > > https://lore.kernel.org/all/20250814-auspicious-thundering-jaybird-b76f4f@ku
> > > oka/

At this point the answer is that there wasn't a direct response.

I acknowledge that I'm not Ryan, and that it is important that he
responds directly to queries on his own patches. However, with the goal
of having an upstream binding that better represents the hardware
(along with a capable driver), I've pulled the concerns out of your
linked feedback for discussion below.

> > +  +-----+   +---------+
> > +  | GIC |---|  INTC0  |
> > +  +-----+   +---------+
> 
> Same problem as last time. This tells me intc0 has not children...

...

> > +            +---------+
> > +            |         |     +---------+
> > +            | INTC0_11| +---| INTC1   |
> > +            |         |     +---------+
> 
> ...This tells that inc1 has no children (only intc0_11, which you said
> is aspeed,ast2700-intc-ic !!!)....
> (keep scrolling)

...

> > +patternProperties:
> > +  "^interrupt-controller@":
> 
> ... but this tells me that intc0 and intc1 has children.

...

> > +        intc0_11: interrupt-controller@...0 {
> > +          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.

This gets to the heart of it. I share the view that the split was
superficial. It tried to recombine existing components to account for
some change in hardware design between early revisions of the AST2700
SoC. The original binding from Kevin was too fine-grained.

I don't think the design of the diagrams helped the cause for
understanding the proposed binding or the hardware architecture, and
were misleading in the manner you outlined in the comments above.

My reply at [1] above was an (indirect) attempt to address your
concerns, though again I acknowledge I'm not Ryan and that review
feedback needs direct responses from patch authors.

The binding proposed in this series eliminates the subnodes and enables
a complete implementation of routing for the hardware, as demonstrated
by the proposed drivers. I think it better conforms to the documented
DOs and DON'Ts for writing bindings by moving anything implied by the
compatible to the driver implementation. I hadn't yet merged the base
arch patches for the SoC because I had contentions with the already-
accepted binding, and merging the arch patches would make an already
difficult job of reversing that acceptance harder again.

In essence, this is some pain, but I view it as pain on the path
towards better DTS outcomes than we had for prior generations of ASPEED
BMC SoCs.

> > > 
> > > "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:"
> > > 
> > > So how did you make our life easier now?
> > 
> > Hi Krzysztof,
> > 
> > Thanks for your feedback.
> > 
> > The series you commented on in Aug 2025 (v4 1/2) attempted to model
> > The hardware by introducing parent compatibles (aspeed,ast2700-intc0 /
> > aspeed,ast2700-intc1) with child "interrupt-controller@" nodes using
> > aspeed,ast2700-intc-ic. In hindsight, that approach did not align well
> > with the actual hardware structure and resulted in inconsistencies
> > between the diagrams, the schema, and the register layout (including
> > unit-address issues). It was also difficult to review in isolation.
> 
> Read my question again:
> 
> "So how did you make our life easier now?"
> 
> And then read the earlier comment - what I expected of you. Please
> answer these after the "look:" part.
> 
> Answer these please. I am not going to review any of these because you
> keep ignoring our process of handling patches and not really responding
> to review comments.

For what it's worth, Ryan has adopted b4 and so hopefully some of the
expectations around the mechanics of patch review are less of a
concern.

In this specific case, because of the divergence in direction, we went
with a separate series. The failure to link that earlier series in the
cover letter was an unfortunate oversight. I'll take responsibility for
that, as part of developing the series with Ryan.

Sorry for causing you frustration.

Is it acceptable if we take the following actions:

   1. Do some b4 magic to transplant this series back onto [1]
   2. Send a follow up revision with a link to this discussion in the
      cover letter

Andrew

Powered by blists - more mailing lists