Re: Race between "Generic PHY" and "bcm53xx" drivers after -EPROBE_DEFER

On Mon, Sep 20, 2021 at 09:36:23AM -0700, Florian Fainelli wrote:

+Andrew, Vladimir, Heiner, Russell, Saravana,

On 9/20/21 5:52 AM, Rafał Miłecki wrote:

quoted

I have problem using a switch b53 MDIO driver with an Ethernet bgmac
driver.

bgmac registers MDIO bus before registering Ethernet controller. That
results in kernel probing switch (available as MDIO device) early which
results in dsa_port_parse_of() returning -EPROBE_DEFER.

Yes, putting the big picture together and assuming you have applied
these 3 patches which is how you observed that:

https://lore.kernel.org/linux-devicetree/20210920123441.9088-1-zajec5@gmail.com/ (local)
https://lore.kernel.org/linux-devicetree/20210920141024.1409-1-zajec5@gmail.com/ (local)
https://lore.kernel.org/linux-devicetree/20210920141024.1409-2-zajec5@gmail.com/ (local)

This is somewhat expected unfortunately and I don't know how we can
break the circular dependencies here.

Why is it expected? AFAIK:
(1) the Generic PHY driver will not match any hardware in phy_bus_match,
    it is only bound by hand. Am I wrong?
(2) of_mdiobus_register sets "mdio->phy_mask = ~0;" anyway, which blocks
    the automatic creation of any phy_device for stuff that responds to
    PHY ID registers 2 and 3.

quoted

It's OK so far but then in goes like this:

[    1.306884] bus: 'bcma': driver_probe_device: matched device bcma0:5 with driver bgmac_bcma
[    1.315427] bus: 'bcma': really_probe: probing driver bgmac_bcma with device bcma0:5
[    1.323468] bgmac_bcma bcma0:5: Found PHY addr: 30 (NOREGS)
[    1.329722] libphy: bcma_mdio mii bus: probed
[    1.334468] bus: 'mdio_bus': driver_probe_device: matched device bcma_mdio-0-0:1e with driver bcm53xx
[    1.343877] bus: 'mdio_bus': really_probe: probing driver bcm53xx with device bcma_mdio-0-0:1e
[    1.353174] bcm53xx bcma_mdio-0-0:1e: found switch: BCM53125, rev 4
[    1.359595] bcm53xx bcma_mdio-0-0:1e: failed to register switch: -517
[    1.366212] mdio_bus bcma_mdio-0-0:1e: Driver bcm53xx requests probe deferral
[    1.373499] mdio_bus bcma_mdio-0-0:1e: Added to deferred list
[    1.379362] bgmac_bcma bcma0:5: Support for Roboswitch not implemented
[    1.387067] bgmac_bcma bcma0:5: Timeout waiting for reg 0x1E0
[    1.393600] driver: 'Generic PHY': driver_bound: bound to device 'bcma_mdio-0-0:1e'
[    1.401390] Generic PHY bcma_mdio-0-0:1e: Removed from deferred list

I can't drop "Generic PHY" driver as it's required for non-CPU switch
ports. I just need kernel to prefer b53 MDIO driver over the "Generic
PHY" one.

Can someone help me fix that, please?

I don't think that you have a race condition, but you have the Ethernet
switch's pseudo PHY

what's a pseudo PHY?

which is accessible via MDIO and the Generic PHY driver happily goes
on trying to read the MII_PHYSID1/PHYS_ID2 which do not map to
anything on that switch, but still you will get a non-zero/non-all Fs
value from there, hence the Generic PHY is happy to take over.

Why would it do that? Why would there be a PHY device created for the
switch? Is there any phy-handle pointing to the switch OF node?

Given that the MDIO node does have a compatible string which is not in
the form of an Ethernet PHY's compatible string, I wonder if we can
somewhat break the circular dependency using that information.

I think you're talking about:

of_mdiobus_register
-> of_mdiobus_child_is_phy

but as mentioned, that code path should not be creating PHY devices.

I think this code path in bgmac_probe might be responsible for it:

	switch (core->core_unit) {
	case 0:
		bgmac->phyaddr = sprom->et0phyaddr;
		break;
	case 1:
		bgmac->phyaddr = sprom->et1phyaddr;
		break;
	case 2:
		bgmac->phyaddr = sprom->et2phyaddr;
		break;
	}
	bgmac->phyaddr &= BGMAC_PHY_MASK;
	if (bgmac->phyaddr == BGMAC_PHY_MASK) {
		dev_err(bgmac->dev, "No PHY found\n");
		err = -ENODEV;
		goto err;
	}
	dev_info(bgmac->dev, "Found PHY addr: %d%s\n", bgmac->phyaddr,
		 bgmac->phyaddr == BGMAC_PHY_NOREGS ? " (NOREGS)" : "");

	if (!bgmac_is_bcm4707_family(core) &&
	    !(ci->id == BCMA_CHIP_ID_BCM53573 && core->core_unit == 1)) {
		struct phy_device *phydev;

		mii_bus = bcma_mdio_mii_register(bgmac);
		if (IS_ERR(mii_bus)) {
			err = PTR_ERR(mii_bus);
			goto err;
		}
		bgmac->mii_bus = mii_bus;

		phydev = mdiobus_get_phy(bgmac->mii_bus, bgmac->phyaddr);
		if (ci->id == BCMA_CHIP_ID_BCM53573 && phydev &&
		    (phydev->drv->phy_id & phydev->drv->phy_id_mask) == PHY_ID_BCM54210E)
			phydev->dev_flags |= PHY_BRCM_EN_MASTER_MODE;
	}

At least, that's what the log indicates:

[    1.323468] bgmac_bcma bcma0:5: Found PHY addr: 30 (NOREGS) <- 30 is 0x1e, which is Rafal's switch MDIO address in the device tree patch here:
https://lore.kernel.org/linux-devicetree/20210920141024.1409-1-zajec5@gmail.com/ (local)

So I haven't investigated what the code tries to do by searching the "sprom", but it probably shouldn't have a PHY address
pointing towards the switch?