ASoC: wm8962: Do not clear SYSCLK_ENA owned by DAPM in configure_bclk()
Subsystems (4)
PATCH 1/1 ASoC: wm8962: Do not clear SYSCLK_ENA owned by DAPM in configure_bclk()
wm8962_configure_bclk() provisionally sets SYSCLK_ENA so that the read-only DSPCLK_DIV field becomes valid, then clears it again whenever the bias level is below SND_SOC_BIAS_ON. It never checks whether the bit was already set.
It can be: the "SYSCLK" DAPM supply widget owns that bit, and wm8962_mic_detect() force-enables the pin. On boards that call it (the Tegra machine driver, i.e. Microsoft Surface RT and Surface 2) DAPM sets SYSCLK_ENA once at card init and, because the widget then never changes power state, never writes it again. The first stream's transition to SND_SOC_BIAS_PREPARE runs configure_bclk(), which clears the bit behind DAPM's back. From then on every stream plays with SYSCLK off: the DACs do not run, CP_ENA refuses to latch, hp_event() reports "DC servo timed out", and the codec is silent although every register DAPM believes in looks right.
Remember whether SYSCLK_ENA was set on entry and only undo our own provisional enable.
The bug was located with the help of an LLM coding assistant, by diffing the driver's regmap cache against raw I2C reads of the chip during playback; the assistant also drafted this patch and the changelog. The analysis was confirmed, and the fix written and tested, on real hardware.
Verified on a Microsoft Surface 2 (Tegra 4, WM8962 rev F): with the bit forced on over I2C during a stream the DC servo completes in ~56 ms and audio is heard; with this patch applied the DC servo completes on every stream and there is no more "DC servo timed out".
---
Notes:
- Found and verified on a Microsoft Surface 2 (Tegra 4); the same silence is reported on Microsoft Surface RT, whose machine driver is the same. Background and the full register-level analysis (regmap cache vs. raw I2C reads during playback):
https://github.com/grate-driver/linux/issues/141
- Boards that do not call wm8962_mic_detect() are unaffected: there SYSCLK_ENA is never already set below SND_SOC_BIAS_ON, sysclk_was_ena is false, and the sequence is bit-identical to the current one.
- The deeper question is whether tegra_wm8962 should be calling wm8962_mic_detect() at all on these boards, but the codec should not be clearing a bit DAPM owns regardless, so this fix stands on its own.
- Tooling, per Documentation/process/generated-content.rst: an LLM coding assistant did the register-diff analysis that found the bug and drafted this patch and changelog, over an interactive session on the affected hardware rather than from a single prompt. Every register claim above was checked against the chip over I2C, and the fix was built and listened to on the machine before sending. I understand the change and stand behind it.
sound/soc/codecs/wm8962.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
sound/soc/codecs/wm8962.c +16 -2
@@ -2472,6 +2472,8 @@ static void wm8962_configure_bclk(struct snd_soc_component *component)int clocking2 =0 ;int clocking4 =0 ;int aif2 =0 ; +int sysclk; +bool sysclk_was_ena;if (!wm8962->sysclk_rate) {dev_dbg (component->dev,"No SYSCLK configured\n" );
@@ -2504,8 +2506,19 @@ static void wm8962_configure_bclk(struct snd_soc_component *component)/* DSPCLK_DIV can be only generated correctly after enabling SYSCLK. * So we here provisionally enable it and then disable it afterward * if current bias_level hasn't reached SND_SOC_BIAS_ON. +* +* SYSCLK_ENA is owned by the "SYSCLK" DAPM supply widget, which may +* already have it set even below SND_SOC_BIAS_ON: wm8962_mic_detect() +* force-enables that pin, so on boards using it (the Tegra machine +* driver) the bit is set once at card init and DAPM never writes it +* again. If we unconditionally clear it here, SYSCLK stays off for +* every stream, DAPM still believes it is on, and the codec is +* silent with "DC servo timed out" errors. Only undo what we did. */ -if (snd_soc_dapm_get_bias_level (dapm) !=SND_SOC_BIAS_ON ) + sysclk =snd_soc_component_read (component,WM8962_CLOCKING2 ); + sysclk_was_ena = sysclk >=0 && (sysclk &WM8962_SYSCLK_ENA ); +if (!sysclk_was_ena && +snd_soc_dapm_get_bias_level (dapm) !=SND_SOC_BIAS_ON )snd_soc_component_update_bits (component,WM8962_CLOCKING2 ,WM8962_SYSCLK_ENA_MASK ,WM8962_SYSCLK_ENA );
@@ -2519,7 +2532,8 @@ static void wm8962_configure_bclk(struct snd_soc_component *component)usleep_range (500 ,1000 ); dspclk =snd_soc_component_read (component,WM8962_CLOCKING1 ); -if (snd_soc_dapm_get_bias_level (dapm) !=SND_SOC_BIAS_ON ) +if (!sysclk_was_ena && +snd_soc_dapm_get_bias_level (dapm) !=SND_SOC_BIAS_ON )snd_soc_component_update_bits (component,WM8962_CLOCKING2 ,WM8962_SYSCLK_ENA_MASK ,0 );
base-commit: 0e2bf66c2f6326c0626c9db22530954ef5c64b08
Signature · 1 line
2.53.0
Quoted 10 lines · Maxime Douailin
sound/soc/codecs/wm8962.c +3
-0+int sysclk; +bool sysclk_was_ena;if (!wm8962->sysclk_rate) {dev_dbg (component->dev,"No SYSCLK configured\n" );@@ -2504,8 +2506,19 @@ static void wm8962_configure_bclk(struct snd_soc_component *component)/* DSPCLK_DIV can be only generated correctly after enabling SYSCLK. * So we here provisionally enable it and then disable it afterward * if current bias_level hasn't reached SND_SOC_BIAS_ON. +*
sound/soc/codecs/wm8962.c static void wm8962_configure_bclk(struct snd_soc_component *component) · line 2518
Quoted 10 lines
sound/soc/codecs/wm8962.c +8
-1+* SYSCLK_ENA is owned by the "SYSCLK" DAPM supply widget, which may +* already have it set even below SND_SOC_BIAS_ON: wm8962_mic_detect() +* force-enables that pin, so on boards using it (the Tegra machine +* driver) the bit is set once at card init and DAPM never writes it +* again. If we unconditionally clear it here, SYSCLK stays off for +* every stream, DAPM still believes it is on, and the codec is +* silent with "DC servo timed out" errors. Only undo what we did. */ -if (snd_soc_dapm_get_bias_level (dapm) !=SND_SOC_BIAS_ON ) + sysclk =snd_soc_component_read (component,WM8962_CLOCKING2 );
Probably should error out if we fail the read here.
sound/soc/codecs/wm8962.c static void wm8962_configure_bclk(struct snd_soc_component *component) · line 2521
Quoted 3 lines
sound/soc/codecs/wm8962.c +3
-0+ sysclk_was_ena = sysclk >=0 && (sysclk &WM8962_SYSCLK_ENA ); +if (!sysclk_was_ena && +snd_soc_dapm_get_bias_level (dapm) !=SND_SOC_BIAS_ON )
Do we still need the get_bias_level here? I feel like it was just a proxy for if the sysclk is enabled so can just be replaced now.
sound/soc/codecs/wm8962.c static void wm8962_configure_bclk(struct snd_soc_component *component) · line 2536
Quoted 10 lines
sound/soc/codecs/wm8962.c +2
-1snd_soc_component_update_bits (component,WM8962_CLOCKING2 ,WM8962_SYSCLK_ENA_MASK ,WM8962_SYSCLK_ENA );@@ -2519,7 +2532,8 @@ static void wm8962_configure_bclk(struct snd_soc_component *component)usleep_range (500 ,1000 ); dspclk =snd_soc_component_read (component,WM8962_CLOCKING1 ); -if (snd_soc_dapm_get_bias_level (dapm) !=SND_SOC_BIAS_ON ) +if (!sysclk_was_ena && +snd_soc_dapm_get_bias_level (dapm) !=SND_SOC_BIAS_ON )
Ditto here, just restore the original state. In fact, probably a neater way to do the whole thing would be to store the original state, unconditionally enable the sysclk, then restore the original state.
Also do we need to hold the dapm mutex across the call to call to wm8962_configure_bclk() in hw_params? I would worry slightly that can race with a DAPM sequence.
Thanks,
Charles