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

[Tom Rini]

On Thu, Aug 24, 2023 at 11:34:29PM +0530, Siddharth Vadapalli wrote:
> Hello Roger,
> 
> On 22-08-2023 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.
> > 
> > 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 at 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);
> > +	}
> 
> After rethinking about the above implementation and the second patch of
> this series, the second patch could be dropped altogether if the
> following implementation is acceptable:
> 
> phydev = phy_connect(cpsw_common->bus,
> 		     priv->phy_addr,
> 		     priv->dev,
> 		     pdata->phy_interface);
> 
> if (!phydev) {
> 	/* Some boards (e.g. beagleplay) don't work on first go */
> 	mdelay(50);
> 	phydev = phy_connect(cpsw_common->bus,
> 			     priv->phy_addr,
> 			     priv->dev,
> 			     pdata->phy_interface);
> }
> 
> if (!phydev) {
> 	dev_err(dev, "phy_connect() failed\n");
> ...
> 
> With this, there would be at most one "PHY not found" print, which
> should be fine. The mdelay value of 50 could be replaced with a
> sufficiently large value which guarantees success for Beagleplay.

Ramon, thoughts?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20230824/7a83d424/attachment.sig>