[U-Boot] [PATCH] drivers: net: phy: Ignore PHY ID 0 during PHY probing

[Bin Meng]

Hi Alex,

On Wed, Jul 3, 2019 at 3:09 AM Alex Marginean <alexm.osslist at gmail.com> wrote:
>
> On 7/1/2019 11:15 AM, Bin Meng wrote:
> > Hi Alex,
> >
> > On Wed, Jun 19, 2019 at 5:07 PM Alexandru Marginean
> > <alexandru.marginean at nxp.com> wrote:
> >>
> >> Current code fails to probe some C45 PHYs that also respond to C22 reads.
> >> This is the case for PHYs like Aquantia AQR112, Marvell 88X2242 (as
> >> previously posted on the u-boot list).
> >> If the PHY ID reads all 0s just ignore it and try the next devad.
> >>
> >> Signed-off-by: Alex Marginean <alexm.osslist at gmail.com>
> >> ---
> >>   drivers/net/phy/phy.c | 9 +++++++++
> >>   1 file changed, 9 insertions(+)
> >>
> >> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> >> index c1c1af9abd..7ccbc4d9da 100644
> >> --- a/drivers/net/phy/phy.c
> >> +++ b/drivers/net/phy/phy.c
> >> @@ -727,6 +727,15 @@ static struct phy_device *create_phy_by_mask(struct mii_dev *bus,
> >>          while (phy_mask) {
> >>                  int addr = ffs(phy_mask) - 1;
> >>                  int r = get_phy_id(bus, addr, devad, &phy_id);
> >> +
> >> +               /* If the PHY ID is flat 0 we ignore it.  There are C45 PHYs
> >
> > nits: the multi-line comment block format is wrong
>
> You're right, I'll fix that.
>
> >
> >> +                * that return all 0s for C22 reads (like Aquantia AQR112) and
> >> +                * there are C22 PHYs that return all 0s for C45 reads (like
> >> +                * Atheros AR8035).
> >> +                */
> >> +               if (phy_id == 0)
> >> +                       return NULL;
> >
> > Should this be "continue"?
>
> In case there are C45 and C22 PHYs mixed on the same bus and they are
> all flagged in phy_mask?  In general this function shouldn't end up
> dealing with multiple PHYs, if it does then it's possible the result
> won't the the right one.
>
> If create_phy_by_mask is called from get_phy_device, then we're only
> looking for one PHY and if that reads PHY_ID 0 we can just assume we're
> not using the correct devad.
>
> create_phy_by_mask can also be called from phy_connect (or some other
> place) with phy_mask = 0xffffffff.  The assumption in that case is that
> there is one PHY on the given MDIO bus.

Yes, this is the user scenario that concerns me. But on a shared MDIO
bus, there are multiple PHYs. I remember lots of old Freescale PowerPC
boards did this way. For example, there are multiple eTSEC in the SoC,
and each eTSEC claims to have one MDIO controller, however Freescale
chose to wire all PHYs on a single MDIO bus which usually is eTSEC0
(the first eTSEC).

> If there are several PHYs then we're going into a gray area, the result
> isn't explicitly defined.  We could try to probe the PHY with the

I suspect this function is not being used widely hence not exposing
any known issues?

> smallest addr.  This change shouldn't break that, assuming PHY_ID is !=
> 0 for at least one devad used.  Or we could keep the current devad and
> continue scanning for the next addr, continue instead of return would
> achieve that.  I don't really have a strong preference for either, the
> message for developers should be to avoid ending up in this situation
> altogether.
>
> What do you think?

Regards,
Bin