RE: [PATCH v2 1/2] dts: arm64: add LS1043A DPAA support

[Madalin-Cristian Bucur]

> -----Original Message-----
> From: Shawn Guo [mailto:shawnguo@xxxxxxxxxx]
> Sent: Monday, March 27, 2017 10:56 AM
> Subject: Re: [PATCH v2 1/2] dts: arm64: add LS1043A DPAA support
> 
> On Mon, Mar 27, 2017 at 07:03:40AM +0000, Madalin-Cristian Bucur wrote:
> > > > > > +	fman@1a00000 {
> > > > > > +		enet0: ethernet@e0000 {
> > > > > > +		};
> > > > > > +
> > > > > > +		enet1: ethernet@e2000 {
> > > > > > +		};
> > > > > > +
> > > > > > +		enet2: ethernet@e4000 {
> > > > > > +		};
> > > > > > +
> > > > > > +		enet3: ethernet@e6000 {
> > > > > > +		};
> > > > > > +
> > > > > > +		enet4: ethernet@e8000 {
> > > > > > +		};
> > > > > > +
> > > > > > +		enet5: ethernet@ea000 {
> > > > > > +		};
> > > > > > +
> > > > > > +		enet6: ethernet@f0000 {
> > > > > > +		};
> > > > > > +	};
> > > > >
> > > > > I do not quite understand why these nodes are empty.
> > > >
> > > > These nodes provide the aliases (and custom SoC mapping) for the
> > > > FMan ports that are used on this particular SoC. The particular
> > > > node details are found in the port dtsi file thus no information
> > > > is required here. Given the fact that the numbering and actual
> > > > ports that are in use can vary between SoCs, the aliases cannot
> > > > be included in the port dtsi nor in the FMan dtsi.
> > >
> > > Do not completely follow.  What do you mean by 'port dtsi file'?
> Maybe
> > > I should wait for you new patches with better commit log and comments
> to
> > > understand these odd empty nodes.
> >
> > The DPAA IP can have a certain number of ports. Out of those, a certain
> > SoC can use all or only a subset, with diverse decisions on actual
> numbering
> > of the used ports. Next, when using the SoC on a particular board, some
> > ports will be used, some will not. The file hierarchy relates to this
> > hierarchy - you have individual port files that are included by the
> > SoC dtsi which in turn is included by the board dts. These nodes do not
> > need any new content as all the node details are provided by the port
> > dtsi files. The information they provide is the alias used for each
> port.
> 
> My impression is that such hierarchy mapping is not really necessary and
> only makes the device tree source messy and hard to follow.  I do not
> like it.

Hi Shawn, I respect your opinion on this, I'm sure it is the result of an
extensive experience dealing with less complicated devices. Before breaking
a construct that to date has served the DPAA users well I'd like to hear
more thoughts on this topic.

> >
> > > >
> > > > > > +};
> > > > > > diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a-qds.dts
> > > > > b/arch/arm64/boot/dts/freescale/fsl-ls1043a-qds.dts
> > > > > > index 0989d63..ee66bb2 100644
> > > > > > --- a/arch/arm64/boot/dts/freescale/fsl-ls1043a-qds.dts
> > > > > > +++ b/arch/arm64/boot/dts/freescale/fsl-ls1043a-qds.dts
> > > > > > @@ -181,3 +181,5 @@
> > > > > >  		reg = <0>;
> > > > > >  	};
> > > > > >  };
> > > > > > +
> > > > > > +/include/ "fsl-ls1043-post.dtsi"
> > > > >
> > > > > Move it to header of the file.
> > > >
> > > > This is to be included at the end, to make sure the references are
> > > > met and to allow overrides if needed.
> > >
> > > What is broken if you move the include to header?
> >
> > Not much besides the structure we've always used for our SoCs device
> > trees. The file is called "-post.dtsi" because here is the place any
> > required overrides can be made, if needed. Moving to the top renders
> > having this separate file useless.
> 
> That's great, and let's kill it then.
> 
> >
> > > >
> > > > > > diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a-rdb.dts
> > > > > b/arch/arm64/boot/dts/freescale/fsl-ls1043a-rdb.dts
> > > > > > index c37110b..d94f003 100644
> > > > > > --- a/arch/arm64/boot/dts/freescale/fsl-ls1043a-rdb.dts
> > > > > > +++ b/arch/arm64/boot/dts/freescale/fsl-ls1043a-rdb.dts
> > > > > > @@ -139,3 +139,78 @@
> > > > > >  &duart1 {
> > > > > >  	status = "okay";
> > > > > >  };
> > > > > > +
> > > > > > +/include/ "fsl-ls1043-post.dtsi"
> > > > > > +
> > > > >
> > > > > Ditto
> > > > >
> > > > > > +&soc {
> > > > > > +	fman@1a00000 {
> > > > > > +		ethernet@e0000 {
> > > > >
> > > > > You defined enet0 label.  Why don't you use it?
> > > > >
> > > >
> > > > The enet0 label is used by u-boot for fix-ups, providing the
> > > > actual offset here makes it easier to follow.
> > >
> > > You will not need to construct the node hierarchy with label.  And
> > > alias/label name is more easier to follow than offset.
> > >
> > > Shawn
> >
> > When I said easier to follow I was referring to someone creating a
> > new device tree for his custom board, not someone reading the device
> > tree. If you have the board and SoC reference manuals in your hands
> > and you are writing a new board device tree, having the offset here
> > makes things easier. The benefit of having one less indentation level
> > is lesser than that.
> 
> The while complex and messy file hierarchy makes users' life harder,
> both the ones reading the device tree and the ones creating board device
> tree.  I would suggest you go the opposite, making the device tree
> simple and easy for users by allowing data duplication.  In arm/arm64
> device tree world, we do not consider DT data reusing/sharing among
> different SoCs that much.
> 
> Shawn

Complex it is, mirroring the IP, but messy it is a word I would not use.

Regards,
Madalin