| Message ID | 20240106122142.235389-1-arinc.unal@arinc9.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [net-next] net: dsa: mt7530: support OF-based registration of switch MDIO bus | expand |
The author is wrong. I'll fix that with v2. I'm waiting for reviews in the meantime. pw-bot: cr Arınç
On Sat, Jan 06, 2024 at 03:21:42PM +0300, Arınç ÜNAL wrote: > From: David Bauer <mail@david-bauer.net> > > Currently the MDIO bus of the switches the MT7530 DSA subdriver controls > can only be registered as non-OF-based. Bring support for registering the > bus OF-based. > > 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. This case should only be covered for the switches which > their dt-bindings documentation didn't document the MDIO bus from the > start. This is to keep supporting the device trees that do not describe the > MDIO bus on the device tree but the MDIO bus is being used nonetheless. > > 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]. > > Switch MDIO bus defined but explicitly disabled: If the device tree says > status = "disabled" for the MDIO bus, we shouldn't need an MDIO bus at all. > Instead, just exit as early as possible and do not call any MDIO API. > > The use of ds->user_mii_bus is inappropriate when the MDIO bus of the > switch is described on the device tree [1], which is why we don't populate > ds->user_mii_bus in that case. > > Link: https://lore.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/ [1] > Suggested-by: David Bauer <mail@david-bauer.net> > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com> > --- > drivers/net/dsa/mt7530.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > index 391c4dbdff42..39d7e7ad7154 100644 > --- a/drivers/net/dsa/mt7530.c > +++ b/drivers/net/dsa/mt7530.c > @@ -2153,17 +2153,25 @@ mt7530_free_irq(struct mt7530_priv *priv) > static int > mt7530_setup_mdio(struct mt7530_priv *priv) > { > + struct device_node *mnp, *np = priv->dev->of_node; > struct dsa_switch *ds = priv->ds; > struct device *dev = priv->dev; > struct mii_bus *bus; > static int idx; > - int ret; > + int ret = 0; > + > + mnp = of_get_child_by_name(np, "mdio"); > + > + if (mnp && !of_device_is_available(mnp)) > + goto out; nit: I think it would easier on the eyes to simply return 0; > > bus = devm_mdiobus_alloc(dev); > if (!bus) > return -ENOMEM; > > - ds->user_mii_bus = bus; > + if (!mnp) > + 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,16 +2182,18 @@ mt7530_setup_mdio(struct mt7530_priv *priv) > bus->parent = dev; > bus->phy_mask = ~ds->phys_mii_mask; > > - if (priv->irq) > + if (priv->irq && !mnp) > 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) > mt7530_free_mdio_irq(priv); > } > > +out: > return ret; > } > > -- > 2.40.1 >
On 7.01.2024 22:52, Simon Horman wrote: > On Sat, Jan 06, 2024 at 03:21:42PM +0300, Arınç ÜNAL wrote: >> From: David Bauer <mail@david-bauer.net> >> >> Currently the MDIO bus of the switches the MT7530 DSA subdriver controls >> can only be registered as non-OF-based. Bring support for registering the >> bus OF-based. >> >> 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. This case should only be covered for the switches which >> their dt-bindings documentation didn't document the MDIO bus from the >> start. This is to keep supporting the device trees that do not describe the >> MDIO bus on the device tree but the MDIO bus is being used nonetheless. >> >> 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]. >> >> Switch MDIO bus defined but explicitly disabled: If the device tree says >> status = "disabled" for the MDIO bus, we shouldn't need an MDIO bus at all. >> Instead, just exit as early as possible and do not call any MDIO API. >> >> The use of ds->user_mii_bus is inappropriate when the MDIO bus of the >> switch is described on the device tree [1], which is why we don't populate >> ds->user_mii_bus in that case. >> >> Link: https://lore.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/ [1] >> Suggested-by: David Bauer <mail@david-bauer.net> >> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com> >> --- >> drivers/net/dsa/mt7530.c | 18 ++++++++++++++---- >> 1 file changed, 14 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c >> index 391c4dbdff42..39d7e7ad7154 100644 >> --- a/drivers/net/dsa/mt7530.c >> +++ b/drivers/net/dsa/mt7530.c >> @@ -2153,17 +2153,25 @@ mt7530_free_irq(struct mt7530_priv *priv) >> static int >> mt7530_setup_mdio(struct mt7530_priv *priv) >> { >> + struct device_node *mnp, *np = priv->dev->of_node; >> struct dsa_switch *ds = priv->ds; >> struct device *dev = priv->dev; >> struct mii_bus *bus; >> static int idx; >> - int ret; >> + int ret = 0; >> + >> + mnp = of_get_child_by_name(np, "mdio"); >> + >> + if (mnp && !of_device_is_available(mnp)) >> + goto out; > > nit: I think it would easier on the eyes to simply > > return 0; Will do. Thanks. Arınç
On Mon, Jan 08, 2024 at 01:22:18PM +0300, Arınç ÜNAL wrote: > > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > > > index 391c4dbdff42..39d7e7ad7154 100644 > > > --- a/drivers/net/dsa/mt7530.c > > > +++ b/drivers/net/dsa/mt7530.c > > > @@ -2153,17 +2153,25 @@ mt7530_free_irq(struct mt7530_priv *priv) > > > static int > > > mt7530_setup_mdio(struct mt7530_priv *priv) > > > { > > > + struct device_node *mnp, *np = priv->dev->of_node; > > > struct dsa_switch *ds = priv->ds; > > > struct device *dev = priv->dev; > > > struct mii_bus *bus; > > > static int idx; > > > - int ret; > > > + int ret = 0; > > > + > > > + mnp = of_get_child_by_name(np, "mdio"); > > > + > > > + if (mnp && !of_device_is_available(mnp)) > > > + goto out; > > > > nit: I think it would easier on the eyes to simply > > > > return 0; Actually "return 0" leaks "mnp". An of_node_put() is needed.
On Mon, Jan 08, 2024 at 01:00:00PM +0200, Vladimir Oltean wrote: > On Mon, Jan 08, 2024 at 01:22:18PM +0300, Arınç ÜNAL wrote: > > > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > > > > index 391c4dbdff42..39d7e7ad7154 100644 > > > > --- a/drivers/net/dsa/mt7530.c > > > > +++ b/drivers/net/dsa/mt7530.c > > > > @@ -2153,17 +2153,25 @@ mt7530_free_irq(struct mt7530_priv *priv) > > > > static int > > > > mt7530_setup_mdio(struct mt7530_priv *priv) > > > > { > > > > + struct device_node *mnp, *np = priv->dev->of_node; > > > > struct dsa_switch *ds = priv->ds; > > > > struct device *dev = priv->dev; > > > > struct mii_bus *bus; > > > > static int idx; > > > > - int ret; > > > > + int ret = 0; > > > > + > > > > + mnp = of_get_child_by_name(np, "mdio"); > > > > + > > > > + if (mnp && !of_device_is_available(mnp)) > > > > + goto out; > > > > > > nit: I think it would easier on the eyes to simply > > > > > > return 0; > > Actually "return 0" leaks "mnp". An of_node_put() is needed. Yes, sorry for not noticing that.
On 8.01.2024 14:00, Vladimir Oltean wrote: > On Mon, Jan 08, 2024 at 01:22:18PM +0300, Arınç ÜNAL wrote: >>>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c >>>> index 391c4dbdff42..39d7e7ad7154 100644 >>>> --- a/drivers/net/dsa/mt7530.c >>>> +++ b/drivers/net/dsa/mt7530.c >>>> @@ -2153,17 +2153,25 @@ mt7530_free_irq(struct mt7530_priv *priv) >>>> static int >>>> mt7530_setup_mdio(struct mt7530_priv *priv) >>>> { >>>> + struct device_node *mnp, *np = priv->dev->of_node; >>>> struct dsa_switch *ds = priv->ds; >>>> struct device *dev = priv->dev; >>>> struct mii_bus *bus; >>>> static int idx; >>>> - int ret; >>>> + int ret = 0; >>>> + >>>> + mnp = of_get_child_by_name(np, "mdio"); >>>> + >>>> + if (mnp && !of_device_is_available(mnp)) >>>> + goto out; >>> >>> nit: I think it would easier on the eyes to simply >>> >>> return 0; > > Actually "return 0" leaks "mnp". An of_node_put() is needed. I realised that I also miss these: - run of_node_put(mnp) if bus = devm_mdiobus_alloc(dev) fails - don't run mt7530_free_mdio_irq if MDIO bus is described on OF Please let me know if this addresses everything: diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index 391c4dbdff42..cf2ff7680c15 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -2146,24 +2146,40 @@ mt7530_free_irq_common(struct mt7530_priv *priv) static void mt7530_free_irq(struct mt7530_priv *priv) { - mt7530_free_mdio_irq(priv); + struct device_node *mnp, *np = priv->dev->of_node; + + mnp = of_get_child_by_name(np, "mdio"); + if (!mnp) + mt7530_free_mdio_irq(priv); + of_node_put(mnp); + mt7530_free_irq_common(priv); } static int mt7530_setup_mdio(struct mt7530_priv *priv) { + struct device_node *mnp, *np = priv->dev->of_node; struct dsa_switch *ds = priv->ds; struct device *dev = priv->dev; struct mii_bus *bus; static int idx; - int ret; + int ret = 0; + + mnp = of_get_child_by_name(np, "mdio"); + + if (mnp && !of_device_is_available(mnp)) + goto out; bus = devm_mdiobus_alloc(dev); - if (!bus) - return -ENOMEM; + if (!bus) { + ret = -ENOMEM; + goto out; + } + + if (!mnp) + ds->user_mii_bus = bus; - 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,16 +2190,18 @@ mt7530_setup_mdio(struct mt7530_priv *priv) bus->parent = dev; bus->phy_mask = ~ds->phys_mii_mask; - if (priv->irq) + if (priv->irq && !mnp) mt7530_setup_mdio_irq(priv); - ret = devm_mdiobus_register(dev, bus); + ret = devm_of_mdiobus_register(dev, bus, mnp); if (ret) { dev_err(dev, "failed to register MDIO bus: %d\n", ret); - if (priv->irq) + if (priv->irq && !mnp) mt7530_free_mdio_irq(priv); } +out: + of_node_put(mnp); return ret; } Arınç
On Mon, Jan 08, 2024 at 03:14:45PM +0300, Arınç ÜNAL wrote: > I realised that I also miss these: > > - run of_node_put(mnp) if bus = devm_mdiobus_alloc(dev) fails > - don't run mt7530_free_mdio_irq if MDIO bus is described on OF > > Please let me know if this addresses everything: > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > index 391c4dbdff42..cf2ff7680c15 100644 > --- a/drivers/net/dsa/mt7530.c > +++ b/drivers/net/dsa/mt7530.c > @@ -2146,24 +2146,40 @@ mt7530_free_irq_common(struct mt7530_priv *priv) > static void > mt7530_free_irq(struct mt7530_priv *priv) > { > - mt7530_free_mdio_irq(priv); > + struct device_node *mnp, *np = priv->dev->of_node; > + > + mnp = of_get_child_by_name(np, "mdio"); > + if (!mnp) > + mt7530_free_mdio_irq(priv); > + of_node_put(mnp); > + > mt7530_free_irq_common(priv); > } > static int > mt7530_setup_mdio(struct mt7530_priv *priv) > { > + struct device_node *mnp, *np = priv->dev->of_node; > struct dsa_switch *ds = priv->ds; > struct device *dev = priv->dev; > struct mii_bus *bus; > static int idx; > - int ret; > + int ret = 0; > + > + mnp = of_get_child_by_name(np, "mdio"); > + > + if (mnp && !of_device_is_available(mnp)) > + goto out; > bus = devm_mdiobus_alloc(dev); > - if (!bus) > - return -ENOMEM; > + if (!bus) { > + ret = -ENOMEM; > + goto out; > + } > + > + if (!mnp) > + ds->user_mii_bus = bus; > - 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,16 +2190,18 @@ mt7530_setup_mdio(struct mt7530_priv *priv) > bus->parent = dev; > bus->phy_mask = ~ds->phys_mii_mask; > - if (priv->irq) > + if (priv->irq && !mnp) > mt7530_setup_mdio_irq(priv); > - ret = devm_mdiobus_register(dev, bus); > + ret = devm_of_mdiobus_register(dev, bus, mnp); > if (ret) { > dev_err(dev, "failed to register MDIO bus: %d\n", ret); > - if (priv->irq) > + if (priv->irq && !mnp) > mt7530_free_mdio_irq(priv); > } > +out: > + of_node_put(mnp); > return ret; > } Looks ok. Please note that net-next has now closed. Sorry that I couldn't bring myself to review more during the weekend and the holidays. https://lore.kernel.org/netdev/20240107172221.733a7a44@kernel.org/
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index 391c4dbdff42..39d7e7ad7154 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -2153,17 +2153,25 @@ mt7530_free_irq(struct mt7530_priv *priv) static int mt7530_setup_mdio(struct mt7530_priv *priv) { + struct device_node *mnp, *np = priv->dev->of_node; struct dsa_switch *ds = priv->ds; struct device *dev = priv->dev; struct mii_bus *bus; static int idx; - int ret; + int ret = 0; + + mnp = of_get_child_by_name(np, "mdio"); + + if (mnp && !of_device_is_available(mnp)) + goto out; bus = devm_mdiobus_alloc(dev); if (!bus) return -ENOMEM; - ds->user_mii_bus = bus; + if (!mnp) + 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,16 +2182,18 @@ mt7530_setup_mdio(struct mt7530_priv *priv) bus->parent = dev; bus->phy_mask = ~ds->phys_mii_mask; - if (priv->irq) + if (priv->irq && !mnp) 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) mt7530_free_mdio_irq(priv); } +out: return ret; }