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

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

[Rafał Miłecki]

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.

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?

-- 
Rafał

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

[Florian Fainelli]

+Andrew, Vladimir, Heiner, Russell, Saravana,

On 9/20/21 5:52 AM, Rafał Miłecki wrote:
> 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/
https://lore.kernel.org/linux-devicetree/20210920141024.1409-1-zajec5@gmail.com/
https://lore.kernel.org/linux-devicetree/20210920141024.1409-2-zajec5@gmail.com/

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

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

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.
-- 
Florian

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

[Vladimir Oltean]

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:
> > 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/
> https://lore.kernel.org/linux-devicetree/20210920141024.1409-1-zajec5@gmail.com/
> https://lore.kernel.org/linux-devicetree/20210920141024.1409-2-zajec5@gmail.com/
> 
> 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.

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

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?

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

[Florian Fainelli]

On 9/20/21 10:03 AM, Vladimir Oltean wrote:
> 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:
>>> 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/
>> https://lore.kernel.org/linux-devicetree/20210920141024.1409-1-zajec5@gmail.com/
>> https://lore.kernel.org/linux-devicetree/20210920141024.1409-2-zajec5@gmail.com/
>>
>> 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.

I should have been clearer here, it is expected to get at least one
EPROBE_DEFER, the outcome of it is not, but you have provided the reason
why below.

> 
>>> 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/
> 
> 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?

The SPROM is a piece of NVRAM that is intended to describe in a set of
key/value pairs various platform configuration details. There can be up
to 3 GMACs on the SoC which you can connect in a variety of ways towards
internal/external PHYs or internal/external Ethernet switches. The SPROM
is used to describe whether you connect to a regular PHY (not at PHY
address 30 decimal, so not the Broadcom pseudo-PHY) or an Ethernet
switch pseudo-PHY via MDIO.

What appears to be missing here is that we should not be executing this
block of code for phyaddr == BGMAC_PHY_NOREGS because we will not have a
PHY device proper to begin with and this collides with registering the
b53_mdio driver.
--
Florian

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

[Vladimir Oltean]

On Mon, Sep 20, 2021 at 10:14:48AM -0700, Florian Fainelli wrote:
> The SPROM is a piece of NVRAM that is intended to describe in a set of
> key/value pairs various platform configuration details. There can be up
> to 3 GMACs on the SoC which you can connect in a variety of ways towards
> internal/external PHYs or internal/external Ethernet switches. The SPROM
> is used to describe whether you connect to a regular PHY (not at PHY
> address 30 decimal, so not the Broadcom pseudo-PHY) or an Ethernet
> switch pseudo-PHY via MDIO.
> 
> What appears to be missing here is that we should not be executing this
> block of code for phyaddr == BGMAC_PHY_NOREGS because we will not have a
> PHY device proper to begin with and this collides with registering the
> b53_mdio driver.

Who provisions the SPROM exactly? It still seems pretty broken to me
that one of the GMACs has a bgmac->phyaddr pointing to a switch.
Special-casing the Broadcom switch seems not enough, the same thing
could happen with a Marvell switch or others. How about looking up the
device tree whether the bgmac->mii_bus' OF node has any child with a
"reg" of bgmac->phyaddr, and if it does, whether of_mdiobus_child_is_phy
actually returns true for it?

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

[Florian Fainelli]

On 9/20/21 10:40 AM, Vladimir Oltean wrote:
> On Mon, Sep 20, 2021 at 10:14:48AM -0700, Florian Fainelli wrote:
>> The SPROM is a piece of NVRAM that is intended to describe in a set of
>> key/value pairs various platform configuration details. There can be up
>> to 3 GMACs on the SoC which you can connect in a variety of ways towards
>> internal/external PHYs or internal/external Ethernet switches. The SPROM
>> is used to describe whether you connect to a regular PHY (not at PHY
>> address 30 decimal, so not the Broadcom pseudo-PHY) or an Ethernet
>> switch pseudo-PHY via MDIO.
>>
>> What appears to be missing here is that we should not be executing this
>> block of code for phyaddr == BGMAC_PHY_NOREGS because we will not have a
>> PHY device proper to begin with and this collides with registering the
>> b53_mdio driver.
> 
> Who provisions the SPROM exactly? It still seems pretty broken to me
> that one of the GMACs has a bgmac->phyaddr pointing to a switch.

The OEMs are typically responsible for that. It is not "broken" per-se,
and you will find additional key/value pairs that e.g.: describe the
initial switch configuration something like:

vlan0ports="0 1 2 3 5t"
vlan1ports="4 5t"

So this has been used as a dumping ground of "how I want the device to
be configured eventually". 0x1e/30 is sort of "universally" within
Broadcom's own universe that this designates an Ethernet switch
pseudo-PHY MDIO bus address, and we all know that nobody in their right
mind would design a Wi-Fi router with a discrete Ethernet switch that is
not from Broadcom, right?

> Special-casing the Broadcom switch seems not enough, the same thing
> could happen with a Marvell switch or others. How about looking up the
> device tree whether the bgmac->mii_bus' OF node has any child with a
> "reg" of bgmac->phyaddr, and if it does, whether of_mdiobus_child_is_phy
> actually returns true for it?

We could do that, however I don't know whether this will break the
arch/mips/bcm47xx devices which are still in active use by the OpenWrt
community and for which there is no Device Tree (no technical
limitation, just no motivation since devices are EOL'd), but maybe out
of tree patches can be carried in the OpenWrt tree to revert anything
that upstream came up with.
-- 
Florian

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

[Vladimir Oltean]

On Mon, Sep 20, 2021 at 10:46:31AM -0700, Florian Fainelli wrote:
> On 9/20/21 10:40 AM, Vladimir Oltean wrote:
> > On Mon, Sep 20, 2021 at 10:14:48AM -0700, Florian Fainelli wrote:
> >> The SPROM is a piece of NVRAM that is intended to describe in a set of
> >> key/value pairs various platform configuration details. There can be up
> >> to 3 GMACs on the SoC which you can connect in a variety of ways towards
> >> internal/external PHYs or internal/external Ethernet switches. The SPROM
> >> is used to describe whether you connect to a regular PHY (not at PHY
> >> address 30 decimal, so not the Broadcom pseudo-PHY) or an Ethernet
> >> switch pseudo-PHY via MDIO.
> >>
> >> What appears to be missing here is that we should not be executing this
> >> block of code for phyaddr == BGMAC_PHY_NOREGS because we will not have a
> >> PHY device proper to begin with and this collides with registering the
> >> b53_mdio driver.
> > 
> > Who provisions the SPROM exactly? It still seems pretty broken to me
> > that one of the GMACs has a bgmac->phyaddr pointing to a switch.
> 
> The OEMs are typically responsible for that. It is not "broken" per-se,
> and you will find additional key/value pairs that e.g.: describe the
> initial switch configuration something like:
> 
> vlan0ports="0 1 2 3 5t"
> vlan1ports="4 5t"
> 
> So this has been used as a dumping ground of "how I want the device to
> be configured eventually". 0x1e/30 is sort of "universally" within
> Broadcom's own universe that this designates an Ethernet switch
> pseudo-PHY MDIO bus address, and we all know that nobody in their right
> mind would design a Wi-Fi router with a discrete Ethernet switch that is
> not from Broadcom, right?
> 

But even so, what's a "pseudo PHY" exactly? I think that's at the bottom
of this issue. In the Linux device model, a device has a single driver.
In this case, the same MDIO device either has a switch driver, if you
accept it's a switch, or a PHY driver, if you accept it's a PHY.
I said it's "broken" because the expectation seems to be that it's a switch,
but it looks like it's treated otherwise. Simply put, the same device
can't be both a switch and a PHY.

The issue is really in bcma_phy_connect. That is what force-binds the
generic PHY driver. Since the bgmac-bcma driver does not support fixed
links, it tries to make do the way it can. This will not work with DSA.

> > Special-casing the Broadcom switch seems not enough, the same thing
> > could happen with a Marvell switch or others. How about looking up the
> > device tree whether the bgmac->mii_bus' OF node has any child with a
> > "reg" of bgmac->phyaddr, and if it does, whether of_mdiobus_child_is_phy
> > actually returns true for it?
> 
> We could do that, however I don't know whether this will break the
> arch/mips/bcm47xx devices which are still in active use by the OpenWrt
> community and for which there is no Device Tree (no technical
> limitation, just no motivation since devices are EOL'd), but maybe out
> of tree patches can be carried in the OpenWrt tree to revert anything
> that upstream came up with.

By OpenWRT do you mean swconfig or actual DSA?

I think Rafal is using device tree, so the check can be conditionally
made based on the presence of an OF node corresponding to the MDIO bus.
That would still work, unless the OpenWRT people want to use DSA without
device tree too...

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

[Florian Fainelli]

On 9/20/21 11:02 AM, Vladimir Oltean wrote:
> On Mon, Sep 20, 2021 at 10:46:31AM -0700, Florian Fainelli wrote:
>> On 9/20/21 10:40 AM, Vladimir Oltean wrote:
>>> On Mon, Sep 20, 2021 at 10:14:48AM -0700, Florian Fainelli wrote:
>>>> The SPROM is a piece of NVRAM that is intended to describe in a set of
>>>> key/value pairs various platform configuration details. There can be up
>>>> to 3 GMACs on the SoC which you can connect in a variety of ways towards
>>>> internal/external PHYs or internal/external Ethernet switches. The SPROM
>>>> is used to describe whether you connect to a regular PHY (not at PHY
>>>> address 30 decimal, so not the Broadcom pseudo-PHY) or an Ethernet
>>>> switch pseudo-PHY via MDIO.
>>>>
>>>> What appears to be missing here is that we should not be executing this
>>>> block of code for phyaddr == BGMAC_PHY_NOREGS because we will not have a
>>>> PHY device proper to begin with and this collides with registering the
>>>> b53_mdio driver.
>>>
>>> Who provisions the SPROM exactly? It still seems pretty broken to me
>>> that one of the GMACs has a bgmac->phyaddr pointing to a switch.
>>
>> The OEMs are typically responsible for that. It is not "broken" per-se,
>> and you will find additional key/value pairs that e.g.: describe the
>> initial switch configuration something like:
>>
>> vlan0ports="0 1 2 3 5t"
>> vlan1ports="4 5t"
>>
>> So this has been used as a dumping ground of "how I want the device to
>> be configured eventually". 0x1e/30 is sort of "universally" within
>> Broadcom's own universe that this designates an Ethernet switch
>> pseudo-PHY MDIO bus address, and we all know that nobody in their right
>> mind would design a Wi-Fi router with a discrete Ethernet switch that is
>> not from Broadcom, right?
>>
> 
> But even so, what's a "pseudo PHY" exactly? I think that's at the bottom
> of this issue. In the Linux device model, a device has a single driver.
> In this case, the same MDIO device either has a switch driver, if you
> accept it's a switch, or a PHY driver, if you accept it's a PHY.
> I said it's "broken" because the expectation seems to be that it's a switch,
> but it looks like it's treated otherwise. Simply put, the same device
> can't be both a switch and a PHY.

A pseudo-PHY is a device that can snoop and respond to MDIO bus
requests. I understand it cannot be both, just explaining to you how the
people at Broadcom have been seeing the world from their perspective.
Anything that is found at MDIO address 0x1e/30 is considered a MDIO
attached switch, that's all.

> 
> The issue is really in bcma_phy_connect. That is what force-binds the
> generic PHY driver. Since the bgmac-bcma driver does not support fixed
> links, it tries to make do the way it can. This will not work with DSA.

Yes, I understand that.

> 
>>> Special-casing the Broadcom switch seems not enough, the same thing
>>> could happen with a Marvell switch or others. How about looking up the
>>> device tree whether the bgmac->mii_bus' OF node has any child with a
>>> "reg" of bgmac->phyaddr, and if it does, whether of_mdiobus_child_is_phy
>>> actually returns true for it?
>>
>> We could do that, however I don't know whether this will break the
>> arch/mips/bcm47xx devices which are still in active use by the OpenWrt
>> community and for which there is no Device Tree (no technical
>> limitation, just no motivation since devices are EOL'd), but maybe out
>> of tree patches can be carried in the OpenWrt tree to revert anything
>> that upstream came up with.
> 
> By OpenWRT do you mean swconfig or actual DSA?

Yes, swconfig in that case with the b53 swconfig driver trying to
register as a PHY device.

> 
> I think Rafal is using device tree, so the check can be conditionally
> made based on the presence of an OF node corresponding to the MDIO bus.
> That would still work, unless the OpenWRT people want to use DSA without
> device tree too...
> 

All I am saying is that there is not really any need to come up with a
Device Tree-based solution since you can inspect the mdio_device and
find out whether it is an Ethernet PHY or a MDIO device proper, and that
ought to cover all cases that I can think of.
-- 
Florian

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

[Vladimir Oltean]

On Mon, Sep 20, 2021 at 11:10:39AM -0700, Florian Fainelli wrote:
> On 9/20/21 11:02 AM, Vladimir Oltean wrote:
> > On Mon, Sep 20, 2021 at 10:46:31AM -0700, Florian Fainelli wrote:
> >> On 9/20/21 10:40 AM, Vladimir Oltean wrote:
> >>> On Mon, Sep 20, 2021 at 10:14:48AM -0700, Florian Fainelli wrote:
> >>>> The SPROM is a piece of NVRAM that is intended to describe in a set of
> >>>> key/value pairs various platform configuration details. There can be up
> >>>> to 3 GMACs on the SoC which you can connect in a variety of ways towards
> >>>> internal/external PHYs or internal/external Ethernet switches. The SPROM
> >>>> is used to describe whether you connect to a regular PHY (not at PHY
> >>>> address 30 decimal, so not the Broadcom pseudo-PHY) or an Ethernet
> >>>> switch pseudo-PHY via MDIO.
> >>>>
> >>>> What appears to be missing here is that we should not be executing this
> >>>> block of code for phyaddr == BGMAC_PHY_NOREGS because we will not have a
> >>>> PHY device proper to begin with and this collides with registering the
> >>>> b53_mdio driver.
> >>>
> >>> Who provisions the SPROM exactly? It still seems pretty broken to me
> >>> that one of the GMACs has a bgmac->phyaddr pointing to a switch.
> >>
> >> The OEMs are typically responsible for that. It is not "broken" per-se,
> >> and you will find additional key/value pairs that e.g.: describe the
> >> initial switch configuration something like:
> >>
> >> vlan0ports="0 1 2 3 5t"
> >> vlan1ports="4 5t"
> >>
> >> So this has been used as a dumping ground of "how I want the device to
> >> be configured eventually". 0x1e/30 is sort of "universally" within
> >> Broadcom's own universe that this designates an Ethernet switch
> >> pseudo-PHY MDIO bus address, and we all know that nobody in their right
> >> mind would design a Wi-Fi router with a discrete Ethernet switch that is
> >> not from Broadcom, right?
> >>
> > 
> > But even so, what's a "pseudo PHY" exactly? I think that's at the bottom
> > of this issue. In the Linux device model, a device has a single driver.
> > In this case, the same MDIO device either has a switch driver, if you
> > accept it's a switch, or a PHY driver, if you accept it's a PHY.
> > I said it's "broken" because the expectation seems to be that it's a switch,
> > but it looks like it's treated otherwise. Simply put, the same device
> > can't be both a switch and a PHY.
> 
> A pseudo-PHY is a device that can snoop and respond to MDIO bus
> requests. I understand it cannot be both, just explaining to you how the
> people at Broadcom have been seeing the world from their perspective.
> Anything that is found at MDIO address 0x1e/30 is considered a MDIO
> attached switch, that's all.
> 
> > 
> > The issue is really in bcma_phy_connect. That is what force-binds the
> > generic PHY driver. Since the bgmac-bcma driver does not support fixed
> > links, it tries to make do the way it can. This will not work with DSA.
> 
> Yes, I understand that.
> 
> > 
> >>> Special-casing the Broadcom switch seems not enough, the same thing
> >>> could happen with a Marvell switch or others. How about looking up the
> >>> device tree whether the bgmac->mii_bus' OF node has any child with a
> >>> "reg" of bgmac->phyaddr, and if it does, whether of_mdiobus_child_is_phy
> >>> actually returns true for it?
> >>
> >> We could do that, however I don't know whether this will break the
> >> arch/mips/bcm47xx devices which are still in active use by the OpenWrt
> >> community and for which there is no Device Tree (no technical
> >> limitation, just no motivation since devices are EOL'd), but maybe out
> >> of tree patches can be carried in the OpenWrt tree to revert anything
> >> that upstream came up with.
> > 
> > By OpenWRT do you mean swconfig or actual DSA?
> 
> Yes, swconfig in that case with the b53 swconfig driver trying to
> register as a PHY device.
> 
> > 
> > I think Rafal is using device tree, so the check can be conditionally
> > made based on the presence of an OF node corresponding to the MDIO bus.
> > That would still work, unless the OpenWRT people want to use DSA without
> > device tree too...
> > 
> 
> All I am saying is that there is not really any need to come up with a
> Device Tree-based solution since you can inspect the mdio_device and
> find out whether it is an Ethernet PHY or a MDIO device proper, and that
> ought to cover all cases that I can think of.

Okay, but where's the problem? I guess we're on the same page, and
you're saying that we should not be calling bcma_mdio_mii_register, and
assigning the result to bgmac->mii_bus, because that makes us call
bcma_phy_connect instead of bgmac_phy_connect_direct. But based on what
condition? Simply if bgmac->phyaddr == BGMAC_PHY_NOREGS?

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

[Florian Fainelli]

On 9/20/21 11:17 AM, Vladimir Oltean wrote:
[snip]
>> All I am saying is that there is not really any need to come up with a
>> Device Tree-based solution since you can inspect the mdio_device and
>> find out whether it is an Ethernet PHY or a MDIO device proper, and that
>> ought to cover all cases that I can think of.
> 
> Okay, but where's the problem? I guess we're on the same page, and
> you're saying that we should not be calling bcma_mdio_mii_register, and
> assigning the result to bgmac->mii_bus, because that makes us call
> bcma_phy_connect instead of bgmac_phy_connect_direct. But based on what
> condition? Simply if bgmac->phyaddr == BGMAC_PHY_NOREGS?

Yes simply that condition, I really believe it ought to be enough for
the space these devices are in use.
-- 
Florian

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

[Vladimir Oltean]

On Mon, Sep 20, 2021 at 11:25:06AM -0700, Florian Fainelli wrote:
> On 9/20/21 11:17 AM, Vladimir Oltean wrote:
> [snip]
> >> All I am saying is that there is not really any need to come up with a
> >> Device Tree-based solution since you can inspect the mdio_device and
> >> find out whether it is an Ethernet PHY or a MDIO device proper, and that
> >> ought to cover all cases that I can think of.
> > 
> > Okay, but where's the problem? I guess we're on the same page, and
> > you're saying that we should not be calling bcma_mdio_mii_register, and
> > assigning the result to bgmac->mii_bus, because that makes us call
> > bcma_phy_connect instead of bgmac_phy_connect_direct. But based on what
> > condition? Simply if bgmac->phyaddr == BGMAC_PHY_NOREGS?
> 
> Yes simply that condition, I really believe it ought to be enough for
> the space these devices are in use.

So the last question is, are the link parameters advertised by the
switch pseudo PHY identical with the link parameters advertised by the
fixed PHY instantiated by bgmac_phy_connect_direct? If they are, is it
intended or merely a coincidence?

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

[Vladimir Oltean]

On Mon, Sep 20, 2021 at 11:10:39AM -0700, Florian Fainelli wrote:
> > But even so, what's a "pseudo PHY" exactly? I think that's at the bottom
> > of this issue. In the Linux device model, a device has a single driver.
> > In this case, the same MDIO device either has a switch driver, if you
> > accept it's a switch, or a PHY driver, if you accept it's a PHY.
> > I said it's "broken" because the expectation seems to be that it's a switch,
> > but it looks like it's treated otherwise. Simply put, the same device
> > can't be both a switch and a PHY.
> 
> A pseudo-PHY is a device that can snoop and respond to MDIO bus
> requests. I understand it cannot be both, just explaining to you how the
> people at Broadcom have been seeing the world from their perspective.
> Anything that is found at MDIO address 0x1e/30 is considered a MDIO
> attached switch, that's all.

Nothing wrong with that per se, at NXP we've been thinking about RevMII
as well, and having a switch expose itself as a PHY over MDIO, in any
case something a bit richer/more interactive than a fixed-link.

One way to bypass that limitation is to access the switch registers
through a different device compared to the PHY device. At the very
least, a different MDIO address. Even better, not over MDIO at all, but
something faster, SPI, PCIe etc.

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

[Rafał Miłecki]

On 20.09.2021 20:25, Florian Fainelli wrote:
> On 9/20/21 11:17 AM, Vladimir Oltean wrote:
> [snip]
>>> All I am saying is that there is not really any need to come up with a
>>> Device Tree-based solution since you can inspect the mdio_device and
>>> find out whether it is an Ethernet PHY or a MDIO device proper, and that
>>> ought to cover all cases that I can think of.
>>
>> Okay, but where's the problem? I guess we're on the same page, and
>> you're saying that we should not be calling bcma_mdio_mii_register, and
>> assigning the result to bgmac->mii_bus, because that makes us call
>> bcma_phy_connect instead of bgmac_phy_connect_direct. But based on what
>> condition? Simply if bgmac->phyaddr == BGMAC_PHY_NOREGS?
> 
> Yes simply that condition, I really believe it ought to be enough for
> the space these devices are in use.

I'm afraid I got lost somewhere in this discussion.

If we don't call bcma_mdio_mii_register() (as suggested in quoted
e-mail) then MDIO device 0x1e won't get created and "bcm53xx"
(b53_mdio.c) won't ever load.

We need b53 to load to support the switch.


On 20.09.2021 19:03, Vladimir Oltean wrote:
 > On Mon, Sep 20, 2021 at 09:36:23AM -0700, Florian Fainelli wrote:
 >> 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.

With the following patch [1] applied:
[PATCH net-next] net: bgmac: support MDIO described in DT
https://lore.kernel.org/linux-devicetree/20210920123441.9088-1-zajec5@gmail.com/

of_mdiobus_register() finds MDIO bus child at 0x1e from:
[PATCH 1/2] ARM: dts: BCM53573: Describe on-SoC BCM53125 rev 4 switch
https://lore.kernel.org/linux-devicetree/20210920141024.1409-1-zajec5@gmail.com/

and it calls of_mdiobus_register_device().

For that MDIO device kernel first tries to load "bcm53xx" (b53_mdio.c)
which returns -EPROBE_DEFER and then kernel loads "Generic PHY".

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

[Rafał Miłecki]

On 21.09.2021 11:45, Rafał Miłecki wrote:
> On 20.09.2021 20:25, Florian Fainelli wrote:
>> On 9/20/21 11:17 AM, Vladimir Oltean wrote:
>> [snip]
>>>> All I am saying is that there is not really any need to come up with a
>>>> Device Tree-based solution since you can inspect the mdio_device and
>>>> find out whether it is an Ethernet PHY or a MDIO device proper, and that
>>>> ought to cover all cases that I can think of.
>>>
>>> Okay, but where's the problem? I guess we're on the same page, and
>>> you're saying that we should not be calling bcma_mdio_mii_register, and
>>> assigning the result to bgmac->mii_bus, because that makes us call
>>> bcma_phy_connect instead of bgmac_phy_connect_direct. But based on what
>>> condition? Simply if bgmac->phyaddr == BGMAC_PHY_NOREGS?
>>
>> Yes simply that condition, I really believe it ought to be enough for
>> the space these devices are in use.
> 
> I'm afraid I got lost somewhere in this discussion.
> 
> If we don't call bcma_mdio_mii_register() (as suggested in quoted
> e-mail) then MDIO device 0x1e won't get created and "bcm53xx"
> (b53_mdio.c) won't ever load.

I did a quick test and replaced bcma_phy_connect with
bgmac_phy_connect_direct.

That fixes the problem for me.

So I guess we should:
1. Leave the bcma_mdio_mii_register() call
2. Use bcma_phy_connect() only for real PHY device

Correct me if I'm wrong please.