| Message ID | 20231018-marvell-88e6152-wan-led-v4-0-3ee0c67383be@linaro.org |
|---|---|
| Headers | show |
| Series | Create a binding for the Marvell MV88E6xxx DSA switches | expand |
On Wed, Oct 18, 2023 at 11:03:42AM +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 switch@0. > - The ports node should have port@0 etc children, no > plural "ports". > > This serves as an example of fixes needed for introducing a > schema for the bindings, but the patch can simply be applied. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Wed, Oct 18, 2023 at 11:03:43AM +0200, Linus Walleij wrote: > Fix some errors in the Marvell MV88E6xxx switch descriptions: > - switch0@0 is not OK, should be switch@0 > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Wed, Oct 18, 2023 at 11:03:44AM +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 switch@0. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Wed, Oct 18, 2023 at 11:03:42AM +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 switch@0. > - The ports node should have port@0 etc children, no > plural "ports". > > This serves as an example of fixes needed for introducing a > schema for the bindings, but the patch can simply be applied. In patch 2, you mention that things should be named ethernet-switch and ethernet-port. As you're renaming the nodes in this patch, wouldn't it make sense to use those names instead now, rather than at some point in the future a patch that converts to these names? Thanks.
On Thu, Oct 19, 2023 at 12:04:46PM +0100, Russell King (Oracle) wrote: > On Wed, Oct 18, 2023 at 11:03:42AM +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 switch@0. > > - The ports node should have port@0 etc children, no > > plural "ports". > > > > This serves as an example of fixes needed for introducing a > > schema for the bindings, but the patch can simply be applied. > > In patch 2, you mention that things should be named ethernet-switch and > ethernet-port. As you're renaming the nodes in this patch, wouldn't it > make sense to use those names instead now, rather than at some point in > the future a patch that converts to these names? I agree, and I was wondering the same thing.
On Wed, Oct 18, 2023 at 11:03:43AM +0200, Linus Walleij wrote: > Fix some errors in the Marvell MV88E6xxx switch descriptions: > - switch0@0 is not OK, should be switch@0 > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- Same comment as on patch 3, prefer ethernet-switch and ethernet-ports.
+Marek On Wed, Oct 18, 2023 at 11:03:44AM +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 switch@0. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > 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..c69cb4e191e5 100644 > --- a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts > +++ b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts > @@ -305,7 +305,7 @@ phy1: ethernet-phy@1 { > }; > > /* switch nodes are enabled by U-Boot if modules are present */ > - switch0@10 { > + switch@10 { As the comment says: U-Boot (https://elixir.bootlin.com/u-boot/latest/source/board/CZ.NIC/turris_mox/turris_mox.c#L728) sets up status = "okay" for these nodes depending on the MOXTET configuration. It doesn't look as if it's doing that by alias, just by path ("%s/switch%i@%x"). I have a Turris MOX, please allow me some time to test if the node name change is going to be significant and cause regressions. I expect the answer to be yes (sadly). > compatible = "marvell,mv88e6190"; > reg = <0x10>; > dsa,member = <0 0>; > @@ -430,7 +430,7 @@ port-sfp@a { > }; > }; > > - switch0@2 { > + switch@2 { > compatible = "marvell,mv88e6085"; > reg = <0x2>; > dsa,member = <0 0>; > @@ -497,7 +497,7 @@ port@5 { > }; > }; > > - switch1@11 { > + switch@11 { > compatible = "marvell,mv88e6190"; > reg = <0x11>; > dsa,member = <0 1>; > @@ -622,7 +622,7 @@ port-sfp@a { > }; > }; > > - switch1@2 { > + switch@2 { > compatible = "marvell,mv88e6085"; > reg = <0x2>; > dsa,member = <0 1>; > @@ -689,7 +689,7 @@ port@5 { > }; > }; > > - switch2@12 { > + switch@12 { > compatible = "marvell,mv88e6190"; > reg = <0x12>; > dsa,member = <0 2>; > @@ -805,7 +805,7 @@ port-sfp@a { > }; > }; > > - switch2@2 { > + switch@2 { > compatible = "marvell,mv88e6085"; > reg = <0x2>; > dsa,member = <0 2>;
On Thu, Oct 19, 2023 at 05:40:22PM +0300, Vladimir Oltean wrote: > +Marek > > On Wed, Oct 18, 2023 at 11:03:44AM +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 switch@0. > > > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > > --- > > 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..c69cb4e191e5 100644 > > --- a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts > > +++ b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts > > @@ -305,7 +305,7 @@ phy1: ethernet-phy@1 { > > }; > > > > /* switch nodes are enabled by U-Boot if modules are present */ > > - switch0@10 { > > + switch@10 { > > As the comment says: U-Boot > (https://elixir.bootlin.com/u-boot/latest/source/board/CZ.NIC/turris_mox/turris_mox.c#L728) > sets up status = "okay" for these nodes depending on the MOXTET > configuration. It doesn't look as if it's doing that by alias, just by > path ("%s/switch%i@%x"). > > I have a Turris MOX, please allow me some time to test if the node name > change is going to be significant and cause regressions. I expect the > answer to be yes (sadly). Yeah, it's bad. U-Boot 2018.11 (Dec 16 2018 - 12:50:19 +0000), Build: jenkins-turris-os-packages-kittens-mox-90 DRAM: 1 GiB Enabling Armada 3720 wComphy-0: SGMII1 3.125 Gbps Comphy-1: PEX0 5 Gbps Comphy-2: USB3_HOST0 5 Gbps MMC: sdhci@d8000: 0 Loading Environment from SPI Flash... SF: Detected w25q64dw with page size 256 Bytes, erase size 4 KiB, total 8 MiB OK Model: CZ.NIC Turris Mox Board Net: eth0: neta@30000 Turris Mox: Board version: 22 RAM size: 1024 MiB SD/eMMC version: SD Module Topology: 1: Peridot Switch Module (8-port) 2: Peridot Switch Module (8-port) 3: Peridot Switch Module (8-port) 4: SFP Module Hit any key to stop autoboot: 0 => run sd_tftp_boot neta@30000 Waiting for PHY auto negotiation to complete....... done BOOTP broadcast 1 BOOTP broadcast 2 DHCP client bound to address 10.0.0.117 (254 ms) Using neta@30000 device TFTP from server 10.0.0.1; our IP address is 10.0.0.117 Filename 'mox/armada-3720-turris-mox.dtb'. Load address: 0x4f00000 Loading: #### 1.5 MiB/s done Bytes transferred = 19479 (4c17 hex) Using neta@30000 device TFTP from server 10.0.0.1; our IP address is 10.0.0.117 Filename 'mox/Image'. Load address: 0x5000000 Loading: ################################################################# ########################################## 6 MiB/s done Bytes transferred = 54069760 (3390a00 hex) ## Flattened Device Tree blob at 04f00000 Booting using the fdt blob at 0x4f00000 Loading Device Tree to 000000003bf16000, end 000000003bf1dc16 ... OK ERROR: board-specific fdt fixup failed: FDT_ERR_NOTFOUND - must RESET the board to recover. FDT creation failed! hanging...### ERROR ### Please RESET the board ###
On Thu, 19 Oct 2023 17:49:35 +0300 Vladimir Oltean <olteanv@gmail.com> wrote: > On Thu, Oct 19, 2023 at 05:40:22PM +0300, Vladimir Oltean wrote: > > +Marek > > > > On Wed, Oct 18, 2023 at 11:03:44AM +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 switch@0. > > > > > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > > > --- > > > 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..c69cb4e191e5 100644 > > > --- a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts > > > +++ b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts > > > @@ -305,7 +305,7 @@ phy1: ethernet-phy@1 { > > > }; > > > > > > /* switch nodes are enabled by U-Boot if modules are present */ > > > - switch0@10 { > > > + switch@10 { > > > > As the comment says: U-Boot > > (https://elixir.bootlin.com/u-boot/latest/source/board/CZ.NIC/turris_mox/turris_mox.c#L728) > > sets up status = "okay" for these nodes depending on the MOXTET > > configuration. It doesn't look as if it's doing that by alias, just by > > path ("%s/switch%i@%x"). > > > > I have a Turris MOX, please allow me some time to test if the node name > > change is going to be significant and cause regressions. I expect the > > answer to be yes (sadly). > > Yeah, it's bad. > > U-Boot 2018.11 (Dec 16 2018 - 12:50:19 +0000), Build: jenkins-turris-os-packages-kittens-mox-90 > > DRAM: 1 GiB > Enabling Armada 3720 wComphy-0: SGMII1 3.125 Gbps > Comphy-1: PEX0 5 Gbps > Comphy-2: USB3_HOST0 5 Gbps > MMC: sdhci@d8000: 0 > Loading Environment from SPI Flash... SF: Detected w25q64dw with page size 256 Bytes, erase size 4 KiB, total 8 MiB > OK > Model: CZ.NIC Turris Mox Board > Net: eth0: neta@30000 > Turris Mox: > Board version: 22 > RAM size: 1024 MiB > SD/eMMC version: SD > Module Topology: > 1: Peridot Switch Module (8-port) > 2: Peridot Switch Module (8-port) > 3: Peridot Switch Module (8-port) > 4: SFP Module > > Hit any key to stop autoboot: 0 > => run sd_tftp_boot > neta@30000 Waiting for PHY auto negotiation to complete....... done > BOOTP broadcast 1 > BOOTP broadcast 2 > DHCP client bound to address 10.0.0.117 (254 ms) > Using neta@30000 device > TFTP from server 10.0.0.1; our IP address is 10.0.0.117 > Filename 'mox/armada-3720-turris-mox.dtb'. > Load address: 0x4f00000 > Loading: #### > 1.5 MiB/s > done > Bytes transferred = 19479 (4c17 hex) > Using neta@30000 device > TFTP from server 10.0.0.1; our IP address is 10.0.0.117 > Filename 'mox/Image'. > Load address: 0x5000000 > Loading: ################################################################# > ########################################## > 6 MiB/s > done > Bytes transferred = 54069760 (3390a00 hex) > ## Flattened Device Tree blob at 04f00000 > Booting using the fdt blob at 0x4f00000 > Loading Device Tree to 000000003bf16000, end 000000003bf1dc16 ... OK > ERROR: board-specific fdt fixup failed: FDT_ERR_NOTFOUND > - must RESET the board to recover. > > FDT creation failed! hanging...### ERROR ### Please RESET the board ### Yes, unfortunately changing that node name will break booting. Maybe we could add a comment into the DTS to describe this unfortunate state of things? :) Marek
On Thu, Oct 19, 2023 at 05:26:49PM +0200, Marek Behún wrote: > Yes, unfortunately changing that node name will break booting. > > Maybe we could add a comment into the DTS to describe this unfortunate > state of things? :) Well, the fact that Linus didn't notice means that there are insufficient signals currently, so I guess a more explicit comment would help. Could you prepare a patch?
On Thu, Oct 19, 2023 at 6:22 PM Vladimir Oltean <olteanv@gmail.com> wrote: > On Thu, Oct 19, 2023 at 05:26:49PM +0200, Marek Behún wrote: > > Yes, unfortunately changing that node name will break booting. > > > > Maybe we could add a comment into the DTS to describe this unfortunate > > state of things? :) > > Well, the fact that Linus didn't notice means that there are insufficient > signals currently, so I guess a more explicit comment would help. Could > you prepare a patch? I can just include a blurb in my patch so we don't get colliding changes. Yours, Linus Walleij
On Fri, Oct 20, 2023 at 02:59:43PM +0200, Linus Walleij wrote: > On Thu, Oct 19, 2023 at 6:22 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > On Thu, Oct 19, 2023 at 05:26:49PM +0200, Marek Behún wrote: > > > Yes, unfortunately changing that node name will break booting. > > > > > > Maybe we could add a comment into the DTS to describe this unfortunate > > > state of things? :) > > > > Well, the fact that Linus didn't notice means that there are insufficient > > signals currently, so I guess a more explicit comment would help. Could > > you prepare a patch? > > I can just include a blurb in my patch so we don't get colliding > changes. Colliding with what, if you shouldn't change the node names?
The Marvell switches are lacking DT bindings. I need proper schema checking to add LED support to the Marvell switch. Just how it is, it can't go on like this. Some Device Tree fixes are included in the series, these remove the major and most annoying warnings fallout noise: some warnings remain, and these are of more serious nature, such as missing phy-mode. They can be applied individually, or to the networking tree with the rest of the patches. This series requires Rob Herrings series "dt-bindings: net: Child node schema cleanups" to be applied first. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- Changes in v4: - Rebase the series on top of Rob's series "dt-bindings: net: Child node schema cleanups" (or the hex numbered ports will not work) - Fix up a whitespacing error corrupting v3... - Add a new patch making the generic DSA binding require ports or ethernet-ports in the switch node. - Drop any corrections of port@a in the patches. - Drop oneOf in the compatible enum for mv88e6xxx - Use ethernet-switch, ethernet-ports and ethernet-phy in the examples - Transclude the dsa.yaml#/$defs/ethernet-ports define for ports - Move the DTS and binding fixes first, before the actual bindings, so they apply without (too many) warnings as fallout. - Drop stray colon in text. - Drop example port in the mveusb binding. - Link to v3: https://lore.kernel.org/r/20231016-marvell-88e6152-wan-led-v3-0-38cd449dfb15@linaro.org Changes in v3: - Fix up a related mvusb example in a different binding that the scripts were complaining about. - Fix up the wording on internal vs external MDIO buses in the mv88e6xxx binding document. - Remove pointless label and put the right rev-mii into the MV88E6060 schema. - Link to v2: https://lore.kernel.org/r/20231014-marvell-88e6152-wan-led-v2-0-7fca08b68849@linaro.org Changes in v2: - Break out a separate Marvell MV88E6060 binding file. I stand corrected. - Drop the idea to rely on nodename mdio-external for the external MDIO bus, keep the compatible, drop patch for the driver. - Fix more Marvell DT mistakes. - Fix NXP DT mistakes in a separate patch. - Fix Marvell ARM64 mistakes in a separate patch. - Link to v1: https://lore.kernel.org/r/20231013-marvell-88e6152-wan-led-v1-0-0712ba99857c@linaro.org --- Linus Walleij (7): dt-bindings: net: dsa: Require ports or ethernet-ports dt-bindings: net: mvusb: Fix up DSA example ARM: dts: marvell: Fix some common switch mistakes ARM: dts: nxp: Fix some common switch mistakes ARM64: dts: marvell: Fix some common switch mistakes dt-bindings: marvell: Rewrite MV88E6xxx in schema dt-bindings: marvell: Add Marvell MV88E6060 DSA schema Documentation/devicetree/bindings/net/dsa/dsa.yaml | 6 + .../bindings/net/dsa/marvell,mv88e6060.yaml | 90 +++++++++ .../bindings/net/dsa/marvell,mv88e6xxx.yaml | 225 +++++++++++++++++++++ .../devicetree/bindings/net/dsa/marvell.txt | 109 ---------- .../devicetree/bindings/net/marvell,mvusb.yaml | 7 +- MAINTAINERS | 3 +- arch/arm/boot/dts/marvell/armada-370-rd.dts | 2 - .../dts/marvell/armada-381-netgear-gs110emx.dts | 2 - .../dts/marvell/armada-385-clearfog-gtr-l8.dts | 2 +- .../dts/marvell/armada-385-clearfog-gtr-s4.dts | 2 +- arch/arm/boot/dts/marvell/armada-385-linksys.dtsi | 2 - .../boot/dts/marvell/armada-385-turris-omnia.dts | 16 +- arch/arm/boot/dts/marvell/armada-388-clearfog.dts | 2 - .../boot/dts/marvell/armada-xp-linksys-mamba.dts | 2 - arch/arm/boot/dts/nxp/vf/vf610-zii-cfu1.dts | 2 +- arch/arm/boot/dts/nxp/vf/vf610-zii-scu4-aib.dts | 8 +- arch/arm/boot/dts/nxp/vf/vf610-zii-spb4.dts | 2 +- arch/arm/boot/dts/nxp/vf/vf610-zii-ssmb-dtu.dts | 4 +- arch/arm/boot/dts/nxp/vf/vf610-zii-ssmb-spu3.dts | 2 +- .../boot/dts/marvell/armada-3720-espressobin.dtsi | 4 +- .../boot/dts/marvell/armada-3720-gl-mv1000.dts | 4 +- .../boot/dts/marvell/armada-3720-turris-mox.dts | 12 +- .../boot/dts/marvell/armada-7040-mochabin.dts | 2 - .../dts/marvell/armada-8040-clearfog-gt-8k.dts | 2 +- arch/arm64/boot/dts/marvell/cn9130-crb.dtsi | 4 +- 25 files changed, 356 insertions(+), 160 deletions(-) --- base-commit: 1c9be5fea84e409542a186893d219bf7cff22f5a change-id: 20231008-marvell-88e6152-wan-led-88c43b7fd2fd Best regards,