[PATCH] dmaengine: omap-dma: Fix cyclic suspend/resume

[PATCH] dmaengine: omap-dma: Fix cyclic suspend/resume

[Peter Ujfalusi]

When the audio stream is paused or suspended we stop the sDMA and when it
is unpsues/resumed we start the channel without reconfiguring it.
The omap_dma_stop() clears the link configuration when we pause the dma, but
it is not setting it back on start. This will result only one audio buffer
to be played back and the DMA will stop, since the linking is disabled.
The link need to be enabled in omap_dma_start() to make sure that cyclic
transfer can continue.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/dma/omap-dma.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/dma/omap-dma.c b/drivers/dma/omap-dma.c
index 4cf7d9a950d7..13a02ff87f28 100644
--- a/drivers/dma/omap-dma.c
+++ b/drivers/dma/omap-dma.c
@@ -280,6 +280,7 @@ static void omap_dma_assign(struct omap_dmadev *od, struct omap_chan *c,
 static void omap_dma_start(struct omap_chan *c, struct omap_desc *d)
 {
 	struct omap_dmadev *od = to_omap_dma_dev(c->vc.chan.device);
+	uint32_t val;
 
 	if (__dma_omap15xx(od->plat->dma_attr))
 		omap_dma_chan_write(c, CPC, 0);
@@ -288,6 +289,17 @@ static void omap_dma_start(struct omap_chan *c, struct omap_desc *d)
 
 	omap_dma_clear_csr(c);
 
+	if (!__dma_omap15xx(od->plat->dma_attr) && c->cyclic) {
+		val = omap_dma_chan_read(c, CLNK_CTRL);
+
+		if (dma_omap1())
+			val &= ~(1 << 14); /* clear the STOP_LNK bit */
+		else
+			val |= CLNK_CTRL_ENABLE_LNK;
+
+		omap_dma_chan_write(c, CLNK_CTRL, val);
+	}
+
 	/* Enable interrupts */
 	omap_dma_chan_write(c, CICR, d->cicr);
 
-- 
2.1.0

Re: [PATCH] dmaengine: omap-dma: Fix cyclic suspend/resume

[Russell King - ARM Linux]

On Tue, Sep 16, 2014 at 04:06:25PM +0300, Peter Ujfalusi wrote:
> When the audio stream is paused or suspended we stop the sDMA and when it
> is unpsues/resumed we start the channel without reconfiguring it.
> The omap_dma_stop() clears the link configuration when we pause the dma, but
> it is not setting it back on start. This will result only one audio buffer
> to be played back and the DMA will stop, since the linking is disabled.
> The link need to be enabled in omap_dma_start() to make sure that cyclic
> transfer can continue.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  drivers/dma/omap-dma.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/dma/omap-dma.c b/drivers/dma/omap-dma.c
> index 4cf7d9a950d7..13a02ff87f28 100644
> --- a/drivers/dma/omap-dma.c
> +++ b/drivers/dma/omap-dma.c
> @@ -280,6 +280,7 @@ static void omap_dma_assign(struct omap_dmadev *od, struct omap_chan *c,
>  static void omap_dma_start(struct omap_chan *c, struct omap_desc *d)
>  {
>  	struct omap_dmadev *od = to_omap_dma_dev(c->vc.chan.device);
> +	uint32_t val;
>  
>  	if (__dma_omap15xx(od->plat->dma_attr))
>  		omap_dma_chan_write(c, CPC, 0);
> @@ -288,6 +289,17 @@ static void omap_dma_start(struct omap_chan *c, struct omap_desc *d)
>  
>  	omap_dma_clear_csr(c);
>  
> +	if (!__dma_omap15xx(od->plat->dma_attr) && c->cyclic) {
> +		val = omap_dma_chan_read(c, CLNK_CTRL);
> +
> +		if (dma_omap1())
> +			val &= ~(1 << 14); /* clear the STOP_LNK bit */
> +		else
> +			val |= CLNK_CTRL_ENABLE_LNK;
> +
> +		omap_dma_chan_write(c, CLNK_CTRL, val);
> +	}
> +

Why is this soo complicated?  What's wrong with simply writing the
stored value back from the omap_desc:

	omap_dma_chan_write(c, CLNK_CTRL, d->clnk_ctrl);

In fact, rather than loading up the above fast path with stuff which
it really doesn't need, why not do this in the resume path?  The
other thing which should be placed in the resume path is a mb() call,
since calling omap_dma_start() won't have a barrier in that path.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] dmaengine: omap-dma: Fix cyclic suspend/resume

[Peter Ujfalusi]

On 09/16/2014 04:22 PM, Russell King - ARM Linux wrote:
> On Tue, Sep 16, 2014 at 04:06:25PM +0300, Peter Ujfalusi wrote:
>> When the audio stream is paused or suspended we stop the sDMA and when it
>> is unpsues/resumed we start the channel without reconfiguring it.
>> The omap_dma_stop() clears the link configuration when we pause the dma, but
>> it is not setting it back on start. This will result only one audio buffer
>> to be played back and the DMA will stop, since the linking is disabled.
>> The link need to be enabled in omap_dma_start() to make sure that cyclic
>> transfer can continue.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>>  drivers/dma/omap-dma.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/dma/omap-dma.c b/drivers/dma/omap-dma.c
>> index 4cf7d9a950d7..13a02ff87f28 100644
>> --- a/drivers/dma/omap-dma.c
>> +++ b/drivers/dma/omap-dma.c
>> @@ -280,6 +280,7 @@ static void omap_dma_assign(struct omap_dmadev *od, struct omap_chan *c,
>>  static void omap_dma_start(struct omap_chan *c, struct omap_desc *d)
>>  {
>>  	struct omap_dmadev *od = to_omap_dma_dev(c->vc.chan.device);
>> +	uint32_t val;
>>  
>>  	if (__dma_omap15xx(od->plat->dma_attr))
>>  		omap_dma_chan_write(c, CPC, 0);
>> @@ -288,6 +289,17 @@ static void omap_dma_start(struct omap_chan *c, struct omap_desc *d)
>>  
>>  	omap_dma_clear_csr(c);
>>  
>> +	if (!__dma_omap15xx(od->plat->dma_attr) && c->cyclic) {
>> +		val = omap_dma_chan_read(c, CLNK_CTRL);
>> +
>> +		if (dma_omap1())
>> +			val &= ~(1 << 14); /* clear the STOP_LNK bit */
>> +		else
>> +			val |= CLNK_CTRL_ENABLE_LNK;
>> +
>> +		omap_dma_chan_write(c, CLNK_CTRL, val);
>> +	}
>> +
> 
> Why is this soo complicated?  What's wrong with simply writing the
> stored value back from the omap_desc:
> 
> 	omap_dma_chan_write(c, CLNK_CTRL, d->clnk_ctrl);

Oh, I have overlooked this. Thanks.

> In fact, rather than loading up the above fast path with stuff which
> it really doesn't need, why not do this in the resume path?  The
> other thing which should be placed in the resume path is a mb() call,
> since calling omap_dma_start() won't have a barrier in that path.

Do you want it as a separate patch (adding the mb() and to restore the
CLNK_CTRL register) or is it OK if I send it as one patch?

-- 
Péter