sound/soc/codecs/tas2783-sdw.c | 157 +++++++++++++++++++++++++++++++++ 1 file changed, 157 insertions(+)
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
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
> 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.
> 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
© 2016 - 2026 Red Hat, Inc.