RE: [PATCH v5] can: Convert to runtime_pm
Hi Marc, > -Original Message- > From: Marc Kleine-Budde [mailto:m...@pengutronix.de] > Sent: Wednesday, January 28, 2015 8:16 PM > To: Appana Durga Kedareswara Rao; w...@grandegger.com; Michal Simek; > Soren Brinkmann; grant.lik...@linaro.org; robh...@kernel.org > Cc: linux-...@vger.kernel.org; net...@vger.kernel.org; linux-arm- > ker...@lists.infradead.org; linux-ker...@vger.kernel.org; > devicetree@vger.kernel.org; Appana Durga Kedareswara Rao > Subject: Re: [PATCH v5] can: Convert to runtime_pm > > On 01/12/2015 04:04 PM, 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 > > Signed-off-by: Kedareswara rao Appana > > --- > > 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. > > Any plans to submit a v6? I was on vacation till yesterday just came to office today only. Will look into it and will send v6 at the earliest. Regards, Kedar. > > Marc > -- > Pengutronix e.K. | Marc Kleine-Budde | > Industrial Linux Solutions| Phone: +49-231-2826-924 | > Vertretung West/Dortmund | Fax: +49-5121-206917- | > Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
Re: [PATCH v5] can: Convert to runtime_pm
On 01/12/2015 04:04 PM, 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 > Signed-off-by: Kedareswara rao Appana > --- > 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. Any plans to submit a v6? Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions| Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917- | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | signature.asc Description: OpenPGP digital signature
Re: [PATCH v5] can: Convert to runtime_pm
On Tue, 2015-01-13 at 07:03PM +0100, Marc Kleine-Budde wrote:
> On 01/13/2015 06:49 PM, Sören Brinkmann wrote:
> > On Tue, 2015-01-13 at 06:44PM +0100, Marc Kleine-Budde wrote:
> >> On 01/13/2015 06:24 PM, Sören Brinkmann wrote:
> >>> 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
> Signed-off-by: Kedareswara rao Appana
> ---
> 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?
> >>
> >> Good question. That should be investigated.
> >>
> >>> 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
> >
Re: [PATCH v5] can: Convert to runtime_pm
On 01/13/2015 06:49 PM, Sören Brinkmann wrote:
> On Tue, 2015-01-13 at 06:44PM +0100, Marc Kleine-Budde wrote:
>> On 01/13/2015 06:24 PM, Sören Brinkmann wrote:
>>> 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
Signed-off-by: Kedareswara rao Appana
---
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?
>>
>> Good question. That should be investigated.
>>
>>> 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.
>>
>> runtime_pm does call the clk_{enable,disable} function. But you mean
>
Re: [PATCH v5] can: Convert to runtime_pm
On Tue, 2015-01-13 at 06:44PM +0100, Marc Kleine-Budde wrote:
> On 01/13/2015 06:24 PM, Sören Brinkmann wrote:
> > 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
> >> Signed-off-by: Kedareswara rao Appana
> >> ---
> >> 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?
>
> Good question. That should be investigated.
>
> > 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.
>
> runtime_pm does call the clk_{enable,disable} function. But you mean
> clk_prepare() + pm_runtime_get_sync() should be use
Re: [PATCH v5] can: Convert to runtime_pm
On 01/13/2015 06:24 PM, Sören Brinkmann wrote:
> 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
>> Signed-off-by: Kedareswara rao Appana
>> ---
>> 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?
Good question. That should be investigated.
> 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.
runtime_pm does call the clk_{enable,disable} function. But you mean
clk_prepare() + pm_runtime_get_sync() should be used in probe() instead
of calling clk_prepare_enable(). Good idea! I think the
"pm_runtime_set_active(&pdev->dev);" has to be removed from the patch.
Coming back whether blocking calls are allowed or not.
If you make a call to pm_runtime_irq_safe(
Re: [PATCH v5] can: Convert to runtime_pm
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
> Signed-off-by: Kedareswara rao Appana
> ---
> 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 devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] can: Convert to runtime_pm
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
Signed-off-by: Kedareswara rao Appana
---
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.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v5] can: Convert to runtime_pm
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
> >> Signed-off-by: Kedareswara rao Appana
> >> ---
> >> 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
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.
Sören
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] can: Convert to runtime_pm
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
>> Signed-off-by: Kedareswara rao Appana
>> ---
>> 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
- clk_disable_unprepare() in remove
- clk_enable() in runtime_resume
- clk_disable() in runtime_suspend
Which is, as far as I understand the right way to do it. Maybe
Kedareswara can post the clock debug output again with this patch
iteration. Have I missed something?
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v5] can: Convert to runtime_pm
On 01/12/2015 04:04 PM, 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
> Signed-off-by: Kedareswara rao Appana
FTBFS:
> drivers/net/can/xilinx_can.c:1064:9: error: undefined identifier
> 'SET_PM_RUNTIME_PM_OPS'
> CC [M] drivers/net/can/xilinx_can.o
> drivers/net/can/xilinx_can.c:1064:2: error: implicit declaration of function
> ‘SET_PM_RUNTIME_PM_OPS’ [-Werror=implicit-function-declaration]
> SET_PM_RUNTIME_PM_OPS(xcan_runtime_suspend, xcan_runtime_resume, NULL)
> ^
> drivers/net/can/xilinx_can.c:1065:1: error: initializer element is not
> constant
> };
> ^
> drivers/net/can/xilinx_can.c:1065:1: error: (near initialization for
> ‘xcan_dev_pm_ops.prepare’)
Have a look at commit 40bd62c6194bdee1bc6652b3b0aa28e34883f603:
More comments inline. Looks quite good now.
> PM: Remove the SET_PM_RUNTIME_PM_OPS() macro
>
> There're now no users left of the SET_PM_RUNTIME_PM_OPS() macro, since
> all have converted to use the SET_RUNTIME_PM_OPS() macro instead, so
> let's remove it.
> ---
> 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(-)
>
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 6c67643..67aef00 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -32,6 +32,7 @@
> #include
> #include
> #include
> +#include
>
> #define DRIVER_NAME "xilinx_can"
>
> @@ -138,7 +139,7 @@ struct xcan_priv {
> u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
> void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
> u32 val);
> - struct net_device *dev;
> + struct device *dev;
> void __iomem *reg_base;
> unsigned long irq_flags;
> struct clk *bus_clk;
> @@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev)
> struct xcan_priv *priv = netdev_priv(ndev);
> int ret;
>
> + ret = pm_runtime_get_sync(priv->dev);
> + if (ret < 0) {
> + netdev_err(ndev, "%s: pm_runtime_get failed(%d)\n",
> + __func__, ret);
> + return ret;
> + }
> +
> ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
> ndev->name, ndev);
> if (ret < 0) {
> @@ -849,29 +857,17 @@ static int xcan_open(struct net_device *ndev)
> goto err;
> }
>
> - ret = clk_prepare_enable(priv->can_clk);
> - if (ret) {
> - netdev_err(ndev, "unable to enable device clock\n");
> - goto err_irq;
> - }
> -
> - ret = clk_prepare_enable(priv->bus_clk);
> - if (ret) {
> - netdev_err(ndev, "unable to enable bus clock\n");
> - goto err_can_clk;
> - }
> -
> /* Set chip into reset mode */
> ret = set_reset_mode(ndev);
> if (ret < 0) {
> netdev_err(ndev, "mode resetting failed!\n");
> - goto err_bus_clk;
> + goto err_irq;
> }
>
> /* Common open */
> ret = open_candev(ndev);
> if (ret)
> - goto err_bus_clk;
> + goto err_irq;
>
> ret = xcan_chip_start(ndev);
> if (ret < 0) {
> @@ -887,13 +883,11 @@ static int xcan_open(struct net_device *ndev)
>
> err_candev:
> close_candev(ndev);
> -err_bus_clk:
> - clk_disable_unprepare(priv->bus_clk);
> -err_can_clk:
> - clk_disable_unprepare(priv->can_clk);
> err_irq:
> free_irq(ndev->irq, ndev);
> err:
> + pm_runtime_put(priv->dev);
> +
> return ret;
> }
>
> @@ -910,12 +904,11 @@ static int xcan_close(struct net_device *ndev)
> netif_stop_queue(ndev);
> napi_disable(&priv->napi);
> xcan_chip_stop(ndev);
> - clk_disable_unprepare(priv->bus_clk);
> - clk_disable_unprepare(priv->can_clk);
> free_irq(ndev->irq, ndev);
> close_candev(ndev);
>
> can_led_event(ndev, CAN_LED_EVENT_STOP);
> + pm_runtime_put(priv->dev);
>
> return 0;
> }
> @@ -934,27 +927,20 @@ static int xcan_get_berr_counter(const struct
> net_device *ndev,
> struct xcan_priv *priv = netdev_priv(ndev);
> int ret;
>
> - ret = clk_prepare_enable(priv->can_clk);
> - if (ret)
> - goto err;
> -
> - ret = clk_prepare_enable(priv->bus_clk);
> - if (ret)
> -
Re: [PATCH v5] can: Convert to runtime_pm
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
> Signed-off-by: Kedareswara rao Appana
> ---
> 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.
Sören
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
