ASoC: au1x: PSC-AC97 bugfixes

This patch fixes the following bugs:

- only reprogram bitdepth if it has changed since last call to hw_params.
- add locking inside ac97_read/write functions:
  When reprogramming sample depth, the ac97 unit has to be disabled,
  which should not be done in the middle of codec register accesses.

- retry timed-out codec register accesses.

- wait for status bits to set/clear when starting/stopping various
  functional blocks; very important after reenabling AC97 unit else
  sound may be distorted (e.g. high-pitch noise in 1kHz sine wave).

- clear fifos before/after starting/stopping RX/TX.

- longer timeouts waiting for PSC/AC97 ready after cold reset
  with certain codecs this can take ridiculous amounts of time.

Run-tested on various Au1200 platforms with various codecs.

Signed-off-by: Manuel Lauss <manuel.lauss@gmail.com>
Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

diff --git a/sound/soc/au1x/psc-ac97.c b/sound/soc/au1x/psc-ac97.c
index 479d7bd..a521aa9 100644 (file)
--- a/sound/soc/au1x/psc-ac97.c
+++ b/sound/soc/au1x/psc-ac97.c
@@ -1,8 +1,8 @@
 /*
  * Au12x0/Au1550 PSC ALSA ASoC audio support.
  *
- * (c) 2007-2008 MSC Vertriebsges.m.b.H.,
- *     Manuel Lauss <mano@roarinelk.homelinux.net>
+ * (c) 2007-2009 MSC Vertriebsges.m.b.H.,
+ *     Manuel Lauss <manuel.lauss@gmail.com>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -19,6 +19,7 @@
 #include <linux/module.h>
 #include <linux/device.h>
 #include <linux/delay.h>
+#include <linux/mutex.h>
 #include <linux/suspend.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
@@ -29,6 +30,9 @@
 
 #include "psc.h"
 
+/* how often to retry failed codec register reads/writes */
+#define AC97_RW_RETRIES        5
+
 #define AC97_DIR       \
        (SND_SOC_DAIDIR_PLAYBACK | SND_SOC_DAIDIR_CAPTURE)
 
@@ -45,6 +49,9 @@
 #define AC97PCR_CLRFIFO(stype) \
        ((stype) == PCM_TX ? PSC_AC97PCR_TC : PSC_AC97PCR_RC)
 
+#define AC97STAT_BUSY(stype)   \
+       ((stype) == PCM_TX ? PSC_AC97STAT_TB : PSC_AC97STAT_RB)
+
 /* instance data. There can be only one, MacLeod!!!! */
 static struct au1xpsc_audio_data *au1xpsc_ac97_workdata;
 
@@ -54,24 +61,33 @@ static unsigned short au1xpsc_ac97_read(struct snd_ac97 *ac97,
 {
        /* FIXME */
        struct au1xpsc_audio_data *pscdata = au1xpsc_ac97_workdata;
-       unsigned short data, tmo;
+       unsigned short data, retry, tmo;
 
-       au_writel(PSC_AC97CDC_RD | PSC_AC97CDC_INDX(reg), AC97_CDC(pscdata));
+       au_writel(PSC_AC97EVNT_CD, AC97_EVNT(pscdata));
        au_sync();
 
-       tmo = 1000;
-       while ((!(au_readl(AC97_EVNT(pscdata)) & PSC_AC97EVNT_CD)) && --tmo)
-               udelay(2);
+       retry = AC97_RW_RETRIES;
+       do {
+               mutex_lock(&pscdata->lock);
+
+               au_writel(PSC_AC97CDC_RD | PSC_AC97CDC_INDX(reg),
+                         AC97_CDC(pscdata));
+               au_sync();
+
+               tmo = 2000;
+               while ((!(au_readl(AC97_EVNT(pscdata)) & PSC_AC97EVNT_CD))
+                       && --tmo)
+                       udelay(2);
 
-       if (!tmo)
-               data = 0xffff;
-       else
                data = au_readl(AC97_CDC(pscdata)) & 0xffff;
 
-       au_writel(PSC_AC97EVNT_CD, AC97_EVNT(pscdata));
-       au_sync();
+               au_writel(PSC_AC97EVNT_CD, AC97_EVNT(pscdata));
+               au_sync();
+
+               mutex_unlock(&pscdata->lock);
+       } while (--retry && !tmo);
 
-       return data;
+       return retry ? data : 0xffff;
 }
 
 /* AC97 controller writes to codec register */
@@ -80,16 +96,29 @@ static void au1xpsc_ac97_write(struct snd_ac97 *ac97, unsigned short reg,
 {
        /* FIXME */
        struct au1xpsc_audio_data *pscdata = au1xpsc_ac97_workdata;
-       unsigned int tmo;
+       unsigned int tmo, retry;
 
-       au_writel(PSC_AC97CDC_INDX(reg) | (val & 0xffff), AC97_CDC(pscdata));
+       au_writel(PSC_AC97EVNT_CD, AC97_EVNT(pscdata));
        au_sync();
-       tmo = 1000;
-       while ((!(au_readl(AC97_EVNT(pscdata)) & PSC_AC97EVNT_CD)) && --tmo)
+
+       retry = AC97_RW_RETRIES;
+       do {
+               mutex_lock(&pscdata->lock);
+
+               au_writel(PSC_AC97CDC_INDX(reg) | (val & 0xffff),
+                         AC97_CDC(pscdata));
                au_sync();
 
-       au_writel(PSC_AC97EVNT_CD, AC97_EVNT(pscdata));
-       au_sync();
+               tmo = 2000;
+               while ((!(au_readl(AC97_EVNT(pscdata)) & PSC_AC97EVNT_CD))
+                      && --tmo)
+                       udelay(2);
+
+               au_writel(PSC_AC97EVNT_CD, AC97_EVNT(pscdata));
+               au_sync();
+
+               mutex_unlock(&pscdata->lock);
+       } while (--retry && !tmo);
 }
 
 /* AC97 controller asserts a warm reset */
@@ -129,9 +158,9 @@ static void au1xpsc_ac97_cold_reset(struct snd_ac97 *ac97)
        au_sync();
 
        /* wait for PSC to indicate it's ready */
-       i = 100000;
+       i = 1000;
        while (!((au_readl(AC97_STAT(pscdata)) & PSC_AC97STAT_SR)) && (--i))
-               au_sync();
+               msleep(1);
 
        if (i == 0) {
                printk(KERN_ERR "au1xpsc-ac97: PSC not ready!\n");
@@ -143,9 +172,9 @@ static void au1xpsc_ac97_cold_reset(struct snd_ac97 *ac97)
        au_sync();
 
        /* wait for AC97 core to become ready */
-       i = 100000;
+       i = 1000;
        while (!((au_readl(AC97_STAT(pscdata)) & PSC_AC97STAT_DR)) && (--i))
-               au_sync();
+               msleep(1);
        if (i == 0)
                printk(KERN_ERR "au1xpsc-ac97: AC97 ctrl not ready\n");
 }
@@ -165,12 +194,12 @@ static int au1xpsc_ac97_hw_params(struct snd_pcm_substream *substream,
 {
        /* FIXME */
        struct au1xpsc_audio_data *pscdata = au1xpsc_ac97_workdata;
-       unsigned long r, stat;
+       unsigned long r, ro, stat;
        int chans, stype = SUBSTREAM_TYPE(substream);
 
        chans = params_channels(params);
 
-       r = au_readl(AC97_CFG(pscdata));
+       r = ro = au_readl(AC97_CFG(pscdata));
        stat = au_readl(AC97_STAT(pscdata));
 
        /* already active? */
@@ -180,9 +209,6 @@ static int au1xpsc_ac97_hw_params(struct snd_pcm_substream *substream,
                    (pscdata->rate != params_rate(params)))
                        return -EINVAL;
        } else {
-               /* disable AC97 device controller first */
-               au_writel(r & ~PSC_AC97CFG_DE_ENABLE, AC97_CFG(pscdata));
-               au_sync();
 
                /* set sample bitdepth: REG[24:21]=(BITS-2)/2 */
                r &= ~PSC_AC97CFG_LEN_MASK;
@@ -199,14 +225,40 @@ static int au1xpsc_ac97_hw_params(struct snd_pcm_substream *substream,
                        r |= PSC_AC97CFG_RXSLOT_ENA(4);
                }
 
-               /* finally enable the AC97 controller again */
+               /* do we need to poke the hardware? */
+               if (!(r ^ ro))
+                       goto out;
+
+               /* ac97 engine is about to be disabled */
+               mutex_lock(&pscdata->lock);
+
+               /* disable AC97 device controller first... */
+               au_writel(r & ~PSC_AC97CFG_DE_ENABLE, AC97_CFG(pscdata));
+               au_sync();
+
+               /* ...wait for it... */
+               while (au_readl(AC97_STAT(pscdata)) & PSC_AC97STAT_DR)
+                       asm volatile ("nop");
+
+               /* ...write config... */
+               au_writel(r, AC97_CFG(pscdata));
+               au_sync();
+
+               /* ...enable the AC97 controller again... */
                au_writel(r | PSC_AC97CFG_DE_ENABLE, AC97_CFG(pscdata));
                au_sync();
 
+               /* ...and wait for ready bit */
+               while (!(au_readl(AC97_STAT(pscdata)) & PSC_AC97STAT_DR))
+                       asm volatile ("nop");
+
+               mutex_unlock(&pscdata->lock);
+
                pscdata->cfg = r;
                pscdata->rate = params_rate(params);
        }
 
+out:
        return 0;
 }
 
@@ -222,6 +274,8 @@ static int au1xpsc_ac97_trigger(struct snd_pcm_substream *substream,
        switch (cmd) {
        case SNDRV_PCM_TRIGGER_START:
        case SNDRV_PCM_TRIGGER_RESUME:
+               au_writel(AC97PCR_CLRFIFO(stype), AC97_PCR(pscdata));
+               au_sync();
                au_writel(AC97PCR_START(stype), AC97_PCR(pscdata));
                au_sync();
                break;
@@ -229,6 +283,13 @@ static int au1xpsc_ac97_trigger(struct snd_pcm_substream *substream,
        case SNDRV_PCM_TRIGGER_SUSPEND:
                au_writel(AC97PCR_STOP(stype), AC97_PCR(pscdata));
                au_sync();
+
+               while (au_readl(AC97_STAT(pscdata)) & AC97STAT_BUSY(stype))
+                       asm volatile ("nop");
+
+               au_writel(AC97PCR_CLRFIFO(stype), AC97_PCR(pscdata));
+               au_sync();
+
                break;
        default:
                ret = -EINVAL;
@@ -251,6 +312,8 @@ static int au1xpsc_ac97_probe(struct platform_device *pdev,
        if (!au1xpsc_ac97_workdata)
                return -ENOMEM;
 
+       mutex_init(&au1xpsc_ac97_workdata->lock);
+
        r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
        if (!r) {
                ret = -ENODEV;
@@ -269,9 +332,9 @@ static int au1xpsc_ac97_probe(struct platform_device *pdev,
                goto out1;
 
        /* configuration: max dma trigger threshold, enable ac97 */
-        au1xpsc_ac97_workdata->cfg = PSC_AC97CFG_RT_FIFO8 |
-                                     PSC_AC97CFG_TT_FIFO8 |
-                                     PSC_AC97CFG_DE_ENABLE;
+       au1xpsc_ac97_workdata->cfg = PSC_AC97CFG_RT_FIFO8 |
+                                    PSC_AC97CFG_TT_FIFO8 |
+                                    PSC_AC97CFG_DE_ENABLE;
 
        /* preserve PSC clock source set up by platform (dev.platform_data
         * is already occupied by soc layer)
@@ -386,4 +449,4 @@ module_exit(au1xpsc_ac97_exit);
 
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("Au12x0/Au1550 PSC AC97 ALSA ASoC audio driver");
-MODULE_AUTHOR("Manuel Lauss <mano@roarinelk.homelinux.net>");
+MODULE_AUTHOR("Manuel Lauss <manuel.lauss@gmail.com>");

diff --git a/sound/soc/au1x/psc.h b/sound/soc/au1x/psc.h
index 8fdb1a0..3f474e8 100644 (file)
--- a/sound/soc/au1x/psc.h
+++ b/sound/soc/au1x/psc.h
@@ -29,6 +29,7 @@ struct au1xpsc_audio_data {
 
        unsigned long pm[2];
        struct resource *ioarea;
+       struct mutex lock;
 };
 
 #define PCM_TX 0