Kevin, On Mon, Jan 24, 2011 at 01:43:31PM -0800, Kevin Hilman wrote: > "G, Manjunath Kondaiah" <manjugk@xxxxxx> writes: > > > From: Manjunath G Kondaiah <manjugk@xxxxxx> > > > > 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@xxxxxx> > > --- > > Discussion and alignment for using runtime API's in DMA can be accessed at: > > http://www.mail-archive.com/linux-omap@xxxxxxxxxxxxxxx/msg37819.html > > http://www.mail-archive.com/linux-omap@xxxxxxxxxxxxxxx/msg38355.html > > http://www.mail-archive.com/linux-omap@xxxxxxxxxxxxxxx/msg38391.html > > http://www.mail-archive.com/linux-omap@xxxxxxxxxxxxxxx/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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
