Message-ID: <d31000b0-c0d9-457f-9493-e54fd228e317@arinc9.com>
Date: Fri, 5 Jan 2024 23:45:29 +0300
From: Arınç ÜNAL <arinc.unal@...nc9.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Daniel Golle <daniel@...rotopia.org>,
 Landen Chao <Landen.Chao@...iatek.com>, DENG Qingfang <dqfext@...il.com>,
 Sean Wang <sean.wang@...iatek.com>, Andrew Lunn <andrew@...n.ch>,
 Florian Fainelli <f.fainelli@...il.com>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Matthias Brugger <matthias.bgg@...il.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 David Bauer <mail@...id-bauer.net>, mithat.guner@...ont.com,
 erkin.bozoglu@...ont.com, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH net-next] net: dsa: mt7530: register OF node for internal
 MDIO bus

On 3.01.2024 22:02, Vladimir Oltean wrote:
> On Thu, Dec 28, 2023 at 07:58:13PM +0300, Arınç ÜNAL wrote:
>> The implementation below follows this logic:
>>
>> No switch MDIO bus defined: Register the MDIO bus, set the interrupts for
>> PHYs if "interrupt-controller" is defined at the switch node.
>>
>> Switch MDIO bus defined: 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].
>>
>> I think this approach fits your description so I'd like to agree that this
>> should be the way for all DSA subdrivers. Please let me know what you
>> think.
>>
>> 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");
>> +
> 
> Empty line between variable declarations and code. Or you can initialize
> them as part of their declaration, but you need to stick to the "longest
> line first" rule.
> 
> Also, it would be good to also check of_device_is_available(mnp).

Will do.

> 
>> +		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);
> 
> This is going to be interesting. There isn't really a correct way to
> manage the reference to "mnp", as far as I can tell. Normally, it should
> have been possible to release the reference as you did. But you need
> something along the lines of what Luiz/Russell have been discussing
> here:
> 
> https://lore.kernel.org/netdev/20231220045228.27079-2-luizluca@gmail.com/
> 
> In any case, the devres variant of of_mdiobus_register() seems incompatible
> with the mt7530 driver owning the "mnp" node for any longer than this,
> because it has no hook to call of_node_put() once the MDIO bus is unregistered.

I'm not sure what's the step I should take here. I don't know how MDIO
registration works and don't intend to spend time studying it at this time.

Looking at the conversation, I see that, in the end, this patch is applied:

https://lore.kernel.org/netdev/E1rLL6p-00EvAd-Ej@rmk-PC.armlinux.org.uk/

> 
>>   	if (ret) {
>>   		dev_err(dev, "failed to register MDIO bus: %d\n", ret);
>>   		if (priv->irq)
>>
>> With this device tree:
>>
>> switch {
>> 	interrupt-controller;
>> }
>>
>> [    1.420534] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=17)
>> [    1.433224] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=18)
>> [    1.445338] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=19)
>> [    1.457472] mt7530-mdio mdio-bus:1f lan4 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=20)
>> [    1.469587] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7530 PHY] (irq=21)
>>
>> With this device tree:
>>
>> switch {
>> 	interrupt-controller;
>>
>> 	mdio {
>> 		phy {
>> 			reg = <0>;
>> 		}
>> 	}
>> }
>>
>> [    1.413101] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=POLL)
>> [    1.429954] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=POLL)
>> [    1.443704] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=POLL)
>> [    1.455876] mt7530-mdio mdio-bus:1f lan4 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=POLL)
>> [    1.468079] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7530 PHY] (irq=POLL)
>>
>> With this device tree:
>>
>> switch {
>> 	interrupt-controller;
>>
>> 	mdio {
>> 		phy {
>> 			reg = <0>;
>> 			interrupts = <0>;
>> 		}
>> 	}
>> }
>>
>> [    1.420534] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=17)
>> [    1.433224] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=18)
>> [    1.445338] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=19)
>> [    1.457472] mt7530-mdio mdio-bus:1f lan4 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=20)
>> [    1.469587] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7530 PHY] (irq=21)
> 
> Looks sane.
> 
> FWIW, I found Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
> where internal PHYs don't have an 'interrupts' property, yet they are
> probably still expected to use interrupts - according to ksz_irq_phy_setup().

This conflicts with the reason we want the subdrivers to use
ds->user_mii_bus for. I'd much like to implement what I've done with this
patch to this subdriver. I believe it's negligible for the old device trees
to have the switch PHYs work with polling until the interrupts are defined
on the device tree.

> 
> Anyway, what's done is done, but I still don't see the point of making
> the binding much more flexible than it needs to be.

I don't see it that way. It's simply about describing the hardware. PHYs
without interrupts is still valid hardware. If the PHYs of this specific
hardware cannot possibly come without interrupts, then it should be the
bindings that ensure that interrupts on the device tree are always defined.
Not the other way around: Keep this information out of the device tree and
do it on the subdriver.

Arınç

Powered by blists - more mailing lists