Re: [PATCH RFC] ethernet: stmmac: clean up the code for release/suspend/resume function

[Jisheng Zhang <Jisheng.Zhang@synaptics.com>]

On Mon,  7 Dec 2020 19:38:49 +0800 Joakim Zhang wrote:


> 
> commit 1c35cc9cf6a0 ("net: stmmac: remove redundant null check before clk_disable_unprepare()"),
> have not clean up check NULL clock parameter completely, this patch did it.
> 
> commit e8377e7a29efb ("net: stmmac: only call pmt() during suspend/resume if HW enables PMT"),
> after this patch, we use
> if (device_may_wakeup(priv->device) && priv->plat->pmt) check MAC wakeup
> if (device_may_wakeup(priv->device)) check PHY wakeup
> Add oneline comment for readability.
> 
> commit 77b2898394e3b ("net: stmmac: Speed down the PHY if WoL to save energy"),
> slow down phy speed when release net device under any condition.
> 
> Slightly adjust the order of the codes so that suspend/resume look more
> symmetrical, generally speaking they should appear symmetrically.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 22 +++++++++----------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index c33db79cdd0a..a46e865c4acc 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2908,8 +2908,7 @@ static int stmmac_release(struct net_device *dev)
>         struct stmmac_priv *priv = netdev_priv(dev);
>         u32 chan;
> 
> -       if (device_may_wakeup(priv->device))

This check is to prevent link speed down if the stmmac isn't a wakeup device.

> -               phylink_speed_down(priv->phylink, false);
> +       phylink_speed_down(priv->phylink, false);
>         /* Stop and disconnect the PHY */
>         phylink_stop(priv->phylink);
>         phylink_disconnect_phy(priv->phylink);
> @@ -5183,6 +5182,7 @@ int stmmac_suspend(struct device *dev)
>         } else {
>                 mutex_unlock(&priv->lock);
>                 rtnl_lock();
> +               /* For PHY wakeup case */
>                 if (device_may_wakeup(priv->device))
>                         phylink_speed_down(priv->phylink, false);
>                 phylink_stop(priv->phylink);
> @@ -5260,11 +5260,17 @@ int stmmac_resume(struct device *dev)
>                 /* enable the clk previously disabled */
>                 clk_prepare_enable(priv->plat->stmmac_clk);
>                 clk_prepare_enable(priv->plat->pclk);
> -               if (priv->plat->clk_ptp_ref)
> -                       clk_prepare_enable(priv->plat->clk_ptp_ref);
> +               clk_prepare_enable(priv->plat->clk_ptp_ref);

I think this 3 line modifications can be a separated patch.

>                 /* reset the phy so that it's ready */
>                 if (priv->mii)
>                         stmmac_mdio_reset(priv->mii);
> +
> +               rtnl_lock();
> +               phylink_start(priv->phylink);
> +               /* We may have called phylink_speed_down before */
> +               if (device_may_wakeup(priv->device))
> +                       phylink_speed_up(priv->phylink);
> +               rtnl_unlock();

This is moving phylink op before mac setup, I'm not sure whether this is safe.

>         }
> 
>         if (priv->plat->serdes_powerup) {
> @@ -5275,14 +5281,6 @@ int stmmac_resume(struct device *dev)
>                         return ret;
>         }
> 
> -       if (!device_may_wakeup(priv->device) || !priv->plat->pmt) {
> -               rtnl_lock();
> -               phylink_start(priv->phylink);
> -               /* We may have called phylink_speed_down before */
> -               phylink_speed_up(priv->phylink);
> -               rtnl_unlock();
> -       }
> -
>         rtnl_lock();
>         mutex_lock(&priv->lock);
> 
> --
> 2.17.1
>