[PATCH v3 13/13] OMAP: PM: DMA: Enable runtime pm

[G, Manjunath Kondaiah]

> -----Original Message-----
> From: linux-omap-owner at vger.kernel.org 
> [mailto:linux-omap-owner at vger.kernel.org] On Behalf Of Kevin Hilman
> Sent: Wednesday, November 10, 2010 4:18 AM
> To: G, Manjunath Kondaiah
> Cc: linux-omap at vger.kernel.org; 
> linux-arm-kernel at lists.infradead.org; Cousson, Benoit; 
> Shilimkar, Santosh
> Subject: Re: [PATCH v3 13/13] OMAP: PM: DMA: Enable runtime pm
> 
> "G, Manjunath Kondaiah" <manjugk at ti.com> writes:
> 
> > Enable runtime pm and use pm_runtime_get and pm_runtime_put
> > for OMAP DMA driver.
> >
> > Signed-off-by: G, Manjunath Kondaiah <manjugk at ti.com>
> > Cc: Benoit Cousson <b-cousson at ti.com>
> > Cc: Kevin Hilman <khilman at deeprootsystems.com>
> > Cc: Santosh Shilimkar <santosh.shilimkar at ti.com>
> > ---
> >  arch/arm/plat-omap/dma.c |   13 +++++++++++++
> >  1 files changed, 13 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/arm/plat-omap/dma.c b/arch/arm/plat-omap/dma.c
> > index 41b14b0..feac7ee 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>
> > @@ -367,6 +368,8 @@ int omap_request_dma(int dev_id, const 
> char *dev_name,
> >  	chan = dma_chan + free_ch;
> >  	chan->dev_id = dev_id;
> >  
> > +	pm_runtime_get(&pd->dev);
> 
> The _get() call is asynchronous.  So if the device was actually
> idled/disabled, immediately after this call it may still be
> idle/disabled.   When using the asynchronous versions of the API, the
> device should not be touched until the driver's ->runtime_resume()
> callback is called.

driver runtime_resume will call omap_device_enable. How do we make 
we check from driver side, whether omap_device_enable execution is complete
or not.

> 
> >  	if (d->dev_caps & IS_WORD_16)
> >  		p->clear_lch_regs(free_ch);
> >  	else
> > @@ -452,6 +455,7 @@ void omap_free_dma(int lch)
> >  		omap_clear_dma(lch);
> >  		p->clear_dma_sglist_mode(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;
> > @@ -819,6 +823,11 @@ static int __devinit 
> omap_system_dma_probe(struct platform_device *pdev)
> >  	dma_chan_count		= d->chan_count;
> >  	dma_chan		= d->chan;
> >  
> > +	/* Enable run time PM */
> 
> comment is redundant

ok.

> 
> > +	pm_runtime_enable(&pd->dev);
> > +
> > +	/* Accessing hw registers, get clock */
> 
> comment is wrong, driver doesn't know anything about clocks.
> Just drop comment as it doesn't tell you anything that the following
> 'get' doesn't.
> 
> > +	pm_runtime_get(&pd->dev);
> 
> see above Re: async calls.
> 
> >  	for (ch = 0; ch < dma_chan_count; ch++) {
> >  		unsigned long flags;
> >  		omap_clear_dma(ch);
> > @@ -846,6 +855,10 @@ static int __devinit 
> omap_system_dma_probe(struct platform_device *pdev)
> >  		dma_chan[0].dev_id = 0;
> >  		dma_chan[1].dev_id = 1;
> >  	}
> > +
> > +	if (!omap_dma_reserve_channels)
> > +		pm_runtime_put(&pd->dev);
> > +
> 
> So if the reserve channels feature is used, PM is essentially disabled
> for sDMA.  You should add a warning here to that effect as well.

I will add warning comment.

-Manjunath