Date: Fri, 22 Dec 2023 17:03:38 -0300
From: Luiz Angelo Daros de Luca <luizluca@...il.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: linus.walleij@...aro.org, alsi@...g-olufsen.dk, andrew@...n.ch, 
	f.fainelli@...il.com, davem@...emloft.net, edumazet@...gle.com, 
	kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org, 
	arinc.unal@...nc9.com
Subject: Re: [PATCH net-next v2 5/7] net: dsa: realtek: Migrate user_mii_bus
 setup to realtek-dsa

> Ok. Please trim the quoted text from your replies to just what's relevant.
> It's easy to scroll past the new bits.

OK

>
> > > +int realtek_common_setup_user_mdio(struct dsa_switch *ds)
> > > +{
> > > +       const char *compatible = "realtek,smi-mdio";
> > > +       struct realtek_priv *priv =  ds->priv;
> > > +       struct device_node *phy_node;
> > > +       struct device_node *mdio_np;
> > > +       struct dsa_port *dp;
> > > +       int ret;
> > > +
> > > +       mdio_np = of_get_child_by_name(priv->dev->of_node, "mdio");
> > > +       if (!mdio_np) {
> > > +               mdio_np = of_get_compatible_child(priv->dev->of_node, compatible);
> > > +               if (!mdio_np) {
> > > +                       dev_err(priv->dev, "no MDIO bus node\n");
> > > +                       return -ENODEV;
> > > +               }
> > > +       }
> >
> > I just kept the code compatible with both realtek-smi and realtek-mdio
> > (that was using the generic "DSA user mii"), even when it might
> > violate the binding docs (for SMI with a node not named "mdio").
> >
> > You suggested using two new compatible strings for this driver
> > ("realtek,rtl8365mb-mdio" and "realtek,rtl8366rb-mdio"). However, it
> > might still not be a good name as it is similar to the MDIO-connected
> > subdriver of each variant. Anyway, if possible, I would like to keep
> > it out of this series as it would first require a change in the
> > bindings before any real code change and it might add some more path
> > cycles.
>
> I suppose what you don't want is to make the code inadvertently accept
> an MDIO bus named "realtek,smi-mdio" on MDIO-controlled switches.

I don't think it would hurt that much. I was just trying to keep the
old code behavior.

> I think it's safe to write a separate commit which just exercises a part
> of the dt-binding that the Linux driver hasn't used thus far: that the
> node name must be "mdio". You don't need to fall back to the search by
> compatible string if there is nothing broken to support, and it's all
> just theoretical (and even then, the theory is not supported by the DT
> binding).

OK. I'll drop the compatible part.

> > > +       priv->user_mii_bus = devm_mdiobus_alloc(priv->dev);
> > > +       if (!priv->user_mii_bus) {
> > > +               ret = -ENOMEM;
> > > +               goto err_put_node;
> > > +       }
> > > +       priv->user_mii_bus->priv = priv;
> > > +       priv->user_mii_bus->name = "Realtek user MII";
> > > +       priv->user_mii_bus->read = realtek_common_user_mdio_read;
> > > +       priv->user_mii_bus->write = realtek_common_user_mdio_write;
> > > +       snprintf(priv->user_mii_bus->id, MII_BUS_ID_SIZE, "Realtek-%d",
> > > +                ds->index);
> > > +       priv->user_mii_bus->parent = priv->dev;
> > > +
> > > +       /* When OF describes the MDIO, connecting ports with phy-handle,
> > > +        * ds->user_mii_bus should not be used *
> > > +        */
> > > +       dsa_switch_for_each_user_port(dp, ds) {
> > > +               phy_node = of_parse_phandle(dp->dn, "phy-handle", 0);
> > > +               of_node_put(phy_node);
> > > +               if (phy_node)
> > > +                       continue;
> > > +
> > > +               dev_warn(priv->dev,
> > > +                        "DS user_mii_bus in use as '%s' is missing phy-handle",
> > > +                        dp->name);
> > > +               ds->user_mii_bus = priv->user_mii_bus;
> > > +               break;
> > > +       }
> >
> > Does this check align with how should ds->user_mii_bus be used (in a
> > first step for phasing it out, at least for this driver)?
>
> No. Thanks for asking.
>
> What I would like to see is a commit which removes the line assigning
> ds->user_mii_bus completely, with the following justification:
>
> ds->user_mii_bus helps when
> (1) the switch probes with platform_data (not on OF), or
> (2) the switch probes on OF but its MDIO bus is not described in OF
>
> Case (1) is eliminated because this driver uses of_device_get_match_data()
> and fails to probe if that returns NULL (which it will, with platform_data).
> So this switch driver only probes on OF.
>
> Case (2) is also eliminated because realtek_smi_setup_mdio() bails out
> if it cannot find the "mdio" node described in OF. So the ds->user_mii_bus
> assignment is only ever executed when the bus has an OF node, aka when
> it is not useful.
>
> Having the MDIO bus described in OF, but no phy-handle to its children
> is a semantically broken device tree, we should make no effort whatsoever
> to support it.

OK. I was trying to keep exactly that setup working. Should I keep the
check and bail out with an error like:

+       dsa_switch_for_each_user_port(dp, ds) {
+               phy_node = of_parse_phandle(dp->dn, "phy-handle", 0);
+               of_node_put(phy_node);
+               if (phy_node)
+                       continue;
+               dev_err(priv->dev,
+                       "'%s' is missing phy-handle",
+                       dp->name);
+               return -EINVAL;
+       }

or should I simply let it break silently? The device-tree writers
might like some feedback if they are doing it wrong. I guess neither
DSA nor MDIO bus will say a thing about the missing phy-handle.

Regards,

Luiz

Powered by blists - more mailing lists