wm8904 and output volume control

wm8904 and output volume control

[Emanuele Ghidoli]

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

Re: wm8904 and output volume control

[Charles Keepax]

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

Re: wm8904 and output volume control

[Emanuele Ghidoli]

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

Re: wm8904 and output volume control

[Charles Keepax]

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

Re: wm8904 and output volume control

[Emanuele Ghidoli]

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

Re: wm8904 and output volume control

[Charles Keepax]

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

Re: wm8904 and output volume control

[Emanuele Ghidoli]

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

Re: wm8904 and output volume control

[Charles Keepax]

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