On January 5, 2019 5:21:03 AM PST, Heiner Kallweit <hkallwe...@gmail.com> wrote:
>During a bug analysis we came across the fact that there's no guarantee
>that reading from the ID registers returns a valid value if PHY is
>powered down. When reading invalid values we may load no or the wrong
>PHY driver. Therefore let's play safe and power up the PHY before
>reading the ID registers in case PHY is powered down.
>
>Suggested-by: Florian Fainelli <f.faine...@gmail.com>
>Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
>---
> drivers/net/phy/phy_device.c | 23 +++++++++++++----------
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
>diff --git a/drivers/net/phy/phy_device.c
>b/drivers/net/phy/phy_device.c
>index 9560a2b84..d4702313d 100644
>--- a/drivers/net/phy/phy_device.c
>+++ b/drivers/net/phy/phy_device.c
>@@ -754,19 +754,22 @@ static int get_phy_id(struct mii_bus *bus, int
>addr, u32 *phy_id,
> if (is_c45)
> return get_phy_c45_ids(bus, addr, phy_id, c45_ids);
>
In premise this would also apply to C45 PHYs though not sure whether this would
be on a per-package basis or global.
>+ phy_reg = mdiobus_read(bus, addr, MII_BMCR);
>+ if (phy_reg < 0)
>+ /* returning -ENODEV allows to continue bus-scanning */
>+ return (phy_reg == -EIO || phy_reg == -ENODEV) ? -ENODEV : -EIO;
>+
>+ /* PHY may be powered down and ID registers invalid */
>+ if (phy_reg & BMCR_PDOWN) {
>+ mdiobus_write(bus, addr, MII_BMCR, phy_reg & ~BMCR_PDOWN);
>+ /* give the PHY some time to resume */
>+ msleep(100);
>+ }
We should check the 802.3 spec and see if we can reference an upper bound being
defined there, if not, I suppose 100ms is as good as any other delay.
There is one potential concern here which is that if the PHY device is probed
but never used subsequently or the connect()/attach() operation is done much
much longer, there is a missed opportunity for keeping the PHY in a low power
mode. I would take the PHY out of power down, read its ID, then put it back to
power down? Then the state machine takes care of taking it out of power down
again.
Another thing to possibly watch out for is what the mii_bus::reset callback
might have done to the PHY.
>+
> /* Grab the bits from PHYIR1, and put them in the upper half */
Typo here: MII_PHYSID1?
--
Florian
