[4/9] ASoC: cs35l41: Fix broken shared boost activation

Enabling the active/passive shared boosts involves writing the MDSYNC UP
register sequence, which cannot be performed before receiving the PLL
lock signal.

Due to improper error handling, it was not obvious the wait operation
times out and, consequently, the shared boost gets never enabled.

Further investigations revealed the signal is triggered while
snd_pcm_start() is executed, right after receiving the
SNDRV_PCM_TRIGGER_START command, which happens long after the
SND_SOC_DAPM_PRE_PMU event handler is invoked as part of
snd_pcm_prepare().  That is where cs35l41_global_enable() is called
from.

Increasing the wait duration doesn't help, as it only causes an
unnecessary delay in the invocation of snd_pcm_start().  Moving the wait
and the subsequent regmap operations to the SNDRV_PCM_TRIGGER_START
callback is not a solution either, since they would be executed in an
IRQ-off atomic context.

Solve the issue by deferring the processing to a workqueue task, which
allows to correctly wait for the signal and then safely proceed with
the required regmap operations.

Fixes: f5030564938b ("ALSA: cs35l41: Add shared boost feature")
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 include/sound/cs35l41.h        |  5 +-
 sound/soc/codecs/cs35l41-lib.c | 86 +++++++++++++++++++++++++---------
 sound/soc/codecs/cs35l41.c     | 38 ++++++++++++++-
 sound/soc/codecs/cs35l41.h     |  2 +
 4 files changed, 104 insertions(+), 27 deletions(-)

On Sun, Sep 03, 2023 at 12:06:16AM +0300, Cristian Ciocaltea wrote:
> Enabling the active/passive shared boosts involves writing the MDSYNC UP
> register sequence, which cannot be performed before receiving the PLL
> lock signal.
> 
> Due to improper error handling, it was not obvious the wait operation
> times out and, consequently, the shared boost gets never enabled.
> 
> Further investigations revealed the signal is triggered while
> snd_pcm_start() is executed, right after receiving the
> SNDRV_PCM_TRIGGER_START command, which happens long after the
> SND_SOC_DAPM_PRE_PMU event handler is invoked as part of
> snd_pcm_prepare().  That is where cs35l41_global_enable() is called
> from.
> 
> Increasing the wait duration doesn't help, as it only causes an
> unnecessary delay in the invocation of snd_pcm_start().  Moving the wait
> and the subsequent regmap operations to the SNDRV_PCM_TRIGGER_START
> callback is not a solution either, since they would be executed in an
> IRQ-off atomic context.
> 
> Solve the issue by deferring the processing to a workqueue task, which
> allows to correctly wait for the signal and then safely proceed with
> the required regmap operations.
> 
> Fixes: f5030564938b ("ALSA: cs35l41: Add shared boost feature")
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---

Thanks for looking at this apologies this was missed in the
initial review of the patch.

> +int cs35l41_mdsync_up(struct regmap *regmap)
> +{
> +	struct reg_sequence cs35l41_mdsync_up_seq[] = {
> +		{CS35L41_PWR_CTRL3, 0},
> +		{CS35L41_PWR_CTRL1, 0x00000000, 3000},
> +		{CS35L41_PWR_CTRL1, 0x00000001, 3000},
> +	};
> +	unsigned int pwr_ctrl3, int_status;
> +	int ret;
> +
> +	regmap_read(regmap, CS35L41_PWR_CTRL3, &pwr_ctrl3);
> +	pwr_ctrl3 |= CS35L41_SYNC_EN_MASK;
> +	cs35l41_mdsync_up_seq[0].def = pwr_ctrl3;
> +
> +	ret = regmap_multi_reg_write(regmap, cs35l41_mdsync_up_seq,
> +				     ARRAY_SIZE(cs35l41_mdsync_up_seq));
> +	if (ret < 0)
> +		return ret;

Is this now safe? By pulling this out into a worker thread, it is
no longer under the DAPM lock, which makes me worry this can race
with the other uses of PWR_CTRL3 which could theoretically change
state between when you read the reg and when you write it.

> @@ -1243,33 +1289,27 @@ int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l4
>  		cs35l41_mdsync_down_seq[2].def = pwr_ctrl1;
>  		ret = regmap_multi_reg_write(regmap, cs35l41_mdsync_down_seq,
>  					     ARRAY_SIZE(cs35l41_mdsync_down_seq));
> -		if (ret || !enable)
> +		if (ret)
>  			break;
>  
> -		if (!pll_lock)
> -			return -EINVAL;
> -
> -		ret = wait_for_completion_timeout(pll_lock, msecs_to_jiffies(1000));
> -		if (ret == 0) {
> -			dev_err(dev, "Timed out waiting for pll_lock\n");
> -			return -ETIMEDOUT;
> +		if (enable) {
> +			if (mdsync_up_work) {
> +				/* Call cs35l41_mdsync_up() after receiving PLL lock signal */
> +				schedule_work(mdsync_up_work);
> +			} else {
> +				dev_err(dev, "MDSYNC UP work not provided\n");
> +				ret = -EINVAL;
> +			}
> +			break;

One question I might also have would be does a worker thread make
more sense or would it be simpler to do the mdsync power up
directly in response to the PLL lock IRQ?

Thanks,
Charles

On 9/5/23 5:29 AM, Charles Keepax wrote:
> On Sun, Sep 03, 2023 at 12:06:16AM +0300, Cristian Ciocaltea wrote:
>> Enabling the active/passive shared boosts involves writing the MDSYNC UP
>> register sequence, which cannot be performed before receiving the PLL
>> lock signal.
>>

> 
> Thanks for looking at this apologies this was missed in the
> initial review of the patch.
> 

Thanks Cristian, I agree with the intent of your patch.
We do not expect that clocks are always available before the DAPM PMU 
event and shared boost should still be configured if they are not.

>> +int cs35l41_mdsync_up(struct regmap *regmap)
>> +{
>> +	struct reg_sequence cs35l41_mdsync_up_seq[] = {
>> +		{CS35L41_PWR_CTRL3, 0},
>> +		{CS35L41_PWR_CTRL1, 0x00000000, 3000},
>> +		{CS35L41_PWR_CTRL1, 0x00000001, 3000},
>> +	};

I don't know why PWR_CTRL1 is included in the up sequence here.
This toggles GLOBAL_EN, which will cause the PLL to unlock and lock 
again. Doing this defeats the purpose of setting SYNC_EN in a separate 
operation, which is to only do so when the amp is powered on and has 
locked the PLL. GLOBAL_EN is set by the mdsync_down_seq, so all that is 
needed when the PLL is locked is to set SYNC_EN.

> 
> Is this now safe? By pulling this out into a worker thread, it is
> no longer under the DAPM lock, which makes me worry this can race
> with the other uses of PWR_CTRL3 which could theoretically change
> state between when you read the reg and when you write it.
> 

The Class-H DAPM widget also uses the PWR_CTRL3 register.

> 
> One question I might also have would be does a worker thread make
> more sense or would it be simpler to do the mdsync power up
> directly in response to the PLL lock IRQ?
> 

I  agree with implementing this in the PLL lock IRQ.
As I described above, all that would need to be done is to set SYNC_EN 
in the PLL Lock IRQ handler.

Thanks,
David

On 9/5/23 21:11, Rhodes, David wrote:
> On 9/5/23 5:29 AM, Charles Keepax wrote:
>> On Sun, Sep 03, 2023 at 12:06:16AM +0300, Cristian Ciocaltea wrote:
>>> Enabling the active/passive shared boosts involves writing the MDSYNC UP
>>> register sequence, which cannot be performed before receiving the PLL
>>> lock signal.
>>>
> 
>>
>> Thanks for looking at this apologies this was missed in the
>> initial review of the patch.
>>
> 
> Thanks Cristian, I agree with the intent of your patch.
> We do not expect that clocks are always available before the DAPM PMU
> event and shared boost should still be configured if they are not.
> 
>>> +int cs35l41_mdsync_up(struct regmap *regmap)
>>> +{
>>> +    struct reg_sequence cs35l41_mdsync_up_seq[] = {
>>> +        {CS35L41_PWR_CTRL3, 0},
>>> +        {CS35L41_PWR_CTRL1, 0x00000000, 3000},
>>> +        {CS35L41_PWR_CTRL1, 0x00000001, 3000},
>>> +    };
> 
> I don't know why PWR_CTRL1 is included in the up sequence here.
> This toggles GLOBAL_EN, which will cause the PLL to unlock and lock
> again. Doing this defeats the purpose of setting SYNC_EN in a separate
> operation, which is to only do so when the amp is powered on and has
> locked the PLL. GLOBAL_EN is set by the mdsync_down_seq, so all that is
> needed when the PLL is locked is to set SYNC_EN.

Unfortunately I had to rely on the existing implementation as I don't 
have access to the datasheet.

If I got it right, we should drop all write operations on PWR_CTRL1, 
and simply set the CS35L41_SYNC_EN_MASK bit in PWR_CTRL3.
 
>>
>> Is this now safe? By pulling this out into a worker thread, it is
>> no longer under the DAPM lock, which makes me worry this can race
>> with the other uses of PWR_CTRL3 which could theoretically change
>> state between when you read the reg and when you write it.

That's a good point, it should be fixed implicitly by replacing the 
read/write operations with a single regmap_update_bits() call, which 
is protected by regmap's internal lock.

> 
> The Class-H DAPM widget also uses the PWR_CTRL3 register.
> 
>>
>> One question I might also have would be does a worker thread make
>> more sense or would it be simpler to do the mdsync power up
>> directly in response to the PLL lock IRQ?
>>
> 
> I  agree with implementing this in the PLL lock IRQ.
> As I described above, all that would need to be done is to set SYNC_EN
> in the PLL Lock IRQ handler.

As a matter of fact I initially considered doing this in the IRQ handler, 
but I also wanted to understand the reasoning behind current solution. 
Therefore I searched the ML archive for any relevant review comments, 
and I came across [1] which raises some concerns regarding the PLL lock 
signal, e.g. lack of documentation regarding trigger time & frequency.

If going with the IRQ handler approach is still the recommended approach,
I will handle it in v2.

Thanks for reviewing,
Cristian

[1] https://lore.kernel.org/all/20230207114855.GC36097@ediswmail.ad.cirrus.com/

On 9/5/23 3:05 PM, Cristian Ciocaltea wrote:
> If I got it right, we should drop all write operations on PWR_CTRL1,
> and simply set the CS35L41_SYNC_EN_MASK bit in PWR_CTRL3.

> That's a good point, it should be fixed implicitly by replacing the
> read/write operations with a single regmap_update_bits() call, which
> is protected by regmap's internal lock.
> 

Yes, my recommendation is to replace the mdsync_up_sequence and 
completion handling with a single regmap_update_bits() call to set the 
CS35L41_SYNC_EN_MASK bit in PWR_CTRL3, which occurs in response to the 
PLL Lock interrupt.

Thanks,
David