On Tue, Jul 9, 2019 at 10:17 AM Alex Marginean <[email protected]> wrote: > > On 7/9/2019 4:25 PM, Joe Hershberger wrote: > > On Tue, Jul 9, 2019 at 7:53 AM Alex Marginean <[email protected]> > > wrote: > >> > >> Hi Joe, > >> > >> On 7/8/2019 8:08 PM, Joe Hershberger wrote: > >>> On Wed, Jul 3, 2019 at 4:01 AM Alex Marginean <[email protected]> > >>> 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 <[email protected]> > >>>>> wrote: > >>>>>> > >>>>>> On 7/1/2019 11:15 AM, Bin Meng wrote: > >>>>>>> Hi Alex, > >>>>>>> > >>>>>>> On Wed, Jun 19, 2019 at 5:07 PM Alexandru Marginean > >>>>>>> <[email protected]> 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 <[email protected]> > >>>>>>>> --- > >>>>>>>> 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. > > > >> > >> 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 > >>>> [email protected] > >>>> https://lists.denx.de/listinfo/u-boot > >> > >> _______________________________________________ > >> U-Boot mailing list > >> [email protected] > >> https://lists.denx.de/listinfo/u-boot > > _______________________________________________ > U-Boot mailing list > [email protected] > https://lists.denx.de/listinfo/u-boot _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
