Re: [PATCH net-next v7 5/7] ARM64: dts: marvell: Fix some common switch mistakes
From: Chris Packham
Date: Tue Oct 24 2023 - 16:10:24 EST
Hi All,
On 25/10/23 07:28, Vladimir Oltean wrote:
> Linus,
>
> On Tue, Oct 24, 2023 at 03:20:31PM +0200, Linus Walleij wrote:
>> Fix some errors in the Marvell MV88E6xxx switch descriptions:
>> - The top node had no address size or cells.
>> - switch0@0 is not OK, should be ethernet-switch@0.
>> - ports should be ethernet-ports
>> - port@0 should be ethernet-port@0
>> - PHYs should be named ethernet-phy@
>>
>> Reviewed-by: Andrew Lunn <andrew@xxxxxxx>
>> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
<snip>
>> ---
>> diff --git a/arch/arm64/boot/dts/marvell/cn9130-crb.dtsi b/arch/arm64/boot/dts/marvell/cn9130-crb.dtsi
>> index 32cfb3e2efc3..7538ed56053b 100644
>> --- a/arch/arm64/boot/dts/marvell/cn9130-crb.dtsi
>> +++ b/arch/arm64/boot/dts/marvell/cn9130-crb.dtsi
>> @@ -207,11 +207,9 @@ phy0: ethernet-phy@0 {
>> reg = <0>;
>> };
>>
>> - switch6: switch0@6 {
>> + switch6: ethernet-switch@6 {
>> /* Actual device is MV88E6393X */
>> compatible = "marvell,mv88e6190";
>> - #address-cells = <1>;
>> - #size-cells = <0>;
>> reg = <6>;
>> interrupt-parent = <&cp0_gpio1>;
>> interrupts = <28 IRQ_TYPE_LEVEL_LOW>;
>> @@ -220,59 +218,59 @@ switch6: switch0@6 {
>>
>> dsa,member = <0 0>;
>>
>> - ports {
>> + ethernet-ports {
>> #address-cells = <1>;
>> #size-cells = <0>;
>>
>> - port@1 {
>> + ethernet-port@1 {
>> reg = <1>;
>> label = "p1";
>> phy-handle = <&switch0phy1>;
>> };
>>
>> - port@2 {
>> + ethernet-port@2 {
>> reg = <2>;
>> label = "p2";
>> phy-handle = <&switch0phy2>;
>> };
>>
>> - port@3 {
>> + ethernet-port@3 {
>> reg = <3>;
>> label = "p3";
>> phy-handle = <&switch0phy3>;
>> };
>>
>> - port@4 {
>> + ethernet-port@4 {
>> reg = <4>;
>> label = "p4";
>> phy-handle = <&switch0phy4>;
>> };
>>
>> - port@5 {
>> + ethernet-port@5 {
>> reg = <5>;
>> label = "p5";
>> phy-handle = <&switch0phy5>;
>> };
>>
>> - port@6 {
>> + ethernet-port@6 {
>> reg = <6>;
>> label = "p6";
>> phy-handle = <&switch0phy6>;
>> };
>>
>> - port@7 {
>> + ethernet-port@7 {
>> reg = <7>;
>> label = "p7";
>> phy-handle = <&switch0phy7>;
>> };
>>
>> - port@8 {
>> + ethernet-port@8 {
>> reg = <8>;
>> label = "p8";
>> phy-handle = <&switch0phy8>;
>> };
>>
>> - port@9 {
>> + ethernet-port@9 {
>> reg = <9>;
>> label = "p9";
>> phy-mode = "10gbase-r";
>> @@ -280,7 +278,7 @@ port@9 {
>> managed = "in-band-status";
>> };
>>
>> - port@a {
>> + ethernet-port@a {
>> reg = <10>;
>> ethernet = <&cp0_eth0>;
>> phy-mode = "10gbase-r";
>> @@ -293,35 +291,35 @@ mdio {
>> #address-cells = <1>;
>> #size-cells = <0>;
>>
>> - switch0phy1: switch0phy1@1 {
>> + switch0phy1: ethernet-phy@1 {
>> reg = <0x1>;
>> };
>>
>> - switch0phy2: switch0phy2@2 {
>> + switch0phy2: ethernet-phy@2 {
>> reg = <0x2>;
>> };
>>
>> - switch0phy3: switch0phy3@3 {
>> + switch0phy3: ethernet-phy@3 {
>> reg = <0x3>;
>> };
>>
>> - switch0phy4: switch0phy4@4 {
>> + switch0phy4: ethernet-phy@4 {
>> reg = <0x4>;
>> };
>>
>> - switch0phy5: switch0phy5@5 {
>> + switch0phy5: ethernet-phy@5 {
>> reg = <0x5>;
>> };
>>
>> - switch0phy6: switch0phy6@6 {
>> + switch0phy6: ethernet-phy@6 {
>> reg = <0x6>;
>> };
>>
>> - switch0phy7: switch0phy7@7 {
>> + switch0phy7: ethernet-phy@7 {
>> reg = <0x7>;
>> };
>>
>> - switch0phy8: switch0phy8@8 {
>> + switch0phy8: ethernet-phy@8 {
>> reg = <0x8>;
>> };
>> };
> Chris, does this look okay?
There's nothing in the u-boot code for the CN9130-CRB that cares about
the switch so I don't think there's any issue ABI wise. We are working
on our own CN9130 based router with a L2 switch but it's at a point we
can follow whatever upstream decide is the final schema.
In terms of my personal preference the schema quoted up thread has the
pattern '^(ethernet-)?switch(@.*)?$' (i.e. the 'ethernet-' part is
optional) so I'd personally prefer switch0@6 -> switch@6 but that's only
a slight preference because I deal with Ethernet switches day in day out.