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

Florian Fainelli
2014-02-01T01:37:12Z

2014-01-31 Max Filippov <jcmvbkbc@gmail.com>:
> On Sat, Feb 1, 2014 at 4:54 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> 2014-01-31 Florian Fainelli <f.fainelli@gmail.com>:
>>>>> 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
>
> That's exactly my case.
>
>> 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.

You are right, that needs to be made conditional. I will give this
patch some more proper testing on a truly non-gigabit PHY. Thanks!
--
Florian