[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJq09z5zN86auxMOtfUOqSj9XzU-Vs8_=7UzfY-d=-N9dgAPyA@mail.gmail.com>
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