[alsa-devel] [PATCH v4] ASoC: sgtl5000: Fix the cache handling
From: Fabio Estevam fabio.estevam@freescale.com
Since commit e5d80e82e32e (ASoC: sgtl5000: Convert to use regmap directly) a kernel oops is observed after a suspend/resume sequence.
According to Mark Brown:
"Looking at the code what I'd expect to happen here is that set_bias_level() manages the cache enable, turning on cache only mode just before it turns the regulators off and restoring normal mode just after enabling them, then calling _restore_regs() after that. The resume call should just be a call to set_bias_level() then.."
Do as suggested and also complete the cache array with the missing registers so that sgtl5000_restore_regs() can properly cache the all the registers it needs.
Reported-by: Shawn Guo shawn.guo@freescale.com Signed-off-by: Fabio Estevam fabio.estevam@freescale.com --- Changes since v3: - Call sgtl5000_restore_regs from set_bias_level and let set_bias_level handle the cache handling. Changes since v2: - Use regmap_update_bits sound/soc/codecs/sgtl5000.c | 134 ++++++++++++++++++++++++-------------------- sound/soc/codecs/sgtl5000.h | 2 + 2 files changed, 76 insertions(+), 60 deletions(-)
diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c index 9626ee0..57bda9b 100644 --- a/sound/soc/codecs/sgtl5000.c +++ b/sound/soc/codecs/sgtl5000.c @@ -36,18 +36,32 @@
/* default value of sgtl5000 registers */ static const struct reg_default sgtl5000_reg_defaults[] = { + { SGTL5000_CHIP_DIG_POWER, 0x0000 }, { SGTL5000_CHIP_CLK_CTRL, 0x0008 }, { SGTL5000_CHIP_I2S_CTRL, 0x0010 }, { SGTL5000_CHIP_SSS_CTRL, 0x0010 }, + { SGTL5000_CHIP_ADCDAC_CTRL, 0x020c }, { SGTL5000_CHIP_DAC_VOL, 0x3c3c }, { SGTL5000_CHIP_PAD_STRENGTH, 0x015f }, + { SGTL5000_CHIP_ANA_ADC_CTRL, 0x0000 }, { SGTL5000_CHIP_ANA_HP_CTRL, 0x1818 }, { SGTL5000_CHIP_ANA_CTRL, 0x0111 }, + { SGTL5000_CHIP_LINREG_CTRL, 0x0000 }, + { SGTL5000_CHIP_REF_CTRL, 0x0000 }, + { SGTL5000_CHIP_MIC_CTRL, 0x0000 }, + { SGTL5000_CHIP_LINE_OUT_CTRL, 0x0000 }, { SGTL5000_CHIP_LINE_OUT_VOL, 0x0404 }, { SGTL5000_CHIP_ANA_POWER, 0x7060 }, { SGTL5000_CHIP_PLL_CTRL, 0x5000 }, + { SGTL5000_CHIP_CLK_TOP_CTRL, 0x0000 }, + { SGTL5000_CHIP_ANA_STATUS, 0x0000 }, + { SGTL5000_CHIP_SHORT_CTRL, 0x0000 }, + { SGTL5000_CHIP_ANA_TEST2, 0x0000 }, + { SGTL5000_DAP_CTRL, 0x0000 }, + { SGTL5000_DAP_PEQ, 0x0000 }, { SGTL5000_DAP_BASS_ENHANCE, 0x0040 }, { SGTL5000_DAP_BASS_ENHANCE_CTRL, 0x051f }, + { SGTL5000_DAP_AUDIO_EQ, 0x0000 }, { SGTL5000_DAP_SURROUND, 0x0040 }, { SGTL5000_DAP_EQ_BASS_BAND0, 0x002f }, { SGTL5000_DAP_EQ_BASS_BAND1, 0x002f }, @@ -55,6 +69,7 @@ static const struct reg_default sgtl5000_reg_defaults[] = { { SGTL5000_DAP_EQ_BASS_BAND3, 0x002f }, { SGTL5000_DAP_EQ_BASS_BAND4, 0x002f }, { SGTL5000_DAP_MAIN_CHAN, 0x8000 }, + { SGTL5000_DAP_MIX_CHAN, 0x0000 }, { SGTL5000_DAP_AVC_CTRL, 0x0510 }, { SGTL5000_DAP_AVC_THRESHOLD, 0x1473 }, { SGTL5000_DAP_AVC_ATTACK, 0x0028 }, @@ -897,6 +912,64 @@ static int ldo_regulator_remove(struct snd_soc_codec *codec) #endif
/* + * restore all sgtl5000 registers, + * since a big hole between dap and regular registers, + * we will restore them respectively. + */ +static int sgtl5000_restore_regs(struct snd_soc_codec *codec) +{ + u16 reg; + struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec); + + /* restore regular registers */ + for (reg = 0; reg <= SGTL5000_CHIP_SHORT_CTRL; reg += 2) { + + /* These regs should restore in particular order */ + if (reg == SGTL5000_CHIP_ANA_POWER || + reg == SGTL5000_CHIP_CLK_CTRL || + reg == SGTL5000_CHIP_LINREG_CTRL || + reg == SGTL5000_CHIP_LINE_OUT_CTRL || + reg == SGTL5000_CHIP_REF_CTRL) + continue; + + regmap_update_bits(sgtl5000->regmap, reg, reg, 0); + } + + /* restore dap registers */ + for (reg = SGTL5000_DAP_REG_OFFSET; reg < SGTL5000_MAX_REG_OFFSET; reg += 2) + regmap_update_bits(sgtl5000->regmap, reg, reg, 0); + + /* + * restore these regs according to the power setting sequence in + * sgtl5000_set_power_regs() and clock setting sequence in + * sgtl5000_set_clock(). + * + * The order of restore is: + * 1. SGTL5000_CHIP_CLK_CTRL MCLK_FREQ bits (1:0) should be restore after + * SGTL5000_CHIP_ANA_POWER PLL bits set + * 2. SGTL5000_CHIP_LINREG_CTRL should be set before + * SGTL5000_CHIP_ANA_POWER LINREG_D restored + * 3. SGTL5000_CHIP_REF_CTRL controls Analog Ground Voltage, + * prefer to resotre it after SGTL5000_CHIP_ANA_POWER restored + */ + regmap_update_bits(sgtl5000->regmap, SGTL5000_CHIP_LINREG_CTRL, + SGTL5000_CHIP_LINREG_CTRL, 0); + + regmap_update_bits(sgtl5000->regmap, SGTL5000_CHIP_ANA_POWER, + SGTL5000_PLL_POWERUP_BIT, 0); + + regmap_update_bits(sgtl5000->regmap, SGTL5000_CHIP_CLK_CTRL, + SGTL5000_MCLK_FREQ_PLL, 0); + + regmap_update_bits(sgtl5000->regmap, SGTL5000_CHIP_REF_CTRL, + SGTL5000_CHIP_REF_CTRL, 0); + + regmap_update_bits(sgtl5000->regmap, SGTL5000_CHIP_LINE_OUT_CTRL, + SGTL5000_CHIP_LINE_OUT_CTRL, 0); + return 0; +} + +/* * set dac bias * common state changes: * startup: @@ -926,6 +999,7 @@ static int sgtl5000_set_bias_level(struct snd_soc_codec *codec, udelay(10);
regcache_cache_only(sgtl5000->regmap, false); + sgtl5000_restore_regs(codec);
ret = regcache_sync(sgtl5000->regmap); if (ret != 0) { @@ -1068,71 +1142,11 @@ static int sgtl5000_suspend(struct snd_soc_codec *codec) return 0; }
-/* - * restore all sgtl5000 registers, - * since a big hole between dap and regular registers, - * we will restore them respectively. - */ -static int sgtl5000_restore_regs(struct snd_soc_codec *codec) -{ - u16 *cache = codec->reg_cache; - u16 reg; - - /* restore regular registers */ - for (reg = 0; reg <= SGTL5000_CHIP_SHORT_CTRL; reg += 2) { - - /* These regs should restore in particular order */ - if (reg == SGTL5000_CHIP_ANA_POWER || - reg == SGTL5000_CHIP_CLK_CTRL || - reg == SGTL5000_CHIP_LINREG_CTRL || - reg == SGTL5000_CHIP_LINE_OUT_CTRL || - reg == SGTL5000_CHIP_REF_CTRL) - continue; - - snd_soc_write(codec, reg, cache[reg]); - } - - /* restore dap registers */ - for (reg = SGTL5000_DAP_REG_OFFSET; reg < SGTL5000_MAX_REG_OFFSET; reg += 2) - snd_soc_write(codec, reg, cache[reg]); - - /* - * restore these regs according to the power setting sequence in - * sgtl5000_set_power_regs() and clock setting sequence in - * sgtl5000_set_clock(). - * - * The order of restore is: - * 1. SGTL5000_CHIP_CLK_CTRL MCLK_FREQ bits (1:0) should be restore after - * SGTL5000_CHIP_ANA_POWER PLL bits set - * 2. SGTL5000_CHIP_LINREG_CTRL should be set before - * SGTL5000_CHIP_ANA_POWER LINREG_D restored - * 3. SGTL5000_CHIP_REF_CTRL controls Analog Ground Voltage, - * prefer to resotre it after SGTL5000_CHIP_ANA_POWER restored - */ - snd_soc_write(codec, SGTL5000_CHIP_LINREG_CTRL, - cache[SGTL5000_CHIP_LINREG_CTRL]); - - snd_soc_write(codec, SGTL5000_CHIP_ANA_POWER, - cache[SGTL5000_CHIP_ANA_POWER]); - - snd_soc_write(codec, SGTL5000_CHIP_CLK_CTRL, - cache[SGTL5000_CHIP_CLK_CTRL]); - - snd_soc_write(codec, SGTL5000_CHIP_REF_CTRL, - cache[SGTL5000_CHIP_REF_CTRL]); - - snd_soc_write(codec, SGTL5000_CHIP_LINE_OUT_CTRL, - cache[SGTL5000_CHIP_LINE_OUT_CTRL]); - return 0; -} - static int sgtl5000_resume(struct snd_soc_codec *codec) { /* Bring the codec back up to standby to enable regulators */ sgtl5000_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
- /* Restore registers by cached in memory */ - sgtl5000_restore_regs(codec); return 0; } #else diff --git a/sound/soc/codecs/sgtl5000.h b/sound/soc/codecs/sgtl5000.h index 2f8c889..72b68c0 100644 --- a/sound/soc/codecs/sgtl5000.h +++ b/sound/soc/codecs/sgtl5000.h @@ -341,6 +341,8 @@ #define SGTL5000_ADC_POWERUP 0x0002 #define SGTL5000_LINE_OUT_POWERUP 0x0001
+#define SGTL5000_PLL_POWERUP_BIT 10 + /* * SGTL5000_CHIP_PLL_CTRL */
Fabio,
On Sat, May 24, 2014 at 04:16:00PM -0300, Fabio Estevam wrote:
From: Fabio Estevam fabio.estevam@freescale.com
Since commit e5d80e82e32e (ASoC: sgtl5000: Convert to use regmap directly) a kernel oops is observed after a suspend/resume sequence.
According to Mark Brown:
"Looking at the code what I'd expect to happen here is that set_bias_level() manages the cache enable, turning on cache only mode just before it turns the regulators off and restoring normal mode just after enabling them, then calling _restore_regs() after that. The resume call should just be a call to set_bias_level() then.."
Do as suggested and also complete the cache array with the missing registers so that sgtl5000_restore_regs() can properly cache the all the registers it needs.
Reported-by: Shawn Guo shawn.guo@freescale.com Signed-off-by: Fabio Estevam fabio.estevam@freescale.com
I'm not sure if it's the problem of this patch, but when I test suspend/resume cycle with a wave playback running background, the playback goes abnormal after system resumes back. I did this test with your patch 'ASoC: fsl_ssi: Add suspend/resume support' applied.
The same test goes fine with wm8962 on imx6q-sabresd board.
Shawn
On 05/24/2014 09:16 PM, Fabio Estevam wrote: [...]
/*
- restore all sgtl5000 registers,
- since a big hole between dap and regular registers,
- we will restore them respectively.
- */
+static int sgtl5000_restore_regs(struct snd_soc_codec *codec) +{
- u16 reg;
- struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
- /* restore regular registers */
- for (reg = 0; reg <= SGTL5000_CHIP_SHORT_CTRL; reg += 2) {
/* These regs should restore in particular order */if (reg == SGTL5000_CHIP_ANA_POWER ||reg == SGTL5000_CHIP_CLK_CTRL ||reg == SGTL5000_CHIP_LINREG_CTRL ||reg == SGTL5000_CHIP_LINE_OUT_CTRL ||reg == SGTL5000_CHIP_REF_CTRL)continue;regmap_update_bits(sgtl5000->regmap, reg, reg, 0);
This makes even less sense than before. The third parameter of regmap_update_bits() is the mask for the update operation and the last parameter is the value. regmap_update_bits() basically does this reg = (reg & ~mask) | value;
I think the issue with this chip is that it wants a special sequence in which the registers are written when syncing the cache to the hardware. The first thing to check is probably if that is actually necessary. If not just drop the whole restore_regs thing. If it is necessary its probably worth investigating whether it makes sense to support custom sync sequences in regmap. We already have regcache_sync_region() so maybe add a regcache_sync_regions() which takes a array of struct regmap_range. And syncs the registers in the order of the ranges.
- Lars
On Mon, May 26, 2014 at 09:26:33AM +0200, Lars-Peter Clausen wrote:
I think the issue with this chip is that it wants a special sequence in which the registers are written when syncing the cache to the hardware. The first thing to check is probably if that is actually necessary. If not just drop the whole restore_regs thing. If it is necessary its probably worth investigating whether it makes sense to support custom sync sequences in regmap. We already have regcache_sync_region() so maybe add a regcache_sync_regions() which takes a array of struct regmap_range. And syncs the registers in the order of the ranges.
There's a few devices which need some sequencing on restore but it's usually just for a very small subset of registers (and sometimes need delays between the writes and things) so what tends to happen is a short open coded sequences that do the ordering sensitive portions of the sequence followed by a regcache_sync() which covers everything else.
On Mon, May 26, 2014 at 4:26 AM, Lars-Peter Clausen lars@metafoo.de wrote:
I think the issue with this chip is that it wants a special sequence in which the registers are written when syncing the cache to the hardware. The first thing to check is probably if that is actually necessary. If not just drop the whole restore_regs thing. If it is necessary its probably worth
Good point. After adding the missing entries into the sgtl5000_reg_defaults[] array we can simply remove sgtl5000_restore_regs() and suspend/resume works fine.
Thanks for the suggestion.
participants (4)
-
Fabio Estevam -
Lars-Peter Clausen -
Mark Brown -
Shawn Guo