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

Alex Marginean alexm.osslist at gmail.com
Thu Jul 11 15:36:04 UTC 2019 

On 7/9/2019 7:15 PM, Joe Hershberger wrote:
> On Tue, Jul 9, 2019 at 10:17 AM Alex Marginean <alexm.osslist at gmail.com> wrote:
>>
>> On 7/9/2019 4:25 PM, Joe Hershberger wrote:
>>> On Tue, Jul 9, 2019 at 7:53 AM Alex Marginean <alexm.osslist at gmail.com> wrote:
>>>>
>>>> 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.
>>>
>>> I agree... if you have more than one phy, on the same MDIO, you had
>>> better have them mapped to MACs explicitly. I'm more curious if there
>>> are use-cases where you would want to ignore a C45 phy. What kind of
>>> performance hit would we take for continuing to scan, just so that we
>>> can warn that the DT should list the phy addrs?
>>
>> Top of my head, reading all 32 addresses could go up to a few
>> milliseconds.  This is really only useful for development set-ups
>> really, for devices in the field it doesn't feel right to scan the
>> entire bus if it happens that the only PHY on that bus is at a low addr.
>> Not to mention that no one would see a warning anyway.
>> So maybe include this as a debug option, or rather just document that
>> phy_find_by_mask should only be called if there is a single PHY on the
>> bus, otherwise the result is not guaranteed.
> 
> I agree that it doesn't make sense in production. I'm not sure there's
> enough value in dev to add this feature. Adding a little documentation
> is probably about the right level of work.

Sent as a separate patch with comments for PHY APIs.

Thank you!
Alex

More information about the U-Boot mailing list