| Message ID | 20240123215606.26716-10-luizluca@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: realtek: variants to drivers, interfaces to a common module | expand |
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@gmail.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? > + > +/** > + * 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". > + 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? > + 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. > + > +err_put_node: > + of_node_put(mdio_np); > + > + return ret; > +} > +EXPORT_SYMBOL_NS_GPL(rtl83xx_setup_user_mdio, REALTEK_DSA); > + > /** > * rtl83xx_probe() - probe a Realtek switch > * @dev: the device being probed > diff --git a/drivers/net/dsa/realtek/rtl83xx.h b/drivers/net/dsa/realtek/rtl83xx.h > index 9eb8197a58fa..b5d464bb850d 100644 > --- a/drivers/net/dsa/realtek/rtl83xx.h > +++ b/drivers/net/dsa/realtek/rtl83xx.h > @@ -12,6 +12,7 @@ struct realtek_interface_info { > > void rtl83xx_lock(void *ctx); > void rtl83xx_unlock(void *ctx); > +int rtl83xx_setup_user_mdio(struct dsa_switch *ds); > struct realtek_priv * > rtl83xx_probe(struct device *dev, > const struct realtek_interface_info *interface_info); > -- > 2.43.0 > Otherwise, this is in principle ok and what I wanted to see.
> 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@gmail.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+@unit (%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@10100000!mdio-bus!switch1@29:00 !ethernet@10100000!mdio-bus!switch1@29:01 !ethernet@10100000!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
On Sun, Jan 28, 2024 at 11:49:42PM -0300, Luiz Angelo Daros de Luca wrote: > 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) Exactly, there is no other remaining call to priv->ops->phy_read() and priv->ops->phy_write(), so their prototypes can be tailored such that they need no extra adapter. > 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 > ... This looks good. > > 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+@unit (%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 > ... SJA1110 has 2 MDIO buses and they are named: snprintf(bus->id, MII_BUS_ID_SIZE, "%s-base-tx", dev_name(priv->ds->dev)); snprintf(bus->id, MII_BUS_ID_SIZE, "%s-base-t1", dev_name(priv->ds->dev)); which I think is more descriptive, because in its case, the indices in "mdio@0" and "mdio@1" are arbitrary numbers. I don't think we'll find a way to unify the naming convention across the board. Let's use dev_name(dev) + "-some-driver-specific-qualifier" here, and hopefully also as a convention from now on. > I also considered %pOFf but it gives a more verbose device name > without adding too much useful information: > > !ethernet@10100000!mdio-bus!switch1@29:00 > !ethernet@10100000!mdio-bus!switch1@29:01 > !ethernet@10100000!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 > ":" . I'm also not in love with the exclamation marks that the sysfs code has to use, to replace the forward slashes that can't be represented in the filesystem.
diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c index a89813e527d2..70f3967e56e8 100644 --- a/drivers/net/dsa/realtek/realtek-smi.c +++ b/drivers/net/dsa/realtek/realtek-smi.c @@ -31,7 +31,6 @@ #include <linux/spinlock.h> #include <linux/skbuff.h> #include <linux/of.h> -#include <linux/of_mdio.h> #include <linux/delay.h> #include <linux/gpio/consumer.h> #include <linux/platform_device.h> @@ -312,60 +311,6 @@ static int realtek_smi_read(void *ctx, u32 reg, u32 *val) return realtek_smi_read_reg(priv, reg, val); } -static int realtek_smi_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 realtek_smi_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); -} - -static int realtek_smi_setup_mdio(struct dsa_switch *ds) -{ - struct realtek_priv *priv = ds->priv; - 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 = "SMI user MII"; - priv->user_mii_bus->read = realtek_smi_mdio_read; - priv->user_mii_bus->write = realtek_smi_mdio_write; - snprintf(priv->user_mii_bus->id, MII_BUS_ID_SIZE, "SMI-%d", - ds->index); - 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; - } - -err_put_node: - of_node_put(mdio_np); - - return ret; -} - static const struct realtek_interface_info realtek_smi_info = { .reg_read = realtek_smi_read, .reg_write = realtek_smi_write, @@ -404,7 +349,7 @@ int realtek_smi_probe(struct platform_device *pdev) return PTR_ERR(priv->mdio); priv->write_reg_noack = realtek_smi_write_reg_noack; - priv->setup_interface = realtek_smi_setup_mdio; + priv->setup_interface = rtl83xx_setup_user_mdio; priv->ds_ops = priv->variant->ds_ops_smi; ret = rtl83xx_register_switch(priv); 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); +} + +/** + * 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; + 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); + 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; + } + +err_put_node: + of_node_put(mdio_np); + + return ret; +} +EXPORT_SYMBOL_NS_GPL(rtl83xx_setup_user_mdio, REALTEK_DSA); + /** * rtl83xx_probe() - probe a Realtek switch * @dev: the device being probed diff --git a/drivers/net/dsa/realtek/rtl83xx.h b/drivers/net/dsa/realtek/rtl83xx.h index 9eb8197a58fa..b5d464bb850d 100644 --- a/drivers/net/dsa/realtek/rtl83xx.h +++ b/drivers/net/dsa/realtek/rtl83xx.h @@ -12,6 +12,7 @@ struct realtek_interface_info { void rtl83xx_lock(void *ctx); void rtl83xx_unlock(void *ctx); +int rtl83xx_setup_user_mdio(struct dsa_switch *ds); struct realtek_priv * rtl83xx_probe(struct device *dev, const struct realtek_interface_info *interface_info);
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@gmail.com> --- drivers/net/dsa/realtek/realtek-smi.c | 57 +---------------------- drivers/net/dsa/realtek/rtl83xx.c | 67 +++++++++++++++++++++++++++ drivers/net/dsa/realtek/rtl83xx.h | 1 + 3 files changed, 69 insertions(+), 56 deletions(-)