Hi Claudiu,

Am 2021-01-14 16:20, schrieb Claudiu Manoil:
-----Original Message-----
From: Michael Walle <mich...@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?

-michael

Reply via email to