Re: [PATCH 1/2] net: ti: am65-cpsw-nuss: Workaround for buggy PHY/Board

[Roger Quadros <rogerq@kernel.org>]

Hi Siddharth,

On 23/08/2023 07:35, Siddharth Vadapalli wrote:
> Roger,
> 
> On 22/08/23 17:43, Roger Quadros wrote:
>> Beagleplay has a buggy Ethernet PHY implementation for the Gigabit
>> PHY in the sense that it is non responsive over MDIO immediately
>> after power-up/reset.
>>
>> We need to either try multiple times or wait sufficiently long enough
>> (couple of 10s of ms?) before the PHY begins to respond correctly.
> 
> Based on the datasheet at:
> https://datasheet.lcsc.com/lcsc/1912111437_Realtek-Semicon-RTL8211F-CG_C187932.pdf
> in the section 7.17. PHY Reset (Hardware Reset), it is stated that
> software has to wait for at least 50 ms before accessing the PHY
> registers. Is this why the for-loop in the code below tries for at most
> 5 times with a delay of 10 ms before the next try? If yes, then isn't it

Good catch. This might be the reason why PHY is not responding in the first
few attempts.

> better to move the delay into the realtek PHY driver at
> drivers/net/phy/realtek.c

We are currently at the MDIO bus driver where we don't even know what PHY
is on the bus. So this delay cannot come at the realtec PHY driver.
It has to come at the MDIO bus driver level.

> Shouldn't it be the PHY driver which ensures that any reads/writes to the PHY
> registers are valid? It can ensure this by having a one time 50ms delay for the
> very first access to the PHY registers.> 
>>
>> One theory is that the PHY is configured to operate on MDIO address 0
>> which it treats as a special broadcast address.
>>
>> Datasheet states:
>> "PHYAD (config pins) sets the PHY address for the device.
>> The RTL8211F(I)/RTL8211FD(I) supports PHY addresses from 0x01 to 0x07.
>> Note 1: An MDIO command with PHY address=0 is a broadcast from the MAC;
>> each PHY device should respond."
>>
>> This issue is not seen with the other PHY (different make) on the board
>> which is configured for address 0x1.
>>
>> As a woraround we try to probe the PHY multiple times instead of giving
>> up on the first attempt.
>>
>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>> ---
>>  drivers/net/ti/am65-cpsw-nuss.c | 19 ++++++++++++++-----
>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c
>> index 51a8167d14..4f8a2f9b93 100644
>> --- a/drivers/net/ti/am65-cpsw-nuss.c
>> +++ b/drivers/net/ti/am65-cpsw-nuss.c
>> @@ -27,6 +27,7 @@
>>  #include <syscon.h>
>>  #include <linux/bitops.h>
>>  #include <linux/soc/ti/ti-udma.h>
>> +#include <linux/delay.h>
>>  
>>  #include "cpsw_mdio.h"
>>  
>> @@ -678,14 +679,22 @@ static int am65_cpsw_phy_init(struct udevice *dev)
>>  	struct am65_cpsw_priv *priv = dev_get_priv(dev);
>>  	struct am65_cpsw_common *cpsw_common = priv->cpsw_common;
>>  	struct eth_pdata *pdata = dev_get_plat(dev);
>> -	struct phy_device *phydev;
>>  	u32 supported = PHY_GBIT_FEATURES;
>> +	struct phy_device *phydev;
>> +	int tries;
>>  	int ret;
>>  
>> -	phydev = phy_connect(cpsw_common->bus,
>> -			     priv->phy_addr,
>> -			     priv->dev,
>> -			     pdata->phy_interface);
>> +	/* Some boards (e.g. beagleplay) don't work on first go */
>> +	for (tries = 1; tries <= 5; tries++) {
>> +		phydev = phy_connect(cpsw_common->bus,
>> +				     priv->phy_addr,
>> +				     priv->dev,
>> +				     pdata->phy_interface);
>> +		if (phydev)
>> +			break;
>> +
>> +		mdelay(10);
>> +	}
>>  
>>  	if (!phydev) {
>>  		dev_err(dev, "phy_connect() failed\n");
> 

-- 
cheers,
-roger