[alsa-devel] AC97 on pcm030 board with WM9712 codec
Hello,
I've been working with Grant Likely exploring an existing issue with resetting the AC97 system on the pcm030 board. The easiest way to produce the problem is by power cycling the board and seeing if the AC97 and wm9712 codec show up in /sys/bus/ac97.
I wanted to share with you folks to make sure there's not something else going on that I should be looking at, and if the results I'm seeing seem to make sense.
A bit of background for those not familiar with this area of the code:
When the board powers up, a reset() message is sent to the wm9712 codec driver which then calls the AC97 driver to perform a cold reset. The AC97 driver performs the cold reset by yanking the reset line on the AC97 link. The AC97 driver also calls warm_reset() after the cold reset is complete. Earlier the warm_reset() call was added in the AC97 cold_reset() routine to attempt to solve this bug, but it appears to not completely solve the issue. After the reset the wm9712 driver attempts to read a register. If the register value is verified the driver assumes the reset was a success, otherwise it flags an error.
The code for the routines we're using is included here for reference.
------- code snippets ------- static int wm9712_reset(struct snd_soc_codec *codec, int try_warm) { if (try_warm && soc_ac97_ops.warm_reset) { soc_ac97_ops.warm_reset(codec->ac97); if (ac97_read(codec, 0) == wm9712_reg[0]) return 1; }
soc_ac97_ops.reset(codec->ac97); if (ac97_read(codec, 0) != wm9712_reg[0]) goto err; return 0;
err: printk(KERN_ERR "WM9712 AC97 reset failed\n"); return -EIO; }
static void psc_ac97_cold_reset(struct snd_ac97 *ac97) { struct mpc52xx_psc __iomem *regs = psc_dma->psc_regs;
/* Do a cold reset */ out_8(®s->op1, MPC52xx_PSC_OP_RES); udelay(10); out_8(®s->op0, MPC52xx_PSC_OP_RES); udelay(50);
psc_ac97_warm_reset(ac97); }
static void psc_ac97_warm_reset(struct snd_ac97 *ac97) { struct mpc52xx_psc __iomem *regs = psc_dma->psc_regs;
out_be32(®s->sicr, psc_dma->sicr | MPC52xx_PSC_SICR_AWR); udelay(3); out_be32(®s->sicr, psc_dma->sicr); } ------- end code snippets -------
I've explored the following possibilities: 1. SDATAOUT or SYNC lines were non-zero during reset, putting the codec chip into a test mode. 2. Perhaps the cold_reset is not necessary and the warm_reset is all that is needed 3. The delay between the cold reset and the warm reset was not long enough.
Possibility #1
I added some test code to force the pcm030 board to use gpio instead of the ac97 to perform the reset. This way the SDATAOUT and the SYNC signals can be forced to 0 while resetting. What I discovered is that this didn't seem to make a difference. I don't know if these signals might contain uninitialized values on power up. Should I still consider this as a potential problem, or is this pretty much a non-issue?
Possibility #2
I tried taking out the cold reset code in the AC97 driver in hopes that perhaps the hardware would come up in a good state on its own. This didn't work at all. I put the cold reset code back in.
Possibility #3
I stumbled upon this when I added a printk to make sure I correctly understood the flow of control. With the printk in place, the problem would go away. So instead I increased the length of time in the udelay() call right before the call to warm_reset(). I've empirically found that delaying 1ms (udelay(50) -> udelay(1000)) seems to avoid the problem. I'm not sure why this delay is necessary, or if it makes sense for the hardware to need this much time to come out of the cold reset.
Thanks for your help.
Cheers, - John
On Mon, Jul 20, 2009 at 04:29:11PM -0700, John Bonesio wrote:
AC97 link. The AC97 driver also calls warm_reset() after the cold reset is complete. Earlier the warm_reset() call was added in the AC97 cold_reset() routine to attempt to solve this bug, but it appears to not completely solve the issue. After the reset the wm9712 driver attempts
No, the warm reset is an unrelated change. The warm reset is there because the WM9712 can be strapped to not start up the AC97 link at startup.
- Perhaps the cold_reset is not necessary and the warm_reset is all
that is needed
No, AC97 cold and warm resets do different things. A cold reset restores the device to power on state while a warm reset will restart the AC97 link if it was not already running without changing any register settings in the device. A warm reset cannot substitute for a cold reset.
[Buggy GPIOs]
might contain uninitialized values on power up. Should I still consider this as a potential problem, or is this pretty much a non-issue?
Depends how much you trust your AC97 controller.
would go away. So instead I increased the length of time in the udelay() call right before the call to warm_reset(). I've empirically found that delaying 1ms (udelay(50) -> udelay(1000)) seems to avoid the problem. I'm not sure why this delay is necessary, or if it makes sense for the hardware to need this much time to come out of the cold reset.
I'm not sure which delay you're talking about here (I'm guessing that it's one of those in the PSC driver?) but this does sound entirely plausible - if it resolves the issue could you produce a patch, please? I'd suggest changing to use msleep() instead of udeley() since delay will busy wait while sleep won't.
Thanks for your work on this.
From ac594ad4aff30af6d65375df1a5e6c3b43134721 Mon Sep 17 00:00:00 2001
From: John Bonesio bones@secretlab.ca Date: Tue, 21 Jul 2009 13:15:40 -0700 Subject: [PATCH] ASoC: MPC5200: Increase the delay time between resets
Reset was failing with the original udelay(50) between the code in psc_ac97_cold_reset() and the call to psc_ac97_warm_reset(). Through testing it was found that a delay of 1ms was necessary for the cold_reset code to consistently complete successfully.
Signed-off-by: John Bonesio bones@secretlab.ca --- sound/soc/fsl/mpc5200_psc_ac97.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/sound/soc/fsl/mpc5200_psc_ac97.c b/sound/soc/fsl/mpc5200_psc_ac97.c index 7eb5499..c4ae3e0 100644 --- a/sound/soc/fsl/mpc5200_psc_ac97.c +++ b/sound/soc/fsl/mpc5200_psc_ac97.c @@ -12,6 +12,7 @@ #include <linux/module.h> #include <linux/of_device.h> #include <linux/of_platform.h> +#include <linux/delay.h>
#include <sound/pcm.h> #include <sound/pcm_params.h> @@ -112,7 +113,7 @@ static void psc_ac97_cold_reset(struct snd_ac97 *ac97) out_8(®s->op1, MPC52xx_PSC_OP_RES); udelay(10); out_8(®s->op0, MPC52xx_PSC_OP_RES); - udelay(50); + msleep(1); psc_ac97_warm_reset(ac97); }
On Tue, Jul 21, 2009 at 01:24:04PM -0700, John Bonesio wrote:
Reset was failing with the original udelay(50) between the code in psc_ac97_cold_reset() and the call to psc_ac97_warm_reset(). Through testing it was found that a delay of 1ms was necessary for the cold_reset code to consistently complete successfully.
Applied, thanks.
On Tue, Jul 21, 2009 at 7:08 PM, Mark Brownbroonie@opensource.wolfsonmicro.com wrote:
On Tue, Jul 21, 2009 at 01:24:04PM -0700, John Bonesio wrote:
Reset was failing with the original udelay(50) between the code in psc_ac97_cold_reset() and the call to psc_ac97_warm_reset(). Through testing it was found that a delay of 1ms was necessary for the cold_reset code to consistently complete successfully.
Something is funky between the mpc5200 and the wm9712 when doing a cold reset. Increasing this delay seems to fix it but it shouldn't be needed per the wm9712 datasheet. The only way to know 100% for sure what is going on is to put a scope (I don't have one) on the reset line and the SDATAOUT or SYNC lines. Maybe SDATAOUT or SYNC initially bounce around and delaying for 1ms waits until after the balancing stops. Datasheet says pulse only needs to be 1us.
But if the 1ms delays fixes things I'm all for it.
participants (3)
-
John Bonesio -
Jon Smirl -
Mark Brown