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

On Mon, Sep 20, 2021 at 11:10:39AM -0700, Florian Fainelli wrote:

On 9/20/21 11:02 AM, Vladimir Oltean wrote:

quoted

On Mon, Sep 20, 2021 at 10:46:31AM -0700, Florian Fainelli wrote:

quoted

On 9/20/21 10:40 AM, Vladimir Oltean wrote:

quoted

On Mon, Sep 20, 2021 at 10:14:48AM -0700, Florian Fainelli wrote:

quoted

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.

quoted

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.

quoted

quoted

quoted

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.

quoted

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?