[PATCH 4/5] arm: dts: ls1028a: Add Ethernet switch node and dependencies

[Claudiu Manoil]

>-----Original Message-----
>From: Michael Walle <michael at walle.cc>
>Sent: Thursday, January 14, 2021 5:40 PM
>To: Claudiu Manoil <claudiu.manoil at nxp.com>
>Cc: Joe Hershberger <joe.hershberger at ni.com>; Simon Glass
><sjg at chromium.org>; Bin Meng <bmeng.cn at gmail.com>; u-
>boot at lists.denx.de; Vladimir Oltean <vladimir.oltean at nxp.com>; Alexandru
>Marginean <alexandru.marginean at nxp.com>
>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 <michael at walle.cc>
>>> 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.