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

[Krzysztof Kozlowski <k.kozlowski@samsung.com>]

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
>