Message-ID: <20231019153552.nndysafvblrkl2zn@skbuf>
Date:   Thu, 19 Oct 2023 18:35:52 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Linus Walleij <linus.walleij@...aro.org>,
        Arınç ÜNAL <arinc.unal@...nc9.com>,
        Marek Behún <kabel@...nel.org>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Gregory Clement <gregory.clement@...tlin.com>,
        Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Russell King <linux@...linux.org.uk>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Christian Marangi <ansuelsmth@...il.com>,
        linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v4 6/7] dt-bindings: marvell: Rewrite MV88E6xxx
 in schema

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.

Powered by blists - more mailing lists