From mboxrd@z Thu Jan  1 00:00:00 1970
From: Krzysztof Kozlowski <krzk@kernel.org>
Date: Wed, 1 May 2024 18:45:43 +0200
Subject: [PATCH] ARM: dts: aspeed-g6: Add nodes for i3c controllers
In-Reply-To: <09ed10182e2282d7e408a4fef7994da010fe058e.camel@codeconstruct.com.au>
References: <20240501033832.1529340-1-jk@codeconstruct.com.au>
 <65af46c3-e48f-4eae-8390-2bc01332ccbf@kernel.org>
 <09ed10182e2282d7e408a4fef7994da010fe058e.camel@codeconstruct.com.au>
Message-ID: <c5331cf8-7295-4e6a-ba39-e0751a2c357e@kernel.org>
List-Id: <linux-aspeed.lists.ozlabs.org>
To: linux-aspeed@lists.ozlabs.org
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit

On 01/05/2024 13:17, Jeremy Kerr wrote:
> Hi Krzysztof,
> 
>>> --- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
>>> +++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
>>> @@ -866,6 +866,13 @@ i2c: bus at 1e78a000 {
>>> ????????????????????????????????ranges = <0 0x1e78a000 0x1000>;
>>> ????????????????????????};
>>> ?
>>> +???????????????????????i3c: bus at 1e7a0000 {
>>> +???????????????????????????????compatible = "simple-bus";
>>
>> What bus is it? Why is it even needed? If it is i3c, then for sure
>> compatible is wrong.
> 
> This is not the i3c bus, it's the MMIO mapping that allows us to specify
> the individual i3c controller mappings as sensible offsets into the main
> address space. Did you miss the ranges property there?
> 
> This is following the existing design for the i2c controllers.
> 
>>> +???????????????????????????????#address-cells = <1>;
>>> +???????????????????????????????#size-cells = <1>;
>>> +???????????????????????????????ranges = <0 0x1e7a0000 0x8000>;
>>> +???????????????????????};
>>> +
>>> ????????????????????????fsim0: fsi at 1e79b000 {
>>> ????????????????????????????????compatible = "aspeed,ast2600-fsi-master", "fsi-master";
>>> ????????????????????????????????reg = <0x1e79b000 0x94>;
>>> @@ -1125,3 +1132,89 @@ i2c15: i2c-bus at 800 {
>>> ????????????????status = "disabled";
>>> ????????};
>>> ?};
>>> +
>>> +&i3c {
>>
>> ????
>>
>> That's not how we construct DTS.? Overrides/extends of nodes are for
>> boards, not within DTSI.
> 
> The overrides are occurring at the &i3cX labels, not &i3c. Platform
> level dts just connect at those labels to define overrides for each bus:

You miss the point. Look how DTS is constructed, read DTS coding style.

Your first node is empty and that is not readable.

Best regards,
Krzysztof