Date:   Mon, 20 Mar 2023 09:45:38 +0000
From:   "Buzarra, Arturo" <Arturo.Buzarra@...i.com>
To:     Heiner Kallweit <hkallweit1@...il.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Russell King - ARM Linux <linux@...linux.org.uk>
Subject: RE: [PATCH] net: phy: return EPROBE_DEFER if PHY is not accessible

Hi,

I will try to answer all your questions:

- "We need more specifics here, what type of PHY device are you seeing this with?"
- " So best start with some details about your use case, which MAC, which PHY, etc"
I'm using a LAN8720A PHY (10/100 on RMII mode) with a ST MAC ( in particular is a stm32mp1 processor).
We have two PHYs one is a Gigabit PHY (RGMII mode) and the another one is a 10/100 (RMII mode).
In the boot process, I think that there is a race condition between configuring the Ethernet MACs and the two PHYs. At same point the RGMII Ethernet MAC is configured and starts the PHY probes.
When the 10/100 PHY starts the probe, it tries to read the genphy_read_abilities() and always reads 0xFFFF ( I assume that this is the default electrical values for that lines before it are configured).
At that point, the PHY initialization assumes like a valid value 0xFFFF and obviously it reports capabilities that the LAN8720A PHY does not have, like for example gigabit support.
A few seconds later, the Ethernet MAC for RMII mode is probed, but I think that it is too late ( we are working with the manufacturer to investigate it)

- " If your PHY requires "some time" before it can be ready you have a number of ways to achieve that"
I reviewed the PHY datasheet and we measure the power-on lines (power, clock, reset, etc.) and we are compliant with the power-on sequence.
Also, I implement a retry system to read several times the same MII_BMSR register, even with a long delays but never reads the right value, so I assume that it is not an issue in the PHY initialization.

- " Are you resetting the device via a GPIO? Turning a regulator off/one etc?"
As I said before, we check the power-on sequence and we match with the datasheet, for our test purposes we tried to perform a hardware reset (using a gpio for the reset line) but nothing helps here.

- " Does the device reliably enumerate on the bus, i.e. reading registers 2 and 3 to get its ID?"
Reading the registers PHYSID1 and PHYSID2 reports also 0xFFFF

- " If the PHY is broken, by some yet to be determined definition of broken, we try to limit the workaround to as narrow as possible. So it should not be in the core probe code. It should be in the PHY specific driver, and ideally for only its ID, not the whole vendors family of PHYs"
I have several workarounds/fixed for that, the easy way is set the PHY capabilities in the smsc.c driver " .features	= PHY_BASIC_FEATURES" like in the past and it works fine. Also I have another fix in the same driver adding a custom function for " get_features" where if I read 0xFFFF or 0x0000 return a EPROBE_DEFER. However after review another drivers (net/usb/pegasus.c , net/Ethernet/toshiba/spider_net.c, net/sis/sis190.c, and more...)  that also checks the result of read MII_BMSR against 0x0000 and 0xFFFF , I tried to send a common fix in the PHY core. From my point of view read a 0x0000 or 0xFFFF value is an error in the probe step like if the phy_read() returns an error, so I think that the PHY core should consider return a EPROBE_DEFER to at least provide a second try for that PHY device.

Thanks,

Arturo.

-----Original Message-----
From: Heiner Kallweit <hkallweit1@...il.com> 
Sent: viernes, 17 de marzo de 2023 19:21
To: Buzarra, Arturo <Arturo.Buzarra@...i.com>
Cc: netdev@...r.kernel.org; Florian Fainelli <f.fainelli@...il.com>; Andrew Lunn <andrew@...n.ch>; Russell King - ARM Linux <linux@...linux.org.uk>
Subject: Re: [PATCH] net: phy: return EPROBE_DEFER if PHY is not accessible

[EXTERNAL E-MAIL] Warning! This email originated outside of the organization! Do not click links or open attachments unless you recognize the sender and know the content is safe.



On 17.03.2023 13:16, arturo.buzarra@...i.com wrote:
> From: Arturo Buzarra <arturo.buzarra@...i.com>
>
> A PHY driver can dynamically determine the devices features, but in 
> some circunstances, the PHY is not yet ready and the read capabilities 
> does not fail but returns an undefined value, so incorrect 
> capabilities are assumed and the initialization process fails. This 
> commit postpones the PHY probe to ensure the PHY is accessible.
>
To complement what has been said by Florian and Andrew:

"under some circumstances" is too vague in general. List potential such circumstances and best what happened exactly in your case.

When genphy_read_abilities() is called the PHY has been accessed already, reading the PHY ID.

So best start with some details about your use case, which MAC, which PHY, etc.

> Signed-off-by: Arturo Buzarra <arturo.buzarra@...i.com>
> ---
>  drivers/net/phy/phy_device.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/phy/phy_device.c 
> b/drivers/net/phy/phy_device.c index 1785f1cead97..f8c31e741936 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -2628,10 +2628,14 @@ int genphy_read_abilities(struct phy_device *phydev)
>                              phydev->supported);
>
>       val = phy_read(phydev, MII_BMSR);
>       if (val < 0)
>               return val;
> +     if (val == 0x0000 || val == 0xffff) {
> +             phydev_err(phydev, "PHY is not accessible\n");
> +             return -EPROBE_DEFER;
> +     }
>
>       linkmode_mod_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported,
>                        val & BMSR_ANEGCAPABLE);
>
>       linkmode_mod_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, 
> phydev->supported,

Powered by blists - more mailing lists