Message-ID: <20230814143601.mnpxtcm2zybnbvoh@skbuf>
Date:   Mon, 14 Aug 2023 17:36:01 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Arınç ÜNAL <arinc.unal@...nc9.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        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>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Woojung Huh <woojung.huh@...rochip.com>,
        UNGLinuxDriver@...rochip.com,
        Linus Walleij <linus.walleij@...aro.org>,
        Alvin Šipraga <alsi@...g-olufsen.dk>,
        Daniel Golle <daniel@...rotopia.org>,
        Landen Chao <Landen.Chao@...iatek.com>,
        DENG Qingfang <dqfext@...il.com>,
        Sean Wang <sean.wang@...iatek.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>, mithat.guner@...ont.com,
        erkin.bozoglu@...ont.com, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH 2/4] dt-bindings: net: dsa: document internal MDIO bus

Arınç,

On Mon, Aug 14, 2023 at 01:06:29PM +0300, Arınç ÜNAL wrote:
> On 13.08.2023 22:01, Vladimir Oltean wrote:
> > SJA1105 has zero internal MDIO buses and zero internal PHYs.
> 
> Ah okay. I didn't consider the switch architecture where the data interface
> of the PHY is connected to the switch, and the PHY management interface is
> connected to the mdio bus that the switch is connected to.
> 
> The schemas of the switches which already utilise the mdio property:
> - /schemas/net/dsa/microchip,lan937x.yaml
> - /schemas/net/dsa/qca8k.yaml
> - /schemas/net/dsa/realtek.yaml
> - /schemas/net/dsa/renesas,rzn1-a5psw.yaml
> 
> The schemas of the switches which don't have an internal MDIO bus, meaning
> the mdio property must be invalid:
> - /schemas/net/mscc,vsc7514-switch.yaml
> - /schemas/net/dsa/nxp,sja1105.yaml
> 
> The schemas of the switches which I don't know if the switch has got an
> internal MDIO bus:
> - /schemas/net/dsa/arrow,xrs700x.yaml
>   - I believe, because there's phy-handle defined on every port without the
>     mdio node on the example, the PHYs are not connected to the internal
>     MDIO bus. Therefore, we can invalidate the mdio property for this
>     schema.
> - /schemas/net/dsa/brcm,b53.yaml
>   - Seems ok to keep it valid.
> - /schemas/net/dsa/brcm,sf2.yaml
>   - Seems ok to keep it valid.
> - /schemas/net/dsa/hirschmann,hellcreek.yaml
>   - Same as /schemas/net/dsa/arrow,xrs700x.yaml.
> - /schemas/net/dsa/microchip,ksz.yaml
>   - Seems ok to keep it valid.
> - /schemas/net/dsa/mscc,ocelot.yaml
>   - Same as /schemas/net/dsa/arrow,xrs700x.yaml.
> 
> Not json-schema documentation, don't care about:
> - ar9331.txt
> - lan9303.txt
> - lantiq-gswip.txt
> - marvell.txt
> - vitesse,vsc73xx.txt
> 
> Arınç

We have to keep in sight why we're here, and stick to that.

You had issues with a device tree that didn't work, but it passed
validation, and you're trying to enforce extra rules through the
json-schema so that next time, it fails. Verbally, that rule would be:
"if the switch has a ds->slave_mii_bus which does not have an OF
presence, then phylink compatible bindings may be omitted, and that has
a special and valid meaning (the port is connected to an internal PHY on
that ds->slave_mii_bus). If ds->slave_mii_bus has an OF presence, or if
ds->slave_mii_bus is NULL, then phylink-compatible bindings (phy-handle
or fixed-link or managed) are required on all user ports".

So it becomes a question of tracking ds->slave_mii_bus for all drivers.
In essence, it's fundamentally about the ds->slave_mii_bus, not about
the "mdio" child node. See more below.

There are 2 code paths that lead to its creation:

1. DSA registers the bus in dsa_slave_mii_bus_init(), based on the
   presence of ds->ops->phy_read() and ds->ops->phy_write(). Traditionally,
   a slave_mii_bus created this way was always non-OF-based, but Luiz,
   in commit fe7324b93222 ("net: dsa: OF-ware slave_mii_bus"), thought
   it would be a good idea for them to be optionally OF-based (and thus,
   useless at their primary purpose of being able to have internal PHYs
   without a phy-handle). But, it was thought that the framework
   registering an MDIO bus automatically would be a plus. So, ds->slave_mii_bus
   created in this way may or may not have an OF presence, with no way
   to know except to look at device trees (and to presume that they do).

   The drivers that populate ds->ops->phy_read() and ds->ops->phy_write() are:
    |
    +--- dsa_loop_driver: not OF-based
    |
    +--- ksz_switch_ops: OF-based or non-OF-based
    |
    +--- mv88e6060_switch_ops: OF-based or non-OF-based
    |
    +--- lan9303_switch_ops: OF-based or non-OF-based
    |
    +--- rtl8365mb_switch_ops_mdio: OF-based or non-OF-based
    |
    +--- b53_switch_ops: OF-based or non-OF-based
    |
    +--- vsc73xx_ds_ops: OF-based or non-OF-based

2. The switch driver registers the bus, and populates ds->slave_mii_bus with
   a pointer to it.
    |
    +--- Bus is not OF-based (it was registered with mdiobus_register()).
    |    This is the normal case:
    |      * mv88e6xxx_default_mdio_bus() in some cases
    |      * qca8k_mdio_register() in the "qca8k-legacy slave mii" case
    |      * bcm_sf2_mdio_register()
    |      * mt7530_setup_mdio()
    |
    +--- Bus is OF-based (it was registered with of_mdiobus_register()).
         I've no idea why you'd do this, because you have neither the
         benefit of using a non-OF-based phy_connect(), nor the benefit
         of having DSA register the bus for you:
           * mv88e6xxx_default_mdio_bus() when of_get_child_by_name(np, "mdio")
             is non-NULL
           * qca8k_mdio_register() when of_get_child_by_name(priv->dev->of_node, "mdio")
             is non-NULL
           * ksz_mdio_register() - it always wants an "mdio" child node
           * gswip_mdio() - it always wants a child node compatible with
             "lantiq,xrx200-mdio"
           * realtek_smi_setup_mdio() - it always wants a child node
             compatible with "realtek,smi-mdio"

For switches in the first category, the presence of the "mdio" child
node is what makes the ds->slave_mii_bus be OF-based or not, since it is
all the same binding, imposed by Luiz in dsa_switch_setup().

For switches in the second category, it all depends on the way in which
the driver finds the node for of_mdiobus_register().


Having identified all switches which make some sort of use of
ds->slave_mii_bus, the rule would sound like this:

1. If the schema is that of (need to replace this with compatible
   strings, I'm too lazy for that):

   - ksz_switch_ops
   - mv88e6060_switch_ops
   - lan9303_switch_ops
   - rtl8365mb_switch_ops_mdio
   - b53_switch_ops
   - vsc73xx_ds_ops
   - mv88e6xxx
   - qca8k

   and we have an "mdio" child, then phylink bindings are mandatory on user ports.

2. If the schema is that of gswip_mdio and we have a child node of "lantiq,xrx200-mdio",
   then phylink bindings are mandatory on user ports (I haven't checked,
   but it might be that the "lantiq,xrx200-mdio" child is mandatory, and
   in that case, this goes to category 4 below).

3. If the schema is that of realtek_smi_setup_mdio and we have a child node of
   "realtek,smi-mdio", then phylink bindings are mandatory on user ports
   (same comment about the child MDIO note maybe being mandatory).

4. If the switch didn't appear in the above set of rules, then phylink
   bindings are unconditionally mandatory on user ports.

We don't care at all what the drivers that don't use ds->slave_mii_bus
do with the "mdio" child node. It doesn't change the fact that their
user ports can't have missing phylink bindings.

Powered by blists - more mailing lists