-----Original Message----- From: Michael Walle michael@walle.cc Sent: Thursday, January 14, 2021 5:40 PM To: Claudiu Manoil claudiu.manoil@nxp.com Cc: Joe Hershberger joe.hershberger@ni.com; Simon Glass sjg@chromium.org; Bin Meng bmeng.cn@gmail.com; u- boot@lists.denx.de; Vladimir Oltean vladimir.oltean@nxp.com; Alexandru Marginean alexandru.marginean@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@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@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@0 {reg = <0>;status = "disabled";label = "swp0";};felix1: port@1 {reg = <1>;status = "disabled";label = "swp1";};felix2: port@2 {reg = <2>;status = "disabled";label = "swp2";};felix3: port@3 {reg = <3>;status = "disabled";label = "swp3";};port@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@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@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@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...
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.