ASoC: Merging at91 and avr32 dirs - test report

ASoC: Merging at91 and avr32 dirs - test report

[ext-jukka.hynninen]

I tested the patches and got everything working with minor changes.

However, there seems to be some problem with PM, occasionally ASoC is
stuck with Standby mode (looking at dapm_widgets) when I start the
playback. Restarting the player brings the sound back. I'll carry out
some further investigation and get back to this.

My kernel is 2.6.27-rc3, and machine driver is adapted from
playpaq_wm8510.c to run on at91rm9200 board.

The SSC peripheral clock was disabled more often than it was enabled,
and that catched the bug trap in arch/arm/mach-at91/clock.c. I'm not
sure if this was the real problem, but it's the way I managed to solve
it.


-Jukka


diff -uprN a/sound/soc/atmel/atmel_ssc_dai.c
b/sound/soc/atmel/atmel_ssc_dai.c
--- a/sound/soc/atmel/atmel_ssc_dai.c   2008-10-10 17:23:46.000000000
+0300
+++ b/sound/soc/atmel/atmel_ssc_dai.c   2008-10-10 17:26:06.000000000
+0300
@@ -261,11 +261,11 @@ static void atmel_ssc_shutdown(struct sn
        spin_lock_irq(&ssc_p->lock);
        ssc_p->dir_mask &= ~dir_mask;
        if (!ssc_p->dir_mask) {
-               /* Shutdown the SSC clock. */
-               pr_debug("atmel_ssc_dau: Stopping clock\n");
-               clk_disable(ssc_p->ssc->clk);
-
                if (ssc_p->initialized) {
+                       /* Shutdown the SSC clock. */
+                       pr_debug("atmel_ssc_dai: Stopping clock\n");
+                       clk_disable(ssc_p->ssc->clk);
+
                        free_irq(ssc_p->ssc->irq, ssc_p);
                        ssc_p->initialized = 0;
                }

Re: ASoC: Merging at91 and avr32 dirs - test report

[Mark Brown]

On Fri, Oct 10, 2008 at 06:15:17PM +0300, ext-jukka.hynninen@vaisala.com wrote:

> However, there seems to be some problem with PM, occasionally ASoC is
> stuck with Standby mode (looking at dapm_widgets) when I start the
> playback. Restarting the player brings the sound back. I'll carry out
> some further investigation and get back to this.

How does this compare with the behaviour of the original at32 machine
support - did that have similar issues?

> The SSC peripheral clock was disabled more often than it was enabled,
> and that catched the bug trap in arch/arm/mach-at91/clock.c. I'm not
> sure if this was the real problem, but it's the way I managed to solve
> it.

Looks reasonable to me.

> diff -uprN a/sound/soc/atmel/atmel_ssc_dai.c
> b/sound/soc/atmel/atmel_ssc_dai.c
> --- a/sound/soc/atmel/atmel_ssc_dai.c   2008-10-10 17:23:46.000000000
> +0300
> +++ b/sound/soc/atmel/atmel_ssc_dai.c   2008-10-10 17:26:06.000000000
> +0300
> @@ -261,11 +261,11 @@ static void atmel_ssc_shutdown(struct sn
>         spin_lock_irq(&ssc_p->lock);
>         ssc_p->dir_mask &= ~dir_mask;
>         if (!ssc_p->dir_mask) {
> -               /* Shutdown the SSC clock. */
> -               pr_debug("atmel_ssc_dau: Stopping clock\n");
> -               clk_disable(ssc_p->ssc->clk);
> -
>                 if (ssc_p->initialized) {
> +                       /* Shutdown the SSC clock. */
> +                       pr_debug("atmel_ssc_dai: Stopping clock\n");
> +                       clk_disable(ssc_p->ssc->clk);
> +
>                         free_irq(ssc_p->ssc->irq, ssc_p);
>                         ssc_p->initialized = 0;
>                 }

Re: ASoC: Merging at91 and avr32 dirs - test report

[ext-jukka.hynninen]

Quoting Mark Brown:

>> However, there seems to be some problem with PM, occasionally ASoC is
>> stuck with Standby mode (looking at dapm_widgets) when I start the
>> playback. Restarting the player brings the sound back. I'll carry out
>> some further investigation and get back to this.
> How does this compare with the behaviour of the original at32 machine
> support - did that have similar issues?

Actually I was previously using at91 support. Nevertheless, didn't
notice such problems. 

I'll gather more info and try to figure out what's going wrong.


-Jukka

Re: ASoC: Merging at91 and avr32 dirs - test report

[ext-jukka.hynninen]

>>> However, there seems to be some problem with PM, occasionally ASoC
is
>>> stuck with Standby mode (looking at dapm_widgets) when I start the
>>> playback. Restarting the player brings the sound back. I'll carry
out
>>> some further investigation and get back to this.
>> How does this compare with the behaviour of the original at32 machine
>> support - did that have similar issues?
> Actually I was previously using at91 support. Nevertheless, didn't
notice 
> such problems. 

This is propably a false alarm, exactly the same happens with at91-ssc.c

The above described behaviour was due to mpg123, which (for some reason)
does 
not wake up system from D3 state. Furthermore, running mpg123 confuses
DAPM
to think the codec is powered up properly, and waiting for delayed
shutdown.
Running aplay within anti-pop timeout will result in no-sound situation.

Using mpg123, the soc_pcm_prepare() never gets called, which makes me
suspect 
that the real problem might lie in mpg123 code. A similar issue has been

reported here in the past, but I couldn't find a solution:

http://thread.gmane.org/gmane.linux.alsa.devel/47908


-Jukka

Re: ASoC: Merging at91 and avr32 dirs - test report

[Mark Brown]

On Tue, Oct 14, 2008 at 03:38:22PM +0300, ext-jukka.hynninen@vaisala.com wrote:

> This is propably a false alarm, exactly the same happens with at91-ssc.c

OK, so you're happy that the new and old AT91 drivers perform
equivalently?

> Using mpg123, the soc_pcm_prepare() never gets called, which makes me
> suspect 
> that the real problem might lie in mpg123 code. A similar issue has been

The drivers should be robust against buggy applications, though.

> reported here in the past, but I couldn't find a solution:

> http://thread.gmane.org/gmane.linux.alsa.devel/47908

Yes, I think it's the same issue -  looks like a core bug which is still
present in current ASoC.  The patch below is *entirely* untested but may
help.  It needs more thought and (obviously) testing to make sure it's
safe before it can be merged.

[BTW, your mailer is doing some *really* strange line-wrapping.]


diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index ad38113..725b30b 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -429,51 +429,42 @@ static int soc_pcm_prepare(struct snd_pcm_substream *substream)
 		}
 	}
 
-	/* we only want to start a DAPM playback stream if we are not waiting
-	 * on an existing one stopping */
-	if (codec_dai->pop_wait) {
-		/* we are waiting for the delayed work to start */
-		if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
-				snd_soc_dapm_stream_event(socdev->codec,
-					codec_dai->capture.stream_name,
-					SND_SOC_DAPM_STREAM_START);
-		else {
-			codec_dai->pop_wait = 0;
-			cancel_delayed_work(&socdev->delayed_work);
-			snd_soc_dai_digital_mute(codec_dai, 0);
-		}
-	} else {
-		/* no delayed work - do we need to power up codec */
-		if (codec->bias_level != SND_SOC_BIAS_ON) {
+	/* cancel any delayed stream shutdown that is pending */
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
+	    codec_dai->pop_wait) {
+		codec_dai->pop_wait = 0;
+		cancel_delayed_work(&socdev->delayed_work);
+	}
 
-			snd_soc_dapm_set_bias_level(socdev,
-						    SND_SOC_BIAS_PREPARE);
+        /* do we need to power up codec */
+	if (codec->bias_level != SND_SOC_BIAS_ON) {
+		snd_soc_dapm_set_bias_level(socdev,
+					    SND_SOC_BIAS_PREPARE);
 
-			if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-				snd_soc_dapm_stream_event(codec,
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+			snd_soc_dapm_stream_event(codec,
 					codec_dai->playback.stream_name,
 					SND_SOC_DAPM_STREAM_START);
-			else
-				snd_soc_dapm_stream_event(codec,
+		else
+			snd_soc_dapm_stream_event(codec,
 					codec_dai->capture.stream_name,
 					SND_SOC_DAPM_STREAM_START);
 
-			snd_soc_dapm_set_bias_level(socdev, SND_SOC_BIAS_ON);
-			snd_soc_dai_digital_mute(codec_dai, 0);
+		snd_soc_dapm_set_bias_level(socdev, SND_SOC_BIAS_ON);
+		snd_soc_dai_digital_mute(codec_dai, 0);
 
-		} else {
-			/* codec already powered - power on widgets */
-			if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-				snd_soc_dapm_stream_event(codec,
+	} else {
+		/* codec already powered - power on widgets */
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+			snd_soc_dapm_stream_event(codec,
 					codec_dai->playback.stream_name,
 					SND_SOC_DAPM_STREAM_START);
-			else
-				snd_soc_dapm_stream_event(codec,
+		else
+			snd_soc_dapm_stream_event(codec,
 					codec_dai->capture.stream_name,
 					SND_SOC_DAPM_STREAM_START);
 
-			snd_soc_dai_digital_mute(codec_dai, 0);
-		}
+		snd_soc_dai_digital_mute(codec_dai, 0);
 	}
 
 out:

Re: ASoC: Merging at91 and avr32 dirs - test report

[ext-jukka.hynninen]

Quoting Mark Brown:

>> This is propably a false alarm, exactly the same happens with
at91-ssc.c
> OK, so you're happy that the new and old AT91 drivers perform
> equivalently?

Life's full of small joys

> present in current ASoC.  The patch below is *entirely* untested but
may
> help.  It needs more thought and (obviously) testing to make sure it's
> safe before it can be merged.

Thanks, works like a charm. I'll put the patch on a test.


-Jukka

Re: ASoC: Merging at91 and avr32 dirs - test report

[Sedji Gaouaou]

Hi,
ext-jukka.hynninen@vaisala.com a écrit :
> Quoting Mark Brown:
> 
>>> This is propably a false alarm, exactly the same happens with
> at91-ssc.c
>> OK, so you're happy that the new and old AT91 drivers perform
>> equivalently?
> 
> Life's full of small joys
> 
>> present in current ASoC.  The patch below is *entirely* untested but
> may
>> help.  It needs more thought and (obviously) testing to make sure it's
>> safe before it can be merged.
> 
> Thanks, works like a charm. I'll put the patch on a test.
> 
> 
> -Jukka
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 
I am writing to know what is the current status about the at91/avr32 
merging?
Is there any progress concerning the testing?
What is required for the patch to be accepted?

Regards,
Sedji

Re: ASoC: Merging at91 and avr32 dirs - test report

[Mark Brown]

On Mon, Oct 27, 2008 at 10:57:43AM +0100, Sedji Gaouaou wrote:

> I am writing to know what is the current status about the at91/avr32 
> merging?
> Is there any progress concerning the testing?
> What is required for the patch to be accepted?

Jukka seemed happy but we've had no testing at all on AVR32 so far as
far as I can see - Geoffrey had been going to test it but his hardware
broke.  I'd say the best thing now -rc1 is out is to merge the patches
as-is and then fix up any further issues that are identified later.

The playpaq support will need a small refresh for the current machine
driver, though.

Re: ASoC: Merging at91 and avr32 dirs - test report

[ext-jukka.hynninen]

Quoting Mark Brown:

>> Is there any progress concerning the testing?
>> What is required for the patch to be accepted?
> Jukka seemed happy but we've had no testing at all on AVR32 so far 
> as far as I can see - Geoffrey had been going to test it but his 

Affirmative, no promlems whatsoever encountered during several days
stress testing. At91 & WM8510 runs smoothly.

Just wanted to remind on the change that was needed to avoid the SSC
peripheral clock problem. I hope this will also be included.


-Jukka

Re: ASoC: Merging at91 and avr32 dirs - test report

[Mark Brown]

On Mon, Oct 27, 2008 at 01:13:06PM +0200, ext-jukka.hynninen@vaisala.com wrote:

> Just wanted to remind on the change that was needed to avoid the SSC
> peripheral clock problem. I hope this will also be included.

Which change was this?  I suspect we need another round of patches from
Sedji and then a final check with that...

Re: ASoC: Merging at91 and avr32 dirs - test report

[ext-jukka.hynninen]

Quoting Mark Brown:

>> Just wanted to remind on the change that was needed to avoid the SSC
>> peripheral clock problem. I hope this will also be included.
> Which change was this?  I suspect we need another round of patches
from
> Sedji and then a final check with that...

http://article.gmane.org/gmane.linux.alsa.devel/56742/


-Jukka

Re: ASoC: Merging at91 and avr32 dirs - test report

[Sedji Gaouaou]

Hi,

ext-jukka.hynninen@vaisala.com a écrit :
> I tested the patches and got everything working with minor changes.
> 
> However, there seems to be some problem with PM, occasionally ASoC is
> stuck with Standby mode (looking at dapm_widgets) when I start the
> playback. Restarting the player brings the sound back. I'll carry out
> some further investigation and get back to this.
Thank you for your help!

I've done more tests using the oss emulation in ALSA, and I have
encountered a problem.
In a previous mail Mark you mentioned this:
> +/*
> > + * Record SSC clock dividers for use in hw_params().
> > + */
> > +static int atmel_ssc_set_dai_clkdiv(struct snd_soc_dai *cpu_dai,
> > +	int div_id, int div)
> > +{
> > +	struct atmel_ssc_info *ssc_p = &ssc_info[cpu_dai->id];
> > +
> > +	switch (div_id) {
> > +	case AT91SSC_CMR_DIV:
> > +		/*
> > +		 * The same master clock divider is used for both
> > +		 * transmit and receive, so if a value has already
> > +		 * been set, it must match this value.
> > +		 */
> > +		if (ssc_p->cmr_div == 0)
> > +			ssc_p->cmr_div = div;
> > +		else
> > +			if (div != ssc_p->cmr_div)
> > +				return -EBUSY;
> > +		break;

What happens if the user wants to change the master clock divider at
runtime - for example, when changing sample rates?

I had actually a crash because of this, so I will send a patch to
correct it(I simply removed it since I don't thonk it is needed).


Regards,
Sedji

Re: ASoC: Merging at91 and avr32 dirs - test report

[Mark Brown]

On Mon, Oct 13, 2008 at 04:22:06PM +0200, Sedji Gaouaou wrote:
> ext-jukka.hynninen@vaisala.com a ?crit :

> >> +	switch (div_id) {
> >> +	case AT91SSC_CMR_DIV:
> >> +		/*
> >> +		 * The same master clock divider is used for both
> >> +		 * transmit and receive, so if a value has already
> >> +		 * been set, it must match this value.
> >> +		 */
> >> +		if (ssc_p->cmr_div == 0)
> >> +			ssc_p->cmr_div = div;
> >> +		else
> >> +			if (div != ssc_p->cmr_div)
> >> +				return -EBUSY;
> >> +		break;

> What happens if the user wants to change the master clock divider at
> runtime - for example, when changing sample rates?

Usually you'd end up using constraints to prevent user space selecting a
different sample rate if it wasn't possible to change safely.