LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Sören Brinkmann" <soren.brinkmann@xilinx.com>
To: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: Kedareswara rao Appana <appana.durga.rao@xilinx.com>,
	<wg@grandegger.com>, <michal.simek@xilinx.com>,
	<grant.likely@linaro.org>, <robh+dt@kernel.org>,
	<linux-can@vger.kernel.org>, <netdev@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>,
	Kedareswara rao Appana <appanad@xilinx.com>
Subject: Re: [PATCH v5] can: Convert to runtime_pm
Date: Tue, 13 Jan 2015 09:24:36 -0800	[thread overview]
Message-ID: <1ef3e0f060f54c888061514bd2926762@BY2FFO11FD033.protection.gbl> (raw)
In-Reply-To: <54B55326.1060606@pengutronix.de>

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@xilinx.com>
> >>>> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> >>>> ---
> >>>> 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

  reply	other threads:[~2015-01-13 17:24 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-01-12 15:04 Kedareswara rao Appana
2015-01-12 18:45 ` Sören Brinkmann
2015-01-13 11:08   ` Marc Kleine-Budde
2015-01-13 17:08     ` Sören Brinkmann
2015-01-13 17:17       ` Marc Kleine-Budde
2015-01-13 17:24         ` Sören Brinkmann [this message]
2015-01-13 17:44           ` Marc Kleine-Budde
2015-01-13 17:49             ` Sören Brinkmann
2015-01-13 18:03               ` Marc Kleine-Budde
2015-01-13 18:43                 ` Sören Brinkmann
2015-01-12 19:52 ` Marc Kleine-Budde
2015-01-28 14:46 ` Marc Kleine-Budde
2015-01-28 15:19   ` Appana Durga Kedareswara Rao

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1ef3e0f060f54c888061514bd2926762@BY2FFO11FD033.protection.gbl \
    --to=soren.brinkmann@xilinx.com \
    --cc=appana.durga.rao@xilinx.com \
    --cc=appanad@xilinx.com \
    --cc=devicetree@vger.kernel.org \
    --cc=grant.likely@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-can@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=michal.simek@xilinx.com \
    --cc=mkl@pengutronix.de \
    --cc=netdev@vger.kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=wg@grandegger.com \
    --subject='Re: [PATCH v5] can: Convert to runtime_pm' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).