Message-ID: <CAJq09z5KJE1D=gCd5WX_B26FxYN_eGn7LwENwNQZ0BSe7aDwOA@mail.gmail.com>
Date: Sun, 28 Jan 2024 23:49:42 -0300
From: Luiz Angelo Daros de Luca <luizluca@...il.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: netdev@...r.kernel.org, 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, 
	arinc.unal@...nc9.com, ansuelsmth@...il.com
Subject: Re: [PATCH net-next v4 09/11] net: dsa: realtek: migrate user_mii_bus
 setup to realtek-dsa

> On Tue, Jan 23, 2024 at 06:56:01PM -0300, Luiz Angelo Daros de Luca wrote:
> > In the user MDIO driver, despite numerous references to SMI, including
> > its compatible string, there's nothing inherently specific about the SMI
> > interface in the user MDIO bus. Consequently, the code has been migrated
> > to the rtl83xx module. All references to SMI have been eliminated.
> >
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@...il.com>
> > ---
> > diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c
> > index 53bacbacc82e..525d8c014136 100644
> > --- a/drivers/net/dsa/realtek/rtl83xx.c
> > +++ b/drivers/net/dsa/realtek/rtl83xx.c
> > @@ -1,6 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0+
> >
> >  #include <linux/module.h>
> > +#include <linux/of_mdio.h>
> >
> >  #include "realtek.h"
> >  #include "rtl83xx.h"
> > @@ -42,6 +43,72 @@ void rtl83xx_unlock(void *ctx)
> >  }
> >  EXPORT_SYMBOL_NS_GPL(rtl83xx_unlock, REALTEK_DSA);
> >
> > +static int rtl83xx_user_mdio_read(struct mii_bus *bus, int addr, int regnum)
> > +{
> > +     struct realtek_priv *priv = bus->priv;
> > +
> > +     return priv->ops->phy_read(priv, addr, regnum);
> > +}
> > +
> > +static int rtl83xx_user_mdio_write(struct mii_bus *bus, int addr, int regnum,
> > +                                u16 val)
> > +{
> > +     struct realtek_priv *priv = bus->priv;
> > +
> > +     return priv->ops->phy_write(priv, addr, regnum, val);
> > +}
>
> Do we actually need to go through this extra indirection, or can the
> priv->ops->phy_read/write() prototypes be made to take just struct
> mii_bus * as their first argument?

We would just postpone the same code in phy_write/read. We only need priv there.

Using mii_bus will also prevent an easy way for the driver to query
those registers (although not used anymore after ds_switch_ops
.phy_read/write are gone)

>
> > +
> > +/**
> > + * rtl83xx_setup_user_mdio() - register the user mii bus driver
> > + * @ds: DSA switch associated with this user_mii_bus
> > + *
> > + * This function first gets and mdio node under the dev OF node, aborting
> > + * if missing. That mdio node describing an mdio bus is used to register a
> > + * new mdio bus.
> > + *
> > + * Context: Any context.
> > + * Return: 0 on success, negative value for failure.
> > + */
> > +int rtl83xx_setup_user_mdio(struct dsa_switch *ds)
> > +{
> > +     struct realtek_priv *priv =  ds->priv;
>
> Please remove the extra space here in " =  ds->priv".

OK

>
> > +     struct device_node *mdio_np;
> > +     int ret = 0;
> > +
> > +     mdio_np = of_get_child_by_name(priv->dev->of_node, "mdio");
> > +     if (!mdio_np) {
> > +             dev_err(priv->dev, "no MDIO bus node\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     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 = rtl83xx_user_mdio_read;
> > +     priv->user_mii_bus->write = rtl83xx_user_mdio_write;
> > +     snprintf(priv->user_mii_bus->id, MII_BUS_ID_SIZE, "Realtek-%d",
> > +              ds->index);
>
> There isn't much consistency here, but maybe something derived from
> dev_name(dev) or %pOF would make it clearer that it describes a switch's
> internal MDIO bus, rather than just any Realtek thing?

Yes, Realtek-0:<port> is not very informative.

Using only dev_name will give me these device names:

mdio-bus:1d <- switch1 in the conduit mdio bus at address 29 (0x1d)
(not the user mdio bus)
mdio-bus:1d:00 <- port 0 in switch1
mdio-bus:1d:01 <- port 1 in switch1
...

It is quite compact and easy to identify which device is under which.
But, in this case, mdio-bus:1d would be both the switch device name in
the conduit bus and the name of the switch internal mdio bus.

devices/platform/10100000.ethernet/mdio_bus/mdio-bus/mdio-bus:1d/mdio_bus/mdio-bus:1d/mdio-bus:1d:00
devices/platform/10100000.ethernet/mdio_bus/mdio-bus/mdio-bus:1d/mdio_bus/mdio-bus:1d/mdio-bus:1d:01

For SMI-connected switches, it changes a little bit but the essence is the same:

devices/platform/switch/mdio_bus/switch/switch:00
devices/platform/switch/mdio_bus/switch/switch:01

I guess the best approach is to append something that identifies the
other mdio bus, for example ":user_mii". The result is something like
this:

mdio-bus:1d
mdio-bus:1d:user_mii:00
mdio-bus:1d:user_mii:01
...

Or, for SMI:

switch:user_mii:00
switch:user_mii:01
...

It is good enough for me as these switches have only one MDIO bus.

We could also bring up some kind of a general suggestion for naming
user_mii buses. In that case, we should be prepared for multiple mdio
buses and the mdio node name+@...t (%pOFP) might be appropriate. We
would get something like this:

mdio-bus:1d:mdio:00
mdio-bus:1d:mdio:01

Or, for SMI:

switch:mdio:00
switch:mdio:01

If there are multiple MDIO buses, it will be mdio@N (not tested).

mdio-bus:1d:mdio@0:00
mdio-bus:1d:mdio@0:01
...
mdio-bus:1d:mdio@1:00
mdio-bus:1d:mdio@1:01
...

The device_name can also be replaced with %pOFP, which will differ
only for MDIO-connected switches:

mdio-bus:1d
switch1@29:mdio:01
switch1@29:mdio:02
...

But it will not have a clear relation with the parent MDIO bus.

I also considered %pOFf but it gives a more verbose device name
without adding too much useful information:

!ethernet@...00000!mdio-bus!switch1@29:00
!ethernet@...00000!mdio-bus!switch1@29:01
!ethernet@...00000!mdio-bus!switch1@29:02

And I'm reluctant to add those "!" as they may not play nice with some
non-ideal scripts reading sysfs. I would, at least, replace them with
":" .

> > +     priv->user_mii_bus->parent = priv->dev;
> > +
> > +     ret = devm_of_mdiobus_register(priv->dev, priv->user_mii_bus, mdio_np);
> > +     if (ret) {
> > +             dev_err(priv->dev, "unable to register MDIO bus %s\n",
> > +                     priv->user_mii_bus->id);
> > +             goto err_put_node;
> > +     }
>
> Maybe this function would look a bit less cluttered with a temporary
> struct mii_bus *bus variable, and priv->user_mii_bus gets assigned to
> "bus" at the end (on success), and another struct device *dev = priv->dev.

Yes, it looks much cleaner.

> Otherwise, this is in principle ok and what I wanted to see.

I'm glad to hear that. Thanks!

Regards,

Luiz

Powered by blists - more mailing lists