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 PHYsnits: 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. 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 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?
Thank you! Alex
/* If the PHY ID is mostly f's, we didn't find anything */ if (r == 0 && (phy_id & 0x1fffffff) != 0x1fffffff) { is_c45 = (devad == MDIO_DEVAD_NONE) ? false : true;--
Regards, Bin _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot