[PATCH 2/2] ASoC: omap: Stop DMA re enabling in self linkage mode

[PATCH 2/2] ASoC: omap: Stop DMA re enabling in self linkage mode

[Olaya, Margarita]

From: Hari Nagalla <hnagalla@ti.com>

While using self linking, there is a chance that the DMA
has re-enabled the channel just after disabling it.

This patch stops the OMAP4 DMA re-enabling after stoping the
DMA channel.

Signed-off-by: Hari Nagalla <hnagalla@ti.com>
Signed-off-by: Margarita Olaya Cabrera <magi.olaya@ti.com>
---
 sound/soc/omap/omap-pcm.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/sound/soc/omap/omap-pcm.c b/sound/soc/omap/omap-pcm.c
index 6a21447..afe91ad 100644
--- a/sound/soc/omap/omap-pcm.c
+++ b/sound/soc/omap/omap-pcm.c
@@ -234,6 +234,13 @@ static int omap_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 		prtd->period_index = -1;
 		omap_stop_dma(prtd->dma_ch);
+		/*
+		 * Since we are using self linking, there is a
+		 * chance that the DMA as re-enabled the channel
+		 * just after disabling it.
+		 */
+		while (omap_get_dma_active_status(prtd->dma_ch))
+			omap_stop_dma(prtd->dma_ch);
 		break;
 	default:
 		ret = -EINVAL;
-- 
1.7.0.4

Re: [PATCH 2/2] ASoC: omap: Stop DMA re enabling in self linkage mode

[Mark Brown]

On Mon, Dec 06, 2010 at 04:34:47PM -0600, Olaya, Margarita wrote:
> From: Hari Nagalla <hnagalla@ti.com>
> 
> While using self linking, there is a chance that the DMA
> has re-enabled the channel just after disabling it.
> 
> This patch stops the OMAP4 DMA re-enabling after stoping the
> DMA channel.

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Re: [PATCH 2/2] ASoC: omap: Stop DMA re enabling in self linkage mode

[Peter Ujfalusi]

On Tuesday 07 December 2010 00:34:47 ext Olaya, Margarita wrote:
> From: Hari Nagalla <hnagalla@ti.com>
> 
> While using self linking, there is a chance that the DMA
> has re-enabled the channel just after disabling it.
> 
> This patch stops the OMAP4 DMA re-enabling after stoping the
> DMA channel.

I have not seen this happening on OMAP2/3, or at least I'm not aware of it.
Is this problem OMAP4 only?
Do you know if this happens on playback, capture or in both direction?
Is it worth to do this workaround only on OMAP4?

> 
> Signed-off-by: Hari Nagalla <hnagalla@ti.com>
> Signed-off-by: Margarita Olaya Cabrera <magi.olaya@ti.com>
> ---
>  sound/soc/omap/omap-pcm.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/sound/soc/omap/omap-pcm.c b/sound/soc/omap/omap-pcm.c
> index 6a21447..afe91ad 100644
> --- a/sound/soc/omap/omap-pcm.c
> +++ b/sound/soc/omap/omap-pcm.c
> @@ -234,6 +234,13 @@ static int omap_pcm_trigger(struct snd_pcm_substream
> *substream, int cmd) case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>  		prtd->period_index = -1;
>  		omap_stop_dma(prtd->dma_ch);
> +		/*
> +		 * Since we are using self linking, there is a
> +		 * chance that the DMA as re-enabled the channel
> +		 * just after disabling it.
> +		 */
> +		while (omap_get_dma_active_status(prtd->dma_ch))
> +			omap_stop_dma(prtd->dma_ch);
>  		break;
>  	default:
>  		ret = -EINVAL;

-- 
Péter

Re: [PATCH 2/2] ASoC: omap: Stop DMA re enabling in self linkage mode

[Mark Brown]

On Tue, Dec 07, 2010 at 11:17:26AM +0200, Peter Ujfalusi wrote:

> I have not seen this happening on OMAP2/3, or at least I'm not aware of it.
> Is this problem OMAP4 only?
> Do you know if this happens on playback, capture or in both direction?
> Is it worth to do this workaround only on OMAP4?

Given that the patch should cause at most a single read from a register
when tearing down DMA if it's not required I'd guess it's safer to just
unconditionally enable the workaround on the off chance that it's just
reallly rare rather than not needed?

Re: [PATCH 2/2] ASoC: omap: Stop DMA re enabling in self linkage mode

[Peter Ujfalusi]

On Tuesday 07 December 2010 16:05:12 ext Mark Brown wrote:
> On Tue, Dec 07, 2010 at 11:17:26AM +0200, Peter Ujfalusi wrote:
> > I have not seen this happening on OMAP2/3, or at least I'm not aware of
> > it. Is this problem OMAP4 only?
> > Do you know if this happens on playback, capture or in both direction?
> > Is it worth to do this workaround only on OMAP4?
> 
> Given that the patch should cause at most a single read from a register
> when tearing down DMA if it's not required I'd guess it's safer to just
> unconditionally enable the workaround on the off chance that it's just
> reallly rare rather than not needed?

Sure, it is a single read to a register, but generally I'm a bit nervous, when 
we have a while loop without timeout counter.
In theory we could have infinite loop, if the HW has a bad day...
This could be safe on all OMAP platforms, but I think it has been only tested on 
OMAP4.
If this could happen, than IMHO it has to be handled by the omap_stop_dma, since 
other drivers could be hit by the same problem (there might be ERRATA for it 
already for OMAP4).

-- 
Péter

Re: [PATCH 2/2] ASoC: omap: Stop DMA re enabling in self linkage mode

[Mark Brown]

On Tue, Dec 07, 2010 at 04:38:35PM +0200, Peter Ujfalusi wrote:

> Sure, it is a single read to a register, but generally I'm a bit nervous, when 
> we have a while loop without timeout counter.
> In theory we could have infinite loop, if the HW has a bad day...
> This could be safe on all OMAP platforms, but I think it has been only tested on 
> OMAP4.
> If this could happen, than IMHO it has to be handled by the omap_stop_dma, since 
> other drivers could be hit by the same problem (there might be ERRATA for it 
> already for OMAP4).

A timeout would be better in general, yes.

Re: [PATCH 2/2] ASoC: omap: Stop DMA re enabling in self linkage mode

[Liam Girdwood]

On Tue, 2010-12-07 at 16:38 +0200, Peter Ujfalusi wrote:
> On Tuesday 07 December 2010 16:05:12 ext Mark Brown wrote:
> > On Tue, Dec 07, 2010 at 11:17:26AM +0200, Peter Ujfalusi wrote:
> > > I have not seen this happening on OMAP2/3, or at least I'm not aware of
> > > it. Is this problem OMAP4 only?
> > > Do you know if this happens on playback, capture or in both direction?
> > > Is it worth to do this workaround only on OMAP4?
> > 
> > Given that the patch should cause at most a single read from a register
> > when tearing down DMA if it's not required I'd guess it's safer to just
> > unconditionally enable the workaround on the off chance that it's just
> > reallly rare rather than not needed?
> 
> Sure, it is a single read to a register, but generally I'm a bit nervous, when 
> we have a while loop without timeout counter.
> In theory we could have infinite loop, if the HW has a bad day...
> This could be safe on all OMAP platforms, but I think it has been only tested on 
> OMAP4.
> If this could happen, than IMHO it has to be handled by the omap_stop_dma, since 
> other drivers could be hit by the same problem (there might be ERRATA for it 
> already for OMAP4).
> 

Afaik, this has also been tested on OMAP3. Not sure about OMAP2 and most
likely not tested on OMAP1.

Liam 

Re: [PATCH 2/2] ASoC: omap: Stop DMA re enabling in self linkage mode

[Jarkko Nikula]

On Tue, 7 Dec 2010 16:38:35 +0200
Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:

> If this could happen, than IMHO it has to be handled by the omap_stop_dma, since 
> other drivers could be hit by the same problem (there might be ERRATA for it 
> already for OMAP4).
> 
I agree with Peter that all DMA stopping related workarounds should be
in omap_stop_dma. I'd expect that DMA is stopped when returning
from there. Commit 9da65a9 was dealing with similar issue so probably
this looping should be implemented somewhere around those lines reading
& clearing the _CCR_EN bit?


-- 
Jarkko