From: David Bauer <[email protected]>
The MT753x switches provide a switch-internal MDIO bus for the embedded
PHYs.
Register a OF sub-node on the switch OF-node for this internal MDIO bus.
This allows to configure the embedded PHYs using device-tree.
Signed-off-by: David Bauer <[email protected]>
Signed-off-by: Daniel Golle <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 391c4dbdff42..f8ecc354630b 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2155,10 +2155,13 @@ 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;
+
bus = devm_mdiobus_alloc(dev);
if (!bus)
return -ENOMEM;
@@ -2177,7 +2180,9 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
if (priv->irq)
mt7530_setup_mdio_irq(priv);
- ret = devm_mdiobus_register(dev, bus);
+ mnp = of_get_child_by_name(np, "mdio");
+ 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)
--
2.40.1
On Wed, Dec 20, 2023 at 07:35:39PM +0200, Arınç ÜNAL wrote:
> From: David Bauer <[email protected]>
>
> The MT753x switches provide a switch-internal MDIO bus for the embedded
> PHYs.
>
> Register a OF sub-node on the switch OF-node for this internal MDIO bus.
> This allows to configure the embedded PHYs using device-tree.
>
> Signed-off-by: David Bauer <[email protected]>
> Signed-off-by: Daniel Golle <[email protected]>
> Signed-off-by: Arınç ÜNAL <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
On 12/20/23 11:05 PM, Arınç ÜNAL wrote:
> From: David Bauer <[email protected]>
>
> The MT753x switches provide a switch-internal MDIO bus for the embedded
> PHYs.
>
> Register a OF sub-node on the switch OF-node for this internal MDIO bus.
> This allows to configure the embedded PHYs using device-tree.
>
> Signed-off-by: David Bauer <[email protected]>
> Signed-off-by: Daniel Golle <[email protected]>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
> drivers/net/dsa/mt7530.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 391c4dbdff42..f8ecc354630b 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2155,10 +2155,13 @@ 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;
> +
> bus = devm_mdiobus_alloc(dev);
> if (!bus)
> return -ENOMEM;
> @@ -2177,7 +2180,9 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
> if (priv->irq)
> mt7530_setup_mdio_irq(priv);
>
> - ret = devm_mdiobus_register(dev, bus);
> + mnp = of_get_child_by_name(np, "mdio");
If the node is not found, then the return value would be NULL.
Though devm_of_mdiobus_register() and of_node_put() take care of NULL references,
other drivers that use devm_of_mdiobus_register() mostly perform a early exit if the node is NULL.
> + 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)
--
Regards,
Ravi
> > @@ -2177,7 +2180,9 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
> > if (priv->irq)
> > mt7530_setup_mdio_irq(priv);
> >
> > - ret = devm_mdiobus_register(dev, bus);
> > + mnp = of_get_child_by_name(np, "mdio");
>
> If the node is not found, then the return value would be NULL.
> Though devm_of_mdiobus_register() and of_node_put() take care of NULL references,
> other drivers that use devm_of_mdiobus_register() mostly perform a early exit if the node is NULL.
Actually, many don't as well. of_mdiobus_register() falls back to
mdiobus_register() is np==NULL. That causes a scan of the bus to find
all the PHYs and you can then use phy_find_first() to access them.
So this code is O.K.
Andrew
On Wed, Dec 20, 2023 at 07:35:39PM +0200, Arınç ÜNAL wrote:
> From: David Bauer <[email protected]>
>
> The MT753x switches provide a switch-internal MDIO bus for the embedded
> PHYs.
>
> Register a OF sub-node on the switch OF-node for this internal MDIO bus.
> This allows to configure the embedded PHYs using device-tree.
>
> Signed-off-by: David Bauer <[email protected]>
> Signed-off-by: Daniel Golle <[email protected]>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
Can you please not assign "bus" to ds->user_mii_bus unless there is no
"mdio" OF node? We don't need ds->user_mii_bus populated when it is
described in device tree.
On 21.12.2023 18:16, Vladimir Oltean wrote:
> On Wed, Dec 20, 2023 at 07:35:39PM +0200, Arınç ÜNAL wrote:
>> From: David Bauer <[email protected]>
>>
>> The MT753x switches provide a switch-internal MDIO bus for the embedded
>> PHYs.
>>
>> Register a OF sub-node on the switch OF-node for this internal MDIO bus.
>> This allows to configure the embedded PHYs using device-tree.
>>
>> Signed-off-by: David Bauer <[email protected]>
>> Signed-off-by: Daniel Golle <[email protected]>
>> Signed-off-by: Arınç ÜNAL <[email protected]>
>> ---
>
> Can you please not assign "bus" to ds->user_mii_bus unless there is no
> "mdio" OF node? We don't need ds->user_mii_bus populated when it is
> described in device tree.
Do you mean like this:
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 391c4dbdff42..d239462a4bd8 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++);
@@ -2177,7 +2183,8 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
if (priv->irq)
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)
If the MDIO bus of the switch is defined on the devicetree, the
(!ds->user_mii_bus && ds->ops->phy_read) check in dsa_switch_setup will be
true so the MDIO bus will be attempted to be registered again, resulting in
a kernel panic.
[ 1.369012] mt7530-mdio mdio-bus:1f: MT7530 adapts as multi-chip module
[ 1.391552] CPU 0 Unable to handle kernel paging request at virtual address 00000700, epc == 802eb948, ra == 802eb938
[ 1.402205] Oops[#1]:
[ 1.404481] CPU: 0 PID: 26 Comm: kworker/u5:1 Not tainted 6.7.0-rc6-next-20231222-00012-g9997041f4e67-dirty #281
[ 1.414622] Hardware name: UniElec U7621-06 (16M flash)
[ 1.419829] Workqueue: events_unbound deferred_probe_work_func
[ 1.425664] $ 0 : 00000000 00000001 00000011 00000700
[ 1.430895] $ 4 : 00000700 814d0000 00000000 00000000
[ 1.436123] $ 8 : 00000001 00000011 00000011 81595a80
[ 1.441353] $12 : 81595a80 81595ad8 81595a80 81595ad8
[ 1.446585] $16 : 81589a80 00000000 82019000 81589600
[ 1.451815] $20 : 807d4a88 8205a280 00000005 00000000
[ 1.457045] $24 : 00000000 802efd60
[ 1.462274] $28 : 81594000 81595c18 80760000 802eb938
[ 1.467505] Hi : 00000000
[ 1.470371] Lo : 00000000
[ 1.473235] epc : 802eb948 mt753x_setup+0x39c/0x3bc
[ 1.478283] ra : 802eb938 mt753x_setup+0x38c/0x3bc
[ 1.483324] Status: 11000403 KERNEL EXL IE
[ 1.487508] Cause : 4080000c (ExcCode 03)
[ 1.491502] BadVA : 00000700
[ 1.494367] PrId : 0001992f (MIPS 1004Kc)
[ 1.498446] Process kworker/u5:1 (pid: 26, threadinfo=(ptrval), task=(ptrval), tls=00000000)
[ 1.506853] Stack : 7fffffff 00000000 8205a280 00000000 806a8ab8 81589a80 8205a280 81521f00
[ 1.515224] 81521f08 8157dc00 806ba49c 8205a280 00000001 804c6374 81589e00 81589c80
[ 1.523601] 806a5ed0 81589600 00000000 81595c9c 805cab30 806d0000 8069e96c 806b99cc
[ 1.531971] 00000006 00000dc0 ffffffff 802bd188 81589600 802bd29c fffffff4 fffffff4
[ 1.540341] 8205a480 807d339c 00000000 00000000 00000000 00000000 00000000 00000000
[ 1.548707] ...
[ 1.551150] Call Trace:
[ 1.553583] [<802eb948>] mt753x_setup+0x39c/0x3bc
[ 1.558286] [<804c6374>] dsa_register_switch+0x924/0xcc4
[ 1.563592] [<802ddad0>] mdio_probe+0x34/0x6c
[ 1.567943] [<802b95d0>] really_probe+0x16c/0x2dc
[ 1.572639] [<802b995c>] driver_probe_device+0x40/0xd0
[ 1.577767] [<802b9a4c>] __device_attach_driver+0x60/0xe8
[ 1.583154] [<802b7694>] bus_for_each_drv+0xc8/0xf4
[ 1.588042] [<802b9d8c>] __device_attach+0xd8/0x14c
[ 1.592912] [<802b831c>] bus_probe_device+0x48/0xd8
[ 1.597788] [<802b8f5c>] deferred_probe_work_func+0xb4/0xcc
[ 1.603348] [<8003ab1c>] process_scheduled_works+0x1dc/0x2b8
[ 1.609015] [<8003b1b0>] worker_thread+0x2dc/0x330
[ 1.613804] [<80041d4c>] kthread+0xf8/0x100
[ 1.617996] [<80002a18>] ret_from_kernel_thread+0x14/0x1c
[ 1.623385]
[ 1.624869] Code: 8ea30030 00042080 00641821 <ac620000> 26310001 1636fff1 00003825 1000ff67 02803025
[ 1.634638]
[ 1.636435] ---[ end trace 0000000000000000 ]---
Arınç
On Sun, Dec 24, 2023 at 10:37:12AM +0300, Arınç ÜNAL wrote:
> If the MDIO bus of the switch is defined on the devicetree, the
> (!ds->user_mii_bus && ds->ops->phy_read) check in dsa_switch_setup will be
> true so the MDIO bus will be attempted to be registered again, resulting in
> a kernel panic.
Where does mt7530 provide ds->ops->phy_read() in upstream?
On 27.12.2023 22:11, Vladimir Oltean wrote:
> On Sun, Dec 24, 2023 at 10:37:12AM +0300, Arınç ÜNAL wrote:
>> If the MDIO bus of the switch is defined on the devicetree, the
>> (!ds->user_mii_bus && ds->ops->phy_read) check in dsa_switch_setup will be
>> true so the MDIO bus will be attempted to be registered again, resulting in
>> a kernel panic.
>
> Where does mt7530 provide ds->ops->phy_read() in upstream?
Sorry, that's a misdiagnosis from my part. There's indeed no
ds->ops->phy_read() or ds->ops->phy_write(). I should know that as I have
intensively studied the MDIO bus registration on all DSA subdrivers. The
issue is at mt7530_setup_mdio_irq():
ds->user_mii_bus->irq[p] = irq;
I didn't realise ds->user_mii_bus is also used to store irq mapping for
each PHY. Should we agree that user_mii_bus is needed for all cases or make
another way to store the irq mappings?
Arınç
On Wed, Dec 27, 2023 at 10:51:08PM +0300, Arınç ÜNAL wrote:
> I didn't realise ds->user_mii_bus is also used to store irq mapping for
> each PHY.
It needs to, if the MDIO bus does not have an OF description through
which PHYs can have an 'interrupts' property. But if there is an OF
description for the MDIO bus and the PHYs, I think it is strange to
expect PHYs to have interrupts if they aren't described in OF.
> Should we agree that user_mii_bus is needed for all cases or make
> another way to store the irq mappings?
I looked at the upstream device trees:
- users of arch/mips/boot/dts/ralink/mt7621.dtsi
- arch/arm/boot/dts/mediatek/mt7623n-bananapi-bpi-r2.dts
- arch/arm/boot/dts/mediatek/mt7623n-rfb-emmc.dts
- arch/arm/boot/dts/mediatek/mt7623a.dtsi
- arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts
- arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3.dts
- arch/arm64/boot/dts/mediatek/mt7986a-rfb.dts
- arch/arm64/boot/dts/mediatek/mt7622-rfb1.dts
and without exception, none of these have the MDIO bus described in OF.
I'm not sure about other device trees. But it may well be that the
situation where "MDIO buses present in OF need an IRQ mapping for their
PHYs" does not need to be handled.
On 27.12.2023 23:02, Vladimir Oltean wrote:
> On Wed, Dec 27, 2023 at 10:51:08PM +0300, Arınç ÜNAL wrote:
>> I didn't realise ds->user_mii_bus is also used to store irq mapping for
>> each PHY.
>
> It needs to, if the MDIO bus does not have an OF description through
> which PHYs can have an 'interrupts' property. But if there is an OF
> description for the MDIO bus and the PHYs, I think it is strange to
> expect PHYs to have interrupts if they aren't described in OF.
>
>> Should we agree that user_mii_bus is needed for all cases or make
>> another way to store the irq mappings?
>
> I looked at the upstream device trees:
> - users of arch/mips/boot/dts/ralink/mt7621.dtsi
> - arch/arm/boot/dts/mediatek/mt7623n-bananapi-bpi-r2.dts
> - arch/arm/boot/dts/mediatek/mt7623n-rfb-emmc.dts
> - arch/arm/boot/dts/mediatek/mt7623a.dtsi
> - arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts
> - arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3.dts
> - arch/arm64/boot/dts/mediatek/mt7986a-rfb.dts
> - arch/arm64/boot/dts/mediatek/mt7622-rfb1.dts
>
> and without exception, none of these have the MDIO bus described in OF.
> I'm not sure about other device trees. But it may well be that the
> situation where "MDIO buses present in OF need an IRQ mapping for their
> PHYs" does not need to be handled.
As Daniel stated on a previous submission of this patch, being able to
reference the PHYs on the switch MDIO bus is mandatory on MT7988 as
calibration data from NVMEM for each PHY is required, so defining the MDIO
bus is required to support MT7988. Therefore, we should support interrupts
on device trees with the switch MDIO bus defined.
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");
+
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)
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)
Arınç
On Thu, Dec 28, 2023 at 07:58:13PM +0300, Arınç ÜNAL wrote:
> As Daniel stated on a previous submission of this patch, being able to
> reference the PHYs on the switch MDIO bus is mandatory on MT7988 as
> calibration data from NVMEM for each PHY is required, so defining the MDIO
> bus is required to support MT7988. Therefore, we should support interrupts
> on device trees with the switch MDIO bus defined.
Understood and no objection there. I was just making sure that there is
no existing case in upstream where the internal PHYs are described in OF,
that we'd have to preserve IRQ functionality for.
> 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).
> bus = devm_mdiobus_alloc(dev);
> if (!bus)
> return -ENOMEM;
> - ds->user_mii_bus = bus;
> + if (mnp == NULL)
!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,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/[email protected]/
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.
> 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().
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.
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/[email protected]/
>
> 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/[email protected]/
>
>> 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ç