wm8904 and output volume control
Hello, I have found that output volume is set to the default value after a limited time (~4 seconds) after play stop. I have analyzed what is happening: - at first play the volume is the expected one - After stopping playing + ~4 seconds wm8904_set_bias_level (..., SND_SOC_BIAS_OFF) is called - Chip is reset and regulator is switched off if "possible" (not in our case, it is important to note) - at second play wm8904_set_bias_level (..., SND_SOC_BIAS_STANDBY) is called. wm8904_hw_params is also called just before.
I see that all I2C transactions are good, registers are written as expected, especially volume control register (even the silly HPOUT_VU bit, to do a volume update is correctly written). Reading out this register, using i2cget (bypassing regcache), report the "expected" value. But the real output volume correspond to the default value (hw default, that it is the same value in wm8904_reg_defaults structure).
I checked that SYSCLK_ENA is 0 during register writing (needed if MCLK/SYSCLK is not present). I do not know if there is some other constrain on these registers.
If I rewrite the volume control register, a second time, the volume is set (tested with i2cset, from user space. And from kernel space, bypassing regcache).
I resolve also by reverting e9149b8c00d2 ("ASoC: wm8904: fix regcache handling"). I'm not here to say that the commit is wrong. I analyzed it and seem perfect (in few words it align the hw with the regcache avoiding potential incoherence).
I'm trying to compare first play and second play register programming sequence. They are similar but not the same. But on the first play the volume output is good.
Do someone remember something similar fixed on another codec? Any idea is appreciated. Thank you. Best regards,
Emanuele Ghidoli
On Sat, Dec 17, 2022 at 12:47:14AM +0100, Emanuele Ghidoli wrote:
Hello, I have found that output volume is set to the default value after a limited time (~4 seconds) after play stop. I have analyzed what is happening:
- at first play the volume is the expected one
- After stopping playing + ~4 seconds wm8904_set_bias_level (...,
SND_SOC_BIAS_OFF) is called
- Chip is reset and regulator is switched off if "possible" (not in
our case, it is important to note)
- at second play wm8904_set_bias_level (..., SND_SOC_BIAS_STANDBY)
is called. wm8904_hw_params is also called just before.
I see that all I2C transactions are good, registers are written as expected, especially volume control register (even the silly HPOUT_VU bit, to do a volume update is correctly written). Reading out this register, using i2cget (bypassing regcache), report the "expected" value. But the real output volume correspond to the default value (hw default, that it is the same value in wm8904_reg_defaults structure).
I checked that SYSCLK_ENA is 0 during register writing (needed if MCLK/SYSCLK is not present). I do not know if there is some other constrain on these registers.
Yes this might be my guess as well, I wonder if the HPOUT_VU bit needs SYSCLK to be present to take effect.
If I rewrite the volume control register, a second time, the volume is set (tested with i2cset, from user space. And from kernel space, bypassing regcache).
When you write the value the second time is that still before SYSCLK is present?
Also does just writing one of HPOUT volumes cause the other to get updated? The HPOUT_VU bit should trigger an update to both.
I resolve also by reverting e9149b8c00d2 ("ASoC: wm8904: fix regcache handling"). I'm not here to say that the commit is wrong. I analyzed it and seem perfect (in few words it align the hw with the regcache avoiding potential incoherence).
Yeah I think that commit is fine, I would wager that your system doesn't turn off the regulators so without that commit the register retains the old volume across the "power down".
Thanks, Charles
On 19/12/2022 10:58, Charles Keepax wrote:
On Sat, Dec 17, 2022 at 12:47:14AM +0100, Emanuele Ghidoli wrote:
Hello, I have found that output volume is set to the default value after a limited time (~4 seconds) after play stop. I have analyzed what is happening:
- at first play the volume is the expected one
- After stopping playing + ~4 seconds wm8904_set_bias_level (...,
SND_SOC_BIAS_OFF) is called
- Chip is reset and regulator is switched off if "possible" (not in
our case, it is important to note)
- at second play wm8904_set_bias_level (..., SND_SOC_BIAS_STANDBY)
is called. wm8904_hw_params is also called just before.
I see that all I2C transactions are good, registers are written as expected, especially volume control register (even the silly HPOUT_VU bit, to do a volume update is correctly written). Reading out this register, using i2cget (bypassing regcache), report the "expected" value. But the real output volume correspond to the default value (hw default, that it is the same value in wm8904_reg_defaults structure).
I checked that SYSCLK_ENA is 0 during register writing (needed if MCLK/SYSCLK is not present). I do not know if there is some other constrain on these registers.
Yes this might be my guess as well, I wonder if the HPOUT_VU bit needs SYSCLK to be present to take effect.
If I rewrite the volume control register, a second time, the volume is set (tested with i2cset, from user space. And from kernel space, bypassing regcache).
When you write the value the second time is that still before SYSCLK is present?
Also does just writing one of HPOUT volumes cause the other to get updated? The HPOUT_VU bit should trigger an update to both.
I resolve also by reverting e9149b8c00d2 ("ASoC: wm8904: fix regcache handling"). I'm not here to say that the commit is wrong. I analyzed it and seem perfect (in few words it align the hw with the regcache avoiding potential incoherence).
Yeah I think that commit is fine, I would wager that your system doesn't turn off the regulators so without that commit the register retains the old volume across the "power down".
Thanks, Charles
Hello Charles, thank you.
With this patch (draft) seem the "bug" is fixed: (bug that is present, for sure, also with an effective regulator)
diff --git a/sound/soc/codecs/wm8904.c b/sound/soc/codecs/wm8904.c index ca6a01a230af..33452daf1ae8 100644 --- a/sound/soc/codecs/wm8904.c +++ b/sound/soc/codecs/wm8904.c @@ -1903,6 +1903,7 @@ static int wm8904_set_bias_level(struct snd_soc_component *component, }
regcache_cache_only(wm8904->regmap, false); + regcache_sync_region(wm8904->regmap, WM8904_CLASS_W_0, WM8904_CLASS_W_0); regcache_sync(wm8904->regmap);
/* Enable bias */
I infer, from datasheet, that volume update is applied in different way based on charge pump dynamic vs register control (CP_DYN_PWR bit in CLASS_W register): "Under Register control, the HPOUTL_VOL, HPOUTR_VOL, LINEOUTL_VOL and LINEOUTR_VOL register settings are used to control the charge pump mode of operation. Under Dynamic control, the audio signal level in the DAC is used to control the charge pump mode of operation."
The second sentence do not explain that volume register is still considered by the component but likely in a different way.
It is important to note that I trace I2C transactions and, without the patch, the CLASS_W register is written JUST after volume update registers (with the patch is written before and after).
At this point I have no doubt that we have to update that register before writing volume.
I guess: is there another way to do same/similar thing (in a better way, like just force write to that register a little bit before of volume update. I walk around regmap/regcache but I do not find a different solution)?
We must pay attention that cached value of this register can be changed by widget "Class G" (my patch should work also if you toggle this widget).
Thank you, best regards.
Emanuele Ghidoli
On Mon, Dec 19, 2022 at 04:20:10PM +0100, Emanuele Ghidoli wrote:
On 19/12/2022 10:58, Charles Keepax wrote:
On Sat, Dec 17, 2022 at 12:47:14AM +0100, Emanuele Ghidoli wrote:
With this patch (draft) seem the "bug" is fixed: (bug that is present, for sure, also with an effective regulator)
diff --git a/sound/soc/codecs/wm8904.c b/sound/soc/codecs/wm8904.c index ca6a01a230af..33452daf1ae8 100644 --- a/sound/soc/codecs/wm8904.c +++ b/sound/soc/codecs/wm8904.c @@ -1903,6 +1903,7 @@ static int wm8904_set_bias_level(struct snd_soc_component *component, }
regcache_cache_only(wm8904->regmap, false);
regcache_sync_region(wm8904->regmap,WM8904_CLASS_W_0, WM8904_CLASS_W_0); regcache_sync(wm8904->regmap);
/* Enable bias */
That is some good detective work.
I infer, from datasheet, that volume update is applied in different way based on charge pump dynamic vs register control (CP_DYN_PWR bit in CLASS_W register): "Under Register control, the HPOUTL_VOL, HPOUTR_VOL, LINEOUTL_VOL and LINEOUTR_VOL register settings are used to control the charge pump mode of operation. Under Dynamic control, the audio signal level in the DAC is used to control the charge pump mode of operation."
The second sentence do not explain that volume register is still considered by the component but likely in a different way.
It is important to note that I trace I2C transactions and, without the patch, the CLASS_W register is written JUST after volume update registers (with the patch is written before and after).
At this point I have no doubt that we have to update that register before writing volume.
Hmm... I think my only concern here is this feels a bit counter intuitive, the default value is described as "controlled by volume register settings" and we are saying in that situation the volume registers don't seem to update properly. That is far from impossible but I think we should perhaps poke a little more to make sure we understand the bounds here.
I see that that CP_DYN_PWR bit is disabled when audio is going through one of the bypass paths. Would you be able to enable one of the bypass paths and see if you can manually adjust the volume on the headphone output, with a bypass path active?
Would also perhaps be interesting as a test to completely remove the write to CP_DYN_PWR from probe and leave things set to manual and see how the volume behaves then?
I guess the interests here are to find out if the SYSCLK is involved at all. And if the issue is changing CP_DYN_PWR after a volume update is what causes the problem. I could perhaps see a situation where the volume update data is sent to a different place depending on the value of CP_DYN_PWR, meaning that changing that bit after doing a volume update causes the volume data to be lost, but that implies we might need a fix for the Class G widget as well.
I guess: is there another way to do same/similar thing (in a better way, like just force write to that register a little bit before of volume update. I walk around regmap/regcache but I do not find a different solution)?
We must pay attention that cached value of this register can be changed by widget "Class G" (my patch should work also if you toggle this widget).
I think assuming we get a point that the requirement is simply that CLASS_W needs written before the volume, the change as you propose it looks good. But I suspect there is a little more going on here.
Thanks, Charles
On 20/12/2022 11:00, Charles Keepax wrote:
On Mon, Dec 19, 2022 at 04:20:10PM +0100, Emanuele Ghidoli wrote:
On 19/12/2022 10:58, Charles Keepax wrote:
On Sat, Dec 17, 2022 at 12:47:14AM +0100, Emanuele Ghidoli wrote:
I infer, from datasheet, that volume update is applied in different way based on charge pump dynamic vs register control (CP_DYN_PWR bit in CLASS_W register): "Under Register control, the HPOUTL_VOL, HPOUTR_VOL, LINEOUTL_VOL and LINEOUTR_VOL register settings are used to control the charge pump mode of operation. Under Dynamic control, the audio signal level in the DAC is used to control the charge pump mode of operation."
The second sentence do not explain that volume register is still considered by the component but likely in a different way.
It is important to note that I trace I2C transactions and, without the patch, the CLASS_W register is written JUST after volume update registers (with the patch is written before and after).
At this point I have no doubt that we have to update that register before writing volume.
Hmm... I think my only concern here is this feels a bit counter intuitive, the default value is described as "controlled by volume register settings" and we are saying in that situation the volume registers don't seem to update properly. That is far from impossible but I think we should perhaps poke a little more to make sure we understand the bounds here.
I did some more test and I'm not 100% sure of what is going on anymore.
I see that that CP_DYN_PWR bit is disabled when audio is going through one of the bypass paths. Would you be able to enable one of the bypass paths and see if you can manually adjust the volume on the headphone output, with a bypass path active?
With the previous change, I tested all the possible combination with one channel from the DAC and the other toggling from DAC to Bypass changing the volume and it's always correct.
Would also perhaps be interesting as a test to completely remove the write to CP_DYN_PWR from probe and leave things set to manual and see how the volume behaves then?
When I tried to remove any write to this register my modification stopped working.
I guess the interests here are to find out if the SYSCLK is involved at all.
I tested keep the clock always enabled, removing clk_disable_unprepare when going into SND_SOC_BIAS_OFF and it has zero effects. Or did you mean something else?
Said all of that, I did one last test, forcing a volume update on the charge pump enable callback, cp_event(), with this and only this modification in everything is working fine.
Could it just be as easy as that the volume is applied only when the charge pump is already active?
From the datasheet this seems a good explanation:
The Charge Pump is enabled by setting the CP_ENA bit. When enabled, the charge pump adjusts the output voltages (CPVOUTP and CPVOUTN).
What do you think?
Emanuele
On Tue, Dec 20, 2022 at 08:12:23PM +0100, Emanuele Ghidoli wrote:
On 20/12/2022 11:00, Charles Keepax wrote:
On Mon, Dec 19, 2022 at 04:20:10PM +0100, Emanuele Ghidoli wrote:
On 19/12/2022 10:58, Charles Keepax wrote:
On Sat, Dec 17, 2022 at 12:47:14AM +0100, Emanuele Ghidoli wrote:
I see that that CP_DYN_PWR bit is disabled when audio is going through one of the bypass paths. Would you be able to enable one of the bypass paths and see if you can manually adjust the volume on the headphone output, with a bypass path active?
With the previous change, I tested all the possible combination with one channel from the DAC and the other toggling from DAC to Bypass changing the volume and it's always correct.
Would also perhaps be interesting as a test to completely remove the write to CP_DYN_PWR from probe and leave things set to manual and see how the volume behaves then?
When I tried to remove any write to this register my modification stopped working.
Apologies just to be totally clear here, you are saying that whilst a bypass path is active (ie. the class G widget has cleared CP_DYN_PWR), you can still control the volume? But if you remove the set of CP_DYN_PWR from probe, the volume doesn't update at all, audio playing or not?
I guess the interests here are to find out if the SYSCLK is involved at all.
I tested keep the clock always enabled, removing clk_disable_unprepare when going into SND_SOC_BIAS_OFF and it has zero effects. Or did you mean something else?
Yeah that is not quite what I was getting at. I am wondering if volume updates work whilst CP_DYN_PWR==0 and CLK_SYS_ENA==1.
Said all of that, I did one last test, forcing a volume update on the charge pump enable callback, cp_event(), with this and only this modification in everything is working fine.
Could it just be as easy as that the volume is applied only when the charge pump is already active?
From the datasheet this seems a good explanation:
The Charge Pump is enabled by setting the CP_ENA bit. When enabled, the charge pump adjusts the output voltages (CPVOUTP and CPVOUTN).
What do you think?
I think we are getting pretty close, but we need to try and narrow down what the requirement is here, is it the charge pump, or the sysclk? That needs to be on for the volume update to work.
Thanks, Charles
On 21/12/2022 17:56, Charles Keepax wrote:
On Tue, Dec 20, 2022 at 08:12:23PM +0100, Emanuele Ghidoli wrote:
On 20/12/2022 11:00, Charles Keepax wrote:
On Mon, Dec 19, 2022 at 04:20:10PM +0100, Emanuele Ghidoli wrote:
On 19/12/2022 10:58, Charles Keepax wrote:
On Sat, Dec 17, 2022 at 12:47:14AM +0100, Emanuele Ghidoli wrote:
I see that that CP_DYN_PWR bit is disabled when audio is going through one of the bypass paths. Would you be able to enable one of the bypass paths and see if you can manually adjust the volume on the headphone output, with a bypass path active?
With the previous change, I tested all the possible combination with one channel from the DAC and the other toggling from DAC to Bypass changing the volume and it's always correct.
Would also perhaps be interesting as a test to completely remove the write to CP_DYN_PWR from probe and leave things set to manual and see how the volume behaves then?
When I tried to remove any write to this register my modification stopped working.
Apologies just to be totally clear here, you are saying that whilst a bypass path is active (ie. the class G widget has cleared CP_DYN_PWR), you can still control the volume? But if you remove the set of CP_DYN_PWR from probe, the volume doesn't update at all, audio playing or not?
Yes, exactly. But I have also commented: SND_SOC_DAPM_SUPPLY("Class G", WM8904_CLASS_W_0, 0, 1, NULL, 0)
In every case the volume updates, while playing, when you write the relevant register (raw i2cset or changing volume using amixer).
To be clear: The volume is not updated, after BIAS off, if we are in CLASS G WITH bypass DISABLED (that, without these modification, it is a condition we cannot trigger. Normally: Bypass ON->class G, bypass OFF->class W).
Effectively, maybe, the test with bypass enabled is affected by the fact codec is not switched off (bias is kept on... otherwise as soon I stop playing something from my linux device bypass will stop working due to codec reset/power off). In other words the Dynamic Audio Power Managent (DAPM), which I "understand" only now, is doing its work.
I guess the interests here are to find out if the SYSCLK is involved at all.
I tested keep the clock always enabled, removing clk_disable_unprepare when going into SND_SOC_BIAS_OFF and it has zero effects. Or did you mean something else?
Yeah that is not quite what I was getting at. I am wondering if volume updates work whilst CP_DYN_PWR==0 and CLK_SYS_ENA==1.
Why are you wondering? It should be a standard working case (obviously with MCLK running). I know, from datasheet, that: "CLK_SYS_ENA = 1 and MCLK is not present, then register access will be unsuccessful". But it is not our case.
Said all of that, I did one last test, forcing a volume update on the charge pump enable callback, cp_event(), with this and only this modification in everything is working fine.
Could it just be as easy as that the volume is applied only when the charge pump is already active?
From the datasheet this seems a good explanation:
The Charge Pump is enabled by setting the CP_ENA bit. When enabled, the charge pump adjusts the output voltages (CPVOUTP and CPVOUTN).
What do you think?
I think we are getting pretty close, but we need to try and narrow down what the requirement is here, is it the charge pump, or the sysclk? That needs to be on for the volume update to work.
Watching another codec driver (wm8964: see out_pga_event comment) and the Startup-sequence (of WM8904) in datasheet we figure out that volume update must be done after PGA enable. I tested another patch, I'm pretty convinced that it is the right way to do it. Now it is working in all conditions (even Class G with disabled bypass). Maybe some hw guy in Cirrus Logic can dig around? Anyway, this is the tested patch, that, to me, sound good:
diff --git a/sound/soc/codecs/wm8904.c b/sound/soc/codecs/wm8904.c index ca6a01a230af..791d8738d1c0 100644 --- a/sound/soc/codecs/wm8904.c +++ b/sound/soc/codecs/wm8904.c @@ -697,6 +697,7 @@ static int out_pga_event(struct snd_soc_dapm_widget *w, int dcs_mask; int dcs_l, dcs_r; int dcs_l_reg, dcs_r_reg; + int an_out_reg; int timeout; int pwr_reg;
@@ -712,6 +713,7 @@ static int out_pga_event(struct snd_soc_dapm_widget *w, dcs_mask = WM8904_DCS_ENA_CHAN_0 | WM8904_DCS_ENA_CHAN_1; dcs_r_reg = WM8904_DC_SERVO_8; dcs_l_reg = WM8904_DC_SERVO_9; + an_out_reg = WM8904_ANALOGUE_OUT1_LEFT; dcs_l = 0; dcs_r = 1; break; @@ -720,6 +722,7 @@ static int out_pga_event(struct snd_soc_dapm_widget *w, dcs_mask = WM8904_DCS_ENA_CHAN_2 | WM8904_DCS_ENA_CHAN_3; dcs_r_reg = WM8904_DC_SERVO_6; dcs_l_reg = WM8904_DC_SERVO_7; + an_out_reg = WM8904_ANALOGUE_OUT2_LEFT; dcs_l = 2; dcs_r = 3; break; @@ -792,6 +795,10 @@ static int out_pga_event(struct snd_soc_dapm_widget *w, snd_soc_component_update_bits(component, reg, WM8904_HPL_ENA_OUTP | WM8904_HPR_ENA_OUTP, WM8904_HPL_ENA_OUTP | WM8904_HPR_ENA_OUTP); + + /* Update volume, requires PGA to be powered */ + val = snd_soc_component_read(component, an_out_reg); + snd_soc_component_write(component, an_out_reg, val); break;
case SND_SOC_DAPM_POST_PMU:
Thanks, Charles
Thank you. Best regards,
Emanuele
On Wed, Dec 21, 2022 at 06:38:16PM +0100, Emanuele Ghidoli wrote:
On 21/12/2022 17:56, Charles Keepax wrote:
On Tue, Dec 20, 2022 at 08:12:23PM +0100, Emanuele Ghidoli wrote:
On 20/12/2022 11:00, Charles Keepax wrote:
On Mon, Dec 19, 2022 at 04:20:10PM +0100, Emanuele Ghidoli wrote:
On 19/12/2022 10:58, Charles Keepax wrote:
On Sat, Dec 17, 2022 at 12:47:14AM +0100, Emanuele Ghidoli wrote:
In every case the volume updates, while playing, when you write the relevant register (raw i2cset or changing volume using amixer).
Yeah that is not quite what I was getting at. I am wondering if volume updates work whilst CP_DYN_PWR==0 and CLK_SYS_ENA==1.
Why are you wondering? It should be a standard working case (obviously with MCLK running). I know, from datasheet, that: "CLK_SYS_ENA = 1 and MCLK is not present, then register access will be unsuccessful". But it is not our case.
Apologies yes I was intending for MCLK to be on as well, but I think we have covered this test condition with your "In every case the volume updates, while playing" comment above.
Watching another codec driver (wm8964: see out_pga_event comment) and the Startup-sequence (of WM8904) in datasheet we figure out that volume update must be done after PGA enable. I tested another patch, I'm pretty convinced that it is the right way to do it. Now it is working in all conditions (even Class G with disabled bypass). Maybe some hw guy in Cirrus Logic can dig around? Anyway, this is the tested patch, that, to me, sound good:
Yeah this patch looks better, as you say tracks with the datasheet and other CODECs of the same era have a similar requirement. I think if send this one as a separate patch we can go with that and feel free to add my ack:
Acked-by: Charles Keepax ckeepax@opensource.cirrus.com
Thanks, Charles
participants (2)
-
Charles Keepax -
Emanuele Ghidoli