[PATCH 0/2] ASoC: renesas: Fix clip sounds

[PATCH 0/2] ASoC: renesas: Fix clip sounds

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Hi,

Series fixes clip sounds that are reproduced on suspend/resume, from time
to time, on the Renesas RZ/G3S SMARC Module + Renesas RZ Carrier II board.

Thank you,
Claudiu

Claudiu Beznea (2):
  ASoC: codecs: Use component driver suspend/resume
  ASoC: renesas: rz-ssi: Use proper dma_buffer_pos after resume

 sound/soc/codecs/da7213.c  | 38 +++++++++++++++++++++++++-------------
 sound/soc/renesas/rz-ssi.c | 25 ++++++++++++-------------
 2 files changed, 37 insertions(+), 26 deletions(-)

-- 
2.43.0

[PATCH 1/2] ASoC: codecs: Use component driver suspend/resume

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Since snd_soc_suspend() is invoked through snd_soc_pm_ops->suspend(),
and snd_soc_pm_ops is associated with the soc_driver (defined in
sound/soc/soc-core.c), and there is no parent-child relationship between
the soc_driver and the DA7213 codec driver, the power management subsystem
does not enforce a specific suspend/resume order between the DA7213 driver
and the soc_driver.

Because of this, the different codec component functionalities, called from
snd_soc_resume() to reconfigure various functions, can race with the
DA7213 resume function, leading to misapplied configuration.
This occasionally results in clipped sound.

Fix this by moving the regmap cache operations into
struct snd_soc_component_driver::{suspend, resume}. This ensures the
proper configuration sequence is handled by the ASoC subsystem.

Cc: stable@vger.kernel.org
Fixes: 431e040065c8 ("ASoC: da7213: Add suspend to RAM support")
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 sound/soc/codecs/da7213.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
index c1657f348ad9..051806982bbe 100644
--- a/sound/soc/codecs/da7213.c
+++ b/sound/soc/codecs/da7213.c
@@ -2124,11 +2124,31 @@ static int da7213_probe(struct snd_soc_component *component)
 	return 0;
 }
 
+static int da7213_suspend(struct snd_soc_component *component)
+{
+	struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component);
+
+	regcache_cache_only(da7213->regmap, true);
+	regcache_mark_dirty(da7213->regmap);
+
+	return 0;
+}
+
+static int da7213_resume(struct snd_soc_component *component)
+{
+	struct da7213_priv *da7213 = snd_soc_component_get_drvdata(component);
+
+	regcache_cache_only(da7213->regmap, false);
+	return regcache_sync(da7213->regmap);
+}
+
 static const struct snd_soc_component_driver soc_component_dev_da7213 = {
 	.probe			= da7213_probe,
 	.set_bias_level		= da7213_set_bias_level,
 	.controls		= da7213_snd_controls,
 	.num_controls		= ARRAY_SIZE(da7213_snd_controls),
+	.suspend		= da7213_suspend,
+	.resume			= da7213_resume,
 	.dapm_widgets		= da7213_dapm_widgets,
 	.num_dapm_widgets	= ARRAY_SIZE(da7213_dapm_widgets),
 	.dapm_routes		= da7213_audio_map,
@@ -2228,27 +2248,19 @@ static int da7213_runtime_suspend(struct device *dev)
 {
 	struct da7213_priv *da7213 = dev_get_drvdata(dev);
 
-	regcache_cache_only(da7213->regmap, true);
-	regcache_mark_dirty(da7213->regmap);
-	regulator_bulk_disable(DA7213_NUM_SUPPLIES, da7213->supplies);
-
-	return 0;
+	return regulator_bulk_disable(DA7213_NUM_SUPPLIES, da7213->supplies);
 }
 
 static int da7213_runtime_resume(struct device *dev)
 {
 	struct da7213_priv *da7213 = dev_get_drvdata(dev);
-	int ret;
 
-	ret = regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213->supplies);
-	if (ret < 0)
-		return ret;
-	regcache_cache_only(da7213->regmap, false);
-	return regcache_sync(da7213->regmap);
+	return regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213->supplies);
 }
 
-static DEFINE_RUNTIME_DEV_PM_OPS(da7213_pm, da7213_runtime_suspend,
-				 da7213_runtime_resume, NULL);
+static const struct dev_pm_ops da7213_pm = {
+	RUNTIME_PM_OPS(da7213_runtime_suspend, da7213_runtime_resume, NULL)
+};
 
 static const struct i2c_device_id da7213_i2c_id[] = {
 	{ "da7213" },
-- 
2.43.0

[PATCH 2/2] ASoC: renesas: rz-ssi: Use proper dma_buffer_pos after resume

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

When the driver supports DMA, it enqueues four DMA descriptors per
substream before the substream is started. New descriptors are enqueued in
the DMA completion callback, and each time a new descriptor is queued, the
dma_buffer_pos is incremented.

During suspend, the DMA transactions are terminated. There might be cases
where the four extra enqueued DMA descriptors are not completed and are
instead canceled on suspend. However, the cancel operation does not take
into account that the dma_buffer_pos was already incremented.

Previously, the suspend code reinitialized dma_buffer_pos to zero, but this
is not always correct.

To avoid losing any audio periods during suspend/resume and to prevent
clip sound, save the completed DMA buffer position in the DMA callback and
reinitialize dma_buffer_pos on resume.

Cc: stable@vger.kernel.org
Fixes: 1fc778f7c833a ("ASoC: renesas: rz-ssi: Add suspend to RAM support")
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 sound/soc/renesas/rz-ssi.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/sound/soc/renesas/rz-ssi.c b/sound/soc/renesas/rz-ssi.c
index e00940814157..81b883e8ac92 100644
--- a/sound/soc/renesas/rz-ssi.c
+++ b/sound/soc/renesas/rz-ssi.c
@@ -85,6 +85,7 @@ struct rz_ssi_stream {
 	struct snd_pcm_substream *substream;
 	int fifo_sample_size;	/* sample capacity of SSI FIFO */
 	int dma_buffer_pos;	/* The address for the next DMA descriptor */
+	int completed_dma_buf_pos; /* The address of the last completed DMA descriptor. */
 	int period_counter;	/* for keeping track of periods transferred */
 	int sample_width;
 	int buffer_pos;		/* current frame position in the buffer */
@@ -215,6 +216,7 @@ static void rz_ssi_stream_init(struct rz_ssi_stream *strm,
 	rz_ssi_set_substream(strm, substream);
 	strm->sample_width = samples_to_bytes(runtime, 1);
 	strm->dma_buffer_pos = 0;
+	strm->completed_dma_buf_pos = 0;
 	strm->period_counter = 0;
 	strm->buffer_pos = 0;
 
@@ -437,6 +439,10 @@ static void rz_ssi_pointer_update(struct rz_ssi_stream *strm, int frames)
 		snd_pcm_period_elapsed(strm->substream);
 		strm->period_counter = current_period;
 	}
+
+	strm->completed_dma_buf_pos += runtime->period_size;
+	if (strm->completed_dma_buf_pos >= runtime->buffer_size)
+		strm->completed_dma_buf_pos = 0;
 }
 
 static int rz_ssi_pio_recv(struct rz_ssi_priv *ssi, struct rz_ssi_stream *strm)
@@ -778,10 +784,14 @@ static int rz_ssi_dma_request(struct rz_ssi_priv *ssi, struct device *dev)
 	return -ENODEV;
 }
 
-static int rz_ssi_trigger_resume(struct rz_ssi_priv *ssi)
+static int rz_ssi_trigger_resume(struct rz_ssi_priv *ssi, struct rz_ssi_stream *strm)
 {
+	struct snd_pcm_substream *substream = strm->substream;
+	struct snd_pcm_runtime *runtime = substream->runtime;
 	int ret;
 
+	strm->dma_buffer_pos = strm->completed_dma_buf_pos + runtime->period_size;
+
 	if (rz_ssi_is_stream_running(&ssi->playback) ||
 	    rz_ssi_is_stream_running(&ssi->capture))
 		return 0;
@@ -794,16 +804,6 @@ static int rz_ssi_trigger_resume(struct rz_ssi_priv *ssi)
 				ssi->hw_params_cache.channels);
 }
 
-static void rz_ssi_streams_suspend(struct rz_ssi_priv *ssi)
-{
-	if (rz_ssi_is_stream_running(&ssi->playback) ||
-	    rz_ssi_is_stream_running(&ssi->capture))
-		return;
-
-	ssi->playback.dma_buffer_pos = 0;
-	ssi->capture.dma_buffer_pos = 0;
-}
-
 static int rz_ssi_dai_trigger(struct snd_pcm_substream *substream, int cmd,
 			      struct snd_soc_dai *dai)
 {
@@ -813,7 +813,7 @@ static int rz_ssi_dai_trigger(struct snd_pcm_substream *substream, int cmd,
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_RESUME:
-		ret = rz_ssi_trigger_resume(ssi);
+		ret = rz_ssi_trigger_resume(ssi, strm);
 		if (ret)
 			return ret;
 
@@ -852,7 +852,6 @@ static int rz_ssi_dai_trigger(struct snd_pcm_substream *substream, int cmd,
 
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 		rz_ssi_stop(ssi, strm);
-		rz_ssi_streams_suspend(ssi);
 		break;
 
 	case SNDRV_PCM_TRIGGER_STOP:
-- 
2.43.0

Re: [PATCH 1/2] ASoC: codecs: Use component driver suspend/resume

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1591 bytes --]

On Wed, Oct 29, 2025 at 04:11:33PM +0200, Claudiu wrote:

> Since snd_soc_suspend() is invoked through snd_soc_pm_ops->suspend(),
> and snd_soc_pm_ops is associated with the soc_driver (defined in
> sound/soc/soc-core.c), and there is no parent-child relationship between
> the soc_driver and the DA7213 codec driver, the power management subsystem
> does not enforce a specific suspend/resume order between the DA7213 driver
> and the soc_driver.

The theory here is that the power management core uses the device
instantiation order for both suspend and resume (reversed on suspend) so
the fact that we use probe deferral to make sure that the card
components are ready should ensure that the card suspends before
anything in the card.  If that is no longer the case then we need to
ensure that all drivers have system PM ops which trigger the card, this
won't be a driver specific issue.

>  static int da7213_runtime_resume(struct device *dev)
>  {
>  	struct da7213_priv *da7213 = dev_get_drvdata(dev);
> -	int ret;
>  
> -	ret = regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213->supplies);
> -	if (ret < 0)
> -		return ret;
> -	regcache_cache_only(da7213->regmap, false);
> -	return regcache_sync(da7213->regmap);
> +	return regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213->supplies);
>  }

This seems obviously buggy, we just power on the device and don't sync
the register state.  If the device actually lost power during a runtime
suspend then we'll end up having a bad time.  There was also no mention
of runtime PM in the patch description...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] ASoC: codecs: Use component driver suspend/resume

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 867 bytes --]

On Wed, Oct 29, 2025 at 04:11:33PM +0200, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Since snd_soc_suspend() is invoked through snd_soc_pm_ops->suspend(),
> and snd_soc_pm_ops is associated with the soc_driver (defined in
> sound/soc/soc-core.c), and there is no parent-child relationship between
> the soc_driver and the DA7213 codec driver, the power management subsystem
> does not enforce a specific suspend/resume order between the DA7213 driver
> and the soc_driver.

Oh, also:

Please submit patches using subject lines reflecting the style for the
subsystem, this makes it easier for people to identify relevant patches.
Look at what existing commits in the area you're changing are doing and
make sure your subject lines visually resemble what they're doing.
There's no need to resubmit to fix this alone.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] ASoC: codecs: Use component driver suspend/resume

[Richard Fitzgerald]

On 29/10/2025 2:11 pm, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Since snd_soc_suspend() is invoked through snd_soc_pm_ops->suspend(),
> and snd_soc_pm_ops is associated with the soc_driver (defined in
> sound/soc/soc-core.c), and there is no parent-child relationship between
> the soc_driver and the DA7213 codec driver, the power management subsystem
> does not enforce a specific suspend/resume order between the DA7213 driver
> and the soc_driver.
> 
> Because of this, the different codec component functionalities, called from
> snd_soc_resume() to reconfigure various functions, can race with the
> DA7213 resume function, leading to misapplied configuration.
> This occasionally results in clipped sound.
> 
> Fix this by moving the regmap cache operations into
> struct snd_soc_component_driver::{suspend, resume}. This ensures the
> proper configuration sequence is handled by the ASoC subsystem.
> 
> Cc: stable@vger.kernel.org
> Fixes: 431e040065c8 ("ASoC: da7213: Add suspend to RAM support")
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>   sound/soc/codecs/da7213.c | 38 +++++++++++++++++++++++++-------------
>   1 file changed, 25 insertions(+), 13 deletions(-)
The commit title starts "ASoC: codecs:", which implies that this
patch affects multiple codecs. But it only changes one file.

The commit title prefix should be "ASoC: da7213:", same as the
commit you reference in the Fixes: tag.

Re: [PATCH 1/2] ASoC: codecs: Use component driver suspend/resume

[claudiu beznea]

Hi, Mark,

On 10/29/25 16:37, Mark Brown wrote:
> On Wed, Oct 29, 2025 at 04:11:33PM +0200, Claudiu wrote:
> 
>> Since snd_soc_suspend() is invoked through snd_soc_pm_ops->suspend(),
>> and snd_soc_pm_ops is associated with the soc_driver (defined in
>> sound/soc/soc-core.c), and there is no parent-child relationship between
>> the soc_driver and the DA7213 codec driver, the power management subsystem
>> does not enforce a specific suspend/resume order between the DA7213 driver
>> and the soc_driver.
> 
> The theory here is that the power management core uses the device
> instantiation order for both suspend and resume (reversed on suspend) so
> the fact that we use probe deferral to make sure that the card
> components are ready should ensure that the card suspends before
> anything in the card.  If that is no longer the case then we need to
> ensure that all drivers have system PM ops which trigger the card, this
> won't be a driver specific issue.

I also saw the behavior described in this commit with the rz-ssi.c driver as 
well. The fix there was commit c1b0f5183a44 ("ASoC: renesas: rz-ssi: Use 
NOIRQ_SYSTEM_SLEEP_PM_OPS()").

In case of this this codec, I saw the code in da7213_runtime_resume() and 
soc_resume_deferred() racing each other on system resume.

> 
>>   static int da7213_runtime_resume(struct device *dev)
>>   {
>>   	struct da7213_priv *da7213 = dev_get_drvdata(dev);
>> -	int ret;
>>   
>> -	ret = regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213->supplies);
>> -	if (ret < 0)
>> -		return ret;
>> -	regcache_cache_only(da7213->regmap, false);
>> -	return regcache_sync(da7213->regmap);
>> +	return regulator_bulk_enable(DA7213_NUM_SUPPLIES, da7213->supplies);
>>   }
> 
> This seems obviously buggy, we just power on the device and don't sync
> the register state.  

You're right! I'll revisit this.

> If the device actually lost power during a runtime
> suspend then we'll end up having a bad time.  There was also no mention
> of runtime PM in the patch description...

I had no issues with runtime PM, but only with suspend to RAM, when this 
function was called though
struct dev_pm_ops::resume = pm_runtime_force_resume().

Would keeping the regcache_cache_only() + regcache_sync() here along with 
populating the struct snd_soc_component_driver::{suspend, resume} be an 
acceptable solution for you? I think that will work as well.

Thank you for your review,
Claudiu

Re: [PATCH 1/2] ASoC: codecs: Use component driver suspend/resume

[claudiu beznea]

On 10/29/25 16:37, Mark Brown wrote:
> On Wed, Oct 29, 2025 at 04:11:33PM +0200, Claudiu wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> Since snd_soc_suspend() is invoked through snd_soc_pm_ops->suspend(),
>> and snd_soc_pm_ops is associated with the soc_driver (defined in
>> sound/soc/soc-core.c), and there is no parent-child relationship between
>> the soc_driver and the DA7213 codec driver, the power management subsystem
>> does not enforce a specific suspend/resume order between the DA7213 driver
>> and the soc_driver.
> 
> Oh, also:
> 
> Please submit patches using subject lines reflecting the style for the
> subsystem, this makes it easier for people to identify relevant patches.
> Look at what existing commits in the area you're changing are doing and
> make sure your subject lines visually resemble what they're doing.
> There's no need to resubmit to fix this alone.

I messed this up. I'll be more careful next time.

Thank you,
Claudiu

Re: [PATCH 1/2] ASoC: codecs: Use component driver suspend/resume

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1832 bytes --]

On Wed, Oct 29, 2025 at 05:22:26PM +0200, claudiu beznea wrote:
> On 10/29/25 16:37, Mark Brown wrote:

> > The theory here is that the power management core uses the device
> > instantiation order for both suspend and resume (reversed on suspend) so
> > the fact that we use probe deferral to make sure that the card
> > components are ready should ensure that the card suspends before
> > anything in the card.  If that is no longer the case then we need to
> > ensure that all drivers have system PM ops which trigger the card, this
> > won't be a driver specific issue.

> I also saw the behavior described in this commit with the rz-ssi.c driver as
> well. The fix there was commit c1b0f5183a44 ("ASoC: renesas: rz-ssi: Use
> NOIRQ_SYSTEM_SLEEP_PM_OPS()").

> In case of this this codec, I saw the code in da7213_runtime_resume() and
> soc_resume_deferred() racing each other on system resume.

So I guess we need the more general fix then, with everything calling
into the core to suspend the cards.

> > If the device actually lost power during a runtime
> > suspend then we'll end up having a bad time.  There was also no mention
> > of runtime PM in the patch description...

> I had no issues with runtime PM, but only with suspend to RAM, when this
> function was called though
> struct dev_pm_ops::resume = pm_runtime_force_resume().

Calling regulator_disable() doesn't guarantee the regulator will be
disabled, the constraints or other devices can ensure that the device
retains power so there's no effect on the actual hardware.

> Would keeping the regcache_cache_only() + regcache_sync() here along with
> populating the struct snd_soc_component_driver::{suspend, resume} be an
> acceptable solution for you? I think that will work as well.

I'm not sure what you're intending to populate the component with there.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: (subset) [PATCH 0/2] ASoC: renesas: Fix clip sounds

[Mark Brown]

On Wed, 29 Oct 2025 16:11:32 +0200, Claudiu wrote:
> Series fixes clip sounds that are reproduced on suspend/resume, from time
> to time, on the Renesas RZ/G3S SMARC Module + Renesas RZ Carrier II board.
> 
> Thank you,
> Claudiu
> 
> Claudiu Beznea (2):
>   ASoC: codecs: Use component driver suspend/resume
>   ASoC: renesas: rz-ssi: Use proper dma_buffer_pos after resume
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[2/2] ASoC: renesas: rz-ssi: Use proper dma_buffer_pos after resume
      commit: 22897e568646de5907d4981eae6cc895be2978d1

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH 1/2] ASoC: codecs: Use component driver suspend/resume

[Claudiu Beznea]

Hi, Mark,

On 10/29/25 18:14, Mark Brown wrote:
>> Would keeping the regcache_cache_only() + regcache_sync() here along with
>> populating the struct snd_soc_component_driver::{suspend, resume} be an
>> acceptable solution for you? I think that will work as well.
> I'm not sure what you're intending to populate the component with there.

Sorry for the late reply, I took the chance and prepared a new version
showing what I intended to say here. v2 posted here:

https://lore.kernel.org/all/20251104114914.2060603-1-claudiu.beznea.uj@bp.renesas.com/

Thank you,
Claudiu