[PATCH] ASoC: tas2783-sdw: drop stale regcache on uninitialized re-attach

[PATCH] ASoC: tas2783-sdw: drop stale regcache on uninitialized re-attach

[Andrey Golovko]

When the peripheral re-attaches after the SoundWire controller was
power-gated during system suspend (s2idle reaching S0i3 on AMD ACP), the
amplifier has lost all of its register and DSP state. tas_update_status()
handles that by re-running tas_io_init(), which soft-resets the device
and re-downloads the firmware, but before doing so it syncs back a
register cache that still holds the pre-suspend values.

That sync is useless, since the soft reset immediately wipes whatever it
wrote, and it leaves the cache claiming that the amplifier is already
powered up and unmuted. Subsequent read-modify-write updates - DAPM
amplifier power-up, SDCA PDE transitions at stream start - then see "no
change" and skip the hardware write. Playback runs without a single
error while the speakers stay silent. Unbinding and rebinding the driver
restores audio, since probe starts from a fresh cache.

Drop the cache instead of syncing it when an uninitialized device
attaches, so that later accesses see the real hardware state.
regcache_mark_dirty() + regcache_sync() is not an option here: the cache
can also hold registers outside the SDCA MBQ map, written during the
init sequence, which the MBQ backend refuses to write back. The sync
then fails with -EINVAL and takes initialization down with it.

Cached user settings fall back to hardware defaults across such a power
loss, which seems clearly preferable to a silent amplifier - the device
is being reset and its firmware reloaded at this point anyway.

Tested on an ASUS ProArt PX13 HN7306EAC (AMD Strix Halo, ACP7.0, two
TAS2783 amplifiers plus RT721 on SoundWire link 1): the speakers work
after an s2idle resume with ~51 s of S0i3 residency, where previously
they stayed silent despite a complete firmware re-download.

Fixes: 4cc9bd8d7b32 ("ASoc: tas2783A: Add soundwire based codec driver")
Reported-by: Antoine Monnet <antoine@montane.tech>
Closes: https://lore.kernel.org/all/c66ae00a-e878-4af0-a05a-272e9574eaa5@montane.tech/
Signed-off-by: Andrey Golovko <andrey.golovko@gmail.com>
---
Based on broonie/sound for-next (asoc-next), i.e. on top of
0d6b2d6f93a6 ("ASoC: codecs: tas2783-sdw: Propagate regcache_sync()
errors"), which touches the same call site.

Tested on 7.2-rc4 plus the ACP MSI-on-resume fix 5893013efabb, which is
a prerequisite for the peripherals to re-attach at all on this board:
https://lore.kernel.org/all/466a905d-8203-46d2-bfe4-a3b3f9b5d68b@montane.tech/

 sound/soc/codecs/tas2783-sdw.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/sound/soc/codecs/tas2783-sdw.c b/sound/soc/codecs/tas2783-sdw.c
index db58c50e8a83..e62470671951 100644
--- a/sound/soc/codecs/tas2783-sdw.c
+++ b/sound/soc/codecs/tas2783-sdw.c
@@ -1216,7 +1216,6 @@ static s32 tas_update_status(struct sdw_slave *slave,
 {
 	struct tas2783_prv *tas_dev = dev_get_drvdata(&slave->dev);
 	struct device *dev = &slave->dev;
-	int ret;
 
 	dev_dbg(dev, "Peripheral status = %s",
 		status == SDW_SLAVE_UNATTACHED ? "unattached" :
@@ -1232,14 +1231,23 @@ static s32 tas_update_status(struct sdw_slave *slave,
 	if (tas_dev->hw_init || tas_dev->status != SDW_SLAVE_ATTACHED)
 		return 0;
 
-	/* updated the cache data to device */
 	regcache_cache_only(tas_dev->regmap, false);
-	ret = regcache_sync(tas_dev->regmap);
-	if (ret) {
-		regcache_cache_only(tas_dev->regmap, true);
-		regcache_mark_dirty(tas_dev->regmap);
-		return ret;
-	}
+
+	/*
+	 * The device is attaching uninitialized: either this is the first
+	 * attach, or it lost power (and with it all register and DSP state)
+	 * while the controller was power-gated during system suspend. The
+	 * cache still holds the pre-suspend values, and tas_io_init() below
+	 * soft-resets the device anyway, so syncing it back is both useless
+	 * and harmful: later read-modify-write updates would compare against
+	 * stale data and skip the hardware write.
+	 *
+	 * Drop the cache instead, so that subsequent accesses see the real
+	 * hardware state. regcache_mark_dirty() + regcache_sync() cannot be
+	 * used here: the cache may hold registers outside the SDCA MBQ map,
+	 * which the MBQ backend refuses to write back.
+	 */
+	regcache_drop_region(tas_dev->regmap, 0, UINT_MAX);
 
 	/* perform I/O transfers required for Slave initialization */
 	return tas_io_init(&slave->dev, slave);
-- 
2.53.0

Re: [PATCH] ASoC: tas2783-sdw: drop stale regcache on uninitialized re-attach

[Pierre-Louis Bossart]

On 7/27/26 10:35, Andrey Golovko wrote:
> When the peripheral re-attaches after the SoundWire controller was
> power-gated during system suspend (s2idle reaching S0i3 on AMD ACP), the
> amplifier has lost all of its register and DSP state. tas_update_status()
> handles that by re-running tas_io_init(), which soft-resets the device
> and re-downloads the firmware, but before doing so it syncs back a
> register cache that still holds the pre-suspend values.
> 
> That sync is useless, since the soft reset immediately wipes whatever it
> wrote, and it leaves the cache claiming that the amplifier is already
> powered up and unmuted. Subsequent read-modify-write updates - DAPM
> amplifier power-up, SDCA PDE transitions at stream start - then see "no
> change" and skip the hardware write. Playback runs without a single
> error while the speakers stay silent. Unbinding and rebinding the driver
> restores audio, since probe starts from a fresh cache.
> 
> Drop the cache instead of syncing it when an uninitialized device
> attaches, so that later accesses see the real hardware state.
> regcache_mark_dirty() + regcache_sync() is not an option here: the cache
> can also hold registers outside the SDCA MBQ map, written during the
> init sequence, which the MBQ backend refuses to write back. The sync
> then fails with -EINVAL and takes initialization down with it.
> 
> Cached user settings fall back to hardware defaults across such a power
> loss, which seems clearly preferable to a silent amplifier - the device
> is being reset and its firmware reloaded at this point anyway.
> 
> Tested on an ASUS ProArt PX13 HN7306EAC (AMD Strix Halo, ACP7.0, two
> TAS2783 amplifiers plus RT721 on SoundWire link 1): the speakers work
> after an s2idle resume with ~51 s of S0i3 residency, where previously
> they stayed silent despite a complete firmware re-download.
> 
> Fixes: 4cc9bd8d7b32 ("ASoc: tas2783A: Add soundwire based codec driver")
> Reported-by: Antoine Monnet <antoine@montane.tech>
> Closes: https://lore.kernel.org/all/c66ae00a-e878-4af0-a05a-272e9574eaa5@montane.tech/
> Signed-off-by: Andrey Golovko <andrey.golovko@gmail.com>
> ---
> Based on broonie/sound for-next (asoc-next), i.e. on top of
> 0d6b2d6f93a6 ("ASoC: codecs: tas2783-sdw: Propagate regcache_sync()
> errors"), which touches the same call site.
> 
> Tested on 7.2-rc4 plus the ACP MSI-on-resume fix 5893013efabb, which is
> a prerequisite for the peripherals to re-attach at all on this board:
> https://lore.kernel.org/all/466a905d-8203-46d2-bfe4-a3b3f9b5d68b@montane.tech/
> 
>  sound/soc/codecs/tas2783-sdw.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/sound/soc/codecs/tas2783-sdw.c b/sound/soc/codecs/tas2783-sdw.c
> index db58c50e8a83..e62470671951 100644
> --- a/sound/soc/codecs/tas2783-sdw.c
> +++ b/sound/soc/codecs/tas2783-sdw.c
> @@ -1216,7 +1216,6 @@ static s32 tas_update_status(struct sdw_slave *slave,
>  {
>  	struct tas2783_prv *tas_dev = dev_get_drvdata(&slave->dev);
>  	struct device *dev = &slave->dev;
> -	int ret;
>  
>  	dev_dbg(dev, "Peripheral status = %s",
>  		status == SDW_SLAVE_UNATTACHED ? "unattached" :
> @@ -1232,14 +1231,23 @@ static s32 tas_update_status(struct sdw_slave *slave,
>  	if (tas_dev->hw_init || tas_dev->status != SDW_SLAVE_ATTACHED)
>  		return 0;
>  
> -	/* updated the cache data to device */
>  	regcache_cache_only(tas_dev->regmap, false);
> -	ret = regcache_sync(tas_dev->regmap);
> -	if (ret) {
> -		regcache_cache_only(tas_dev->regmap, true);
> -		regcache_mark_dirty(tas_dev->regmap);
> -		return ret;
> -	}

Agree that this sequence didn't make sense, but you have a set of
comments below that could be clearer.

> +
> +	/*
> +	 * The device is attaching uninitialized: either this is the first
> +	 * attach, or it lost power (and with it all register and DSP state)
> +	 * while the controller was power-gated during system suspend. The
> +	 * cache still holds the pre-suspend values, and tas_io_init() below
> +	 * soft-resets the device anyway, so syncing it back is both useless

you may want to clarify what 'soft-reset' means. This isn't a SoundWire
term, all forms of reset defined in the standard will require
re-enumeration. Some devices from Cirrus Logic perform a 'device reset'
and a second enumeration, if that was the case here then you could
end-up in a boot loop.

> +	 * and harmful: later read-modify-write updates would compare against
> +	 * stale data and skip the hardware write.
> +	 *
> +	 * Drop the cache instead, so that subsequent accesses see the real
> +	 * hardware state. regcache_mark_dirty() + regcache_sync() cannot be
> +	 * used here: the cache may hold registers outside the SDCA MBQ map,
> +	 * which the MBQ backend refuses to write back.

Not following this comment, there's a single cache with specific
registers tagged as requiring the MBQ-specific sequence with multiple
ordered read/writes. it doesn't matter whether the registers are in the
MBQ area and I don't know what the 'MBQ backend' refers to.

> +	 */
> +	regcache_drop_region(tas_dev->regmap, 0, UINT_MAX);
>  
>  	/* perform I/O transfers required for Slave initialization */
>  	return tas_io_init(&slave->dev, slave);

[PATCH v2] ASoC: tas2783-sdw: drop stale regcache on uninitialized re-attach

[Andrey Golovko]

When the peripheral re-attaches after the SoundWire controller was
power-gated during system suspend (s2idle reaching S0i3 on AMD ACP), the
amplifier has lost all of its register and DSP state. tas_update_status()
handles that by re-running tas_io_init(), which writes the device's
TAS2783_SW_RESET register - a vendor register write that clears the
device's register file and DSP state, not a SoundWire reset, so no
re-enumeration is involved - and re-downloads the firmware. Before doing
any of that, it syncs back a register cache that still holds the
pre-suspend values.

That sync is useless, since the reset immediately wipes whatever it
wrote, and it leaves the cache claiming that the amplifier is already
powered up and unmuted. Subsequent read-modify-write updates - DAPM
amplifier power-up, SDCA PDE transitions at stream start - then see "no
change" and skip the hardware write. Playback runs without a single
error while the speakers stay silent. Unbinding and rebinding the driver
restores audio, since probe starts from a fresh cache.

Drop the cache instead of syncing it when an uninitialized device
attaches, so that later accesses see the real hardware state.

Reordering the sync after tas_io_init() and marking the cache dirty is
not a workable alternative here: tas_regmap has no .writeable_reg, so
the cache accepts every register up to .max_register, including ones for
which tas2783_sdca_mbq_size() returns 0. regmap_sdw_mbq_size() rejects
those with -EINVAL, so the replay fails on the first such register and
takes initialization down with it.

Cached user settings fall back to hardware defaults across such a power
loss, which seems clearly preferable to a silent amplifier - the device
is being reset and its firmware reloaded at this point anyway.

Tested on an ASUS ProArt PX13 HN7306EAC (AMD Strix Halo, ACP7.0, two
TAS2783 amplifiers plus RT721 on SoundWire link 1): the speakers work
after an s2idle resume with ~51 s of S0i3 residency, where previously
they stayed silent despite a complete firmware re-download.

Fixes: 4cc9bd8d7b32 ("ASoc: tas2783A: Add soundwire based codec driver")
Reported-by: Antoine Monnet <antoine@montane.tech>
Closes: https://lore.kernel.org/all/c66ae00a-e878-4af0-a05a-272e9574eaa5@montane.tech/
Signed-off-by: Andrey Golovko <andrey.golovko@gmail.com>
---
Changes since v1 (thanks Pierre-Louis for the review):
 - describe the reset precisely: tas_io_init() writes the vendor
   TAS2783_SW_RESET register, which is not a SoundWire reset and does
   not trigger re-enumeration. 'soft reset' is gone from both the
   commit message and the code comment.
 - drop the vague 'MBQ backend' wording. The reason a sync is not
   usable is concrete: tas_regmap has no .writeable_reg, so the cache
   takes registers for which tas2783_sdca_mbq_size() returns 0, and
   regmap_sdw_mbq_size() then returns -EINVAL for them.
 - no functional change; the diff is the same modulo the comment.

Based on broonie/sound for-next, on top of 0d6b2d6f93a6 ("ASoC: codecs:
tas2783-sdw: Propagate regcache_sync() errors").

 sound/soc/codecs/tas2783-sdw.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/sound/soc/codecs/tas2783-sdw.c b/sound/soc/codecs/tas2783-sdw.c
index db58c50e8a83..f96a53a08175 100644
--- a/sound/soc/codecs/tas2783-sdw.c
+++ b/sound/soc/codecs/tas2783-sdw.c
@@ -1216,7 +1216,6 @@ static s32 tas_update_status(struct sdw_slave *slave,
 {
 	struct tas2783_prv *tas_dev = dev_get_drvdata(&slave->dev);
 	struct device *dev = &slave->dev;
-	int ret;
 
 	dev_dbg(dev, "Peripheral status = %s",
 		status == SDW_SLAVE_UNATTACHED ? "unattached" :
@@ -1232,14 +1231,23 @@ static s32 tas_update_status(struct sdw_slave *slave,
 	if (tas_dev->hw_init || tas_dev->status != SDW_SLAVE_ATTACHED)
 		return 0;
 
-	/* updated the cache data to device */
 	regcache_cache_only(tas_dev->regmap, false);
-	ret = regcache_sync(tas_dev->regmap);
-	if (ret) {
-		regcache_cache_only(tas_dev->regmap, true);
-		regcache_mark_dirty(tas_dev->regmap);
-		return ret;
-	}
+
+	/*
+	 * The device is attaching uninitialized: either this is the first
+	 * attach, or it lost power (and with it all register and DSP state)
+	 * while the controller was power-gated during system suspend. The
+	 * cache still holds the pre-suspend values, and tas_io_init() below
+	 * resets the device via TAS2783_SW_RESET anyway, so syncing it back
+	 * is both useless and harmful: later read-modify-write updates would
+	 * compare against stale data and skip the hardware write.
+	 *
+	 * Drop the cache instead, so that subsequent accesses see the real
+	 * hardware state. Syncing after the reset is not an option either:
+	 * the cache accepts registers for which tas2783_sdca_mbq_size()
+	 * returns 0, and writing those back fails with -EINVAL.
+	 */
+	regcache_drop_region(tas_dev->regmap, 0, UINT_MAX);
 
 	/* perform I/O transfers required for Slave initialization */
 	return tas_io_init(&slave->dev, slave);
-- 
2.53.0

Re: [PATCH v2] ASoC: tas2783-sdw: drop stale regcache on uninitialized re-attach

[Mark Brown]

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

On Mon, Jul 27, 2026 at 12:33:09PM +0300, Andrey Golovko wrote:
> When the peripheral re-attaches after the SoundWire controller was
> power-gated during system suspend (s2idle reaching S0i3 on AMD ACP), the
> amplifier has lost all of its register and DSP state. tas_update_status()

Please don't send new patches in reply to old patches or serieses, this
makes it harder for both people and tools to understand what is going
on - it can bury things in mailboxes and make it difficult to keep track
of what current patches are, both for the new patches and the old ones.

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

Re: [PATCH v2] ASoC: tas2783-sdw: drop stale regcache on uninitialized re-attach

[Mark Brown]

On Mon, 27 Jul 2026 12:33:09 +0300, Andrey Golovko wrote:
> ASoC: tas2783-sdw: drop stale regcache on uninitialized re-attach

Applied to

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

Thanks!

[1/1] ASoC: tas2783-sdw: drop stale regcache on uninitialized re-attach
      https://git.kernel.org/broonie/sound/c/b627da430357

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] ASoC: tas2783-sdw: drop stale regcache on uninitialized re-attach

[Andrey Golovko]

On Mon, Aug 24, 2026 at 11:55:35AM +0000, Xu, Baojun wrote:

> Based on my test, this modification is also needed in
> tas2783_sdca_dev_resume().

(Restoring linux-sound and the rest of the Cc list, since the patch was
posted there.)

Thank you for looking at it.  I agree the same sync is a problem there,
and the ordering makes that unambiguous: sdw_handle_slave_status() calls
the driver's update_status() callback - where the cache is now dropped
and tas_io_init() re-downloads the firmware - and only afterwards does
complete_all(&slave->initialization_complete).  That completion is what
sdw_slave_wait_for_init() at the top of tas2783_sdca_dev_resume() is
waiting for, so by the time regcache_sync() runs the part has already
been soft-reset and re-initialized.  Syncing there writes the cache back
onto a device that has just been brought up from scratch.

What I would not do is drop unconditionally in dev_resume(), because
that function is also the RUNTIME_PM_OPS resume callback.  On a resume
where the peripheral kept its context, or came back through clock stop
without losing state, regcache_sync() is the only thing that restores
the user's settings, and dropping the cache there would silently reset
volume and mute on every runtime resume.

So the shape I have in mind is a flag rather than a second drop: set it
in tas_update_status() on the uninitialized-attach path where the cache
is dropped today, consume it in dev_resume(), and simply skip the sync
when it is set - after tas_io_init() the cache already mirrors the
hardware, so there is nothing worth syncing, only registers that can
fail.  Something like:

	if (test_and_clear_bit(TAS_REINIT, &tas_dev->flags))
		return 0;	/* re-initialized from scratch, cache is fresh */

	regcache_cache_only(tas_dev->regmap, false);
	ret = regcache_sync(tas_dev->regmap);

Would you prefer that, or do you have a different fix in progress on
your side?  I am happy to write and test it either way - I just do not
want us to post two versions of the same thing.

Before I write the changelog, though, I need to describe a failure I can
actually point at, and this is where I have to ask what you saw.  On the
board I have here - ASUS ProArt PX13, AMD ACP7.0, two TAS2783 plus RT721
on link 1 - I cannot reproduce a failure on that path:

- the codec never reaches runtime suspend at all (runtime_status stays
  active, the usage count never drops to zero), so the runtime resume
  path is not exercised;

- on the system resume path, across s2idle cycles where both amplifiers
  genuinely lose power, re-attach and re-download the firmware, the
  journal shows no resume error at all.  My reading is that
  regcache_sync() returns early: regcache_cache_only(true) does not by
  itself set cache_dirty, and if nothing writes through the cache while
  the device is suspended, sync takes the "if (!map->cache_dirty) goto
  out" exit and never touches the bus.  Which would mean the bug is
  latent here and armed only when something does dirty the cache during
  suspend.

That it is armed at all is easy to show: when I force the sync on this
part (through a small debug module, outside of any suspend), it aborts
at 0x40400108, FU23 Mute ch0, with -ENODATA - reg_defaults claims 0x1
while the init sequence writes 0x00, so sync tries to "restore" a value
the device refuses.  A dev_resume() that reaches the sync on this
hardware would therefore fail outright and return -ENODATA to the PM
core, not merely leave the amplifier stale.  That is one more instance
of the reg_defaults question in my other mail of 24 August [1].

So could you tell me a bit more about your test:

1. Which resume path - runtime resume, or system resume from s2idle/S3?
2. Which tree, and does it already contain b627da430357 in
   update_status()?
3. What did you observe - a sync error code in the log, or silent
   speakers with no error at all?
4. Does your board's controller power-gate the link across suspend, so
   the amplifiers re-attach uninitialized, or do they keep context?

With that I can write the patch against a failure that is described
rather than assumed, and test it here by forcing the cache dirty across
suspend.

[1] https://lore.kernel.org/linux-sound/20260824104500.7588-1-andrey.golovko@gmail.com/

Thanks,
Andrey