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

[Michael Walle]

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?

-michael