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

[Alex Marginean <alexm.osslist@gmail.com>]

On 7/9/2019 4:25 PM, Joe Hershberger wrote:
> On Tue, Jul 9, 2019 at 7:53 AM Alex Marginean <alexm.osslist@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@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@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@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@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.


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