Re: [PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on attached PHY
From: Max Filippov <jcmvbkbc@gmail.com>
Date: 2014-02-01 01:10:25
Also in:
lkml
Possibly related (same subject, not in this thread)
- 2014-01-29 · Re: [PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on attached PHY · Max Filippov <jcmvbkbc@gmail.com>
- 2014-01-29 · Re: [PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on attached PHY · Florian Fainelli <f.fainelli@gmail.com>
- 2014-01-29 · [PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on attached PHY · Max Filippov <jcmvbkbc@gmail.com>
On Sat, Feb 1, 2014 at 4:54 AM, Florian Fainelli [off-list ref] wrote:
2014-01-31 Florian Fainelli [off-list ref]:quoted
quoted
quoted
Maybe they boot up with gigabit advertisement disabled in their PHY and thus they don't see the problem?quoted
Other drivers do the following: - connect to the PHY - phydev->supported = PHY_BASIC_FEATURES - phydev->advertising &= phydev->supported - start the PHY state machine And they work just fine. Is the PHY driver you are bound to the "Generic PHY" or something else which does something funky in config_aneg()?It's marvell 88E1111 from the KC-705 board, but the behaviour doesn't change if I disable it and the generic phy is used.Florian, I don't see how the generic genphy_config_advert can ever change gigabit advertisement if phydev->supported has gigabit speeds masked off.It does not, but it makes sure that phydev->advertising gets masked with phydev->supported. So, prior to starting your PHY state machine, if you do: phydev->supported &= PHY_BASIC_FEATURES; phydev->advertising = phydev->supported;It looks like there might be one problem however, if we have the following: - phydev->supported masks off the Gigabit features - PHY device comes out of reset/default with Gigabit features set in MII_CTRL1000
That's exactly my case.
quoted hunk ↗ jump to hunk
Could you try the following patch:diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 78bf1a4..b607f4a 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c@@ -749,25 +749,24 @@ static int genphy_config_advert(struct phy_device *phydev) } /* Configure gigabit if it's supported */ + adv = phy_read(phydev, MII_CTRL1000); + if (adv < 0) + return adv; + + oldadv = adv; + adv &= ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF); + if (phydev->supported & (SUPPORTED_1000baseT_Half | SUPPORTED_1000baseT_Full)) { - adv = phy_read(phydev, MII_CTRL1000); - if (adv < 0) - return adv; - - oldadv = adv; - adv &= ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF); adv |= ethtool_adv_to_mii_ctrl1000_t(advertise); - - if (adv != oldadv) { - err = phy_write(phydev, MII_CTRL1000, adv); - - if (err < 0) - return err; + if (adv != oldadv) changed = 1; - } } + err = phy_write(phydev, MII_CTRL1000, adv);
I don't think this is correct: MII_CTRL1000 is reserved for 10/100 PHYs, we probably need to read/write it conditionally, depending on MII_BMSR BMSR_ESTATEN bit. Otherwise yes, it works for me too.
+ if (err < 0)
+ return err;
+
return changed;
}-- Thanks. -- Max