Re: [PATCH 2/3] net: stmmac: dwmac-rk: keep the PHY up for WoL

[Heiko Stuebner]

Am Freitag, 10. Juni 2016, 18:00:38 schrieb Vincent Palatin:
> When suspending the machine, do not shutdown the external PHY by cutting
> its regulator in the mac platform driver suspend code if Wake-on-Lan is
> enabled, else it cannot wake us up.
> In order to do this, split the suspend/resume callbacks from the
> init/exit callbacks, so we can condition the power-down on the lack of
> need to wake-up from the LAN but do it unconditionally when unloading the
> module.
> 
> Signed-off-by: Vincent Palatin <vpalatin@xxxxxxxxxxxx>
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 49
> +++++++++++++++++++++++--- 1 file changed, 44 insertions(+), 5
> deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c index 0cd3ecf..fa05771
> 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> @@ -46,6 +46,7 @@ struct rk_priv_data {
>  	struct platform_device *pdev;
>  	int phy_iface;
>  	struct regulator *regulator;
> +	bool powered_down;
>  	const struct rk_gmac_ops *ops;
> 
>  	bool clk_enabled;
> @@ -529,9 +530,8 @@ static struct rk_priv_data *rk_gmac_setup(struct
> platform_device *pdev, return bsp_priv;
>  }
> 
> -static int rk_gmac_init(struct platform_device *pdev, void *priv)
> +static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
>  {
> -	struct rk_priv_data *bsp_priv = priv;
>  	int ret;
> 
>  	ret = phy_power_on(bsp_priv, true);
> @@ -542,15 +542,52 @@ static int rk_gmac_init(struct platform_device
> *pdev, void *priv) if (ret)
>  		return ret;
> 
> +	bsp_priv->powered_down = true;
> +
>  	return 0;
>  }
> 
> -static void rk_gmac_exit(struct platform_device *pdev, void *priv)
> +static void rk_gmac_powerdown(struct rk_priv_data *gmac)
>  {
> -	struct rk_priv_data *gmac = priv;
> -
>  	phy_power_on(gmac, false);
>  	gmac_clk_enable(gmac, false);
> +	gmac->powered_down = true;

naming it gmac->suspended and doing all accesses in the suspend/resume 
callback might provide a nicer way? Now the check is in resume while the 
powerdown callback is setting it.

> +}
> +
> +static int rk_gmac_init(struct platform_device *pdev, void *priv)
> +{
> +	struct rk_priv_data *bsp_priv = priv;
> +
> +	return rk_gmac_powerup(bsp_priv);
> +}
> +
> +static void rk_gmac_exit(struct platform_device *pdev, void *priv)
> +{
> +	struct rk_priv_data *bsp_priv = priv;
> +
> +	rk_gmac_powerdown(bsp_priv);
> +}
> +
> +static void rk_gmac_suspend(struct platform_device *pdev, void *priv)
> +{
> +	struct rk_priv_data *bsp_priv = priv;
> +
> +	/* Keep the PHY up if we use Wake-on-Lan. */
> +	if (device_may_wakeup(&pdev->dev))
> +		return;
> +
> +	rk_gmac_powerdown(bsp_priv);

aka do
	bsp_priv->suspended = true;
here

> +}
> +
> +static void rk_gmac_resume(struct platform_device *pdev, void *priv)
> +{
> +	struct rk_priv_data *bsp_priv = priv;
> +
> +	/* The PHY was up for Wake-on-Lan. */
> +	if (!bsp_priv->powered_down)
> +		return;
> +
> +	rk_gmac_powerup(bsp_priv);

missing something like
	bsp_priv->suspended = false;

Right now it looks like your bsp_priv->powered_down will always be true 
after the first suspend with powerdown.

>  }
> 
>  static void rk_fix_speed(void *priv, unsigned int speed)
> @@ -591,6 +628,8 @@ static int rk_gmac_probe(struct platform_device *pdev)
> plat_dat->init = rk_gmac_init;
>  	plat_dat->exit = rk_gmac_exit;
>  	plat_dat->fix_mac_speed = rk_fix_speed;
> +	plat_dat->suspend = rk_gmac_suspend;
> +	plat_dat->resume = rk_gmac_resume;
> 
>  	plat_dat->bsp_priv = rk_gmac_setup(pdev, data);
>  	if (IS_ERR(plat_dat->bsp_priv))
> --
> 2.8.0.rc3.226.g39d4020