Message-ID: <ce3044aa-2b62-4516-99ff-88dcb56b128b@kernel.org>
Date: Mon, 16 Jun 2025 12:29:38 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Ryan Chen <ryan_chen@...eedtech.com>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Joel Stanley <joel@....id.au>,
 Andrew Jeffery <andrew@...econstruct.com.au>,
 Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
 Arnd Bergmann <arnd@...db.de>,
 Bjorn Andersson <bjorn.andersson@....qualcomm.com>,
 Geert Uytterhoeven <geert@...ux-m68k.org>, Nishanth Menon <nm@...com>,
 "nfraprado@...labora.com" <nfraprado@...labora.com>,
 Taniya Das <quic_tdas@...cinc.com>,
 Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>,
 Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>,
 Eric Biggers <ebiggers@...gle.com>,
 "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-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "soc@...ts.linux.dev" <soc@...ts.linux.dev>, Mo Elbadry
 <elbadrym@...gle.com>, Rom Lemarchand <romlem@...gle.com>,
 William Kennington <wak@...gle.com>, Yuxiao Zhang <yuxiaozhang@...gle.com>,
 "wthai@...dia.com" <wthai@...dia.com>, "leohu@...dia.com"
 <leohu@...dia.com>, "dkodihalli@...dia.com" <dkodihalli@...dia.com>,
 "spuranik@...dia.com" <spuranik@...dia.com>
Subject: Re: [PATCH v0 3/5] arm64: dts: aspeed: Add initial AST2700 SoC device
 tree

On 16/06/2025 09:52, Ryan Chen wrote:
> 
>> Subject: Re: [PATCH v0 3/5] arm64: dts: aspeed: Add initial AST2700 SoC device
>> tree
>>
>> On 16/06/2025 08:54, Ryan Chen wrote:
>>>> Subject: Re: [PATCH v0 3/5] arm64: dts: aspeed: Add initial AST2700
>>>> SoC device tree
>>>>
>>>> On 16/06/2025 08:32, Ryan Chen wrote:
>>>>>>>
>>>>>>> But I don't know your previous "NAK, never tested" mean.
>>>>>>> I did make CHECK_DTBS=y arch/arm64/boot/dts/aspeed/ don't see the
>>>>>>> fail with
>>>>>>> intc0: interrupt-controller@...00000 {
>>>>>>> 	compatible = "simple-mfd";
>>>>>>>
>>>>>>> So, could you point me more test instruction for this?
>>>>>> See syscon.yaml. And writing bindings or talks on conferences:
>>>>>> simple-mfd cannot be alone.
>>>>>>
>>>>>
>>>>>         intc0: interrupt-controller@...00000 { Sorry, do you mean
>>>>> add by following?
>>>>> 				 compatible = "aspeed,intc-controller", "simple-mfd";
>>>>>  					.....
>>>>>                  intc0_11: interrupt-controller@...0 {
>>>>> 					compatible = "aspeed,ast2700-intc-ic";
>>>>>  					......
>>>>>                  };
>>>>>          };
>>>>
>>>> Maybe, but you said this is base address, so how can it be some
>>>> separate device?
>>>>
>>>> I mean really, don't add fake nodes just to satisfy some device instantiation.
>>>> Describe what this really is. That is the job of DTS. Not some fake nodes.
>>>
>>>
>>> Understood. Let me explain more about the hardware layout.
>>> The interrupt controller space is decoded starting from 0x12100000,
>>> which includes both a set of global configuration registers and
>>> individual interrupt controller instances.
>>>
>>> The region at 0x12100000 contains global interrupt control registers
>>> (e.g., protect config, interrupt routing etc.).
>>
>> This does not explain me why global controller registers are a BUS or MFD as
>> you claimed here.
>>>
>>> The actual interrupt controller logic starts at 0x12101b00, where each
>>> sub-controller instance (e.g., intc0_11, intc0_12, etc.) has its own set of
>> registers.
>>
>> I don't know what is a "global controller register" and "own set of registers". If
>> you have device spanning over multiple memory blocks, device takes multiple
>> 'reg' entries for example. There are many other configurations, depending on
>> real hardware and relationships. Just look at other DTS.
> 
> 
> Here are two possible representations of the interrupt controller layout for the AST2700 platform:
> Please advise which approach would be more appropriate or preferred? 
> 
> Option 1: Hierarchical representation with a parent node
> This models the entire interrupt controller registers space (start from 0x12100000), 
> where the parent node includes the global register area and acts as a container for per-instance sub-controllers:
> 
>     intc0: interrupt-controller@...00000 {
>         compatible = "aspeed,intc-controller";
>         reg = <0 0x12100000 0x4000>;
> 		...................
>         intc0_11: interrupt-controller@...0 {
>             compatible = "aspeed,ast2700-intc-ic";
>             reg = <0x1b00 0x100>;
> 			...................        };
>     };
> And I find the others dtsi have global register use syscon ex.
> https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/sprd/ums512.dtsi#L177-L192
> 
> Option 2: Flat representation with only the per-domain node
> This focuses on just the interrupt controller logic at 0x12101b00, skipping the global register modeling:
> 
>     intc0_11: interrupt-controller@...01b00 {
>         compatible = "aspeed,ast2700-intc-ic";
>         reg = <0 0x12101b00 0x100>;
> 		...................
>     };
> 


I don't understand this: you already have a binding for this, you
already described the device, so why now this is being changed?

You are supposed to send complete bindings for your device (see writing
bindings). Not some half-baked parts and then half year later different
DTS which points that your bindings were just incomplete.

Look how a completely new SoC is supposed to be upstreamed, on the day
of public announcement of the hardware:

https://lore.kernel.org/all/20231121-topic-sm8650-upstream-dt-v3-0-db9d0507ffd3@linaro.org/

Are all or most bindings posted? Yes
Is DTS for all devices from above bindings posted? Yes
Do we see complete picture? Yes

I still have no clue what is global interrupt registers. I already said
it, but you keep repeating the same. I have no clue.

Why this would be a parent?

Why this would not be a parent?

Best regards,
Krzysztof

Powered by blists - more mailing lists