Re: [RFC PATCH 9/9] ethernet: sun8i-emac: add pm_runtime support

[LABBE Corentin]

On Mon, Sep 12, 2016 at 10:44:51PM +0200, Maxime Ripard wrote:
> Hi,
> 
> On Fri, Sep 09, 2016 at 02:45:17PM +0200, Corentin Labbe wrote:
> > This patch add pm_runtime support to sun8i-emac.
> > For the moment, only basic support is added, (the device is marked as
> > used when net/open)
> > 
> > Signed-off-by: Corentin Labbe <clabbe.montjoie@xxxxxxxxx>
> > ---
> >  drivers/net/ethernet/allwinner/sun8i-emac.c | 62 ++++++++++++++++++++++++++++-
> >  1 file changed, 60 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/allwinner/sun8i-emac.c b/drivers/net/ethernet/allwinner/sun8i-emac.c
> > index 1c4bc80..cce886e 100644
> > --- a/drivers/net/ethernet/allwinner/sun8i-emac.c
> > +++ b/drivers/net/ethernet/allwinner/sun8i-emac.c
> > @@ -9,7 +9,6 @@
> >   * - MAC filtering
> >   * - Jumbo frame
> >   * - features rx-all (NETIF_F_RXALL_BIT)
> > - * - PM runtime
> >   */
> >  #include <linux/bitops.h>
> >  #include <linux/clk.h>
> > @@ -27,6 +26,7 @@
> >  #include <linux/pinctrl/consumer.h>
> >  #include <linux/pinctrl/pinctrl.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/reset.h>
> >  #include <linux/scatterlist.h>
> >  #include <linux/skbuff.h>
> > @@ -1301,11 +1301,18 @@ static int sun8i_emac_open(struct net_device *ndev)
> >  	int err;
> >  	u32 v;
> >  
> > +	err = pm_runtime_get_sync(priv->dev);
> > +	if (err) {
> > +		pm_runtime_put_noidle(priv->dev);
> > +		dev_err(priv->dev, "pm_runtime error: %d\n", err);
> > +		return err;
> > +	}
> > +
> >  	err = request_irq(priv->irq, sun8i_emac_dma_interrupt, 0,
> >  			  dev_name(priv->dev), ndev);
> >  	if (err) {
> >  		dev_err(priv->dev, "Cannot request IRQ: %d\n", err);
> > -		return err;
> > +		goto err_runtime;
> >  	}
> >  
> >  	/* Set interface mode (and configure internal PHY on H3) */
> > @@ -1395,6 +1402,8 @@ err_syscon:
> >  	sun8i_emac_unset_syscon(ndev);
> >  err_irq:
> >  	free_irq(priv->irq, ndev);
> > +err_runtime:
> > +	pm_runtime_put(priv->dev);
> >  	return err;
> >  }
> >  
> > @@ -1483,6 +1492,8 @@ static int sun8i_emac_stop(struct net_device *ndev)
> >  	dma_free_coherent(priv->dev, priv->nbdesc_tx * sizeof(struct dma_desc),
> >  			  priv->dd_tx, priv->dd_tx_phy);
> >  
> > +	pm_runtime_put(priv->dev);
> > +
> >  	return 0;
> >  }
> >  
> > @@ -2210,6 +2221,8 @@ static int sun8i_emac_probe(struct platform_device *pdev)
> >  		goto probe_err;
> >  	}
> >  
> > +	pm_runtime_enable(priv->dev);
> > +
> >  	return 0;
> >  
> >  probe_err:
> > @@ -2221,6 +2234,8 @@ static int sun8i_emac_remove(struct platform_device *pdev)
> >  {
> >  	struct net_device *ndev = platform_get_drvdata(pdev);
> >  
> > +	pm_runtime_disable(&pdev->dev);
> > +
> >  	unregister_netdev(ndev);
> >  	platform_set_drvdata(pdev, NULL);
> >  	free_netdev(ndev);
> > @@ -2228,6 +2243,47 @@ static int sun8i_emac_remove(struct platform_device *pdev)
> >  	return 0;
> >  }
> >  
> > +static int __maybe_unused sun8i_emac_suspend(struct platform_device *pdev, pm_message_t state)
> > +{
> > +	struct net_device *ndev = platform_get_drvdata(pdev);
> > +	struct sun8i_emac_priv *priv = netdev_priv(ndev);
> > +
> > +	napi_disable(&priv->napi);
> > +
> > +	if (netif_running(ndev))
> > +		netif_device_detach(ndev);
> > +
> > +	sun8i_emac_stop_tx(ndev);
> > +	sun8i_emac_stop_rx(ndev);
> > +
> > +	sun8i_emac_rx_clean(ndev);
> > +	sun8i_emac_tx_clean(ndev);
> > +
> > +	phy_stop(ndev->phydev);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused sun8i_emac_resume(struct platform_device *pdev)
> > +{
> > +	struct net_device *ndev = platform_get_drvdata(pdev);
> > +	struct sun8i_emac_priv *priv = netdev_priv(ndev);
> > +
> > +	phy_start(ndev->phydev);
> > +
> > +	sun8i_emac_start_tx(ndev);
> > +	sun8i_emac_start_rx(ndev);
> > +
> > +	if (netif_running(ndev))
> > +		netif_device_attach(ndev);
> > +
> > +	netif_start_queue(ndev);
> > +
> > +	napi_enable(&priv->napi);
> > +
> > +	return 0;
> > +}
> 
> The main idea behind the runtime PM hooks is that they bring the
> device to a working state and shuts it down when it's not needed
> anymore.
> 

I expect that the first part (all pm_runtime_xxx) of the patch bring that.
When the interface is not opened:
cat /sys/devices/platform/soc/1c30000.ethernet/power/runtime_status 
suspended

> However, they shouldn't be called when the device is still in used, so
> all the mangling with NAPI, the phy and so on is irrelevant here, but
> the clocks, resets, for example, are.
> 

I do the same as other ethernet driver for suspend/resume.

> >  static const struct of_device_id sun8i_emac_of_match_table[] = {
> >  	{ .compatible = "allwinner,sun8i-a83t-emac",
> >  	  .data = &emac_variant_a83t },
> > @@ -2246,6 +2302,8 @@ static struct platform_driver sun8i_emac_driver = {
> >  		.name           = "sun8i-emac",
> >  		.of_match_table	= sun8i_emac_of_match_table,
> >  	},
> > +	.suspend	= sun8i_emac_suspend,
> > +	.resume		= sun8i_emac_resume,
> 
> These are not the runtime PM hooks. How did you test that?
> 

Anyway I didnt test suspend/resume so I will remove it until I successfully found how to hibernate my board.

Thanks

Regards

Corentin Labbe