[alsa-devel] [PATCH] ASoC: MPC5200: Support for buffer wrap around
We've encountered strange behavior in the alsamixer settings using the wm9712 codec. If we unmute the headphone output and then unmute the PCM output, the headphone output gets reset to mute in the hardware register. At this point the hardware register does not match the value in the register cache.
I've spent some time debugging this, and the headphone setting is set outside of any code path that would call the ac97_write() routine. As best as I can tell, there is something strange going on in hardware.
I've provided this patch that works around the problem.
Have any of you seen this before? Is this patch the right approach?
- John
The code in psc_dma_bcom_enqueue_tx() didn't account for the fact that s->runtime->control->appl_ptr can wrap around to the beginning of the buffer. This change fixes this problem.
Signed-off-by: John Bonesio bones@secretlab.ca ---
sound/soc/fsl/mpc5200_dma.c | 17 +++++++++++++++++ 1 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c index cfe0ea4..2551c58 100644 --- a/sound/soc/fsl/mpc5200_dma.c +++ b/sound/soc/fsl/mpc5200_dma.c @@ -70,6 +70,23 @@ static void psc_dma_bcom_enqueue_next_buffer(struct psc_dma_stream *s)
static void psc_dma_bcom_enqueue_tx(struct psc_dma_stream *s) { + if (s->appl_ptr > s->runtime->control->appl_ptr) { + /* + * In this case s->runtime->control->appl_ptr has wrapped around. + * Play the data to the end of the boundary, then wrap our own + * appl_ptr back around. + */ + while (s->appl_ptr < s->runtime->boundary) { + if (bcom_queue_full(s->bcom_task)) + return; + + s->appl_ptr += s->period_size; + + psc_dma_bcom_enqueue_next_buffer(s); + } + s->appl_ptr -= s->runtime->boundary; + } + while (s->appl_ptr < s->runtime->control->appl_ptr) {
if (bcom_queue_full(s->bcom_task))
On Friday, July 31, 2009, John Bonesio bones@secretlab.ca wrote:
Hey John,
It would appear that you've attached the wrong patch.
g.
On Fri, Jul 31, 2009 at 04:08:55PM -0700, John Bonesio wrote:
Which specific controls are you looking at here?
I've provided this patch that works around the problem.
Like Grant says I suspect this isn't the patch you meant...
Have any of you seen this before? Is this patch the right approach?
I've never heard of that before and the WM9712 is very widely deployed so if it were a hardware issue in the CODEC I'd imagine it would have come up. On the other hand, it's not beyond the bounds of possibility. If you could post the patch showing exactly what you're doing I could probably say more.
If you could get a scope on the bus that might be interesting...
On Fri, Jul 31, 2009 at 7:08 PM, John Bonesiobones@secretlab.ca wrote:
This could be something wrong in the mpc5200 code that writes the AC97 registers too. Maybe a register write command is getting generated when it wasn't supposed to.
You can set the mpc5200 to generate an interrupt everything it finishes writing a AC97 register. If you get more interrupts than you expected the problem is in the driver.
Hi All,
Woops! Yep, this is the wrong patch. I've included the correct one. I've also attached the patch in case email messes with its formatting. Sorry about the confusion.
Jon,
I put in a debug hook in the out_be32() routine. It would printk output anytime the AC97_HEADPHONE register was written to. This printk text would come out as expected when the Headphone mixer setting was muted/unmuted.
When I umuted the PCM mixer setting, this printk text did not appear, yet when I read the mixer register setting for the headphone, it was set back to mute.
If there is code in software doing this, it's very subtle.
- John
----- correct patch -----
From 24b52cf2836f711118fd6d2c8895c91c944978cd Mon Sep 17 00:00:00 2001
From: John Bonesio bones@secretlab.ca Date: Fri, 31 Jul 2009 16:01:33 -0700 Subject: [PATCH] ASoC: WM9712 Codec: Workaround an unmute problem
When setting the PCM mixer (unuting), the main Headphone mixer setting gets set back to mute. At this time it appears this is occuring in hardware.
Signed-off-by: John Bonesio bones@secretlab.ca --- sound/soc/codecs/wm9712.c | 21 +++++++++++++++++++-- 1 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/wm9712.c b/sound/soc/codecs/wm9712.c index b57c817..6f8d164 100644 --- a/sound/soc/codecs/wm9712.c +++ b/sound/soc/codecs/wm9712.c @@ -28,6 +28,7 @@ static unsigned int ac97_read(struct snd_soc_codec *codec, unsigned int reg); static int ac97_write(struct snd_soc_codec *codec, unsigned int reg, unsigned int val); +static int ac97_flush(struct snd_soc_codec *codec, unsigned int reg);
/* * WM9712 register cache @@ -177,9 +178,14 @@ static int mixer_event(struct snd_soc_dapm_widget *w, else ac97_write(w->codec, AC97_VIDEO, mic | 0x8000);
- if (l & 0x2 || r & 0x2) + if (l & 0x2 || r & 0x2) { ac97_write(w->codec, AC97_PCM, pcm & 0x7fff); - else + /* + * Workaround an apparent bug where the headphone mute setting + * is modified when the PCM mute setting is enabled. + */ + ac97_flush(w->codec, AC97_HEADPHONE); + } else ac97_write(w->codec, AC97_PCM, pcm | 0x8000);
if (l & 0x4 || r & 0x4) @@ -472,6 +478,17 @@ static int ac97_write(struct snd_soc_codec *codec, unsigned int reg, return 0; }
+static int ac97_flush(struct snd_soc_codec *codec, unsigned int reg) +{ + unsigned int val; + u16 *cache = codec->reg_cache; + + if ((reg >> 1) < (ARRAY_SIZE(wm9712_reg))) { + val = cache[reg >> 1]; + soc_ac97_ops.write(codec->ac97, reg, val); + } +} + static int ac97_prepare(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) {
On Sat, Aug 1, 2009 at 11:04 AM, John Bonesiobones@secretlab.ca wrote:
I was thinking a stale register write may have gotten into the mpc5200 AC97 hardware somehow and not been sent immediately to the codec. Something you do later causes it to be sent. I don't trust the mpc5200 AC97 register write hardware 100%, it was an after thought added in the mpc5200b.
Some logic analyzers have AC97 protocol decode capability. That would isolate easily isolate the problem.
On Sat, Aug 01, 2009 at 08:04:56AM -0700, John Bonesio wrote:
Like I said before, exactly which control are you adjusting here?
If there is code in software doing this, it's very subtle.
My money would be on the AC97 controller having problems; the quality of SoC AC97 controllers is variable. It certainly doesn't sound like a WM9712 issue; as I say I'd be very surprised if such an issue hadn't come up before given how widely deployed the part is.
Hi Mark,
I've retitled the email to better reflect the real patch. I believe there has been some general confusion because I originally sent the wrong patch.
You wrote:
Like I said before, exactly which control are you adjusting here?
My description of this got lost in all the confusion. Let me try again. We are adjusting the mixer bits for mute/unmute on two of the mixer settings. The first one is general headphone mute setting on register 0x4 (bit 15). The second one is the PCM mute setting on register 0x18 (bit 15).
What we are seeing is that if we first unmute the general headphone (reg 0x4 bit15), then unmute the PCM (reg 0x18 bit 15) [HPL PCM in the alsamixer application], the general headphone gets muted again, even though software didn't write to that register.
We don't have access to an AC97 analyzer. Do you have any suggestions on other ways we can pinpoint the error?
- John
Here's the patch again for reference:
Author: John Bonesio bones@secretlab.ca Date: Fri Jul 31 16:01:33 2009 -0700
ASoC: WM9712 Codec: Workaround an unmute problem
When setting the PCM mixer (unuting), the main Headphone mixer setting gets set back to mute. At this time it appears this is occuring in hardware.
Signed-off-by: John Bonesio bones@secretlab.ca
diff --git a/sound/soc/codecs/wm9712.c b/sound/soc/codecs/wm9712.c index b57c817..6f8d164 100644 --- a/sound/soc/codecs/wm9712.c +++ b/sound/soc/codecs/wm9712.c @@ -28,6 +28,7 @@ static unsigned int ac97_read(struct snd_soc_codec *codec, unsigned int reg); static int ac97_write(struct snd_soc_codec *codec, unsigned int reg, unsigned int val); +static int ac97_flush(struct snd_soc_codec *codec, unsigned int reg);
/* * WM9712 register cache @@ -177,9 +178,14 @@ static int mixer_event(struct snd_soc_dapm_widget *w, else ac97_write(w->codec, AC97_VIDEO, mic | 0x8000);
- if (l & 0x2 || r & 0x2) + if (l & 0x2 || r & 0x2) { ac97_write(w->codec, AC97_PCM, pcm & 0x7fff); - else + /* + * Workaround an apparent bug where the headphone mute setting + * is modified when the PCM mute setting is enabled. + */ + ac97_flush(w->codec, AC97_HEADPHONE); + } else ac97_write(w->codec, AC97_PCM, pcm | 0x8000);
if (l & 0x4 || r & 0x4) @@ -472,6 +478,17 @@ static int ac97_write(struct snd_soc_codec *codec, unsigned int reg, return 0; }
+static int ac97_flush(struct snd_soc_codec *codec, unsigned int reg) +{ + unsigned int val; + u16 *cache = codec->reg_cache; + + if ((reg >> 1) < (ARRAY_SIZE(wm9712_reg))) { + val = cache[reg >> 1]; + soc_ac97_ops.write(codec->ac97, reg, val); + } +} + static int ac97_prepare(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) {
On Sun, 2009-08-02 at 12:49 +0100, Mark Brown wrote:
On Wed, Aug 05, 2009 at 09:30:53AM -0700, John Bonesio wrote:
Retitling again; I don't think we've diagnosed what is causing issues here. Doing a quick test here I wasn't able to reproduce this behaviour so I suspect either the AC97 controller by itself or some interaction between it and the WM9712. Like I say, the WM9712 is not a new part and the drivers aren't new either so it'd be surprising to find an issue like this now.
Like I said before, exactly which control are you adjusting here?
Hrm, so the same bit in both registers. Suspicious...
Are any other bits in register 4 affected or is it only bit 15?
We don't have access to an AC97 analyzer. Do you have any suggestions on other ways we can pinpoint the error?
Any scope that can capture would be useful to see what's going on; obviously decode would have to be by hand. Coding out the register cache may also be useful - it'd allow you to see the current hardware register values.
On Wed, 2009-08-05 at 18:03 +0100, Mark Brown wrote:
Ok. At this point I'm not trying to get the patch approved so much as I'd like to see if we can get to the root cause of the problem.
So far, I've only seen this one bit affected. There are lots of combinations that could be tested, and I've not done comprehensive test of every bit. It's possible that other bits are affected in ways that don't show up in my testing.
Below I describe in more detail how I've tested.
In case anyone else sees this problem, and would like to jump in, here is what I've done to investigate this problem.
First I added a debugfs file that displays nearly all of the mixer registers and decodes their bits for convenience. I've attached a patch for this change. I believe this patch is just for debugging. I don't expect that this patch needs to go into the main source.
The debugfs file bypasses the register cache in the codec. It also reads the value in the cache and compares them. If the register value is different than that of the cache, text is added to the debugfs file to alert the viewer of this.
When the error occurs, the cache no longer reflects the value of the hardware register.
As mentioned before, I've also hooked the out_be32() routine, and had it print a message whenever register 0x4 of the codec (The headphone mixer register) is modified by software.
The register is written indirectly, so to be clear, the hook in out_be32() checked for writes to the ac97_cmd register and the register number field of the value written had 0x4 in it. (e.g. (value >> 24) & 0x7f) == 0x4)
The hook didn't check for the "write" bit so it also ended up displaying a message whenever register 0x4 was read as well as written.
In my tests register 0x4 is being modified outside the software path that would deliberately modify this register using out_be32().
I didn't include a patch for this hook, as it was very hacky, and I wasn't so sure about its usefulness.
This issue has been seen on MPC5200 based boards.
I've chatted with Grant a bit. We'll see what we can scrounge up to hook up some test equipment. I'm not sure when we'll get to this.
I'm open to more suggestions or thoughts on the approach I've taken so far.
- John
On Thu, Aug 06, 2009 at 10:28:41AM -0700, John Bonesio wrote:
I'm open to more suggestions or thoughts on the approach I've taken so far.
The approach you've been taking sounds reasonable; at this point I'd be looking at trying to confirm that the controller is doing what's been asked of it. Checking that the CODEC power rails are all within spec and looking at other bits in the same register may also be useful.
Is the system otherwise idle when the problem occurs?
On Fri, Aug 7, 2009 at 10:48 AM, John Bonesiobones@secretlab.ca wrote:
You can turn on an interrupt that gets triggered when the register wire completes. That would give you a clue if too many register writes are happening.
A DSO or logical analyzer would make this much faster to isolate.
On Wed, Aug 5, 2009 at 12:30 PM, John Bonesiobones@secretlab.ca wrote:
Do you have a normal logic analyzer? You can decode the AC97 by hand, its only a couple of frames.
On Wed, Aug 5, 2009 at 1:16 PM, Jon Smirljonsmirl@gmail.com wrote:
My hardware engineer has this one, but it is in Seattle and I am in Florida. The OpenMoko guys use it. http://www.pctestinstruments.com/
You can write your own interpreters, someone may have done AC97 for it but I haven't checked.
He also has a Rigol DS1102E which he says is an awesome scope for $636. Another tip from OpenMoku. http://www.tequipment.net/RigolDS1102E.html
participants (5)
-
Grant Likely
-
John Bonesio
-
Jon Smirl
-
Mark Brown
-
Mark Brown