Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754492AbaBABhM (ORCPT ); Fri, 31 Jan 2014 20:37:12 -0500 Received: from mail-pd0-f174.google.com ([209.85.192.174]:59674 "EHLO mail-pd0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754145AbaBABhK (ORCPT ); Fri, 31 Jan 2014 20:37:10 -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> From: Florian Fainelli Date: Fri, 31 Jan 2014 17:36:29 -0800 Message-ID: Subject: Re: [PATCH v2 2/4] net: ethoc: don't advertise gigabit speed on attached PHY To: Max Filippov 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 2014-01-31 Max Filippov : > 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. 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 -- 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/