Re: [PATCH net-next] net: dsa: mt7530: register OF node for internal MDIO bus

["Arınç ÜNAL" <arinc.unal@arinc9.com>]

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ç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel