OMAP: PM: DMA: Enable runtime pm

From: Manjunath G Kondaiah <[email protected]>

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 <[email protected]>

---
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(-)

arch/arm/plat-omap/dma.c +5 -1

index c4b2b47..48ee292 100644
@@ -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;

arch/arm/plat-omap/dma.c +3 -0

 	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;

arch/arm/plat-omap/dma.c +0 -0

 	return 0;
 
 exit_dma_irq_fail:

Quoted 10 lines · Manjunath Kondaiah

From: Manjunath G Kondaiah <[email protected]>

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?

Quoted 2 lines

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

Any OMAP2 or OMAP3 testing?

Quoted 57 lines

Signed-off-by G, Manjunath Kondaiah <[email protected]>

---
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(-)

arch/arm/plat-omap/dma.c +5 -1

index c4b2b47..48ee292 100644
@@ -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;

arch/arm/plat-omap/dma.c static int __devinit omap_system_dma_probe(struct platform_device *pdev) · line 1986

Quoted 4 lines

arch/arm/plat-omap/dma.c +1 -0

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

Quoted 21 lines

arch/arm/plat-omap/dma.c +7 -0

 	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.

arch/arm/plat-omap/dma.c static int __devinit omap_system_dma_probe(struct platform_device *pdev) · line 2083

Quoted 4 lines

arch/arm/plat-omap/dma.c +4 -0

+	 */
+	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,

Quoted 13 lines · Kevin Hilman

Quoted 10 lines · Manjunath Kondaiah

From: Manjunath G Kondaiah <[email protected]>

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.

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 1 line

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.

Quoted 93 lines

'autosuspend' feature with a timeout so that back-to-back DMA transfers will not have have additional latency between transfers?

Quoted 2 lines

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

Any OMAP2 or OMAP3 testing?

Quoted 57 lines

Signed-off-by G, Manjunath Kondaiah <[email protected]>

---
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(-)

arch/arm/plat-omap/dma.c +5 -1

index c4b2b47..48ee292 100644
@@ -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;

arch/arm/plat-omap/dma.c static int __devinit omap_system_dma_probe(struct platform_device *pdev) · line 1986

Quoted 4 lines

arch/arm/plat-omap/dma.c +1 -0

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

Quoted 21 lines

arch/arm/plat-omap/dma.c +7 -0

 	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.

Quoted 8 lines

arch/arm/plat-omap/dma.c static int __devinit omap_system_dma_probe(struct platform_device *pdev) · line 2083

Quoted 4 lines

arch/arm/plat-omap/dma.c +4 -0

+	 */
+	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

Quoted 16 lines · Manjunath Kondaiah

Kevin,

Quoted 13 lines · Kevin Hilman

Quoted 10 lines · Manjunath Kondaiah

From: Manjunath G Kondaiah <[email protected]>

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.

Quoted 7 lines

Quoted 4 lines

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

Quoted 11 lines

I can update this info in patch description if it is useful.

Quoted 5 lines

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.

Quoted 2 lines

Quoted 1 line

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

Quoted 2 lines

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.

Quoted 3 lines

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.