Re: net: stmmac: dwmac-rk: fixes for Wake-on-Lan on RK3288

Hi Giuseppe,

On Thu, Jun 16, 2016 at 6:37 AM, Giuseppe CAVALLARO
[off-list ref] wrote:

Hi Vincent


On 6/15/2016 7:04 PM, Vincent Palatin wrote:

quoted

On Sun, Jun 12, 2016 at 11:46 PM, Giuseppe CAVALLARO
[off-list ref] wrote:

quoted

On 6/11/2016 3:00 AM, Vincent Palatin wrote:

quoted

In order to support Wake-On-Lan when using the RK3288 integrated MAC
(with an external RGMII PHY), we need to avoid shutting down the regulator
of the external PHY when the MAC is suspended as it's currently done in
the MAC
platform code.
As a first step, create independant callbacks for suspend/resume rather
than
re-using exit/init callbacks. So the dwmac platform driver can behave
differently
on suspend where it might skip shutting the PHY and at module unloading.
Then update the dwmac-rk driver to switch off the PHY regulator only if we
are
not planning to wake up from the LAN.
Finally add the PMT interrupt to the MAC device tree configuration, so we
can
wake up the core from it when the PHY has received the magic packet.

IMO these could be sent for net-next and also other glue logic
files should be reworked in order to use the new API for coherence.

Given they will have the same set of functions for exit/init and
suspend/resume, you mean duplicating the callbacks like this :

--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c

@@ -359,6 +359,8 @@ static int sti_dwmac_probe(struct platform_device *pdev)
        plat_dat->bsp_priv = dwmac;
        plat_dat->init = sti_dwmac_init;
        plat_dat->exit = sti_dwmac_exit;
+       plat_dat->suspend = sti_dwmac_exit;
+       plat_dat->resume = sti_dwmac_init;
        plat_dat->fix_mac_speed = data->fix_retime_src;

        ret = sti_dwmac_init(pdev, plat_dat->bsp_priv);

Is this anyhow useful ?

I think this is mandatory otherwise you are not guaranteeing the PM
stuff working on the rest of the glue-logics (not only sti); because
init/exit calls won't be called anymore.

As mentioned in the PATCH 1/3 description: "If the driver does not
provide the suspend or resume callback, we fall
back to the old behavior trying to use exit or init. [...]"
ie.

--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c

@@ -411,7 +411,9 @@ static int stmmac_pltfr_suspend(struct device *dev)
        struct platform_device *pdev = to_platform_device(dev);

        ret = stmmac_suspend(dev);
-       if (priv->plat->exit)
+       if (priv->plat->suspend)
+               priv->plat->suspend(pdev, priv->plat->bsp_priv);
+       else if (priv->plat->exit)
                priv->plat->exit(pdev, priv->plat->bsp_priv);

        return ret;

@@ -430,7 +432,9 @@ static int stmmac_pltfr_resume(struct device *dev)
        struct stmmac_priv *priv = netdev_priv(ndev);
        struct platform_device *pdev = to_platform_device(dev);

-       if (priv->plat->init)
+       if (priv->plat->resume)
+               priv->plat->resume(pdev, priv->plat->bsp_priv);
+       else if (priv->plat->init)
                priv->plat->init(pdev, priv->plat->bsp_priv);

        return stmmac_resume(dev);

So I was under the impression that everything should continue working
as before for drivers only providing init/exit,
by falling back on calling ->exit() if there is no suspend() callback
initialized for the PM calls.
You think that won't work ?

So I kindly ask you to
propagate the fix and send the V3.  The implementation above is ok for
me.

peppe