| Message ID | 20231018-marvell-88e6152-wan-led-v4-6-3ee0c67383be@linaro.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Create a binding for the Marvell MV88E6xxx DSA switches | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 233 lines checked |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 233 lines checked |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Wed, 18 Oct 2023 11:03:45 +0200, Linus Walleij wrote: > This is an attempt to rewrite the Marvell MV88E6xxx switch bindings > in YAML schema. > > The current text binding says: > WARNING: This binding is currently unstable. Do not program it into a > FLASH never to be changed again. Once this binding is stable, this > warning will be removed. > > Well that never happened before we switched to YAML markup, > we can't have it like this, what about fixing the mess? > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > .../bindings/net/dsa/marvell,mv88e6xxx.yaml | 225 +++++++++++++++++++++ > .../devicetree/bindings/net/dsa/marvell.txt | 109 ---------- > MAINTAINERS | 2 +- > 3 files changed, 226 insertions(+), 110 deletions(-) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/marvell,mvusb.example.dtb: switch@0: ports: '#address-cells' is a required property from schema $id: http://devicetree.org/schemas/net/dsa/marvell,mv88e6xxx.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/marvell,mvusb.example.dtb: switch@0: ports: '#size-cells' is a required property from schema $id: http://devicetree.org/schemas/net/dsa/marvell,mv88e6xxx.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/marvell,mvusb.example.dtb: switch@0: ports: '#address-cells' is a required property from schema $id: http://devicetree.org/schemas/net/dsa/marvell,mv88e6xxx.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/marvell,mvusb.example.dtb: switch@0: ports: '#size-cells' is a required property from schema $id: http://devicetree.org/schemas/net/dsa/marvell,mv88e6xxx.yaml# doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231018-marvell-88e6152-wan-led-v4-6-3ee0c67383be@linaro.org The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Wed, Oct 18, 2023 at 12:32 PM Rob Herring <robh@kernel.org> wrote: > yamllint warnings/errors: > > dtschema/dtc warnings/errors: > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/marvell,mvusb.example.dtb: switch@0: ports: '#address-cells' is a required property > from schema $id: http://devicetree.org/schemas/net/dsa/marvell,mv88e6xxx.yaml# > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/marvell,mvusb.example.dtb: switch@0: ports: '#size-cells' is a required property > from schema $id: http://devicetree.org/schemas/net/dsa/marvell,mv88e6xxx.yaml# > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/marvell,mvusb.example.dtb: switch@0: ports: '#address-cells' is a required property > from schema $id: http://devicetree.org/schemas/net/dsa/marvell,mv88e6xxx.yaml# > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/marvell,mvusb.example.dtb: switch@0: ports: '#size-cells' is a required property > from schema $id: http://devicetree.org/schemas/net/dsa/marvell,mv88e6xxx.yaml# Fixed in patch 2/7? Yours, Linus Walleij
On Wed, Oct 18, 2023 at 11:03:45AM +0200, Linus Walleij wrote: > This is an attempt to rewrite the Marvell MV88E6xxx switch bindings > in YAML schema. > > The current text binding says: > WARNING: This binding is currently unstable. Do not program it into a > FLASH never to be changed again. Once this binding is stable, this > warning will be removed. > > Well that never happened before we switched to YAML markup, > we can't have it like this, what about fixing the mess? > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> For the text describing the properties: Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Wed, Oct 18, 2023 at 01:39:45PM +0200, Linus Walleij wrote: > On Wed, Oct 18, 2023 at 12:32 PM Rob Herring <robh@kernel.org> wrote: > > > yamllint warnings/errors: > > > > dtschema/dtc warnings/errors: > > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/marvell,mvusb.example.dtb: switch@0: ports: '#address-cells' is a required property > > from schema $id: http://devicetree.org/schemas/net/dsa/marvell,mv88e6xxx.yaml# > > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/marvell,mvusb.example.dtb: switch@0: ports: '#size-cells' is a required property > > from schema $id: http://devicetree.org/schemas/net/dsa/marvell,mv88e6xxx.yaml# > > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/marvell,mvusb.example.dtb: switch@0: ports: '#address-cells' is a required property > > from schema $id: http://devicetree.org/schemas/net/dsa/marvell,mv88e6xxx.yaml# > > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/marvell,mvusb.example.dtb: switch@0: ports: '#size-cells' is a required property > > from schema $id: http://devicetree.org/schemas/net/dsa/marvell,mv88e6xxx.yaml# > > Fixed in patch 2/7? Yes. If one patch has errors we drop it. I should probably just give up on the rest of the series instead. Rob
On Wed, Oct 18, 2023 at 11:03:45AM +0200, Linus Walleij wrote: > This is an attempt to rewrite the Marvell MV88E6xxx switch bindings > in YAML schema. > > The current text binding says: > WARNING: This binding is currently unstable. Do not program it into a > FLASH never to be changed again. Once this binding is stable, this > warning will be removed. > > Well that never happened before we switched to YAML markup, > we can't have it like this, what about fixing the mess? > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > .../bindings/net/dsa/marvell,mv88e6xxx.yaml | 225 +++++++++++++++++++++ > .../devicetree/bindings/net/dsa/marvell.txt | 109 ---------- > MAINTAINERS | 2 +- > 3 files changed, 226 insertions(+), 110 deletions(-) > > diff --git a/Documentation/devicetree/bindings/net/dsa/marvell,mv88e6xxx.yaml b/Documentation/devicetree/bindings/net/dsa/marvell,mv88e6xxx.yaml > new file mode 100644 > index 000000000000..8013ac411b15 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/dsa/marvell,mv88e6xxx.yaml > @@ -0,0 +1,225 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/net/dsa/marvell,mv88e6xxx.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Marvell MV88E6xxx DSA switch family > + > +maintainers: > + - Andrew Lunn <andrew@lunn.ch> > + > +description: > + The Marvell MV88E6xxx switch series has been produced and sold > + by Marvell since at least 2010. The switch has a few compatibles which > + just indicate the base address of the switch, then operating systems > + can investigate switch ID registers to find out which actual version > + of the switch it is dealing with. > + > +properties: > + compatible: > + enum: > + - marvell,mv88e6085 > + - marvell,mv88e6190 > + - marvell,mv88e6250 > + description: | > + marvell,mv88e6085: This switch uses base address 0x10. > + This switch and its siblings will be autodetected from > + ID registers found in the switch, so only "marvell,mv88e6085" should be > + specified. This includes the following list of MV88Exxxx switches: > + 6085, 6095, 6097, 6123, 6131, 6141, 6161, 6165, 6171, 6172, 6175, 6176, > + 6185, 6240, 6320, 6321, 6341, 6350, 6351, 6352 > + marvell,mv88e6190: This switch uses base address 0x00. > + This switch and its siblings will be autodetected from > + ID registers found in the switch, so only "marvell,mv88e6190" should be > + specified. This includes the following list of MV88Exxxx switches: > + 6190, 6190X, 6191, 6290, 6361, 6390, 6390X > + marvell,mv88e6250: This switch uses base address 0x08 or 0x18. > + This switch and its siblings will be autodetected from > + ID registers found in the switch, so only "marvell,mv88e6250" should be > + specified. This includes the following list of MV88Exxxx switches: > + 6220, 6250 > + > + reg: > + maxItems: 1 > + > + eeprom-length: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: Set to the length of an EEPROM connected to the switch. Must be > + set if the switch can not detect the presence and/or size of a connected > + EEPROM, otherwise optional. > + > + reset-gpios: > + description: > + GPIO to be used to reset the whole device > + maxItems: 1 > + > + interrupts: > + description: The switch provides an external interrupt line, but it is > + not always used by target systems. > + maxItems: 1 > + > + interrupt-controller: > + description: The switch has an internal interrupt controller used by > + the different sub-blocks. > + > + '#interrupt-cells': > + description: The internal interrupt controller only supports triggering > + on active high level interrupts so the second cell must alway be set to > + IRQ_TYPE_LEVEL_HIGH. > + const: 2 > + > + mdio: > + $ref: /schemas/net/mdio.yaml# > + unevaluatedProperties: false > + description: Marvell MV88E6xxx switches have an varying combination of > + internal and external MDIO buses, in some cases a combined bus that > + can be used both internally and externally. This node is for the > + primary bus, used internally and sometimes also externally. > + > + mdio-external: > + $ref: /schemas/net/mdio.yaml# > + unevaluatedProperties: false > + description: Marvell MV88E6xxx switches that have a separate external > + MDIO bus use this port to access external components on the MDIO bus. > + > + properties: > + compatible: > + const: marvell,mv88e6xxx-mdio-external > + > + required: > + - compatible > + > +$ref: dsa.yaml# Drop this. Covered by the line below. > + > +allOf: > + - $ref: dsa.yaml#/$defs/ethernet-ports
On Wed, Oct 18, 2023 at 11:03:45AM +0200, Linus Walleij wrote: > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + mdio { > + #address-cells = <1>; > + #size-cells = <0>; > + > + switch0: ethernet-switch@0 { > + compatible = "marvell,mv88e6085"; > + reg = <0>; > + reset-gpios = <&gpio5 1 GPIO_ACTIVE_LOW>; > + interrupts-extended = <&gpio0 27 IRQ_TYPE_LEVEL_LOW>; > + interrupt-controller; > + #interrupt-cells = <2>; > + > + ethernet-ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + label = "lan1"; > + }; > + port@1 { > + reg = <1>; > + label = "lan2"; > + }; > + port@2 { > + reg = <2>; > + label = "lan3"; > + }; > + port@3 { > + reg = <3>; > + label = "lan4"; > + }; > + port@4 { > + reg = <4>; > + label = "wan"; > + }; > + > + port@5 { > + reg = <5>; > + phy-mode = "sgmii"; > + ethernet = <ð2>; > + > + fixed-link { > + speed = <1000>; > + full-duplex; > + }; > + }; > + }; > + > + mdio { > + #address-cells = <1>; > + #size-cells = <0>; > + > + switch0phy0: ethernet-phy@0 { > + reg = <0>; > + interrupts-extended = <&switch0 0 IRQ_TYPE_LEVEL_HIGH>; > + }; > + }; > + }; > + }; > + > + - | > + #include <dt-bindings/gpio/gpio.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + mdio { > + #address-cells = <1>; > + #size-cells = <0>; > + > + switch1: ethernet-switch@0 { > + compatible = "marvell,mv88e6190"; > + reg = <0>; > + reset-gpios = <&gpio5 1 GPIO_ACTIVE_LOW>; > + interrupts-extended = <&gpio0 27 IRQ_TYPE_LEVEL_LOW>; > + interrupt-controller; > + #interrupt-cells = <2>; > + > + ethernet-ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + reg = <0>; > + label = "lan1"; > + }; > + port@1 { > + reg = <1>; > + label = "lan2"; > + }; > + port@2 { > + reg = <2>; > + label = "lan3"; > + }; > + port@3 { > + reg = <3>; > + label = "lan4"; > + }; > + }; > + > + mdio { > + #address-cells = <1>; > + #size-cells = <0>; > + > + switch1phy0: ethernet-phy@0 { > + reg = <0>; > + interrupts-extended = <&switch1 0 IRQ_TYPE_LEVEL_HIGH>; > + }; > + }; > + > + mdio-external { > + compatible = "marvell,mv88e6xxx-mdio-external"; > + #address-cells = <1>; > + #size-cells = <0>; > + switch1phy9: ethernet-phy@9 { > + reg = <9>; > + }; > + }; > + }; > + }; Yikes, both these examples are actually broken, for a reason that was extensively discussed with Arınç in the past, and that he tried to automatically detect through dt-schema but was ultimately told it's too complicated. https://patchwork.kernel.org/project/netdevbpf/cover/20230916110902.234273-1-arinc.unal@arinc9.com/ Long story short: the "mdio" node is also the ds->slave_mii_bus (soon to be ds->user_mii_bus after Florian's inclusivity changes). Having a slave_mii_bus makes DSA know what to do with port nodes like this, which don't have an explicit phy-handle: port@3 { reg = <3>; label = "lan4"; }; but actually, call phy_connect() on the ds->slave_mii_bus at address 3 (the port "reg"). The issue is that phy_connect() won't work if ds->slave_mii_bus has an OF presence, and ethernet-phy@3 isn't defined under it (which it isn't, you only put ethernet-phy@9). The super detailed reason is that the OF-based __of_mdiobus_register() does this: /* Mask out all PHYs from auto probing. Instead the PHYs listed in * the device tree are populated after the bus has been registered */ mdio->phy_mask = ~0; So either: - you delete the "mdio" node and the ethernet-phys under it, or - you add all ethernet-phys under the mdio node, and put phy-handles from ports to each of them, and phy-modes of "internal" What you have now is exactly what won't work, i.e. an OF-based slave_mii_bus with a non-OF-based phy_connect(). I don't want to see DT examples that are structurally broken, sorry, because then we wonder why users are confused. Personally, I would opt for adding the more modern explicit phy-handle and phy-mode everywhere. Those also work with the U-Boot DM_DSA driver. Just because we tolerate the bindings defined in the dark ages of DT doesn't mean we should make an example out of them. Also, you seem to have duplicated some work also done by Arınç but not finalized (the mv88e6xxx schema conversion, on which you were also copied). Let me add Marek here too, to make sure he's aware of 2 previous attempts and doesn't start working on a 3rd one :) One other thing I see as a deal breaker for this schema conversion is that $nodename for Marvell needs to allow basically anything (invalidating the constraint from ethernet-switch.yaml), because we can't change node names in the case of some boards, otherwise we risk breaking them (see MOX). If the schema starts emitting warnings for those node names, then it's inevitable that some pixie in the future will eventually break them by "fixing" the node name.
On Thu, Oct 19, 2023 at 8:49 AM Rob Herring <robh@kernel.org> wrote: > > On Wed, Oct 18, 2023 at 01:39:45PM +0200, Linus Walleij wrote: > > On Wed, Oct 18, 2023 at 12:32 PM Rob Herring <robh@kernel.org> wrote: > > > > > yamllint warnings/errors: > > > > > > dtschema/dtc warnings/errors: > > > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/marvell,mvusb.example.dtb: switch@0: ports: '#address-cells' is a required property > > > from schema $id: http://devicetree.org/schemas/net/dsa/marvell,mv88e6xxx.yaml# > > > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/marvell,mvusb.example.dtb: switch@0: ports: '#size-cells' is a required property > > > from schema $id: http://devicetree.org/schemas/net/dsa/marvell,mv88e6xxx.yaml# > > > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/marvell,mvusb.example.dtb: switch@0: ports: '#address-cells' is a required property > > > from schema $id: http://devicetree.org/schemas/net/dsa/marvell,mv88e6xxx.yaml# > > > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/marvell,mvusb.example.dtb: switch@0: ports: '#size-cells' is a required property > > > from schema $id: http://devicetree.org/schemas/net/dsa/marvell,mv88e6xxx.yaml# > > > > Fixed in patch 2/7? > > Yes. If one patch has errors we drop it. I should probably just give up > on the rest of the series instead. The bot should work better now not dropping patches when there are warnings. It will give incremental new warnings with each patch. Rob
On 19.10.2023 18:35, Vladimir Oltean wrote: > Yikes, both these examples are actually broken, for a reason that was > extensively discussed with Arınç in the past, and that he tried to > automatically detect through dt-schema but was ultimately told it's too > complicated. > https://patchwork.kernel.org/project/netdevbpf/cover/20230916110902.234273-1-arinc.unal@arinc9.com/ True, and then I've figured that that approach was wrong from the start. Read below. > > Long story short: the "mdio" node is also the ds->slave_mii_bus (soon to > be ds->user_mii_bus after Florian's inclusivity changes). Having a > slave_mii_bus makes DSA know what to do with port nodes like this, which > don't have an explicit phy-handle: > > port@3 { > reg = <3>; > label = "lan4"; > }; > > but actually, call phy_connect() on the ds->slave_mii_bus at address 3 > (the port "reg"). > > The issue is that phy_connect() won't work if ds->slave_mii_bus has an > OF presence, and ethernet-phy@3 isn't defined under it (which it isn't, > you only put ethernet-phy@9). The super detailed reason is that the > OF-based __of_mdiobus_register() does this: > > /* Mask out all PHYs from auto probing. Instead the PHYs listed in > * the device tree are populated after the bus has been registered */ > mdio->phy_mask = ~0; > > So either: > > - you delete the "mdio" node and the ethernet-phys under it, or > - you add all ethernet-phys under the mdio node, and put phy-handles > from ports to each of them, and phy-modes of "internal" > > What you have now is exactly what won't work, i.e. an OF-based > slave_mii_bus with a non-OF-based phy_connect(). > > I don't want to see DT examples that are structurally broken, sorry, > because then we wonder why users are confused. > > Personally, I would opt for adding the more modern explicit phy-handle > and phy-mode everywhere. Those also work with the U-Boot DM_DSA driver. As can be seen on the conversation on the patch series you've referred to above, this is ultimately what I've proposed to enforce on all ethernet controllers (I hadn't mentioned phy-mode there yet). As long as Andrew holds his stance, this won't happen. The whole conversation on that patch series is a great read as to how a devicetree should be defined and how drivers should interact with it. But sure, they can at least be put on the examples for this specific schema. Arınç
On Thu, Oct 19, 2023 at 5:35 PM Vladimir Oltean <olteanv@gmail.com> wrote: > Yikes, both these examples are actually broken, As you can see from the patch, they are just carried over from Documentation/devicetree/bindings/net/dsa/marvell.txt +/- fixes to make them pass schema checks. > So either: > > - you delete the "mdio" node and the ethernet-phys under it, or > - you add all ethernet-phys under the mdio node, and put phy-handles > from ports to each of them, and phy-modes of "internal" > > What you have now is exactly what won't work, i.e. an OF-based > slave_mii_bus with a non-OF-based phy_connect(). Yeah when I run check_dtbs I get a few (not many) warnings like this on aarch64 and armv7_multi: arch/arm/boot/dts/nxp/imx/imx6q-b450v3.dtb: switch@0: ports:port@4: 'phy-mode' is a required property from schema $id: http://devicetree.org/schemas/net/dsa/marvell,mv88e6xxx.yaml# Isn't there some in-kernel DTS file with a *good* example of how a Marvell mv88e6xxx switch is supposed to look I can just copy instead? We shouldn't conjure synthetic examples. > I don't want to see DT examples that are structurally broken, sorry, > because then we wonder why users are confused. These examples are already in the kernel. Migrating them from marvell.txt to marvell,mv88e6xxx.yaml doesn't make the situation worse, it's not like people magically start trusting the examples more because they are in YAML than in .txt. But sure let's try to put in better examples! > Personally, I would opt for adding the more modern explicit phy-handle > and phy-mode everywhere. I'm game. Point out the DTS file and I will take that. > Also, you seem to have duplicated some work also done by Arınç but not > finalized (the mv88e6xxx schema conversion, on which you were also > copied). Let me add Marek here too, to make sure he's aware of 2 > previous attempts and doesn't start working on a 3rd one :) Haha I forgot :D > One other thing I see as a deal breaker for this schema conversion is > that $nodename for Marvell needs to allow basically anything (invalidating > the constraint from ethernet-switch.yaml), because we can't change node > names in the case of some boards, otherwise we risk breaking them > (see MOX). If the schema starts emitting warnings for those node names, > then it's inevitable that some pixie in the future will eventually break > them by "fixing" the node name. I already did a bit of hippo-in-china-porcelain store in the patches in this series mostly renaming things like "switch0@0" to "switch@0" (yeah that's all). Is this part of the problem or something else? I run the binding checks across all aarch64 and armv7_multi platforms on this patch set without any major issues. Yours, Linus Walleij
On Fri, Oct 20, 2023 at 02:47:20PM +0200, Linus Walleij wrote: > On Thu, Oct 19, 2023 at 5:35 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > > Yikes, both these examples are actually broken, > > As you can see from the patch, they are just carried over from > Documentation/devicetree/bindings/net/dsa/marvell.txt > > +/- fixes to make them pass schema checks. (...) > These examples are already in the kernel. Migrating them > from marvell.txt to marvell,mv88e6xxx.yaml doesn't make > the situation worse, it's not like people magically start trusting > the examples more because they are in YAML than in .txt. > > But sure let's try to put in better examples! You are not correct here. The examples from Documentation/devicetree/bindings/net/dsa/marvell.txt don't have ports, and the way in which you added the ports is wrong (at least relative to the way in which you kept the mdio node). > > What you have now is exactly what won't work, i.e. an OF-based > > slave_mii_bus with a non-OF-based phy_connect(). > > Yeah when I run check_dtbs I get a few (not many) warnings > like this on aarch64 and armv7_multi: > > arch/arm/boot/dts/nxp/imx/imx6q-b450v3.dtb: switch@0: ports:port@4: > 'phy-mode' is a required property > from schema $id: > http://devicetree.org/schemas/net/dsa/marvell,mv88e6xxx.yaml# Ok, the warning is valid, but I don't know what phy-mode to put there. It is unrelated anyway. Some warnings will be expected after the schema conversion, and they are not all mechanical to fix. When we put schema validation in place for checking that CPU ports have valid link descriptions that phylink can use, we decided to be lax in the kernel, but strict in the dt-schema. Hmm, not sure what were we thinking. We didn't have a schema for Marvell, so we weren't even seeing many of the validation errors that you're now uncovering. > Isn't there some in-kernel DTS file with a *good* example of how > a Marvell mv88e6xxx switch is supposed to look I can just > copy instead? We shouldn't conjure synthetic examples. (...) > I'm game. Point out the DTS file and I will take that. You can use https://elixir.bootlin.com/u-boot/latest/source/arch/arm/dts/imx6qdl-gw5904.dtsi#L211 (optionally renaming switch to ethernet-switch, and ports to ethernet-ports). That uses the subset of the mv88e6xxx kernel bindings that U-Boot also understands, so taking a U-Boot example is actually preferable. > > One other thing I see as a deal breaker for this schema conversion is > > that $nodename for Marvell needs to allow basically anything (invalidating > > the constraint from ethernet-switch.yaml), because we can't change node > > names in the case of some boards, otherwise we risk breaking them > > (see MOX). If the schema starts emitting warnings for those node names, > > then it's inevitable that some pixie in the future will eventually break > > them by "fixing" the node name. > > I already did a bit of hippo-in-china-porcelain store in the patches > in this series mostly renaming things like "switch0@0" to "switch@0" > (yeah that's all). > > Is this part of the problem or something else? Yes, for most of the switches, renaming their OF nodes should not be a problem. For Marvell, I'd exercise extra caution and only rename those OF nodes where I can confirm that doing so won't break anything. Marvell is one of the oldest DSA drivers, and you can tell that the bindings have gone through a lot before becoming more or less uniform. Anyway, for the $nodename constraint, it _looks_ all mechanical and trivial to fix (unlike the missing phy-mode that you point to, above), so someone will jump to fix it. I would like to avoid that, because boot testing will be key, and a board is not always available.
> Isn't there some in-kernel DTS file with a *good* example of how > a Marvell mv88e6xxx switch is supposed to look I can just > copy instead? We shouldn't conjure synthetic examples. arch/arm/boot/dts/marvell/armada-381-netgear-gs110emx.dts is an example of a 6390 with external PHYs. The nodes are in a somewhat unconventional order, but nothing requires them to be any specific order. arch/arm/boot/dts/nxp/vf/vf610-zii-ssmb-spu3.dts is another 6390 which only uses the internal PHYs, so there are no mdio busses listed or needed. arch/arm/boot/dts/marvell/armada-370-rd.dts is another example of how it can be done. It lists the PHYs on the MDIO bus, but does not have any phy-handles, it lets the DSA core link them. Some people might consider this a bad example? But it works, i use this machine for development work. arch/arm/boot/dts/marvell/armada-385-linksys.dtsi is another alternative, which does not have the MDIO bus. Andrew
diff --git a/Documentation/devicetree/bindings/net/dsa/marvell,mv88e6xxx.yaml b/Documentation/devicetree/bindings/net/dsa/marvell,mv88e6xxx.yaml new file mode 100644 index 000000000000..8013ac411b15 --- /dev/null +++ b/Documentation/devicetree/bindings/net/dsa/marvell,mv88e6xxx.yaml @@ -0,0 +1,225 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/dsa/marvell,mv88e6xxx.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Marvell MV88E6xxx DSA switch family + +maintainers: + - Andrew Lunn <andrew@lunn.ch> + +description: + The Marvell MV88E6xxx switch series has been produced and sold + by Marvell since at least 2010. The switch has a few compatibles which + just indicate the base address of the switch, then operating systems + can investigate switch ID registers to find out which actual version + of the switch it is dealing with. + +properties: + compatible: + enum: + - marvell,mv88e6085 + - marvell,mv88e6190 + - marvell,mv88e6250 + description: | + marvell,mv88e6085: This switch uses base address 0x10. + This switch and its siblings will be autodetected from + ID registers found in the switch, so only "marvell,mv88e6085" should be + specified. This includes the following list of MV88Exxxx switches: + 6085, 6095, 6097, 6123, 6131, 6141, 6161, 6165, 6171, 6172, 6175, 6176, + 6185, 6240, 6320, 6321, 6341, 6350, 6351, 6352 + marvell,mv88e6190: This switch uses base address 0x00. + This switch and its siblings will be autodetected from + ID registers found in the switch, so only "marvell,mv88e6190" should be + specified. This includes the following list of MV88Exxxx switches: + 6190, 6190X, 6191, 6290, 6361, 6390, 6390X + marvell,mv88e6250: This switch uses base address 0x08 or 0x18. + This switch and its siblings will be autodetected from + ID registers found in the switch, so only "marvell,mv88e6250" should be + specified. This includes the following list of MV88Exxxx switches: + 6220, 6250 + + reg: + maxItems: 1 + + eeprom-length: + $ref: /schemas/types.yaml#/definitions/uint32 + description: Set to the length of an EEPROM connected to the switch. Must be + set if the switch can not detect the presence and/or size of a connected + EEPROM, otherwise optional. + + reset-gpios: + description: + GPIO to be used to reset the whole device + maxItems: 1 + + interrupts: + description: The switch provides an external interrupt line, but it is + not always used by target systems. + maxItems: 1 + + interrupt-controller: + description: The switch has an internal interrupt controller used by + the different sub-blocks. + + '#interrupt-cells': + description: The internal interrupt controller only supports triggering + on active high level interrupts so the second cell must alway be set to + IRQ_TYPE_LEVEL_HIGH. + const: 2 + + mdio: + $ref: /schemas/net/mdio.yaml# + unevaluatedProperties: false + description: Marvell MV88E6xxx switches have an varying combination of + internal and external MDIO buses, in some cases a combined bus that + can be used both internally and externally. This node is for the + primary bus, used internally and sometimes also externally. + + mdio-external: + $ref: /schemas/net/mdio.yaml# + unevaluatedProperties: false + description: Marvell MV88E6xxx switches that have a separate external + MDIO bus use this port to access external components on the MDIO bus. + + properties: + compatible: + const: marvell,mv88e6xxx-mdio-external + + required: + - compatible + +$ref: dsa.yaml# + +allOf: + - $ref: dsa.yaml#/$defs/ethernet-ports + +required: + - compatible + - reg + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + mdio { + #address-cells = <1>; + #size-cells = <0>; + + switch0: ethernet-switch@0 { + compatible = "marvell,mv88e6085"; + reg = <0>; + reset-gpios = <&gpio5 1 GPIO_ACTIVE_LOW>; + interrupts-extended = <&gpio0 27 IRQ_TYPE_LEVEL_LOW>; + interrupt-controller; + #interrupt-cells = <2>; + + ethernet-ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + label = "lan1"; + }; + port@1 { + reg = <1>; + label = "lan2"; + }; + port@2 { + reg = <2>; + label = "lan3"; + }; + port@3 { + reg = <3>; + label = "lan4"; + }; + port@4 { + reg = <4>; + label = "wan"; + }; + + port@5 { + reg = <5>; + phy-mode = "sgmii"; + ethernet = <ð2>; + + fixed-link { + speed = <1000>; + full-duplex; + }; + }; + }; + + mdio { + #address-cells = <1>; + #size-cells = <0>; + + switch0phy0: ethernet-phy@0 { + reg = <0>; + interrupts-extended = <&switch0 0 IRQ_TYPE_LEVEL_HIGH>; + }; + }; + }; + }; + + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + mdio { + #address-cells = <1>; + #size-cells = <0>; + + switch1: ethernet-switch@0 { + compatible = "marvell,mv88e6190"; + reg = <0>; + reset-gpios = <&gpio5 1 GPIO_ACTIVE_LOW>; + interrupts-extended = <&gpio0 27 IRQ_TYPE_LEVEL_LOW>; + interrupt-controller; + #interrupt-cells = <2>; + + ethernet-ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + label = "lan1"; + }; + port@1 { + reg = <1>; + label = "lan2"; + }; + port@2 { + reg = <2>; + label = "lan3"; + }; + port@3 { + reg = <3>; + label = "lan4"; + }; + }; + + mdio { + #address-cells = <1>; + #size-cells = <0>; + + switch1phy0: ethernet-phy@0 { + reg = <0>; + interrupts-extended = <&switch1 0 IRQ_TYPE_LEVEL_HIGH>; + }; + }; + + mdio-external { + compatible = "marvell,mv88e6xxx-mdio-external"; + #address-cells = <1>; + #size-cells = <0>; + switch1phy9: ethernet-phy@9 { + reg = <9>; + }; + }; + }; + }; diff --git a/Documentation/devicetree/bindings/net/dsa/marvell.txt b/Documentation/devicetree/bindings/net/dsa/marvell.txt deleted file mode 100644 index 6ec0c181b6db..000000000000 --- a/Documentation/devicetree/bindings/net/dsa/marvell.txt +++ /dev/null @@ -1,109 +0,0 @@ -Marvell DSA Switch Device Tree Bindings ---------------------------------------- - -WARNING: This binding is currently unstable. Do not program it into a -FLASH never to be changed again. Once this binding is stable, this -warning will be removed. - -If you need a stable binding, use the old dsa.txt binding. - -Marvell Switches are MDIO devices. The following properties should be -placed as a child node of an mdio device. - -The properties described here are those specific to Marvell devices. -Additional required and optional properties can be found in dsa.txt. - -The compatibility string is used only to find an identification register, -which is at a different MDIO base address in different switch families. -- "marvell,mv88e6085" : Switch has base address 0x10. Use with models: - 6085, 6095, 6097, 6123, 6131, 6141, 6161, 6165, - 6171, 6172, 6175, 6176, 6185, 6240, 6320, 6321, - 6341, 6350, 6351, 6352 -- "marvell,mv88e6190" : Switch has base address 0x00. Use with models: - 6190, 6190X, 6191, 6290, 6361, 6390, 6390X -- "marvell,mv88e6250" : Switch has base address 0x08 or 0x18. Use with model: - 6220, 6250 - -Required properties: -- compatible : Should be one of "marvell,mv88e6085", - "marvell,mv88e6190" or "marvell,mv88e6250" as - indicated above -- reg : Address on the MII bus for the switch. - -Optional properties: - -- reset-gpios : Should be a gpio specifier for a reset line -- interrupts : Interrupt from the switch -- interrupt-controller : Indicates the switch is itself an interrupt - controller. This is used for the PHY interrupts. -#interrupt-cells = <2> : Controller uses two cells, number and flag -- eeprom-length : Set to the length of an EEPROM connected to the - switch. Must be set if the switch can not detect - the presence and/or size of a connected EEPROM, - otherwise optional. -- mdio : Container of PHY and devices on the switches MDIO - bus. -- mdio? : Container of PHYs and devices on the external MDIO - bus. The node must contains a compatible string of - "marvell,mv88e6xxx-mdio-external" - -Example: - - mdio { - #address-cells = <1>; - #size-cells = <0>; - interrupt-parent = <&gpio0>; - interrupts = <27 IRQ_TYPE_LEVEL_LOW>; - interrupt-controller; - #interrupt-cells = <2>; - - switch0: switch@0 { - compatible = "marvell,mv88e6085"; - reg = <0>; - reset-gpios = <&gpio5 1 GPIO_ACTIVE_LOW>; - - mdio { - #address-cells = <1>; - #size-cells = <0>; - switch1phy0: switch1phy0@0 { - reg = <0>; - interrupt-parent = <&switch0>; - interrupts = <0 IRQ_TYPE_LEVEL_HIGH>; - }; - }; - }; - }; - - mdio { - #address-cells = <1>; - #size-cells = <0>; - interrupt-parent = <&gpio0>; - interrupts = <27 IRQ_TYPE_LEVEL_LOW>; - interrupt-controller; - #interrupt-cells = <2>; - - switch0: switch@0 { - compatible = "marvell,mv88e6190"; - reg = <0>; - reset-gpios = <&gpio5 1 GPIO_ACTIVE_LOW>; - - mdio { - #address-cells = <1>; - #size-cells = <0>; - switch1phy0: switch1phy0@0 { - reg = <0>; - interrupt-parent = <&switch0>; - interrupts = <0 IRQ_TYPE_LEVEL_HIGH>; - }; - }; - - mdio1 { - compatible = "marvell,mv88e6xxx-mdio-external"; - #address-cells = <1>; - #size-cells = <0>; - switch1phy9: switch1phy0@9 { - reg = <9>; - }; - }; - }; - }; diff --git a/MAINTAINERS b/MAINTAINERS index 90f13281d297..1b4475254d27 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12625,7 +12625,7 @@ MARVELL 88E6XXX ETHERNET SWITCH FABRIC DRIVER M: Andrew Lunn <andrew@lunn.ch> L: netdev@vger.kernel.org S: Maintained -F: Documentation/devicetree/bindings/net/dsa/marvell.txt +F: Documentation/devicetree/bindings/net/dsa/marvell,mv88e6xxx.yaml F: Documentation/networking/devlink/mv88e6xxx.rst F: drivers/net/dsa/mv88e6xxx/ F: include/linux/dsa/mv88e6xxx.h
This is an attempt to rewrite the Marvell MV88E6xxx switch bindings in YAML schema. The current text binding says: WARNING: This binding is currently unstable. Do not program it into a FLASH never to be changed again. Once this binding is stable, this warning will be removed. Well that never happened before we switched to YAML markup, we can't have it like this, what about fixing the mess? Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- .../bindings/net/dsa/marvell,mv88e6xxx.yaml | 225 +++++++++++++++++++++ .../devicetree/bindings/net/dsa/marvell.txt | 109 ---------- MAINTAINERS | 2 +- 3 files changed, 226 insertions(+), 110 deletions(-)