Re: [PATCH net-next v3 8/8] Revert "net: dsa: OF-ware slave_mii_bus"

Arınç ÜNAL
2023-12-30T07:19:04Z

On 23.12.2023 03:46, Luiz Angelo Daros de Luca wrote:
> This reverts commit fe7324b932222574a0721b80e72c6c5fe57960d1.
>
> The use of user_mii_bus is inappropriate when the hardware is described
> with a device-tree [1].
>
> Since all drivers currently implementing ds_switch_ops.phy_{read,write}
> were not updated to utilize the MDIO information from OF with the
> generic "dsa user mii", they might not be affected by this change.
>
> [1] https://lkml.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/T/#u

This doesn't make sense to me: "The use of user_mii_bus is inappropriate
when the hardware is described with a device-tree." I think this is the
correct statement: "The use of user_mii_bus is inappropriate when the MDIO
bus of the switch is described on the device-tree."

The patch log is also not clear on why this leads to the removal of
OF-based registration from the DSA core driver.

I would change the patch log here to something like this:

net: dsa: do not populate user_mii_bus when switch MDIO bus is described

The use of ds->user_mii_bus is inappropriate when the MDIO bus of the
switch is described on the device-tree [1].

To keep things simple, make [all subdrivers that control a switch [with
MDIO bus] probed on OF] register the bus on the subdriver.

The subdrivers that control switches [with MDIO bus] probed on OF must
follow this logic to support all cases properly:

No switch MDIO bus defined: Populate ds->user_mii_bus, register the MDIO
bus, set the interrupts for PHYs if "interrupt-controller" is defined at
the switch node.

Switch MDIO bus defined: Don't populate ds->user_mii_bus, register the MDIO
bus, set the interrupts for PHYs if ["interrupt-controller" is defined at
the switch node and "interrupts" is defined at the PHY nodes under the
switch MDIO bus node].

There can be a case where ds->user_mii_bus is not populated, but
ds_switch_ops.phy_{read,write} is. That would mean the subdriver controls a
switch probed on OF, and it lets the DSA core driver to populate
ds->user_mii_bus and register the bus. We don't want this to happen.
Therefore, ds_switch_ops.phy_{read,write} should be only used on the
subdrivers that control switches probed on platform_data.

With that, the "!ds->user_mii_bus && ds->ops->phy_read" check under
dsa_switch_setup() remains in use only for switches probed on
platform_data. Therefore, remove OF-based registration as it's useless now.

---

I think we should do all this in a single patch. I've done it on the MT7530
DSA subdriver which I maintain.

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 391c4dbdff42..bbd230a73ead 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2155,15 +2155,21 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
{
struct dsa_switch *ds = priv->ds;
struct device *dev = priv->dev;
+ struct device_node *np, *mnp;
struct mii_bus *bus;
static int idx;
int ret;

+ np = priv->dev->of_node;
+ mnp = of_get_child_by_name(np, "mdio");
+
bus = devm_mdiobus_alloc(dev);
if (!bus)
return -ENOMEM;

- ds->user_mii_bus = bus;
+ if (mnp == NULL)
+ ds->user_mii_bus = bus;
+
bus->priv = priv;
bus->name = KBUILD_MODNAME "-mii";
snprintf(bus->id, MII_BUS_ID_SIZE, KBUILD_MODNAME "-%d", idx++);
@@ -2174,10 +2180,11 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
bus->parent = dev;
bus->phy_mask = ~ds->phys_mii_mask;

- if (priv->irq)
+ if (priv->irq && mnp == NULL)
mt7530_setup_mdio_irq(priv);

- ret = devm_mdiobus_register(dev, bus);
+ ret = devm_of_mdiobus_register(dev, bus, mnp);
+ of_node_put(mnp);
if (ret) {
dev_err(dev, "failed to register MDIO bus: %d\n", ret);
if (priv->irq)

Arınç