From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiner Kallweit Subject: Re: [PATCH net-next 07/10] r8169: migrate speed_down function to phylib Date: Mon, 9 Jul 2018 23:04:26 +0200 Message-ID: References: <096a5326-963c-9bef-6218-29fcde004111@gmail.com> <5be3d523-bb26-c2d6-71f4-165e9c6e45a9@gmail.com> <20180702212005.GF12564@lunn.ch> <8d516fa8-30a5-df7e-737c-119d6e458d80@gmail.com> <22120794-fc3c-d813-2529-d37b823a80c2@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: David Miller , Realtek linux nic maintainers , "netdev@vger.kernel.org" To: Florian Fainelli , Andrew Lunn Return-path: Received: from mail-wr1-f68.google.com ([209.85.221.68]:39525 "EHLO mail-wr1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933003AbeGIVK7 (ORCPT ); Mon, 9 Jul 2018 17:10:59 -0400 Received: by mail-wr1-f68.google.com with SMTP id h10-v6so12376484wre.6 for ; Mon, 09 Jul 2018 14:10:59 -0700 (PDT) In-Reply-To: <22120794-fc3c-d813-2529-d37b823a80c2@gmail.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 03.07.2018 18:48, Florian Fainelli wrote: > > > On 07/02/2018 02:31 PM, Heiner Kallweit wrote: >> On 02.07.2018 23:20, Andrew Lunn wrote: >>> On Mon, Jul 02, 2018 at 09:37:08PM +0200, Heiner Kallweit wrote: >>>> Change rtl_speed_down() to use phylib. >>>> >>>> Signed-off-by: Heiner Kallweit >>>> --- >>>> drivers/net/ethernet/realtek/r8169.c | 33 +++++++++++++--------------- >>>> 1 file changed, 15 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c >>>> index 311321ee..807fbc75 100644 >>>> --- a/drivers/net/ethernet/realtek/r8169.c >>>> +++ b/drivers/net/ethernet/realtek/r8169.c >>>> @@ -4240,6 +4240,10 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp) >>>> rtl_writephy(tp, 0x0b, 0x0000); //w 0x0b 15 0 0 >>>> } >>>> >>>> + /* We may have called rtl_speed_down before */ >>>> + dev->phydev->advertising = dev->phydev->supported; >>>> + genphy_config_aneg(dev->phydev); >>>> + >>>> genphy_soft_reset(dev->phydev); >>>> >>>> rtl8169_set_speed(dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL, >>>> @@ -4323,28 +4327,21 @@ static void rtl_init_mdio_ops(struct rtl8169_private *tp) >>>> } >>>> } >>>> >>>> +#define BASET10 (ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full) >>>> +#define BASET100 (ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full) >>>> +#define BASET1000 (ADVERTISED_1000baseT_Half | ADVERTISED_1000baseT_Full) >>>> + >>>> static void rtl_speed_down(struct rtl8169_private *tp) >>>> { >>>> - u32 adv; >>>> - int lpa; >>>> + struct phy_device *phydev = tp->dev->phydev; >>>> + u32 adv = phydev->lp_advertising & phydev->supported; >>>> >>>> - rtl_writephy(tp, 0x1f, 0x0000); >>>> - lpa = rtl_readphy(tp, MII_LPA); >>>> + if (adv & BASET10) >>>> + phydev->advertising &= ~(BASET100 | BASET1000); >>>> + else if (adv & BASET100) >>>> + phydev->advertising &= ~BASET1000; >>>> >>>> - if (lpa & (LPA_10HALF | LPA_10FULL)) >>>> - adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full; >>>> - else if (lpa & (LPA_100HALF | LPA_100FULL)) >>>> - adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full | >>>> - ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full; >>>> - else >>>> - adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full | >>>> - ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full | >>>> - (tp->mii.supports_gmii ? >>>> - ADVERTISED_1000baseT_Half | >>>> - ADVERTISED_1000baseT_Full : 0); >>>> - >>>> - rtl8169_set_speed(tp->dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL, >>>> - adv); >>>> + genphy_config_aneg(phydev); >>>> } >>> >>> It probably it is me being too tired, but i don't get what this is >>> doing? Changing the local advertisement based on what the remote is >>> advertising. Why? >>> >> It also took me some time to understand what this speed_down is doing. >> If we suspend and wait for a WoL packet, then we don't have to burn all >> the energy for a GBit connection. Therefore we switch to the lowest >> speed supported by chip and link partner. This is done by removing >> higher speeds from the advertised modes and restarting an autonego. > > This is something that the tg3 driver also does, we should probably > consider doing this as part of a generic PHY library helpers since I was > told by several HW engineers that usually 10Mbits for WoL is much more > energy efficient. > Yes, I agree this should become part of phylib. I'd prefer to do it after this r8169 patch series, will spend a few thoughts on how to do it best, also considering your remark below. Heiner > One thing that bothers me a bit is that this should ideally be offered > as both blocking and non-blocking options, because we might want to make > sure that at the time we suspend, and we already had a link established, > we successfully re-negotiate the link with the partner. I agree that > there could be any sort of link disruption happening at any point though.. >