[alsa-devel] [PATCH v2] intel8x0m: wait a bit before warm reset check
At every resume a laptop I use prints this message (at KERN_ERR level): ALSA sound/pci/intel8x0m.c:904: AC'97 warm reset still in progress? [0x2]
The thing to note here is that 0x2 corresponds to ICH_AC97COLD. Ie, what seems to be happening is that the register involved indicated a warm reset for some time (as the ICH_AC97WARM bit was set) but by the time the warning is printed, and that same register is checked again, that bit is already cleared and only the ICH_AC97COLD bit is still set.
It turns out a warm reset needs some time to settle, but it is currently checked right away. The test therefore fails the first time it is done and schedule_timeout_uninterruptible() will be called. Once we return from that jiffies is already (far) past end_time on this laptop, so we exit the loop, print a warning, and exit the function while the warm reset actually succeeded.
One way to fix this is to call udelay() after writing to the register involved. A handful of tests suggests 500 usecs is a safe value. (This might punish the "finish cold reset" case, but on this laptop such a cold reset apparently never happens, so I can't say for sure.)
While we're at it drop the extra single tick from end_time, as it looks rather silly.
Signed-off-by: Paul Bolle pebolle@tiscali.nl --- 0) v1 was called " intel8x0m: schedule timeout before warm reset check".
1) A udelay of 250 usecs always failed while 300 usecs always worked. So 500 usecs might be a bit cautious.
sound/pci/intel8x0m.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/sound/pci/intel8x0m.c b/sound/pci/intel8x0m.c index 13cec1e..62fad24 100644 --- a/sound/pci/intel8x0m.c +++ b/sound/pci/intel8x0m.c @@ -894,7 +894,8 @@ static int snd_intel8x0m_ich_chip_init(struct intel8x0m *chip, int probing) /* finish cold or do warm reset */ cnt |= (cnt & ICH_AC97COLD) == 0 ? ICH_AC97COLD : ICH_AC97WARM; iputdword(chip, ICHREG(GLOB_CNT), cnt); - end_time = (jiffies + (HZ / 4)) + 1; + udelay(500); + end_time = jiffies + HZ / 4; do { if ((igetdword(chip, ICHREG(GLOB_CNT)) & ICH_AC97WARM) == 0) goto __ok;
On Sun, 6 Mar 2011, Paul Bolle wrote:
At every resume a laptop I use prints this message (at KERN_ERR level): ALSA sound/pci/intel8x0m.c:904: AC'97 warm reset still in progress? [0x2]
The thing to note here is that 0x2 corresponds to ICH_AC97COLD. Ie, what seems to be happening is that the register involved indicated a warm reset for some time (as the ICH_AC97WARM bit was set) but by the time the warning is printed, and that same register is checked again, that bit is already cleared and only the ICH_AC97COLD bit is still set.
It turns out a warm reset needs some time to settle, but it is currently checked right away. The test therefore fails the first time it is done and schedule_timeout_uninterruptible() will be called. Once we return from that jiffies is already (far) past end_time on this laptop, so we exit the loop, print a warning, and exit the function while the warm reset actually succeeded.
One way to fix this is to call udelay() after writing to the register involved. A handful of tests suggests 500 usecs is a safe value. (This might punish the "finish cold reset" case, but on this laptop such a cold reset apparently never happens, so I can't say for sure.)
While we're at it drop the extra single tick from end_time, as it looks rather silly.
Signed-off-by: Paul Bolle pebolle@tiscali.nl
v1 was called " intel8x0m: schedule timeout before warm reset check".
A udelay of 250 usecs always failed while 300 usecs always worked. So
500 usecs might be a bit cautious.
Wouldn't it be nice to put these notes in a comment in the code so that future readers don't have to scratch their heads and wonder where the udelay() with some magical constant comes from?
Also, if udelay(300) always worked in your tests, wouldn't udelay(400) be more than enough to provide a nice fat safety margin (33% more than "always works")? Are there no official documents that specify how long this is expected to take that we could base this delay on instead of just testing on one system?
At Sun, 6 Mar 2011 01:12:26 +0100 (CET), Jesper Juhl wrote:
On Sun, 6 Mar 2011, Paul Bolle wrote:
At every resume a laptop I use prints this message (at KERN_ERR level): ALSA sound/pci/intel8x0m.c:904: AC'97 warm reset still in progress? [0x2]
The thing to note here is that 0x2 corresponds to ICH_AC97COLD. Ie, what seems to be happening is that the register involved indicated a warm reset for some time (as the ICH_AC97WARM bit was set) but by the time the warning is printed, and that same register is checked again, that bit is already cleared and only the ICH_AC97COLD bit is still set.
It turns out a warm reset needs some time to settle, but it is currently checked right away. The test therefore fails the first time it is done and schedule_timeout_uninterruptible() will be called. Once we return from that jiffies is already (far) past end_time on this laptop, so we exit the loop, print a warning, and exit the function while the warm reset actually succeeded.
One way to fix this is to call udelay() after writing to the register involved. A handful of tests suggests 500 usecs is a safe value. (This might punish the "finish cold reset" case, but on this laptop such a cold reset apparently never happens, so I can't say for sure.)
While we're at it drop the extra single tick from end_time, as it looks rather silly.
Signed-off-by: Paul Bolle pebolle@tiscali.nl
v1 was called " intel8x0m: schedule timeout before warm reset check".
A udelay of 250 usecs always failed while 300 usecs always worked. So
500 usecs might be a bit cautious.
Wouldn't it be nice to put these notes in a comment in the code so that future readers don't have to scratch their heads and wonder where the udelay() with some magical constant comes from?
That'd be better, indeed.
Also, if udelay(300) always worked in your tests, wouldn't udelay(400) be more than enough to provide a nice fat safety margin (33% more than "always works")?
Well... I think you know how meaningless to quote a percentage representation in such a case...
Are there no official documents that specify how long this is expected to take that we could base this delay on instead of just testing on one system?
Heh, can you really trust such a document? ;)
It's basically a workaround for a specific codec chip that doesn't set the bit reflecting to the right status after resume/power-up. That is, it's just for the codec chip on Paul's machine. Others work well without this delay.
Also, as mentioned earlier, it's better to use usleep_range() than udelay(). There is no hard upper-limit in this case, and any longer sleep works.
thanks,
Takashi
At Mon, 07 Mar 2011 11:36:34 +0100, Paul Bolle wrote:
On Mon, 2011-03-07 at 10:55 +0100, Takashi Iwai wrote:
Also, as mentioned earlier, it's better to use usleep_range() than udelay(). There is no hard upper-limit in this case, and any longer sleep works.
So something like, say, usleep_range(500, 1000) should be OK?
Yes. It can be longer (as it's no path with critical time-out).
Takashi
participants (3)
-
Jesper Juhl -
Paul Bolle -
Takashi Iwai