Re: [PATCH RFC 13/18] r8168: replace speed_down with genphy_restart_aneg

[Andrew Lunn <andrew@lunn.ch>]

On Thu, Dec 21, 2017 at 09:50:39PM +0100, Heiner Kallweit wrote:
> Dealing with link partner abilities is handled by phylib, so let's
> just trigger autonegotiation here.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/ethernet/realtek/r8168.c | 26 +-------------------------
>  1 file changed, 1 insertion(+), 25 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8168.c b/drivers/net/ethernet/realtek/r8168.c
> index d33f93a31..6b398915f 100644
> --- a/drivers/net/ethernet/realtek/r8168.c
> +++ b/drivers/net/ethernet/realtek/r8168.c
> @@ -4360,30 +4360,6 @@ static void rtl_init_mdio_ops(struct rtl8168_private *tp)
>  	}
>  }
>  
> -static void rtl_speed_down(struct rtl8168_private *tp)
> -{
> -	u32 adv;
> -	int lpa;
> -
> -	rtl_writephy(tp, 0x1f, 0x0000);
> -	lpa = rtl_readphy(tp, MII_LPA);
> -
> -	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);
> -
> -	rtl8168_set_speed(tp->dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
> -			  adv);
> -}
> -
>  static void rtl_wol_suspend_quirk(struct rtl8168_private *tp)
>  {
>  	void __iomem *ioaddr = tp->mmio_addr;
> @@ -4424,7 +4400,7 @@ static bool rtl_wol_pll_power_down(struct rtl8168_private *tp)
>  	if (!(__rtl8168_get_wol(tp) & WAKE_ANY))
>  		return false;
>  
> -	rtl_speed_down(tp);
> +	genphy_restart_aneg(tp->dev->phydev);
>  	rtl_wol_suspend_quirk(tp);
>  
>  	return true;

I'm not too clear what is going on here? Is this suspend while WOL is
enabled? There should be no need to change the PHY settings. The PHY
driver should leave the PHY running in whatever state it was
configured to. The only danger here is that the MAC driver has called
phy_stop() during suspend. That should not be done when WOL is
enabled.

Is the wol being passed to the phylib? phy_ethtool_set_wol() and
phy_ethtool_get_wol()?

	Andrew