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

[Peter Ujfalusi <peter.ujfalusi@ti.com>]

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