>-----Original Message-----
>From: Michael Walle <mich...@walle.cc>
>Sent: Thursday, January 14, 2021 5:40 PM
>To: Claudiu Manoil <claudiu.man...@nxp.com>
>Cc: Joe Hershberger <joe.hershber...@ni.com>; Simon Glass
><s...@chromium.org>; Bin Meng <bmeng...@gmail.com>; u-
>b...@lists.denx.de; Vladimir Oltean <vladimir.olt...@nxp.com>; Alexandru
>Marginean <alexandru.margin...@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 <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?
>
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/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.
