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

Tue Jul 9 12:53:28 UTC 2019

Hi Joe,

On 7/8/2019 8:08 PM, Joe Hershberger wrote:
> On Wed, Jul 3, 2019 at 4:01 AM Alex Marginean <alexm.osslist at gmail.com> wrote:
>>
>> Hi Bin,
>>
>> On 7/3/2019 10:39 AM, Bin Meng wrote:
>>> 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).
>>
>> Virtually all Freescale networking SoCs are like that, each MAC has its
>> own MDIO controller but those are in fact connected to the SoC internal
>> PHYs not to the external PHYs on the board.  These SoCs have one or two
>> MDIOs dedicated to the external PHYs.  If the SoC supports 10G
>> interfaces then the SoC typically has two external MDIOs, the intent
>> being that one is used for C22 and the other for C45 PHYs.
>> Of course MDIO buses with multiple PHYs have to work.  My point is that
>> one should not end up in this function with a phy_mask with multiple
>> bits set if the MDIO bus has multiple PHYs connected to it, in that
>> set-up the caller should know the PHY address up front and set only one
>> bit in phy_mask.
>>
>>>
>>>> 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?
>>
>> At least for qoriq and layerscape Freescale SoCs the PHY address is
>> known up front before calling this function, precisely because the MDIO
>> bus is shared, but I'm guessing other SoCs do rely on scanning to get
>> to the PHYs.
>>
>> Joe, do you have a preference between return and continue when we hit
>> a PHY_ID == 0?  Are you OK with continue?
>>
>> I wonder if it would be useful to scan all IDs in this function, instead
>> of returning on 1st hit, just to issue a warning if multiple addresses
>> flagged in phy_mask return valid PHY IDs.  Any thoughts?
> 
> Scanning all seems like a bit of an independent question. I think the
> return vs continue decision at a higher level is a decision about
> whether a phy scan should include or ignore C45 phys, correct? Scan
> all seems like it applies to all included phys, C45 being included or
> not based on the former decision.
> 
> Is there a use case anyone is aware of where C45 phys are expected to
> be scanned for?

Assuming there's just one PHY on the bus that's scanned, and it's a C45
PHY, scanning should actually work (that case works with both continue
and return NULL in fact).

create_phy_by_mask is going to read PHY_ID 0 over C22, it can continue
and not find anything else using C22 reads, finally return NULL.
get_phy_device_by_mask will call again with the next devad.  Assuming
the PHY replies to devad 1 or one of the other ones used in
get_phy_device_by_mask, the PHY will be probed as a C45 PHY.

Things aren't as good if there is a bus with multiple PHYs, especially
if they are mixed, C22 with C45.  In that case relying on scanning looks
like a bad idea though.

Alex

>>
>>
>>>> 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
>>>
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> https://lists.denx.de/listinfo/u-boot