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

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

[G, Manjunath Kondaiah]

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.

Boot tested on OMAP4 blaze and all applicable tests are executed
along with dma hwmod series.

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;
 	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);
+
 	return 0;
 
 exit_dma_irq_fail:
-- 
1.7.1

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

[G, Manjunath Kondaiah]

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.

Boot tested on OMAP4 blaze and all applicable tests are executed
along with dma hwmod series.

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 at vger.kernel.org/msg37819.html
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg38355.html
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg38391.html
http://www.mail-archive.com/linux-omap at 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;
 	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);
+
 	return 0;
 
 exit_dma_irq_fail:
-- 
1.7.1

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

[Kevin Hilman]

"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.

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 and
the 'put' when the callback has finished, possibly using the
'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.

Kevin

>  	return 0;
>  
>  exit_dma_irq_fail:

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

[Kevin Hilman]

"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.

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 and
the 'put' when the callback has finished, possibly using the
'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 at vger.kernel.org/msg37819.html
> http://www.mail-archive.com/linux-omap at vger.kernel.org/msg38355.html
> http://www.mail-archive.com/linux-omap at vger.kernel.org/msg38391.html
> http://www.mail-archive.com/linux-omap at 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.

Kevin

>  	return 0;
>  
>  exit_dma_irq_fail:

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

[G, Manjunath Kondaiah]

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

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

[G, Manjunath Kondaiah]

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 at vger.kernel.org/msg37819.html
> > http://www.mail-archive.com/linux-omap at vger.kernel.org/msg38355.html
> > http://www.mail-archive.com/linux-omap at vger.kernel.org/msg38391.html
> > http://www.mail-archive.com/linux-omap at 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

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

[Kevin Hilman]

"G, Manjunath Kondaiah" <manjugk@ti.com> writes:

> 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. 

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@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

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

[Kevin Hilman]

"G, Manjunath Kondaiah" <manjugk@ti.com> writes:

> 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. 

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@ti.com>
>> > ---
>> > Discussion and alignment for using runtime API's in DMA can be accessed at:
>> > http://www.mail-archive.com/linux-omap at vger.kernel.org/msg37819.html
>> > http://www.mail-archive.com/linux-omap at vger.kernel.org/msg38355.html
>> > http://www.mail-archive.com/linux-omap at vger.kernel.org/msg38391.html
>> > http://www.mail-archive.com/linux-omap at 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

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

[G, Manjunath Kondaiah]

On Mon, Jan 24, 2011 at 04:26:01PM -0800, Kevin Hilman wrote:
> "G, Manjunath Kondaiah" <manjugk@ti.com> writes:
> 
> > 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. 
> 
> 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 use autosuspend feature which can handle if there is no free_dma and also
if there is no callback registered by the client.

> 
> > 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.
ok.
> 
> > 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.

You are right. We can get this info from DMA interrupt handler.

-Manjunath
[...]

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

[G, Manjunath Kondaiah]

On Mon, Jan 24, 2011 at 04:26:01PM -0800, Kevin Hilman wrote:
> "G, Manjunath Kondaiah" <manjugk@ti.com> writes:
> 
> > 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. 
> 
> 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 use autosuspend feature which can handle if there is no free_dma and also
if there is no callback registered by the client.

> 
> > 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.
ok.
> 
> > 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.

You are right. We can get this info from DMA interrupt handler.

-Manjunath
[...]