From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jeremy Kerr <jk@codeconstruct.com.au>
Date: Wed, 01 May 2024 19:17:29 +0800
Subject: [PATCH] ARM: dts: aspeed-g6: Add nodes for i3c controllers
In-Reply-To: <65af46c3-e48f-4eae-8390-2bc01332ccbf@kernel.org>
References: <20240501033832.1529340-1-jk@codeconstruct.com.au>
	 <65af46c3-e48f-4eae-8390-2bc01332ccbf@kernel.org>
Message-ID: <09ed10182e2282d7e408a4fef7994da010fe058e.camel@codeconstruct.com.au>
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

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:

    &i3c0 {
            status = "okay";
            mctp-controller;
    };

    &i3c1 {
            status = "okay";
            mctp-controller;
    };

There is existing precedence for this layout; the i2c and pinctrl
mappings already use dtsi-internal labels. It keeps the bus definitions
more manageable.

Cheers,


Jeremy