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 <rog...@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
signature.asc
Description: PGP signature
