Re: [PATCH net-next v7 5/7] ARM64: dts: marvell: Fix some common switch mistakes

From: Vladimir Oltean
Date: Tue Oct 24 2023 - 14:28:56 EST

Next message: H. Peter Anvin: "Re: [PATCH v2 1/6] x86/bugs: Add asm helpers for executing VERW"
Previous message: Rafael J. Wysocki: "Re: [RFC PATCH v3 00/39] ACPI/arm64: add support for virtual cpuhotplug"
In reply to: Florian Fainelli: "Re: [PATCH net-next v7 5/7] ARM64: dts: marvell: Fix some common switch mistakes"
Next in thread: Russell King (Oracle): "Re: [PATCH net-next v7 5/7] ARM64: dts: marvell: Fix some common switch mistakes"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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>
> ---
> .../dts/marvell/armada-3720-espressobin-ultra.dts | 14 +-
> .../boot/dts/marvell/armada-3720-espressobin.dtsi | 20 +--
> .../boot/dts/marvell/armada-3720-gl-mv1000.dts | 20 +--
> .../boot/dts/marvell/armada-3720-turris-mox.dts | 189 +++++++++++----------
> .../boot/dts/marvell/armada-7040-mochabin.dts | 24 ++-
> .../dts/marvell/armada-8040-clearfog-gt-8k.dts | 22 +--
> arch/arm64/boot/dts/marvell/cn9130-crb.dtsi | 42 +++--
> 7 files changed, 164 insertions(+), 167 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin-ultra.dts b/arch/arm64/boot/dts/marvell/armada-3720-espressobin-ultra.dts
> index f9abef8dcc94..870bb380a40a 100644
> --- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin-ultra.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin-ultra.dts
> @@ -126,32 +126,32 @@ &switch0 {
>
> reset-gpios = <&gpiosb 23 GPIO_ACTIVE_LOW>;
>
> - ports {
> - switch0port1: port@1 {
> + ethernet-ports {
> + switch0port1: ethernet-port@1 {
> reg = <1>;
> label = "lan0";
> phy-handle = <&switch0phy0>;
> };
>
> - switch0port2: port@2 {
> + switch0port2: ethernet-port@2 {
> reg = <2>;
> label = "lan1";
> phy-handle = <&switch0phy1>;
> };
>
> - switch0port3: port@3 {
> + switch0port3: ethernet-port@3 {
> reg = <3>;
> label = "lan2";
> phy-handle = <&switch0phy2>;
> };
>
> - switch0port4: port@4 {
> + switch0port4: ethernet-port@4 {
> reg = <4>;
> label = "lan3";
> phy-handle = <&switch0phy3>;
> };
>
> - switch0port5: port@5 {
> + switch0port5: ethernet-port@5 {
> reg = <5>;
> label = "wan";
> phy-handle = <&extphy>;
> @@ -160,7 +160,7 @@ switch0port5: port@5 {
> };
>
> mdio {
> - switch0phy3: switch0phy3@14 {
> + switch0phy3: ethernet-phy@14 {
> reg = <0x14>;
> };
> };
> diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
> index 5fc613d24151..86ec0df1c676 100644
> --- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
> @@ -145,19 +145,17 @@ &usb2 {
> };
>
> &mdio {
> - switch0: switch0@1 {
> + switch0: ethernet-switch@1 {
> compatible = "marvell,mv88e6085";
> - #address-cells = <1>;
> - #size-cells = <0>;
> reg = <1>;
>
> dsa,member = <0 0>;
>
> - ports {
> + ethernet-ports {
> #address-cells = <1>;
> #size-cells = <0>;
>
> - switch0port0: port@0 {
> + switch0port0: ethernet-port@0 {
> reg = <0>;
> label = "cpu";
> ethernet = <&eth0>;
> @@ -168,19 +166,19 @@ fixed-link {
> };
> };
>
> - switch0port1: port@1 {
> + switch0port1: ethernet-port@1 {
> reg = <1>;
> label = "wan";
> phy-handle = <&switch0phy0>;
> };
>
> - switch0port2: port@2 {
> + switch0port2: ethernet-port@2 {
> reg = <2>;
> label = "lan0";
> phy-handle = <&switch0phy1>;
> };
>
> - switch0port3: port@3 {
> + switch0port3: ethernet-port@3 {
> reg = <3>;
> label = "lan1";
> phy-handle = <&switch0phy2>;
> @@ -192,13 +190,13 @@ mdio {
> #address-cells = <1>;
> #size-cells = <0>;
>
> - switch0phy0: switch0phy0@11 {
> + switch0phy0: ethernet-phy@11 {
> reg = <0x11>;
> };
> - switch0phy1: switch0phy1@12 {
> + switch0phy1: ethernet-phy@12 {
> reg = <0x12>;
> };
> - switch0phy2: switch0phy2@13 {
> + switch0phy2: ethernet-phy@13 {
> reg = <0x13>;
> };
> };

I looked at U-Boot's ft_board_setup() from board/Marvell/mvebu_armada-37xx/board.c
and it doesn't appear to do anything with the switch. But after the MOX precedent
(which is _still_ problematic, more below), I still think we are way too
trigger-happy with this, and it would be good to ask someone who has the
Espressobin to test.

Pali, you are the last committer on the Linux DTS, could you please boot-test
this change, or at least confirm that as far as you know, there are no bootloader
dependencies on the precise node name for the switch and its child nodes?

> diff --git a/arch/arm64/boot/dts/marvell/armada-3720-gl-mv1000.dts b/arch/arm64/boot/dts/marvell/armada-3720-gl-mv1000.dts
> index b1b45b4fa9d4..63fbc8352161 100644
> --- a/arch/arm64/boot/dts/marvell/armada-3720-gl-mv1000.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-gl-mv1000.dts
> @@ -152,31 +152,29 @@ &uart0 {
> };
>
> &mdio {
> - switch0: switch0@1 {
> + switch0: ethernet-switch@1 {
> compatible = "marvell,mv88e6085";
> - #address-cells = <1>;
> - #size-cells = <0>;
> reg = <1>;
>
> dsa,member = <0 0>;
>
> - ports: ports {
> + ports: ethernet-ports {
> #address-cells = <1>;
> #size-cells = <0>;
>
> - port@0 {
> + ethernet-port@0 {
> reg = <0>;
> label = "cpu";
> ethernet = <&eth0>;
> };
>
> - port@1 {
> + ethernet-port@1 {
> reg = <1>;
> label = "wan";
> phy-handle = <&switch0phy0>;
> };
>
> - port@2 {
> + ethernet-port@2 {
> reg = <2>;
> label = "lan0";
> phy-handle = <&switch0phy1>;
> @@ -185,7 +183,7 @@ port@2 {
> nvmem-cell-names = "mac-address";
> };
>
> - port@3 {
> + ethernet-port@3 {
> reg = <3>;
> label = "lan1";
> phy-handle = <&switch0phy2>;
> @@ -199,13 +197,13 @@ mdio {
> #address-cells = <1>;
> #size-cells = <0>;
>
> - switch0phy0: switch0phy0@11 {
> + switch0phy0: ethernet-phy@11 {
> reg = <0x11>;
> };
> - switch0phy1: switch0phy1@12 {
> + switch0phy1: ethernet-phy@12 {
> reg = <0x12>;
> };
> - switch0phy2: switch0phy2@13 {
> + switch0phy2: ethernet-phy@13 {
> reg = <0x13>;
> };
> };

Enrico, I see the GL-MV1000 device tree submission is relatively new.
Could you please ACK this change as well?

> diff --git a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
> index 9eab2bb22134..cdf1b8bdb230 100644
> --- a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
> @@ -304,7 +304,12 @@ phy1: ethernet-phy@1 {
> reg = <1>;
> };
>
> - /* switch nodes are enabled by U-Boot if modules are present */
> + /*
> + * NOTE: switch nodes are enabled by U-Boot if modules are present
> + * DO NOT change this node name (switch0@10) even if it is not following
> + * conventions! Deployed U-Boot binaries are explicitly looking for
> + * this node in order to augment the device tree!
> + */

Not "this node", but all switch nodes!

> switch0@10 {
> compatible = "marvell,mv88e6190";
> reg = <0x10>;
> @@ -317,92 +322,92 @@ 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>;
> };
> };
>
> - ports {
> + ethernet-ports {

U-Boot code does this, so you can't rename "ports":

/*
* now if there are more switches or a SFP module coming after,
* enable corresponding ports
*/
if (id < peridot + topaz - 1) {
res = fdt_status_okay_by_pathf(blob,
"%s/switch%i@%x/ports/port@a",
mdio_path, id, addr);
} else if (id == peridot - 1 && !topaz && sfp) {
res = fdt_status_okay_by_pathf(blob,
"%s/switch%i@%x/ports/port-sfp@a",
mdio_path, id, addr);
} else {
res = 0;
}

> #address-cells = <1>;
> #size-cells = <0>;
>
> - port@1 {
> + ethernet-port@1 {

or "port@.*", or "port-sfp@a", for the same reason. Here and everywhere
in this device tree. Basically only the ethernet-phy rename seems safe.

> reg = <0x1>;
> label = "lan1";
> phy-handle = <&switch0phy1>;
> };
>
> - port@2 {
> + ethernet-port@2 {
> reg = <0x2>;
> label = "lan2";
> phy-handle = <&switch0phy2>;
> };
>
> - port@3 {
> + ethernet-port@3 {
> reg = <0x3>;
> label = "lan3";
> phy-handle = <&switch0phy3>;
> };
>
> - port@4 {
> + ethernet-port@4 {
> reg = <0x4>;
> label = "lan4";
> phy-handle = <&switch0phy4>;
> };
>
> - port@5 {
> + ethernet-port@5 {
> reg = <0x5>;
> label = "lan5";
> phy-handle = <&switch0phy5>;
> };
>
> - port@6 {
> + ethernet-port@6 {
> reg = <0x6>;
> label = "lan6";
> phy-handle = <&switch0phy6>;
> };
>
> - port@7 {
> + ethernet-port@7 {
> reg = <0x7>;
> label = "lan7";
> phy-handle = <&switch0phy7>;
> };
>
> - port@8 {
> + ethernet-port@8 {
> reg = <0x8>;
> label = "lan8";
> phy-handle = <&switch0phy8>;
> };
>
> - port@9 {
> + ethernet-port@9 {
> reg = <0x9>;
> label = "cpu";
> ethernet = <&eth1>;
> @@ -410,7 +415,7 @@ port@9 {
> managed = "in-band-status";
> };
>
> - switch0port10: port@a {
> + switch0port10: ethernet-port@a {
> reg = <0xa>;
> label = "dsa";
> phy-mode = "2500base-x";
> @@ -430,7 +435,7 @@ port-sfp@a {
> };
> };
>
> - switch0@2 {
> + ethernet-switch@2 {

It's funny that you add a comment TO NOT rename switch nodes, then you
proceed to do just that.

Having that said, we need to suppress these warnings for the Marvell
schema only:

arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dtb: switch0@10: $nodename:0: 'switch0@10' does not match '^(ethernet-)?switch(@.*)?$'
from schema $id: http://devicetree.org/schemas/net/dsa/marvell,mv88e6xxx.yaml#
arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dtb: ethernet-switch@12: ethernet-ports: 'port-sfp@a' does not match any of the regexes: '^(ethernet-)?port@[0-9]+$', 'pinctrl-[0-9]+'
from schema $id: http://devicetree.org/schemas/net/dsa/marvell,mv88e6xxx.yaml#

because someone _will_ fix them and break the boot in the process.

Rob, Krzysztof, Conor, do you have any suggestion on how to achieve that?

> compatible = "marvell,mv88e6085";
> reg = <0x2>;
> dsa,member = <0 0>;
> diff --git a/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts b/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts
> index 48202810bf78..40b7ee7ead72 100644
> --- a/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts
> @@ -301,10 +301,8 @@ eth2phy: ethernet-phy@1 {
> };
>
> /* 88E6141 Topaz switch */
> - switch: switch@3 {
> + switch: ethernet-switch@3 {
> compatible = "marvell,mv88e6085";
> - #address-cells = <1>;
> - #size-cells = <0>;
> reg = <3>;
>
> pinctrl-names = "default";
> @@ -314,35 +312,35 @@ switch: switch@3 {
> interrupt-parent = <&cp0_gpio1>;
> interrupts = <1 IRQ_TYPE_LEVEL_LOW>;
>
> - ports {
> + ethernet-ports {
> #address-cells = <1>;
> #size-cells = <0>;
>
> - swport1: port@1 {
> + swport1: ethernet-port@1 {
> reg = <1>;
> label = "lan0";
> phy-handle = <&swphy1>;
> };
>
> - swport2: port@2 {
> + swport2: ethernet-port@2 {
> reg = <2>;
> label = "lan1";
> phy-handle = <&swphy2>;
> };
>
> - swport3: port@3 {
> + swport3: ethernet-port@3 {
> reg = <3>;
> label = "lan2";
> phy-handle = <&swphy3>;
> };
>
> - swport4: port@4 {
> + swport4: ethernet-port@4 {
> reg = <4>;
> label = "lan3";
> phy-handle = <&swphy4>;
> };
>
> - port@5 {
> + ethernet-port@5 {
> reg = <5>;
> label = "cpu";
> ethernet = <&cp0_eth1>;
> @@ -355,19 +353,19 @@ mdio {
> #address-cells = <1>;
> #size-cells = <0>;
>
> - swphy1: swphy1@17 {
> + swphy1: ethernet-phy@17 {
> reg = <17>;
> };
>
> - swphy2: swphy2@18 {
> + swphy2: ethernet-phy@18 {
> reg = <18>;
> };
>
> - swphy3: swphy3@19 {
> + swphy3: ethernet-phy@19 {
> reg = <19>;
> };
>
> - swphy4: swphy4@20 {
> + swphy4: ethernet-phy@20 {
> reg = <20>;
> };
> };

Robert, would you mind ACKing the MOCHAbin change?

> diff --git a/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts b/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
> index 4125202028c8..67892f0d2863 100644
> --- a/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
> @@ -497,42 +497,42 @@ ge_phy: ethernet-phy@0 {
> reset-deassert-us = <10000>;
> };
>
> - switch0: switch0@4 {
> + switch0: ethernet-switch@4 {
> compatible = "marvell,mv88e6085";
> reg = <4>;
> pinctrl-names = "default";
> pinctrl-0 = <&cp1_switch_reset_pins>;
> reset-gpios = <&cp1_gpio1 24 GPIO_ACTIVE_LOW>;
>
> - ports {
> + ethernet-ports {
> #address-cells = <1>;
> #size-cells = <0>;
>
> - port@1 {
> + ethernet-port@1 {
> reg = <1>;
> label = "lan2";
> phy-handle = <&switch0phy0>;
> };
>
> - port@2 {
> + ethernet-port@2 {
> reg = <2>;
> label = "lan1";
> phy-handle = <&switch0phy1>;
> };
>
> - port@3 {
> + ethernet-port@3 {
> reg = <3>;
> label = "lan4";
> phy-handle = <&switch0phy2>;
> };
>
> - port@4 {
> + ethernet-port@4 {
> reg = <4>;
> label = "lan3";
> phy-handle = <&switch0phy3>;
> };
>
> - port@5 {
> + ethernet-port@5 {
> reg = <5>;
> label = "cpu";
> ethernet = <&cp1_eth2>;
> @@ -545,19 +545,19 @@ mdio {
> #address-cells = <1>;
> #size-cells = <0>;
>
> - switch0phy0: switch0phy0@11 {
> + switch0phy0: ethernet-phy@11 {
> reg = <0x11>;
> };
>
> - switch0phy1: switch0phy1@12 {
> + switch0phy1: ethernet-phy@12 {
> reg = <0x12>;
> };
>
> - switch0phy2: switch0phy2@13 {
> + switch0phy2: ethernet-phy@13 {
> reg = <0x13>;
> };
>
> - switch0phy3: switch0phy3@14 {
> + switch0phy3: ethernet-phy@14 {
> reg = <0x14>;
> };
> };

Russell, could you please do the same for this device tree?

> 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?

Next message: H. Peter Anvin: "Re: [PATCH v2 1/6] x86/bugs: Add asm helpers for executing VERW"
Previous message: Rafael J. Wysocki: "Re: [RFC PATCH v3 00/39] ACPI/arm64: add support for virtual cpuhotplug"
In reply to: Florian Fainelli: "Re: [PATCH net-next v7 5/7] ARM64: dts: marvell: Fix some common switch mistakes"
Next in thread: Russell King (Oracle): "Re: [PATCH net-next v7 5/7] ARM64: dts: marvell: Fix some common switch mistakes"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]