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