netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next] net: dsa: mt7530: support OF-based registration of switch MDIO bus
@ 2024-01-06 12:21 Arınç ÜNAL
  2024-01-06 12:27 ` Arınç ÜNAL
  2024-01-07 19:52 ` Simon Horman
  0 siblings, 2 replies; 11+ messages in thread
From: Arınç ÜNAL @ 2024-01-06 12:21 UTC (permalink / raw)
  To: Arınç ÜNAL, Daniel Golle, Landen Chao,
	DENG Qingfang, Sean Wang, Andrew Lunn, Florian Fainelli,
	Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Matthias Brugger, AngeloGioacchino Del Regno
  Cc: David Bauer, mithat.guner, erkin.bozoglu,
	Luiz Angelo Daros de Luca, netdev, linux-kernel, linux-arm-kernel,
	linux-mediatek

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;
 
 	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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] net: dsa: mt7530: support OF-based registration of switch MDIO bus
  2024-01-06 12:21 [PATCH net-next] net: dsa: mt7530: support OF-based registration of switch MDIO bus Arınç ÜNAL
@ 2024-01-06 12:27 ` Arınç ÜNAL
  2024-01-07 19:52 ` Simon Horman
  1 sibling, 0 replies; 11+ messages in thread
From: Arınç ÜNAL @ 2024-01-06 12:27 UTC (permalink / raw)
  To: Daniel Golle, Landen Chao, DENG Qingfang, Sean Wang, Andrew Lunn,
	Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Matthias Brugger,
	AngeloGioacchino Del Regno
  Cc: David Bauer, mithat.guner, erkin.bozoglu,
	Luiz Angelo Daros de Luca, netdev, linux-kernel, linux-arm-kernel,
	linux-mediatek

The author is wrong. I'll fix that with v2. I'm waiting for reviews in the
meantime.

pw-bot: cr

Arınç

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] net: dsa: mt7530: support OF-based registration of switch MDIO bus
  2024-01-06 12:21 [PATCH net-next] net: dsa: mt7530: support OF-based registration of switch MDIO bus Arınç ÜNAL
  2024-01-06 12:27 ` Arınç ÜNAL
@ 2024-01-07 19:52 ` Simon Horman
  2024-01-08 10:22   ` Arınç ÜNAL
  1 sibling, 1 reply; 11+ messages in thread
From: Simon Horman @ 2024-01-07 19:52 UTC (permalink / raw)
  To: Arınç ÜNAL
  Cc: Daniel Golle, Landen Chao, DENG Qingfang, Sean Wang, Andrew Lunn,
	Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Matthias Brugger,
	AngeloGioacchino Del Regno, David Bauer, mithat.guner,
	erkin.bozoglu, Luiz Angelo Daros de Luca, netdev, linux-kernel,
	linux-arm-kernel, linux-mediatek

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
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] net: dsa: mt7530: support OF-based registration of switch MDIO bus
  2024-01-07 19:52 ` Simon Horman
@ 2024-01-08 10:22   ` Arınç ÜNAL
  2024-01-08 11:00     ` Vladimir Oltean
  0 siblings, 1 reply; 11+ messages in thread
From: Arınç ÜNAL @ 2024-01-08 10:22 UTC (permalink / raw)
  To: Simon Horman
  Cc: Daniel Golle, Landen Chao, DENG Qingfang, Sean Wang, Andrew Lunn,
	Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Matthias Brugger,
	AngeloGioacchino Del Regno, David Bauer, mithat.guner,
	erkin.bozoglu, Luiz Angelo Daros de Luca, netdev, linux-kernel,
	linux-arm-kernel, linux-mediatek

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ç

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] net: dsa: mt7530: support OF-based registration of switch MDIO bus
  2024-01-08 10:22   ` Arınç ÜNAL
@ 2024-01-08 11:00     ` Vladimir Oltean
  2024-01-08 11:15       ` Simon Horman
  2024-01-08 12:14       ` Arınç ÜNAL
  0 siblings, 2 replies; 11+ messages in thread
From: Vladimir Oltean @ 2024-01-08 11:00 UTC (permalink / raw)
  To: Arınç ÜNAL
  Cc: Simon Horman, Daniel Golle, Landen Chao, DENG Qingfang, Sean Wang,
	Andrew Lunn, Florian Fainelli, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Matthias Brugger,
	AngeloGioacchino Del Regno, David Bauer, mithat.guner,
	erkin.bozoglu, Luiz Angelo Daros de Luca, netdev, linux-kernel,
	linux-arm-kernel, linux-mediatek

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] net: dsa: mt7530: support OF-based registration of switch MDIO bus
  2024-01-08 11:00     ` Vladimir Oltean
@ 2024-01-08 11:15       ` Simon Horman
  2024-01-08 12:14       ` Arınç ÜNAL
  1 sibling, 0 replies; 11+ messages in thread
From: Simon Horman @ 2024-01-08 11:15 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Arınç ÜNAL, Daniel Golle, Landen Chao,
	DENG Qingfang, Sean Wang, Andrew Lunn, Florian Fainelli,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Matthias Brugger, AngeloGioacchino Del Regno, David Bauer,
	mithat.guner, erkin.bozoglu, Luiz Angelo Daros de Luca, netdev,
	linux-kernel, linux-arm-kernel, linux-mediatek

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] net: dsa: mt7530: support OF-based registration of switch MDIO bus
  2024-01-08 11:00     ` Vladimir Oltean
  2024-01-08 11:15       ` Simon Horman
@ 2024-01-08 12:14       ` Arınç ÜNAL
  2024-01-08 12:34         ` Vladimir Oltean
  1 sibling, 1 reply; 11+ messages in thread
From: Arınç ÜNAL @ 2024-01-08 12:14 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Simon Horman, Daniel Golle, Landen Chao, DENG Qingfang, Sean Wang,
	Andrew Lunn, Florian Fainelli, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Matthias Brugger,
	AngeloGioacchino Del Regno, David Bauer, mithat.guner,
	erkin.bozoglu, Luiz Angelo Daros de Luca, netdev, linux-kernel,
	linux-arm-kernel, linux-mediatek

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ç

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] net: dsa: mt7530: support OF-based registration of switch MDIO bus
  2024-01-08 12:14       ` Arınç ÜNAL
@ 2024-01-08 12:34         ` Vladimir Oltean
  0 siblings, 0 replies; 11+ messages in thread
From: Vladimir Oltean @ 2024-01-08 12:34 UTC (permalink / raw)
  To: Arınç ÜNAL
  Cc: Simon Horman, Daniel Golle, Landen Chao, DENG Qingfang, Sean Wang,
	Andrew Lunn, Florian Fainelli, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Matthias Brugger,
	AngeloGioacchino Del Regno, David Bauer, mithat.guner,
	erkin.bozoglu, Luiz Angelo Daros de Luca, netdev, linux-kernel,
	linux-arm-kernel, linux-mediatek

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/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH net-next] net: dsa: mt7530: support OF-based registration of switch MDIO bus
@ 2024-01-22  5:34 Arınç ÜNAL
  2024-01-25  9:22 ` Vladimir Oltean
  2024-01-25 15:00 ` patchwork-bot+netdevbpf
  0 siblings, 2 replies; 11+ messages in thread
From: Arınç ÜNAL @ 2024-01-22  5:34 UTC (permalink / raw)
  To: Arınç ÜNAL, Daniel Golle, Landen Chao,
	DENG Qingfang, Sean Wang, Andrew Lunn, Florian Fainelli,
	Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Matthias Brugger, AngeloGioacchino Del Regno
  Cc: David Bauer, mithat.guner, erkin.bozoglu,
	Luiz Angelo Daros de Luca, netdev, linux-kernel, linux-arm-kernel,
	linux-mediatek

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 | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

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;
 }
 
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] net: dsa: mt7530: support OF-based registration of switch MDIO bus
  2024-01-22  5:34 Arınç ÜNAL
@ 2024-01-25  9:22 ` Vladimir Oltean
  2024-01-25 15:00 ` patchwork-bot+netdevbpf
  1 sibling, 0 replies; 11+ messages in thread
From: Vladimir Oltean @ 2024-01-25  9:22 UTC (permalink / raw)
  To: Arınç ÜNAL
  Cc: Daniel Golle, Landen Chao, DENG Qingfang, Sean Wang, Andrew Lunn,
	Florian Fainelli, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Matthias Brugger, AngeloGioacchino Del Regno,
	David Bauer, mithat.guner, erkin.bozoglu,
	Luiz Angelo Daros de Luca, netdev, linux-kernel, linux-arm-kernel,
	linux-mediatek

On Mon, Jan 22, 2024 at 08:34:31AM +0300, Arınç ÜNAL wrote:
> 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>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] net: dsa: mt7530: support OF-based registration of switch MDIO bus
  2024-01-22  5:34 Arınç ÜNAL
  2024-01-25  9:22 ` Vladimir Oltean
@ 2024-01-25 15:00 ` patchwork-bot+netdevbpf
  1 sibling, 0 replies; 11+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-01-25 15:00 UTC (permalink / raw)
  To: =?utf-8?b?QXLEsW7DpyDDnE5BTCA8YXJpbmMudW5hbEBhcmluYzkuY29tPg==?=
  Cc: daniel, Landen.Chao, dqfext, sean.wang, andrew, f.fainelli,
	olteanv, davem, edumazet, kuba, pabeni, matthias.bgg,
	angelogioacchino.delregno, mail, mithat.guner, erkin.bozoglu,
	luizluca, netdev, linux-kernel, linux-arm-kernel, linux-mediatek

Hello:

This patch was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Mon, 22 Jan 2024 08:34:31 +0300 you wrote:
> 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:
> 
> [...]

Here is the summary with links:
  - [net-next] net: dsa: mt7530: support OF-based registration of switch MDIO bus
    https://git.kernel.org/netdev/net-next/c/91374ba537bd

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2024-01-25 15:00 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-06 12:21 [PATCH net-next] net: dsa: mt7530: support OF-based registration of switch MDIO bus Arınç ÜNAL
2024-01-06 12:27 ` Arınç ÜNAL
2024-01-07 19:52 ` Simon Horman
2024-01-08 10:22   ` Arınç ÜNAL
2024-01-08 11:00     ` Vladimir Oltean
2024-01-08 11:15       ` Simon Horman
2024-01-08 12:14       ` Arınç ÜNAL
2024-01-08 12:34         ` Vladimir Oltean
  -- strict thread matches above, loose matches on Subject: below --
2024-01-22  5:34 Arınç ÜNAL
2024-01-25  9:22 ` Vladimir Oltean
2024-01-25 15:00 ` patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).