Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932750AbaBABKZ (ORCPT ); Fri, 31 Jan 2014 20:10:25 -0500 Received: from mail-oa0-f49.google.com ([209.85.219.49]:44713 "EHLO mail-oa0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754534AbaBABKX (ORCPT ); Fri, 31 Jan 2014 20:10:23 -0500 MIME-Version: 1.0 In-Reply-To: References: <1390975218-13863-1-git-send-email-jcmvbkbc@gmail.com> <1390975218-13863-3-git-send-email-jcmvbkbc@gmail.com> <52E8A407.1020809@gmail.com> Date: Sat, 1 Feb 2014 05:10:22 +0400 Message-ID: Subject: Re: [PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on attached PHY From: Max Filippov To: Florian Fainelli Cc: Marc Gauthier , Ben Hutchings , LKML , "David S. Miller" , Chris Zankel , "linux-xtensa@linux-xtensa.org" , netdev Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, Feb 1, 2014 at 4:54 AM, Florian Fainelli wrote: > 2014-01-31 Florian Fainelli : >>>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/