Re: [PATCH] OMAP: PM: DMA: Enable runtime pm

["G, Manjunath Kondaiah" <manjugk@ti.com>]

Kevin,

On Mon, Jan 24, 2011 at 01:43:31PM -0800, Kevin Hilman wrote:
> "G, Manjunath Kondaiah" <manjugk@ti.com> writes:
> 
> > From: Manjunath G Kondaiah <manjugk@ti.com>
> >
> > Enable runtime pm and use pm_runtime_get_sync and pm_runtime_put
> > for OMAP DMA driver.
> >
> > Since DMA driver callback will happen from interrupt context and
> > DMA client driver will release all DMA resources from interrupt
> > context itself, pm_runtime_put_sync() cannot be used in DMA driver.
> > Instead, pm_runtime_put() is used which is asynchronous call and
> > gets executed in work queue.
> 
> It's fine that the asynchronous version of put is uses (it's actually
> preferred.)  However, the description is confusing here.  You talk about
> driver callbacks here but in the patch, your calling _put() from
> omap_dma_free(), not from the callback.

All dma client drivers are calling omap_dma_free from callback context. I can
update this info in patch description if it is useful.

> 
> You're also calling _get() from the request.  That means, as long as the
> DMA channel is allocated, it will be active.   
> 
> Wouldn't it be better to do the 'get' when the channel is started

No. omap_dma_request will call omap_clear_dma which in turn access all channel
specific registers for writing zeros.

> and the 'put' when the callback has finished, possibly using the

after omap_free_dma, none of the dma registers are accessed hence it is safe to
use _put immediately after free_dma.
Also, dma driver is not aware of callback completion status since it will be
executed in client driver.

> 'autosuspend' feature with a timeout so that back-to-back DMA transfers
> will not have have additional latency between transfers?
> 
> > Boot tested on OMAP4 blaze and all applicable tests are executed
> > along with dma hwmod series.
> 
> Any OMAP2 or OMAP3 testing?
> 
> > Signed-off-by: G, Manjunath Kondaiah <manjugk@ti.com>
> > ---
> > Discussion and alignment for using runtime API's in DMA can be accessed at:
> > http://www.mail-archive.com/linux-omap@vger.kernel.org/msg37819.html
> > http://www.mail-archive.com/linux-omap@vger.kernel.org/msg38355.html
> > http://www.mail-archive.com/linux-omap@vger.kernel.org/msg38391.html
> > http://www.mail-archive.com/linux-omap@vger.kernel.org/msg38400.html
> >
> >  arch/arm/plat-omap/dma.c |   18 +++++++++++++++++-
> >  1 files changed, 17 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/arm/plat-omap/dma.c b/arch/arm/plat-omap/dma.c
> > index c4b2b47..48ee292 100644
> > --- a/arch/arm/plat-omap/dma.c
> > +++ b/arch/arm/plat-omap/dma.c
> > @@ -35,6 +35,7 @@
> >  #include <linux/io.h>
> >  #include <linux/slab.h>
> >  #include <linux/delay.h>
> > +#include <linux/pm_runtime.h>
> >  
> >  #include <asm/system.h>
> >  #include <mach/hardware.h>
> > @@ -58,6 +59,7 @@ enum { DMA_CHAIN_STARTED, DMA_CHAIN_NOTSTARTED };
> >  #define OMAP_FUNC_MUX_ARM_BASE		(0xfffe1000 + 0xec)
> >  
> >  static struct omap_system_dma_plat_info *p;
> > +static struct platform_device           *pd;
> >  static struct omap_dma_dev_attr *d;
> >  
> >  static int enable_1510_mode;
> > @@ -676,6 +678,7 @@ int omap_request_dma(int dev_id, const char *dev_name,
> >  	unsigned long flags;
> >  	struct omap_dma_lch *chan;
> >  
> > +	pm_runtime_get_sync(&pd->dev);
> >  	spin_lock_irqsave(&dma_chan_lock, flags);
> >  	for (ch = 0; ch < dma_chan_count; ch++) {
> >  		if (free_ch == -1 && dma_chan[ch].dev_id == -1) {
> > @@ -686,6 +689,7 @@ int omap_request_dma(int dev_id, const char *dev_name,
> >  	}
> >  	if (free_ch == -1) {
> >  		spin_unlock_irqrestore(&dma_chan_lock, flags);
> > +		pm_runtime_put(&pd->dev);
> >  		return -EBUSY;
> >  	}
> >  	chan = dma_chan + free_ch;
> > @@ -779,7 +783,7 @@ void omap_free_dma(int lch)
> >  		p->dma_write(0, CCR, lch);
> >  		omap_clear_dma(lch);
> >  	}
> > -
> > +	pm_runtime_put(&pd->dev);
> >  	spin_lock_irqsave(&dma_chan_lock, flags);
> >  	dma_chan[lch].dev_id = -1;
> >  	dma_chan[lch].next_lch = -1;
> > @@ -1979,6 +1983,7 @@ static int __devinit omap_system_dma_probe(struct platform_device *pdev)
> >  		return -EINVAL;
> >  	}
> >  
> > +	pd			= pdev;
> 
> minor: platform_device pointers are more commonly named pdev
> 
> >  	d			= p->dma_attr;
> >  	errata			= p->errata;
> >  
> > @@ -2000,6 +2005,9 @@ static int __devinit omap_system_dma_probe(struct platform_device *pdev)
> >  		}
> >  	}
> >  
> > +	pm_runtime_enable(&pd->dev);
> > +	pm_runtime_get_sync(&pd->dev);
> > +
> >  	spin_lock_init(&dma_chan_lock);
> >  	for (ch = 0; ch < dma_chan_count; ch++) {
> >  		omap_clear_dma(ch);
> > @@ -2065,6 +2073,14 @@ static int __devinit omap_system_dma_probe(struct platform_device *pdev)
> >  		dma_chan[1].dev_id = 1;
> >  	}
> >  	p->show_dma_caps();
> > +
> > +	/*
> > +	 * Note: If dma channels are reserved through boot paramters,
> > +	 * then dma device is always enabled.
> > +	 */
> > +	if (!omap_dma_reserve_channels)
> > +		pm_runtime_put(&pd->dev);
> > +
> 
> Readability would be improved if there was an unconditional
> pm_runtime_put() at the end of this function preceeded by an extra
> pm_runtime_get() (async version) if reserve_channels has been used.
ok. I will update.

-Manjunath