Re: [PATCH v5] can: Convert to runtime_pm

[SÃren Brinkmann]

On Tue, 2015-01-13 at 06:17PM +0100, Marc Kleine-Budde wrote:
> On 01/13/2015 06:08 PM, SÃren Brinkmann wrote:
> > On Tue, 2015-01-13 at 12:08PM +0100, Marc Kleine-Budde wrote:
> >> On 01/12/2015 07:45 PM, SÃren Brinkmann wrote:
> >>> On Mon, 2015-01-12 at 08:34PM +0530, Kedareswara rao Appana wrote:
> >>>> Instead of enabling/disabling clocks at several locations in the driver,
> >>>> Use the runtime_pm framework. This consolidates the actions for runtime PM
> >>>> In the appropriate callbacks and makes the driver more readable and mantainable.
> >>>>
> >>>> Signed-off-by: Soren Brinkmann <soren.brinkmann@xxxxxxxxxx>
> >>>> Signed-off-by: Kedareswara rao Appana <appanad@xxxxxxxxxx>
> >>>> ---
> >>>> Changes for v5:
> >>>>  - Updated with the review comments.
> >>>>    Updated the remove fuction to use runtime_pm.
> >>>> Chnages for v4:
> >>>>  - Updated with the review comments.
> >>>> Changes for v3:
> >>>>   - Converted the driver to use runtime_pm.
> >>>> Changes for v2:
> >>>>   - Removed the struct platform_device* from suspend/resume
> >>>>     as suggest by Lothar.
> >>>>
> >>>>  drivers/net/can/xilinx_can.c |  157 ++++++++++++++++++++++++++++-------------
> >>>>  1 files changed, 107 insertions(+), 50 deletions(-)
> >>> [..]
> >>>> +static int __maybe_unused xcan_runtime_resume(struct device *dev)
> >>>>  {
> >>>> -	struct platform_device *pdev = dev_get_drvdata(dev);
> >>>> -	struct net_device *ndev = platform_get_drvdata(pdev);
> >>>> +	struct net_device *ndev = dev_get_drvdata(dev);
> >>>>  	struct xcan_priv *priv = netdev_priv(ndev);
> >>>>  	int ret;
> >>>> +	u32 isr, status;
> >>>>  
> >>>>  	ret = clk_enable(priv->bus_clk);
> >>>>  	if (ret) {
> >>>> @@ -1014,15 +1030,28 @@ static int __maybe_unused xcan_resume(struct device *dev)
> >>>>  	ret = clk_enable(priv->can_clk);
> >>>>  	if (ret) {
> >>>>  		dev_err(dev, "Cannot enable clock.\n");
> >>>> -		clk_disable_unprepare(priv->bus_clk);
> >>>> +		clk_disable(priv->bus_clk);
> >>> [...]
> >>>> @@ -1173,12 +1219,23 @@ static int xcan_remove(struct platform_device *pdev)
> >>>>  {
> >>>>  	struct net_device *ndev = platform_get_drvdata(pdev);
> >>>>  	struct xcan_priv *priv = netdev_priv(ndev);
> >>>> +	int ret;
> >>>> +
> >>>> +	ret = pm_runtime_get_sync(&pdev->dev);
> >>>> +	if (ret < 0) {
> >>>> +		netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
> >>>> +				__func__, ret);
> >>>> +		return ret;
> >>>> +	}
> >>>>  
> >>>>  	if (set_reset_mode(ndev) < 0)
> >>>>  		netdev_err(ndev, "mode resetting failed!\n");
> >>>>  
> >>>>  	unregister_candev(ndev);
> >>>> +	pm_runtime_disable(&pdev->dev);
> >>>>  	netif_napi_del(&priv->napi);
> >>>> +	clk_disable_unprepare(priv->bus_clk);
> >>>> +	clk_disable_unprepare(priv->can_clk);
> >>>
> >>> Shouldn't pretty much all these occurrences of clk_disable/enable
> >>> disappear? This should all be handled by the runtime_pm framework now.
> >>
> >> We have:
> >> - clk_prepare_enable() in probe
> > 
> > This should become something like pm_runtime_get_sync(), shouldn't it?
> > 
> >> - clk_disable_unprepare() in remove
> > 
> > pm_runtime_put()
> > 
> >> - clk_enable() in runtime_resume
> >> - clk_disable() in runtime_suspend
> > 
> > These are the ones needed.
> > 
> > The above makes me suspect that the clocks are always on, regardless of
> 
> Define "on" :)
> The clocks are prepared after probe() exists, but not enabled. The first
> pm_runtime_get_sync() will enable the clocks.
> 
> > the runtime suspend state since they are enabled in probe and disabled
> > in remove, is that right? Ideally, the usage in probe and remove should
> > be migrated to runtime_pm and clocks should really only be running when
> > needed and not throughout the whole lifetime of the driver.
> 
> The clocks are not en/disabled via pm_runtime, because
> pm_runtime_get_sync() is called from atomic contect. We can have another
> look into the driver and try to change this.

Wasn't that why the call to pm_runtime_irq_safe() was added?
Also, clk_enable/disable should be okay to be run from atomic context.
And if the clock are already prepared after the exit of probe that
should be enough. Then remove() should just have to do the unprepare.
But I don't see why runtime_pm shouldn't be able to do the
enable/disable.

	SÃren
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/