[PATCH v10 0/5] amba/dmaengine: pl330: add Power Management support

[PATCH v10 0/5] amba/dmaengine: pl330: add Power Management support

[Krzysztof Kozlowski]

Hi,


Changes since v9:
=================
1. Add patch 1/5: Move dev->power.irq_safe out of CONFIG_PM_RUNTIME.
   If CONFIG_PM_RUNTIME is not set, amba bus driver must still know
   whether child driver set irq_safe or not. Suggested by Ulf Hansson.

Changes since v8:
=================
1. Remove the pm_runtime_is_irq_safe() wrapper (patch 1).
2. Patch 2/4 (amba): Store current irq_safe during runtime suspend.
   Resume will mirror suspend's work. Add a macro for
   safe access of irq_safe if CONFIG_PM_RUNTIME is unset.
   Dropped reviewed-by Ulf Hansson (major changes in patch).

Changes since v7:
=================
1. Add reviewed-by Ulf Hansson (patches 3, 4 and 5).
2. Patch 2/5: Fix missing return in amba_pclk_prepare() (suggested by
   Ulf Hansson).
3. Rebased on next-20141020.

Changes since v6:
=================
1. Add patch 5 removing the amba_pclk_*able macros.
2. Patch 2/5: Remove IS_ERR, use static inline functions instead
   of macros.
3. Patch 4/5: Force runtime suspend/resume in system sleep
   callbacks. Put with autosuspend at end of probe instead of no_idle.
   Suggested by Ulf Hansson.

Changes since v5:
=================
1. Patch 1/4: Add Ulf Hansson's reviewed-by.
2. Patch 4/4: Use PM runtime autosuspend (suggested by Ulf Hansson).
3. Rebase on next-20140922. 

Changes since v4:
1. Patch 3/4: Explicitly initialize amba_device.irq_safe field after
   probing driver (suggested by Russell King).

Changes since v3:
1. Patch 1/4: Document new API in Documentation/power/runtime_pm.txt
   (pointed by Alan Stern).

Changes since v2:
1. Add patch 1 (PM / Runtime: Add getter for querying the IRQ safe option)
2. Add patch 2 (amba: Add helper macros for (un)preparing AMBA clock)
3. Patch 3/4: Rewrite the idea. If IRQ safe runtime PM is set then
   do not unprepare/prepare the clocks. Suggested by Russell King.
4. Patch 4/4: During system sleep unprepare the clock.

Changes since v1:
1. Add patch 1 (amba: Allow AMBA drivers to use their own runtime PM).
2. Patch 2/2: Apply Michal Simek's suggestions.
3. Patch 2/2: Fix atomic context safeness in pl330_issue_pending().


Description:
============
This patchset adds runtime and system PM to the pl330 driver.

The runtime PM of pl330 driver requires interrupt safe suspend/resume
callbacks which is in conflict with current amba bus driver.
The latter also unprepares and prepares the AMBA bus clock which
is not safe for atomic context.

The patchset solves this in patch 3/5 by handling clocks in different
way if device driver set interrupt safe runtime PM.

Comments are welcome.


Tested on board with pl330 DMA driver:
 - Trats2 (Exynos4212)


Best regards,
Krzysztof Kozlowski


Krzysztof Kozlowski (5):
  PM / Runtime: Allow accessing irq_safe if no PM_RUNTIME
  amba: Add helpers for (un)preparing AMBA clock
  amba: Don't unprepare the clocks if device driver wants IRQ safe
    runtime PM
  dmaengine: pl330: add Power Management support
  amba: Remove unused amba_pclk_enable/disable macros

 drivers/amba/bus.c         | 17 ++++++--
 drivers/dma/pl330.c        | 96 ++++++++++++++++++++++++++++++++++++++++++++--
 include/linux/amba/bus.h   | 13 +++++--
 include/linux/pm.h         |  2 +-
 include/linux/pm_runtime.h |  4 +-
 5 files changed, 118 insertions(+), 14 deletions(-)

-- 
1.9.1

[PATCH v10 1/5] PM / Runtime: Allow accessing irq_safe if no PM_RUNTIME

[Krzysztof Kozlowski]

Some drivers (e.g. bus drivers) may want to check if power.irq_safe was
called by child driver, regardless of CONFIG_PM_RUNTIME.

An example scenario is amba/bus.c and dma/pl330.c drivers. The runtime
suspend/resume callbacks in amba bus driver act differently if irq_safe
was set by child driver (in irq_safe mode bus clock is only disabled).

The pl330 driver sets irq_safe and assumes that amba bus driver will
only disable the clock in runtime PM. So in system sleep suspend
callback the pl330 driver unprepares the clock after calling
pm_runtime_force_suspend().

However inconsistency would appear if CONFIG_PM_RUNTIME is not set and
child drivers do not want the irq_safe runtime PM. In such case amba bus
driver still has to know whether child driver wanted irq_safe - by
looking at dev->power.irq_safe field.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 include/linux/pm.h         | 2 +-
 include/linux/pm_runtime.h | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/linux/pm.h b/include/linux/pm.h
index 383fd68aaee1..b05fa954f50d 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -566,6 +566,7 @@ struct dev_pm_info {
 	bool			ignore_children:1;
 	bool			early_init:1;	/* Owned by the PM core */
 	bool			direct_complete:1;	/* Owned by the PM core */
+	unsigned int		irq_safe:1;	/* PM runtime */
 	spinlock_t		lock;
 #ifdef CONFIG_PM_SLEEP
 	struct list_head	entry;
@@ -590,7 +591,6 @@ struct dev_pm_info {
 	unsigned int		run_wake:1;
 	unsigned int		runtime_auto:1;
 	unsigned int		no_callbacks:1;
-	unsigned int		irq_safe:1;
 	unsigned int		use_autosuspend:1;
 	unsigned int		timer_autosuspends:1;
 	unsigned int		memalloc_noio:1;
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 367f49b9a1c9..d94a65662a60 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -166,7 +166,10 @@ static inline bool pm_runtime_suspended_if_enabled(struct device *dev) { return
 static inline bool pm_runtime_enabled(struct device *dev) { return false; }
 
 static inline void pm_runtime_no_callbacks(struct device *dev) {}
-static inline void pm_runtime_irq_safe(struct device *dev) {}
+static inline void pm_runtime_irq_safe(struct device *dev)
+{
+	dev->power.irq_safe = 1;
+}
 
 static inline bool pm_runtime_callbacks_present(struct device *dev) { return false; }
 static inline void pm_runtime_mark_last_busy(struct device *dev) {}
-- 
1.9.1

Re: [PATCH v10 1/5] PM / Runtime: Allow accessing irq_safe if no PM_RUNTIME

[Ulf Hansson]

On 6 November 2014 09:36, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> Some drivers (e.g. bus drivers) may want to check if power.irq_safe was
> called by child driver, regardless of CONFIG_PM_RUNTIME.
>
> An example scenario is amba/bus.c and dma/pl330.c drivers. The runtime
> suspend/resume callbacks in amba bus driver act differently if irq_safe
> was set by child driver (in irq_safe mode bus clock is only disabled).
>
> The pl330 driver sets irq_safe and assumes that amba bus driver will
> only disable the clock in runtime PM. So in system sleep suspend
> callback the pl330 driver unprepares the clock after calling
> pm_runtime_force_suspend().
>
> However inconsistency would appear if CONFIG_PM_RUNTIME is not set and
> child drivers do not want the irq_safe runtime PM. In such case amba bus
> driver still has to know whether child driver wanted irq_safe - by
> looking at dev->power.irq_safe field.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

FWIW: Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  include/linux/pm.h         | 2 +-
>  include/linux/pm_runtime.h | 5 ++++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 383fd68aaee1..b05fa954f50d 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -566,6 +566,7 @@ struct dev_pm_info {
>         bool                    ignore_children:1;
>         bool                    early_init:1;   /* Owned by the PM core */
>         bool                    direct_complete:1;      /* Owned by the PM core */
> +       unsigned int            irq_safe:1;     /* PM runtime */
>         spinlock_t              lock;
>  #ifdef CONFIG_PM_SLEEP
>         struct list_head        entry;
> @@ -590,7 +591,6 @@ struct dev_pm_info {
>         unsigned int            run_wake:1;
>         unsigned int            runtime_auto:1;
>         unsigned int            no_callbacks:1;
> -       unsigned int            irq_safe:1;
>         unsigned int            use_autosuspend:1;
>         unsigned int            timer_autosuspends:1;
>         unsigned int            memalloc_noio:1;
> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index 367f49b9a1c9..d94a65662a60 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -166,7 +166,10 @@ static inline bool pm_runtime_suspended_if_enabled(struct device *dev) { return
>  static inline bool pm_runtime_enabled(struct device *dev) { return false; }
>
>  static inline void pm_runtime_no_callbacks(struct device *dev) {}
> -static inline void pm_runtime_irq_safe(struct device *dev) {}
> +static inline void pm_runtime_irq_safe(struct device *dev)
> +{
> +       dev->power.irq_safe = 1;
> +}
>
>  static inline bool pm_runtime_callbacks_present(struct device *dev) { return false; }
>  static inline void pm_runtime_mark_last_busy(struct device *dev) {}
> --
> 1.9.1
>

Re: [PATCH v10 1/5] PM / Runtime: Allow accessing irq_safe if no PM_RUNTIME

[Rafael J. Wysocki]

On Thursday, November 06, 2014 09:36:46 AM Krzysztof Kozlowski wrote:
> Some drivers (e.g. bus drivers) may want to check if power.irq_safe was
> called by child driver, regardless of CONFIG_PM_RUNTIME.
> 
> An example scenario is amba/bus.c and dma/pl330.c drivers. The runtime
> suspend/resume callbacks in amba bus driver act differently if irq_safe
> was set by child driver (in irq_safe mode bus clock is only disabled).
> 
> The pl330 driver sets irq_safe and assumes that amba bus driver will
> only disable the clock in runtime PM. So in system sleep suspend
> callback the pl330 driver unprepares the clock after calling
> pm_runtime_force_suspend().
> 
> However inconsistency would appear if CONFIG_PM_RUNTIME is not set and
> child drivers do not want the irq_safe runtime PM. In such case amba bus
> driver still has to know whether child driver wanted irq_safe - by
> looking at dev->power.irq_safe field.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
>  include/linux/pm.h         | 2 +-
>  include/linux/pm_runtime.h | 5 ++++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 383fd68aaee1..b05fa954f50d 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -566,6 +566,7 @@ struct dev_pm_info {
>  	bool			ignore_children:1;
>  	bool			early_init:1;	/* Owned by the PM core */
>  	bool			direct_complete:1;	/* Owned by the PM core */
> +	unsigned int		irq_safe:1;	/* PM runtime */
>  	spinlock_t		lock;
>  #ifdef CONFIG_PM_SLEEP
>  	struct list_head	entry;
> @@ -590,7 +591,6 @@ struct dev_pm_info {
>  	unsigned int		run_wake:1;
>  	unsigned int		runtime_auto:1;
>  	unsigned int		no_callbacks:1;
> -	unsigned int		irq_safe:1;
>  	unsigned int		use_autosuspend:1;
>  	unsigned int		timer_autosuspends:1;
>  	unsigned int		memalloc_noio:1;

Well, that is a good reason to introduce a wrapper around power.irq_safe in my
view.

And define the wrapper so that it always returns false for CONFIG_PM_RUNTIME
unset.

This way not only you wouldn't need to move the flag from under the #ifdef,
but also you would make the compiler skip the relevant pieces of code
entiretly for CONFIG_PM_RUNTIME unset.

> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index 367f49b9a1c9..d94a65662a60 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -166,7 +166,10 @@ static inline bool pm_runtime_suspended_if_enabled(struct device *dev) { return
>  static inline bool pm_runtime_enabled(struct device *dev) { return false; }
>  
>  static inline void pm_runtime_no_callbacks(struct device *dev) {}
> -static inline void pm_runtime_irq_safe(struct device *dev) {}
> +static inline void pm_runtime_irq_safe(struct device *dev)
> +{
> +	dev->power.irq_safe = 1;
> +}
>  
>  static inline bool pm_runtime_callbacks_present(struct device *dev) { return false; }
>  static inline void pm_runtime_mark_last_busy(struct device *dev) {}
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v10 1/5] PM / Runtime: Allow accessing irq_safe if no PM_RUNTIME

[Krzysztof Kozlowski]

On czw, 2014-11-06 at 23:51 +0100, Rafael J. Wysocki wrote:
> On Thursday, November 06, 2014 09:36:46 AM Krzysztof Kozlowski wrote:
> > Some drivers (e.g. bus drivers) may want to check if power.irq_safe was
> > called by child driver, regardless of CONFIG_PM_RUNTIME.
> > 
> > An example scenario is amba/bus.c and dma/pl330.c drivers. The runtime
> > suspend/resume callbacks in amba bus driver act differently if irq_safe
> > was set by child driver (in irq_safe mode bus clock is only disabled).
> > 
> > The pl330 driver sets irq_safe and assumes that amba bus driver will
> > only disable the clock in runtime PM. So in system sleep suspend
> > callback the pl330 driver unprepares the clock after calling
> > pm_runtime_force_suspend().
> > 
> > However inconsistency would appear if CONFIG_PM_RUNTIME is not set and
> > child drivers do not want the irq_safe runtime PM. In such case amba bus
> > driver still has to know whether child driver wanted irq_safe - by
> > looking at dev->power.irq_safe field.
> > 
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > ---
> >  include/linux/pm.h         | 2 +-
> >  include/linux/pm_runtime.h | 5 ++++-
> >  2 files changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/pm.h b/include/linux/pm.h
> > index 383fd68aaee1..b05fa954f50d 100644
> > --- a/include/linux/pm.h
> > +++ b/include/linux/pm.h
> > @@ -566,6 +566,7 @@ struct dev_pm_info {
> >  	bool			ignore_children:1;
> >  	bool			early_init:1;	/* Owned by the PM core */
> >  	bool			direct_complete:1;	/* Owned by the PM core */
> > +	unsigned int		irq_safe:1;	/* PM runtime */
> >  	spinlock_t		lock;
> >  #ifdef CONFIG_PM_SLEEP
> >  	struct list_head	entry;
> > @@ -590,7 +591,6 @@ struct dev_pm_info {
> >  	unsigned int		run_wake:1;
> >  	unsigned int		runtime_auto:1;
> >  	unsigned int		no_callbacks:1;
> > -	unsigned int		irq_safe:1;
> >  	unsigned int		use_autosuspend:1;
> >  	unsigned int		timer_autosuspends:1;
> >  	unsigned int		memalloc_noio:1;
> 
> Well, that is a good reason to introduce a wrapper around power.irq_safe in my
> view.
> 
> And define the wrapper so that it always returns false for CONFIG_PM_RUNTIME
> unset.
> 
> This way not only you wouldn't need to move the flag from under the #ifdef,
> but also you would make the compiler skip the relevant pieces of code
> entiretly for CONFIG_PM_RUNTIME unset.

Few days ago I would be happy with your opinion :), but know I think
this is better solution than wrapper. Consider case:
1. PM_RUNTIME unset.
2. System suspends.
3. The pl330 in its suspend callback calls force_runtime_suspend which
leads us to amba/bus.
4. The amba/bus.c in runtime suspend checks for irq_safe (it is FALSE),
so it disables and unprepares the clock.
5. The pl330 in probe requested irq_safe so it assumes amba/bus will
only disable the clock. So the pl330 unprepares the clock. Again.

Best regards,
Krzysztof

Re: [PATCH v10 1/5] PM / Runtime: Allow accessing irq_safe if no PM_RUNTIME

[Alan Stern]

On Fri, 7 Nov 2014, Krzysztof Kozlowski wrote:

> > Well, that is a good reason to introduce a wrapper around power.irq_safe in my
> > view.
> > 
> > And define the wrapper so that it always returns false for CONFIG_PM_RUNTIME
> > unset.
> > 
> > This way not only you wouldn't need to move the flag from under the #ifdef,
> > but also you would make the compiler skip the relevant pieces of code
> > entiretly for CONFIG_PM_RUNTIME unset.
> 
> Few days ago I would be happy with your opinion :), but know I think
> this is better solution than wrapper. Consider case:
> 1. PM_RUNTIME unset.
> 2. System suspends.
> 3. The pl330 in its suspend callback calls force_runtime_suspend which
> leads us to amba/bus.
> 4. The amba/bus.c in runtime suspend checks for irq_safe (it is FALSE),
> so it disables and unprepares the clock.
> 5. The pl330 in probe requested irq_safe so it assumes amba/bus will
> only disable the clock. So the pl330 unprepares the clock. Again.

To me, this sounds like a good reason to avoid using 
force_runtime_suspend().  In fact, it sounds like a good reason to 
avoid relying on the runtime PM mechanism to handle non-runtime-PM 
things (like a system suspend callback).  If CONFIG_PM_RUNTIME isn't 
enabled then the runtime PM stack simply should not be used.

Alan Stern

Re: [PATCH v10 1/5] PM / Runtime: Allow accessing irq_safe if no PM_RUNTIME

[Rafael J. Wysocki]

On Friday, November 07, 2014 09:50:58 AM Alan Stern wrote:
> On Fri, 7 Nov 2014, Krzysztof Kozlowski wrote:
> 
> > > Well, that is a good reason to introduce a wrapper around power.irq_safe in my
> > > view.
> > > 
> > > And define the wrapper so that it always returns false for CONFIG_PM_RUNTIME
> > > unset.
> > > 
> > > This way not only you wouldn't need to move the flag from under the #ifdef,
> > > but also you would make the compiler skip the relevant pieces of code
> > > entiretly for CONFIG_PM_RUNTIME unset.
> > 
> > Few days ago I would be happy with your opinion :), but know I think
> > this is better solution than wrapper. Consider case:
> > 1. PM_RUNTIME unset.
> > 2. System suspends.
> > 3. The pl330 in its suspend callback calls force_runtime_suspend which
> > leads us to amba/bus.
> > 4. The amba/bus.c in runtime suspend checks for irq_safe (it is FALSE),
> > so it disables and unprepares the clock.
> > 5. The pl330 in probe requested irq_safe so it assumes amba/bus will
> > only disable the clock. So the pl330 unprepares the clock. Again.
> 
> To me, this sounds like a good reason to avoid using 
> force_runtime_suspend().  In fact, it sounds like a good reason to 
> avoid relying on the runtime PM mechanism to handle non-runtime-PM 
> things (like a system suspend callback).  If CONFIG_PM_RUNTIME isn't 
> enabled then the runtime PM stack simply should not be used.

Amen.

Re: [PATCH v10 1/5] PM / Runtime: Allow accessing irq_safe if no PM_RUNTIME

[Ulf Hansson]

On 7 November 2014 15:50, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Fri, 7 Nov 2014, Krzysztof Kozlowski wrote:
>
>> > Well, that is a good reason to introduce a wrapper around power.irq_safe in my
>> > view.
>> >
>> > And define the wrapper so that it always returns false for CONFIG_PM_RUNTIME
>> > unset.
>> >
>> > This way not only you wouldn't need to move the flag from under the #ifdef,
>> > but also you would make the compiler skip the relevant pieces of code
>> > entiretly for CONFIG_PM_RUNTIME unset.
>>
>> Few days ago I would be happy with your opinion :), but know I think
>> this is better solution than wrapper. Consider case:
>> 1. PM_RUNTIME unset.
>> 2. System suspends.
>> 3. The pl330 in its suspend callback calls force_runtime_suspend which
>> leads us to amba/bus.
>> 4. The amba/bus.c in runtime suspend checks for irq_safe (it is FALSE),
>> so it disables and unprepares the clock.
>> 5. The pl330 in probe requested irq_safe so it assumes amba/bus will
>> only disable the clock. So the pl330 unprepares the clock. Again.
>
> To me, this sounds like a good reason to avoid using
> force_runtime_suspend().  In fact, it sounds like a good reason to
> avoid relying on the runtime PM mechanism to handle non-runtime-PM
> things (like a system suspend callback).  If CONFIG_PM_RUNTIME isn't
> enabled then the runtime PM stack simply should not be used.

There are an important advantage of using the pm_runtime_force_suspend() here.

For the driver to handle clock gating at system PM suspend, it first
needs to bring the device into full power, through
pm_runtime_get_sync(). Otherwise it's not safe to gate the clock,
since it may already be gated.

Kind regards
Uffe

Re: [PATCH v10 1/5] PM / Runtime: Allow accessing irq_safe if no PM_RUNTIME

[Alan Stern]

On Mon, 10 Nov 2014, Ulf Hansson wrote:

> > To me, this sounds like a good reason to avoid using
> > force_runtime_suspend().  In fact, it sounds like a good reason to
> > avoid relying on the runtime PM mechanism to handle non-runtime-PM
> > things (like a system suspend callback).  If CONFIG_PM_RUNTIME isn't
> > enabled then the runtime PM stack simply should not be used.
> 
> There are an important advantage of using the pm_runtime_force_suspend() here.
> 
> For the driver to handle clock gating at system PM suspend, it first
> needs to bring the device into full power, through
> pm_runtime_get_sync(). Otherwise it's not safe to gate the clock,
> since it may already be gated.

That's fine, but it has nothing to do with pm_runtime_force_suspend().

Besides, if the real question is whether or not to gate the clock (or 
in other words, has the clock already been gated), why not just store a 
"clock_is_gated" flag somewhere?

Alan Stern

Re: [PATCH v10 1/5] PM / Runtime: Allow accessing irq_safe if no PM_RUNTIME

[Ulf Hansson]

On 10 November 2014 17:36, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Mon, 10 Nov 2014, Ulf Hansson wrote:
>
>> > To me, this sounds like a good reason to avoid using
>> > force_runtime_suspend().  In fact, it sounds like a good reason to
>> > avoid relying on the runtime PM mechanism to handle non-runtime-PM
>> > things (like a system suspend callback).  If CONFIG_PM_RUNTIME isn't
>> > enabled then the runtime PM stack simply should not be used.
>>
>> There are an important advantage of using the pm_runtime_force_suspend() here.
>>
>> For the driver to handle clock gating at system PM suspend, it first
>> needs to bring the device into full power, through
>> pm_runtime_get_sync(). Otherwise it's not safe to gate the clock,
>> since it may already be gated.
>
> That's fine, but it has nothing to do with pm_runtime_force_suspend().
>
> Besides, if the real question is whether or not to gate the clock (or
> in other words, has the clock already been gated), why not just store a
> "clock_is_gated" flag somewhere?

You could do that, but it's easier to not.

You will need to update the runtime PM status and disable runtime PM
anyway, done by the API.

Kind regards
Uffe

Re: [PATCH v10 1/5] PM / Runtime: Allow accessing irq_safe if no PM_RUNTIME

[Alan Stern]

[CC: list severely trimmed]

On Mon, 10 Nov 2014, Ulf Hansson wrote:

> >> There are an important advantage of using the pm_runtime_force_suspend() here.
> >>
> >> For the driver to handle clock gating at system PM suspend, it first
> >> needs to bring the device into full power, through
> >> pm_runtime_get_sync(). Otherwise it's not safe to gate the clock,
> >> since it may already be gated.
> >
> > That's fine, but it has nothing to do with pm_runtime_force_suspend().
> >
> > Besides, if the real question is whether or not to gate the clock (or
> > in other words, has the clock already been gated), why not just store a
> > "clock_is_gated" flag somewhere?
> 
> You could do that, but it's easier to not.

I'm not convinced.  And you haven't said how this is related to
pm_runtime_force_suspend().

> You will need to update the runtime PM status and disable runtime PM
> anyway, done by the API.

So what?  And what API are you referring to: the runtime PM API or 
something else?

I get the feeling that I'm missing a lot of important points here.  
What have you left out?

As I understand the problem, you've got a bus type where some of the
drivers have IRQ-safe runtime PM (and therefore carry out their
suspend/resume operations in interrupt context) and others don't.  The
subsystem core wants to maximize the power saving by deconfiguring some
clocks whenever a device is suspended, but this requires a process
context and so it can't be done directly when IRQ-safe runtime PM is
used.

Is that a good summary?  If it is, there are ways of dealing with this 
that don't involve messing around with the runtime PM internals, or 
using pm_runtime_force_suspend().

Alan Stern

Re: [PATCH v10 1/5] PM / Runtime: Allow accessing irq_safe if no PM_RUNTIME

[Krzysztof Kozlowski]

On pon, 2014-11-10 at 13:59 -0500, Alan Stern wrote:
> [CC: list severely trimmed]
> 
> On Mon, 10 Nov 2014, Ulf Hansson wrote:
> 
> > >> There are an important advantage of using the pm_runtime_force_suspend() here.
> > >>
> > >> For the driver to handle clock gating at system PM suspend, it first
> > >> needs to bring the device into full power, through
> > >> pm_runtime_get_sync(). Otherwise it's not safe to gate the clock,
> > >> since it may already be gated.
> > >
> > > That's fine, but it has nothing to do with pm_runtime_force_suspend().
> > >
> > > Besides, if the real question is whether or not to gate the clock (or
> > > in other words, has the clock already been gated), why not just store a
> > > "clock_is_gated" flag somewhere?
> > 
> > You could do that, but it's easier to not.
> 
> I'm not convinced.  And you haven't said how this is related to
> pm_runtime_force_suspend().
> 
> > You will need to update the runtime PM status and disable runtime PM
> > anyway, done by the API.
> 
> So what?  And what API are you referring to: the runtime PM API or 
> something else?
> 
> I get the feeling that I'm missing a lot of important points here.  
> What have you left out?
> 
> As I understand the problem, you've got a bus type where some of the
> drivers have IRQ-safe runtime PM (and therefore carry out their
> suspend/resume operations in interrupt context) and others don't.  The
> subsystem core wants to maximize the power saving by deconfiguring some
> clocks whenever a device is suspended, but this requires a process
> context and so it can't be done directly when IRQ-safe runtime PM is
> used.
> 
> Is that a good summary?  If it is, there are ways of dealing with this 
> that don't involve messing around with the runtime PM internals, or 
> using pm_runtime_force_suspend().

Thank you all for this precious feedback. I'll try the way with skipping
runtime PM stack completely during system suspend. Probably that will
add some complexity to the amba/bus and pl330 but there shouldn't be any
changes to PM core.

Best regards,
Krzysztof

Re: [PATCH v10 1/5] PM / Runtime: Allow accessing irq_safe if no PM_RUNTIME

[Ulf Hansson]

On 10 November 2014 19:59, Alan Stern <stern@rowland.harvard.edu> wrote:
> [CC: list severely trimmed]
>
> On Mon, 10 Nov 2014, Ulf Hansson wrote:
>
>> >> There are an important advantage of using the pm_runtime_force_suspend() here.
>> >>
>> >> For the driver to handle clock gating at system PM suspend, it first
>> >> needs to bring the device into full power, through
>> >> pm_runtime_get_sync(). Otherwise it's not safe to gate the clock,
>> >> since it may already be gated.
>> >
>> > That's fine, but it has nothing to do with pm_runtime_force_suspend().
>> >
>> > Besides, if the real question is whether or not to gate the clock (or
>> > in other words, has the clock already been gated), why not just store a
>> > "clock_is_gated" flag somewhere?
>>
>> You could do that, but it's easier to not.
>
> I'm not convinced.  And you haven't said how this is related to
> pm_runtime_force_suspend().
>
>> You will need to update the runtime PM status and disable runtime PM
>> anyway, done by the API.
>
> So what?  And what API are you referring to: the runtime PM API or
> something else?

Sorry if I was unclear, I was referring to pm_runtime_force_suspend().

>
> I get the feeling that I'm missing a lot of important points here.
> What have you left out?

First, this patch which we are discussing is about how the amba
bus/drivers should cope with irq_safe devices. I think we have already
solved that through Rafael's latest suggestion.

In principle what you want me to elaborate upon, is why the
pm_runtime_force_suspend|resume() helpers is a good choice for AMBA
bus/drivers to use, right?

Indeed the pm_runtime_force_suspend|resume() helpers was discussed
thoroughly before Rafael, decided to merge them. I do now realize that
I forgot to add some information about them into the runtime PM
documentation, my bad. Will fix it soon.

Anyway, this is copied from the function header, which gives you an
overall idea why they are good for AMBA.

/**
 * pm_runtime_force_suspend - Force a device into suspend state if needed.
 * @dev: Device to suspend.
 *
 * Disable runtime PM so we safely can check the device's runtime PM status and
 * if it is active, invoke it's .runtime_suspend callback to bring it into
 * suspend state. Keep runtime PM disabled to preserve the state unless we
 * encounter errors.
 *
 * Typically this function may be invoked from a system suspend callback to make
 * sure the device is put into low power state.
 */

Since AMBA bus/drivers are handling runtime PM resources like
clocks/pinctrl/etc from their runtime PM callbacks, using
pm_runtime_force_suspen|resume() from their system PM callbacks makes
perfect sense to me.

>
> As I understand the problem, you've got a bus type where some of the
> drivers have IRQ-safe runtime PM (and therefore carry out their
> suspend/resume operations in interrupt context) and others don't.  The
> subsystem core wants to maximize the power saving by deconfiguring some
> clocks whenever a device is suspended, but this requires a process
> context and so it can't be done directly when IRQ-safe runtime PM is
> used.
>
> Is that a good summary?  If it is, there are ways of dealing with this

Yes.

> that don't involve messing around with the runtime PM internals, or
> using pm_runtime_force_suspend().

The logic to handle that would in principle mean to keep track of the
device's runtime PM status. That doesn't make sense, it's the job of
the runtime PM core to do that!?

Additionally, you would also need to protect the runtime PM resume
callback from doing clock enable during system PM, unless you also
disable runtime PM.

To me, pm_runtime_force_suspend|resume() will do exactly what we need
for the AMBA case.

Kind regards
Uffe

Re: [PATCH v10 1/5] PM / Runtime: Allow accessing irq_safe if no PM_RUNTIME

[Krzysztof Kozlowski]

On śro, 2014-11-12 at 11:39 +0100, Ulf Hansson wrote:
> On 10 November 2014 19:59, Alan Stern <stern@rowland.harvard.edu> wrote:
> > [CC: list severely trimmed]
> >
> > On Mon, 10 Nov 2014, Ulf Hansson wrote:
> >
> >> >> There are an important advantage of using the pm_runtime_force_suspend() here.
> >> >>
> >> >> For the driver to handle clock gating at system PM suspend, it first
> >> >> needs to bring the device into full power, through
> >> >> pm_runtime_get_sync(). Otherwise it's not safe to gate the clock,
> >> >> since it may already be gated.
> >> >
> >> > That's fine, but it has nothing to do with pm_runtime_force_suspend().
> >> >
> >> > Besides, if the real question is whether or not to gate the clock (or
> >> > in other words, has the clock already been gated), why not just store a
> >> > "clock_is_gated" flag somewhere?
> >>
> >> You could do that, but it's easier to not.
> >
> > I'm not convinced.  And you haven't said how this is related to
> > pm_runtime_force_suspend().
> >
> >> You will need to update the runtime PM status and disable runtime PM
> >> anyway, done by the API.
> >
> > So what?  And what API are you referring to: the runtime PM API or
> > something else?
> 
> Sorry if I was unclear, I was referring to pm_runtime_force_suspend().
> 
> >
> > I get the feeling that I'm missing a lot of important points here.
> > What have you left out?
> 
> First, this patch which we are discussing is about how the amba
> bus/drivers should cope with irq_safe devices. I think we have already
> solved that through Rafael's latest suggestion.
> 
> In principle what you want me to elaborate upon, is why the
> pm_runtime_force_suspend|resume() helpers is a good choice for AMBA
> bus/drivers to use, right?
> 
> Indeed the pm_runtime_force_suspend|resume() helpers was discussed
> thoroughly before Rafael, decided to merge them. I do now realize that
> I forgot to add some information about them into the runtime PM
> documentation, my bad. Will fix it soon.
> 
> Anyway, this is copied from the function header, which gives you an
> overall idea why they are good for AMBA.
> 
> /**
>  * pm_runtime_force_suspend - Force a device into suspend state if needed.
>  * @dev: Device to suspend.
>  *
>  * Disable runtime PM so we safely can check the device's runtime PM status and
>  * if it is active, invoke it's .runtime_suspend callback to bring it into
>  * suspend state. Keep runtime PM disabled to preserve the state unless we
>  * encounter errors.
>  *
>  * Typically this function may be invoked from a system suspend callback to make
>  * sure the device is put into low power state.
>  */
> 
> Since AMBA bus/drivers are handling runtime PM resources like
> clocks/pinctrl/etc from their runtime PM callbacks, using
> pm_runtime_force_suspen|resume() from their system PM callbacks makes
> perfect sense to me.
> 
> >
> > As I understand the problem, you've got a bus type where some of the
> > drivers have IRQ-safe runtime PM (and therefore carry out their
> > suspend/resume operations in interrupt context) and others don't.  The
> > subsystem core wants to maximize the power saving by deconfiguring some
> > clocks whenever a device is suspended, but this requires a process
> > context and so it can't be done directly when IRQ-safe runtime PM is
> > used.
> >
> > Is that a good summary?  If it is, there are ways of dealing with this
> 
> Yes.
> 
> > that don't involve messing around with the runtime PM internals, or
> > using pm_runtime_force_suspend().
> 
> The logic to handle that would in principle mean to keep track of the
> device's runtime PM status. That doesn't make sense, it's the job of
> the runtime PM core to do that!?
> 
> Additionally, you would also need to protect the runtime PM resume
> callback from doing clock enable during system PM, unless you also
> disable runtime PM.
> 
> To me, pm_runtime_force_suspend|resume() will do exactly what we need
> for the AMBA case.

Hmm... you're saying that runtime resume/suspend could happen during
execution of system resume/suspend callback?

Or maybe the runtime resume callback could be called after suspending
the device but before system sleep finishes?

In pl330 I wanted to do something like:

static int __maybe_unused pl330_suspend(struct device *dev)
{
	struct amba_device *pcdev = to_amba_device(dev);

	if (!pm_runtime_status_suspended(dev)) {
		/* amba did not disable the clock */
		amba_pclk_disable(pcdev);
	}
	amba_pclk_unprepare(pcdev);

	return 0;
}

However I still wonder how this is safe against concurrent runtime PM...

Best regards,
Krzysztof

Re: [PATCH v10 1/5] PM / Runtime: Allow accessing irq_safe if no PM_RUNTIME

[Alan Stern]

On Wed, 12 Nov 2014, Ulf Hansson wrote:

> Since AMBA bus/drivers are handling runtime PM resources like
> clocks/pinctrl/etc from their runtime PM callbacks, using
> pm_runtime_force_suspen|resume() from their system PM callbacks makes
> perfect sense to me.

This sounds like a case of misdesign.  If clocks/pinctrl/etc are all
purely _runtime_ PM resources then the _system_ PM callbacks should
have nothing to do with them.  On the other hand, if they are general
PM resources then they should be handled from a general-purpose routine
that can be invoked by both the runtime-PM and system-PM callbacks.

It makes very little sense to have a special-purpose routine manage a 
general-purpose resource.

> > As I understand the problem, you've got a bus type where some of the
> > drivers have IRQ-safe runtime PM (and therefore carry out their
> > suspend/resume operations in interrupt context) and others don't.  The
> > subsystem core wants to maximize the power saving by deconfiguring some
> > clocks whenever a device is suspended, but this requires a process
> > context and so it can't be done directly when IRQ-safe runtime PM is
> > used.
> >
> > Is that a good summary?  If it is, there are ways of dealing with this
> 
> Yes.
> 
> > that don't involve messing around with the runtime PM internals, or
> > using pm_runtime_force_suspend().
> 
> The logic to handle that would in principle mean to keep track of the
> device's runtime PM status. That doesn't make sense, it's the job of
> the runtime PM core to do that!?

Why do think we have pm_runtime_suspended() and related routines?  They 
are there so that your code can make decisions based on the runtime PM 
status without having to keep track of it.

> Additionally, you would also need to protect the runtime PM resume
> callback from doing clock enable during system PM, unless you also
> disable runtime PM.
> 
> To me, pm_runtime_force_suspend|resume() will do exactly what we need
> for the AMBA case.

Your description leaves out large parts of the design.  Please list the
relevant resources, together with an explanation of exactly how each
one is handled by the system- and runtime-PM callback routines, in your
scheme.

Alan Stern

Re: [PATCH v10 1/5] PM / Runtime: Allow accessing irq_safe if no PM_RUNTIME

[Ulf Hansson]

On 12 November 2014 17:34, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 12 Nov 2014, Ulf Hansson wrote:
>
>> Since AMBA bus/drivers are handling runtime PM resources like
>> clocks/pinctrl/etc from their runtime PM callbacks, using
>> pm_runtime_force_suspen|resume() from their system PM callbacks makes
>> perfect sense to me.
>
> This sounds like a case of misdesign.  If clocks/pinctrl/etc are all
> purely _runtime_ PM resources then the _system_ PM callbacks should
> have nothing to do with them.  On the other hand, if they are general
> PM resources then they should be handled from a general-purpose routine
> that can be invoked by both the runtime-PM and system-PM callbacks.
>
> It makes very little sense to have a special-purpose routine manage a
> general-purpose resource.

I don't agree and thought you also were of the opposite opinion. :-)

If I remember correctly, it was you who came up with the really
brilliant idea to reuse the runtime PM callbacks for this matter!?

>From that, we ended up in adding the SET_PM_RUNTIME_PM_OPS macro.

>
>> > As I understand the problem, you've got a bus type where some of the
>> > drivers have IRQ-safe runtime PM (and therefore carry out their
>> > suspend/resume operations in interrupt context) and others don't.  The
>> > subsystem core wants to maximize the power saving by deconfiguring some
>> > clocks whenever a device is suspended, but this requires a process
>> > context and so it can't be done directly when IRQ-safe runtime PM is
>> > used.
>> >
>> > Is that a good summary?  If it is, there are ways of dealing with this
>>
>> Yes.
>>
>> > that don't involve messing around with the runtime PM internals, or
>> > using pm_runtime_force_suspend().
>>
>> The logic to handle that would in principle mean to keep track of the
>> device's runtime PM status. That doesn't make sense, it's the job of
>> the runtime PM core to do that!?
>
> Why do think we have pm_runtime_suspended() and related routines?  They
> are there so that your code can make decisions based on the runtime PM
> status without having to keep track of it.

Those runtime PM routines your refer to, are exactly what the
pm_runtime_force_suspend|resume() helpers make use of. It seems like a
bad idea to duplicate similar code into each an every bus/driver to
handle the same issue.

If we shouldn't use pm_runtime_force_suspend|resume() helpers for
these cases, when do you think it would be appropriate to use them?

>
>> Additionally, you would also need to protect the runtime PM resume
>> callback from doing clock enable during system PM, unless you also
>> disable runtime PM.
>>
>> To me, pm_runtime_force_suspend|resume() will do exactly what we need
>> for the AMBA case.
>
> Your description leaves out large parts of the design.  Please list the
> relevant resources, together with an explanation of exactly how each
> one is handled by the system- and runtime-PM callback routines, in your
> scheme.

The runtime PM resources depends on what AMBA device/driver is being
used, typically clocks and pinctrls.

Simplified described, these bus/drivers PM support are designed in a
runtime PM centric manner. It means at system PM suspend, the
bus/driver wants to make sure the runtime PM resources are in a
runtime PM suspend state.

Does that answer your question?

Kind regards
Uffe

Re: [PATCH v10 1/5] PM / Runtime: Allow accessing irq_safe if no PM_RUNTIME

[Alan Stern]

On Thu, 13 Nov 2014, Ulf Hansson wrote:

> The runtime PM resources depends on what AMBA device/driver is being
> used, typically clocks and pinctrls.
> 
> Simplified described, these bus/drivers PM support are designed in a
> runtime PM centric manner. It means at system PM suspend, the
> bus/driver wants to make sure the runtime PM resources are in a
> runtime PM suspend state.
> 
> Does that answer your question?

Yes, pretty much.  It sounds like pm_runtime_force_suspend does do want 
you want, provided you don't have to worry about wakeup settings.

And I guess if you're going to take this runtime-PM-centric approach,
and if you're also going to rely on some drivers being IRQ-safe and
others not, then the irq_safe flag needs to exist even when runtime PM
is not configured.

It still feels like a backward approach, not the way I would have done
it.  But never mind...

Alan Stern

Re: [PATCH v10 1/5] PM / Runtime: Allow accessing irq_safe if no PM_RUNTIME

[Ulf Hansson]

On 7 November 2014 09:06, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> On czw, 2014-11-06 at 23:51 +0100, Rafael J. Wysocki wrote:
>> On Thursday, November 06, 2014 09:36:46 AM Krzysztof Kozlowski wrote:
>> > Some drivers (e.g. bus drivers) may want to check if power.irq_safe was
>> > called by child driver, regardless of CONFIG_PM_RUNTIME.
>> >
>> > An example scenario is amba/bus.c and dma/pl330.c drivers. The runtime
>> > suspend/resume callbacks in amba bus driver act differently if irq_safe
>> > was set by child driver (in irq_safe mode bus clock is only disabled).
>> >
>> > The pl330 driver sets irq_safe and assumes that amba bus driver will
>> > only disable the clock in runtime PM. So in system sleep suspend
>> > callback the pl330 driver unprepares the clock after calling
>> > pm_runtime_force_suspend().
>> >
>> > However inconsistency would appear if CONFIG_PM_RUNTIME is not set and
>> > child drivers do not want the irq_safe runtime PM. In such case amba bus
>> > driver still has to know whether child driver wanted irq_safe - by
>> > looking at dev->power.irq_safe field.
>> >
>> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>> > ---
>> >  include/linux/pm.h         | 2 +-
>> >  include/linux/pm_runtime.h | 5 ++++-
>> >  2 files changed, 5 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/include/linux/pm.h b/include/linux/pm.h
>> > index 383fd68aaee1..b05fa954f50d 100644
>> > --- a/include/linux/pm.h
>> > +++ b/include/linux/pm.h
>> > @@ -566,6 +566,7 @@ struct dev_pm_info {
>> >     bool                    ignore_children:1;
>> >     bool                    early_init:1;   /* Owned by the PM core */
>> >     bool                    direct_complete:1;      /* Owned by the PM core */
>> > +   unsigned int            irq_safe:1;     /* PM runtime */
>> >     spinlock_t              lock;
>> >  #ifdef CONFIG_PM_SLEEP
>> >     struct list_head        entry;
>> > @@ -590,7 +591,6 @@ struct dev_pm_info {
>> >     unsigned int            run_wake:1;
>> >     unsigned int            runtime_auto:1;
>> >     unsigned int            no_callbacks:1;
>> > -   unsigned int            irq_safe:1;
>> >     unsigned int            use_autosuspend:1;
>> >     unsigned int            timer_autosuspends:1;
>> >     unsigned int            memalloc_noio:1;
>>
>> Well, that is a good reason to introduce a wrapper around power.irq_safe in my
>> view.
>>
>> And define the wrapper so that it always returns false for CONFIG_PM_RUNTIME
>> unset.
>>
>> This way not only you wouldn't need to move the flag from under the #ifdef,
>> but also you would make the compiler skip the relevant pieces of code
>> entiretly for CONFIG_PM_RUNTIME unset.
>
> Few days ago I would be happy with your opinion :), but know I think
> this is better solution than wrapper. Consider case:
> 1. PM_RUNTIME unset.
> 2. System suspends.
> 3. The pl330 in its suspend callback calls force_runtime_suspend which
> leads us to amba/bus.
> 4. The amba/bus.c in runtime suspend checks for irq_safe (it is FALSE),
> so it disables and unprepares the clock.
> 5. The pl330 in probe requested irq_safe so it assumes amba/bus will
> only disable the clock. So the pl330 unprepares the clock. Again.

This is easy to solve, still by using Rafael's approach.

In the pl330 system PM callbacks, you need to check
"pm_runtime_is_irqsafe()" or whatever the wrapper would be called.
When it returns true, that's when you should do
clk_prepare|unprepare().

I think that would be quite nice.

Kind regards
Uffe

Re: [PATCH v10 1/5] PM / Runtime: Allow accessing irq_safe if no PM_RUNTIME

[Srikanth K]

unsubscribe

[PATCH v10 2/5] amba: Add helpers for (un)preparing AMBA clock

[Krzysztof Kozlowski]

Add amba_pclk_prepare() and amba_pclk_unprepare() inline functions for
handling the AMBA bus clock by device drivers.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 include/linux/amba/bus.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index c324f5700d1a..ac02f9bd63dc 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -97,6 +97,16 @@ void amba_release_regions(struct amba_device *);
 #define amba_pclk_disable(d)	\
 	do { if (!IS_ERR((d)->pclk)) clk_disable((d)->pclk); } while (0)
 
+static inline int amba_pclk_prepare(struct amba_device *dev)
+{
+	return clk_prepare(dev->pclk);
+}
+
+static inline void amba_pclk_unprepare(struct amba_device *dev)
+{
+	clk_unprepare(dev->pclk);
+}
+
 /* Some drivers don't use the struct amba_device */
 #define AMBA_CONFIG_BITS(a) (((a) >> 24) & 0xff)
 #define AMBA_REV_BITS(a) (((a) >> 20) & 0x0f)
-- 
1.9.1

[PATCH v10 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Krzysztof Kozlowski]

The AMBA bus driver defines runtime Power Management functions which
disable and unprepare AMBA bus clock. This is problematic for runtime PM
because unpreparing a clock might sleep so it is not interrupt safe.

However some drivers may want to implement runtime PM functions in
interrupt-safe way (see pm_runtime_irq_safe()). In such case the AMBA
bus driver should only disable/enable the clock in runtime suspend and
resume callbacks.

Detect the device driver behavior during runtime suspend. During runtime
resume deal with clocks according to stored value.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/amba/bus.c       | 17 +++++++++++++----
 include/linux/amba/bus.h |  1 +
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 47bbdc1b5be3..356f906c6966 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -95,8 +95,14 @@ static int amba_pm_runtime_suspend(struct device *dev)
 	struct amba_device *pcdev = to_amba_device(dev);
 	int ret = pm_generic_runtime_suspend(dev);
 
-	if (ret == 0 && dev->driver)
-		clk_disable_unprepare(pcdev->pclk);
+	if (ret == 0 && dev->driver) {
+		pcdev->irq_safe = dev->power.irq_safe;
+
+		if (pcdev->irq_safe)
+			clk_disable(pcdev->pclk);
+		else
+			clk_disable_unprepare(pcdev->pclk);
+	}
 
 	return ret;
 }
@@ -107,7 +113,10 @@ static int amba_pm_runtime_resume(struct device *dev)
 	int ret;
 
 	if (dev->driver) {
-		ret = clk_prepare_enable(pcdev->pclk);
+		if (pcdev->irq_safe)
+			ret = clk_enable(pcdev->pclk);
+		else
+			ret = clk_prepare_enable(pcdev->pclk);
 		/* Failure is probably fatal to the system, but... */
 		if (ret)
 			return ret;
@@ -115,7 +124,7 @@ static int amba_pm_runtime_resume(struct device *dev)
 
 	return pm_generic_runtime_resume(dev);
 }
-#endif
+#endif /* CONFIG_PM */
 
 static const struct dev_pm_ops amba_pm = {
 	.suspend	= pm_generic_suspend,
diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index ac02f9bd63dc..c4bae79851fb 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -32,6 +32,7 @@ struct amba_device {
 	struct clk		*pclk;
 	unsigned int		periphid;
 	unsigned int		irq[AMBA_NR_IRQS];
+	unsigned int		irq_safe:1;
 };
 
 struct amba_driver {
-- 
1.9.1

Re: [PATCH v10 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Ulf Hansson]

On 6 November 2014 09:36, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> The AMBA bus driver defines runtime Power Management functions which
> disable and unprepare AMBA bus clock. This is problematic for runtime PM
> because unpreparing a clock might sleep so it is not interrupt safe.
>
> However some drivers may want to implement runtime PM functions in
> interrupt-safe way (see pm_runtime_irq_safe()). In such case the AMBA
> bus driver should only disable/enable the clock in runtime suspend and
> resume callbacks.
>
> Detect the device driver behavior during runtime suspend. During runtime
> resume deal with clocks according to stored value.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

> ---
>  drivers/amba/bus.c       | 17 +++++++++++++----
>  include/linux/amba/bus.h |  1 +
>  2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 47bbdc1b5be3..356f906c6966 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -95,8 +95,14 @@ static int amba_pm_runtime_suspend(struct device *dev)
>         struct amba_device *pcdev = to_amba_device(dev);
>         int ret = pm_generic_runtime_suspend(dev);
>
> -       if (ret == 0 && dev->driver)
> -               clk_disable_unprepare(pcdev->pclk);
> +       if (ret == 0 && dev->driver) {
> +               pcdev->irq_safe = dev->power.irq_safe;
> +
> +               if (pcdev->irq_safe)
> +                       clk_disable(pcdev->pclk);
> +               else
> +                       clk_disable_unprepare(pcdev->pclk);
> +       }
>
>         return ret;
>  }
> @@ -107,7 +113,10 @@ static int amba_pm_runtime_resume(struct device *dev)
>         int ret;
>
>         if (dev->driver) {
> -               ret = clk_prepare_enable(pcdev->pclk);
> +               if (pcdev->irq_safe)
> +                       ret = clk_enable(pcdev->pclk);
> +               else
> +                       ret = clk_prepare_enable(pcdev->pclk);
>                 /* Failure is probably fatal to the system, but... */
>                 if (ret)
>                         return ret;
> @@ -115,7 +124,7 @@ static int amba_pm_runtime_resume(struct device *dev)
>
>         return pm_generic_runtime_resume(dev);
>  }
> -#endif
> +#endif /* CONFIG_PM */
>
>  static const struct dev_pm_ops amba_pm = {
>         .suspend        = pm_generic_suspend,
> diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
> index ac02f9bd63dc..c4bae79851fb 100644
> --- a/include/linux/amba/bus.h
> +++ b/include/linux/amba/bus.h
> @@ -32,6 +32,7 @@ struct amba_device {
>         struct clk              *pclk;
>         unsigned int            periphid;
>         unsigned int            irq[AMBA_NR_IRQS];
> +       unsigned int            irq_safe:1;
>  };
>
>  struct amba_driver {
> --
> 1.9.1
>

Re: [PATCH v10 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Rafael J. Wysocki]

On Thursday, November 06, 2014 09:36:48 AM Krzysztof Kozlowski wrote:
> The AMBA bus driver defines runtime Power Management functions which
> disable and unprepare AMBA bus clock. This is problematic for runtime PM
> because unpreparing a clock might sleep so it is not interrupt safe.
> 
> However some drivers may want to implement runtime PM functions in
> interrupt-safe way (see pm_runtime_irq_safe()). In such case the AMBA
> bus driver should only disable/enable the clock in runtime suspend and
> resume callbacks.
> 
> Detect the device driver behavior during runtime suspend. During runtime
> resume deal with clocks according to stored value.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
>  drivers/amba/bus.c       | 17 +++++++++++++----
>  include/linux/amba/bus.h |  1 +
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 47bbdc1b5be3..356f906c6966 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -95,8 +95,14 @@ static int amba_pm_runtime_suspend(struct device *dev)
>  	struct amba_device *pcdev = to_amba_device(dev);
>  	int ret = pm_generic_runtime_suspend(dev);
>  
> -	if (ret == 0 && dev->driver)
> -		clk_disable_unprepare(pcdev->pclk);
> +	if (ret == 0 && dev->driver) {
> +		pcdev->irq_safe = dev->power.irq_safe;
> +
> +		if (pcdev->irq_safe)
> +			clk_disable(pcdev->pclk);
> +		else
> +			clk_disable_unprepare(pcdev->pclk);
> +	}
>  
>  	return ret;
>  }
> @@ -107,7 +113,10 @@ static int amba_pm_runtime_resume(struct device *dev)
>  	int ret;
>  
>  	if (dev->driver) {
> -		ret = clk_prepare_enable(pcdev->pclk);
> +		if (pcdev->irq_safe)
> +			ret = clk_enable(pcdev->pclk);
> +		else
> +			ret = clk_prepare_enable(pcdev->pclk);
>  		/* Failure is probably fatal to the system, but... */
>  		if (ret)
>  			return ret;
> @@ -115,7 +124,7 @@ static int amba_pm_runtime_resume(struct device *dev)
>  
>  	return pm_generic_runtime_resume(dev);
>  }
> -#endif
> +#endif /* CONFIG_PM */
>  
>  static const struct dev_pm_ops amba_pm = {
>  	.suspend	= pm_generic_suspend,
> diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
> index ac02f9bd63dc..c4bae79851fb 100644
> --- a/include/linux/amba/bus.h
> +++ b/include/linux/amba/bus.h
> @@ -32,6 +32,7 @@ struct amba_device {
>  	struct clk		*pclk;
>  	unsigned int		periphid;
>  	unsigned int		irq[AMBA_NR_IRQS];
> +	unsigned int		irq_safe:1;

Why do we need the new flag?  Seems redundant to me.

>  };
>  
>  struct amba_driver {
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v10 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Krzysztof Kozlowski]

On czw, 2014-11-06 at 23:52 +0100, Rafael J. Wysocki wrote:
> On Thursday, November 06, 2014 09:36:48 AM Krzysztof Kozlowski wrote:
> > The AMBA bus driver defines runtime Power Management functions which
> > disable and unprepare AMBA bus clock. This is problematic for runtime PM
> > because unpreparing a clock might sleep so it is not interrupt safe.
> > 
> > However some drivers may want to implement runtime PM functions in
> > interrupt-safe way (see pm_runtime_irq_safe()). In such case the AMBA
> > bus driver should only disable/enable the clock in runtime suspend and
> > resume callbacks.
> > 
> > Detect the device driver behavior during runtime suspend. During runtime
> > resume deal with clocks according to stored value.
> > 
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > ---
> >  drivers/amba/bus.c       | 17 +++++++++++++----
> >  include/linux/amba/bus.h |  1 +
> >  2 files changed, 14 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> > index 47bbdc1b5be3..356f906c6966 100644
> > --- a/drivers/amba/bus.c
> > +++ b/drivers/amba/bus.c
> > @@ -95,8 +95,14 @@ static int amba_pm_runtime_suspend(struct device *dev)
> >  	struct amba_device *pcdev = to_amba_device(dev);
> >  	int ret = pm_generic_runtime_suspend(dev);
> >  
> > -	if (ret == 0 && dev->driver)
> > -		clk_disable_unprepare(pcdev->pclk);
> > +	if (ret == 0 && dev->driver) {
> > +		pcdev->irq_safe = dev->power.irq_safe;
> > +
> > +		if (pcdev->irq_safe)
> > +			clk_disable(pcdev->pclk);
> > +		else
> > +			clk_disable_unprepare(pcdev->pclk);
> > +	}
> >  
> >  	return ret;
> >  }
> > @@ -107,7 +113,10 @@ static int amba_pm_runtime_resume(struct device *dev)
> >  	int ret;
> >  
> >  	if (dev->driver) {
> > -		ret = clk_prepare_enable(pcdev->pclk);
> > +		if (pcdev->irq_safe)
> > +			ret = clk_enable(pcdev->pclk);
> > +		else
> > +			ret = clk_prepare_enable(pcdev->pclk);
> >  		/* Failure is probably fatal to the system, but... */
> >  		if (ret)
> >  			return ret;
> > @@ -115,7 +124,7 @@ static int amba_pm_runtime_resume(struct device *dev)
> >  
> >  	return pm_generic_runtime_resume(dev);
> >  }
> > -#endif
> > +#endif /* CONFIG_PM */
> >  
> >  static const struct dev_pm_ops amba_pm = {
> >  	.suspend	= pm_generic_suspend,
> > diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
> > index ac02f9bd63dc..c4bae79851fb 100644
> > --- a/include/linux/amba/bus.h
> > +++ b/include/linux/amba/bus.h
> > @@ -32,6 +32,7 @@ struct amba_device {
> >  	struct clk		*pclk;
> >  	unsigned int		periphid;
> >  	unsigned int		irq[AMBA_NR_IRQS];
> > +	unsigned int		irq_safe:1;
> 
> Why do we need the new flag?  Seems redundant to me.

Some poorly written driver could set irq_safe somewhere between suspend
and resume (e.g. in suspend callback).

Best regards,
Krzysztof

Re: [PATCH v10 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Rafael J. Wysocki]

On Friday, November 07, 2014 09:01:09 AM Krzysztof Kozlowski wrote:
> On czw, 2014-11-06 at 23:52 +0100, Rafael J. Wysocki wrote:
> > On Thursday, November 06, 2014 09:36:48 AM Krzysztof Kozlowski wrote:
> > > The AMBA bus driver defines runtime Power Management functions which
> > > disable and unprepare AMBA bus clock. This is problematic for runtime PM
> > > because unpreparing a clock might sleep so it is not interrupt safe.
> > > 
> > > However some drivers may want to implement runtime PM functions in
> > > interrupt-safe way (see pm_runtime_irq_safe()). In such case the AMBA
> > > bus driver should only disable/enable the clock in runtime suspend and
> > > resume callbacks.
> > > 
> > > Detect the device driver behavior during runtime suspend. During runtime
> > > resume deal with clocks according to stored value.
> > > 
> > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > > ---
> > >  drivers/amba/bus.c       | 17 +++++++++++++----
> > >  include/linux/amba/bus.h |  1 +
> > >  2 files changed, 14 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> > > index 47bbdc1b5be3..356f906c6966 100644
> > > --- a/drivers/amba/bus.c
> > > +++ b/drivers/amba/bus.c
> > > @@ -95,8 +95,14 @@ static int amba_pm_runtime_suspend(struct device *dev)
> > >  	struct amba_device *pcdev = to_amba_device(dev);
> > >  	int ret = pm_generic_runtime_suspend(dev);
> > >  
> > > -	if (ret == 0 && dev->driver)
> > > -		clk_disable_unprepare(pcdev->pclk);
> > > +	if (ret == 0 && dev->driver) {
> > > +		pcdev->irq_safe = dev->power.irq_safe;
> > > +
> > > +		if (pcdev->irq_safe)
> > > +			clk_disable(pcdev->pclk);
> > > +		else
> > > +			clk_disable_unprepare(pcdev->pclk);
> > > +	}
> > >  
> > >  	return ret;
> > >  }
> > > @@ -107,7 +113,10 @@ static int amba_pm_runtime_resume(struct device *dev)
> > >  	int ret;
> > >  
> > >  	if (dev->driver) {
> > > -		ret = clk_prepare_enable(pcdev->pclk);
> > > +		if (pcdev->irq_safe)
> > > +			ret = clk_enable(pcdev->pclk);
> > > +		else
> > > +			ret = clk_prepare_enable(pcdev->pclk);
> > >  		/* Failure is probably fatal to the system, but... */
> > >  		if (ret)
> > >  			return ret;
> > > @@ -115,7 +124,7 @@ static int amba_pm_runtime_resume(struct device *dev)
> > >  
> > >  	return pm_generic_runtime_resume(dev);
> > >  }
> > > -#endif
> > > +#endif /* CONFIG_PM */
> > >  
> > >  static const struct dev_pm_ops amba_pm = {
> > >  	.suspend	= pm_generic_suspend,
> > > diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
> > > index ac02f9bd63dc..c4bae79851fb 100644
> > > --- a/include/linux/amba/bus.h
> > > +++ b/include/linux/amba/bus.h
> > > @@ -32,6 +32,7 @@ struct amba_device {
> > >  	struct clk		*pclk;
> > >  	unsigned int		periphid;
> > >  	unsigned int		irq[AMBA_NR_IRQS];
> > > +	unsigned int		irq_safe:1;
> > 
> > Why do we need the new flag?  Seems redundant to me.
> 
> Some poorly written driver could set irq_safe somewhere between suspend
> and resume (e.g. in suspend callback).

It could also modify your bus type irq_safe flag to hide the fact.  In my
opinion, it doesn't really help.

Rafael

[PATCH v10 4/5] dmaengine: pl330: add Power Management support

[Krzysztof Kozlowski]

This patch adds both normal PM suspend/resume support and runtime PM
support to pl330 DMA engine driver.

The runtime power management for pl330 DMA driver allows gating of AMBA
clock (PDMA) in FSYS clock domain, when the device is not processing any
requests. This is necessary to enter low power modes on Exynos SoCs
(e.g. LPA on Exynos4x12 or W-AFTR on Exynos3250).

Runtime PM resuming of the device may happen in atomic context (during
call device_issue_pending()) so pm_runtime_irq_safe() is used. This will
lead only to disabling/enabling of the clock but this is sufficient for
gating the clock and for reducing energy usage.

During system sleep the AMBA bus clock is also unprepared.

Suggested-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/dma/pl330.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 92 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 4839bfa74a10..2bb0d08239bb 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -27,6 +27,7 @@
 #include <linux/of.h>
 #include <linux/of_dma.h>
 #include <linux/err.h>
+#include <linux/pm_runtime.h>
 
 #include "dmaengine.h"
 #define PL330_MAX_CHAN		8
@@ -265,6 +266,9 @@ static unsigned cmd_line;
 
 #define NR_DEFAULT_DESC	16
 
+/* Delay for runtime PM autosuspend, ms */
+#define PL330_AUTOSUSPEND_DELAY 20
+
 /* Populated by the PL330 core driver for DMA API driver's info */
 struct pl330_config {
 	u32	periph_id;
@@ -1958,6 +1962,7 @@ static void pl330_tasklet(unsigned long data)
 	struct dma_pl330_chan *pch = (struct dma_pl330_chan *)data;
 	struct dma_pl330_desc *desc, *_dt;
 	unsigned long flags;
+	bool power_down = false;
 
 	spin_lock_irqsave(&pch->lock, flags);
 
@@ -1972,10 +1977,17 @@ static void pl330_tasklet(unsigned long data)
 	/* Try to submit a req imm. next to the last completed cookie */
 	fill_queue(pch);
 
-	/* Make sure the PL330 Channel thread is active */
-	spin_lock(&pch->thread->dmac->lock);
-	_start(pch->thread);
-	spin_unlock(&pch->thread->dmac->lock);
+	if (list_empty(&pch->work_list)) {
+		spin_lock(&pch->thread->dmac->lock);
+		_stop(pch->thread);
+		spin_unlock(&pch->thread->dmac->lock);
+		power_down = true;
+	} else {
+		/* Make sure the PL330 Channel thread is active */
+		spin_lock(&pch->thread->dmac->lock);
+		_start(pch->thread);
+		spin_unlock(&pch->thread->dmac->lock);
+	}
 
 	while (!list_empty(&pch->completed_list)) {
 		dma_async_tx_callback callback;
@@ -1990,6 +2002,12 @@ static void pl330_tasklet(unsigned long data)
 		if (pch->cyclic) {
 			desc->status = PREP;
 			list_move_tail(&desc->node, &pch->work_list);
+			if (power_down) {
+				spin_lock(&pch->thread->dmac->lock);
+				_start(pch->thread);
+				spin_unlock(&pch->thread->dmac->lock);
+				power_down = false;
+			}
 		} else {
 			desc->status = FREE;
 			list_move_tail(&desc->node, &pch->dmac->desc_pool);
@@ -2004,6 +2022,12 @@ static void pl330_tasklet(unsigned long data)
 		}
 	}
 	spin_unlock_irqrestore(&pch->lock, flags);
+
+	/* If work list empty, power down */
+	if (power_down) {
+		pm_runtime_mark_last_busy(pch->dmac->ddma.dev);
+		pm_runtime_put_autosuspend(pch->dmac->ddma.dev);
+	}
 }
 
 bool pl330_filter(struct dma_chan *chan, void *param)
@@ -2073,6 +2097,7 @@ static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, unsigned
 
 	switch (cmd) {
 	case DMA_TERMINATE_ALL:
+		pm_runtime_get_sync(pl330->ddma.dev);
 		spin_lock_irqsave(&pch->lock, flags);
 
 		spin_lock(&pl330->lock);
@@ -2099,10 +2124,15 @@ static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, unsigned
 			dma_cookie_complete(&desc->txd);
 		}
 
+		if (!list_empty(&pch->work_list))
+			pm_runtime_put(pl330->ddma.dev);
+
 		list_splice_tail_init(&pch->submitted_list, &pl330->desc_pool);
 		list_splice_tail_init(&pch->work_list, &pl330->desc_pool);
 		list_splice_tail_init(&pch->completed_list, &pl330->desc_pool);
 		spin_unlock_irqrestore(&pch->lock, flags);
+		pm_runtime_mark_last_busy(pl330->ddma.dev);
+		pm_runtime_put_autosuspend(pl330->ddma.dev);
 		break;
 	case DMA_SLAVE_CONFIG:
 		slave_config = (struct dma_slave_config *)arg;
@@ -2138,6 +2168,7 @@ static void pl330_free_chan_resources(struct dma_chan *chan)
 
 	tasklet_kill(&pch->task);
 
+	pm_runtime_get_sync(pch->dmac->ddma.dev);
 	spin_lock_irqsave(&pch->lock, flags);
 
 	pl330_release_channel(pch->thread);
@@ -2147,6 +2178,8 @@ static void pl330_free_chan_resources(struct dma_chan *chan)
 		list_splice_tail_init(&pch->work_list, &pch->dmac->desc_pool);
 
 	spin_unlock_irqrestore(&pch->lock, flags);
+	pm_runtime_mark_last_busy(pch->dmac->ddma.dev);
+	pm_runtime_put_autosuspend(pch->dmac->ddma.dev);
 }
 
 static enum dma_status
@@ -2162,6 +2195,15 @@ static void pl330_issue_pending(struct dma_chan *chan)
 	unsigned long flags;
 
 	spin_lock_irqsave(&pch->lock, flags);
+	if (list_empty(&pch->work_list)) {
+		/*
+		 * Warn on nothing pending. Empty submitted_list may
+		 * break our pm_runtime usage counter as it is
+		 * updated on work_list emptiness status.
+		 */
+		WARN_ON(list_empty(&pch->submitted_list));
+		pm_runtime_get_sync(pch->dmac->ddma.dev);
+	}
 	list_splice_tail_init(&pch->submitted_list, &pch->work_list);
 	spin_unlock_irqrestore(&pch->lock, flags);
 
@@ -2585,6 +2627,43 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan,
 	return 0;
 }
 
+/*
+ * Runtime PM callbacks are provided by amba/bus.c driver.
+ *
+ * It is assumed here that IRQ safe runtime PM is chosen in probe and amba
+ * bus driver will only disable/enable the clock in runtime PM suspend/resume.
+ */
+static int __maybe_unused pl330_suspend(struct device *dev)
+{
+	struct amba_device *pcdev = to_amba_device(dev);
+	int ret;
+
+	ret = pm_runtime_force_suspend(dev);
+	if (ret)
+		return ret;
+
+	amba_pclk_unprepare(pcdev);
+
+	return 0;
+}
+
+static int __maybe_unused pl330_resume(struct device *dev)
+{
+	struct amba_device *pcdev = to_amba_device(dev);
+
+	amba_pclk_prepare(pcdev);
+
+	/*
+	 * TODO: Idea for future. The device should not be woken up after
+	 * system resume if it is not needed. It could stay runtime suspended
+	 * waiting for DMA requests. However for safe suspend and resume we
+	 * forcibly resume the device here.
+	 */
+	return pm_runtime_force_resume(dev);
+}
+
+static SIMPLE_DEV_PM_OPS(pl330_pm, pl330_suspend, pl330_resume);
+
 static int
 pl330_probe(struct amba_device *adev, const struct amba_id *id)
 {
@@ -2738,6 +2817,12 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 		pcfg->data_buf_dep, pcfg->data_bus_width / 8, pcfg->num_chan,
 		pcfg->num_peri, pcfg->num_events);
 
+	pm_runtime_irq_safe(&adev->dev);
+	pm_runtime_use_autosuspend(&adev->dev);
+	pm_runtime_set_autosuspend_delay(&adev->dev, PL330_AUTOSUSPEND_DELAY);
+	pm_runtime_mark_last_busy(&adev->dev);
+	pm_runtime_put_autosuspend(&adev->dev);
+
 	return 0;
 probe_err3:
 	/* Idle the DMAC */
@@ -2764,6 +2849,8 @@ static int pl330_remove(struct amba_device *adev)
 	struct pl330_dmac *pl330 = amba_get_drvdata(adev);
 	struct dma_pl330_chan *pch, *_p;
 
+	pm_runtime_get_noresume(pl330->ddma.dev);
+
 	if (adev->dev.of_node)
 		of_dma_controller_free(adev->dev.of_node);
 
@@ -2802,6 +2889,7 @@ static struct amba_driver pl330_driver = {
 	.drv = {
 		.owner = THIS_MODULE,
 		.name = "dma-pl330",
+		.pm = &pl330_pm,
 	},
 	.id_table = pl330_ids,
 	.probe = pl330_probe,
-- 
1.9.1

Re: [PATCH v10 4/5] dmaengine: pl330: add Power Management support

[Vinod Koul]

On Thu, Nov 06, 2014 at 09:36:49AM +0100, Krzysztof Kozlowski wrote:
> This patch adds both normal PM suspend/resume support and runtime PM
> support to pl330 DMA engine driver.
> 
> The runtime power management for pl330 DMA driver allows gating of AMBA
> clock (PDMA) in FSYS clock domain, when the device is not processing any
> requests. This is necessary to enter low power modes on Exynos SoCs
> (e.g. LPA on Exynos4x12 or W-AFTR on Exynos3250).
> 
> Runtime PM resuming of the device may happen in atomic context (during
> call device_issue_pending()) so pm_runtime_irq_safe() is used. This will
> lead only to disabling/enabling of the clock but this is sufficient for
> gating the clock and for reducing energy usage.
> 
> During system sleep the AMBA bus clock is also unprepared.

Acked-by: Vinod Koul <vinod.koul@intel.com>

-- 
~Vinod

[PATCH v10 5/5] amba: Remove unused amba_pclk_enable/disable macros

[Krzysztof Kozlowski]

Remove the amba_pclk_enable and amba_pclk_disable macros because they
are not used by the drivers.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 include/linux/amba/bus.h | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index c4bae79851fb..566adf0e0412 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -92,12 +92,6 @@ struct amba_device *amba_find_device(const char *, struct device *, unsigned int
 int amba_request_regions(struct amba_device *, const char *);
 void amba_release_regions(struct amba_device *);
 
-#define amba_pclk_enable(d)	\
-	(IS_ERR((d)->pclk) ? 0 : clk_enable((d)->pclk))
-
-#define amba_pclk_disable(d)	\
-	do { if (!IS_ERR((d)->pclk)) clk_disable((d)->pclk); } while (0)
-
 static inline int amba_pclk_prepare(struct amba_device *dev)
 {
 	return clk_prepare(dev->pclk);
-- 
1.9.1