[PATCH net-next v7 5/7] ARM64: dts: marvell: Fix some common switch mistakes
Vladimir Oltean
olteanv at gmail.com
Tue Oct 24 11:28:42 PDT 2023
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 at 0 is not OK, should be ethernet-switch at 0.
> - ports should be ethernet-ports
> - port at 0 should be ethernet-port at 0
> - PHYs should be named ethernet-phy@
>
> Reviewed-by: Andrew Lunn <andrew at lunn.ch>
> Signed-off-by: Linus Walleij <linus.walleij at linaro.org>
> ---
> .../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 at 1 {
> + ethernet-ports {
> + switch0port1: ethernet-port at 1 {
> reg = <1>;
> label = "lan0";
> phy-handle = <&switch0phy0>;
> };
>
> - switch0port2: port at 2 {
> + switch0port2: ethernet-port at 2 {
> reg = <2>;
> label = "lan1";
> phy-handle = <&switch0phy1>;
> };
>
> - switch0port3: port at 3 {
> + switch0port3: ethernet-port at 3 {
> reg = <3>;
> label = "lan2";
> phy-handle = <&switch0phy2>;
> };
>
> - switch0port4: port at 4 {
> + switch0port4: ethernet-port at 4 {
> reg = <4>;
> label = "lan3";
> phy-handle = <&switch0phy3>;
> };
>
> - switch0port5: port at 5 {
> + switch0port5: ethernet-port at 5 {
> reg = <5>;
> label = "wan";
> phy-handle = <&extphy>;
> @@ -160,7 +160,7 @@ switch0port5: port at 5 {
> };
>
> mdio {
> - switch0phy3: switch0phy3 at 14 {
> + switch0phy3: ethernet-phy at 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 at 1 {
> + switch0: ethernet-switch at 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 at 0 {
> + switch0port0: ethernet-port at 0 {
> reg = <0>;
> label = "cpu";
> ethernet = <ð0>;
> @@ -168,19 +166,19 @@ fixed-link {
> };
> };
>
> - switch0port1: port at 1 {
> + switch0port1: ethernet-port at 1 {
> reg = <1>;
> label = "wan";
> phy-handle = <&switch0phy0>;
> };
>
> - switch0port2: port at 2 {
> + switch0port2: ethernet-port at 2 {
> reg = <2>;
> label = "lan0";
> phy-handle = <&switch0phy1>;
> };
>
> - switch0port3: port at 3 {
> + switch0port3: ethernet-port at 3 {
> reg = <3>;
> label = "lan1";
> phy-handle = <&switch0phy2>;
> @@ -192,13 +190,13 @@ mdio {
> #address-cells = <1>;
> #size-cells = <0>;
>
> - switch0phy0: switch0phy0 at 11 {
> + switch0phy0: ethernet-phy at 11 {
> reg = <0x11>;
> };
> - switch0phy1: switch0phy1 at 12 {
> + switch0phy1: ethernet-phy at 12 {
> reg = <0x12>;
> };
> - switch0phy2: switch0phy2 at 13 {
> + switch0phy2: ethernet-phy at 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 at 1 {
> + switch0: ethernet-switch at 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 at 0 {
> + ethernet-port at 0 {
> reg = <0>;
> label = "cpu";
> ethernet = <ð0>;
> };
>
> - port at 1 {
> + ethernet-port at 1 {
> reg = <1>;
> label = "wan";
> phy-handle = <&switch0phy0>;
> };
>
> - port at 2 {
> + ethernet-port at 2 {
> reg = <2>;
> label = "lan0";
> phy-handle = <&switch0phy1>;
> @@ -185,7 +183,7 @@ port at 2 {
> nvmem-cell-names = "mac-address";
> };
>
> - port at 3 {
> + ethernet-port at 3 {
> reg = <3>;
> label = "lan1";
> phy-handle = <&switch0phy2>;
> @@ -199,13 +197,13 @@ mdio {
> #address-cells = <1>;
> #size-cells = <0>;
>
> - switch0phy0: switch0phy0 at 11 {
> + switch0phy0: ethernet-phy at 11 {
> reg = <0x11>;
> };
> - switch0phy1: switch0phy1 at 12 {
> + switch0phy1: ethernet-phy at 12 {
> reg = <0x12>;
> };
> - switch0phy2: switch0phy2 at 13 {
> + switch0phy2: ethernet-phy at 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 at 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 at 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 at 10 {
> compatible = "marvell,mv88e6190";
> reg = <0x10>;
> @@ -317,92 +322,92 @@ mdio {
> #address-cells = <1>;
> #size-cells = <0>;
>
> - switch0phy1: switch0phy1 at 1 {
> + switch0phy1: ethernet-phy at 1 {
> reg = <0x1>;
> };
>
> - switch0phy2: switch0phy2 at 2 {
> + switch0phy2: ethernet-phy at 2 {
> reg = <0x2>;
> };
>
> - switch0phy3: switch0phy3 at 3 {
> + switch0phy3: ethernet-phy at 3 {
> reg = <0x3>;
> };
>
> - switch0phy4: switch0phy4 at 4 {
> + switch0phy4: ethernet-phy at 4 {
> reg = <0x4>;
> };
>
> - switch0phy5: switch0phy5 at 5 {
> + switch0phy5: ethernet-phy at 5 {
> reg = <0x5>;
> };
>
> - switch0phy6: switch0phy6 at 6 {
> + switch0phy6: ethernet-phy at 6 {
> reg = <0x6>;
> };
>
> - switch0phy7: switch0phy7 at 7 {
> + switch0phy7: ethernet-phy at 7 {
> reg = <0x7>;
> };
>
> - switch0phy8: switch0phy8 at 8 {
> + switch0phy8: ethernet-phy at 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 at 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 at a",
mdio_path, id, addr);
} else {
res = 0;
}
> #address-cells = <1>;
> #size-cells = <0>;
>
> - port at 1 {
> + ethernet-port at 1 {
or "port at .*", or "port-sfp at 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 at 2 {
> + ethernet-port at 2 {
> reg = <0x2>;
> label = "lan2";
> phy-handle = <&switch0phy2>;
> };
>
> - port at 3 {
> + ethernet-port at 3 {
> reg = <0x3>;
> label = "lan3";
> phy-handle = <&switch0phy3>;
> };
>
> - port at 4 {
> + ethernet-port at 4 {
> reg = <0x4>;
> label = "lan4";
> phy-handle = <&switch0phy4>;
> };
>
> - port at 5 {
> + ethernet-port at 5 {
> reg = <0x5>;
> label = "lan5";
> phy-handle = <&switch0phy5>;
> };
>
> - port at 6 {
> + ethernet-port at 6 {
> reg = <0x6>;
> label = "lan6";
> phy-handle = <&switch0phy6>;
> };
>
> - port at 7 {
> + ethernet-port at 7 {
> reg = <0x7>;
> label = "lan7";
> phy-handle = <&switch0phy7>;
> };
>
> - port at 8 {
> + ethernet-port at 8 {
> reg = <0x8>;
> label = "lan8";
> phy-handle = <&switch0phy8>;
> };
>
> - port at 9 {
> + ethernet-port at 9 {
> reg = <0x9>;
> label = "cpu";
> ethernet = <ð1>;
> @@ -410,7 +415,7 @@ port at 9 {
> managed = "in-band-status";
> };
>
> - switch0port10: port at a {
> + switch0port10: ethernet-port at a {
> reg = <0xa>;
> label = "dsa";
> phy-mode = "2500base-x";
> @@ -430,7 +435,7 @@ port-sfp at a {
> };
> };
>
> - switch0 at 2 {
> + ethernet-switch at 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 at 10: $nodename:0: 'switch0 at 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 at 12: ethernet-ports: 'port-sfp at 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 at 1 {
> };
>
> /* 88E6141 Topaz switch */
> - switch: switch at 3 {
> + switch: ethernet-switch at 3 {
> compatible = "marvell,mv88e6085";
> - #address-cells = <1>;
> - #size-cells = <0>;
> reg = <3>;
>
> pinctrl-names = "default";
> @@ -314,35 +312,35 @@ switch: switch at 3 {
> interrupt-parent = <&cp0_gpio1>;
> interrupts = <1 IRQ_TYPE_LEVEL_LOW>;
>
> - ports {
> + ethernet-ports {
> #address-cells = <1>;
> #size-cells = <0>;
>
> - swport1: port at 1 {
> + swport1: ethernet-port at 1 {
> reg = <1>;
> label = "lan0";
> phy-handle = <&swphy1>;
> };
>
> - swport2: port at 2 {
> + swport2: ethernet-port at 2 {
> reg = <2>;
> label = "lan1";
> phy-handle = <&swphy2>;
> };
>
> - swport3: port at 3 {
> + swport3: ethernet-port at 3 {
> reg = <3>;
> label = "lan2";
> phy-handle = <&swphy3>;
> };
>
> - swport4: port at 4 {
> + swport4: ethernet-port at 4 {
> reg = <4>;
> label = "lan3";
> phy-handle = <&swphy4>;
> };
>
> - port at 5 {
> + ethernet-port at 5 {
> reg = <5>;
> label = "cpu";
> ethernet = <&cp0_eth1>;
> @@ -355,19 +353,19 @@ mdio {
> #address-cells = <1>;
> #size-cells = <0>;
>
> - swphy1: swphy1 at 17 {
> + swphy1: ethernet-phy at 17 {
> reg = <17>;
> };
>
> - swphy2: swphy2 at 18 {
> + swphy2: ethernet-phy at 18 {
> reg = <18>;
> };
>
> - swphy3: swphy3 at 19 {
> + swphy3: ethernet-phy at 19 {
> reg = <19>;
> };
>
> - swphy4: swphy4 at 20 {
> + swphy4: ethernet-phy at 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 at 0 {
> reset-deassert-us = <10000>;
> };
>
> - switch0: switch0 at 4 {
> + switch0: ethernet-switch at 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 at 1 {
> + ethernet-port at 1 {
> reg = <1>;
> label = "lan2";
> phy-handle = <&switch0phy0>;
> };
>
> - port at 2 {
> + ethernet-port at 2 {
> reg = <2>;
> label = "lan1";
> phy-handle = <&switch0phy1>;
> };
>
> - port at 3 {
> + ethernet-port at 3 {
> reg = <3>;
> label = "lan4";
> phy-handle = <&switch0phy2>;
> };
>
> - port at 4 {
> + ethernet-port at 4 {
> reg = <4>;
> label = "lan3";
> phy-handle = <&switch0phy3>;
> };
>
> - port at 5 {
> + ethernet-port at 5 {
> reg = <5>;
> label = "cpu";
> ethernet = <&cp1_eth2>;
> @@ -545,19 +545,19 @@ mdio {
> #address-cells = <1>;
> #size-cells = <0>;
>
> - switch0phy0: switch0phy0 at 11 {
> + switch0phy0: ethernet-phy at 11 {
> reg = <0x11>;
> };
>
> - switch0phy1: switch0phy1 at 12 {
> + switch0phy1: ethernet-phy at 12 {
> reg = <0x12>;
> };
>
> - switch0phy2: switch0phy2 at 13 {
> + switch0phy2: ethernet-phy at 13 {
> reg = <0x13>;
> };
>
> - switch0phy3: switch0phy3 at 14 {
> + switch0phy3: ethernet-phy at 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 at 0 {
> reg = <0>;
> };
>
> - switch6: switch0 at 6 {
> + switch6: ethernet-switch at 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 at 6 {
>
> dsa,member = <0 0>;
>
> - ports {
> + ethernet-ports {
> #address-cells = <1>;
> #size-cells = <0>;
>
> - port at 1 {
> + ethernet-port at 1 {
> reg = <1>;
> label = "p1";
> phy-handle = <&switch0phy1>;
> };
>
> - port at 2 {
> + ethernet-port at 2 {
> reg = <2>;
> label = "p2";
> phy-handle = <&switch0phy2>;
> };
>
> - port at 3 {
> + ethernet-port at 3 {
> reg = <3>;
> label = "p3";
> phy-handle = <&switch0phy3>;
> };
>
> - port at 4 {
> + ethernet-port at 4 {
> reg = <4>;
> label = "p4";
> phy-handle = <&switch0phy4>;
> };
>
> - port at 5 {
> + ethernet-port at 5 {
> reg = <5>;
> label = "p5";
> phy-handle = <&switch0phy5>;
> };
>
> - port at 6 {
> + ethernet-port at 6 {
> reg = <6>;
> label = "p6";
> phy-handle = <&switch0phy6>;
> };
>
> - port at 7 {
> + ethernet-port at 7 {
> reg = <7>;
> label = "p7";
> phy-handle = <&switch0phy7>;
> };
>
> - port at 8 {
> + ethernet-port at 8 {
> reg = <8>;
> label = "p8";
> phy-handle = <&switch0phy8>;
> };
>
> - port at 9 {
> + ethernet-port at 9 {
> reg = <9>;
> label = "p9";
> phy-mode = "10gbase-r";
> @@ -280,7 +278,7 @@ port at 9 {
> managed = "in-band-status";
> };
>
> - port at a {
> + ethernet-port at a {
> reg = <10>;
> ethernet = <&cp0_eth0>;
> phy-mode = "10gbase-r";
> @@ -293,35 +291,35 @@ mdio {
> #address-cells = <1>;
> #size-cells = <0>;
>
> - switch0phy1: switch0phy1 at 1 {
> + switch0phy1: ethernet-phy at 1 {
> reg = <0x1>;
> };
>
> - switch0phy2: switch0phy2 at 2 {
> + switch0phy2: ethernet-phy at 2 {
> reg = <0x2>;
> };
>
> - switch0phy3: switch0phy3 at 3 {
> + switch0phy3: ethernet-phy at 3 {
> reg = <0x3>;
> };
>
> - switch0phy4: switch0phy4 at 4 {
> + switch0phy4: ethernet-phy at 4 {
> reg = <0x4>;
> };
>
> - switch0phy5: switch0phy5 at 5 {
> + switch0phy5: ethernet-phy at 5 {
> reg = <0x5>;
> };
>
> - switch0phy6: switch0phy6 at 6 {
> + switch0phy6: ethernet-phy at 6 {
> reg = <0x6>;
> };
>
> - switch0phy7: switch0phy7 at 7 {
> + switch0phy7: ethernet-phy at 7 {
> reg = <0x7>;
> };
>
> - switch0phy8: switch0phy8 at 8 {
> + switch0phy8: ethernet-phy at 8 {
> reg = <0x8>;
> };
> };
Chris, does this look okay?
More information about the linux-arm-kernel
mailing list