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

[Alex Marginean]

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.
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
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?

Thank you!
Alex

> 
>> +
>>                  /* If the PHY ID is mostly f's, we didn't find anything */
>>                  if (r == 0 && (phy_id & 0x1fffffff) != 0x1fffffff) {
>>                          is_c45 = (devad == MDIO_DEVAD_NONE) ? false : true;
>> --
> 
> Regards,
> Bin
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
>