[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a638d0de-bfb3-4937-969e-13d494b6a2c3@arinc9.com>
Date: Sat, 30 Dec 2023 10:18:57 +0300
From: Arınç ÜNAL <arinc.unal@...nc9.com>
To: Luiz Angelo Daros de Luca <luizluca@...il.com>, netdev@...r.kernel.org
Cc: linus.walleij@...aro.org, alsi@...g-olufsen.dk, andrew@...n.ch,
f.fainelli@...il.com, olteanv@...il.com, davem@...emloft.net,
edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com
Subject: Re: [PATCH net-next v3 8/8] Revert "net: dsa: OF-ware slave_mii_bus"
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ç
Powered by blists - more mailing lists