diff mbox series

[net-next,v4,6/7] dt-bindings: marvell: Rewrite MV88E6xxx in schema

Message ID 20231018-marvell-88e6152-wan-led-v4-6-3ee0c67383be@linaro.org (mailing list archive)
State New, archived
Headers show
Series Create a binding for the Marvell MV88E6xxx DSA switches | expand

Commit Message

Linus Walleij Oct. 18, 2023, 9:03 a.m. UTC
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(-)

Comments

Rob Herring (Arm) Oct. 18, 2023, 10:32 a.m. UTC | #1
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.
Linus Walleij Oct. 18, 2023, 11:39 a.m. UTC | #2
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
Andrew Lunn Oct. 18, 2023, 1:48 p.m. UTC | #3
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
Rob Herring (Arm) Oct. 19, 2023, 1:49 p.m. UTC | #4
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
Rob Herring (Arm) Oct. 19, 2023, 1:51 p.m. UTC | #5
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
Vladimir Oltean Oct. 19, 2023, 3:35 p.m. UTC | #6
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 = <&eth2>;
> +
> +                    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.
Rob Herring (Arm) Oct. 19, 2023, 8:02 p.m. UTC | #7
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
Arınç ÜNAL Oct. 20, 2023, 9:13 a.m. UTC | #8
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ç
Linus Walleij Oct. 20, 2023, 12:47 p.m. UTC | #9
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
Vladimir Oltean Oct. 20, 2023, 3:12 p.m. UTC | #10
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.
Andrew Lunn Oct. 20, 2023, 4:07 p.m. UTC | #11
> 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 mbox series

Patch

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 = <&eth2>;
+
+                    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