"G, Manjunath Kondaiah" <manjugk@xxxxxx> writes: > 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. Maybe so, but that's not a requirement of the API. I have a DMA test driver that doesn't do that. It's also legitimate (and IMO, expected) for a client driver to, for example do a omap_dma_request() on module load and omap_free_dma() on module unload and only use omap_start_dma() + callbacks for xfers. It would be nice (and IMO, expected) that the channels would go idle between xfers (using the autosuspend feature for timeouts.) > 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. Of course, you always have to do get/put around any device access. >> 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. Right, but my point above is: what if the user does not call free_dma? What if the client will be using the channel again sometime in the future, but will be idle. What I would expect is that the channel could go idle until another xfer is initiated rather than waiting for the channel to be freed. > Also, dma driver is not aware of callback completion status since it will be > executed in client driver. Why not? DMA driver knows when the callback returns. Kevin >> '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
