From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claudiu Manoil Date: Fri, 15 Jan 2021 17:03:19 +0000 Subject: [PATCH 4/5] arm: dts: ls1028a: Add Ethernet switch node and dependencies In-Reply-To: <66b1985841858560b2099a61c0eacf9c@walle.cc> References: <20210113180526.21797-1-claudiu.manoil@nxp.com> <20210113180526.21797-5-claudiu.manoil@nxp.com> <61557b3ffcb3df6f693a9114aac983c7@walle.cc> <66b1985841858560b2099a61c0eacf9c@walle.cc> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de >-----Original Message----- >From: Michael Walle >Sent: Thursday, January 14, 2021 5:40 PM >To: Claudiu Manoil >Cc: Joe Hershberger ; Simon Glass >; Bin Meng ; u- >boot at lists.denx.de; Vladimir Oltean ; Alexandru >Marginean >Subject: Re: [PATCH 4/5] arm: dts: ls1028a: Add Ethernet switch node and >dependencies > >Hi Claudiu, > >Am 2021-01-14 16:20, schrieb Claudiu Manoil: >>> -----Original Message----- >>> From: Michael Walle >>> Sent: Wednesday, January 13, 2021 10:11 PM >> [...] >>>> --- a/arch/arm/dts/fsl-ls1028a.dtsi >>>> +++ b/arch/arm/dts/fsl-ls1028a.dtsi >>>> @@ -14,6 +14,17 @@ >>>> #address-cells = <2>; >>>> #size-cells = <2>; >>>> >>>> + aliases { >>>> + eth0 = &enetc0; >>>> + eth1 = &enetc1; >>>> + eth2 = &enetc2; >>>> + eth3 = &enetc6; >>>> + eth4 = &felix0; >>>> + eth5 = &felix1; >>>> + eth6 = &felix2; >>>> + eth7 = &felix3; >>>> + }; >>> >>> Don't include the aliases in the common dtsi. There are serveral >>> reasons for that: >>> (1) it is really board dependent. not every board has all these >>> ports. >>> (2) it will mess up the device numbering for boards which use >>> this dtsi. And with this it will also mess up the ethNaddr >>> environment variable logic. >>> >>> Please move them into the corresponding boards. >>> >> I think the point of this patch was to enable the felix switch for the >> LS1028A RDB only for now, >> for simplicity, to make the patch set smaller. Follow-up patches would >> enable it for the remaining >> boards. >> But I understand that keeping the aliases in the fsl-ls1028a.dtsi can >> mess up other boards >> that include it, including the Kontron boards. Would you agree to >> update only the ls1028 RDB >> board DT for now? > >Sure, once accepted, I'll post a follow-up for our board. > >> Alternatively you could test these patches on your boards and maybe >> provide the DT updates for >> the Kontron boards as part of this patch set. > >I'm going to test this anyways and add a Tested-by to this set, once it >tested successfully. > >At the moment the only board variant which this would apply to is not >upstream yet. Patches are pending. But sure, if it will be in upstream >you could pick it up for this set. > >>>> + >>>> sysclk: sysclk { >>>> compatible = "fixed-clock"; >>>> #clock-cells = <0>; >>>> @@ -151,9 +162,51 @@ >>>> reg = <0x000300 0 0 0 0>; >>>> status = "disabled"; >>>> }; >>>> + ethsw: pci at 0,5 { >>>> + #address-cells=<0>; >>>> + #size-cells=<1>; >>>> + reg = <0x000500 0 0 0 0>; > >This should also have > status = "disabled" > >>>> + >>>> + ethsw_ports: ports { >>>> + #address-cells = <1>; >>>> + #size-cells = <0>; >>>> + >>>> + felix0: port at 0 { >>>> + reg = <0>; >>>> + status = "disabled"; >>>> + label = "swp0"; >>>> + }; >>>> + felix1: port at 1 { >>>> + reg = <1>; >>>> + status = "disabled"; >>>> + label = "swp1"; >>>> + }; >>>> + felix2: port at 2 { >>>> + reg = <2>; >>>> + status = "disabled"; >>>> + label = "swp2"; >>>> + }; >>>> + felix3: port at 3 { >>>> + reg = <3>; >>>> + status = "disabled"; >>>> + label = "swp3"; >>>> + }; >>>> + port at 4 { >>>> + reg = <4>; >>>> + phy-mode = "internal"; >>>> + status = "okay"; >>>> + ethernet = <&enetc2>; >>>> + }; >>> >>> status = "disabled". >>> >>> Why would you enable just this port if all the switch ports >>> are disabled. >>> >> >> The number of active front panel ports may vary from board to board. >> These ports are supposed to be enabled in each board DT, depending >> on setup. But a DSA switch always needs a CPU port regardless of how >> many front side ports are active in each particular setup, and port at 4 >> is >> the CPU port. > >Sure, but my point was that the default should be disabled - as it >should be for any peripheral in the dtsi - and it should be enabled per >board. >What if I'd rather use the other internal port? The linux dtsi also has all >ports disabled by default. Speaking of the linux device tree. The u-boot >one and the linux one are out-of-sync. Is there a reason why you >couldn't take the "ethernet-switch at 0,5" fragment from the linux tree? > Michel, I agree with keeping the device tree nodes between linux and uboot as consistent as possible, so port 5 can look like this in the dtsi : mscc_felix_port5: port at 5 { reg = <5>; phy-mode = "internal"; status = "disabled"; fixed-link { speed = <1000>; full-duplex; }; }; as it is currently in the upstream kernel: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi?h=v5.11-rc3 I'd drop however properties that are not used, like managed = "in-band-status" for the external ports, since this is phylink specific, and I don't expect we'll have phylink in uboot anytime soon.