[PATCH] Wait for all codecs to be ready if doing a cold reset

[PATCH] Wait for all codecs to be ready if doing a cold reset

[Thadeu Lima de Souza Cascardo]

If AC97_POWER_SAVE is enabled, intel8x0 does a cold reset when
initializing the codecs. While resuming, it does not wait for all codecs
to be active. Sound card does not work after a resume without it,
however. This patch fixes it.
---
 sound/pci/intel8x0.c |   24 ++++++++++++++----------
 1 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
index 048d99e..7228a0a 100644
--- a/sound/pci/intel8x0.c
+++ b/sound/pci/intel8x0.c
@@ -2335,16 +2335,6 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
 				   igetdword(chip, ICHREG(GLOB_STA)));
 			return -EIO;
 		}
-
-		/* wait for other codecs ready status. */
-		end_time = jiffies + HZ / 4;
-		while (status != chip->codec_isr_bits &&
-		       time_after_eq(end_time, jiffies)) {
-			schedule_timeout_uninterruptible(1);
-			status |= igetdword(chip, ICHREG(GLOB_STA)) &
-				chip->codec_isr_bits;
-		}
-
 	} else {
 		/* resume phase */
 		int i;
@@ -2363,6 +2353,20 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
 		} while (time_after_eq(end_time, jiffies));
 	}
 
+#ifndef CONFIG_SND_AC97_POWER_SAVE
+	if (probing)
+#endif
+	{
+		/* wait for other codecs ready status. */
+		end_time = jiffies + HZ / 4;
+		while (status != chip->codec_isr_bits &&
+		       time_after_eq(end_time, jiffies)) {
+			schedule_timeout_uninterruptible(1);
+			status |= igetdword(chip, ICHREG(GLOB_STA)) &
+				chip->codec_isr_bits;
+		}
+	}
+
 	if (chip->device_type == DEVICE_SIS) {
 		/* unmute the output on SIS7012 */
 		iputword(chip, 0x4c, igetword(chip, 0x4c) | 1);
-- 
1.5.6

Re: [PATCH] Wait for all codecs to be ready if doing a cold reset

[Takashi Iwai]

At Sun, 6 Jul 2008 14:15:56 -0300,
Thadeu Lima de Souza Cascardo wrote:
> 
> If AC97_POWER_SAVE is enabled, intel8x0 does a cold reset when
> initializing the codecs. While resuming, it does not wait for all codecs
> to be active. Sound card does not work after a resume without it,
> however. This patch fixes it.

Thanks for the patch.

But, I still don't figure out why this is needed.
In the else block (with the comment "resume phase"), you can find the
loop to wait for the all *probed* codecs.  The difference with the
code you moved is that it checks only the bits corresponding to the
already probed codecs.  In other words, the other bits should be
irrelevant with the hardware.

I guess it's not about the loop but the additional 1/4 sec delay that
did fix the resume on your device.  Please check how is the status
bits and whether it passed the loop in the middle.


Takashi


> ---
>  sound/pci/intel8x0.c |   24 ++++++++++++++----------
>  1 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
> index 048d99e..7228a0a 100644
> --- a/sound/pci/intel8x0.c
> +++ b/sound/pci/intel8x0.c
> @@ -2335,16 +2335,6 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
>  				   igetdword(chip, ICHREG(GLOB_STA)));
>  			return -EIO;
>  		}
> -
> -		/* wait for other codecs ready status. */
> -		end_time = jiffies + HZ / 4;
> -		while (status != chip->codec_isr_bits &&
> -		       time_after_eq(end_time, jiffies)) {
> -			schedule_timeout_uninterruptible(1);
> -			status |= igetdword(chip, ICHREG(GLOB_STA)) &
> -				chip->codec_isr_bits;
> -		}
> -
>  	} else {
>  		/* resume phase */
>  		int i;
> @@ -2363,6 +2353,20 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
>  		} while (time_after_eq(end_time, jiffies));
>  	}
>  
> +#ifndef CONFIG_SND_AC97_POWER_SAVE
> +	if (probing)
> +#endif
> +	{
> +		/* wait for other codecs ready status. */
> +		end_time = jiffies + HZ / 4;
> +		while (status != chip->codec_isr_bits &&
> +		       time_after_eq(end_time, jiffies)) {
> +			schedule_timeout_uninterruptible(1);
> +			status |= igetdword(chip, ICHREG(GLOB_STA)) &
> +				chip->codec_isr_bits;
> +		}
> +	}
> +
>  	if (chip->device_type == DEVICE_SIS) {
>  		/* unmute the output on SIS7012 */
>  		iputword(chip, 0x4c, igetword(chip, 0x4c) | 1);
> -- 
> 1.5.6
> 

Re: [PATCH] Wait for all codecs to be ready if doing a cold reset

[Thadeu Lima de Souza Cascardo]

[-- Attachment #1: Type: text/plain, Size: 3327 bytes --]

On Mon, Jul 07, 2008 at 06:39:09PM +0200, Takashi Iwai wrote:
> At Sun, 6 Jul 2008 14:15:56 -0300,
> Thadeu Lima de Souza Cascardo wrote:
> > 
> > If AC97_POWER_SAVE is enabled, intel8x0 does a cold reset when
> > initializing the codecs. While resuming, it does not wait for all codecs
> > to be active. Sound card does not work after a resume without it,
> > however. This patch fixes it.
> 
> Thanks for the patch.
> 
> But, I still don't figure out why this is needed.
> In the else block (with the comment "resume phase"), you can find the
> loop to wait for the all *probed* codecs.  The difference with the
> code you moved is that it checks only the bits corresponding to the
> already probed codecs.  In other words, the other bits should be
> irrelevant with the hardware.
> 
> I guess it's not about the loop but the additional 1/4 sec delay that
> did fix the resume on your device.  Please check how is the status
> bits and whether it passed the loop in the middle.
> 
> 
> Takashi
> 
> 

The 1/4 sec delay came in my mind as one of the possible reasons, and
that's why I've made some tests. status and nstatus are 0x200, while
codec_isr_bits is 0x300. The loop waits for the status register give us
0x300 as the active codecs, instead of the only one probed. Since the
cold reset in the case of a power saving cleans up all codec registers,
it is needed that we wait for all codecs again (like in the probe case).

If you need more information or that I ran any more tests, I'd be glad
to do it. By the way, I've tested the patch against your audio and modem
merged version and it works OK.

Best Regards,
Thadeu Cascardo.

> > ---
> >  sound/pci/intel8x0.c |   24 ++++++++++++++----------
> >  1 files changed, 14 insertions(+), 10 deletions(-)
> > 
> > diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
> > index 048d99e..7228a0a 100644
> > --- a/sound/pci/intel8x0.c
> > +++ b/sound/pci/intel8x0.c
> > @@ -2335,16 +2335,6 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
> >  				   igetdword(chip, ICHREG(GLOB_STA)));
> >  			return -EIO;
> >  		}
> > -
> > -		/* wait for other codecs ready status. */
> > -		end_time = jiffies + HZ / 4;
> > -		while (status != chip->codec_isr_bits &&
> > -		       time_after_eq(end_time, jiffies)) {
> > -			schedule_timeout_uninterruptible(1);
> > -			status |= igetdword(chip, ICHREG(GLOB_STA)) &
> > -				chip->codec_isr_bits;
> > -		}
> > -
> >  	} else {
> >  		/* resume phase */
> >  		int i;
> > @@ -2363,6 +2353,20 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
> >  		} while (time_after_eq(end_time, jiffies));
> >  	}
> >  
> > +#ifndef CONFIG_SND_AC97_POWER_SAVE
> > +	if (probing)
> > +#endif
> > +	{
> > +		/* wait for other codecs ready status. */
> > +		end_time = jiffies + HZ / 4;
> > +		while (status != chip->codec_isr_bits &&
> > +		       time_after_eq(end_time, jiffies)) {
> > +			schedule_timeout_uninterruptible(1);
> > +			status |= igetdword(chip, ICHREG(GLOB_STA)) &
> > +				chip->codec_isr_bits;
> > +		}
> > +	}
> > +
> >  	if (chip->device_type == DEVICE_SIS) {
> >  		/* unmute the output on SIS7012 */
> >  		iputword(chip, 0x4c, igetword(chip, 0x4c) | 1);
> > -- 
> > 1.5.6
> > 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Wait for all codecs to be ready if doing a cold reset

[Takashi Iwai]

At Mon, 7 Jul 2008 14:36:55 -0300,
Thadeu Lima de Souza Cascardo wrote:
> 
> On Mon, Jul 07, 2008 at 06:39:09PM +0200, Takashi Iwai wrote:
> > At Sun, 6 Jul 2008 14:15:56 -0300,
> > Thadeu Lima de Souza Cascardo wrote:
> > > 
> > > If AC97_POWER_SAVE is enabled, intel8x0 does a cold reset when
> > > initializing the codecs. While resuming, it does not wait for all codecs
> > > to be active. Sound card does not work after a resume without it,
> > > however. This patch fixes it.
> > 
> > Thanks for the patch.
> > 
> > But, I still don't figure out why this is needed.
> > In the else block (with the comment "resume phase"), you can find the
> > loop to wait for the all *probed* codecs.  The difference with the
> > code you moved is that it checks only the bits corresponding to the
> > already probed codecs.  In other words, the other bits should be
> > irrelevant with the hardware.
> > 
> > I guess it's not about the loop but the additional 1/4 sec delay that
> > did fix the resume on your device.  Please check how is the status
> > bits and whether it passed the loop in the middle.
> > 
> > 
> > Takashi
> > 
> > 
> 
> The 1/4 sec delay came in my mind as one of the possible reasons, and
> that's why I've made some tests. status and nstatus are 0x200, while
> codec_isr_bits is 0x300. The loop waits for the status register give us
> 0x300 as the active codecs, instead of the only one probed. Since the
> cold reset in the case of a power saving cleans up all codec registers,
> it is needed that we wait for all codecs again (like in the probe case).

You loaded the modem driver as well?
If so, what happens if you unload modem driver?


thanks,

Takashi


> If you need more information or that I ran any more tests, I'd be glad
> to do it. By the way, I've tested the patch against your audio and modem
> merged version and it works OK.
>
> Best Regards,
> Thadeu Cascardo.
> 
> > > ---
> > >  sound/pci/intel8x0.c |   24 ++++++++++++++----------
> > >  1 files changed, 14 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
> > > index 048d99e..7228a0a 100644
> > > --- a/sound/pci/intel8x0.c
> > > +++ b/sound/pci/intel8x0.c
> > > @@ -2335,16 +2335,6 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
> > >  				   igetdword(chip, ICHREG(GLOB_STA)));
> > >  			return -EIO;
> > >  		}
> > > -
> > > -		/* wait for other codecs ready status. */
> > > -		end_time = jiffies + HZ / 4;
> > > -		while (status != chip->codec_isr_bits &&
> > > -		       time_after_eq(end_time, jiffies)) {
> > > -			schedule_timeout_uninterruptible(1);
> > > -			status |= igetdword(chip, ICHREG(GLOB_STA)) &
> > > -				chip->codec_isr_bits;
> > > -		}
> > > -
> > >  	} else {
> > >  		/* resume phase */
> > >  		int i;
> > > @@ -2363,6 +2353,20 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
> > >  		} while (time_after_eq(end_time, jiffies));
> > >  	}
> > >  
> > > +#ifndef CONFIG_SND_AC97_POWER_SAVE
> > > +	if (probing)
> > > +#endif
> > > +	{
> > > +		/* wait for other codecs ready status. */
> > > +		end_time = jiffies + HZ / 4;
> > > +		while (status != chip->codec_isr_bits &&
> > > +		       time_after_eq(end_time, jiffies)) {
> > > +			schedule_timeout_uninterruptible(1);
> > > +			status |= igetdword(chip, ICHREG(GLOB_STA)) &
> > > +				chip->codec_isr_bits;
> > > +		}
> > > +	}
> > > +
> > >  	if (chip->device_type == DEVICE_SIS) {
> > >  		/* unmute the output on SIS7012 */
> > >  		iputword(chip, 0x4c, igetword(chip, 0x4c) | 1);
> > > -- 
> > > 1.5.6
> > > 

Re: [PATCH] Wait for all codecs to be ready if doing a cold reset

[Thadeu Lima de Souza Cascardo]

[-- Attachment #1: Type: text/plain, Size: 4048 bytes --]

On Tue, Jul 08, 2008 at 12:16:35PM +0200, Takashi Iwai wrote:
> At Mon, 7 Jul 2008 14:36:55 -0300,
> Thadeu Lima de Souza Cascardo wrote:
> > 
> > On Mon, Jul 07, 2008 at 06:39:09PM +0200, Takashi Iwai wrote:
> > > At Sun, 6 Jul 2008 14:15:56 -0300,
> > > Thadeu Lima de Souza Cascardo wrote:
> > > > 
> > > > If AC97_POWER_SAVE is enabled, intel8x0 does a cold reset when
> > > > initializing the codecs. While resuming, it does not wait for all codecs
> > > > to be active. Sound card does not work after a resume without it,
> > > > however. This patch fixes it.
> > > 
> > > Thanks for the patch.
> > > 
> > > But, I still don't figure out why this is needed.
> > > In the else block (with the comment "resume phase"), you can find the
> > > loop to wait for the all *probed* codecs.  The difference with the
> > > code you moved is that it checks only the bits corresponding to the
> > > already probed codecs.  In other words, the other bits should be
> > > irrelevant with the hardware.
> > > 
> > > I guess it's not about the loop but the additional 1/4 sec delay that
> > > did fix the resume on your device.  Please check how is the status
> > > bits and whether it passed the loop in the middle.
> > > 
> > > 
> > > Takashi
> > > 
> > > 
> > 
> > The 1/4 sec delay came in my mind as one of the possible reasons, and
> > that's why I've made some tests. status and nstatus are 0x200, while
> > codec_isr_bits is 0x300. The loop waits for the status register give us
> > 0x300 as the active codecs, instead of the only one probed. Since the
> > cold reset in the case of a power saving cleans up all codec registers,
> > it is needed that we wait for all codecs again (like in the probe case).
> 
> You loaded the modem driver as well?
> If so, what happens if you unload modem driver?
> 
> 
> thanks,
> 
> Takashi
> 

The modem driver has always been loaded. Unloading it with my patch
works OK.

> 
> > If you need more information or that I ran any more tests, I'd be glad
> > to do it. By the way, I've tested the patch against your audio and modem
> > merged version and it works OK.
> >
> > Best Regards,
> > Thadeu Cascardo.
> > 
> > > > ---
> > > >  sound/pci/intel8x0.c |   24 ++++++++++++++----------
> > > >  1 files changed, 14 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
> > > > index 048d99e..7228a0a 100644
> > > > --- a/sound/pci/intel8x0.c
> > > > +++ b/sound/pci/intel8x0.c
> > > > @@ -2335,16 +2335,6 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
> > > >  				   igetdword(chip, ICHREG(GLOB_STA)));
> > > >  			return -EIO;
> > > >  		}
> > > > -
> > > > -		/* wait for other codecs ready status. */
> > > > -		end_time = jiffies + HZ / 4;
> > > > -		while (status != chip->codec_isr_bits &&
> > > > -		       time_after_eq(end_time, jiffies)) {
> > > > -			schedule_timeout_uninterruptible(1);
> > > > -			status |= igetdword(chip, ICHREG(GLOB_STA)) &
> > > > -				chip->codec_isr_bits;
> > > > -		}
> > > > -
> > > >  	} else {
> > > >  		/* resume phase */
> > > >  		int i;
> > > > @@ -2363,6 +2353,20 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
> > > >  		} while (time_after_eq(end_time, jiffies));
> > > >  	}
> > > >  
> > > > +#ifndef CONFIG_SND_AC97_POWER_SAVE
> > > > +	if (probing)
> > > > +#endif
> > > > +	{
> > > > +		/* wait for other codecs ready status. */
> > > > +		end_time = jiffies + HZ / 4;
> > > > +		while (status != chip->codec_isr_bits &&
> > > > +		       time_after_eq(end_time, jiffies)) {
> > > > +			schedule_timeout_uninterruptible(1);
> > > > +			status |= igetdword(chip, ICHREG(GLOB_STA)) &
> > > > +				chip->codec_isr_bits;
> > > > +		}
> > > > +	}
> > > > +
> > > >  	if (chip->device_type == DEVICE_SIS) {
> > > >  		/* unmute the output on SIS7012 */
> > > >  		iputword(chip, 0x4c, igetword(chip, 0x4c) | 1);
> > > > -- 
> > > > 1.5.6
> > > > 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Wait for all codecs to be ready if doing a cold reset

[Takashi Iwai]

At Tue, 8 Jul 2008 13:59:32 -0300,
Thadeu Lima de Souza Cascardo wrote:
> 
> On Tue, Jul 08, 2008 at 12:16:35PM +0200, Takashi Iwai wrote:
> > At Mon, 7 Jul 2008 14:36:55 -0300,
> > Thadeu Lima de Souza Cascardo wrote:
> > > 
> > > On Mon, Jul 07, 2008 at 06:39:09PM +0200, Takashi Iwai wrote:
> > > > At Sun, 6 Jul 2008 14:15:56 -0300,
> > > > Thadeu Lima de Souza Cascardo wrote:
> > > > > 
> > > > > If AC97_POWER_SAVE is enabled, intel8x0 does a cold reset when
> > > > > initializing the codecs. While resuming, it does not wait for all codecs
> > > > > to be active. Sound card does not work after a resume without it,
> > > > > however. This patch fixes it.
> > > > 
> > > > Thanks for the patch.
> > > > 
> > > > But, I still don't figure out why this is needed.
> > > > In the else block (with the comment "resume phase"), you can find the
> > > > loop to wait for the all *probed* codecs.  The difference with the
> > > > code you moved is that it checks only the bits corresponding to the
> > > > already probed codecs.  In other words, the other bits should be
> > > > irrelevant with the hardware.
> > > > 
> > > > I guess it's not about the loop but the additional 1/4 sec delay that
> > > > did fix the resume on your device.  Please check how is the status
> > > > bits and whether it passed the loop in the middle.
> > > > 
> > > > 
> > > > Takashi
> > > > 
> > > > 
> > > 
> > > The 1/4 sec delay came in my mind as one of the possible reasons, and
> > > that's why I've made some tests. status and nstatus are 0x200, while
> > > codec_isr_bits is 0x300. The loop waits for the status register give us
> > > 0x300 as the active codecs, instead of the only one probed. Since the
> > > cold reset in the case of a power saving cleans up all codec registers,
> > > it is needed that we wait for all codecs again (like in the probe case).
> > 
> > You loaded the modem driver as well?
> > If so, what happens if you unload modem driver?
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> 
> The modem driver has always been loaded. Unloading it with my patch
> works OK.

Well, I meant to unload the modem driver *without* your patch.
That is, does it the unmodified version work if you unload the modem
driver beforehand (e.g. adding it to blacklist)?


thanks,

Takashi

> > > If you need more information or that I ran any more tests, I'd be glad
> > > to do it. By the way, I've tested the patch against your audio and modem
> > > merged version and it works OK.
> > >
> > > Best Regards,
> > > Thadeu Cascardo.
> > > 
> > > > > ---
> > > > >  sound/pci/intel8x0.c |   24 ++++++++++++++----------
> > > > >  1 files changed, 14 insertions(+), 10 deletions(-)
> > > > > 
> > > > > diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
> > > > > index 048d99e..7228a0a 100644
> > > > > --- a/sound/pci/intel8x0.c
> > > > > +++ b/sound/pci/intel8x0.c
> > > > > @@ -2335,16 +2335,6 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
> > > > >  				   igetdword(chip, ICHREG(GLOB_STA)));
> > > > >  			return -EIO;
> > > > >  		}
> > > > > -
> > > > > -		/* wait for other codecs ready status. */
> > > > > -		end_time = jiffies + HZ / 4;
> > > > > -		while (status != chip->codec_isr_bits &&
> > > > > -		       time_after_eq(end_time, jiffies)) {
> > > > > -			schedule_timeout_uninterruptible(1);
> > > > > -			status |= igetdword(chip, ICHREG(GLOB_STA)) &
> > > > > -				chip->codec_isr_bits;
> > > > > -		}
> > > > > -
> > > > >  	} else {
> > > > >  		/* resume phase */
> > > > >  		int i;
> > > > > @@ -2363,6 +2353,20 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
> > > > >  		} while (time_after_eq(end_time, jiffies));
> > > > >  	}
> > > > >  
> > > > > +#ifndef CONFIG_SND_AC97_POWER_SAVE
> > > > > +	if (probing)
> > > > > +#endif
> > > > > +	{
> > > > > +		/* wait for other codecs ready status. */
> > > > > +		end_time = jiffies + HZ / 4;
> > > > > +		while (status != chip->codec_isr_bits &&
> > > > > +		       time_after_eq(end_time, jiffies)) {
> > > > > +			schedule_timeout_uninterruptible(1);
> > > > > +			status |= igetdword(chip, ICHREG(GLOB_STA)) &
> > > > > +				chip->codec_isr_bits;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > >  	if (chip->device_type == DEVICE_SIS) {
> > > > >  		/* unmute the output on SIS7012 */
> > > > >  		iputword(chip, 0x4c, igetword(chip, 0x4c) | 1);
> > > > > -- 
> > > > > 1.5.6
> > > > > 

Re: [PATCH] Wait for all codecs to be ready if doing a cold reset

[Thadeu Lima de Souza Cascardo]

[-- Attachment #1: Type: text/plain, Size: 4968 bytes --]

On Wed, Jul 09, 2008 at 05:07:17PM +0200, Takashi Iwai wrote:
> At Tue, 8 Jul 2008 13:59:32 -0300,
> Thadeu Lima de Souza Cascardo wrote:
> > 
> > On Tue, Jul 08, 2008 at 12:16:35PM +0200, Takashi Iwai wrote:
> > > At Mon, 7 Jul 2008 14:36:55 -0300,
> > > Thadeu Lima de Souza Cascardo wrote:
> > > > 
> > > > On Mon, Jul 07, 2008 at 06:39:09PM +0200, Takashi Iwai wrote:
> > > > > At Sun, 6 Jul 2008 14:15:56 -0300,
> > > > > Thadeu Lima de Souza Cascardo wrote:
> > > > > > 
> > > > > > If AC97_POWER_SAVE is enabled, intel8x0 does a cold reset when
> > > > > > initializing the codecs. While resuming, it does not wait for all codecs
> > > > > > to be active. Sound card does not work after a resume without it,
> > > > > > however. This patch fixes it.
> > > > > 
> > > > > Thanks for the patch.
> > > > > 
> > > > > But, I still don't figure out why this is needed.
> > > > > In the else block (with the comment "resume phase"), you can find the
> > > > > loop to wait for the all *probed* codecs.  The difference with the
> > > > > code you moved is that it checks only the bits corresponding to the
> > > > > already probed codecs.  In other words, the other bits should be
> > > > > irrelevant with the hardware.
> > > > > 
> > > > > I guess it's not about the loop but the additional 1/4 sec delay that
> > > > > did fix the resume on your device.  Please check how is the status
> > > > > bits and whether it passed the loop in the middle.
> > > > > 
> > > > > 
> > > > > Takashi
> > > > > 
> > > > > 
> > > > 
> > > > The 1/4 sec delay came in my mind as one of the possible reasons, and
> > > > that's why I've made some tests. status and nstatus are 0x200, while
> > > > codec_isr_bits is 0x300. The loop waits for the status register give us
> > > > 0x300 as the active codecs, instead of the only one probed. Since the
> > > > cold reset in the case of a power saving cleans up all codec registers,
> > > > it is needed that we wait for all codecs again (like in the probe case).
> > > 
> > > You loaded the modem driver as well?
> > > If so, what happens if you unload modem driver?
> > > 
> > > 
> > > thanks,
> > > 
> > > Takashi
> > > 
> > 
> > The modem driver has always been loaded. Unloading it with my patch
> > works OK.
> 
> Well, I meant to unload the modem driver *without* your patch.
> That is, does it the unmodified version work if you unload the modem
> driver beforehand (e.g. adding it to blacklist)?
> 
> 
> thanks,
> 
> Takashi
> 

I blacklisted snd_intel8x0m, rebooted, it was not loaded, sound worked.
Suspended, resumed, sound didn't work, as usual.

> > > > If you need more information or that I ran any more tests, I'd be glad
> > > > to do it. By the way, I've tested the patch against your audio and modem
> > > > merged version and it works OK.
> > > >
> > > > Best Regards,
> > > > Thadeu Cascardo.
> > > > 
> > > > > > ---
> > > > > >  sound/pci/intel8x0.c |   24 ++++++++++++++----------
> > > > > >  1 files changed, 14 insertions(+), 10 deletions(-)
> > > > > > 
> > > > > > diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
> > > > > > index 048d99e..7228a0a 100644
> > > > > > --- a/sound/pci/intel8x0.c
> > > > > > +++ b/sound/pci/intel8x0.c
> > > > > > @@ -2335,16 +2335,6 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
> > > > > >  				   igetdword(chip, ICHREG(GLOB_STA)));
> > > > > >  			return -EIO;
> > > > > >  		}
> > > > > > -
> > > > > > -		/* wait for other codecs ready status. */
> > > > > > -		end_time = jiffies + HZ / 4;
> > > > > > -		while (status != chip->codec_isr_bits &&
> > > > > > -		       time_after_eq(end_time, jiffies)) {
> > > > > > -			schedule_timeout_uninterruptible(1);
> > > > > > -			status |= igetdword(chip, ICHREG(GLOB_STA)) &
> > > > > > -				chip->codec_isr_bits;
> > > > > > -		}
> > > > > > -
> > > > > >  	} else {
> > > > > >  		/* resume phase */
> > > > > >  		int i;
> > > > > > @@ -2363,6 +2353,20 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
> > > > > >  		} while (time_after_eq(end_time, jiffies));
> > > > > >  	}
> > > > > >  
> > > > > > +#ifndef CONFIG_SND_AC97_POWER_SAVE
> > > > > > +	if (probing)
> > > > > > +#endif
> > > > > > +	{
> > > > > > +		/* wait for other codecs ready status. */
> > > > > > +		end_time = jiffies + HZ / 4;
> > > > > > +		while (status != chip->codec_isr_bits &&
> > > > > > +		       time_after_eq(end_time, jiffies)) {
> > > > > > +			schedule_timeout_uninterruptible(1);
> > > > > > +			status |= igetdword(chip, ICHREG(GLOB_STA)) &
> > > > > > +				chip->codec_isr_bits;
> > > > > > +		}
> > > > > > +	}
> > > > > > +
> > > > > >  	if (chip->device_type == DEVICE_SIS) {
> > > > > >  		/* unmute the output on SIS7012 */
> > > > > >  		iputword(chip, 0x4c, igetword(chip, 0x4c) | 1);
> > > > > > -- 
> > > > > > 1.5.6
> > > > > > 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Wait for all codecs to be ready if doing a cold reset

[Thadeu Lima de Souza Cascardo]

[-- Attachment #1: Type: text/plain, Size: 3903 bytes --]

On Wed, Jul 09, 2008 at 08:23:12PM +0200, Takashi Iwai wrote:
> At Tue, 8 Jul 2008 14:31:22 -0300,
> Thadeu Lima de Souza Cascardo wrote:
> > 
> > On Wed, Jul 09, 2008 at 05:07:17PM +0200, Takashi Iwai wrote:
> > > At Tue, 8 Jul 2008 13:59:32 -0300,
> > > Thadeu Lima de Souza Cascardo wrote:
> > > > 
> > > > On Tue, Jul 08, 2008 at 12:16:35PM +0200, Takashi Iwai wrote:
> > > > > At Mon, 7 Jul 2008 14:36:55 -0300,
> > > > > Thadeu Lima de Souza Cascardo wrote:
> > > > > > 
> > > > > > On Mon, Jul 07, 2008 at 06:39:09PM +0200, Takashi Iwai wrote:
> > > > > > > At Sun, 6 Jul 2008 14:15:56 -0300,
> > > > > > > Thadeu Lima de Souza Cascardo wrote:
> > > > > > > > 
> > > > > > > > If AC97_POWER_SAVE is enabled, intel8x0 does a cold reset when
> > > > > > > > initializing the codecs. While resuming, it does not wait for all codecs
> > > > > > > > to be active. Sound card does not work after a resume without it,
> > > > > > > > however. This patch fixes it.
> > > > > > > 
> > > > > > > Thanks for the patch.
> > > > > > > 
> > > > > > > But, I still don't figure out why this is needed.
> > > > > > > In the else block (with the comment "resume phase"), you can find the
> > > > > > > loop to wait for the all *probed* codecs.  The difference with the
> > > > > > > code you moved is that it checks only the bits corresponding to the
> > > > > > > already probed codecs.  In other words, the other bits should be
> > > > > > > irrelevant with the hardware.
> > > > > > > 
> > > > > > > I guess it's not about the loop but the additional 1/4 sec delay that
> > > > > > > did fix the resume on your device.  Please check how is the status
> > > > > > > bits and whether it passed the loop in the middle.
> > > > > > > 
> > > > > > > 
> > > > > > > Takashi
> > > > > > > 
> > > > > > > 
> > > > > > 
> > > > > > The 1/4 sec delay came in my mind as one of the possible reasons, and
> > > > > > that's why I've made some tests. status and nstatus are 0x200, while
> > > > > > codec_isr_bits is 0x300. The loop waits for the status register give us
> > > > > > 0x300 as the active codecs, instead of the only one probed. Since the
> > > > > > cold reset in the case of a power saving cleans up all codec registers,
> > > > > > it is needed that we wait for all codecs again (like in the probe case).
> > > > > 
> > > > > You loaded the modem driver as well?
> > > > > If so, what happens if you unload modem driver?
> > > > > 
> > > > > 
> > > > > thanks,
> > > > > 
> > > > > Takashi
> > > > > 
> > > > 
> > > > The modem driver has always been loaded. Unloading it with my patch
> > > > works OK.
> > > 
> > > Well, I meant to unload the modem driver *without* your patch.
> > > That is, does it the unmodified version work if you unload the modem
> > > driver beforehand (e.g. adding it to blacklist)?
> > > 
> > > 
> > > thanks,
> > > 
> > > Takashi
> > > 
> > 
> > I blacklisted snd_intel8x0m, rebooted, it was not loaded, sound worked.
> > Suspended, resumed, sound didn't work, as usual.
> 
> Thanks for checking.
> 
> I still don't understand why you need to wait for all codecs.
> Certainly the secondary one is the modem codec, and it has nothing to
> do with the audio codec...
> 
> I don't mind to apply the patch as is since it's harmless except for
> the extra delay.  But, the real question is whether it's the codec
> probe or just another delay.
> 
> Note that not all devices have codecs filling all slots like yours.
> On the later ICH, it has three slots while usually two of them are
> used.  So, on these hardwares, your code results always in just a 25ms
> delay.
> 
> Anyway, could you give your sign-off?
> 
> 
> thanks,
> 
> Takashi


Maybe we could treat this as a quirk in my device, which is 8086:2485?

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@minaslivre.org>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Wait for all codecs to be ready if doing a cold reset

[Takashi Iwai]

At Wed, 9 Jul 2008 14:26:38 -0300,
Thadeu Lima de Souza Cascardo wrote:
> 
> On Wed, Jul 09, 2008 at 08:23:12PM +0200, Takashi Iwai wrote:
> > At Tue, 8 Jul 2008 14:31:22 -0300,
> > Thadeu Lima de Souza Cascardo wrote:
> > > 
> > > On Wed, Jul 09, 2008 at 05:07:17PM +0200, Takashi Iwai wrote:
> > > > At Tue, 8 Jul 2008 13:59:32 -0300,
> > > > Thadeu Lima de Souza Cascardo wrote:
> > > > > 
> > > > > On Tue, Jul 08, 2008 at 12:16:35PM +0200, Takashi Iwai wrote:
> > > > > > At Mon, 7 Jul 2008 14:36:55 -0300,
> > > > > > Thadeu Lima de Souza Cascardo wrote:
> > > > > > > 
> > > > > > > On Mon, Jul 07, 2008 at 06:39:09PM +0200, Takashi Iwai wrote:
> > > > > > > > At Sun, 6 Jul 2008 14:15:56 -0300,
> > > > > > > > Thadeu Lima de Souza Cascardo wrote:
> > > > > > > > > 
> > > > > > > > > If AC97_POWER_SAVE is enabled, intel8x0 does a cold reset when
> > > > > > > > > initializing the codecs. While resuming, it does not wait for all codecs
> > > > > > > > > to be active. Sound card does not work after a resume without it,
> > > > > > > > > however. This patch fixes it.
> > > > > > > > 
> > > > > > > > Thanks for the patch.
> > > > > > > > 
> > > > > > > > But, I still don't figure out why this is needed.
> > > > > > > > In the else block (with the comment "resume phase"), you can find the
> > > > > > > > loop to wait for the all *probed* codecs.  The difference with the
> > > > > > > > code you moved is that it checks only the bits corresponding to the
> > > > > > > > already probed codecs.  In other words, the other bits should be
> > > > > > > > irrelevant with the hardware.
> > > > > > > > 
> > > > > > > > I guess it's not about the loop but the additional 1/4 sec delay that
> > > > > > > > did fix the resume on your device.  Please check how is the status
> > > > > > > > bits and whether it passed the loop in the middle.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > Takashi
> > > > > > > > 
> > > > > > > > 
> > > > > > > 
> > > > > > > The 1/4 sec delay came in my mind as one of the possible reasons, and
> > > > > > > that's why I've made some tests. status and nstatus are 0x200, while
> > > > > > > codec_isr_bits is 0x300. The loop waits for the status register give us
> > > > > > > 0x300 as the active codecs, instead of the only one probed. Since the
> > > > > > > cold reset in the case of a power saving cleans up all codec registers,
> > > > > > > it is needed that we wait for all codecs again (like in the probe case).
> > > > > > 
> > > > > > You loaded the modem driver as well?
> > > > > > If so, what happens if you unload modem driver?
> > > > > > 
> > > > > > 
> > > > > > thanks,
> > > > > > 
> > > > > > Takashi
> > > > > > 
> > > > > 
> > > > > The modem driver has always been loaded. Unloading it with my patch
> > > > > works OK.
> > > > 
> > > > Well, I meant to unload the modem driver *without* your patch.
> > > > That is, does it the unmodified version work if you unload the modem
> > > > driver beforehand (e.g. adding it to blacklist)?
> > > > 
> > > > 
> > > > thanks,
> > > > 
> > > > Takashi
> > > > 
> > > 
> > > I blacklisted snd_intel8x0m, rebooted, it was not loaded, sound worked.
> > > Suspended, resumed, sound didn't work, as usual.
> > 
> > Thanks for checking.
> > 
> > I still don't understand why you need to wait for all codecs.
> > Certainly the secondary one is the modem codec, and it has nothing to
> > do with the audio codec...
> > 
> > I don't mind to apply the patch as is since it's harmless except for
> > the extra delay.  But, the real question is whether it's the codec
> > probe or just another delay.
> > 
> > Note that not all devices have codecs filling all slots like yours.
> > On the later ICH, it has three slots while usually two of them are
> > used.  So, on these hardwares, your code results always in just a 25ms
> > delay.
> > 
> > Anyway, could you give your sign-off?
> > 
> > 
> > thanks,
> > 
> > Takashi
> 
> 
> Maybe we could treat this as a quirk in my device, which is 8086:2485?
> 
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@minaslivre.org>

Thanks.

Meanwhile, I reconsidered about this problem.  I think the check of
the whole "active" codec slots can be done better in a way like the
following.  Could you give it a try?

If this still doesn't work, I suspect it's really the matter of
additional delay.


thanks,

Takashi
---
diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
index 048d99e..a282c7c 100644
--- a/sound/pci/intel8x0.c
+++ b/sound/pci/intel8x0.c
@@ -404,6 +404,7 @@ struct intel8x0 {
 	unsigned int *codec_bit;
 	unsigned int codec_isr_bits;
 	unsigned int codec_ready_bits;
+	unsigned int codec_init_bits;
 
 	spinlock_t reg_lock;
 	
@@ -2278,7 +2279,7 @@ static void do_ali_reset(struct intel8x0 *chip)
 static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
 {
 	unsigned long end_time;
-	unsigned int cnt, status, nstatus;
+	unsigned int cnt, status;
 	
 	/* put logic to right state */
 	/* first clear status bits */
@@ -2344,20 +2345,15 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
 			status |= igetdword(chip, ICHREG(GLOB_STA)) &
 				chip->codec_isr_bits;
 		}
-
+		chip->codec_init_bits = status;
 	} else {
 		/* resume phase */
-		int i;
-		status = 0;
-		for (i = 0; i < chip->ncodecs; i++)
-			if (chip->ac97[i])
-				status |= chip->codec_bit[chip->ac97_sdin[i]];
 		/* wait until all the probed codecs are ready */
 		end_time = jiffies + HZ;
 		do {
-			nstatus = igetdword(chip, ICHREG(GLOB_STA)) &
+			status = igetdword(chip, ICHREG(GLOB_STA)) &
 				chip->codec_isr_bits;
-			if (status == nstatus)
+			if (status == chip->codec_init_bits)
 				break;
 			schedule_timeout_uninterruptible(1);
 		} while (time_after_eq(end_time, jiffies));

Re: [PATCH] Wait for all codecs to be ready if doing a cold reset

[Thadeu Lima de Souza Cascardo]

[-- Attachment #1: Type: text/plain, Size: 9739 bytes --]

On Wed, Jul 16, 2008 at 01:47:03AM +0200, Takashi Iwai wrote:
> At Wed, 9 Jul 2008 14:26:38 -0300,
> Thadeu Lima de Souza Cascardo wrote:
> > 
> > On Wed, Jul 09, 2008 at 08:23:12PM +0200, Takashi Iwai wrote:
> > > At Tue, 8 Jul 2008 14:31:22 -0300,
> > > Thadeu Lima de Souza Cascardo wrote:
> > > > 
> > > > On Wed, Jul 09, 2008 at 05:07:17PM +0200, Takashi Iwai wrote:
> > > > > At Tue, 8 Jul 2008 13:59:32 -0300,
> > > > > Thadeu Lima de Souza Cascardo wrote:
> > > > > > 
> > > > > > On Tue, Jul 08, 2008 at 12:16:35PM +0200, Takashi Iwai wrote:
> > > > > > > At Mon, 7 Jul 2008 14:36:55 -0300,
> > > > > > > Thadeu Lima de Souza Cascardo wrote:
> > > > > > > > 
> > > > > > > > On Mon, Jul 07, 2008 at 06:39:09PM +0200, Takashi Iwai wrote:
> > > > > > > > > At Sun, 6 Jul 2008 14:15:56 -0300,
> > > > > > > > > Thadeu Lima de Souza Cascardo wrote:
> > > > > > > > > > 
> > > > > > > > > > If AC97_POWER_SAVE is enabled, intel8x0 does a cold reset when
> > > > > > > > > > initializing the codecs. While resuming, it does not wait for all codecs
> > > > > > > > > > to be active. Sound card does not work after a resume without it,
> > > > > > > > > > however. This patch fixes it.
> > > > > > > > > 
> > > > > > > > > Thanks for the patch.
> > > > > > > > > 
> > > > > > > > > But, I still don't figure out why this is needed.
> > > > > > > > > In the else block (with the comment "resume phase"), you can find the
> > > > > > > > > loop to wait for the all *probed* codecs.  The difference with the
> > > > > > > > > code you moved is that it checks only the bits corresponding to the
> > > > > > > > > already probed codecs.  In other words, the other bits should be
> > > > > > > > > irrelevant with the hardware.
> > > > > > > > > 
> > > > > > > > > I guess it's not about the loop but the additional 1/4 sec delay that
> > > > > > > > > did fix the resume on your device.  Please check how is the status
> > > > > > > > > bits and whether it passed the loop in the middle.
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Takashi
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > The 1/4 sec delay came in my mind as one of the possible reasons, and
> > > > > > > > that's why I've made some tests. status and nstatus are 0x200, while
> > > > > > > > codec_isr_bits is 0x300. The loop waits for the status register give us
> > > > > > > > 0x300 as the active codecs, instead of the only one probed. Since the
> > > > > > > > cold reset in the case of a power saving cleans up all codec registers,
> > > > > > > > it is needed that we wait for all codecs again (like in the probe case).
> > > > > > > 
> > > > > > > You loaded the modem driver as well?
> > > > > > > If so, what happens if you unload modem driver?
> > > > > > > 
> > > > > > > 
> > > > > > > thanks,
> > > > > > > 
> > > > > > > Takashi
> > > > > > > 
> > > > > > 
> > > > > > The modem driver has always been loaded. Unloading it with my patch
> > > > > > works OK.
> > > > > 
> > > > > Well, I meant to unload the modem driver *without* your patch.
> > > > > That is, does it the unmodified version work if you unload the modem
> > > > > driver beforehand (e.g. adding it to blacklist)?
> > > > > 
> > > > > 
> > > > > thanks,
> > > > > 
> > > > > Takashi
> > > > > 
> > > > 
> > > > I blacklisted snd_intel8x0m, rebooted, it was not loaded, sound worked.
> > > > Suspended, resumed, sound didn't work, as usual.
> > > 
> > > Thanks for checking.
> > > 
> > > I still don't understand why you need to wait for all codecs.
> > > Certainly the secondary one is the modem codec, and it has nothing to
> > > do with the audio codec...
> > > 
> > > I don't mind to apply the patch as is since it's harmless except for
> > > the extra delay.  But, the real question is whether it's the codec
> > > probe or just another delay.
> > > 
> > > Note that not all devices have codecs filling all slots like yours.
> > > On the later ICH, it has three slots while usually two of them are
> > > used.  So, on these hardwares, your code results always in just a 25ms
> > > delay.
> > > 
> > > Anyway, could you give your sign-off?
> > > 
> > > 
> > > thanks,
> > > 
> > > Takashi
> > 
> > 
> > Maybe we could treat this as a quirk in my device, which is 8086:2485?
> > 
> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@minaslivre.org>
> 
> Thanks.
> 
> Meanwhile, I reconsidered about this problem.  I think the check of
> the whole "active" codec slots can be done better in a way like the
> following.  Could you give it a try?
> 
> If this still doesn't work, I suspect it's really the matter of
> additional delay.
> 

It did not work, but it is not really a matter of additional delay
either. Perhaps, when resuming, we are not doing something else. Here is
what I've done:

I printed codec_isr_bits, codec_init_bits and status when probind and
when resuming.

--
diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
index a282c7c..9ab8383 100644
--- a/sound/pci/intel8x0.c
+++ b/sound/pci/intel8x0.c
@@ -2346,6 +2346,7 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
 				chip->codec_isr_bits;
 		}
 		chip->codec_init_bits = status;
+               snd_printk ("cascardo - resume: %x %x %x\n", chip->codec_isr_bits, chip->codec_init_bits, status);
 	} else {
 		/* resume phase */
 		/* wait until all the probed codecs are ready */
@@ -2357,6 +2358,7 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
 				break;
 			schedule_timeout_uninterruptible(1);
 		} while (time_after_eq(end_time, jiffies));
+               snd_printk ("cascardo - resume: %x %x %x\n", chip->codec_isr_bits, chip->codec_init_bits, status);
 	}
 
 	if (chip->device_type == DEVICE_SIS) {
--

Here is what I've got:

cascardo@vespa:~/code/linux/build/alsa$ dmesg | grep cascardo
[ 1835.512078] cascardo - probe: 300 200 200
[ 1848.511084] cascardo - resume: 300 200 200

Then, I've waited for codec_isr_bits, instead of codec_init_bits when
resuming, and here is what I've got.


[ 2033.909078] cascardo - probe: 300 200 200
[ 2069.692183] cascardo - resume: 300 200 300
cascardo@vespa:~/code/linux/build/alsa$


Considering that status is 0x0200 when probing, which means we spend
those HZ/4 for nothing, and the driver works, there is something the
driver should do when resuming so it could work with status 0x0200.
However, we wait for 0x0300 and we get it in less than one second.

Remember this only happens when there is a cold reset while resuming. If
not cold reseting, here is what I get:

[ 3855.656460] cascardo - probe: 300 300 300
[ 3866.633160] cascardo - resume: 300 300 300

Printing the control register the driver writes in this section of the
code:

--
@@ -2305,3 +2305,3 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
        /* finish cold or do warm reset */
        cnt |= (cnt & ICH_AC97COLD) == 0 ? ICH_AC97COLD : ICH_AC97WARM;
        iputdword(chip, ICHREG(GLOB_CNT), cnt);
--

I've got
[ 4261.231603] cascardo - reset: 0x6
[ 4269.748091] cascardo - reset: 0x2

The first while probing (which means we do a warm reset) and the second
while resuming, which does not mean a cold reset, but finishing a cold
reset. All this is with ac97 power save enabled:

cascardo@vespa:~/code/linux/build/alsa$ cat /sys/module/snd_ac97_codec/parameters/power_save
Y
cascardo@vespa:~/code/linux/build/alsa$

This version I've built does not have your patch for power_save to be an
integer with the timeout in seconds. So this works as a timeout of 2
seconds. I've tried the driver with 4 seconds of timeout, without my
patch, and it didn't work either. So I don't think this timeout is
related.

So, my question is: is there any device (perhaps the later ICH) that
needs a cold reset with power_save enabled? Mine didn't and, perhaps, we
should simply remove that ifdef and do the reset as usual, which will
work out for me.

Thanks for the attention,
Thadeu Cascardo.

> 
> thanks,
> 
> Takashi
> ---
> diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
> index 048d99e..a282c7c 100644
> --- a/sound/pci/intel8x0.c
> +++ b/sound/pci/intel8x0.c
> @@ -404,6 +404,7 @@ struct intel8x0 {
>  	unsigned int *codec_bit;
>  	unsigned int codec_isr_bits;
>  	unsigned int codec_ready_bits;
> +	unsigned int codec_init_bits;
>  
>  	spinlock_t reg_lock;
>  	
> @@ -2278,7 +2279,7 @@ static void do_ali_reset(struct intel8x0 *chip)
>  static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
>  {
>  	unsigned long end_time;
> -	unsigned int cnt, status, nstatus;
> +	unsigned int cnt, status;
>  	
>  	/* put logic to right state */
>  	/* first clear status bits */
> @@ -2344,20 +2345,15 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
>  			status |= igetdword(chip, ICHREG(GLOB_STA)) &
>  				chip->codec_isr_bits;
>  		}
> -
> +		chip->codec_init_bits = status;
>  	} else {
>  		/* resume phase */
> -		int i;
> -		status = 0;
> -		for (i = 0; i < chip->ncodecs; i++)
> -			if (chip->ac97[i])
> -				status |= chip->codec_bit[chip->ac97_sdin[i]];
>  		/* wait until all the probed codecs are ready */
>  		end_time = jiffies + HZ;
>  		do {
> -			nstatus = igetdword(chip, ICHREG(GLOB_STA)) &
> +			status = igetdword(chip, ICHREG(GLOB_STA)) &
>  				chip->codec_isr_bits;
> -			if (status == nstatus)
> +			if (status == chip->codec_init_bits)
>  				break;
>  			schedule_timeout_uninterruptible(1);
>  		} while (time_after_eq(end_time, jiffies));

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Wait for all codecs to be ready if doing a cold reset

[Thadeu Lima de Souza Cascardo]

[-- Attachment #1: Type: text/plain, Size: 2598 bytes --]

On Mon, Jul 14, 2008 at 09:10:39PM -0300, Thadeu Lima de Souza Cascardo wrote:
> On Wed, Jul 16, 2008 at 01:47:03AM +0200, Takashi Iwai wrote:
> > Thanks.
> > 
> > Meanwhile, I reconsidered about this problem.  I think the check of
> > the whole "active" codec slots can be done better in a way like the
> > following.  Could you give it a try?
> > 
> > If this still doesn't work, I suspect it's really the matter of
> > additional delay.
> > 

I've put some more effort on this issue last weekend. Here is the
result:

The codec I have is AD1881A. The additional delay makes a pretty
difference, right. Only the delay is enough to solve it.

However, I agree this is not the right solution. The codec_init_bits
solution does not work.

I have diff'ed the value of the codec registers and they are different
from all the other situations I tested, that are:

* booted my notebook: OK
* suspended, resumed, reloaded the driver: OK
* loaded the driver with no cold reset
  (no AC97_POWER_SAVE on intel8x0): OK
* same as above, after resuming: OK
* AC97_POWER_SAVE enabled (cold reset), after resume: NOT ok

As previously reported, even when probing, after loading the driver, we
get a different behaviour if we cold reset. When probing, we wait for
those HZ/4, which may explain why everything works.

I tested if ac97_resume was writing the same values to the same
registers if I had the delay or if I didn't. It was. However, the values
I read from /proc were still different, very similar to what the codec
specification refered to as the default values.

The best solution would be to do whatever is needed to do after cold
resetting that we are not doing, unless it is a hardware issue and the
driver can't really do anything about it but wait. Reading the Intel
ICH3 doc and AD1881A doc, I couldn't figure out anything that the driver
is doing wrong.

So, right now, the alternatives amount to:

* Wait for the HZ/4 additional delay. Probing is doing it and my
previously submitted patch does not add any delay for the probe case
and, it the resume case, it only adds a delay if we are cold resetting,
that is, AC97_POWER_SAVE is enabled.

* Do not do the cold reset at all. It works pretty well for me if ac97
power save is enabled and we only do a warm reset.

* Do only one of the above as a quirk for my controller or codec or
their combination.


Any help trying to figure out what's really happening, I'd appreciate.
If any more details of my tests are needed, I can collect and publish
them.

Regards,
Thadeu Cascardo.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Wait for all codecs to be ready if doing a cold reset

[Takashi Iwai]

At Wed, 17 Dec 2008 20:08:30 -0200,
Thadeu Lima de Souza Cascardo wrote:
> 
> On Mon, Jul 14, 2008 at 09:10:39PM -0300, Thadeu Lima de Souza Cascardo wrote:
> > On Wed, Jul 16, 2008 at 01:47:03AM +0200, Takashi Iwai wrote:
> > > Thanks.
> > > 
> > > Meanwhile, I reconsidered about this problem.  I think the check of
> > > the whole "active" codec slots can be done better in a way like the
> > > following.  Could you give it a try?
> > > 
> > > If this still doesn't work, I suspect it's really the matter of
> > > additional delay.
> > > 
> 
> I've put some more effort on this issue last weekend. Here is the
> result:
> 
> The codec I have is AD1881A. The additional delay makes a pretty
> difference, right. Only the delay is enough to solve it.

It's a good news, at least.

> However, I agree this is not the right solution. The codec_init_bits
> solution does not work.
> 
> I have diff'ed the value of the codec registers and they are different
> from all the other situations I tested, that are:
> 
> * booted my notebook: OK
> * suspended, resumed, reloaded the driver: OK
> * loaded the driver with no cold reset
>   (no AC97_POWER_SAVE on intel8x0): OK
> * same as above, after resuming: OK
> * AC97_POWER_SAVE enabled (cold reset), after resume: NOT ok
> 
> As previously reported, even when probing, after loading the driver, we
> get a different behaviour if we cold reset. When probing, we wait for
> those HZ/4, which may explain why everything works.
> 
> I tested if ac97_resume was writing the same values to the same
> registers if I had the delay or if I didn't. It was. However, the values
> I read from /proc were still different, very similar to what the codec
> specification refered to as the default values.
> 
> The best solution would be to do whatever is needed to do after cold
> resetting that we are not doing, unless it is a hardware issue and the
> driver can't really do anything about it but wait. Reading the Intel
> ICH3 doc and AD1881A doc, I couldn't figure out anything that the driver
> is doing wrong.
> 
> So, right now, the alternatives amount to:
> 
> * Wait for the HZ/4 additional delay. Probing is doing it and my
> previously submitted patch does not add any delay for the probe case
> and, it the resume case, it only adds a delay if we are cold resetting,
> that is, AC97_POWER_SAVE is enabled.
> 
> * Do not do the cold reset at all. It works pretty well for me if ac97
> power save is enabled and we only do a warm reset.
> 
> * Do only one of the above as a quirk for my controller or codec or
> their combination.

I feel it's not only your machine that a similar thing happens.
OTOH, adding the delay selectively would be good, at least, for an old
driver with AC97.  So, checking the machine could be a better option
at this stage.

So, my bet is the third one, adding a (sort of) quirk to disable the
cold reset.  I'm not sure whether yet another module option is the
best way, though...


thanks,

Takashi