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

On Fri, Jun 10, 2016 at 6:57 PM, Heiko Stuebner [off-list ref] wrote:

Am Freitag, 10. Juni 2016, 18:00:38 schrieb Vincent Palatin:

quoted

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 <redacted>
---
 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.

quoted

+}
+
+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

quoted

+}
+
+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.

Yes I screw up badly, that's a good reason to use a more sensible name
for the variable.

quoted

 }

 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