| Message ID | 20231220042632.26825-6-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 |
Hello Vladimir, I'm sorry to bother you again but I would like your attention for two points that I'm not completely sure about. > 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 common module. All references to SMI have been eliminated, with > the exception of the compatible string, which will continue to function > as before. > > The realtek-mdio will now use this driver instead of the generic DSA > driver ("dsa user smi"), which should not be used with OF[1]. > > There was a change in how the driver looks for the MDIO node in the > device tree. Now, it first checks for a child node named "mdio," which > is required by both interfaces in binding docs but used previously only > by realtek-mdio. If the node is not found, it will also look for a > compatible string, required only by SMI-connected devices in binding > docs and compatible with the old realtek-smi behavior. > > The line assigning dev.of_node in mdio_bus has been removed since the > subsequent of_mdiobus_register will always overwrite it. > > ds->user_mii_bus is only defined if all user ports do not declare a > phy-handle, providing a warning about the erroneous device tree[2]. > > With a single ds_ops for both interfaces, the ds_ops in realtek_priv is > no longer necessary. Now, the realtek_variant.ds_ops can be used > directly. > > The realtek_priv.setup_interface() has been removed as we can directly > call the new common function. > > The switch unregistration and the MDIO node decrement in refcount were > moved into realtek_common_remove() as both interfaces now need to put > the MDIO node. > > [1] https://lkml.kernel.org/netdev/20220630200423.tieprdu5fpabflj7@bang-olufsen.dk/T/ > [2] https://lkml.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/T/#u > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > --- > drivers/net/dsa/realtek/realtek-common.c | 87 +++++++++++++++++++++++- > drivers/net/dsa/realtek/realtek-common.h | 1 + > drivers/net/dsa/realtek/realtek-mdio.c | 6 -- > drivers/net/dsa/realtek/realtek-smi.c | 68 ------------------ > drivers/net/dsa/realtek/realtek.h | 5 +- > drivers/net/dsa/realtek/rtl8365mb.c | 49 ++----------- > drivers/net/dsa/realtek/rtl8366rb.c | 52 ++------------ > 7 files changed, 100 insertions(+), 168 deletions(-) > > diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c > index bf3933a99072..b1f0095d5bce 100644 > --- a/drivers/net/dsa/realtek/realtek-common.c > +++ b/drivers/net/dsa/realtek/realtek-common.c > @@ -1,6 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0+ > > #include <linux/module.h> > +#include <linux/of_mdio.h> > > #include "realtek.h" > #include "realtek-common.h" > @@ -21,6 +22,85 @@ void realtek_common_unlock(void *ctx) > } > EXPORT_SYMBOL_GPL(realtek_common_unlock); > > +static int realtek_common_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 realtek_common_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); > +} > + > +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. > + 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)? > + > + 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; > + } > + > + return 0; > + > +err_put_node: > + of_node_put(mdio_np); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(realtek_common_setup_user_mdio); > + > /* sets up driver private data struct, sets up regmaps, parse common device-tree > * properties and finally issues a hardware reset. > */ > @@ -108,7 +188,7 @@ int realtek_common_register_switch(struct realtek_priv *priv) > > priv->ds->priv = priv; > priv->ds->dev = priv->dev; > - priv->ds->ops = priv->ds_ops; > + priv->ds->ops = priv->variant->ds_ops; > priv->ds->num_ports = priv->num_ports; > > ret = dsa_register_switch(priv->ds); > @@ -126,6 +206,11 @@ void realtek_common_remove(struct realtek_priv *priv) > if (!priv) > return; > > + dsa_unregister_switch(priv->ds); > + > + if (priv->user_mii_bus) > + of_node_put(priv->user_mii_bus->dev.of_node); > + > /* leave the device reset asserted */ > if (priv->reset) > gpiod_set_value(priv->reset, 1); > diff --git a/drivers/net/dsa/realtek/realtek-common.h b/drivers/net/dsa/realtek/realtek-common.h > index 518d091ff496..b1c2a50d85cd 100644 > --- a/drivers/net/dsa/realtek/realtek-common.h > +++ b/drivers/net/dsa/realtek/realtek-common.h > @@ -7,6 +7,7 @@ > > void realtek_common_lock(void *ctx); > void realtek_common_unlock(void *ctx); > +int realtek_common_setup_user_mdio(struct dsa_switch *ds); > struct realtek_priv * > realtek_common_probe(struct device *dev, struct regmap_config rc, > struct regmap_config rc_nolock); > diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c > index 967f6c1e8df0..e2b5432eeb26 100644 > --- a/drivers/net/dsa/realtek/realtek-mdio.c > +++ b/drivers/net/dsa/realtek/realtek-mdio.c > @@ -142,7 +142,6 @@ int realtek_mdio_probe(struct mdio_device *mdiodev) > priv->bus = mdiodev->bus; > priv->mdio_addr = mdiodev->addr; > priv->write_reg_noack = realtek_mdio_write; > - priv->ds_ops = priv->variant->ds_ops_mdio; > > ret = realtek_common_register_switch(priv); > if (ret) > @@ -156,11 +155,6 @@ void realtek_mdio_remove(struct mdio_device *mdiodev) > { > struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev); > > - if (!priv) > - return; > - > - dsa_unregister_switch(priv->ds); > - > realtek_common_remove(priv); > } > EXPORT_SYMBOL_GPL(realtek_mdio_remove); > diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c > index 2b2c6e34bae5..383689163057 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> > @@ -339,63 +338,6 @@ static const struct regmap_config realtek_smi_nolock_regmap_config = { > .disable_locking = true, > }; > > -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; > - > - mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-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->dev.of_node = mdio_np; > - priv->user_mii_bus->parent = priv->dev; > - ds->user_mii_bus = priv->user_mii_bus; > - > - 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; > - } > - > - return 0; > - > -err_put_node: > - of_node_put(mdio_np); > - > - return ret; > -} > - > int realtek_smi_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -417,8 +359,6 @@ 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->ds_ops = priv->variant->ds_ops_smi; > > ret = realtek_common_register_switch(priv); > if (ret) > @@ -432,14 +372,6 @@ void realtek_smi_remove(struct platform_device *pdev) > { > struct realtek_priv *priv = platform_get_drvdata(pdev); > > - if (!priv) > - return; > - > - dsa_unregister_switch(priv->ds); > - > - if (priv->user_mii_bus) > - of_node_put(priv->user_mii_bus->dev.of_node); > - > realtek_common_remove(priv); > } > EXPORT_SYMBOL_GPL(realtek_smi_remove); > diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h > index fbd0616c1df3..7af6dcc1bb24 100644 > --- a/drivers/net/dsa/realtek/realtek.h > +++ b/drivers/net/dsa/realtek/realtek.h > @@ -60,7 +60,6 @@ struct realtek_priv { > > spinlock_t lock; /* Locks around command writes */ > struct dsa_switch *ds; > - const struct dsa_switch_ops *ds_ops; > struct irq_domain *irqdomain; > bool leds_disabled; > > @@ -71,7 +70,6 @@ struct realtek_priv { > struct rtl8366_mib_counter *mib_counters; > > const struct realtek_ops *ops; > - int (*setup_interface)(struct dsa_switch *ds); > int (*write_reg_noack)(void *ctx, u32 addr, u32 data); > > int vlan_enabled; > @@ -115,8 +113,7 @@ struct realtek_ops { > }; > > struct realtek_variant { > - const struct dsa_switch_ops *ds_ops_smi; > - const struct dsa_switch_ops *ds_ops_mdio; > + const struct dsa_switch_ops *ds_ops; > const struct realtek_ops *ops; > unsigned int clk_delay; > u8 cmd_read; > diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c > index 58ec057b6c32..e890ad113ba3 100644 > --- a/drivers/net/dsa/realtek/rtl8365mb.c > +++ b/drivers/net/dsa/realtek/rtl8365mb.c > @@ -828,17 +828,6 @@ static int rtl8365mb_phy_write(struct realtek_priv *priv, int phy, int regnum, > return 0; > } > > -static int rtl8365mb_dsa_phy_read(struct dsa_switch *ds, int phy, int regnum) > -{ > - return rtl8365mb_phy_read(ds->priv, phy, regnum); > -} > - > -static int rtl8365mb_dsa_phy_write(struct dsa_switch *ds, int phy, int regnum, > - u16 val) > -{ > - return rtl8365mb_phy_write(ds->priv, phy, regnum, val); > -} > - > static const struct rtl8365mb_extint * > rtl8365mb_get_port_extint(struct realtek_priv *priv, int port) > { > @@ -2017,12 +2006,10 @@ static int rtl8365mb_setup(struct dsa_switch *ds) > if (ret) > goto out_teardown_irq; > > - if (priv->setup_interface) { > - ret = priv->setup_interface(ds); > - if (ret) { > - dev_err(priv->dev, "could not set up MDIO bus\n"); > - goto out_teardown_irq; > - } > + ret = realtek_common_setup_user_mdio(ds); > + if (ret) { > + dev_err(priv->dev, "could not set up MDIO bus\n"); > + goto out_teardown_irq; > } > > /* Start statistics counter polling */ > @@ -2116,28 +2103,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv) > return 0; > } > > -static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = { > - .get_tag_protocol = rtl8365mb_get_tag_protocol, > - .change_tag_protocol = rtl8365mb_change_tag_protocol, > - .setup = rtl8365mb_setup, > - .teardown = rtl8365mb_teardown, > - .phylink_get_caps = rtl8365mb_phylink_get_caps, > - .phylink_mac_config = rtl8365mb_phylink_mac_config, > - .phylink_mac_link_down = rtl8365mb_phylink_mac_link_down, > - .phylink_mac_link_up = rtl8365mb_phylink_mac_link_up, > - .port_stp_state_set = rtl8365mb_port_stp_state_set, > - .get_strings = rtl8365mb_get_strings, > - .get_ethtool_stats = rtl8365mb_get_ethtool_stats, > - .get_sset_count = rtl8365mb_get_sset_count, > - .get_eth_phy_stats = rtl8365mb_get_phy_stats, > - .get_eth_mac_stats = rtl8365mb_get_mac_stats, > - .get_eth_ctrl_stats = rtl8365mb_get_ctrl_stats, > - .get_stats64 = rtl8365mb_get_stats64, > - .port_change_mtu = rtl8365mb_port_change_mtu, > - .port_max_mtu = rtl8365mb_port_max_mtu, > -}; > - > -static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = { > +static const struct dsa_switch_ops rtl8365mb_switch_ops = { > .get_tag_protocol = rtl8365mb_get_tag_protocol, > .change_tag_protocol = rtl8365mb_change_tag_protocol, > .setup = rtl8365mb_setup, > @@ -2146,8 +2112,6 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = { > .phylink_mac_config = rtl8365mb_phylink_mac_config, > .phylink_mac_link_down = rtl8365mb_phylink_mac_link_down, > .phylink_mac_link_up = rtl8365mb_phylink_mac_link_up, > - .phy_read = rtl8365mb_dsa_phy_read, > - .phy_write = rtl8365mb_dsa_phy_write, > .port_stp_state_set = rtl8365mb_port_stp_state_set, > .get_strings = rtl8365mb_get_strings, > .get_ethtool_stats = rtl8365mb_get_ethtool_stats, > @@ -2167,8 +2131,7 @@ static const struct realtek_ops rtl8365mb_ops = { > }; > > const struct realtek_variant rtl8365mb_variant = { > - .ds_ops_smi = &rtl8365mb_switch_ops_smi, > - .ds_ops_mdio = &rtl8365mb_switch_ops_mdio, > + .ds_ops = &rtl8365mb_switch_ops, > .ops = &rtl8365mb_ops, > .clk_delay = 10, > .cmd_read = 0xb9, > diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c > index e60a0a81d426..56619aa592ec 100644 > --- a/drivers/net/dsa/realtek/rtl8366rb.c > +++ b/drivers/net/dsa/realtek/rtl8366rb.c > @@ -1027,12 +1027,10 @@ static int rtl8366rb_setup(struct dsa_switch *ds) > if (ret) > dev_info(priv->dev, "no interrupt support\n"); > > - if (priv->setup_interface) { > - ret = priv->setup_interface(ds); > - if (ret) { > - dev_err(priv->dev, "could not set up MDIO bus\n"); > - return -ENODEV; > - } > + ret = realtek_common_setup_user_mdio(ds); > + if (ret) { > + dev_err(priv->dev, "could not set up MDIO bus\n"); > + return -ENODEV; > } > > return 0; > @@ -1772,17 +1770,6 @@ static int rtl8366rb_phy_write(struct realtek_priv *priv, int phy, int regnum, > return ret; > } > > -static int rtl8366rb_dsa_phy_read(struct dsa_switch *ds, int phy, int regnum) > -{ > - return rtl8366rb_phy_read(ds->priv, phy, regnum); > -} > - > -static int rtl8366rb_dsa_phy_write(struct dsa_switch *ds, int phy, int regnum, > - u16 val) > -{ > - return rtl8366rb_phy_write(ds->priv, phy, regnum, val); > -} > - > static int rtl8366rb_reset_chip(struct realtek_priv *priv) > { > int timeout = 10; > @@ -1848,35 +1835,9 @@ static int rtl8366rb_detect(struct realtek_priv *priv) > return 0; > } > > -static const struct dsa_switch_ops rtl8366rb_switch_ops_smi = { > - .get_tag_protocol = rtl8366_get_tag_protocol, > - .setup = rtl8366rb_setup, > - .phylink_get_caps = rtl8366rb_phylink_get_caps, > - .phylink_mac_link_up = rtl8366rb_mac_link_up, > - .phylink_mac_link_down = rtl8366rb_mac_link_down, > - .get_strings = rtl8366_get_strings, > - .get_ethtool_stats = rtl8366_get_ethtool_stats, > - .get_sset_count = rtl8366_get_sset_count, > - .port_bridge_join = rtl8366rb_port_bridge_join, > - .port_bridge_leave = rtl8366rb_port_bridge_leave, > - .port_vlan_filtering = rtl8366rb_vlan_filtering, > - .port_vlan_add = rtl8366_vlan_add, > - .port_vlan_del = rtl8366_vlan_del, > - .port_enable = rtl8366rb_port_enable, > - .port_disable = rtl8366rb_port_disable, > - .port_pre_bridge_flags = rtl8366rb_port_pre_bridge_flags, > - .port_bridge_flags = rtl8366rb_port_bridge_flags, > - .port_stp_state_set = rtl8366rb_port_stp_state_set, > - .port_fast_age = rtl8366rb_port_fast_age, > - .port_change_mtu = rtl8366rb_change_mtu, > - .port_max_mtu = rtl8366rb_max_mtu, > -}; > - > -static const struct dsa_switch_ops rtl8366rb_switch_ops_mdio = { > +static const struct dsa_switch_ops rtl8366rb_switch_ops = { > .get_tag_protocol = rtl8366_get_tag_protocol, > .setup = rtl8366rb_setup, > - .phy_read = rtl8366rb_dsa_phy_read, > - .phy_write = rtl8366rb_dsa_phy_write, > .phylink_get_caps = rtl8366rb_phylink_get_caps, > .phylink_mac_link_up = rtl8366rb_mac_link_up, > .phylink_mac_link_down = rtl8366rb_mac_link_down, > @@ -1915,8 +1876,7 @@ static const struct realtek_ops rtl8366rb_ops = { > }; > > const struct realtek_variant rtl8366rb_variant = { > - .ds_ops_smi = &rtl8366rb_switch_ops_smi, > - .ds_ops_mdio = &rtl8366rb_switch_ops_mdio, > + .ds_ops = &rtl8366rb_switch_ops, > .ops = &rtl8366rb_ops, > .clk_delay = 10, > .cmd_read = 0xa9, > -- > 2.43.0 >
On Wed, Dec 20, 2023 at 01:24:28AM -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 common module. All references to SMI have been eliminated, with > the exception of the compatible string, which will continue to function > as before. > > The realtek-mdio will now use this driver instead of the generic DSA > driver ("dsa user smi"), which should not be used with OF[1]. > > There was a change in how the driver looks for the MDIO node in the > device tree. Now, it first checks for a child node named "mdio," which > is required by both interfaces in binding docs but used previously only > by realtek-mdio. If the node is not found, it will also look for a > compatible string, required only by SMI-connected devices in binding > docs and compatible with the old realtek-smi behavior. > > The line assigning dev.of_node in mdio_bus has been removed since the > subsequent of_mdiobus_register will always overwrite it. > > ds->user_mii_bus is only defined if all user ports do not declare a > phy-handle, providing a warning about the erroneous device tree[2]. > > With a single ds_ops for both interfaces, the ds_ops in realtek_priv is > no longer necessary. Now, the realtek_variant.ds_ops can be used > directly. > > The realtek_priv.setup_interface() has been removed as we can directly > call the new common function. > > The switch unregistration and the MDIO node decrement in refcount were > moved into realtek_common_remove() as both interfaces now need to put > the MDIO node. > > [1] https://lkml.kernel.org/netdev/20220630200423.tieprdu5fpabflj7@bang-olufsen.dk/T/ > [2] https://lkml.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/T/#u > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > --- > drivers/net/dsa/realtek/realtek-common.c | 87 +++++++++++++++++++++++- > drivers/net/dsa/realtek/realtek-common.h | 1 + > drivers/net/dsa/realtek/realtek-mdio.c | 6 -- > drivers/net/dsa/realtek/realtek-smi.c | 68 ------------------ > drivers/net/dsa/realtek/realtek.h | 5 +- > drivers/net/dsa/realtek/rtl8365mb.c | 49 ++----------- > drivers/net/dsa/realtek/rtl8366rb.c | 52 ++------------ > 7 files changed, 100 insertions(+), 168 deletions(-) > > diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c > index bf3933a99072..b1f0095d5bce 100644 > --- a/drivers/net/dsa/realtek/realtek-common.c > +++ b/drivers/net/dsa/realtek/realtek-common.c > @@ -1,6 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0+ > > #include <linux/module.h> > +#include <linux/of_mdio.h> > > #include "realtek.h" > #include "realtek-common.h" > @@ -21,6 +22,85 @@ void realtek_common_unlock(void *ctx) > } > EXPORT_SYMBOL_GPL(realtek_common_unlock); > > +static int realtek_common_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 realtek_common_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); > +} > + > +int realtek_common_setup_user_mdio(struct dsa_switch *ds) > +{ > + const char *compatible = "realtek,smi-mdio"; No need to put this in its own variable, it makes the code harder to read. > + 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; > + } > + } > + > + 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 * Stray *, put a full stop. > + */ > + 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 in use" is a very cryptic warning, can you not just warn about a missing phy-handle, since that is what is expected? > + ds->user_mii_bus = priv->user_mii_bus; > + break; > + } > + > + ret = devm_of_mdiobus_register(priv->dev, priv->user_mii_bus, mdio_np); You use devres here, but this means the mdiobus will outlive the switch after dsa_switch_teardown(). I don't really know if this can cause any unwanted runtime behaviour, but the code feels unbalanced. > + if (ret) { > + dev_err(priv->dev, "unable to register MDIO bus %s\n", > + priv->user_mii_bus->id); > + goto err_put_node; > + } > + > + return 0; > + > +err_put_node: > + of_node_put(mdio_np); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(realtek_common_setup_user_mdio); > + > /* sets up driver private data struct, sets up regmaps, parse common device-tree > * properties and finally issues a hardware reset. > */ > @@ -108,7 +188,7 @@ int realtek_common_register_switch(struct realtek_priv *priv) > > priv->ds->priv = priv; > priv->ds->dev = priv->dev; > - priv->ds->ops = priv->ds_ops; > + priv->ds->ops = priv->variant->ds_ops; > priv->ds->num_ports = priv->num_ports; > > ret = dsa_register_switch(priv->ds); > @@ -126,6 +206,11 @@ void realtek_common_remove(struct realtek_priv *priv) > if (!priv) > return; > > + dsa_unregister_switch(priv->ds); Wait, wasn't this supposed to belong in the previous patch that added realtek_common_remove? Why is it being put here in this patch? Regarding the series as a whole, I still think all you really need is something like this: - realtek_common_probe - realtek_common_remove (if there is actually anything to do?) - realtek_common_setup_user_mdio - realtek_common_teardown_user_mdio The chip variant drivers can then do: /* Interface-agnostic chip driver code */ rtl836xxx_setup() { // ... realtek_common_setup_user_mdio(); // ... } rtl836xxx_teardown() { // ... realtek_common_teardown_user_mdio(); // ... } rtl836xxx_probe() { // assert // enable regulators // deassert reset // do what was in detect() before dsa_register_switch(priv->ds); } rtl836xxx_remove() { dsa_unregister_switch(); // assert reset again } /* SMI boilerplate */ rtl836xxx_smi_probe() { priv = realtek_smi_probe(); return rtl836xxx_probe(priv); } rtl836xxx_smi_remove() { rtl836xxx_remove(); realtek_smi_remove(); // if needed } /* MDIO boilerplate */ rtl836xxx_mdio_probe() { priv = realtek_mdio_probe(); return rtl836xxx_probe(priv); } rtl836xxx_mdio_remove() { rtl836xxx_remove(); realtek_mdio_remove(); // if needed } And your interface probe functions: realtek_smi_probe() { priv = realtek_common_probe(); // SMI specific setup return priv; } realtek_mdio_probe() { priv = realtek_common_probe(); // MDIO specific setup return priv; } Am I missing something? I'm open to other suggestions but I can't help but feel that the current proposal is half-baked. As a side note: I also think this will make it a bit easier to reason about the ownership of variables in struct realtek_priv. Ultimately its contents ought all to belong to the code which ends up in the realtek-dsa IMO. With a little effort, the rest can be moved into the variant-specific private data structs. And cases like realtek_priv::num_ports can be removed completely. But that cleanup is for another time. > + > + if (priv->user_mii_bus) > + of_node_put(priv->user_mii_bus->dev.of_node); I think you should undo your work in the corresponding undo function. You set up priv->user_mii_bus in ds_ops::setup, so you should undo any such stuff in ds_ops::teardown as well. > + > /* leave the device reset asserted */ > if (priv->reset) > gpiod_set_value(priv->reset, 1); > diff --git a/drivers/net/dsa/realtek/realtek-common.h b/drivers/net/dsa/realtek/realtek-common.h > index 518d091ff496..b1c2a50d85cd 100644 > --- a/drivers/net/dsa/realtek/realtek-common.h > +++ b/drivers/net/dsa/realtek/realtek-common.h > @@ -7,6 +7,7 @@ > > void realtek_common_lock(void *ctx); > void realtek_common_unlock(void *ctx); > +int realtek_common_setup_user_mdio(struct dsa_switch *ds); > struct realtek_priv * > realtek_common_probe(struct device *dev, struct regmap_config rc, > struct regmap_config rc_nolock); > diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c > index 967f6c1e8df0..e2b5432eeb26 100644 > --- a/drivers/net/dsa/realtek/realtek-mdio.c > +++ b/drivers/net/dsa/realtek/realtek-mdio.c > @@ -142,7 +142,6 @@ int realtek_mdio_probe(struct mdio_device *mdiodev) > priv->bus = mdiodev->bus; > priv->mdio_addr = mdiodev->addr; > priv->write_reg_noack = realtek_mdio_write; > - priv->ds_ops = priv->variant->ds_ops_mdio; > > ret = realtek_common_register_switch(priv); > if (ret) > @@ -156,11 +155,6 @@ void realtek_mdio_remove(struct mdio_device *mdiodev) > { > struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev); > > - if (!priv) > - return; > - > - dsa_unregister_switch(priv->ds); > - > realtek_common_remove(priv); > } > EXPORT_SYMBOL_GPL(realtek_mdio_remove); > diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c > index 2b2c6e34bae5..383689163057 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> > @@ -339,63 +338,6 @@ static const struct regmap_config realtek_smi_nolock_regmap_config = { > .disable_locking = true, > }; > > -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; > - > - mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-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->dev.of_node = mdio_np; > - priv->user_mii_bus->parent = priv->dev; > - ds->user_mii_bus = priv->user_mii_bus; > - > - 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; > - } > - > - return 0; > - > -err_put_node: > - of_node_put(mdio_np); > - > - return ret; > -} > - > int realtek_smi_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -417,8 +359,6 @@ 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->ds_ops = priv->variant->ds_ops_smi; > > ret = realtek_common_register_switch(priv); > if (ret) > @@ -432,14 +372,6 @@ void realtek_smi_remove(struct platform_device *pdev) > { > struct realtek_priv *priv = platform_get_drvdata(pdev); > > - if (!priv) > - return; > - > - dsa_unregister_switch(priv->ds); > - > - if (priv->user_mii_bus) > - of_node_put(priv->user_mii_bus->dev.of_node); > - > realtek_common_remove(priv); > } > EXPORT_SYMBOL_GPL(realtek_smi_remove); > diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h > index fbd0616c1df3..7af6dcc1bb24 100644 > --- a/drivers/net/dsa/realtek/realtek.h > +++ b/drivers/net/dsa/realtek/realtek.h > @@ -60,7 +60,6 @@ struct realtek_priv { > > spinlock_t lock; /* Locks around command writes */ > struct dsa_switch *ds; > - const struct dsa_switch_ops *ds_ops; > struct irq_domain *irqdomain; > bool leds_disabled; > > @@ -71,7 +70,6 @@ struct realtek_priv { > struct rtl8366_mib_counter *mib_counters; > > const struct realtek_ops *ops; > - int (*setup_interface)(struct dsa_switch *ds); > int (*write_reg_noack)(void *ctx, u32 addr, u32 data); > > int vlan_enabled; > @@ -115,8 +113,7 @@ struct realtek_ops { > }; > > struct realtek_variant { > - const struct dsa_switch_ops *ds_ops_smi; > - const struct dsa_switch_ops *ds_ops_mdio; > + const struct dsa_switch_ops *ds_ops; > const struct realtek_ops *ops; > unsigned int clk_delay; > u8 cmd_read; > diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c > index 58ec057b6c32..e890ad113ba3 100644 > --- a/drivers/net/dsa/realtek/rtl8365mb.c > +++ b/drivers/net/dsa/realtek/rtl8365mb.c > @@ -828,17 +828,6 @@ static int rtl8365mb_phy_write(struct realtek_priv *priv, int phy, int regnum, > return 0; > } > > -static int rtl8365mb_dsa_phy_read(struct dsa_switch *ds, int phy, int regnum) > -{ > - return rtl8365mb_phy_read(ds->priv, phy, regnum); > -} > - > -static int rtl8365mb_dsa_phy_write(struct dsa_switch *ds, int phy, int regnum, > - u16 val) > -{ > - return rtl8365mb_phy_write(ds->priv, phy, regnum, val); > -} > - > static const struct rtl8365mb_extint * > rtl8365mb_get_port_extint(struct realtek_priv *priv, int port) > { > @@ -2017,12 +2006,10 @@ static int rtl8365mb_setup(struct dsa_switch *ds) > if (ret) > goto out_teardown_irq; > > - if (priv->setup_interface) { > - ret = priv->setup_interface(ds); > - if (ret) { > - dev_err(priv->dev, "could not set up MDIO bus\n"); > - goto out_teardown_irq; > - } > + ret = realtek_common_setup_user_mdio(ds); > + if (ret) { > + dev_err(priv->dev, "could not set up MDIO bus\n"); > + goto out_teardown_irq; > } > > /* Start statistics counter polling */ > @@ -2116,28 +2103,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv) > return 0; > } > > -static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = { > - .get_tag_protocol = rtl8365mb_get_tag_protocol, > - .change_tag_protocol = rtl8365mb_change_tag_protocol, > - .setup = rtl8365mb_setup, > - .teardown = rtl8365mb_teardown, > - .phylink_get_caps = rtl8365mb_phylink_get_caps, > - .phylink_mac_config = rtl8365mb_phylink_mac_config, > - .phylink_mac_link_down = rtl8365mb_phylink_mac_link_down, > - .phylink_mac_link_up = rtl8365mb_phylink_mac_link_up, > - .port_stp_state_set = rtl8365mb_port_stp_state_set, > - .get_strings = rtl8365mb_get_strings, > - .get_ethtool_stats = rtl8365mb_get_ethtool_stats, > - .get_sset_count = rtl8365mb_get_sset_count, > - .get_eth_phy_stats = rtl8365mb_get_phy_stats, > - .get_eth_mac_stats = rtl8365mb_get_mac_stats, > - .get_eth_ctrl_stats = rtl8365mb_get_ctrl_stats, > - .get_stats64 = rtl8365mb_get_stats64, > - .port_change_mtu = rtl8365mb_port_change_mtu, > - .port_max_mtu = rtl8365mb_port_max_mtu, > -}; > - > -static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = { > +static const struct dsa_switch_ops rtl8365mb_switch_ops = { > .get_tag_protocol = rtl8365mb_get_tag_protocol, > .change_tag_protocol = rtl8365mb_change_tag_protocol, > .setup = rtl8365mb_setup, > @@ -2146,8 +2112,6 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = { > .phylink_mac_config = rtl8365mb_phylink_mac_config, > .phylink_mac_link_down = rtl8365mb_phylink_mac_link_down, > .phylink_mac_link_up = rtl8365mb_phylink_mac_link_up, > - .phy_read = rtl8365mb_dsa_phy_read, > - .phy_write = rtl8365mb_dsa_phy_write, > .port_stp_state_set = rtl8365mb_port_stp_state_set, > .get_strings = rtl8365mb_get_strings, > .get_ethtool_stats = rtl8365mb_get_ethtool_stats, > @@ -2167,8 +2131,7 @@ static const struct realtek_ops rtl8365mb_ops = { > }; > > const struct realtek_variant rtl8365mb_variant = { > - .ds_ops_smi = &rtl8365mb_switch_ops_smi, > - .ds_ops_mdio = &rtl8365mb_switch_ops_mdio, > + .ds_ops = &rtl8365mb_switch_ops, > .ops = &rtl8365mb_ops, > .clk_delay = 10, > .cmd_read = 0xb9, > diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c > index e60a0a81d426..56619aa592ec 100644 > --- a/drivers/net/dsa/realtek/rtl8366rb.c > +++ b/drivers/net/dsa/realtek/rtl8366rb.c > @@ -1027,12 +1027,10 @@ static int rtl8366rb_setup(struct dsa_switch *ds) > if (ret) > dev_info(priv->dev, "no interrupt support\n"); > > - if (priv->setup_interface) { > - ret = priv->setup_interface(ds); > - if (ret) { > - dev_err(priv->dev, "could not set up MDIO bus\n"); > - return -ENODEV; > - } > + ret = realtek_common_setup_user_mdio(ds); > + if (ret) { > + dev_err(priv->dev, "could not set up MDIO bus\n"); > + return -ENODEV; > } > > return 0; > @@ -1772,17 +1770,6 @@ static int rtl8366rb_phy_write(struct realtek_priv *priv, int phy, int regnum, > return ret; > } > > -static int rtl8366rb_dsa_phy_read(struct dsa_switch *ds, int phy, int regnum) > -{ > - return rtl8366rb_phy_read(ds->priv, phy, regnum); > -} > - > -static int rtl8366rb_dsa_phy_write(struct dsa_switch *ds, int phy, int regnum, > - u16 val) > -{ > - return rtl8366rb_phy_write(ds->priv, phy, regnum, val); > -} > - > static int rtl8366rb_reset_chip(struct realtek_priv *priv) > { > int timeout = 10; > @@ -1848,35 +1835,9 @@ static int rtl8366rb_detect(struct realtek_priv *priv) > return 0; > } > > -static const struct dsa_switch_ops rtl8366rb_switch_ops_smi = { > - .get_tag_protocol = rtl8366_get_tag_protocol, > - .setup = rtl8366rb_setup, > - .phylink_get_caps = rtl8366rb_phylink_get_caps, > - .phylink_mac_link_up = rtl8366rb_mac_link_up, > - .phylink_mac_link_down = rtl8366rb_mac_link_down, > - .get_strings = rtl8366_get_strings, > - .get_ethtool_stats = rtl8366_get_ethtool_stats, > - .get_sset_count = rtl8366_get_sset_count, > - .port_bridge_join = rtl8366rb_port_bridge_join, > - .port_bridge_leave = rtl8366rb_port_bridge_leave, > - .port_vlan_filtering = rtl8366rb_vlan_filtering, > - .port_vlan_add = rtl8366_vlan_add, > - .port_vlan_del = rtl8366_vlan_del, > - .port_enable = rtl8366rb_port_enable, > - .port_disable = rtl8366rb_port_disable, > - .port_pre_bridge_flags = rtl8366rb_port_pre_bridge_flags, > - .port_bridge_flags = rtl8366rb_port_bridge_flags, > - .port_stp_state_set = rtl8366rb_port_stp_state_set, > - .port_fast_age = rtl8366rb_port_fast_age, > - .port_change_mtu = rtl8366rb_change_mtu, > - .port_max_mtu = rtl8366rb_max_mtu, > -}; > - > -static const struct dsa_switch_ops rtl8366rb_switch_ops_mdio = { > +static const struct dsa_switch_ops rtl8366rb_switch_ops = { > .get_tag_protocol = rtl8366_get_tag_protocol, > .setup = rtl8366rb_setup, > - .phy_read = rtl8366rb_dsa_phy_read, > - .phy_write = rtl8366rb_dsa_phy_write, > .phylink_get_caps = rtl8366rb_phylink_get_caps, > .phylink_mac_link_up = rtl8366rb_mac_link_up, > .phylink_mac_link_down = rtl8366rb_mac_link_down, > @@ -1915,8 +1876,7 @@ static const struct realtek_ops rtl8366rb_ops = { > }; > > const struct realtek_variant rtl8366rb_variant = { > - .ds_ops_smi = &rtl8366rb_switch_ops_smi, > - .ds_ops_mdio = &rtl8366rb_switch_ops_mdio, > + .ds_ops = &rtl8366rb_switch_ops, > .ops = &rtl8366rb_ops, > .clk_delay = 10, > .cmd_read = 0xa9, > -- > 2.43.0 >
> On Wed, Dec 20, 2023 at 01:24:28AM -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 common module. All references to SMI have been eliminated, with > > the exception of the compatible string, which will continue to function > > as before. > > > > The realtek-mdio will now use this driver instead of the generic DSA > > driver ("dsa user smi"), which should not be used with OF[1]. > > > > There was a change in how the driver looks for the MDIO node in the > > device tree. Now, it first checks for a child node named "mdio," which > > is required by both interfaces in binding docs but used previously only > > by realtek-mdio. If the node is not found, it will also look for a > > compatible string, required only by SMI-connected devices in binding > > docs and compatible with the old realtek-smi behavior. > > > > The line assigning dev.of_node in mdio_bus has been removed since the > > subsequent of_mdiobus_register will always overwrite it. > > > > ds->user_mii_bus is only defined if all user ports do not declare a > > phy-handle, providing a warning about the erroneous device tree[2]. > > > > With a single ds_ops for both interfaces, the ds_ops in realtek_priv is > > no longer necessary. Now, the realtek_variant.ds_ops can be used > > directly. > > > > The realtek_priv.setup_interface() has been removed as we can directly > > call the new common function. > > > > The switch unregistration and the MDIO node decrement in refcount were > > moved into realtek_common_remove() as both interfaces now need to put > > the MDIO node. > > > > [1] https://lkml.kernel.org/netdev/20220630200423.tieprdu5fpabflj7@bang-olufsen.dk/T/ > > [2] https://lkml.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/T/#u > > > > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> > > --- > > drivers/net/dsa/realtek/realtek-common.c | 87 +++++++++++++++++++++++- > > drivers/net/dsa/realtek/realtek-common.h | 1 + > > drivers/net/dsa/realtek/realtek-mdio.c | 6 -- > > drivers/net/dsa/realtek/realtek-smi.c | 68 ------------------ > > drivers/net/dsa/realtek/realtek.h | 5 +- > > drivers/net/dsa/realtek/rtl8365mb.c | 49 ++----------- > > drivers/net/dsa/realtek/rtl8366rb.c | 52 ++------------ > > 7 files changed, 100 insertions(+), 168 deletions(-) > > > > diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c > > index bf3933a99072..b1f0095d5bce 100644 > > --- a/drivers/net/dsa/realtek/realtek-common.c > > +++ b/drivers/net/dsa/realtek/realtek-common.c > > @@ -1,6 +1,7 @@ > > // SPDX-License-Identifier: GPL-2.0+ > > > > #include <linux/module.h> > > +#include <linux/of_mdio.h> > > > > #include "realtek.h" > > #include "realtek-common.h" > > @@ -21,6 +22,85 @@ void realtek_common_unlock(void *ctx) > > } > > EXPORT_SYMBOL_GPL(realtek_common_unlock); > > > > +static int realtek_common_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 realtek_common_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); > > +} > > + > > +int realtek_common_setup_user_mdio(struct dsa_switch *ds) > > +{ > > + const char *compatible = "realtek,smi-mdio"; > > No need to put this in its own variable, it makes the code harder to read. OK. Without a warning message that would use it again, it is better to simply use the literal. > > + 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; > > + } > > + } > > + > > + 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 * > > Stray *, put a full stop. OK > > > + */ > > + 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 in use" is a very cryptic warning, can you not just > warn about a missing phy-handle, since that is what is expected? I was struggling to fit an informative message in the 80 cols limit. However, kernel messages are not the place for whys, just whats. I'll change to: dev_warn(priv->dev, "'%s' should have a phy-handle", dp->name); > > + ds->user_mii_bus = priv->user_mii_bus; > > + break; > > + } > > + > > + ret = devm_of_mdiobus_register(priv->dev, priv->user_mii_bus, mdio_np); > > You use devres here, but this means the mdiobus will outlive the switch > after dsa_switch_teardown(). I don't really know if this can cause any > unwanted runtime behaviour, but the code feels unbalanced. devm_* functions prepare objects to be destroyed/unregistered/removed when a parent device is gone. So, if you use devm_* anywhere but in the probe function, it might look like it is unbalanced because the unwinding process happens automatically, outside the driver control. So, technically, it will always be at least a little bit unbalanced. However, the guarantee it will be cleaned and in the correct order is worth it. Vladimir suggested that new drivers should split the MDIO bus driver from the DSA driver. I believe it is expected that mdiobus will outlive the switch and it could also be registered before the switch is registered or even allocated. We just need it to be registered before ports are ready (dsa_tree_setup_ports?). The switch setup is just the last opportunity we have to register an MDIO bus. Would it be better if we move the realtek_common_setup_user_mdio call from variants to just before the switch is registered in realtek_common_register_switch? I didn't test it but it should work. > > + if (ret) { > > + dev_err(priv->dev, "unable to register MDIO bus %s\n", > > + priv->user_mii_bus->id); > > + goto err_put_node; > > + } > > + > > + return 0; > > + > > +err_put_node: > > + of_node_put(mdio_np); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(realtek_common_setup_user_mdio); > > + > > /* sets up driver private data struct, sets up regmaps, parse common device-tree > > * properties and finally issues a hardware reset. > > */ > > @@ -108,7 +188,7 @@ int realtek_common_register_switch(struct realtek_priv *priv) > > > > priv->ds->priv = priv; > > priv->ds->dev = priv->dev; > > - priv->ds->ops = priv->ds_ops; > > + priv->ds->ops = priv->variant->ds_ops; > > priv->ds->num_ports = priv->num_ports; > > > > ret = dsa_register_switch(priv->ds); > > @@ -126,6 +206,11 @@ void realtek_common_remove(struct realtek_priv *priv) > > if (!priv) > > return; > > > > + dsa_unregister_switch(priv->ds); > > Wait, wasn't this supposed to belong in the previous patch that added > realtek_common_remove? Why is it being put here in this patch? It wasn't. The of_node_put, before this patch, is only used by realtek-smi. So, for realtek-smi, I need to add the put between the dsa_unregister_switch() and the realtek_common_remove(), when we lose the mdiobus reference. I thought it wasn't worth it to create a realtek_common_unregister() that simply calls dsa_unregister_switch(). That's why I left the dsa_unregister_switch (both) and the of_node_put (realtek-smi) on each interface code. With this commit, realtek-mdio will also need to put the node, so it makes sense to move the common code to the realtek_common_remove(). If we could put the node just after the registration (another patch I sent to OF MDIO API), we wouldn't even need to think about the mdio bus during removal. It would just go in peace. > Regarding the series as a whole, I still think all you really need is > something like this: > > - realtek_common_probe > - realtek_common_remove (if there is actually anything to do?) > - realtek_common_setup_user_mdio > - realtek_common_teardown_user_mdio > > The chip variant drivers can then do: > > /* Interface-agnostic chip driver code */ > > rtl836xxx_setup() { > // ... > realtek_common_setup_user_mdio(); > // ... > } > > rtl836xxx_teardown() { > // ... > realtek_common_teardown_user_mdio(); > // ... > } > > rtl836xxx_probe() { > // assert > // enable regulators > // deassert reset > // do what was in detect() before > dsa_register_switch(priv->ds); > } > > rtl836xxx_remove() { > dsa_unregister_switch(); > // assert reset again > } > > /* SMI boilerplate */ > > rtl836xxx_smi_probe() { > priv = realtek_smi_probe(); > return rtl836xxx_probe(priv); > } > > rtl836xxx_smi_remove() { > rtl836xxx_remove(); > realtek_smi_remove(); // if needed > } > > /* MDIO boilerplate */ > > rtl836xxx_mdio_probe() { > priv = realtek_mdio_probe(); > return rtl836xxx_probe(priv); > } > > rtl836xxx_mdio_remove() { > rtl836xxx_remove(); > realtek_mdio_remove(); // if needed > } > > And your interface probe functions: > > realtek_smi_probe() { > priv = realtek_common_probe(); > // SMI specific setup > return priv; > } > > realtek_mdio_probe() { > priv = realtek_common_probe(); > // MDIO specific setup > return priv; > } > > Am I missing something? I'm open to other suggestions but I can't help > but feel that the current proposal is half-baked. Until we have some specific code for probe/remove that is both interface and variant specific, it just creates some new functions that have the same code. Up to this point, the detect is responsible for the variant-specific logic. We could rename it to match a broader role, like resetting the device before detecting it. However, it will introduce another unbalance as the "detect" will reset the switch but, without an "undetect-like" function, the common remove will leave it asserted. I'm OK with that as resetting and leaving the device reset asserted, although using the same tools, have different objectives. But we can discuss that in the 3/7 patch. While we use devm, there is not much need for tearing down the MDIO (except, for now, putting the node). I don't think we should put the node during switch teardown. At this point, all user ports are down and the mdiobus might not be reachable but, maybe, something might still interact with the mdio bus (sysfs? a queued syscall?). Even during driver removal, mdiobus will still be registered and allocated and it is still odd for me to put a node still referenced in a registered mdio bus device. At least, it is the closest point we have before mdiobus_unregister. The correct place to put it would be between mdiobus_unregister and mdiobus_free, both called by devm on device destruction and out of our control. There is no devm_of_put_node and creating a new devm callback to put the node just looks hacky. All this just reinforces my belief that of_mdiobus_register should get the node (and mdio_unregister put it), especially with devm where the unregister happens out of the driver control. I just don't know if I did that right. > As a side note: I also think this will make it a bit easier to reason > about the ownership of variables in struct realtek_priv. Ultimately its > contents ought all to belong to the code which ends up in the > realtek-dsa IMO. With a little effort, the rest can be moved into the > variant-specific private data structs. And cases like > realtek_priv::num_ports can be removed completely. But that cleanup is > for another time. Please, keep it for another time. I wish I don't end up reimplementing all the driver just to add a reset control. The driver might need more love than I'm giving but it would be easier after this series gets in. > > + > > + if (priv->user_mii_bus) > > + of_node_put(priv->user_mii_bus->dev.of_node); > > I think you should undo your work in the corresponding undo > function. You set up priv->user_mii_bus in ds_ops::setup, so you should > undo any such stuff in ds_ops::teardown as well. As I said, priv->user_mii_bus could (or should?) be set outside dsa setup and priv->user_mii_bus->dev.of_node should actually be put between mdiobus_unregister and mdiobus_free, that all called by devm and out of our control. Here is just the closest we have before devm does its job. > > > + > > /* leave the device reset asserted */ > > if (priv->reset) > > gpiod_set_value(priv->reset, 1); > > diff --git a/drivers/net/dsa/realtek/realtek-common.h b/drivers/net/dsa/realtek/realtek-common.h > > index 518d091ff496..b1c2a50d85cd 100644 > > --- a/drivers/net/dsa/realtek/realtek-common.h > > +++ b/drivers/net/dsa/realtek/realtek-common.h > > @@ -7,6 +7,7 @@ > > > > void realtek_common_lock(void *ctx); > > void realtek_common_unlock(void *ctx); > > +int realtek_common_setup_user_mdio(struct dsa_switch *ds); > > struct realtek_priv * > > realtek_common_probe(struct device *dev, struct regmap_config rc, > > struct regmap_config rc_nolock); > > diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c > > index 967f6c1e8df0..e2b5432eeb26 100644 > > --- a/drivers/net/dsa/realtek/realtek-mdio.c > > +++ b/drivers/net/dsa/realtek/realtek-mdio.c > > @@ -142,7 +142,6 @@ int realtek_mdio_probe(struct mdio_device *mdiodev) > > priv->bus = mdiodev->bus; > > priv->mdio_addr = mdiodev->addr; > > priv->write_reg_noack = realtek_mdio_write; > > - priv->ds_ops = priv->variant->ds_ops_mdio; > > > > ret = realtek_common_register_switch(priv); > > if (ret) > > @@ -156,11 +155,6 @@ void realtek_mdio_remove(struct mdio_device *mdiodev) > > { > > struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev); > > > > - if (!priv) > > - return; > > - > > - dsa_unregister_switch(priv->ds); > > - > > realtek_common_remove(priv); > > } > > EXPORT_SYMBOL_GPL(realtek_mdio_remove); > > diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c > > index 2b2c6e34bae5..383689163057 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> > > @@ -339,63 +338,6 @@ static const struct regmap_config realtek_smi_nolock_regmap_config = { > > .disable_locking = true, > > }; > > > > -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; > > - > > - mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-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->dev.of_node = mdio_np; > > - priv->user_mii_bus->parent = priv->dev; > > - ds->user_mii_bus = priv->user_mii_bus; > > - > > - 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; > > - } > > - > > - return 0; > > - > > -err_put_node: > > - of_node_put(mdio_np); > > - > > - return ret; > > -} > > - > > int realtek_smi_probe(struct platform_device *pdev) > > { > > struct device *dev = &pdev->dev; > > @@ -417,8 +359,6 @@ 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->ds_ops = priv->variant->ds_ops_smi; > > > > ret = realtek_common_register_switch(priv); > > if (ret) > > @@ -432,14 +372,6 @@ void realtek_smi_remove(struct platform_device *pdev) > > { > > struct realtek_priv *priv = platform_get_drvdata(pdev); > > > > - if (!priv) > > - return; > > - > > - dsa_unregister_switch(priv->ds); > > - > > - if (priv->user_mii_bus) > > - of_node_put(priv->user_mii_bus->dev.of_node); > > - > > realtek_common_remove(priv); > > } > > EXPORT_SYMBOL_GPL(realtek_smi_remove); > > diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h > > index fbd0616c1df3..7af6dcc1bb24 100644 > > --- a/drivers/net/dsa/realtek/realtek.h > > +++ b/drivers/net/dsa/realtek/realtek.h > > @@ -60,7 +60,6 @@ struct realtek_priv { > > > > spinlock_t lock; /* Locks around command writes */ > > struct dsa_switch *ds; > > - const struct dsa_switch_ops *ds_ops; > > struct irq_domain *irqdomain; > > bool leds_disabled; > > > > @@ -71,7 +70,6 @@ struct realtek_priv { > > struct rtl8366_mib_counter *mib_counters; > > > > const struct realtek_ops *ops; > > - int (*setup_interface)(struct dsa_switch *ds); > > int (*write_reg_noack)(void *ctx, u32 addr, u32 data); > > > > int vlan_enabled; > > @@ -115,8 +113,7 @@ struct realtek_ops { > > }; > > > > struct realtek_variant { > > - const struct dsa_switch_ops *ds_ops_smi; > > - const struct dsa_switch_ops *ds_ops_mdio; > > + const struct dsa_switch_ops *ds_ops; > > const struct realtek_ops *ops; > > unsigned int clk_delay; > > u8 cmd_read; > > diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c > > index 58ec057b6c32..e890ad113ba3 100644 > > --- a/drivers/net/dsa/realtek/rtl8365mb.c > > +++ b/drivers/net/dsa/realtek/rtl8365mb.c > > @@ -828,17 +828,6 @@ static int rtl8365mb_phy_write(struct realtek_priv *priv, int phy, int regnum, > > return 0; > > } > > > > -static int rtl8365mb_dsa_phy_read(struct dsa_switch *ds, int phy, int regnum) > > -{ > > - return rtl8365mb_phy_read(ds->priv, phy, regnum); > > -} > > - > > -static int rtl8365mb_dsa_phy_write(struct dsa_switch *ds, int phy, int regnum, > > - u16 val) > > -{ > > - return rtl8365mb_phy_write(ds->priv, phy, regnum, val); > > -} > > - > > static const struct rtl8365mb_extint * > > rtl8365mb_get_port_extint(struct realtek_priv *priv, int port) > > { > > @@ -2017,12 +2006,10 @@ static int rtl8365mb_setup(struct dsa_switch *ds) > > if (ret) > > goto out_teardown_irq; > > > > - if (priv->setup_interface) { > > - ret = priv->setup_interface(ds); > > - if (ret) { > > - dev_err(priv->dev, "could not set up MDIO bus\n"); > > - goto out_teardown_irq; > > - } > > + ret = realtek_common_setup_user_mdio(ds); > > + if (ret) { > > + dev_err(priv->dev, "could not set up MDIO bus\n"); > > + goto out_teardown_irq; > > } > > > > /* Start statistics counter polling */ > > @@ -2116,28 +2103,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv) > > return 0; > > } > > > > -static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = { > > - .get_tag_protocol = rtl8365mb_get_tag_protocol, > > - .change_tag_protocol = rtl8365mb_change_tag_protocol, > > - .setup = rtl8365mb_setup, > > - .teardown = rtl8365mb_teardown, > > - .phylink_get_caps = rtl8365mb_phylink_get_caps, > > - .phylink_mac_config = rtl8365mb_phylink_mac_config, > > - .phylink_mac_link_down = rtl8365mb_phylink_mac_link_down, > > - .phylink_mac_link_up = rtl8365mb_phylink_mac_link_up, > > - .port_stp_state_set = rtl8365mb_port_stp_state_set, > > - .get_strings = rtl8365mb_get_strings, > > - .get_ethtool_stats = rtl8365mb_get_ethtool_stats, > > - .get_sset_count = rtl8365mb_get_sset_count, > > - .get_eth_phy_stats = rtl8365mb_get_phy_stats, > > - .get_eth_mac_stats = rtl8365mb_get_mac_stats, > > - .get_eth_ctrl_stats = rtl8365mb_get_ctrl_stats, > > - .get_stats64 = rtl8365mb_get_stats64, > > - .port_change_mtu = rtl8365mb_port_change_mtu, > > - .port_max_mtu = rtl8365mb_port_max_mtu, > > -}; > > - > > -static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = { > > +static const struct dsa_switch_ops rtl8365mb_switch_ops = { > > .get_tag_protocol = rtl8365mb_get_tag_protocol, > > .change_tag_protocol = rtl8365mb_change_tag_protocol, > > .setup = rtl8365mb_setup, > > @@ -2146,8 +2112,6 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = { > > .phylink_mac_config = rtl8365mb_phylink_mac_config, > > .phylink_mac_link_down = rtl8365mb_phylink_mac_link_down, > > .phylink_mac_link_up = rtl8365mb_phylink_mac_link_up, > > - .phy_read = rtl8365mb_dsa_phy_read, > > - .phy_write = rtl8365mb_dsa_phy_write, > > .port_stp_state_set = rtl8365mb_port_stp_state_set, > > .get_strings = rtl8365mb_get_strings, > > .get_ethtool_stats = rtl8365mb_get_ethtool_stats, > > @@ -2167,8 +2131,7 @@ static const struct realtek_ops rtl8365mb_ops = { > > }; > > > > const struct realtek_variant rtl8365mb_variant = { > > - .ds_ops_smi = &rtl8365mb_switch_ops_smi, > > - .ds_ops_mdio = &rtl8365mb_switch_ops_mdio, > > + .ds_ops = &rtl8365mb_switch_ops, > > .ops = &rtl8365mb_ops, > > .clk_delay = 10, > > .cmd_read = 0xb9, > > diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c > > index e60a0a81d426..56619aa592ec 100644 > > --- a/drivers/net/dsa/realtek/rtl8366rb.c > > +++ b/drivers/net/dsa/realtek/rtl8366rb.c > > @@ -1027,12 +1027,10 @@ static int rtl8366rb_setup(struct dsa_switch *ds) > > if (ret) > > dev_info(priv->dev, "no interrupt support\n"); > > > > - if (priv->setup_interface) { > > - ret = priv->setup_interface(ds); > > - if (ret) { > > - dev_err(priv->dev, "could not set up MDIO bus\n"); > > - return -ENODEV; > > - } > > + ret = realtek_common_setup_user_mdio(ds); > > + if (ret) { > > + dev_err(priv->dev, "could not set up MDIO bus\n"); > > + return -ENODEV; > > } > > > > return 0; > > @@ -1772,17 +1770,6 @@ static int rtl8366rb_phy_write(struct realtek_priv *priv, int phy, int regnum, > > return ret; > > } > > > > -static int rtl8366rb_dsa_phy_read(struct dsa_switch *ds, int phy, int regnum) > > -{ > > - return rtl8366rb_phy_read(ds->priv, phy, regnum); > > -} > > - > > -static int rtl8366rb_dsa_phy_write(struct dsa_switch *ds, int phy, int regnum, > > - u16 val) > > -{ > > - return rtl8366rb_phy_write(ds->priv, phy, regnum, val); > > -} > > - > > static int rtl8366rb_reset_chip(struct realtek_priv *priv) > > { > > int timeout = 10; > > @@ -1848,35 +1835,9 @@ static int rtl8366rb_detect(struct realtek_priv *priv) > > return 0; > > } > > > > -static const struct dsa_switch_ops rtl8366rb_switch_ops_smi = { > > - .get_tag_protocol = rtl8366_get_tag_protocol, > > - .setup = rtl8366rb_setup, > > - .phylink_get_caps = rtl8366rb_phylink_get_caps, > > - .phylink_mac_link_up = rtl8366rb_mac_link_up, > > - .phylink_mac_link_down = rtl8366rb_mac_link_down, > > - .get_strings = rtl8366_get_strings, > > - .get_ethtool_stats = rtl8366_get_ethtool_stats, > > - .get_sset_count = rtl8366_get_sset_count, > > - .port_bridge_join = rtl8366rb_port_bridge_join, > > - .port_bridge_leave = rtl8366rb_port_bridge_leave, > > - .port_vlan_filtering = rtl8366rb_vlan_filtering, > > - .port_vlan_add = rtl8366_vlan_add, > > - .port_vlan_del = rtl8366_vlan_del, > > - .port_enable = rtl8366rb_port_enable, > > - .port_disable = rtl8366rb_port_disable, > > - .port_pre_bridge_flags = rtl8366rb_port_pre_bridge_flags, > > - .port_bridge_flags = rtl8366rb_port_bridge_flags, > > - .port_stp_state_set = rtl8366rb_port_stp_state_set, > > - .port_fast_age = rtl8366rb_port_fast_age, > > - .port_change_mtu = rtl8366rb_change_mtu, > > - .port_max_mtu = rtl8366rb_max_mtu, > > -}; > > - > > -static const struct dsa_switch_ops rtl8366rb_switch_ops_mdio = { > > +static const struct dsa_switch_ops rtl8366rb_switch_ops = { > > .get_tag_protocol = rtl8366_get_tag_protocol, > > .setup = rtl8366rb_setup, > > - .phy_read = rtl8366rb_dsa_phy_read, > > - .phy_write = rtl8366rb_dsa_phy_write, > > .phylink_get_caps = rtl8366rb_phylink_get_caps, > > .phylink_mac_link_up = rtl8366rb_mac_link_up, > > .phylink_mac_link_down = rtl8366rb_mac_link_down, > > @@ -1915,8 +1876,7 @@ static const struct realtek_ops rtl8366rb_ops = { > > }; > > > > const struct realtek_variant rtl8366rb_variant = { > > - .ds_ops_smi = &rtl8366rb_switch_ops_smi, > > - .ds_ops_mdio = &rtl8366rb_switch_ops_mdio, > > + .ds_ops = &rtl8366rb_switch_ops, > > .ops = &rtl8366rb_ops, > > .clk_delay = 10, > > .cmd_read = 0xa9, > > -- > > 2.43.0 > >
On Wed, Dec 20, 2023 at 01:51:01AM -0300, Luiz Angelo Daros de Luca wrote: > Hello Vladimir, > > I'm sorry to bother you again but I would like your attention for two > points that I'm not completely sure about. Ok. Please trim the quoted text from your replies to just what's relevant. It's easy to scroll past the new bits. > > +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 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). > > + 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. Thus, because the dsa_user_phy_connect() behavior given by the DSA core through ds->user_mii_bus does not help in any valid circumstance, let's deactivate that possible code path and simplify the interaction between the driver and DSA. And then go on with the driver cleanup as if ds->user_mii_bus didn't exist. Makes sense? Parsing "phy-handle" is just absurdly complicated. > > + > > + 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; > > + } > > + > > + return 0; > > + > > +err_put_node: > > + of_node_put(mdio_np); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(realtek_common_setup_user_mdio);
On 21.12.2023 20:47, Vladimir Oltean wrote: > On Wed, Dec 20, 2023 at 01:51:01AM -0300, Luiz Angelo Daros de Luca wrote: >>> + 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. > > Thus, because the dsa_user_phy_connect() behavior given by the DSA core > through ds->user_mii_bus does not help in any valid circumstance, let's > deactivate that possible code path and simplify the interaction between > the driver and DSA. > > And then go on with the driver cleanup as if ds->user_mii_bus didn't exist. > > Makes sense? Parsing "phy-handle" is just absurdly complicated. I don't like the fact that the driver bails out if it doesn't find the "mdio" child node. This basically forces the hardware design to use the MDIO bus of the switch. Hardware designs which don't use the MDIO bus of the switch are perfectly valid. It looks to me that, to make all types of hardware designs work, we must not use ds->user_mii_bus for switch probes on OF. Case (2) is one of the cases of the ethernet controller lacking link definitions in OF so we should enforce link definitions on ethernet controllers. This way, we make sure all types of hardware designs work and are described in OF properly. Arınç
On Thu, Dec 21, 2023 at 09:34:52PM +0300, Arınç ÜNAL wrote: > On 21.12.2023 20:47, Vladimir Oltean wrote: > > 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 (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. > > I don't like the fact that the driver bails out if it doesn't find the > "mdio" child node. This basically forces the hardware design to use the > MDIO bus of the switch. Hardware designs which don't use the MDIO bus of > the switch are perfectly valid. > > It looks to me that, to make all types of hardware designs work, we must > not use ds->user_mii_bus for switch probes on OF. Case (2) is one of the > cases of the ethernet controller lacking link definitions in OF so we > should enforce link definitions on ethernet controllers. This way, we make > sure all types of hardware designs work and are described in OF properly. > > Arınç The bindings for the realtek switches can be extended in compatible ways, e.g. by making the 'mdio' node optional. If we want that to mean "there is no internal PHY that needs to be used", there is no better time than now to drop the driver's linkage to ds->user_mii_bus, while its bindings still strictly require an 'mdio' node. If we don't drop that linkage _before_ making 'mdio' optional, there is no way to disprove the existence of device trees which lack a link description on user ports (which is now possible). So the driver will always have to pay the penalty of mdiobus_register(ds->user_mii_bus), which will always enumerate the internal PHYs even if they will end up unused, as you say should be possible. Listing the MDIO bus in OF deactivates bus scanning, which speeds up probing and booting in most cases. There are other ways to reduce that PHY enumeration pain, like manually setting the bus->phy_mask and moving code around such that it gets executed only once in the presence of -EPROBE_DEFER. This is what Klaus Kudielka had to go through with mv88e6xxx, all because the Turris Omnia device tree lacks phy-handle to the internal PHYs, his boot time shot up by a wide margin. https://lore.kernel.org/lkml/449bde236c08d5ab5e54abd73b645d8b29955894.camel@gmail.com/ commit 2c7e46edbd03 ("net: dsa: mv88e6xxx: mask apparently non-existing phys during probing") commit 2cb0658d4f88 ("net: dsa: mv88e6xxx: move call to mv88e6xxx_mdios_register()") We support device trees with 'hidden' switch internal MDIO buses and it would be unwise to break them. But they are a self-inflicted pain and it would be even more unwise for me to go on record not discouraging their use. Honestly, I don't want any more of them.
On Fri, Dec 22, 2023 at 12:48:31PM +0200, Vladimir Oltean wrote: > On Thu, Dec 21, 2023 at 09:34:52PM +0300, Arınç ÜNAL wrote: > > On 21.12.2023 20:47, Vladimir Oltean wrote: > > > 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 (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. > > > > I don't like the fact that the driver bails out if it doesn't find the > > "mdio" child node. This basically forces the hardware design to use the > > MDIO bus of the switch. Hardware designs which don't use the MDIO bus of > > the switch are perfectly valid. > > > > It looks to me that, to make all types of hardware designs work, we must > > not use ds->user_mii_bus for switch probes on OF. Case (2) is one of the > > cases of the ethernet controller lacking link definitions in OF so we > > should enforce link definitions on ethernet controllers. This way, we make > > sure all types of hardware designs work and are described in OF properly. > > > > Arınç > > The bindings for the realtek switches can be extended in compatible ways, > e.g. by making the 'mdio' node optional. If we want that to mean "there > is no internal PHY that needs to be used", there is no better time than > now to drop the driver's linkage to ds->user_mii_bus, while its bindings > still strictly require an 'mdio' node. > > If we don't drop that linkage _before_ making 'mdio' optional, there > is no way to disprove the existence of device trees which lack a link > description on user ports (which is now possible). I strongly agree and I think that the direction you have suggested is crystal clear, Vladimir. Nothing prohibits us from relaxing the bindings later on to support whatever hardware Arınç is describing. But for my own understanding - and maybe this is more a question for Arınç since he brought it up up - what does this supposed hardware look like, where the internal MDIO bus is not used? Here are my two (probably wrong?) guesses: (1) you use the MDIO bus of the parent Ethernet controller and access the internal PHYs directly, hence the internal MDIO bus goes unused; (2) none of the internal PHYs are actually used, so only the so-called extension ports are available. I don't know if (1) really qualifies. And (2) is also a bit strange, because this family of switches has variants with up to only three extension ports, most often two, which doesn't make for much of a switch. So while I agree in theory with your remark Arınç, I'm just wondering if you had something specific in mind. Kind regards, Alvin
On 22.12.2023 13:48, Vladimir Oltean wrote: > On Thu, Dec 21, 2023 at 09:34:52PM +0300, Arınç ÜNAL wrote: >> On 21.12.2023 20:47, Vladimir Oltean wrote: >>> 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 (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. >> >> I don't like the fact that the driver bails out if it doesn't find the >> "mdio" child node. This basically forces the hardware design to use the >> MDIO bus of the switch. Hardware designs which don't use the MDIO bus of >> the switch are perfectly valid. >> >> It looks to me that, to make all types of hardware designs work, we must >> not use ds->user_mii_bus for switch probes on OF. Case (2) is one of the >> cases of the ethernet controller lacking link definitions in OF so we >> should enforce link definitions on ethernet controllers. This way, we make >> sure all types of hardware designs work and are described in OF properly. >> >> Arınç > > The bindings for the realtek switches can be extended in compatible ways, > e.g. by making the 'mdio' node optional. If we want that to mean "there > is no internal PHY that needs to be used", there is no better time than > now to drop the driver's linkage to ds->user_mii_bus, while its bindings > still strictly require an 'mdio' node. "There is no internal PHY that needs to be used" is not the right statement for all cases. The internal PHYs can be wired to another MDIO bus or they may be described as fixed-link which would mean using the MDIO bus to read link information from the PHYs becomes unnecessary. These may be very rare hardware designs to come across but they are valid hardware descriptions in OF. So "the MDIO bus of the switch is not being used for the purpose of reading/writing from/to the PHYs (and not necessarily internal PHYs)" is the correct statement. > > If we don't drop that linkage _before_ making 'mdio' optional, there > is no way to disprove the existence of device trees which lack a link > description on user ports (which is now possible). So the driver will > always have to pay the penalty of mdiobus_register(ds->user_mii_bus), > which will always enumerate the internal PHYs even if they will end up > unused, as you say should be possible. Listing the MDIO bus in OF > deactivates bus scanning, which speeds up probing and booting in most > cases. > > There are other ways to reduce that PHY enumeration pain, like manually > setting the bus->phy_mask and moving code around such that it gets > executed only once in the presence of -EPROBE_DEFER. This is what Klaus > Kudielka had to go through with mv88e6xxx, all because the Turris Omnia > device tree lacks phy-handle to the internal PHYs, his boot time shot up > by a wide margin. > https://lore.kernel.org/lkml/449bde236c08d5ab5e54abd73b645d8b29955894.camel@gmail.com/ > commit 2c7e46edbd03 ("net: dsa: mv88e6xxx: mask apparently non-existing phys during probing") > commit 2cb0658d4f88 ("net: dsa: mv88e6xxx: move call to mv88e6xxx_mdios_register()") > > We support device trees with 'hidden' switch internal MDIO buses and it > would be unwise to break them. But they are a self-inflicted pain and it > would be even more unwise for me to go on record not discouraging their use. > Honestly, I don't want any more of them. Looks like with the direction you're suggesting here, we can enforce link descriptions and, at the same time, support device trees with undescribed switch MDIO bus on DSA. So I see all this as a step in the right direction. So yeah, let's keep ds->user_mii_bus for switch probes on OF without the switch MDIO bus defined, provided these switches have an MDIO bus. We should also align all DSA subdrivers with this understanding. I will modify the MDIO bus patch I submitted for the MT7530 DSA subdriver accordingly. I was wondering of moving the MDIO bus registration from DSA subdrivers to the DSA core driver but probably it's not generic enough across switch models with multiple MDIO buses and whatnot to manage this. Arınç
On 22.12.2023 14:13, Alvin Šipraga wrote: > On Fri, Dec 22, 2023 at 12:48:31PM +0200, Vladimir Oltean wrote: >> On Thu, Dec 21, 2023 at 09:34:52PM +0300, Arınç ÜNAL wrote: >>> On 21.12.2023 20:47, Vladimir Oltean wrote: >>>> 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 (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. >>> >>> I don't like the fact that the driver bails out if it doesn't find the >>> "mdio" child node. This basically forces the hardware design to use the >>> MDIO bus of the switch. Hardware designs which don't use the MDIO bus of >>> the switch are perfectly valid. >>> >>> It looks to me that, to make all types of hardware designs work, we must >>> not use ds->user_mii_bus for switch probes on OF. Case (2) is one of the >>> cases of the ethernet controller lacking link definitions in OF so we >>> should enforce link definitions on ethernet controllers. This way, we make >>> sure all types of hardware designs work and are described in OF properly. >>> >>> Arınç >> >> The bindings for the realtek switches can be extended in compatible ways, >> e.g. by making the 'mdio' node optional. If we want that to mean "there >> is no internal PHY that needs to be used", there is no better time than >> now to drop the driver's linkage to ds->user_mii_bus, while its bindings >> still strictly require an 'mdio' node. >> >> If we don't drop that linkage _before_ making 'mdio' optional, there >> is no way to disprove the existence of device trees which lack a link >> description on user ports (which is now possible). > > I strongly agree and I think that the direction you have suggested is > crystal clear, Vladimir. Nothing prohibits us from relaxing the bindings > later on to support whatever hardware Arınç is describing. > > But for my own understanding - and maybe this is more a question for > Arınç since he brought it up up - what does this supposed hardware look > like, where the internal MDIO bus is not used? Here are my two (probably > wrong?) guesses: > > (1) you use the MDIO bus of the parent Ethernet controller and access > the internal PHYs directly, hence the internal MDIO bus goes unused; > > (2) none of the internal PHYs are actually used, so only the so-called > extension ports are available. > > I don't know if (1) really qualifies. And (2) is also a bit strange, > because this family of switches has variants with up to only three > extension ports, most often two, which doesn't make for much of a > switch. > > So while I agree in theory with your remark Arınç, I'm just wondering if > you had something specific in mind. I was speaking in the sense of all switches with CPU ports, which is controlled by the DSA subsystem on Linux. I am only stating the fact that if we don't take the literal approach with hardware descriptions on the driver implementation, there will always be cases where the drivers will fail to support certain hardware designs. Arınç
> 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
> >>>> 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 (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. > >>> > >>> I don't like the fact that the driver bails out if it doesn't find the > >>> "mdio" child node. This basically forces the hardware design to use the > >>> MDIO bus of the switch. Hardware designs which don't use the MDIO bus of > >>> the switch are perfectly valid. > >>> > >>> It looks to me that, to make all types of hardware designs work, we must > >>> not use ds->user_mii_bus for switch probes on OF. Case (2) is one of the > >>> cases of the ethernet controller lacking link definitions in OF so we > >>> should enforce link definitions on ethernet controllers. This way, we make > >>> sure all types of hardware designs work and are described in OF properly. > >>> > >>> Arınç > >> > >> The bindings for the realtek switches can be extended in compatible ways, > >> e.g. by making the 'mdio' node optional. If we want that to mean "there > >> is no internal PHY that needs to be used", there is no better time than > >> now to drop the driver's linkage to ds->user_mii_bus, while its bindings > >> still strictly require an 'mdio' node. > >> > >> If we don't drop that linkage _before_ making 'mdio' optional, there > >> is no way to disprove the existence of device trees which lack a link > >> description on user ports (which is now possible). > > > > I strongly agree and I think that the direction you have suggested is > > crystal clear, Vladimir. Nothing prohibits us from relaxing the bindings > > later on to support whatever hardware Arınç is describing. > > > > But for my own understanding - and maybe this is more a question for > > Arınç since he brought it up up - what does this supposed hardware look > > like, where the internal MDIO bus is not used? Here are my two (probably > > wrong?) guesses: > > > > (1) you use the MDIO bus of the parent Ethernet controller and access > > the internal PHYs directly, hence the internal MDIO bus goes unused; > > > > (2) none of the internal PHYs are actually used, so only the so-called > > extension ports are available. > > > > I don't know if (1) really qualifies. And (2) is also a bit strange, > > because this family of switches has variants with up to only three > > extension ports, most often two, which doesn't make for much of a > > switch. > > > > So while I agree in theory with your remark Arınç, I'm just wondering if > > you had something specific in mind. > > I was speaking in the sense of all switches with CPU ports, which is > controlled by the DSA subsystem on Linux. > > I am only stating the fact that if we don't take the literal approach with > hardware descriptions on the driver implementation, there will always be > cases where the drivers will fail to support certain hardware designs. Hi Arinç, The old code was already requiring a single switch child node describing the internal MDIO bus akin to binding docs. I believe what we use to match it, being the name or the compatible string property, wouldn't improve the diversity of HW we could support. This series doesn't want to solve all issues and limitations nor prepare the ground for different HWs. It's mostly a reorganization without nice new stuff. After this series, we could easily turn the mdio node optional, skipping the MDIO bus when not found. Anyway, if such HW appears just now, I believe we could simply workaround it by declaring an empty mdio node. Regards, Luiz
Hey Luiz. On 22.12.2023 23:28, Luiz Angelo Daros de Luca wrote: >> I was speaking in the sense of all switches with CPU ports, which is >> controlled by the DSA subsystem on Linux. >> >> I am only stating the fact that if we don't take the literal approach with >> hardware descriptions on the driver implementation, there will always be >> cases where the drivers will fail to support certain hardware designs. > > Hi Arinç, > > The old code was already requiring a single switch child node > describing the internal MDIO bus akin to binding docs. I believe what > we use to match it, being the name or the compatible string property, > wouldn't improve the diversity of HW we could support. This series > doesn't want to solve all issues and limitations nor prepare the > ground for different HWs. It's mostly a reorganization without nice > new stuff. > > After this series, we could easily turn the mdio node optional, > skipping the MDIO bus when not found. Anyway, if such HW appears just > now, I believe we could simply workaround it by declaring an empty > mdio node. I agree that there's no need to add new things to this patch series. Just address Vladimir's suggestions on the next version. You got into this rabbit hole because you were just trying to add reset controller support on the rtl8365mb driver, no? Geez! Arınç
On Fri, Dec 22, 2023 at 05:03:38PM -0300, Luiz Angelo Daros de Luca wrote: > > 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. Which setup exactly? > 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. FWIW, it will not break silently, but like this (very easy to test, no need to guess): [ 7.196687] mscc_felix 0000:00:00.5 swp3 (uninitialized): failed to connect to PHY: -ENODEV [ 7.205168] mscc_felix 0000:00:00.5 swp3 (uninitialized): error -19 setting up PHY for tree 0, switch 0, port 3 If you have no other decision to make in the driver based on the presence/absence of "phy-handle", it doesn't make much sense to bloat the driver with dubious logic just to get an arguably prettier error. I'm saying "dubious" because my understanding is that rtl8365mb "extint" ports can also serve as user ports, and can additionally be fixed-links. But arbitrary logic like this breaks that. The cost/benefit ratio does not seem too favorable for this addition.
> On Fri, Dec 22, 2023 at 05:03:38PM -0300, Luiz Angelo Daros de Luca wrote: > > > 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. > > Which setup exactly? Ports without a phy-handle. But, as you said, it is a broken device tree without a known device using it. > > 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. > > FWIW, it will not break silently, but like this (very easy to test, no need to guess): > > [ 7.196687] mscc_felix 0000:00:00.5 swp3 (uninitialized): failed to connect to PHY: -ENODEV > [ 7.205168] mscc_felix 0000:00:00.5 swp3 (uninitialized): error -19 setting up PHY for tree 0, switch 0, port 3 > > If you have no other decision to make in the driver based on the > presence/absence of "phy-handle", it doesn't make much sense to bloat > the driver with dubious logic just to get an arguably prettier error. > I'm saying "dubious" because my understanding is that rtl8365mb "extint" > ports can also serve as user ports, and can additionally be fixed-links. > But arbitrary logic like this breaks that. > > The cost/benefit ratio does not seem too favorable for this addition. OK, I'll remove them. Thanks. Regards, Luiz
On Fri, Dec 22, 2023 at 07:56:48PM +0300, Arınç ÜNAL wrote: > We should also align all DSA subdrivers with this understanding. I will > modify the MDIO bus patch I submitted for the MT7530 DSA subdriver > accordingly. I began working on this, and I do have some patches. But returns start to diminish very quickly. Some drivers are just not worth it to change. So I will also respin the documentation patch set to at least advise to not continue the pattern to new drivers. > I was wondering of moving the MDIO bus registration from DSA subdrivers to > the DSA core driver but probably it's not generic enough across switch > models with multiple MDIO buses and whatnot to manage this. Actually this is the logic after which everything starts to unravel - "multiple DSA switches have internal MDIO buses, so let's make DSA assist with their registration". If you can't do a good job at it, it's more honest to not even try - and you gave the perfect example of handling multiple internal MDIO buses. I just don't want to maintain stuff that I am really clueless about. If registering an MDIO bus is so hard that DSA has to help with it, make the MDIO API better. Where things would be comfortable for me is if the optional ds->user_mii_bus pointer could be always provided by individual subdrivers, and never allocated by the framework. So that dsa_switch_ops :: phy_read() and :: phy_write() would not exist at all.
On 3.01.2024 21:44, Vladimir Oltean wrote: > On Fri, Dec 22, 2023 at 07:56:48PM +0300, Arınç ÜNAL wrote: >> We should also align all DSA subdrivers with this understanding. I will >> modify the MDIO bus patch I submitted for the MT7530 DSA subdriver >> accordingly. > > I began working on this, and I do have some patches. But returns start > to diminish very quickly. Some drivers are just not worth it to change. > So I will also respin the documentation patch set to at least advise to > not continue the pattern to new drivers. I've seen your patch series regarding this. I like that you've thought to skip registering the bus if its node is explicitly disabled. I will implement that on the MT7530 subdriver as well. > >> I was wondering of moving the MDIO bus registration from DSA subdrivers to >> the DSA core driver but probably it's not generic enough across switch >> models with multiple MDIO buses and whatnot to manage this. > > Actually this is the logic after which everything starts to unravel - > "multiple DSA switches have internal MDIO buses, so let's make DSA > assist with their registration". > > If you can't do a good job at it, it's more honest to not even try - > and you gave the perfect example of handling multiple internal MDIO buses. Makes sense. Setting the interrupts is another good example. Currently, DSA registers the bus non-OF-based but won't set the interrupts, as far as I can see. > > I just don't want to maintain stuff that I am really clueless about. > If registering an MDIO bus is so hard that DSA has to help with it, > make the MDIO API better. > > Where things would be comfortable for me is if the optional ds->user_mii_bus > pointer could be always provided by individual subdrivers, and never allocated > by the framework. So that dsa_switch_ops :: phy_read() and :: phy_write() > would not exist at all. I agree. Why don't we do this? These are the subdrivers that we need to deal with before getting rid of dsa_switch_ops :: phy_read() and :: phy_write(), and the code block for registering the MDIO bus on the DSA core driver: drivers/net/dsa/b53/b53_common.c: - The DSA subdriver lets the DSA driver register the bus. drivers/net/dsa/microchip/ksz_common.c: - The DSA subdriver lets the DSA driver register the bus when "mdio" child node is not defined. drivers/net/dsa/realtek/realtek-mdio.c: - The DSA subdriver lets the DSA driver register the bus. This won't be the case after "[PATCH net-next v3 0/8] net: dsa: realtek: variants to drivers, interfaces to a common module" is applied. drivers/net/dsa/lan9303-core.c: - The DSA subdriver lets the DSA driver register the bus. drivers/net/dsa/vitesse-vsc73xx-core.c: - The DSA subdriver lets the DSA driver register the bus. All these subdrivers populate dsa_switch_ops :: phy_read() and :: phy_write() and won't populate ds->user_mii_bus. Arınç
diff --git a/drivers/net/dsa/realtek/realtek-common.c b/drivers/net/dsa/realtek/realtek-common.c index bf3933a99072..b1f0095d5bce 100644 --- a/drivers/net/dsa/realtek/realtek-common.c +++ b/drivers/net/dsa/realtek/realtek-common.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0+ #include <linux/module.h> +#include <linux/of_mdio.h> #include "realtek.h" #include "realtek-common.h" @@ -21,6 +22,85 @@ void realtek_common_unlock(void *ctx) } EXPORT_SYMBOL_GPL(realtek_common_unlock); +static int realtek_common_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 realtek_common_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); +} + +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; + } + } + + 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; + } + + 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; + } + + return 0; + +err_put_node: + of_node_put(mdio_np); + + return ret; +} +EXPORT_SYMBOL_GPL(realtek_common_setup_user_mdio); + /* sets up driver private data struct, sets up regmaps, parse common device-tree * properties and finally issues a hardware reset. */ @@ -108,7 +188,7 @@ int realtek_common_register_switch(struct realtek_priv *priv) priv->ds->priv = priv; priv->ds->dev = priv->dev; - priv->ds->ops = priv->ds_ops; + priv->ds->ops = priv->variant->ds_ops; priv->ds->num_ports = priv->num_ports; ret = dsa_register_switch(priv->ds); @@ -126,6 +206,11 @@ void realtek_common_remove(struct realtek_priv *priv) if (!priv) return; + dsa_unregister_switch(priv->ds); + + if (priv->user_mii_bus) + of_node_put(priv->user_mii_bus->dev.of_node); + /* leave the device reset asserted */ if (priv->reset) gpiod_set_value(priv->reset, 1); diff --git a/drivers/net/dsa/realtek/realtek-common.h b/drivers/net/dsa/realtek/realtek-common.h index 518d091ff496..b1c2a50d85cd 100644 --- a/drivers/net/dsa/realtek/realtek-common.h +++ b/drivers/net/dsa/realtek/realtek-common.h @@ -7,6 +7,7 @@ void realtek_common_lock(void *ctx); void realtek_common_unlock(void *ctx); +int realtek_common_setup_user_mdio(struct dsa_switch *ds); struct realtek_priv * realtek_common_probe(struct device *dev, struct regmap_config rc, struct regmap_config rc_nolock); diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c index 967f6c1e8df0..e2b5432eeb26 100644 --- a/drivers/net/dsa/realtek/realtek-mdio.c +++ b/drivers/net/dsa/realtek/realtek-mdio.c @@ -142,7 +142,6 @@ int realtek_mdio_probe(struct mdio_device *mdiodev) priv->bus = mdiodev->bus; priv->mdio_addr = mdiodev->addr; priv->write_reg_noack = realtek_mdio_write; - priv->ds_ops = priv->variant->ds_ops_mdio; ret = realtek_common_register_switch(priv); if (ret) @@ -156,11 +155,6 @@ void realtek_mdio_remove(struct mdio_device *mdiodev) { struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev); - if (!priv) - return; - - dsa_unregister_switch(priv->ds); - realtek_common_remove(priv); } EXPORT_SYMBOL_GPL(realtek_mdio_remove); diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c index 2b2c6e34bae5..383689163057 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> @@ -339,63 +338,6 @@ static const struct regmap_config realtek_smi_nolock_regmap_config = { .disable_locking = true, }; -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; - - mdio_np = of_get_compatible_child(priv->dev->of_node, "realtek,smi-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->dev.of_node = mdio_np; - priv->user_mii_bus->parent = priv->dev; - ds->user_mii_bus = priv->user_mii_bus; - - 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; - } - - return 0; - -err_put_node: - of_node_put(mdio_np); - - return ret; -} - int realtek_smi_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -417,8 +359,6 @@ 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->ds_ops = priv->variant->ds_ops_smi; ret = realtek_common_register_switch(priv); if (ret) @@ -432,14 +372,6 @@ void realtek_smi_remove(struct platform_device *pdev) { struct realtek_priv *priv = platform_get_drvdata(pdev); - if (!priv) - return; - - dsa_unregister_switch(priv->ds); - - if (priv->user_mii_bus) - of_node_put(priv->user_mii_bus->dev.of_node); - realtek_common_remove(priv); } EXPORT_SYMBOL_GPL(realtek_smi_remove); diff --git a/drivers/net/dsa/realtek/realtek.h b/drivers/net/dsa/realtek/realtek.h index fbd0616c1df3..7af6dcc1bb24 100644 --- a/drivers/net/dsa/realtek/realtek.h +++ b/drivers/net/dsa/realtek/realtek.h @@ -60,7 +60,6 @@ struct realtek_priv { spinlock_t lock; /* Locks around command writes */ struct dsa_switch *ds; - const struct dsa_switch_ops *ds_ops; struct irq_domain *irqdomain; bool leds_disabled; @@ -71,7 +70,6 @@ struct realtek_priv { struct rtl8366_mib_counter *mib_counters; const struct realtek_ops *ops; - int (*setup_interface)(struct dsa_switch *ds); int (*write_reg_noack)(void *ctx, u32 addr, u32 data); int vlan_enabled; @@ -115,8 +113,7 @@ struct realtek_ops { }; struct realtek_variant { - const struct dsa_switch_ops *ds_ops_smi; - const struct dsa_switch_ops *ds_ops_mdio; + const struct dsa_switch_ops *ds_ops; const struct realtek_ops *ops; unsigned int clk_delay; u8 cmd_read; diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c index 58ec057b6c32..e890ad113ba3 100644 --- a/drivers/net/dsa/realtek/rtl8365mb.c +++ b/drivers/net/dsa/realtek/rtl8365mb.c @@ -828,17 +828,6 @@ static int rtl8365mb_phy_write(struct realtek_priv *priv, int phy, int regnum, return 0; } -static int rtl8365mb_dsa_phy_read(struct dsa_switch *ds, int phy, int regnum) -{ - return rtl8365mb_phy_read(ds->priv, phy, regnum); -} - -static int rtl8365mb_dsa_phy_write(struct dsa_switch *ds, int phy, int regnum, - u16 val) -{ - return rtl8365mb_phy_write(ds->priv, phy, regnum, val); -} - static const struct rtl8365mb_extint * rtl8365mb_get_port_extint(struct realtek_priv *priv, int port) { @@ -2017,12 +2006,10 @@ static int rtl8365mb_setup(struct dsa_switch *ds) if (ret) goto out_teardown_irq; - if (priv->setup_interface) { - ret = priv->setup_interface(ds); - if (ret) { - dev_err(priv->dev, "could not set up MDIO bus\n"); - goto out_teardown_irq; - } + ret = realtek_common_setup_user_mdio(ds); + if (ret) { + dev_err(priv->dev, "could not set up MDIO bus\n"); + goto out_teardown_irq; } /* Start statistics counter polling */ @@ -2116,28 +2103,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv) return 0; } -static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = { - .get_tag_protocol = rtl8365mb_get_tag_protocol, - .change_tag_protocol = rtl8365mb_change_tag_protocol, - .setup = rtl8365mb_setup, - .teardown = rtl8365mb_teardown, - .phylink_get_caps = rtl8365mb_phylink_get_caps, - .phylink_mac_config = rtl8365mb_phylink_mac_config, - .phylink_mac_link_down = rtl8365mb_phylink_mac_link_down, - .phylink_mac_link_up = rtl8365mb_phylink_mac_link_up, - .port_stp_state_set = rtl8365mb_port_stp_state_set, - .get_strings = rtl8365mb_get_strings, - .get_ethtool_stats = rtl8365mb_get_ethtool_stats, - .get_sset_count = rtl8365mb_get_sset_count, - .get_eth_phy_stats = rtl8365mb_get_phy_stats, - .get_eth_mac_stats = rtl8365mb_get_mac_stats, - .get_eth_ctrl_stats = rtl8365mb_get_ctrl_stats, - .get_stats64 = rtl8365mb_get_stats64, - .port_change_mtu = rtl8365mb_port_change_mtu, - .port_max_mtu = rtl8365mb_port_max_mtu, -}; - -static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = { +static const struct dsa_switch_ops rtl8365mb_switch_ops = { .get_tag_protocol = rtl8365mb_get_tag_protocol, .change_tag_protocol = rtl8365mb_change_tag_protocol, .setup = rtl8365mb_setup, @@ -2146,8 +2112,6 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = { .phylink_mac_config = rtl8365mb_phylink_mac_config, .phylink_mac_link_down = rtl8365mb_phylink_mac_link_down, .phylink_mac_link_up = rtl8365mb_phylink_mac_link_up, - .phy_read = rtl8365mb_dsa_phy_read, - .phy_write = rtl8365mb_dsa_phy_write, .port_stp_state_set = rtl8365mb_port_stp_state_set, .get_strings = rtl8365mb_get_strings, .get_ethtool_stats = rtl8365mb_get_ethtool_stats, @@ -2167,8 +2131,7 @@ static const struct realtek_ops rtl8365mb_ops = { }; const struct realtek_variant rtl8365mb_variant = { - .ds_ops_smi = &rtl8365mb_switch_ops_smi, - .ds_ops_mdio = &rtl8365mb_switch_ops_mdio, + .ds_ops = &rtl8365mb_switch_ops, .ops = &rtl8365mb_ops, .clk_delay = 10, .cmd_read = 0xb9, diff --git a/drivers/net/dsa/realtek/rtl8366rb.c b/drivers/net/dsa/realtek/rtl8366rb.c index e60a0a81d426..56619aa592ec 100644 --- a/drivers/net/dsa/realtek/rtl8366rb.c +++ b/drivers/net/dsa/realtek/rtl8366rb.c @@ -1027,12 +1027,10 @@ static int rtl8366rb_setup(struct dsa_switch *ds) if (ret) dev_info(priv->dev, "no interrupt support\n"); - if (priv->setup_interface) { - ret = priv->setup_interface(ds); - if (ret) { - dev_err(priv->dev, "could not set up MDIO bus\n"); - return -ENODEV; - } + ret = realtek_common_setup_user_mdio(ds); + if (ret) { + dev_err(priv->dev, "could not set up MDIO bus\n"); + return -ENODEV; } return 0; @@ -1772,17 +1770,6 @@ static int rtl8366rb_phy_write(struct realtek_priv *priv, int phy, int regnum, return ret; } -static int rtl8366rb_dsa_phy_read(struct dsa_switch *ds, int phy, int regnum) -{ - return rtl8366rb_phy_read(ds->priv, phy, regnum); -} - -static int rtl8366rb_dsa_phy_write(struct dsa_switch *ds, int phy, int regnum, - u16 val) -{ - return rtl8366rb_phy_write(ds->priv, phy, regnum, val); -} - static int rtl8366rb_reset_chip(struct realtek_priv *priv) { int timeout = 10; @@ -1848,35 +1835,9 @@ static int rtl8366rb_detect(struct realtek_priv *priv) return 0; } -static const struct dsa_switch_ops rtl8366rb_switch_ops_smi = { - .get_tag_protocol = rtl8366_get_tag_protocol, - .setup = rtl8366rb_setup, - .phylink_get_caps = rtl8366rb_phylink_get_caps, - .phylink_mac_link_up = rtl8366rb_mac_link_up, - .phylink_mac_link_down = rtl8366rb_mac_link_down, - .get_strings = rtl8366_get_strings, - .get_ethtool_stats = rtl8366_get_ethtool_stats, - .get_sset_count = rtl8366_get_sset_count, - .port_bridge_join = rtl8366rb_port_bridge_join, - .port_bridge_leave = rtl8366rb_port_bridge_leave, - .port_vlan_filtering = rtl8366rb_vlan_filtering, - .port_vlan_add = rtl8366_vlan_add, - .port_vlan_del = rtl8366_vlan_del, - .port_enable = rtl8366rb_port_enable, - .port_disable = rtl8366rb_port_disable, - .port_pre_bridge_flags = rtl8366rb_port_pre_bridge_flags, - .port_bridge_flags = rtl8366rb_port_bridge_flags, - .port_stp_state_set = rtl8366rb_port_stp_state_set, - .port_fast_age = rtl8366rb_port_fast_age, - .port_change_mtu = rtl8366rb_change_mtu, - .port_max_mtu = rtl8366rb_max_mtu, -}; - -static const struct dsa_switch_ops rtl8366rb_switch_ops_mdio = { +static const struct dsa_switch_ops rtl8366rb_switch_ops = { .get_tag_protocol = rtl8366_get_tag_protocol, .setup = rtl8366rb_setup, - .phy_read = rtl8366rb_dsa_phy_read, - .phy_write = rtl8366rb_dsa_phy_write, .phylink_get_caps = rtl8366rb_phylink_get_caps, .phylink_mac_link_up = rtl8366rb_mac_link_up, .phylink_mac_link_down = rtl8366rb_mac_link_down, @@ -1915,8 +1876,7 @@ static const struct realtek_ops rtl8366rb_ops = { }; const struct realtek_variant rtl8366rb_variant = { - .ds_ops_smi = &rtl8366rb_switch_ops_smi, - .ds_ops_mdio = &rtl8366rb_switch_ops_mdio, + .ds_ops = &rtl8366rb_switch_ops, .ops = &rtl8366rb_ops, .clk_delay = 10, .cmd_read = 0xa9,
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 common module. All references to SMI have been eliminated, with the exception of the compatible string, which will continue to function as before. The realtek-mdio will now use this driver instead of the generic DSA driver ("dsa user smi"), which should not be used with OF[1]. There was a change in how the driver looks for the MDIO node in the device tree. Now, it first checks for a child node named "mdio," which is required by both interfaces in binding docs but used previously only by realtek-mdio. If the node is not found, it will also look for a compatible string, required only by SMI-connected devices in binding docs and compatible with the old realtek-smi behavior. The line assigning dev.of_node in mdio_bus has been removed since the subsequent of_mdiobus_register will always overwrite it. ds->user_mii_bus is only defined if all user ports do not declare a phy-handle, providing a warning about the erroneous device tree[2]. With a single ds_ops for both interfaces, the ds_ops in realtek_priv is no longer necessary. Now, the realtek_variant.ds_ops can be used directly. The realtek_priv.setup_interface() has been removed as we can directly call the new common function. The switch unregistration and the MDIO node decrement in refcount were moved into realtek_common_remove() as both interfaces now need to put the MDIO node. [1] https://lkml.kernel.org/netdev/20220630200423.tieprdu5fpabflj7@bang-olufsen.dk/T/ [2] https://lkml.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/T/#u Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> --- drivers/net/dsa/realtek/realtek-common.c | 87 +++++++++++++++++++++++- drivers/net/dsa/realtek/realtek-common.h | 1 + drivers/net/dsa/realtek/realtek-mdio.c | 6 -- drivers/net/dsa/realtek/realtek-smi.c | 68 ------------------ drivers/net/dsa/realtek/realtek.h | 5 +- drivers/net/dsa/realtek/rtl8365mb.c | 49 ++----------- drivers/net/dsa/realtek/rtl8366rb.c | 52 ++------------ 7 files changed, 100 insertions(+), 168 deletions(-)