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

Max Filippov
2014-02-01T01:10:25Z

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.

> + if (err < 0)
> + return err;
> +
> return changed;
> }

--
Thanks.
-- Max