[RFC PATCH] dma: pl330: add Power Management support

[RFC PATCH] 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 patch is based on previous work of Bartlomiej Zolnierkiewicz
<b.zolnierkie@samsung.com>.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/dma/pl330.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 74 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 66e89f04504c..bdc6ff8ee2bf 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
@@ -1966,6 +1967,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);
 
@@ -1980,10 +1982,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;
@@ -1998,6 +2007,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);
@@ -2012,6 +2027,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)
@@ -2077,10 +2096,12 @@ static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, unsigned
 	unsigned long flags;
 	struct pl330_dmac *pl330 = pch->dmac;
 	struct dma_slave_config *slave_config;
+	bool power_down = false;
 	LIST_HEAD(list);
 
 	switch (cmd) {
 	case DMA_TERMINATE_ALL:
+		pm_runtime_get_sync(pl330->ddma.dev);
 		spin_lock_irqsave(&pch->lock, flags);
 
 		spin_lock(&pl330->lock);
@@ -2107,10 +2128,18 @@ 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))
+			power_down = true;
+
 		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);
+
+		/* If work list empty, power down */
+		if (power_down)
+			pm_runtime_put(pl330->ddma.dev);
 		break;
 	case DMA_SLAVE_CONFIG:
 		slave_config = (struct dma_slave_config *)arg;
@@ -2146,6 +2175,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);
@@ -2155,6 +2185,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
@@ -2168,11 +2199,23 @@ static void pl330_issue_pending(struct dma_chan *chan)
 {
 	struct dma_pl330_chan *pch = to_pchan(chan);
 	unsigned long flags;
+	bool power_up;
 
 	spin_lock_irqsave(&pch->lock, flags);
+	power_up = list_empty(&pch->work_list);
 	list_splice_tail_init(&pch->submitted_list, &pch->work_list);
 	spin_unlock_irqrestore(&pch->lock, flags);
 
+	if (power_up) {
+		/*
+		 * Warn on nothing pending. This may break our pm_runtime
+		 * usage counter as it is updated on work_list emptiness
+		 * status.
+		 */
+		WARN_ON(list_empty(&pch->work_list));
+		pm_runtime_get_sync(pch->dmac->ddma.dev);
+	}
+
 	pl330_tasklet((unsigned long)pch);
 }
 
@@ -2205,6 +2248,7 @@ static dma_cookie_t pl330_tx_submit(struct dma_async_tx_descriptor *tx)
 
 	cookie = dma_cookie_assign(&last->txd);
 	list_add_tail(&last->node, &pch->submitted_list);
+
 	spin_unlock_irqrestore(&pch->lock, flags);
 
 	return cookie;
@@ -2593,6 +2637,28 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan,
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int 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 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;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(pl330_pm, pl330_suspend, pl330_resume);
+
 static int
 pl330_probe(struct amba_device *adev, const struct amba_id *id)
 {
@@ -2746,6 +2812,7 @@ 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_put_noidle(&adev->dev);
 	return 0;
 probe_err3:
 	/* Idle the DMAC */
@@ -2772,6 +2839,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);
 
@@ -2810,6 +2879,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: [RFC PATCH] dma: pl330: add Power Management support

[Lars-Peter Clausen]

On 09/05/2014 05:06 PM, Krzysztof Kozlowski wrote:
[...]
> @@ -2168,11 +2199,23 @@ static void pl330_issue_pending(struct dma_chan *chan)
>   {
>   	struct dma_pl330_chan *pch = to_pchan(chan);
>   	unsigned long flags;
> +	bool power_up;
>
>   	spin_lock_irqsave(&pch->lock, flags);
> +	power_up = list_empty(&pch->work_list);
>   	list_splice_tail_init(&pch->submitted_list, &pch->work_list);
>   	spin_unlock_irqrestore(&pch->lock, flags);
>
> +	if (power_up) {
> +		/*
> +		 * Warn on nothing pending. This may break our pm_runtime
> +		 * usage counter as it is updated on work_list emptiness
> +		 * status.
> +		 */
> +		WARN_ON(list_empty(&pch->work_list));
> +		pm_runtime_get_sync(pch->dmac->ddma.dev);

Unfortunately this does not work. pm_runtime_get_sync() may sleep, where as 
the issue_pending callback needs to be able to run from contexts in which 
sleeping is not possible.

> +	}
> +
>   	pl330_tasklet((unsigned long)pch);
>   }
>
[...]

Re: [RFC PATCH] dma: pl330: add Power Management support

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 6591 bytes --]

On 09/05/2014 05:06 PM, 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 W-AFTR low power mode on Exynos3250.
> 
> The patch is based on previous work of Bartlomiej Zolnierkiewicz
> <b.zolnierkie@samsung.com>.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> ---
>  drivers/dma/pl330.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 74 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 66e89f04504c..bdc6ff8ee2bf 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
> @@ -1966,6 +1967,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);
>  
> @@ -1980,10 +1982,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;
> @@ -1998,6 +2007,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);
> @@ -2012,6 +2027,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)
> @@ -2077,10 +2096,12 @@ static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, unsigned
>  	unsigned long flags;
>  	struct pl330_dmac *pl330 = pch->dmac;
>  	struct dma_slave_config *slave_config;
> +	bool power_down = false;
>  	LIST_HEAD(list);
>  
>  	switch (cmd) {
>  	case DMA_TERMINATE_ALL:
> +		pm_runtime_get_sync(pl330->ddma.dev);
>  		spin_lock_irqsave(&pch->lock, flags);
>  
>  		spin_lock(&pl330->lock);
> @@ -2107,10 +2128,18 @@ 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))
> +			power_down = true;
> +
>  		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);
> +
> +		/* If work list empty, power down */
> +		if (power_down)
> +			pm_runtime_put(pl330->ddma.dev);
>  		break;
>  	case DMA_SLAVE_CONFIG:
>  		slave_config = (struct dma_slave_config *)arg;
> @@ -2146,6 +2175,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);
> @@ -2155,6 +2185,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
> @@ -2168,11 +2199,23 @@ static void pl330_issue_pending(struct dma_chan *chan)
>  {
>  	struct dma_pl330_chan *pch = to_pchan(chan);
>  	unsigned long flags;
> +	bool power_up;
>  
>  	spin_lock_irqsave(&pch->lock, flags);
> +	power_up = list_empty(&pch->work_list);
>  	list_splice_tail_init(&pch->submitted_list, &pch->work_list);
>  	spin_unlock_irqrestore(&pch->lock, flags);
>  
> +	if (power_up) {
> +		/*
> +		 * Warn on nothing pending. This may break our pm_runtime
> +		 * usage counter as it is updated on work_list emptiness
> +		 * status.
> +		 */
> +		WARN_ON(list_empty(&pch->work_list));
> +		pm_runtime_get_sync(pch->dmac->ddma.dev);
> +	}
> +
>  	pl330_tasklet((unsigned long)pch);
>  }
>  
> @@ -2205,6 +2248,7 @@ static dma_cookie_t pl330_tx_submit(struct dma_async_tx_descriptor *tx)
>  
>  	cookie = dma_cookie_assign(&last->txd);
>  	list_add_tail(&last->node, &pch->submitted_list);
> +

unrelated change.

>  	spin_unlock_irqrestore(&pch->lock, flags);
>  
>  	return cookie;
> @@ -2593,6 +2637,28 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan,
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM_SLEEP

remove this line

> +static int pl330_suspend(struct device *dev)

__maybe_unused here

> +{
> +	struct amba_device *pcdev = to_amba_device(dev);
> +
> +	if (!pm_runtime_suspended(dev))
> +		amba_pclk_disable(pcdev);
> +	return 0;
> +}
> +
> +static int pl330_resume(struct device *dev)

__maybe_unused here

> +{
> +	struct amba_device *pcdev = to_amba_device(dev);
> +
> +	if (!pm_runtime_suspended(dev))
> +		return amba_pclk_enable(pcdev);
> +	return 0;
> +}
> +#endif

remove this line

> +
> +static SIMPLE_DEV_PM_OPS(pl330_pm, pl330_suspend, pl330_resume);

Everything is handle inside this macro and your functions are not calling
anything what it is not defined for both cases.

Thanks,
Michal


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [RFC PATCH] dma: pl330: add Power Management support

[Krzysztof Kozlowski]

On pią, 2014-09-05 at 17:23 +0200, Lars-Peter Clausen wrote:
> On 09/05/2014 05:06 PM, Krzysztof Kozlowski wrote:
> [...]
> > @@ -2168,11 +2199,23 @@ static void pl330_issue_pending(struct dma_chan *chan)
> >   {
> >   	struct dma_pl330_chan *pch = to_pchan(chan);
> >   	unsigned long flags;
> > +	bool power_up;
> >
> >   	spin_lock_irqsave(&pch->lock, flags);
> > +	power_up = list_empty(&pch->work_list);
> >   	list_splice_tail_init(&pch->submitted_list, &pch->work_list);
> >   	spin_unlock_irqrestore(&pch->lock, flags);
> >
> > +	if (power_up) {
> > +		/*
> > +		 * Warn on nothing pending. This may break our pm_runtime
> > +		 * usage counter as it is updated on work_list emptiness
> > +		 * status.
> > +		 */
> > +		WARN_ON(list_empty(&pch->work_list));
> > +		pm_runtime_get_sync(pch->dmac->ddma.dev);
> 
> Unfortunately this does not work. pm_runtime_get_sync() may sleep, where as 
> the issue_pending callback needs to be able to run from contexts in which 
> sleeping is not possible.

Thank you for pointing this out. It seems I'll have to find another
solution because I really need to runtime suspend this driver...

Best regards,
Krzysztof

> 
> > +	}
> > +
> >   	pl330_tasklet((unsigned long)pch);
> >   }
> >
> [...]

Re: [RFC PATCH] dma: pl330: add Power Management support

[Krzysztof Kozlowski]

On pon, 2014-09-08 at 10:06 +0200, Michal Simek wrote:
> On 09/05/2014 05:06 PM, 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 W-AFTR low power mode on Exynos3250.
> > 
> > The patch is based on previous work of Bartlomiej Zolnierkiewicz
> > <b.zolnierkie@samsung.com>.
> > 
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > ---
> >  drivers/dma/pl330.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 74 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> > index 66e89f04504c..bdc6ff8ee2bf 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
> > @@ -1966,6 +1967,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);
> >  
> > @@ -1980,10 +1982,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;
> > @@ -1998,6 +2007,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);
> > @@ -2012,6 +2027,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)
> > @@ -2077,10 +2096,12 @@ static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, unsigned
> >  	unsigned long flags;
> >  	struct pl330_dmac *pl330 = pch->dmac;
> >  	struct dma_slave_config *slave_config;
> > +	bool power_down = false;
> >  	LIST_HEAD(list);
> >  
> >  	switch (cmd) {
> >  	case DMA_TERMINATE_ALL:
> > +		pm_runtime_get_sync(pl330->ddma.dev);
> >  		spin_lock_irqsave(&pch->lock, flags);
> >  
> >  		spin_lock(&pl330->lock);
> > @@ -2107,10 +2128,18 @@ 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))
> > +			power_down = true;
> > +
> >  		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);
> > +
> > +		/* If work list empty, power down */
> > +		if (power_down)
> > +			pm_runtime_put(pl330->ddma.dev);
> >  		break;
> >  	case DMA_SLAVE_CONFIG:
> >  		slave_config = (struct dma_slave_config *)arg;
> > @@ -2146,6 +2175,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);
> > @@ -2155,6 +2185,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
> > @@ -2168,11 +2199,23 @@ static void pl330_issue_pending(struct dma_chan *chan)
> >  {
> >  	struct dma_pl330_chan *pch = to_pchan(chan);
> >  	unsigned long flags;
> > +	bool power_up;
> >  
> >  	spin_lock_irqsave(&pch->lock, flags);
> > +	power_up = list_empty(&pch->work_list);
> >  	list_splice_tail_init(&pch->submitted_list, &pch->work_list);
> >  	spin_unlock_irqrestore(&pch->lock, flags);
> >  
> > +	if (power_up) {
> > +		/*
> > +		 * Warn on nothing pending. This may break our pm_runtime
> > +		 * usage counter as it is updated on work_list emptiness
> > +		 * status.
> > +		 */
> > +		WARN_ON(list_empty(&pch->work_list));
> > +		pm_runtime_get_sync(pch->dmac->ddma.dev);
> > +	}
> > +
> >  	pl330_tasklet((unsigned long)pch);
> >  }
> >  
> > @@ -2205,6 +2248,7 @@ static dma_cookie_t pl330_tx_submit(struct dma_async_tx_descriptor *tx)
> >  
> >  	cookie = dma_cookie_assign(&last->txd);
> >  	list_add_tail(&last->node, &pch->submitted_list);
> > +
> 
> unrelated change.

Right.

> 
> >  	spin_unlock_irqrestore(&pch->lock, flags);
> >  
> >  	return cookie;
> > @@ -2593,6 +2637,28 @@ static int pl330_dma_device_slave_caps(struct dma_chan *dchan,
> >  	return 0;
> >  }
> >  
> > +#ifdef CONFIG_PM_SLEEP
> 
> remove this line
> 
> > +static int pl330_suspend(struct device *dev)
> 
> __maybe_unused here
> 
> > +{
> > +	struct amba_device *pcdev = to_amba_device(dev);
> > +
> > +	if (!pm_runtime_suspended(dev))
> > +		amba_pclk_disable(pcdev);
> > +	return 0;
> > +}
> > +
> > +static int pl330_resume(struct device *dev)
> 
> __maybe_unused here
> 
> > +{
> > +	struct amba_device *pcdev = to_amba_device(dev);
> > +
> > +	if (!pm_runtime_suspended(dev))
> > +		return amba_pclk_enable(pcdev);
> > +	return 0;
> > +}
> > +#endif
> 
> remove this line
> 
> > +
> > +static SIMPLE_DEV_PM_OPS(pl330_pm, pl330_suspend, pl330_resume);
> 
> Everything is handle inside this macro and your functions are not calling
> anything what it is not defined for both cases.

Thank you for feedback. I'll fix this in next version.

Best regards,
Krzysztof


> 
> Thanks,
> Michal
> 

Re: [RFC PATCH] dma: pl330: add Power Management support

[Krzysztof Kozlowski]

On pon, 2014-09-08 at 10:11 +0200, Krzysztof Kozlowski wrote:
> On pią, 2014-09-05 at 17:23 +0200, Lars-Peter Clausen wrote:
> > On 09/05/2014 05:06 PM, Krzysztof Kozlowski wrote:
> > [...]
> > > @@ -2168,11 +2199,23 @@ static void pl330_issue_pending(struct dma_chan *chan)
> > >   {
> > >   	struct dma_pl330_chan *pch = to_pchan(chan);
> > >   	unsigned long flags;
> > > +	bool power_up;
> > >
> > >   	spin_lock_irqsave(&pch->lock, flags);
> > > +	power_up = list_empty(&pch->work_list);
> > >   	list_splice_tail_init(&pch->submitted_list, &pch->work_list);
> > >   	spin_unlock_irqrestore(&pch->lock, flags);
> > >
> > > +	if (power_up) {
> > > +		/*
> > > +		 * Warn on nothing pending. This may break our pm_runtime
> > > +		 * usage counter as it is updated on work_list emptiness
> > > +		 * status.
> > > +		 */
> > > +		WARN_ON(list_empty(&pch->work_list));
> > > +		pm_runtime_get_sync(pch->dmac->ddma.dev);
> > 
> > Unfortunately this does not work. pm_runtime_get_sync() may sleep, where as 
> > the issue_pending callback needs to be able to run from contexts in which 
> > sleeping is not possible.
> 
> Thank you for pointing this out. It seems I'll have to find another
> solution because I really need to runtime suspend this driver...

I found that idea was wrong in few more places. The most important is
that the pl330 is an AMBA device. This means that runtime suspend/resume
callbacks are already implemented (drivers/amba/bus.c) and they are
called instead of my callbacks.

That would be fine... except that these callbacks in amba/bus.c call
clk_preare()/clk_unprepare() which is not safe for atomic context. This
prohibits me from using pm_runtime_irq_safe() which could solve problem
pointed by you. 

Best regards,
Krzysztof


> 
> Best regards,
> Krzysztof
> 
> > 
> > > +	}
> > > +
> > >   	pl330_tasklet((unsigned long)pch);
> > >   }
> > >
> > [...]