Re: [PATCH v2 2/4] net: ethoc: don't advertise gigabit speed onattached PHY

[Florian Fainelli]

2014-01-31 Florian Fainelli <f.fainelli@xxxxxxxxx>:
>>> Maybe they boot up with gigabit advertisement disabled in their PHY
>>> and thus they don't see the problem?
>>>
>>>> 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

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);
+       if (err < 0)
+               return err;
+
        return changed;
 }

-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/