[RFC PATCH v2 0/2] amba/dma: pl330: add Power Management support

[RFC PATCH v2 0/2] amba/dma: pl330: add Power Management support

[Krzysztof Kozlowski]

Hi,


This is second request for comments for adding runtime PM to the pl330
driver.

The runtime PM of pl330 driver requires interrupt safe suspend/resume
callbacks which is in conflict with 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 1/2.

Any comments are welcome.


TODO:
=====
 - Document somewhere the suspend/resume callbacks behavior between
   amba bus driver and device driver.

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().


Best regards,
Krzysztof Kozlowski

Krzysztof Kozlowski (2):
  amba: Allow AMBA drivers to use their own runtime PM
  dma: pl330: add Power Management support

 drivers/amba/bus.c  | 15 +++++++++++-
 drivers/dma/pl330.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 79 insertions(+), 5 deletions(-)

-- 
1.9.1

[RFC PATCH v2 1/2] amba: Allow AMBA drivers to use their own 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()). If such driver
implements its own runtime PM functions then assume it will handle the
runtime PM completely and it will replace our clock handling.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/amba/bus.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 3cf61a127ee5..38461430b7cf 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -94,9 +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)
+	if (ret == 0 && dev->driver) {
+		/*
+		 * If driver handles runtime suspend don't duplicate
+		 * the work here.
+		 */
+		if (dev->driver->pm && dev->driver->pm->runtime_suspend)
+			goto out;
+
 		clk_disable_unprepare(pcdev->pclk);
+	}
 
+out:
 	return ret;
 }
 
@@ -106,12 +115,16 @@ static int amba_pm_runtime_resume(struct device *dev)
 	int ret;
 
 	if (dev->driver) {
+		if (dev->driver->pm && dev->driver->pm->runtime_resume)
+			goto out;
+
 		ret = clk_prepare_enable(pcdev->pclk);
 		/* Failure is probably fatal to the system, but... */
 		if (ret)
 			return ret;
 	}
 
+out:
 	return pm_generic_runtime_resume(dev);
 }
 #endif
-- 
1.9.1

Re: [RFC PATCH v2 1/2] amba: Allow AMBA drivers to use their own runtime PM

[Russell King - ARM Linux]

On Fri, Sep 12, 2014 at 04:56:18PM +0200, 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()). If such driver
> implements its own runtime PM functions then assume it will handle the
> runtime PM completely and it will replace our clock handling.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Actually, I'd rather just revert 5303c0f46c8708fff4148ebcc491f78710356952
which is clearly the wrong thing to do when we have non-IRQ safe runtime
PM.

What we /could/ do instead is to check whether irq_safe is set after
probe, record that, and then select whether to use the prepare/unprepare
methods based on that.  (Drivers should never dynamically change this.)

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

Re: [RFC PATCH v2 1/2] amba: Allow AMBA drivers to use their own runtime PM

[Krzysztof Kozlowski]

On pią, 2014-09-12 at 16:13 +0100, Russell King - ARM Linux wrote:
> On Fri, Sep 12, 2014 at 04:56:18PM +0200, 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()). If such driver
> > implements its own runtime PM functions then assume it will handle the
> > runtime PM completely and it will replace our clock handling.
> > 
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> 
> Actually, I'd rather just revert 5303c0f46c8708fff4148ebcc491f78710356952
> which is clearly the wrong thing to do when we have non-IRQ safe runtime
> PM.
> 
> What we /could/ do instead is to check whether irq_safe is set after
> probe, record that, and then select whether to use the prepare/unprepare
> methods based on that.  (Drivers should never dynamically change this.)

I'll try this approach.

Thank you for feedback,
Krzysztof

[RFC PATCH v2 2/2] 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 W-AFTR low power mode on Exynos3250.

The driver uses different runtime PM functions than amba bus driver
(drivers/amba/bus.c). Resuming the device may happen in atomic context
(during call device_issue_pending()) so pm_runtime_irq_safe() is used.
During suspend and resume the AMBA bus clock is only disabled/enabled
(not unprepared/prepared) but this is sufficient for gating the clock
and for reducing energy usage.

Suggested-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/dma/pl330.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 65 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 4839bfa74a10..8868d8be72fd 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
@@ -1958,6 +1959,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 +1974,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 +1999,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 +2019,10 @@ static void pl330_tasklet(unsigned long data)
 		}
 	}
 	spin_unlock_irqrestore(&pch->lock, flags);
+
+	/* If work list empty, power down */
+	if (power_down)
+		pm_runtime_put(pch->dmac->ddma.dev);
 }
 
 bool pl330_filter(struct dma_chan *chan, void *param)
@@ -2073,6 +2092,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 +2119,14 @@ 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_put(pl330->ddma.dev);
 		break;
 	case DMA_SLAVE_CONFIG:
 		slave_config = (struct dma_slave_config *)arg;
@@ -2138,6 +2162,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 +2172,7 @@ 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_put(pch->dmac->ddma.dev);
 }
 
 static enum dma_status
@@ -2162,6 +2188,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 +2620,26 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan,
 	return 0;
 }
 
+static int __maybe_unused pl330_suspend(struct device *dev)
+{
+	struct amba_device *pcdev = to_amba_device(dev);
+
+	if (!pm_runtime_suspended(dev))
+		amba_pclk_disable(pcdev);
+	return 0;
+}
+
+static int __maybe_unused pl330_resume(struct device *dev)
+{
+	struct amba_device *pcdev = to_amba_device(dev);
+
+	if (!pm_runtime_suspended(dev))
+		return amba_pclk_enable(pcdev);
+	return 0;
+}
+
+static UNIVERSAL_DEV_PM_OPS(pl330_pm, pl330_suspend, pl330_resume, NULL);
+
 static int
 pl330_probe(struct amba_device *adev, const struct amba_id *id)
 {
@@ -2738,6 +2793,9 @@ 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_put_noidle(&adev->dev);
+
 	return 0;
 probe_err3:
 	/* Idle the DMAC */
@@ -2764,6 +2822,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 +2862,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