[PATCH] ASoC: AM3517: Fix AIC23 suspend/resume hang

[PATCH] ASoC: AM3517: Fix AIC23 suspend/resume hang

[Anuj Aggarwal]

System hang is observed While trying to do suspend. It was because
of an infinite loop in the AIC23 resume path which was trying to
restore AIC23 register values from the register cache.

This patch fixes the problem by correcting the resume path and
properly activating/deactivating the digital interface while
doing the suspend / off transitions.

Signed-off-by: Anuj Aggarwal <anuj.aggarwal@ti.com>
---
 sound/soc/codecs/tlv320aic23.c |   16 +++-------------
 1 files changed, 3 insertions(+), 13 deletions(-)

diff --git a/sound/soc/codecs/tlv320aic23.c b/sound/soc/codecs/tlv320aic23.c
index 6b24d8b..cabe60c 100644
--- a/sound/soc/codecs/tlv320aic23.c
+++ b/sound/soc/codecs/tlv320aic23.c
@@ -565,13 +565,12 @@ static int tlv320aic23_set_bias_level(struct snd_soc_codec *codec,
 	case SND_SOC_BIAS_PREPARE:
 		break;
 	case SND_SOC_BIAS_STANDBY:
-		/* everything off except vref/vmid, */
-		tlv320aic23_write(codec, TLV320AIC23_PWR, reg | 0x0040);
+		/* Activate the digital interface */
+		tlv320aic23_write(codec, TLV320AIC23_ACTIVE, 0x1);
 		break;
 	case SND_SOC_BIAS_OFF:
-		/* everything off, dac mute, inactive */
+		/* Deactivate the digital interface */
 		tlv320aic23_write(codec, TLV320AIC23_ACTIVE, 0x0);
-		tlv320aic23_write(codec, TLV320AIC23_PWR, 0xffff);
 		break;
 	}
 	codec->bias_level = level;
@@ -615,7 +614,6 @@ static int tlv320aic23_suspend(struct platform_device *pdev,
 	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
 	struct snd_soc_codec *codec = socdev->card->codec;
 
-	tlv320aic23_write(codec, TLV320AIC23_ACTIVE, 0x0);
 	tlv320aic23_set_bias_level(codec, SND_SOC_BIAS_OFF);
 
 	return 0;
@@ -625,14 +623,6 @@ static int tlv320aic23_resume(struct platform_device *pdev)
 {
 	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
 	struct snd_soc_codec *codec = socdev->card->codec;
-	int i;
-	u16 reg;
-
-	/* Sync reg_cache with the hardware */
-	for (reg = 0; reg < ARRAY_SIZE(tlv320aic23_reg); i++) {
-		u16 val = tlv320aic23_read_reg_cache(codec, reg);
-		tlv320aic23_write(codec, reg, val);
-	}
 
 	tlv320aic23_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
 	tlv320aic23_set_bias_level(codec, codec->suspend_bias_level);
-- 
1.6.2.4

Re: [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume hang

[Mark Brown]

On Wed, Nov 25, 2009 at 06:40:31PM +0530, Anuj Aggarwal wrote:

> System hang is observed While trying to do suspend. It was because
> of an infinite loop in the AIC23 resume path which was trying to
> restore AIC23 register values from the register cache.

This doesn't appear to tie in with the actual patch - where's the
infinite loop?  The register cache restore loop has a constant bound and
I can't see anything that would make it spin infinitely.

> This patch fixes the problem by correcting the resume path and
> properly activating/deactivating the digital interface while
> doing the suspend / off transitions.

What do you mean by "properly activating/deactivating the digital
interface"?

>  	case SND_SOC_BIAS_STANDBY:
> -		/* everything off except vref/vmid, */
> -		tlv320aic23_write(codec, TLV320AIC23_PWR, reg | 0x0040);
> +		/* Activate the digital interface */
> +		tlv320aic23_write(codec, TLV320AIC23_ACTIVE, 0x1);
>  		break;
>  	case SND_SOC_BIAS_OFF:
> -		/* everything off, dac mute, inactive */
> +		/* Deactivate the digital interface */
>  		tlv320aic23_write(codec, TLV320AIC23_ACTIVE, 0x0);
> -		tlv320aic23_write(codec, TLV320AIC23_PWR, 0xffff);
>  		break;

This looks wrong - the driver is now no longer managing bias levels at
all.

> -	u16 reg;
> -
> -	/* Sync reg_cache with the hardware */
> -	for (reg = 0; reg < ARRAY_SIZE(tlv320aic23_reg); i++) {
> -		u16 val = tlv320aic23_read_reg_cache(codec, reg);
> -		tlv320aic23_write(codec, reg, val);
> -	}

This also looks wrong, the register cache restore has been completely
removed so if the power to the device is cut during suspend then the
device will go back to register defaults and no longer function
correctly after resume.

RE: [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume hang

[Aggarwal, Anuj]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: Wednesday, November 25, 2009 7:04 PM
> To: Aggarwal, Anuj
> Cc: alsa-devel@alsa-project.org; linux-omap@vger.kernel.org
> Subject: Re: [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume hang
> 
> On Wed, Nov 25, 2009 at 06:40:31PM +0530, Anuj Aggarwal wrote:
> 
> > System hang is observed While trying to do suspend. It was because
> > of an infinite loop in the AIC23 resume path which was trying to
> > restore AIC23 register values from the register cache.
> 
> This doesn't appear to tie in with the actual patch - where's the
> infinite loop?  The register cache restore loop has a constant bound and
> I can't see anything that would make it spin infinitely.
[Aggarwal, Anuj] Here is the original code:

<snip>
        int i;
        u16 reg;

        /* Sync reg_cache with the hardware */
        for (reg = 0; reg < ARRAY_SIZE(tlv320aic23_reg); i++) {
                u16 val = tlv320aic23_read_reg_cache(codec, reg);
                tlv320aic23_write(codec, reg, val);
        }
</snip>

I can see 'i' getting incremented, instead of 'reg'. Isn't this an
infinite loop?
> 
> > This patch fixes the problem by correcting the resume path and
> > properly activating/deactivating the digital interface while
> > doing the suspend / off transitions.
> 
> What do you mean by "properly activating/deactivating the digital
> interface"?
[Aggarwal, Anuj] The driver was only deactivating the digital
interface and not activating it. Hence added that part. 
> 
> >  	case SND_SOC_BIAS_STANDBY:
> > -		/* everything off except vref/vmid, */
> > -		tlv320aic23_write(codec, TLV320AIC23_PWR, reg | 0x0040);
> > +		/* Activate the digital interface */
> > +		tlv320aic23_write(codec, TLV320AIC23_ACTIVE, 0x1);
> >  		break;
> >  	case SND_SOC_BIAS_OFF:
> > -		/* everything off, dac mute, inactive */
> > +		/* Deactivate the digital interface */
> >  		tlv320aic23_write(codec, TLV320AIC23_ACTIVE, 0x0);
> > -		tlv320aic23_write(codec, TLV320AIC23_PWR, 0xffff);
> >  		break;
> 
> This looks wrong - the driver is now no longer managing bias levels at
> all.
[Aggarwal, Anuj] The bias levels were wrongly managed earlier. While doing 
a suspend, it was powering off all the interfaces and while coming up, it
was not switching on the required interfaces. Hence no sound was coming
out if audio was played back. 
Moreover, I referred twl4030 codec and there also the driver was only
handling the codec power control bit in _STANDBY. So based my patch
on that. 
On each bias level, which are the interfaces which need to be switched
on / off?
> 
> > -	u16 reg;
> > -
> > -	/* Sync reg_cache with the hardware */
> > -	for (reg = 0; reg < ARRAY_SIZE(tlv320aic23_reg); i++) {
> > -		u16 val = tlv320aic23_read_reg_cache(codec, reg);
> > -		tlv320aic23_write(codec, reg, val);
> > -	}
> 
> This also looks wrong, the register cache restore has been completely
> removed so if the power to the device is cut during suspend then the
> device will go back to register defaults and no longer function
> correctly after resume.
[Aggarwal, Anuj] I can add that register cache restore again with the
necessary checks.

Re: [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume hang

[Mark Brown]

On Thu, Nov 26, 2009 at 12:49:11AM +0530, Aggarwal, Anuj wrote:

> <snip>
>         int i;
>         u16 reg;
> 
>         /* Sync reg_cache with the hardware */
>         for (reg = 0; reg < ARRAY_SIZE(tlv320aic23_reg); i++) {
>                 u16 val = tlv320aic23_read_reg_cache(codec, reg);
>                 tlv320aic23_write(codec, reg, val);
>         }
> </snip>

> I can see 'i' getting incremented, instead of 'reg'. Isn't this an
> infinite loop?

Yes, that does look entirely buggy - it seems fairly clear that suspend
and resume have never worked with this driver.  The fact that you were
removing the loop entirely meant I didn't read the actual code for bugs.

> > > This patch fixes the problem by correcting the resume path and
> > > properly activating/deactivating the digital interface while
> > > doing the suspend / off transitions.

> > What do you mean by "properly activating/deactivating the digital
> > interface"?

> [Aggarwal, Anuj] The driver was only deactivating the digital
> interface and not activating it. Hence added that part. 

So this is a separate issue, and should ideally be a separate patch - it
looks like you've got three different changes here, one to fix the buggy
register cache restore, one to manage the PWR bit on suspend and another
to possibly fix the bias configuration.  You certainly need to identify
all three changes in the changelog, especially for things like this
which look like they should go in as bug fixes (and bearing in mind that
it's the end of the release cycle).

Glancing at the code there's much more management required here -
there's other places where it's managed and the code needs to be joined
up with them.  The setting should only be restored if the device was
active when suspended, not unconditionally.

> > >  	case SND_SOC_BIAS_STANDBY:
> > > -		/* everything off except vref/vmid, */
> > > -		tlv320aic23_write(codec, TLV320AIC23_PWR, reg | 0x0040);
> > > +		/* Activate the digital interface */
> > > +		tlv320aic23_write(codec, TLV320AIC23_ACTIVE, 0x1);
> > >  		break;
> > >  	case SND_SOC_BIAS_OFF:
> > > -		/* everything off, dac mute, inactive */
> > > +		/* Deactivate the digital interface */
> > >  		tlv320aic23_write(codec, TLV320AIC23_ACTIVE, 0x0);
> > > -		tlv320aic23_write(codec, TLV320AIC23_PWR, 0xffff);
> > >  		break;
> > 
> > This looks wrong - the driver is now no longer managing bias levels at
> > all.
> [Aggarwal, Anuj] The bias levels were wrongly managed earlier. While doing 
> a suspend, it was powering off all the interfaces and while coming up, it
> was not switching on the required interfaces. Hence no sound was coming
> out if audio was played back. 

Note that this CODEC driver impelements DAPM and so all the bits in the
power register which are controlled by DAPM will be managed by the ASoC
core without any action by the driver.

> On each bias level, which are the interfaces which need to be switched
> on / off?

The biases and any other device-wide settings, everything else should be
managed via DAPM if it's in use.

Re: [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume hang

[Aggarwal, Anuj]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: Thursday, November 26, 2009 1:10 AM
> To: Aggarwal, Anuj
> Cc: alsa-devel@alsa-project.org; linux-omap@vger.kernel.org
> Subject: Re: [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume hang
> 
> On Thu, Nov 26, 2009 at 12:49:11AM +0530, Aggarwal, Anuj wrote:
> 
> > <snip>
> >         int i;
> >         u16 reg;
> >
> >         /* Sync reg_cache with the hardware */
> >         for (reg = 0; reg < ARRAY_SIZE(tlv320aic23_reg); i++) {
> >                 u16 val = tlv320aic23_read_reg_cache(codec, reg);
> >                 tlv320aic23_write(codec, reg, val);
> >         }
> > </snip>
> 
> > I can see 'i' getting incremented, instead of 'reg'. Isn't this an
> > infinite loop?
> 
> Yes, that does look entirely buggy - it seems fairly clear that suspend
> and resume have never worked with this driver.  The fact that you were
> removing the loop entirely meant I didn't read the actual code for bugs.
> 
> > > > This patch fixes the problem by correcting the resume path and
> > > > properly activating/deactivating the digital interface while
> > > > doing the suspend / off transitions.
> 
> > > What do you mean by "properly activating/deactivating the digital
> > > interface"?
> 
> > [Aggarwal, Anuj] The driver was only deactivating the digital
> > interface and not activating it. Hence added that part.
> 
> So this is a separate issue, and should ideally be a separate patch - it
> looks like you've got three different changes here, one to fix the buggy
> register cache restore, one to manage the PWR bit on suspend and another
> to possibly fix the bias configuration.  You certainly need to identify
> all three changes in the changelog, especially for things like this
> which look like they should go in as bug fixes (and bearing in mind that
> it's the end of the release cycle).
Fine with me, I will push individual patches for all the issues.
> 
> Glancing at the code there's much more management required here -
> there's other places where it's managed and the code needs to be joined
> up with them.  The setting should only be restored if the device was
> active when suspended, not unconditionally.
> 
> > > >  	case SND_SOC_BIAS_STANDBY:
> > > > -		/* everything off except vref/vmid, */
> > > > -		tlv320aic23_write(codec, TLV320AIC23_PWR, reg | 0x0040);
> > > > +		/* Activate the digital interface */
> > > > +		tlv320aic23_write(codec, TLV320AIC23_ACTIVE, 0x1);
> > > >  		break;
> > > >  	case SND_SOC_BIAS_OFF:
> > > > -		/* everything off, dac mute, inactive */
> > > > +		/* Deactivate the digital interface */
> > > >  		tlv320aic23_write(codec, TLV320AIC23_ACTIVE, 0x0);
> > > > -		tlv320aic23_write(codec, TLV320AIC23_PWR, 0xffff);
> > > >  		break;
> > >
> > > This looks wrong - the driver is now no longer managing bias levels at
> > > all.
> > [Aggarwal, Anuj] The bias levels were wrongly managed earlier. While
> doing
> > a suspend, it was powering off all the interfaces and while coming up,
> it
> > was not switching on the required interfaces. Hence no sound was coming
> > out if audio was played back.
> 
> Note that this CODEC driver impelements DAPM and so all the bits in the
> power register which are controlled by DAPM will be managed by the ASoC
> core without any action by the driver.
> 
> > On each bias level, which are the interfaces which need to be switched
> > on / off?
> 
> The biases and any other device-wide settings, everything else should be
> managed via DAPM if it's in use.

Re: [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume hang

[Troy Kisky]

Anuj Aggarwal wrote:
> System hang is observed While trying to do suspend. It was because
> of an infinite loop in the AIC23 resume path which was trying to
> restore AIC23 register values from the register cache.
> 
> This patch fixes the problem by correcting the resume path and
> properly activating/deactivating the digital interface while
> doing the suspend / off transitions.
> 
> Signed-off-by: Anuj Aggarwal <anuj.aggarwal@ti.com>
> ---
>  sound/soc/codecs/tlv320aic23.c |   16 +++-------------
>  1 files changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/sound/soc/codecs/tlv320aic23.c b/sound/soc/codecs/tlv320aic23.c
> index 6b24d8b..cabe60c 100644
> --- a/sound/soc/codecs/tlv320aic23.c
> +++ b/sound/soc/codecs/tlv320aic23.c
> @@ -565,13 +565,12 @@ static int tlv320aic23_set_bias_level(struct snd_soc_codec *codec,
>  	case SND_SOC_BIAS_PREPARE:
>  		break;
>  	case SND_SOC_BIAS_STANDBY:
> -		/* everything off except vref/vmid, */
> -		tlv320aic23_write(codec, TLV320AIC23_PWR, reg | 0x0040);
> +		/* Activate the digital interface */
> +		tlv320aic23_write(codec, TLV320AIC23_ACTIVE, 0x1);
>  		break;
>  	case SND_SOC_BIAS_OFF:
> -		/* everything off, dac mute, inactive */
> +		/* Deactivate the digital interface */
>  		tlv320aic23_write(codec, TLV320AIC23_ACTIVE, 0x0);
> -		tlv320aic23_write(codec, TLV320AIC23_PWR, 0xffff);
>  		break;
>  	}
>  	codec->bias_level = level;
> @@ -615,7 +614,6 @@ static int tlv320aic23_suspend(struct platform_device *pdev,
>  	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
>  	struct snd_soc_codec *codec = socdev->card->codec;
>  
> -	tlv320aic23_write(codec, TLV320AIC23_ACTIVE, 0x0);
>  	tlv320aic23_set_bias_level(codec, SND_SOC_BIAS_OFF);
>  
>  	return 0;
> @@ -625,14 +623,6 @@ static int tlv320aic23_resume(struct platform_device *pdev)
>  {
>  	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
>  	struct snd_soc_codec *codec = socdev->card->codec;
> -	int i;
> -	u16 reg;
> -
> -	/* Sync reg_cache with the hardware */
> -	for (reg = 0; reg < ARRAY_SIZE(tlv320aic23_reg); i++) {
> -		u16 val = tlv320aic23_read_reg_cache(codec, reg);
> -		tlv320aic23_write(codec, reg, val);
> -	}

Changing to
	for (reg = 0; reg < ARRAY_SIZE(tlv320aic23_reg); reg++) {

should be enough to fix the infinite loop.
Could you try this without the rest of the patch?

>  
>  	tlv320aic23_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
>  	tlv320aic23_set_bias_level(codec, codec->suspend_bias_level);

Adding Arun to cc list

Re: [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume hang

[Aggarwal, Anuj]

> -----Original Message-----
> From: Troy Kisky [mailto:troy.kisky@boundarydevices.com]
> Sent: Thursday, November 26, 2009 1:53 AM
> To: Aggarwal, Anuj
> Cc: alsa-devel@alsa-project.org; linux-omap@vger.kernel.org; Mark Brown;
> Arun KS
> Subject: Re: [alsa-devel] [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume
> hang
> 
> Anuj Aggarwal wrote:
> > System hang is observed While trying to do suspend. It was because
> > of an infinite loop in the AIC23 resume path which was trying to
> > restore AIC23 register values from the register cache.
> >
> > This patch fixes the problem by correcting the resume path and
> > properly activating/deactivating the digital interface while
> > doing the suspend / off transitions.
> >
> > Signed-off-by: Anuj Aggarwal <anuj.aggarwal@ti.com>
> > ---
> >  sound/soc/codecs/tlv320aic23.c |   16 +++-------------
> >  1 files changed, 3 insertions(+), 13 deletions(-)
> >
> > diff --git a/sound/soc/codecs/tlv320aic23.c
> b/sound/soc/codecs/tlv320aic23.c
> > index 6b24d8b..cabe60c 100644
> > --- a/sound/soc/codecs/tlv320aic23.c
> > +++ b/sound/soc/codecs/tlv320aic23.c
> > @@ -565,13 +565,12 @@ static int tlv320aic23_set_bias_level(struct
> snd_soc_codec *codec,
> >  	case SND_SOC_BIAS_PREPARE:
> >  		break;
> >  	case SND_SOC_BIAS_STANDBY:
> > -		/* everything off except vref/vmid, */
> > -		tlv320aic23_write(codec, TLV320AIC23_PWR, reg | 0x0040);
> > +		/* Activate the digital interface */
> > +		tlv320aic23_write(codec, TLV320AIC23_ACTIVE, 0x1);
> >  		break;
> >  	case SND_SOC_BIAS_OFF:
> > -		/* everything off, dac mute, inactive */
> > +		/* Deactivate the digital interface */
> >  		tlv320aic23_write(codec, TLV320AIC23_ACTIVE, 0x0);
> > -		tlv320aic23_write(codec, TLV320AIC23_PWR, 0xffff);
> >  		break;
> >  	}
> >  	codec->bias_level = level;
> > @@ -615,7 +614,6 @@ static int tlv320aic23_suspend(struct
> platform_device *pdev,
> >  	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
> >  	struct snd_soc_codec *codec = socdev->card->codec;
> >
> > -	tlv320aic23_write(codec, TLV320AIC23_ACTIVE, 0x0);
> >  	tlv320aic23_set_bias_level(codec, SND_SOC_BIAS_OFF);
> >
> >  	return 0;
> > @@ -625,14 +623,6 @@ static int tlv320aic23_resume(struct
> platform_device *pdev)
> >  {
> >  	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
> >  	struct snd_soc_codec *codec = socdev->card->codec;
> > -	int i;
> > -	u16 reg;
> > -
> > -	/* Sync reg_cache with the hardware */
> > -	for (reg = 0; reg < ARRAY_SIZE(tlv320aic23_reg); i++) {
> > -		u16 val = tlv320aic23_read_reg_cache(codec, reg);
> > -		tlv320aic23_write(codec, reg, val);
> > -	}
> 
> Changing to
> 	for (reg = 0; reg < ARRAY_SIZE(tlv320aic23_reg); reg++) {
> 
> should be enough to fix the infinite loop.
> Could you try this without the rest of the patch?
I have already tried that but it doesn't fix the problem entirely.
Still, I was not able to playback properly because the bias settings
were not handled correctly and as you can see, it writes to some 
invalid registers too (all registers don't exist between 0 and ARRAY_SIZE).
> 
> >
> >  	tlv320aic23_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
> >  	tlv320aic23_set_bias_level(codec, codec->suspend_bias_level);
> 
> Adding Arun to cc list

Re: [alsa-devel] [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume hang

[Mark Brown]

On Thu, Nov 26, 2009 at 08:31:11AM +0530, Aggarwal, Anuj wrote:

> I have already tried that but it doesn't fix the problem entirely.
> Still, I was not able to playback properly because the bias settings
> were not handled correctly and as you can see, it writes to some 
> invalid registers too (all registers don't exist between 0 and ARRAY_SIZE).

Please also note the following patch which is queued for 2.6.32:

commit 50b6bce59d154b5db137907a5c0ed45a4e7a3829
Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
Date:   Mon Nov 23 13:11:53 2009 +0000

    ASoC: Fix suspend with active audio streams
    
    When we get a stream suspend event force the power down since otherwise
    the stream would remain marked as active.  In future we'll probably want
    to make this stream-specific and add an interface to make the power down
    of other widgets optional in order to support leaving bypass paths
    active while suspending the processor.
    
    Cc: stable@kernel.org
    Reported-by: Joonyoung Shim <jy0922.shim@samsung.com>
    Tested-by: Joonyoung Shim <jy0922.shim@samsung.com>
    Acked-by: Liam Girdwood <lrg@slimlogic.co.uk>
    Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index d89f6dc..66d4c16 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -973,9 +973,19 @@ static int dapm_power_widgets(struct snd_soc_codec *codec, int event)
 			if (!w->power_check)
 				continue;
 
-			power = w->power_check(w);
-			if (power)
-				sys_power = 1;
+			/* If we're suspending then pull down all the 
+			 * power. */
+			switch (event) {
+			case SND_SOC_DAPM_STREAM_SUSPEND:
+				power = 0;
+				break;
+
+			default:
+				power = w->power_check(w);
+				if (power)
+					sys_power = 1;
+				break;
+			}
 
 			if (w->power == power)
 				continue;
@@ -999,8 +1009,12 @@ static int dapm_power_widgets(struct snd_soc_codec *codec, int event)
 		case SND_SOC_DAPM_STREAM_RESUME:
 			sys_power = 1;
 			break;
+		case SND_SOC_DAPM_STREAM_SUSPEND:
+			sys_power = 0;
+			break;
 		case SND_SOC_DAPM_STREAM_NOP:
 			sys_power = codec->bias_level != SND_SOC_BIAS_STANDBY;
+			break;
 		default:
 			break;
 		}

RE: [alsa-devel] [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume hang

[Aggarwal, Anuj]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: Thursday, November 26, 2009 3:53 PM
> To: Aggarwal, Anuj
> Cc: 'Troy Kisky'; alsa-devel@alsa-project.org; linux-omap@vger.kernel.org;
> Arun KS
> Subject: Re: [alsa-devel] [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume
> hang
> 
> On Thu, Nov 26, 2009 at 08:31:11AM +0530, Aggarwal, Anuj wrote:
> 
> > I have already tried that but it doesn't fix the problem entirely.
> > Still, I was not able to playback properly because the bias settings
> > were not handled correctly and as you can see, it writes to some
> > invalid registers too (all registers don't exist between 0 and
> ARRAY_SIZE).
> 
> Please also note the following patch which is queued for 2.6.32:
> 
> commit 50b6bce59d154b5db137907a5c0ed45a4e7a3829
> Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Date:   Mon Nov 23 13:11:53 2009 +0000
> 
>     ASoC: Fix suspend with active audio streams
> 
[Aggarwal, Anuj] I am also seeing one more problem with the original 
driver, although I am not sure if the problem is in the audio driver. 
When tried to capture, using NFS as storage, it gives overrun error and comes out with:
arecord: pcm_read:1617: read error: Input/output error

It happen always after ~20 sec, file size ~5MB. Tried with multiple
configurations in arecord but no use.
When tried: arecord -f cd /dev/null, it works fine. Same issue doesn't 
come too when I try to store the captured audio file on a MMC card.

Any idea what could be the problem? Why arecord goes for a toss after a 
single overrun error and why it is happening always after ~20 sec? Is
there something which can be tried to narrow down the problem?

Re: [alsa-devel] [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume hang

[Mark Brown]

On Thu, Nov 26, 2009 at 08:26:44PM +0530, Aggarwal, Anuj wrote:

> driver, although I am not sure if the problem is in the audio driver. 
> When tried to capture, using NFS as storage, it gives overrun error and comes out with:
> arecord: pcm_read:1617: read error: Input/output error

> It happen always after ~20 sec, file size ~5MB. Tried with multiple
> configurations in arecord but no use.
> When tried: arecord -f cd /dev/null, it works fine. Same issue doesn't 
> come too when I try to store the captured audio file on a MMC card.

> Any idea what could be the problem? Why arecord goes for a toss after a 
> single overrun error and why it is happening always after ~20 sec? Is
> there something which can be tried to narrow down the problem?

Sounds like you've narrowed the problem down to a performance issue with
NFS writeout - it's probably having trouble keeping up with your I/O
rate.  This isn't 100% surprising with smaller systems, sometimes tuning
the NFS configuration can resolve the issue but sometimes the hardware
is just underspecified.

arecord is a pretty basic program and doesn't try terribly hard to
recover from errors.

RE: [alsa-devel] [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume hang

[Aggarwal, Anuj]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: Thursday, November 26, 2009 8:41 PM
> To: Aggarwal, Anuj
> Cc: 'Troy Kisky'; alsa-devel@alsa-project.org; linux-omap@vger.kernel.org;
> Arun KS
> Subject: Re: [alsa-devel] [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume
> hang
> 
> On Thu, Nov 26, 2009 at 08:26:44PM +0530, Aggarwal, Anuj wrote:
> 
> > driver, although I am not sure if the problem is in the audio driver.
> > When tried to capture, using NFS as storage, it gives overrun error and
> comes out with:
> > arecord: pcm_read:1617: read error: Input/output error
> 
> > It happen always after ~20 sec, file size ~5MB. Tried with multiple
> > configurations in arecord but no use.
> > When tried: arecord -f cd /dev/null, it works fine. Same issue doesn't
> > come too when I try to store the captured audio file on a MMC card.
> 
> > Any idea what could be the problem? Why arecord goes for a toss after a
> > single overrun error and why it is happening always after ~20 sec? Is
> > there something which can be tried to narrow down the problem?
> 
> Sounds like you've narrowed the problem down to a performance issue with
> NFS writeout - it's probably having trouble keeping up with your I/O
> rate.  This isn't 100% surprising with smaller systems, sometimes tuning
> the NFS configuration can resolve the issue but sometimes the hardware
> is just underspecified.
[Aggarwal, Anuj] I am still surprised how this could be a NFS writeout issue
as we are seeing a consistent read/write rate of 2Mbps over tftp. When dd
command is used for read/write to further check NFS performance, 2Mbps for write and 4Mbps for read is observed.
Does that still mean nfs is the culprit? What could be tweaked in audio/network driver to avoid this problem, any suggestions?
> 
> arecord is a pretty basic program and doesn't try terribly hard to
> recover from errors.
[Aggarwal, Anuj] Any other utility to try capture which does error 
recovering too?

RE: [alsa-devel] [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume hang

[Aggarwal, Anuj]

The performance numbers are in Mbytes/sec. Sorry for the typo :(

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Aggarwal, Anuj
> Sent: Thursday, November 26, 2009 8:52 PM
> To: Mark Brown
> Cc: 'Troy Kisky'; alsa-devel@alsa-project.org; linux-omap@vger.kernel.org;
> Arun KS
> Subject: RE: [alsa-devel] [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume
> hang
> 
> > -----Original Message-----
> > From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> > Sent: Thursday, November 26, 2009 8:41 PM
> > To: Aggarwal, Anuj
> > Cc: 'Troy Kisky'; alsa-devel@alsa-project.org; linux-
> omap@vger.kernel.org;
> > Arun KS
> > Subject: Re: [alsa-devel] [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume
> > hang
> >
> > On Thu, Nov 26, 2009 at 08:26:44PM +0530, Aggarwal, Anuj wrote:
> >
> > > driver, although I am not sure if the problem is in the audio driver.
> > > When tried to capture, using NFS as storage, it gives overrun error
> and
> > comes out with:
> > > arecord: pcm_read:1617: read error: Input/output error
> >
> > > It happen always after ~20 sec, file size ~5MB. Tried with multiple
> > > configurations in arecord but no use.
> > > When tried: arecord -f cd /dev/null, it works fine. Same issue doesn't
> > > come too when I try to store the captured audio file on a MMC card.
> >
> > > Any idea what could be the problem? Why arecord goes for a toss after
> a
> > > single overrun error and why it is happening always after ~20 sec? Is
> > > there something which can be tried to narrow down the problem?
> >
> > Sounds like you've narrowed the problem down to a performance issue with
> > NFS writeout - it's probably having trouble keeping up with your I/O
> > rate.  This isn't 100% surprising with smaller systems, sometimes tuning
> > the NFS configuration can resolve the issue but sometimes the hardware
> > is just underspecified.
> [Aggarwal, Anuj] I am still surprised how this could be a NFS writeout
> issue
> as we are seeing a consistent read/write rate of 2Mbps over tftp. When dd
> command is used for read/write to further check NFS performance, 2Mbps for
> write and 4Mbps for read is observed.
> Does that still mean nfs is the culprit? What could be tweaked in
> audio/network driver to avoid this problem, any suggestions?
> >
> > arecord is a pretty basic program and doesn't try terribly hard to
> > recover from errors.
> [Aggarwal, Anuj] Any other utility to try capture which does error
> recovering too?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume hang

[Mark Brown]

On Thu, Nov 26, 2009 at 08:52:08PM +0530, Aggarwal, Anuj wrote:

> [Aggarwal, Anuj] I am still surprised how this could be a NFS writeout issue
> as we are seeing a consistent read/write rate of 2Mbps over tftp. When dd
> command is used for read/write to further check NFS performance, 2Mbps for write and 4Mbps for read is observed.
> Does that still mean nfs is the culprit? What could be tweaked in audio/network driver to avoid this problem, any suggestions?

There can also be issues with the way the data gets pushed into NFS
interacting poorly - it's not just the raw data rate that's in play
here, it's also things like how often the writes are done and how big
they are.  Possibly also overhead from interacting with the ethernet
chip but that's not normally an issue for anything modern.

The fact that this only happens when NFS is in use seems a fairly clear
pointer to an interacton there.

> [Aggarwal, Anuj] Any other utility to try capture which does error
> recovering too?

Not for the console off the top of my head, and TBH I don't really know
how good the error handling is in the various apps.  You could also try
playing with the buffer size options in arecord.

Re: [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume hang

[Aggarwal, Anuj]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: Thursday, November 26, 2009 9:00 PM
> To: Aggarwal, Anuj
> Cc: 'Troy Kisky'; alsa-devel@alsa-project.org; linux-omap@vger.kernel.org;
> Arun KS
> Subject: Re: [alsa-devel] [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume
> hang
> 
> On Thu, Nov 26, 2009 at 08:52:08PM +0530, Aggarwal, Anuj wrote:
> 
> > [Aggarwal, Anuj] I am still surprised how this could be a NFS writeout
> issue
> > as we are seeing a consistent read/write rate of 2Mbps over tftp. When
> dd
> > command is used for read/write to further check NFS performance, 2Mbps
> for write and 4Mbps for read is observed.
> > Does that still mean nfs is the culprit? What could be tweaked in
> audio/network driver to avoid this problem, any suggestions?
> 
> There can also be issues with the way the data gets pushed into NFS
> interacting poorly - it's not just the raw data rate that's in play
> here, it's also things like how often the writes are done and how big
> they are.  Possibly also overhead from interacting with the ethernet
> chip but that's not normally an issue for anything modern.
> 
> The fact that this only happens when NFS is in use seems a fairly clear
> pointer to an interacton there.
[Aggarwal, Anuj] We were able to fine tune NFS and use arecord to capture
large files. But some more problems cropped up when tried to suspend /
resume. Basic playback is working fine wrt suspend/resume. But capture,
either tried independently or with playback, is creating a system wide
hang. I fixed that infinite-loop in resume path but I believe something
else needs cleanup too.
Any pointers? 
> 
> > [Aggarwal, Anuj] Any other utility to try capture which does error
> > recovering too?
> 
> Not for the console off the top of my head, and TBH I don't really know
> how good the error handling is in the various apps.  You could also try
> playing with the buffer size options in arecord.

Re: [alsa-devel] [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume hang

[Mark Brown]

On Fri, Nov 27, 2009 at 01:37:54PM +0530, Aggarwal, Anuj wrote:

> [Aggarwal, Anuj] We were able to fine tune NFS and use arecord to capture
> large files. But some more problems cropped up when tried to suspend /
> resume. Basic playback is working fine wrt suspend/resume. But capture,
> either tried independently or with playback, is creating a system wide
> hang. I fixed that infinite-loop in resume path but I believe something
> else needs cleanup too.
> Any pointers? 

Have you got CONFIG_DETECT_SOFTLOCKUP turned on?  With any luck that
will at least pinpoint where the code has locked up.  Otherwise it's
going to be a case of bisection via printk().

A contrast and compare of the hardware initialisation on initial boot
compared to coming out of suspend might identify the problem.  If the
playback is working it's *probably* something missing in the CPU init
that's specific to capture.

Re: [alsa-devel] [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume hang

[Mark Brown]

On Fri, Nov 27, 2009 at 01:37:54PM +0530, Aggarwal, Anuj wrote:

> [Aggarwal, Anuj] We were able to fine tune NFS and use arecord to capture
> large files. But some more problems cropped up when tried to suspend /
> resume. Basic playback is working fine wrt suspend/resume. But capture,

Incidentally, it'd be good to get the fixes for the CODEC driver (at
least the infinite loop one) in sooner rather than later - there's a
clear bug there which it'd good to try to get into 2.6.32.

Re: [alsa-devel] [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume hang

[Philby John]

On Thu, 2009-11-26 at 20:52 +0530, Aggarwal, Anuj wrote:
> > -----Original Message-----
> > From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> > Sent: Thursday, November 26, 2009 8:41 PM
> > To: Aggarwal, Anuj
> > Cc: 'Troy Kisky'; alsa-devel@alsa-project.org; linux-omap@vger.kernel.org;
> > Arun KS
> > Subject: Re: [alsa-devel] [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume
> > hang
> > 
> > On Thu, Nov 26, 2009 at 08:26:44PM +0530, Aggarwal, Anuj wrote:
> > 
> > > driver, although I am not sure if the problem is in the audio driver.
> > > When tried to capture, using NFS as storage, it gives overrun error and
> > comes out with:
> > > arecord: pcm_read:1617: read error: Input/output error
> > 
> > > It happen always after ~20 sec, file size ~5MB. Tried with multiple
> > > configurations in arecord but no use.
> > > When tried: arecord -f cd /dev/null, it works fine. Same issue doesn't
> > > come too when I try to store the captured audio file on a MMC card.
> > 
> > > Any idea what could be the problem? Why arecord goes for a toss after a
> > > single overrun error and why it is happening always after ~20 sec? Is
> > > there something which can be tried to narrow down the problem?
> > 
> > Sounds like you've narrowed the problem down to a performance issue with
> > NFS writeout - it's probably having trouble keeping up with your I/O
> > rate.  This isn't 100% surprising with smaller systems, sometimes tuning
> > the NFS configuration can resolve the issue but sometimes the hardware
> > is just underspecified.
> [Aggarwal, Anuj] I am still surprised how this could be a NFS writeout issue
> as we are seeing a consistent read/write rate of 2Mbps over tftp. When dd
> command is used for read/write to further check NFS performance, 2Mbps for write and 4Mbps for read is observed.
> Does that still mean nfs is the culprit? What could be tweaked in audio/network driver to avoid this problem, any suggestions?

My two cents... could also try connecting the target directly to your
host machine (using nfs), through a cross-cable (one with the Tx and Rx
crossed). If no errors are seen, then it surely must be an attribution
of network congestion.

Regards,
Philby