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 9:27 AM
> Subject: Re: [PATCH v2 1/2] dts: arm64: add LS1043A DPAA support
> 
> On Wed, Mar 22, 2017 at 10:58:12AM +0000, Madalin-Cristian Bucur wrote:
> > > > +&soc {
> > > > +
> > > > +/include/ "qoriq-fman3-0.dtsi"
> > > > +/include/ "qoriq-fman3-0-1g-0.dtsi"
> > > > +/include/ "qoriq-fman3-0-1g-1.dtsi"
> > > > +/include/ "qoriq-fman3-0-1g-2.dtsi"
> > > > +/include/ "qoriq-fman3-0-1g-3.dtsi"
> > > > +/include/ "qoriq-fman3-0-1g-4.dtsi"
> > > > +/include/ "qoriq-fman3-0-1g-5.dtsi"
> > > > +/include/ "qoriq-fman3-0-10g-0.dtsi"
> > >
> > > We usually put the includes at the beginning of the file, and #include
> > > is more recommended than /include/.
> >
> > I'm not making use of the header file inclusion feature #include
> provides
> > (nor plan to) in these files thus I've selected /include/ here.
> 
> Let's be simple and consistent.  Use #include please.

I can do that, running the preprocessor on these files without being required
does not add that much time in the end. I'm not a fan of this feature, I never
liked the fact one loses the liberty of easily using dtc directly to debug
using dtc -O dts when adding #includes.

> > > > +	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.

> >
> > > > +};
> > > > 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.

> >
> > > > 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.

Madalin