[v4] ASoC: sgtl5000: Fix the cache handling

Commit Message

Fabio Estevam May 24, 2014, 7:16 p.m. UTC

From: Fabio Estevam <[e-mail]>

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 <[e-mail]>
Signed-off-by: Fabio Estevam <[e-mail]>
---
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(-)

Comments

Shawn Guo May 25, 2014, 8:13 a.m. UTC | #1

Fabio,

On Sat, May 24, 2014 at 04:16:00PM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Fabio Estevam <[e-mail]>

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

Lars-Peter Clausen May 26, 2014, 7:26 a.m. UTC | #2

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

Mark Brown May 26, 2014, 10:11 a.m. UTC | #3

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.

Fabio Estevam May 26, 2014, 1:28 p.m. UTC | #4

On Mon, May 26, 2014 at 4:26 AM, Lars-Peter Clausen <[e-mail]> 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.

Patch

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
  */