[alsa-devel] [PATCH] ALSA: ASoC: fix PXA SSP port resume
Invalidate the cached hardware format on resume for PXA SSP ports. Otherwise hw_params() will bail out early at the next stream start, leaving the registers in a bogus state.
Signed-off-by: Daniel Mack daniel@caiaq.de Cc: Eric Miao eric.y.miao@gmail.com Cc: Mark Brown broonie@opensource.wolfsonmicro.com Cc: Philipp Zabel philipp.zabel@gmail.com --- sound/soc/pxa/pxa-ssp.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c index 3bd7712..ebde673 100644 --- a/sound/soc/pxa/pxa-ssp.c +++ b/sound/soc/pxa/pxa-ssp.c @@ -146,6 +146,9 @@ static int pxa_ssp_resume(struct snd_soc_dai *cpu_dai) { struct ssp_priv *priv = cpu_dai->private_data;
+ /* the cached format information is invalid now */ + priv->dai_fmt = (unsigned int) -1; + if (!cpu_dai->active) return 0;
On Thu, Jan 28, 2010 at 06:34:18PM +0800, Daniel Mack wrote:
Invalidate the cached hardware format on resume for PXA SSP ports. Otherwise hw_params() will bail out early at the next stream start, leaving the registers in a bogus state.
Would it not be better to write the cached state back to the hardware? Putting an invalid value in the cache seems like asking for trouble.
On Fri, Jan 29, 2010 at 10:13:08AM +0000, Mark Brown wrote:
On Thu, Jan 28, 2010 at 06:34:18PM +0800, Daniel Mack wrote:
Invalidate the cached hardware format on resume for PXA SSP ports. Otherwise hw_params() will bail out early at the next stream start, leaving the registers in a bogus state.
Would it not be better to write the cached state back to the hardware? Putting an invalid value in the cache seems like asking for trouble.
Hmm. I considered that, but the reason why the system failed to resume in my case was that the ssp port was not active at this time. Otherwise, the suspend/resume code would have already done the right thing.
And as the next client will indirectly call hw_params() again, the code there will do the right thing with the invalidated cache.
Restoring the register values from priv->dai_fmt would imply adding code to do everything what the other bit-fiddling functions do in a reverse manner. Which is something I'd like to avoid :)
Another idea is to unconditionally save and restore the register set, and deal with possible side-effects. Not sure what's really better.
Daniel
On Fri, Jan 29, 2010 at 12:08:36PM +0100, Daniel Mack wrote:
And as the next client will indirectly call hw_params() again, the code there will do the right thing with the invalidated cache.
Yeah, I know the code is actually reasonably safe - it just doesn't give me the warm and fuzzies to do the invalidation by programming a random value in there simply on the off chance that at some point in the future that winds up being a valid value and so the same problem recurs.
Restoring the register values from priv->dai_fmt would imply adding code to do everything what the other bit-fiddling functions do in a reverse manner. Which is something I'd like to avoid :)
Oh, right. I'd forgotten that that was what it was actually storing.
Another idea is to unconditionally save and restore the register set, and deal with possible side-effects. Not sure what's really better.
I think that'd be the the most robust solution with the way things are structured in the driver.
Unconditionally save the register states in resume and restore them again at resume time. Register contents are not preserved over suspend, and the driver takes false assumptions about them otherwise.
The clock must be enabled to access the register block.
Signed-off-by: Daniel Mack daniel@caiaq.de Cc: Eric Miao eric.y.miao@gmail.com Cc: Mark Brown broonie@opensource.wolfsonmicro.com Cc: Philipp Zabel philipp.zabel@gmail.com --- sound/soc/pxa/pxa-ssp.c | 12 +++++++----- 1 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c index 3bd7712..e69397f 100644 --- a/sound/soc/pxa/pxa-ssp.c +++ b/sound/soc/pxa/pxa-ssp.c @@ -135,10 +135,11 @@ static int pxa_ssp_suspend(struct snd_soc_dai *cpu_dai) struct ssp_priv *priv = cpu_dai->private_data;
if (!cpu_dai->active) - return 0; + clk_enable(priv->dev.ssp->clk);
ssp_save_state(&priv->dev, &priv->state); clk_disable(priv->dev.ssp->clk); + return 0; }
@@ -146,12 +147,13 @@ static int pxa_ssp_resume(struct snd_soc_dai *cpu_dai) { struct ssp_priv *priv = cpu_dai->private_data;
- if (!cpu_dai->active) - return 0; - clk_enable(priv->dev.ssp->clk); ssp_restore_state(&priv->dev, &priv->state); - ssp_enable(&priv->dev); + + if (cpu_dai->active) + ssp_enable(&priv->dev); + else + clk_disable(priv->dev.ssp->clk);
return 0; }
Looks fine except for commit message (sorry to nitpick).
On Tue, 2010-02-02 at 11:32 +0800, Daniel Mack wrote:
Unconditionally save the register states in resume
I assume you mean suspend here ?
and restore them again at resume time. Register contents are not preserved over suspend,
were not being preserved
and the driver takes false assumptions about them otherwise.
Thanks
Liam
Unconditionally save the register states when suspending and restore them again at resume time. Register contents were not preserved over suspend, and hence the driver takes false assumptions about them.
The clock must be enabled to access the register block.
Signed-off-by: Daniel Mack daniel@caiaq.de Cc: Eric Miao eric.y.miao@gmail.com Cc: Mark Brown broonie@opensource.wolfsonmicro.com Cc: Philipp Zabel philipp.zabel@gmail.com --- sound/soc/pxa/pxa-ssp.c | 12 +++++++----- 1 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c index 3bd7712..e69397f 100644 --- a/sound/soc/pxa/pxa-ssp.c +++ b/sound/soc/pxa/pxa-ssp.c @@ -135,10 +135,11 @@ static int pxa_ssp_suspend(struct snd_soc_dai *cpu_dai) struct ssp_priv *priv = cpu_dai->private_data;
if (!cpu_dai->active) - return 0; + clk_enable(priv->dev.ssp->clk);
ssp_save_state(&priv->dev, &priv->state); clk_disable(priv->dev.ssp->clk); + return 0; }
@@ -146,12 +147,13 @@ static int pxa_ssp_resume(struct snd_soc_dai *cpu_dai) { struct ssp_priv *priv = cpu_dai->private_data;
- if (!cpu_dai->active) - return 0; - clk_enable(priv->dev.ssp->clk); ssp_restore_state(&priv->dev, &priv->state); - ssp_enable(&priv->dev); + + if (cpu_dai->active) + ssp_enable(&priv->dev); + else + clk_disable(priv->dev.ssp->clk);
return 0; }
On Tue, Feb 02, 2010 at 06:45:27PM +0800, Daniel Mack wrote:
Unconditionally save the register states when suspending and restore them again at resume time. Register contents were not preserved over suspend, and hence the driver takes false assumptions about them.
The clock must be enabled to access the register block.
Applied, thanks.
participants (3)
-
Daniel Mack -
Liam Girdwood -
Mark Brown