[alsa-devel] [PATCH 1/3] ALSA: pxa: slightly refactor reset handling
PXA25x also shows some problems when using interrupts during reset handling. Thus do not use interrupts on all pxa kinds (to detect codec ready state). Instead use a common mdelay-loop on all platforms to detect codecs becoming ready.
Signed-off-by: Dmitry Eremin-Solenikov dbaryshkov@gmail.com --- sound/arm/pxa2xx-ac97-lib.c | 27 ++++++++++----------------- 1 file changed, 10 insertions(+), 17 deletions(-)
diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c index e6f4633..99a4668 100644 --- a/sound/arm/pxa2xx-ac97-lib.c +++ b/sound/arm/pxa2xx-ac97-lib.c @@ -117,8 +117,7 @@ static inline void pxa_ac97_warm_pxa25x(void) { gsr_bits = 0;
- GCR |= GCR_WARM_RST | GCR_PRIRDY_IEN | GCR_SECRDY_IEN; - wait_event_timeout(gsr_wq, gsr_bits & (GSR_PCR | GSR_SCR), 1); + GCR |= GCR_WARM_RST; }
static inline void pxa_ac97_cold_pxa25x(void) @@ -129,8 +128,6 @@ static inline void pxa_ac97_cold_pxa25x(void) gsr_bits = 0;
GCR = GCR_COLD_RST; - GCR |= GCR_CDONE_IE|GCR_SDONE_IE; - wait_event_timeout(gsr_wq, gsr_bits & (GSR_PCR | GSR_SCR), 1); } #endif
@@ -149,8 +146,6 @@ static inline void pxa_ac97_warm_pxa27x(void)
static inline void pxa_ac97_cold_pxa27x(void) { - unsigned int timeout; - GCR &= GCR_COLD_RST; /* clear everything but nCRST */ GCR &= ~GCR_COLD_RST; /* then assert nCRST */
@@ -161,29 +156,20 @@ static inline void pxa_ac97_cold_pxa27x(void) udelay(5); clk_disable(ac97conf_clk); GCR = GCR_COLD_RST | GCR_WARM_RST; - timeout = 100; /* wait for the codec-ready bit to be set */ - while (!((GSR | gsr_bits) & (GSR_PCR | GSR_SCR)) && timeout--) - mdelay(1); } #endif
#ifdef CONFIG_PXA3xx static inline void pxa_ac97_warm_pxa3xx(void) { - int timeout = 100; - gsr_bits = 0;
/* Can't use interrupts */ GCR |= GCR_WARM_RST; - while (!((GSR | gsr_bits) & (GSR_PCR | GSR_SCR)) && timeout--) - mdelay(1); }
static inline void pxa_ac97_cold_pxa3xx(void) { - int timeout = 1000; - /* Hold CLKBPB for 100us */ GCR = 0; GCR = GCR_CLKBPB; @@ -199,14 +185,13 @@ static inline void pxa_ac97_cold_pxa3xx(void) GCR &= ~(GCR_PRIRDY_IEN|GCR_SECRDY_IEN);
GCR = GCR_WARM_RST | GCR_COLD_RST; - while (!(GSR & (GSR_PCR | GSR_SCR)) && timeout--) - mdelay(10); } #endif
bool pxa2xx_ac97_try_warm_reset(struct snd_ac97 *ac97) { unsigned long gsr; + unsigned int timeout = 100;
#ifdef CONFIG_PXA25x if (cpu_is_pxa25x()) @@ -224,6 +209,10 @@ bool pxa2xx_ac97_try_warm_reset(struct snd_ac97 *ac97) else #endif BUG(); + + while (!((GSR | gsr_bits) & (GSR_PCR | GSR_SCR)) && timeout--) + mdelay(1); + gsr = GSR | gsr_bits; if (!(gsr & (GSR_PCR | GSR_SCR))) { printk(KERN_INFO "%s: warm reset timeout (GSR=%#lx)\n", @@ -239,6 +228,7 @@ EXPORT_SYMBOL_GPL(pxa2xx_ac97_try_warm_reset); bool pxa2xx_ac97_try_cold_reset(struct snd_ac97 *ac97) { unsigned long gsr; + unsigned int timeout = 1000;
#ifdef CONFIG_PXA25x if (cpu_is_pxa25x()) @@ -257,6 +247,9 @@ bool pxa2xx_ac97_try_cold_reset(struct snd_ac97 *ac97) #endif BUG();
+ while (!((GSR | gsr_bits) & (GSR_PCR | GSR_SCR)) && timeout--) + mdelay(1); + gsr = GSR | gsr_bits; if (!(gsr & (GSR_PCR | GSR_SCR))) { printk(KERN_INFO "%s: cold reset timeout (GSR=%#lx)\n",
After recent changes to codec/DAI initialization order changes, codec driver (wm9712 in my case) tries to access codec prior to pxa2xx_ac97_hw_probe() being called (because DAIs are probed after all codecs are probed). Move hw-related probe/remove/suspend/resume functions to pxa2xx-ac97 driver level, instead of DAI level.
Signed-off-by: Dmitry Eremin-Solenikov dbaryshkov@gmail.com --- sound/soc/pxa/pxa2xx-ac97.c | 56 ++++++++++++++++++++------------------------- 1 file changed, 25 insertions(+), 31 deletions(-)
diff --git a/sound/soc/pxa/pxa2xx-ac97.c b/sound/soc/pxa/pxa2xx-ac97.c index f1059d9..ae956e3 100644 --- a/sound/soc/pxa/pxa2xx-ac97.c +++ b/sound/soc/pxa/pxa2xx-ac97.c @@ -89,33 +89,6 @@ static struct snd_dmaengine_dai_dma_data pxa2xx_ac97_pcm_mic_mono_in = { .filter_data = &pxa2xx_ac97_pcm_aux_mic_mono_req, };
-#ifdef CONFIG_PM -static int pxa2xx_ac97_suspend(struct snd_soc_dai *dai) -{ - return pxa2xx_ac97_hw_suspend(); -} - -static int pxa2xx_ac97_resume(struct snd_soc_dai *dai) -{ - return pxa2xx_ac97_hw_resume(); -} - -#else -#define pxa2xx_ac97_suspend NULL -#define pxa2xx_ac97_resume NULL -#endif - -static int pxa2xx_ac97_probe(struct snd_soc_dai *dai) -{ - return pxa2xx_ac97_hw_probe(to_platform_device(dai->dev)); -} - -static int pxa2xx_ac97_remove(struct snd_soc_dai *dai) -{ - pxa2xx_ac97_hw_remove(to_platform_device(dai->dev)); - return 0; -} - static int pxa2xx_ac97_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params, struct snd_soc_dai *cpu_dai) @@ -185,10 +158,6 @@ static struct snd_soc_dai_driver pxa_ac97_dai_driver[] = { { .name = "pxa2xx-ac97", .ac97_control = 1, - .probe = pxa2xx_ac97_probe, - .remove = pxa2xx_ac97_remove, - .suspend = pxa2xx_ac97_suspend, - .resume = pxa2xx_ac97_resume, .playback = { .stream_name = "AC97 Playback", .channels_min = 2, @@ -246,6 +215,12 @@ static int pxa2xx_ac97_dev_probe(struct platform_device *pdev) return -ENXIO; }
+ ret = pxa2xx_ac97_hw_probe(pdev); + if (ret) { + dev_err(&pdev->dev, "PXA2xx AC97 hw probe error (%d)\n", ret); + return ret; + } + ret = snd_soc_set_ac97_ops(&pxa2xx_ac97_ops); if (ret != 0) return ret; @@ -262,15 +237,34 @@ static int pxa2xx_ac97_dev_remove(struct platform_device *pdev) { snd_soc_unregister_component(&pdev->dev); snd_soc_set_ac97_ops(NULL); + pxa2xx_ac97_hw_remove(pdev); return 0; }
+#ifdef CONFIG_PM_SLEEP +static int pxa2xx_ac97_dev_suspend(struct device *dev) +{ + return pxa2xx_ac97_hw_suspend(); +} + +static int pxa2xx_ac97_dev_resume(struct device *dev) +{ + return pxa2xx_ac97_hw_resume(); +} + +static SIMPLE_DEV_PM_OPS(pxa2xx_ac97_pm_ops, + pxa2xx_ac97_dev_suspend, pxa2xx_ac97_dev_resume); +#endif + static struct platform_driver pxa2xx_ac97_driver = { .probe = pxa2xx_ac97_dev_probe, .remove = pxa2xx_ac97_dev_remove, .driver = { .name = "pxa2xx-ac97", .owner = THIS_MODULE, +#ifdef CONFIG_PM_SLEEP + .pm = &pxa2xx_ac97_pm_ops, +#endif }, };
After convertion to snd_soc_register_card, platform driver should reference snd_soc_pm_ops callbacks to properly suspend/resume sound hardware. This was missed during conversion of PXA sound devices.
Signed-off-by: Dmitry Eremin-Solenikov dbaryshkov@gmail.com --- sound/soc/pxa/brownstone.c | 1 + sound/soc/pxa/corgi.c | 1 + sound/soc/pxa/e740_wm9705.c | 1 + sound/soc/pxa/e750_wm9705.c | 1 + sound/soc/pxa/e800_wm9712.c | 1 + sound/soc/pxa/imote2.c | 1 + sound/soc/pxa/mioa701_wm9713.c | 1 + sound/soc/pxa/palm27x.c | 1 + sound/soc/pxa/poodle.c | 1 + sound/soc/pxa/tosa.c | 1 + sound/soc/pxa/ttc-dkb.c | 1 + 11 files changed, 11 insertions(+)
diff --git a/sound/soc/pxa/brownstone.c b/sound/soc/pxa/brownstone.c index 5b7d969..08acdc2 100644 --- a/sound/soc/pxa/brownstone.c +++ b/sound/soc/pxa/brownstone.c @@ -163,6 +163,7 @@ static struct platform_driver mmp_driver = { .driver = { .name = "brownstone-audio", .owner = THIS_MODULE, + .pm = &snd_soc_pm_ops, }, .probe = brownstone_probe, .remove = brownstone_remove, diff --git a/sound/soc/pxa/corgi.c b/sound/soc/pxa/corgi.c index f4cce1e..1853d41 100644 --- a/sound/soc/pxa/corgi.c +++ b/sound/soc/pxa/corgi.c @@ -329,6 +329,7 @@ static struct platform_driver corgi_driver = { .driver = { .name = "corgi-audio", .owner = THIS_MODULE, + .pm = &snd_soc_pm_ops, }, .probe = corgi_probe, .remove = corgi_remove, diff --git a/sound/soc/pxa/e740_wm9705.c b/sound/soc/pxa/e740_wm9705.c index 70d799b..44b5c09 100644 --- a/sound/soc/pxa/e740_wm9705.c +++ b/sound/soc/pxa/e740_wm9705.c @@ -178,6 +178,7 @@ static struct platform_driver e740_driver = { .driver = { .name = "e740-audio", .owner = THIS_MODULE, + .pm = &snd_soc_pm_ops, }, .probe = e740_probe, .remove = e740_remove, diff --git a/sound/soc/pxa/e750_wm9705.c b/sound/soc/pxa/e750_wm9705.c index f94d2ab..c34e447 100644 --- a/sound/soc/pxa/e750_wm9705.c +++ b/sound/soc/pxa/e750_wm9705.c @@ -160,6 +160,7 @@ static struct platform_driver e750_driver = { .driver = { .name = "e750-audio", .owner = THIS_MODULE, + .pm = &snd_soc_pm_ops, }, .probe = e750_probe, .remove = e750_remove, diff --git a/sound/soc/pxa/e800_wm9712.c b/sound/soc/pxa/e800_wm9712.c index 8768a64..3137f80 100644 --- a/sound/soc/pxa/e800_wm9712.c +++ b/sound/soc/pxa/e800_wm9712.c @@ -150,6 +150,7 @@ static struct platform_driver e800_driver = { .driver = { .name = "e800-audio", .owner = THIS_MODULE, + .pm = &snd_soc_pm_ops, }, .probe = e800_probe, .remove = e800_remove, diff --git a/sound/soc/pxa/imote2.c b/sound/soc/pxa/imote2.c index eef1f7b..fd2f4ed 100644 --- a/sound/soc/pxa/imote2.c +++ b/sound/soc/pxa/imote2.c @@ -91,6 +91,7 @@ static struct platform_driver imote2_driver = { .driver = { .name = "imote2-audio", .owner = THIS_MODULE, + .pm = &snd_soc_pm_ops, }, .probe = imote2_probe, .remove = imote2_remove, diff --git a/sound/soc/pxa/mioa701_wm9713.c b/sound/soc/pxa/mioa701_wm9713.c index bbea778..160c524 100644 --- a/sound/soc/pxa/mioa701_wm9713.c +++ b/sound/soc/pxa/mioa701_wm9713.c @@ -215,6 +215,7 @@ static struct platform_driver mioa701_wm9713_driver = { .driver = { .name = "mioa701-wm9713", .owner = THIS_MODULE, + .pm = &snd_soc_pm_ops, }, };
diff --git a/sound/soc/pxa/palm27x.c b/sound/soc/pxa/palm27x.c index e1ffcdd..3284c4b 100644 --- a/sound/soc/pxa/palm27x.c +++ b/sound/soc/pxa/palm27x.c @@ -181,6 +181,7 @@ static struct platform_driver palm27x_wm9712_driver = { .driver = { .name = "palm27x-asoc", .owner = THIS_MODULE, + .pm = &snd_soc_pm_ops, }, };
diff --git a/sound/soc/pxa/poodle.c b/sound/soc/pxa/poodle.c index fafe463..c93e138 100644 --- a/sound/soc/pxa/poodle.c +++ b/sound/soc/pxa/poodle.c @@ -303,6 +303,7 @@ static struct platform_driver poodle_driver = { .driver = { .name = "poodle-audio", .owner = THIS_MODULE, + .pm = &snd_soc_pm_ops, }, .probe = poodle_probe, .remove = poodle_remove, diff --git a/sound/soc/pxa/tosa.c b/sound/soc/pxa/tosa.c index a3fe191..1d9c2ed 100644 --- a/sound/soc/pxa/tosa.c +++ b/sound/soc/pxa/tosa.c @@ -275,6 +275,7 @@ static struct platform_driver tosa_driver = { .driver = { .name = "tosa-audio", .owner = THIS_MODULE, + .pm = &snd_soc_pm_ops, }, .probe = tosa_probe, .remove = tosa_remove, diff --git a/sound/soc/pxa/ttc-dkb.c b/sound/soc/pxa/ttc-dkb.c index 13c9ee0..0b535b5 100644 --- a/sound/soc/pxa/ttc-dkb.c +++ b/sound/soc/pxa/ttc-dkb.c @@ -160,6 +160,7 @@ static struct platform_driver ttc_dkb_driver = { .driver = { .name = "ttc-dkb-audio", .owner = THIS_MODULE, + .pm = &snd_soc_pm_ops, }, .probe = ttc_dkb_probe, .remove = ttc_dkb_remove,
On Thu, Oct 17, 2013 at 02:01:35PM +0400, Dmitry Eremin-Solenikov wrote:
PXA25x also shows some problems when using interrupts during reset handling. Thus do not use interrupts on all pxa kinds (to detect codec ready state). Instead use a common mdelay-loop on all platforms to detect codecs becoming ready.
Applied all, thanks. I'm somewhat surprised nobody noticed the reset issues on the pxa25x before - this stuff all got really heavily tested back in the day.
Hello,
On Fri, Oct 18, 2013 at 3:36 AM, Mark Brown broonie@kernel.org wrote:
On Thu, Oct 17, 2013 at 02:01:35PM +0400, Dmitry Eremin-Solenikov wrote:
PXA25x also shows some problems when using interrupts during reset handling. Thus do not use interrupts on all pxa kinds (to detect codec ready state). Instead use a common mdelay-loop on all platforms to detect codecs becoming ready.
Applied all, thanks. I'm somewhat surprised nobody noticed the reset issues on the pxa25x before - this stuff all got really heavily tested back in the day.
Maybe it was some coincidence/whatever. Maybe it is some hardware issue of exact mask version. When I first started hacking around PXA, it was known that tosa sometimes does not reset the AC97 codec for the first time. However it looks like nobody bothered to debug that, because in most (~3/4) cases reset went through. Now some kernel changes made kernel hang looping in the interrupt, if the reset is not properly handled.
On Fri, Oct 18, 2013 at 03:53:31AM +0400, Dmitry Eremin-Solenikov wrote:
Maybe it was some coincidence/whatever. Maybe it is some hardware issue of exact mask version. When I first started hacking around PXA, it was known that tosa sometimes does not reset the AC97 codec for the first time. However it looks like nobody bothered to debug that, because in most (~3/4) cases reset went through. Now some kernel changes made kernel hang looping in the interrupt, if the reset is not properly handled.
Ah, now you mention that extra detail I do recall Wolfson CODEC drivers with suspicious extra resets - probably this issue was why.
participants (2)
-
Dmitry Eremin-Solenikov -
Mark Brown