[PATCH 0/3] ASoC: tas2783: fix stereo split and resume on a two-amp pair

Ville Saarinen posted 3 patches 1 month, 2 weeks ago
sound/soc/codecs/tas2783-sdw.c | 157 +++++++++++++++++++++++++++++++++
1 file changed, 157 insertions(+)
[PATCH 0/3] ASoC: tas2783: fix stereo split and resume on a two-amp pair
Posted by Ville Saarinen 1 month, 2 weeks ago
Three fixes for the SoundWire TAS2783 driver, found while bringing up an
HP OmniBook X Flip 14-kc0xxx, which carries two TAS2783 amplifiers
aggregated on one link. Two of the three are not board-specific: as far as
I can tell any machine with an aggregated TAS2783 pair is affected today.

Patch 1 makes deferred SDCA writes work. A Function answering
COMMAND_IGNORED has deferred the transaction, and regmap-sdw-mbq is
supposed to poll Entity-0 Function Status and retry -- but this driver
neither makes that status register readable nor sets the poll interval and
deadline, so the poll is skipped and the retry is instantaneous. Every
deferred write fails by construction with -ENODATA.

Patch 2 makes an aggregated pair render stereo. Both amplifiers currently
play the same channel, so right-channel content is inaudible. The SDCA
control that would select the channel, the UDMPU23 Cluster Index, turns
out not to be implemented on this part -- a genuine device read of it
returns -ENODATA -- so the split is done host-side, by claiming a single
channel per amplifier and dropping the pair out of the SoundWire core's
mirror mode. That is exposed as a boolean "RX Single Channel Switch",
off by default, so the change is opt-in from the machine's UCM profile.

The switch deliberately does not name a side. sdw_compute_slave_ports()
advances the payload offset by the popcount of ch_mask and never looks at
which bit is set, so a driver cannot choose which channel an amplifier
renders; that follows from the amplifier's position in the slave iteration
order, i.e. from the machine driver's codec order. An earlier version of
this series exposed an rt1316/rt1318-style "RX Channel Select" enum with
Left/Right values, and I am glad it did not go out: those values were
measured to be inert, and the control would have promised an ABI the bus
allocator cannot honour.

Patch 3 stops the amplifiers losing their firmware tuning on resume.
Firmware is downloaded with sdw_nwrite_no_pm(), which bypasses the regmap
cache, so the cache keeps stale reg_defaults for every firmware-owned
register; the regcache_sync() on resume then writes those defaults back
over live firmware values. On the affected machine the speakers are dead
after every system resume and stay dead until reboot.

Patch 3 is the one I would most like reviewed on its own merits. It is the
smallest of the three, it is not specific to a stereo pair, and its
symptom is severe.

The three are independent and can be taken separately, though patch 2
builds on the mbq configuration touched by patch 1.

Testing
=======

All three are running on the affected machine and the whole stack works
from a cold boot with no manual steps, including across suspend/resume.

Patch 2 was measured rather than judged by ear: a 1 kHz tone that is
left-only for its first half and right-only for its second, played as one
continuous stream, isolating each amplifier by muting the other and
capturing on the machine's internal DMIC array, with every condition
normalised against both amplifiers muted. Each run was verified from the
kernel log to have re-run hw_params with the setting under test, since the
switch is read at hw_params and a sink that never suspends will not pick
up a change. Numbers are in that patch's changelog; briefly, with the
switch on each amplifier carries one side and the other side sits within
0.4 dB of the muted floor, and with it off both amplifiers render the same
left channel. Ear tests on this hardware are unreliable -- the two
speakers are close enough together that localisation gave a confidently
wrong answer more than once.

Patch 3's mechanism was confirmed by cache-bypassing debugfs register
reads taken while the amplifiers were dead, showing all 12 firmware-owned
registers reverted to their defaults. The fix has been through one
suspend/resume cycle with the speakers still working; that confirmation is
behavioural and I have not re-read the registers after a resume on the
fixed build.

Caveats
=======

The hardware testing above was done on v7.1.6 with clang. The three
patches apply to the master commit named below and have been compile-
tested there with gcc and W=1, with no new warnings; the series adds
exactly two new external references, regcache_drop_region (EXPORT_SYMBOL_
GPL) and snd_ctl_boolean_mono_info (EXPORT_SYMBOL), established by diffing
nm -u output against the unpatched objects. They have not been run on a
master kernel. I have exactly one TAS2783 machine, so the stereo split in
patch 2 is verified on one board with two amplifiers and nothing else.

Tool disclosure, per Documentation/process/generated-content.rst
================================================================

This work was done in extended interactive sessions with Claude (Anthropic,
model claude-opus-5) acting as a coding and debugging assistant, and a
substantial amount of the analysis and of the patch text originated with
it. All three patches carry an Assisted-by tag as described in
Documentation/process/coding-assistants.rst.

The division of work:

  - The assistant did the register-level analysis, read the relevant core
    code, formed the hypotheses, wrote the driver changes and the
    measurement harness, and drafted the changelogs.
  - I ran everything needing root or physical access, rebooted into each
    build, and ran the acoustic measurements.
  - No single prompt produced these patches. It was iterative over roughly
    a day, and a fair number of the assistant's intermediate conclusions
    were wrong: that one amplifier was dead, that the firmware page-0
    configuration set the channel, that the Cluster Index could be made to
    work from the driver, and -- latest and most relevant to what you are
    reading -- that an enum could assign a specific side to each
    amplifier. That last one survived into a fully drafted patch whose
    changelog claimed a per-side assignment, and was caught only by
    re-reading the bus allocator and then measuring both amplifiers set to
    the same value. Each wrong turn was discarded only because it was
    measured. The dead ends around the Cluster Index are summarised in
    patch 2's changelog, because they are the reason the split is done
    host-side.

I have reviewed all three patches, I understand what they do, and I take
responsibility for them.

Ville Saarinen (3):
  ASoC: tas2783: let regmap-sdw-mbq poll for deferred transactions
  ASoC: tas2783: add RX Single Channel Switch to split a two-amp stereo
    pair
  ASoC: tas2783: drop firmware-owned registers from the regmap cache

 sound/soc/codecs/tas2783-sdw.c | 157 +++++++++++++++++++++++++++++++++
 1 file changed, 157 insertions(+)


base-commit: 06cf61899d6498b33e4b7c87d99d5bd471ccc375
-- 
2.55.0
Re: [PATCH 0/3] ASoC: tas2783: fix stereo split and resume on a two-amp pair
Posted by Ville Saarinen 1 month, 2 weeks ago
I posted this series a few hours ago without having found the existing
work on exactly these bugs. That was my mistake: the threads were easy to
find and I did not look before sending. Apologies to those of you who have
been through this already. Adding Pierre-Louis, Robin, Antoine and Andrey
to Cc, and setting out below how the series relates to what is already
done, since a good part of it is not new.

Prior work I should have cited
==============================

  Robin Everaars, [BUG] every amp on the link selects the same channel
  https://lore.kernel.org/all/20260805183517.8665-1-robineveraars@pm.me/

  Antoine Monnet, no stereo channel split for two mono amps -> mono output
  https://lore.kernel.org/all/29e8c08b-9475-4aba-bce0-6d4a45a26d3b@gmail.com/

  Antoine Monnet, calibration firmware not re-downloaded after s2idle resume
  https://lore.kernel.org/all/c66ae00a-e878-4af0-a05a-272e9574eaa5@montane.tech/

  Andrey Golovko, ASoC: tas2783-sdw: drop stale regcache on uninitialized
  re-attach -- applied as b627da430357

  Andrey Golovko, port prepare never completes after S0i3
  https://lore.kernel.org/all/b1bc21c8a403fe15e742b6a6ff30b27f@gmail.com/

Everything above is on an ASUS ProArt PX13 HN7306EAC. My machine is an HP
OmniBook X Flip 14-kc0xxx (AMD Strix Point, ACP 7.2, two TAS2783 plus an
rt712-sdca on one link), so at least the reports now span two different
platforms and three different machines.

Patch 2 (RX Single Channel Switch): mostly not new
==================================================

The central finding in my changelog -- that sdw_compute_slave_ports()
advances the payload offset by hweight32(ch_mask) and never looks at which
bit is set, so a one-channel mask defeats mirror mode while L/R follows
slave iteration order rather than the mask value -- was published by Robin
before I sent, and Andrey restated it precisely in the 08-07 message. I
reached it independently, which is worth exactly nothing in terms of
credit; it is Robin's result and I should have cited it.

Antoine's patch derives the per-amp mask from name_prefix. Mine exposes a
boolean control, off by default, and leaves the decision to the machine's
UCM profile. The honest difference is narrow: Antoine's works with no
userspace change on boards where the prefix order matches the speakers,
mine needs a UCM cset but does not encode a side in the driver at all,
which was my reaction to the same "the bit does not pick the channel"
problem. I do not think mine is obviously better and I am happy to drop it
in favour of Antoine's, or to rebase whatever is useful in it on top.

One thing that may be worth keeping either way is the naming. Andrey's
note that the name_prefix -> BIT(n) mapping "reads as if the bit picks the
channel" is the same objection that made me rename my own control: an
earlier version of this patch was an rt1316-style "RX Channel Select" enum
with Left/Right values, and those values measured inert, exactly as the
allocator predicts. A control that names a side is an ABI promise the bus
cannot keep.

A data point for the UDMPU23 ClusterIndex question
==================================================

Pierre-Louis, in the 08-07 message you suggested experimenting with
non-zero cluster indices per amp, and asked TI to comment on whether the
index is the right place for this. I have measurements on that, and they
are discouraging on this part.

SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_UDMPU23, TAS2783_SDCA_CTL_UDMPU_CLUSTER, 0)
cannot be written at all here. The amplifier answers COMMAND_IGNORED
(-ENODATA) in every state I tried: streaming, idle, and with the SDCA
function confirmed powered on via PDE23 Actual Power State == ON; as a
4-byte MBQ write, as a plain single-byte write, after clearing the latched
Entity-0 status bits, and on the Next rank of the dual-ranked control.

The tell is that a genuine device read of the same Control with
sdw_read_no_pm() also returns -ENODATA. It is not write-protected and it
is not a ranking problem: on this device the Control is not implemented,
even though tas2783_reg_default[] carries an entry for it as 0x0. That
does not settle what the SDCA spec intends, and another TAS2783 revision
may well implement it -- but on this silicon the ClusterIndex route is
closed, which is why I did the split at the port level despite your point
that this is not what SDCA designs are supposed to do.

I would still like to hear TI on the intended mechanism. If the answer is
that PostureNumber is the right control and the Posture Table is supposed
to come from platform firmware, then none of the host-side approaches in
these threads is the real fix and it would be good to know that before one
of them lands.

Patch 3 (regcache): narrower than I described, and possibly still needed
========================================================================

I based this series on torvalds master, which does not yet carry Andrey's
b627da430357, so my changelog describes a bug that is already partly
fixed. Correcting that:

b627da430357 replaces the regcache_sync() in tas_update_status() with
regcache_drop_region(regmap, 0, UINT_MAX) on the uninitialized re-attach
path. That covers the case where the device lost power, went UNATTACHED
and cleared hw_init -- which is the case I measured.

What it does not cover is tas2783_sdca_dev_resume(), which still calls
regcache_sync() unconditionally (tas2783-sdw.c:1099 in broonie/for-next).
On a resume where the peripheral stayed attached and hw_init was never
cleared, that sync still writes stale reg_defaults over every
firmware-owned register, because the firmware is downloaded with
sdw_nwrite_no_pm() and the cache never saw those values. My patch drops
the firmware-owned regions from the cache at download time, which closes
that path too.

I want to be clear about the limits of my evidence: my measurement was on
v7.1.6, which predates b627da430357, so what I actually observed may have
been the UNATTACHED path that is now fixed. The residual dev_resume() path
is a code reading, not something I have measured in isolation on a tree
that already has Andrey's fix. I will test that properly and report back
rather than asking anyone to take the patch on this basis.

Patch 1 (deferred MBQ transactions)
===================================

I did not find prior coverage of this one. tas_regmap does not make
Entity-0 Function Status readable and does not set the mbq poll interval
or deadline, so regmap-sdw-mbq's retry for a Function answering
COMMAND_IGNORED never polls and every deferred write fails with -ENODATA
by construction. It may be relevant to the "port prepare never completes
after S0i3" thread; I have not tried to reproduce that symptom.

What I will do next
===================

Unless anyone would rather I did otherwise:

 - respin against broonie/sound for-next rather than master;
 - carry Link:/Reported-by: tags for Robin's and Antoine's reports;
 - drop or rework patch 2 depending on what happens with Antoine's;
 - hold patch 3 until I have measured the dev_resume() path on a tree
   containing b627da430357;
 - keep patch 1 as the one piece I believe is unencumbered.

Robin, Antoine, Andrey -- if you would like Reported-by: or Suggested-by:
on any of this, say so and I will add it; I did not want to attach your
names to a series you have not seen.

One disclosure that applies to this mail as much as to the patches: I work
on this with Claude (Anthropic, claude-opus-5) as an assistant, and a
substantial part of the analysis above, including the register-level
ClusterIndex work, originated with it. The cover letter has the full
statement. The measurements are mine, run on my hardware, and I take
responsibility for the claims either way.

Thanks, and sorry again for the duplicated effort.

Ville
Re: [PATCH 0/3] ASoC: tas2783: fix stereo split and resume on a two-amp pair
Posted by Robin Everaars 1 month, 2 weeks ago
> Robin, Antoine, Andrey -- if you would like Reported-by: or Suggested-by:
> on any of this, say so and I will add it; I did not want to attach your
> names to a series you have not seen.

For patch 2, please add both:

  Reported-by: Robin Everaars <robineveraars@pm.me>
  Suggested-by: Robin Everaars <robineveraars@pm.me>

The report and the follow-up measurement established both the one-channel mask
approach and the positional behaviour in sdw_compute_slave_ports(), so those
tags fit. I have no basis for a tag on patches 1 or 3.

I also retested the machine today after booting kernel 7.1.7. The cold-boot path
still fails on the second amplifier:

  command timeout for Slave 2
  trf on Slave 2 failed:-110 write addr 8088 count 32632
  FW download failed: -110
  SDW1 manager is in bad state

The existing reprobe service recovered both amplifiers on its first attempt.
A fresh acoustic run after that recovery gave:

  LEFT only    +69.0 dB over baseline
  RIGHT 
only   +64.4 dB over baseline
  BOTH         +71.0 dB over baseline
  imbalance     +4.6 dB

Both channels reached the speakers.

That measurement only establishes the 7.1.7 baseline; I did not run your
series. My configured out-of-tree module still carries the local per-amp PPU21
write that Pierre correctly rejected as an upstream mechanism. I therefore am
not offering a Tested-by or Reviewed-by for patch 2 yet. I also have not
isolated the residual dev_resume() path for patch 3 on top of b627da430357.

The boot failure is why I am keeping both the boot and resume reprobe services
for now. I will remove them only after the unassisted paths work upstream.

Thanks for the careful follow-up and for separating the prior work from the new
parts of the series.
Re: [PATCH 0/3] ASoC: tas2783: fix stereo split and resume on a two-amp pair
Posted by Ville Saarinen 1 month, 2 weeks ago
> For patch 2, please add both:
>
>   Reported-by: Robin Everaars <robineveraars@pm.me>
>   Suggested-by: Robin Everaars <robineveraars@pm.me>

Both are yours, but don't wait on a v2 from me -- I'm stepping back and
leaving this to people who know the subsystem. On the record for whoever
picks it up: the channel-mask approach and the positional behaviour of
sdw_compute_slave_ports() were established by your report of 2026-08-05,

  https://lore.kernel.org/all/20260805183517.8665-1-robineveraars@pm.me/

four days before I posted. I got there independently, which counts for
nothing -- it was already published. Antoine's patch also predates mine
and is the more likely vehicle. The tags above belong on it.

So nobody is waiting on me: patch 1 is Charles Keepax's now, fixing the
reversed timeouts in the regmap core first
(https://lore.kernel.org/all/ansTPGgVNoDJlA5r@opensource.cirrus.com/).
Patch 2 I'm not pursuing -- whether the mapping belongs in UDMPU23/FU23
needs the spec and TI, not another host-side patch from me. Patch 3 I'm
not pressing: it was measured on v7.1.6, predating b627da430357, so the
evidence is weaker than my changelog claimed. Nothing there is worth
your time isolating on my account.

Thanks for the 7.1.7 retest, and for saying plainly what it did and
didn't cover.

Ville