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@xxxxxxxxxxxx>
> Closes: https://lore.kernel.org/all/c66ae00a-e878-4af0-a05a-272e9574eaa5@xxxxxxxxxxxx/
> Signed-off-by: Andrey Golovko <andrey.golovko@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx/
> 
>  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);