[PATCH v7 0/5] amba/dma: pl330: add Power Management support

[PATCH v7 0/5] amba/dma: pl330: add Power Management support

[Krzysztof Kozlowski]

Hi,


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 quering 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 is seventh version of patches adding runtime 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/4 by handling clocks in different
way if device driver set interrupt safe runtime PM.

Any comments are welcome.


Tested on boards with pl330 DMA driver:
 - Trats2 (Exynos4212)
 - Samsung Gear2-like development board (Exynos3250)


TODO/ideas:
===========
Consider renaming existing pm_runtime_irq_safe() function to keep
up with naming convention after adding pm_runtime_is_irq_safe() in
patch 1/5.
This naming:
 - pm_runtime_irq_safe() as setter,
 - pm_runtime_is_irq_safe() as getter,
is contradictory to naming of other PM runtime functions, e.g.:
 - pm_runtime_set_active() as setter,
 - pm_runtime_active() as getter.
I didn't change the naming of pm_runtime_irq_safe() because I wanted
to minimize the impact of the patches.


Best regards,
Krzysztof Kozlowski


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

 Documentation/power/runtime_pm.txt |  4 ++
 drivers/amba/bus.c                 | 29 ++++++++++--
 drivers/dma/pl330.c                | 96 ++++++++++++++++++++++++++++++++++++--
 include/linux/amba/bus.h           | 13 ++++--
 include/linux/pm_runtime.h         |  6 +++
 5 files changed, 136 insertions(+), 12 deletions(-)

-- 
1.9.1

[PATCH v7 1/5] PM / Runtime: Add getter for quering the IRQ safe option

[Krzysztof Kozlowski]

Add a simple getter pm_runtime_is_irq_safe() for quering whether runtime
PM IRQ safe was set or not.

Various bus drivers implementing runtime PM may use choose to suspend
differently based on IRQ safeness status of child driver (e.g. do not
unprepare the clock if IRQ safe is not set).

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 Documentation/power/runtime_pm.txt | 4 ++++
 include/linux/pm_runtime.h         | 6 ++++++
 2 files changed, 10 insertions(+)

diff --git a/Documentation/power/runtime_pm.txt b/Documentation/power/runtime_pm.txt
index f32ce5419573..397b81593142 100644
--- a/Documentation/power/runtime_pm.txt
+++ b/Documentation/power/runtime_pm.txt
@@ -468,6 +468,10 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h:
     - set the power.irq_safe flag for the device, causing the runtime-PM
       callbacks to be invoked with interrupts off
 
+  bool pm_runtime_is_irq_safe(struct device *dev);
+    - return true if power.irq_safe flag was set for the device, causing
+      the runtime-PM callbacks to be invoked with interrupts off
+
   void pm_runtime_mark_last_busy(struct device *dev);
     - set the power.last_busy field to the current time
 
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 367f49b9a1c9..44d74f0f182e 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -128,6 +128,11 @@ static inline void pm_runtime_mark_last_busy(struct device *dev)
 	ACCESS_ONCE(dev->power.last_busy) = jiffies;
 }
 
+static inline bool pm_runtime_is_irq_safe(struct device *dev)
+{
+	return dev->power.irq_safe;
+}
+
 #else /* !CONFIG_PM_RUNTIME */
 
 static inline int __pm_runtime_idle(struct device *dev, int rpmflags)
@@ -167,6 +172,7 @@ 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 bool pm_runtime_is_irq_safe(struct device *dev) { return false; }
 
 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 v7 1/5] PM / Runtime: Add getter for quering the IRQ safe option

[Pavel Machek]

On Wed 2014-09-24 15:50:08, Krzysztof Kozlowski wrote:
> Add a simple getter pm_runtime_is_irq_safe() for quering whether runtime
> PM IRQ safe was set or not.
> 
> Various bus drivers implementing runtime PM may use choose to suspend
> differently based on IRQ safeness status of child driver (e.g. do not
> unprepare the clock if IRQ safe is not set).
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Are you sure this is good interface?

"Tell me if another function works this or that way".

That's certainly not traditional interface, and it seems dangerous to
me. Callbacks now have different semantic requirements based on value
of some flag...

Would it be possible to have two sets of callbacks, one irq safe and
one not?
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v7 1/5] PM / Runtime: Add getter for quering the IRQ safe option

[Alan Stern]

On Wed, 24 Sep 2014, Pavel Machek wrote:

> On Wed 2014-09-24 15:50:08, Krzysztof Kozlowski wrote:
> > Add a simple getter pm_runtime_is_irq_safe() for quering whether runtime
> > PM IRQ safe was set or not.
> > 
> > Various bus drivers implementing runtime PM may use choose to suspend
> > differently based on IRQ safeness status of child driver (e.g. do not
> > unprepare the clock if IRQ safe is not set).
> > 
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> 
> Are you sure this is good interface?
> 
> "Tell me if another function works this or that way".
> 
> That's certainly not traditional interface, and it seems dangerous to
> me. Callbacks now have different semantic requirements based on value
> of some flag...
> 
> Would it be possible to have two sets of callbacks, one irq safe and
> one not?

Or maybe add a flag to the bus-specific device structures, indicating
specifically whether or not the clock should be unprepared during a
runtime suspend.  Then individual drivers could set this flag or not, 
independent of the irq-safe setting.

Alan Stern

Re: [PATCH v7 1/5] PM / Runtime: Add getter for quering the IRQ safe option

[Russell King - ARM Linux]

On Wed, Sep 24, 2014 at 03:47:17PM -0400, Alan Stern wrote:
> On Wed, 24 Sep 2014, Pavel Machek wrote:
> 
> > On Wed 2014-09-24 15:50:08, Krzysztof Kozlowski wrote:
> > > Add a simple getter pm_runtime_is_irq_safe() for quering whether runtime
> > > PM IRQ safe was set or not.
> > > 
> > > Various bus drivers implementing runtime PM may use choose to suspend
> > > differently based on IRQ safeness status of child driver (e.g. do not
> > > unprepare the clock if IRQ safe is not set).
> > > 
> > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> > 
> > Are you sure this is good interface?
> > 
> > "Tell me if another function works this or that way".
> > 
> > That's certainly not traditional interface, and it seems dangerous to
> > me. Callbacks now have different semantic requirements based on value
> > of some flag...
> > 
> > Would it be possible to have two sets of callbacks, one irq safe and
> > one not?
> 
> Or maybe add a flag to the bus-specific device structures, indicating
> specifically whether or not the clock should be unprepared during a
> runtime suspend.  Then individual drivers could set this flag or not, 
> independent of the irq-safe setting.

What you're proposing is _less_ safe, because with your proposal, you
now have the possibility that drivers will tell runtime PM that it has
IRQ safe callbacks, but the bus code tries to prepare/unprepare the
clock, which causes a might-sleep-if warning.

This is fragile.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v7 1/5] PM / Runtime: Add getter for quering the IRQ safe option

[Ulf Hansson]

On 24 September 2014 21:52, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Sep 24, 2014 at 03:47:17PM -0400, Alan Stern wrote:
>> On Wed, 24 Sep 2014, Pavel Machek wrote:
>>
>> > On Wed 2014-09-24 15:50:08, Krzysztof Kozlowski wrote:
>> > > Add a simple getter pm_runtime_is_irq_safe() for quering whether runtime
>> > > PM IRQ safe was set or not.
>> > >
>> > > Various bus drivers implementing runtime PM may use choose to suspend
>> > > differently based on IRQ safeness status of child driver (e.g. do not
>> > > unprepare the clock if IRQ safe is not set).
>> > >
>> > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>> > > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>> >
>> > Are you sure this is good interface?
>> >
>> > "Tell me if another function works this or that way".
>> >
>> > That's certainly not traditional interface, and it seems dangerous to
>> > me. Callbacks now have different semantic requirements based on value
>> > of some flag...
>> >
>> > Would it be possible to have two sets of callbacks, one irq safe and
>> > one not?
>>
>> Or maybe add a flag to the bus-specific device structures, indicating
>> specifically whether or not the clock should be unprepared during a
>> runtime suspend.  Then individual drivers could set this flag or not,
>> independent of the irq-safe setting.
>
> What you're proposing is _less_ safe, because with your proposal, you
> now have the possibility that drivers will tell runtime PM that it has
> IRQ safe callbacks, but the bus code tries to prepare/unprepare the
> clock, which causes a might-sleep-if warning.
>
> This is fragile.

I agree.

I would also like to feed another argument into this discussion on why
I like the new API. Currently the generic power domain accesses
"dev->power.irq_safe" directly.

Kind regards
Uffe

Re: [PATCH v7 1/5] PM / Runtime: Add getter for quering the IRQ safe option

[Krzysztof Kozlowski]

On śro, 2014-09-24 at 19:53 +0200, Pavel Machek wrote:
> On Wed 2014-09-24 15:50:08, Krzysztof Kozlowski wrote:
> > Add a simple getter pm_runtime_is_irq_safe() for quering whether runtime
> > PM IRQ safe was set or not.
> > 
> > Various bus drivers implementing runtime PM may use choose to suspend
> > differently based on IRQ safeness status of child driver (e.g. do not
> > unprepare the clock if IRQ safe is not set).
> > 
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> 
> Are you sure this is good interface?
> 
> "Tell me if another function works this or that way".
> 
> That's certainly not traditional interface, and it seems dangerous to
> me. Callbacks now have different semantic requirements based on value
> of some flag...

Yes... but to me that semantic change looks similar to the PM runtime
code which behaves differently when the flag is set. Some PM runtime
functions might sleep or might not... depending on the flag.

> Would it be possible to have two sets of callbacks, one irq safe and
> one not?

That would require changing the callbacks during probe of each driver.
It is possible - I'm fine with that. However we still would need some
kind of interface for selecting the callbacks according to irq safe
status - something like pm_runtime_is_irq_safe():

static int amba_probe(struct device *dev)
{
...
	if (pm_runtime_is_irq_safe())
		dev->driver->pm = &amba_pm_irq_safe;
	else
		dev->driver->pm = &amba_pm;

Best regards,
Krzysztof

[PATCH v7 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>
---
 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 fdd7e1b61f60..bb352d0bd737 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -92,6 +92,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 void amba_pclk_prepare(struct amba_device *dev)
+{
+	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 v7 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 after calling its probe function and
store it. During runtime suspend/resume deal with clocks according to
stored value.

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

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 3cf61a127ee5..e8fd5706954f 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -94,8 +94,18 @@ 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) {
+		/*
+		 * Drivers should not change pm_runtime_irq_safe()
+		 * after probe.
+		 */
+		WARN_ON(pcdev->irq_safe != pm_runtime_is_irq_safe(dev));
+
+		if (pcdev->irq_safe)
+			clk_disable(pcdev->pclk);
+		else
+			clk_disable_unprepare(pcdev->pclk);
+	}
 
 	return ret;
 }
@@ -106,7 +116,16 @@ static int amba_pm_runtime_resume(struct device *dev)
 	int ret;
 
 	if (dev->driver) {
-		ret = clk_prepare_enable(pcdev->pclk);
+		/*
+		 * Drivers should not change pm_runtime_irq_safe()
+		 * after probe.
+		 */
+		WARN_ON(pcdev->irq_safe != pm_runtime_is_irq_safe(dev));
+
+		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;
@@ -191,8 +210,10 @@ static int amba_probe(struct device *dev)
 		pm_runtime_enable(dev);
 
 		ret = pcdrv->probe(pcdev, id);
-		if (ret == 0)
+		if (ret == 0) {
+			pcdev->irq_safe = pm_runtime_is_irq_safe(dev);
 			break;
+		}
 
 		pm_runtime_disable(dev);
 		pm_runtime_set_suspended(dev);
diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index bb352d0bd737..fd0c0bfa1d38 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 v7 3/5] amba: Don't unprepare the clocks if device driver wants IRQ safe runtime PM

[Ulf Hansson]

On 24 September 2014 15:50, 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 after calling its probe function and
> store it. During runtime suspend/resume deal with clocks according to
> stored value.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

As I stated earlier, I think the local copy of the irq_safe flag and
the including WARN_ON is a bit unnecessary. Still since Russell
insisted in having them, I am happy as well. :-)

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

Kind regards
Uffe

> ---
>  drivers/amba/bus.c       | 29 +++++++++++++++++++++++++----
>  include/linux/amba/bus.h |  1 +
>  2 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 3cf61a127ee5..e8fd5706954f 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -94,8 +94,18 @@ 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) {
> +               /*
> +                * Drivers should not change pm_runtime_irq_safe()
> +                * after probe.
> +                */
> +               WARN_ON(pcdev->irq_safe != pm_runtime_is_irq_safe(dev));
> +
> +               if (pcdev->irq_safe)
> +                       clk_disable(pcdev->pclk);
> +               else
> +                       clk_disable_unprepare(pcdev->pclk);
> +       }
>
>         return ret;
>  }
> @@ -106,7 +116,16 @@ static int amba_pm_runtime_resume(struct device *dev)
>         int ret;
>
>         if (dev->driver) {
> -               ret = clk_prepare_enable(pcdev->pclk);
> +               /*
> +                * Drivers should not change pm_runtime_irq_safe()
> +                * after probe.
> +                */
> +               WARN_ON(pcdev->irq_safe != pm_runtime_is_irq_safe(dev));
> +
> +               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;
> @@ -191,8 +210,10 @@ static int amba_probe(struct device *dev)
>                 pm_runtime_enable(dev);
>
>                 ret = pcdrv->probe(pcdev, id);
> -               if (ret == 0)
> +               if (ret == 0) {
> +                       pcdev->irq_safe = pm_runtime_is_irq_safe(dev);
>                         break;
> +               }
>
>                 pm_runtime_disable(dev);
>                 pm_runtime_set_suspended(dev);
> diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
> index bb352d0bd737..fd0c0bfa1d38 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
>

[PATCH v7 4/5] dma: 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>
---
 drivers/dma/pl330.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 90 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 4839bfa74a10..b123431e62ed 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,41 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan,
 	return 0;
 }
 
+/*
+ * Assume 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 +2815,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 +2847,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 +2887,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 v7 4/5] dma: pl330: add Power Management support

[Ulf Hansson]

On 24 September 2014 15:50, Krzysztof Kozlowski <k.kozlowski@samsung.com> 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.
>
> 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>

Kind regards
Uffe

> ---
>  drivers/dma/pl330.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 90 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 4839bfa74a10..b123431e62ed 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,41 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan,
>         return 0;
>  }
>
> +/*
> + * Assume 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 +2815,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 +2847,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 +2887,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 v7 4/5] dma: pl330: add Power Management support

[Krzysztof Kozlowski]

On śro, 2014-10-15 at 09:19 +0200, Ulf Hansson wrote:
> On 24 September 2014 15:50, Krzysztof Kozlowski <k.kozlowski@samsung.com> 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.
> >
> > 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>

Thanks for review!

You pointed one issue with patch 2/5 off-the-list. I'll wait for merge
window to end, fix it and re-send.

Best regards,
Krzysztof


> 
> Kind regards
> Uffe
> 
> > ---
> >  drivers/dma/pl330.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 90 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> > index 4839bfa74a10..b123431e62ed 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,41 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan,
> >         return 0;
> >  }
> >
> > +/*
> > + * Assume 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 +2815,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 +2847,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 +2887,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
> >

[PATCH v7 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>
---
 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 fd0c0bfa1d38..275e4a346de7 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -87,12 +87,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 void amba_pclk_prepare(struct amba_device *dev)
 {
 	clk_prepare(dev->pclk);
-- 
1.9.1

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

[Ulf Hansson]

On 24 September 2014 15:50, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote:
> 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>

Kind regards
Uffe

> ---
>  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 fd0c0bfa1d38..275e4a346de7 100644
> --- a/include/linux/amba/bus.h
> +++ b/include/linux/amba/bus.h
> @@ -87,12 +87,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 void amba_pclk_prepare(struct amba_device *dev)
>  {
>         clk_prepare(dev->pclk);
> --
> 1.9.1
>