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

Tue Jul 9 13:25:38 UTC 2019

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?

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