From cdc65fbe18aef15e92d2ebb410a189fbf956fb06 Mon Sep 17 00:00:00 2001 From: Manuel Lauss Date: Tue, 8 Sep 2009 19:45:17 +0200 Subject: 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 Signed-off-by: Mark Brown --- sound/soc/au1x/psc-ac97.c | 129 ++++++++++++++++++++++++++++++++++------------ sound/soc/au1x/psc.h | 1 + 2 files changed, 97 insertions(+), 33 deletions(-) (limited to 'sound') diff --git a/sound/soc/au1x/psc-ac97.c b/sound/soc/au1x/psc-ac97.c index 479d7bd..a521aa9 100644 --- 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 + * (c) 2007-2009 MSC Vertriebsges.m.b.H., + * Manuel Lauss * * 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 #include #include +#include #include #include #include @@ -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 "); +MODULE_AUTHOR("Manuel Lauss "); diff --git a/sound/soc/au1x/psc.h b/sound/soc/au1x/psc.h index 8fdb1a0..3f474e8 100644 --- 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 -- cgit v1.1