linux-kernel - Re: [PATCH v2 1/2] dts: arm64: add LS1043A DPAA support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 27 Mar 2017 14:27:21 +0800
From:   Shawn Guo <shawnguo@...nel.org>
To:     Madalin-Cristian Bucur <madalin.bucur@....com>
Cc:     Roy Pledge <roy.pledge@....com>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "catalin.marinas@....com" <catalin.marinas@....com>,
        "will.deacon@....com" <will.deacon@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Russell King - ARM Linux <linux@...linux.org.uk>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        Kumar Gala <galak@...eaurora.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
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.

> > > +	fman@...0000 {
> > > +		enet0: ethernet@...00 {
> > > +		};
> > > +
> > > +		enet1: ethernet@...00 {
> > > +		};
> > > +
> > > +		enet2: ethernet@...00 {
> > > +		};
> > > +
> > > +		enet3: ethernet@...00 {
> > > +		};
> > > +
> > > +		enet4: ethernet@...00 {
> > > +		};
> > > +
> > > +		enet5: ethernet@...00 {
> > > +		};
> > > +
> > > +		enet6: ethernet@...00 {
> > > +		};
> > > +	};
> > 
> > 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.

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

> 
> > > 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@...0000 {
> > > +		ethernet@...00 {
> > 
> > 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