| Message ID | 20230902210621.1184693-5-cristian.ciocaltea@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve CS35l41 ALSA SoC audio driver | expand |
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
diff --git a/include/sound/cs35l41.h b/include/sound/cs35l41.h index 1bf757901d02..91024411f8a1 100644 --- a/include/sound/cs35l41.h +++ b/include/sound/cs35l41.h @@ -11,7 +11,7 @@ #define __CS35L41_H #include <linux/regmap.h> -#include <linux/completion.h> +#include <linux/workqueue.h> #include <linux/firmware/cirrus/cs_dsp.h> #define CS35L41_FIRSTREG 0x00000000 @@ -902,7 +902,8 @@ int cs35l41_exit_hibernate(struct device *dev, struct regmap *regmap); int cs35l41_init_boost(struct device *dev, struct regmap *regmap, struct cs35l41_hw_cfg *hw_cfg); bool cs35l41_safe_reset(struct regmap *regmap, enum cs35l41_boost_type b_type); +int cs35l41_mdsync_up(struct regmap *regmap); int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l41_boost_type b_type, - int enable, struct completion *pll_lock, bool firmware_running); + int enable, struct work_struct *mdsync_up_work, bool firmware_running); #endif /* __CS35L41_H */ diff --git a/sound/soc/codecs/cs35l41-lib.c b/sound/soc/codecs/cs35l41-lib.c index a6c6bb23b957..16e7f46d3853 100644 --- a/sound/soc/codecs/cs35l41-lib.c +++ b/sound/soc/codecs/cs35l41-lib.c @@ -1192,8 +1192,56 @@ bool cs35l41_safe_reset(struct regmap *regmap, enum cs35l41_boost_type b_type) } EXPORT_SYMBOL_GPL(cs35l41_safe_reset); +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; + + ret = regmap_read_poll_timeout(regmap, CS35L41_IRQ1_STATUS1, int_status, + int_status & CS35L41_PUP_DONE_MASK, + 1000, 100000); + + /* Clear PUP/PDN status */ + regmap_write(regmap, CS35L41_IRQ1_STATUS1, CS35L41_PUP_DONE_MASK); + return ret; +} +EXPORT_SYMBOL_GPL(cs35l41_mdsync_up); + +/* + * NOTE: Enabling the CS35L41_SHD_BOOST_ACTV and CS35L41_SHD_BOOST_PASS shared + * boosts involves writing the MDSYNC UP reg sequence, which must be performed + * only after getting the PLL lock signal. This signal seems to be triggered + * soon after snd_pcm_start() is executed and SNDRV_PCM_TRIGGER_START command + * is processed, which happens (long) after the SND_SOC_DAPM_PRE_PMU event + * handler is invoked as part of snd_pcm_prepare(). + * + * The event handler is where cs35l41_global_enable() should be normally called + * from, but waiting for the PLL lock signal here will time out. Increasing the + * wait duration will not help, as the only consequence of that would be to add + * an unnecessary delay in the invocation of snd_pcm_start(). + * + * Trying to move the wait in the SNDRV_PCM_TRIGGER_START callback is not a + * solution either, as the trigger is executed in an IRQ-off atomic context, + * hence the current approach is to defer the processing to a workqueue task. + * This allows to properly wait for the signal and then safely write the + * necessary MDSYNC sequence by calling cs35l41_mdsync_up() above. + */ int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l41_boost_type b_type, - int enable, struct completion *pll_lock, bool firmware_running) + int enable, struct work_struct *mdsync_up_work, bool firmware_running) { int ret; unsigned int gpio1_func, pad_control, pwr_ctrl1, pwr_ctrl3, int_status, pup_pdn_mask; @@ -1203,11 +1251,6 @@ int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l4 {CS35L41_GPIO_PAD_CONTROL, 0}, {CS35L41_PWR_CTRL1, 0, 3000}, }; - struct reg_sequence cs35l41_mdsync_up_seq[] = { - {CS35L41_PWR_CTRL3, 0}, - {CS35L41_PWR_CTRL1, 0x00000000, 3000}, - {CS35L41_PWR_CTRL1, 0x00000001, 3000}, - }; pup_pdn_mask = enable ? CS35L41_PUP_DONE_MASK : CS35L41_PDN_DONE_MASK; @@ -1226,6 +1269,9 @@ int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l4 switch (b_type) { case CS35L41_SHD_BOOST_ACTV: case CS35L41_SHD_BOOST_PASS: + if (mdsync_up_work) + cancel_work_sync(mdsync_up_work); + regmap_read(regmap, CS35L41_PWR_CTRL3, &pwr_ctrl3); regmap_read(regmap, CS35L41_GPIO_PAD_CONTROL, &pad_control); @@ -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; } - 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) - return ret; - ret = regmap_read_poll_timeout(regmap, CS35L41_IRQ1_STATUS1, int_status, int_status & pup_pdn_mask, 1000, 100000); if (ret) dev_err(dev, "Enable(%d) failed: %d\n", enable, ret); - // Clear PUP/PDN status + /* Clear PUP/PDN status */ regmap_write(regmap, CS35L41_IRQ1_STATUS1, pup_pdn_mask); break; case CS35L41_INT_BOOST: diff --git a/sound/soc/codecs/cs35l41.c b/sound/soc/codecs/cs35l41.c index fe5376b3e01b..9bf70da03972 100644 --- a/sound/soc/codecs/cs35l41.c +++ b/sound/soc/codecs/cs35l41.c @@ -500,11 +500,11 @@ static int cs35l41_main_amp_event(struct snd_soc_dapm_widget *w, ARRAY_SIZE(cs35l41_pup_patch)); ret = cs35l41_global_enable(cs35l41->dev, cs35l41->regmap, cs35l41->hw_cfg.bst_type, - 1, &cs35l41->pll_lock, cs35l41->dsp.cs_dsp.running); + 1, &cs35l41->mdsync_up_work, cs35l41->dsp.cs_dsp.running); break; case SND_SOC_DAPM_POST_PMD: ret = cs35l41_global_enable(cs35l41->dev, cs35l41->regmap, cs35l41->hw_cfg.bst_type, - 0, &cs35l41->pll_lock, cs35l41->dsp.cs_dsp.running); + 0, &cs35l41->mdsync_up_work, cs35l41->dsp.cs_dsp.running); regmap_multi_reg_write_bypassed(cs35l41->regmap, cs35l41_pdn_patch, @@ -1155,6 +1155,36 @@ static int cs35l41_acpi_get_name(struct cs35l41_private *cs35l41) return 0; } +static void cs35l41_mdsync_up_work(struct work_struct *work) +{ + struct cs35l41_private *cs35l41 = container_of(work, + struct cs35l41_private, + mdsync_up_work); + int ret = wait_for_completion_timeout(&cs35l41->pll_lock, + msecs_to_jiffies(100)); + if (ret == 0) { + dev_err(cs35l41->dev, "Timed out waiting for pll_lock signal\n"); + return; + } + + dev_dbg(cs35l41->dev, "Received pll_lock signal\n"); + + ret = pm_runtime_resume_and_get(cs35l41->dev); + if (ret < 0) { + dev_err(cs35l41->dev, + "pm_runtime_resume_and_get failed in %s: %d\n", + __func__, ret); + return; + } + + ret = cs35l41_mdsync_up(cs35l41->regmap); + if (ret < 0) + dev_err(cs35l41->dev, "MDSYNC UP failed: %d\n", ret); + + pm_runtime_mark_last_busy(cs35l41->dev); + pm_runtime_put_autosuspend(cs35l41->dev); +} + int cs35l41_probe(struct cs35l41_private *cs35l41, const struct cs35l41_hw_cfg *hw_cfg) { u32 regid, reg_revid, i, mtl_revid, int_status, chipid_match; @@ -1297,6 +1327,8 @@ int cs35l41_probe(struct cs35l41_private *cs35l41, const struct cs35l41_hw_cfg * if (ret < 0) goto err; + INIT_WORK(&cs35l41->mdsync_up_work, cs35l41_mdsync_up_work); + pm_runtime_set_autosuspend_delay(cs35l41->dev, 3000); pm_runtime_use_autosuspend(cs35l41->dev); pm_runtime_mark_last_busy(cs35l41->dev); @@ -1335,6 +1367,8 @@ EXPORT_SYMBOL_GPL(cs35l41_probe); void cs35l41_remove(struct cs35l41_private *cs35l41) { + cancel_work_sync(&cs35l41->mdsync_up_work); + pm_runtime_get_sync(cs35l41->dev); pm_runtime_disable(cs35l41->dev); diff --git a/sound/soc/codecs/cs35l41.h b/sound/soc/codecs/cs35l41.h index 34d967d4372b..f9f85796a13a 100644 --- a/sound/soc/codecs/cs35l41.h +++ b/sound/soc/codecs/cs35l41.h @@ -13,6 +13,7 @@ #include <linux/gpio/consumer.h> #include <linux/regulator/consumer.h> #include <linux/firmware.h> +#include <linux/completion.h> #include <sound/core.h> #include <sound/cs35l41.h> @@ -34,6 +35,7 @@ struct cs35l41_private { /* GPIO for /RST */ struct gpio_desc *reset_gpio; struct completion pll_lock; + struct work_struct mdsync_up_work; }; int cs35l41_probe(struct cs35l41_private *cs35l41, const struct cs35l41_hw_cfg *hw_cfg);
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(-)