tas2783-sdw: questions about the register tables

tas2783-sdw: questions about the register tables

[Andrey Golovko]

Hi,

Working on the read-only Control series [1] left me with a set of
questions about tas2783_reg_default[] and tas2783_sdca_mbq_size() that
only you can answer.  They are collected here rather than spread over
patch threads.  Nothing here breaks anything in tree today - the driver
reads almost none of these registers - but the tables, the DisCo
description and the part disagree with each other in ways that will
matter as soon as anything does read them.

Measurements are from an ASUS ProArt PX13 (HN7306EAC, BIOS 307), two
TAS2783 aggregated on one AMD ACP link, reads issued with
sdw_read_no_pm() or through the regmap with the cache bypassed.  Both
amplifiers answer identically unless stated, and their ACPI Function
nodes are byte-identical.

1. The tables and DisCo describe different parts.

   The platform describes the Smart Amp Function in some detail: 27
   Entities with their Controls.  The driver ignores that and uses its
   own tables, so the two can be compared.

   Entities the driver knows that DisCo does not describe: CS24 (0x05),
   OT24 (0x17), OT127 (0x1E), CS127 (0x20), MFPU21 (0x22), MFPU26
   (0x23).  Entities DisCo describes that the driver does not know:
   0x0B, 0x0D, 0x1D and 0x37 - 0x0B and 0x0D look like two more Power
   Domain Entities, each with Controls 0x01 and 0x10 like PDE23.

   Is the driver's table a superset written against a different
   configuration of the part, or is the platform description short?

2. Latency Controls.

   The driver's read-only list holds 21 Latency Controls.  DisCo
   describes exactly one of them: FU21 0x10.  For eighteen the Entity is
   described and the Latency is not; two more belong to Entities DisCo
   does not have at all.

   And FU21 0x10 - the single one the platform describes - is one of the
   two the driver cannot reach: it appears in no case of
   tas2783_sdca_mbq_size(), so the callback returns 0, and both
   tas2783_readable_register() and the cache refuse it.  The same holds
   for UDMPU23 0x06.  The other nineteen are declared four bytes wide
   and carry a 0x0 placeholder in tas2783_reg_default[].

   The DSDT settles what FU21 0x10 is: access mode 0x05, i.e. DC, with
   mipi-sdca-control-dc-value 0x0.  A DisCo constant, as Charles Keepax
   described - hard coded in ACPI, not implemented in the hardware, and
   normally given a default-array entry with no readable/writeable case
   so the constant can still be read.  Which is the one shape the driver
   does not give it.

   But that only covers the one.  What are the other twenty Latencies,
   which no DisCo entry describes and which the tables answer with a 0x0
   placeholder?  Are they implemented in this part?

3. XU22.

   The driver's read-only list has XU22 0x06 (Latency), 0x07 (XU_ID) and
   0x08 (XU_Version).  DisCo describes none of the three, and XU22 0x07
   answers COMMAND_IGNORED here, powered and unpowered, on both
   amplifiers.  Not implemented?

   Conversely, XU22 0x09 and 0x0a are in tas2783_reg_default[] and in
   the size callback - 0x09 as two bytes, 0x0a as four - and DisCo does
   describe them, as constants rather than registers (see 4), even
   though SDCA 1.0 section 6.3.10.3 defines no Control Selectors 0x09
   and 0x0a for an Extension Unit.  What are they?

   XU22 0x12 (FDL_MessageOffset) is described by DisCo and still answers
   COMMAND_IGNORED in both power states.  (Before the regmap val_bits
   widening in 1/2 of [1] it returned -EINVAL without reaching the bus,
   since it is declared four bytes wide.)

4. DisCo access modes disagree with the tables in both directions.

   Reading the access mode of every Control in the Function turned up
   Controls the driver treats as ordinary writable registers that the
   platform declares constant or read-only:

     XU22 0x09, XU22 0x0a   DC, dc-value 0x0
     UDMPU23 0x10           DC, dc-value 0x1 - the driver's default is 0x0
     SAPU29 0x10            RO (Protection Mode)
     XU22 0x15              RO
     IT21 and IT26 0x10 and 0x11, IT28 0x11, OT25 and OT28 0x11,
     FU127 0x01, 0x04 and 0x05, MU26 0x01     all DC

   Nothing in the driver writes most of these today, so this is latent
   like the rest, but tas2783_writeable_register() would let a write
   through to a Control the part does not implement, and for UDMPU23
   0x10 the cached value differs from the constant the platform states.

   One more of that kind, outside the read-only question: FU21 Channel
   Volume 0x02 has a DisCo default of 0xFA00 with a range of 0x9C00 to
   0x0000 in steps of 0x19, while tas2783_reg_default[] gives it 0x9C00,
   the bottom of that range.  Is the table value deliberate?

   More generally: the kernel can parse this description now.  Is moving
   the driver onto the SDCA parser, rather than maintaining tables that
   have drifted from it, something you are considering?

5. Placeholders for state readings.

   PDE23 Actual Power State has a default of 0x3 while the peripheral
   answers 0x0; SAPU29 Protection Status has 0x0 while the peripheral
   answers 0x3.  Both are cached, so a read returns the opposite of the
   device state in both cases.  Patch 2/2 of [1] addresses this by
   marking them volatile - is there a reason the entries are there?

6. FU23 Mute, channel 0.

   regcache_sync() aborts there, at 0x40400108, every time: the default
   is 0x1, the cache holds 0x00 after bring-up, so the sync tries to
   restore 0x1 and the peripheral answers COMMAND_IGNORED.  Being the
   first mismatch in address order, nothing after it is restored either,
   so a sync effectively does nothing on this part.  What is the reset
   value of the FU23 Mutes, and should they be in the defaults at all?

7. Calibration register width.

   tas2783_set_calib_params_to_device() wrote each of the five
   calibration values with regmap_bulk_write(..., buf, sizeof(u32)),
   relying on val_bits = 8 to spread one 32-bit value over four
   consecutive single-byte registers, most significant byte first.  With
   val_bits widened that call would mean something else, so 1/2 of [1]
   writes the four registers explicitly in the same order; read back
   over the bus, the five values of both amplifiers are byte for byte
   what they were before.  Please confirm the layout is what the part
   expects - the tables declare these registers one byte wide and
   nothing else documents the order.

8. simple_ch_prep_sm.

   The platform sets it for DP1, the only data port these amplifiers
   have, so the core skips the DPn_PrepareStatus poll.  That is what
   made the resume failure fixed by 119046319e77 silent: the port never
   finished preparing and nothing in the kernel noticed.  Does the part
   implement the full channel prepare state machine, i.e. is the
   property right?

9. Power symmetry.

   With 119046319e77 the Function is powered up in the port PRE_PREP
   callback while the power down stayed in tas_sdw_pcm_hw_free().  If
   you would prefer the two symmetric, moving the power down to
   POST_DEPREP is a small patch and I am happy to send it.

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

Thanks,
Andrey

Re: tas2783-sdw: questions about the register tables

[Pierre-Louis Bossart]

> 1. The tables and DisCo describe different parts.
> 
>    The platform describes the Smart Amp Function in some detail: 27
>    Entities with their Controls.  The driver ignores that and uses its
>    own tables, so the two can be compared.
> 
>    Entities the driver knows that DisCo does not describe: CS24 (0x05),
>    OT24 (0x17), OT127 (0x1E), CS127 (0x20), MFPU21 (0x22), MFPU26
>    (0x23).  Entities DisCo describes that the driver does not know:
>    0x0B, 0x0D, 0x1D and 0x37 - 0x0B and 0x0D look like two more Power
>    Domain Entities, each with Controls 0x01 and 0x10 like PDE23.
> 
>    Is the driver's table a superset written against a different
>    configuration of the part, or is the platform description short?

In a perfect world, the DisCo tables would contain all the required
information and the drivers would just use it blindly.
In reality, it's very common for the platform firmware to contain
errors, and for drivers developers to start their own code with what
works for them...

> 2. Latency Controls.
> 
>    The driver's read-only list holds 21 Latency Controls.  DisCo
>    describes exactly one of them: FU21 0x10.  For eighteen the Entity is
>    described and the Latency is not; two more belong to Entities DisCo
>    does not have at all.
> 
>    And FU21 0x10 - the single one the platform describes - is one of the
>    two the driver cannot reach: it appears in no case of
>    tas2783_sdca_mbq_size(), so the callback returns 0, and both
>    tas2783_readable_register() and the cache refuse it.  The same holds
>    for UDMPU23 0x06.  The other nineteen are declared four bytes wide
>    and carry a 0x0 placeholder in tas2783_reg_default[].
> 
>    The DSDT settles what FU21 0x10 is: access mode 0x05, i.e. DC, with
>    mipi-sdca-control-dc-value 0x0.  A DisCo constant, as Charles Keepax
>    described - hard coded in ACPI, not implemented in the hardware, and
>    normally given a default-array entry with no readable/writeable case
>    so the constant can still be read.  Which is the one shape the driver
>    does not give it.
> 
>    But that only covers the one.  What are the other twenty Latencies,
>    which no DisCo entry describes and which the tables answer with a 0x0
>    placeholder?  Are they implemented in this part?

Huh, you can safely ignore Latency Controls. This is something that was
'inspired' by USB audio, where this was also ignored. No one knows what
to do with Latency controls...

> 3. XU22.
> 
>    The driver's read-only list has XU22 0x06 (Latency), 0x07 (XU_ID) and
>    0x08 (XU_Version).  DisCo describes none of the three, and XU22 0x07
>    answers COMMAND_IGNORED here, powered and unpowered, on both
>    amplifiers.  Not implemented?
> 
>    Conversely, XU22 0x09 and 0x0a are in tas2783_reg_default[] and in
>    the size callback - 0x09 as two bytes, 0x0a as four - and DisCo does
>    describe them, as constants rather than registers (see 4), even
>    though SDCA 1.0 section 6.3.10.3 defines no Control Selectors 0x09
>    and 0x0a for an Extension Unit.  What are they?
> 
>    XU22 0x12 (FDL_MessageOffset) is described by DisCo and still answers
>    COMMAND_IGNORED in both power states.  (Before the regmap val_bits
>    widening in 1/2 of [1] it returned -EINVAL without reaching the bus,
>    since it is declared four bytes wide.)

XU stands for eXtension Unit, it's where all the secret-sauce is
handled. In practice this is what's used for firmware download. As long
as the firmware download works I wouldn't spend time looking at this.

> 4. DisCo access modes disagree with the tables in both directions.
> 
>    Reading the access mode of every Control in the Function turned up
>    Controls the driver treats as ordinary writable registers that the
>    platform declares constant or read-only:
> 
>      XU22 0x09, XU22 0x0a   DC, dc-value 0x0
>      UDMPU23 0x10           DC, dc-value 0x1 - the driver's default is 0x0
>      SAPU29 0x10            RO (Protection Mode)
>      XU22 0x15              RO
>      IT21 and IT26 0x10 and 0x11, IT28 0x11, OT25 and OT28 0x11,
>      FU127 0x01, 0x04 and 0x05, MU26 0x01     all DC
> 
>    Nothing in the driver writes most of these today, so this is latent
>    like the rest, but tas2783_writeable_register() would let a write
>    through to a Control the part does not implement, and for UDMPU23
>    0x10 the cached value differs from the constant the platform states.

Yeah, in general the DC and RO access are a bit problematic, sources of
errors...

>    One more of that kind, outside the read-only question: FU21 Channel
>    Volume 0x02 has a DisCo default of 0xFA00 with a range of 0x9C00 to
>    0x0000 in steps of 0x19, while tas2783_reg_default[] gives it 0x9C00,
>    the bottom of that range.  Is the table value deliberate?

In theory the DisCo default is supposed to be a platform-specific
information. Setting the volume at the driver level is pretty bad in
that all platforms using that amp would use the same volume default.
That said, if the BIOS writers copy-paste the value without asking the
audio integration folks then that'd be equally bad.

>    More generally: the kernel can parse this description now.  Is moving
>    the driver onto the SDCA parser, rather than maintaining tables that
>    have drifted from it, something you are considering?
> 
> 5. Placeholders for state readings.
> 
>    PDE23 Actual Power State has a default of 0x3 while the peripheral
>    answers 0x0; SAPU29 Protection Status has 0x0 while the peripheral
>    answers 0x3.  Both are cached, so a read returns the opposite of the
>    device state in both cases.  Patch 2/2 of [1] addresses this by
>    marking them volatile - is there a reason the entries are there?

all volatile registers should be handled as such with no defaults...

> 6. FU23 Mute, channel 0.
> 
>    regcache_sync() aborts there, at 0x40400108, every time: the default
>    is 0x1, the cache holds 0x00 after bring-up, so the sync tries to
>    restore 0x1 and the peripheral answers COMMAND_IGNORED.  Being the
>    first mismatch in address order, nothing after it is restored either,
>    so a sync effectively does nothing on this part.  What is the reset
>    value of the FU23 Mutes, and should they be in the defaults at all?

that points to an unimplemented register...>
> 7. Calibration register width.
> 
>    tas2783_set_calib_params_to_device() wrote each of the five
>    calibration values with regmap_bulk_write(..., buf, sizeof(u32)),
>    relying on val_bits = 8 to spread one 32-bit value over four
>    consecutive single-byte registers, most significant byte first.  With
>    val_bits widened that call would mean something else, so 1/2 of [1]
>    writes the four registers explicitly in the same order; read back
>    over the bus, the five values of both amplifiers are byte for byte
>    what they were before.  Please confirm the layout is what the part
>    expects - the tables declare these registers one byte wide and
>    nothing else documents the order.
> 
> 8. simple_ch_prep_sm.
> 
>    The platform sets it for DP1, the only data port these amplifiers
>    have, so the core skips the DPn_PrepareStatus poll.  That is what
>    made the resume failure fixed by 119046319e77 silent: the port never
>    finished preparing and nothing in the kernel noticed.  Does the part
>    implement the full channel prepare state machine, i.e. is the
>    property right?

this has been addressed in multiple threads. the short story is that we
stick to this simple_ch_prep_sm mode.>
> 9. Power symmetry.
> 
>    With 119046319e77 the Function is powered up in the port PRE_PREP
>    callback while the power down stayed in tas_sdw_pcm_hw_free().  If
>    you would prefer the two symmetric, moving the power down to
>    POST_DEPREP is a small patch and I am happy to send it.
possibly. In general, ports and functions are not really at the same
level and there's no reason why the two are linked. I don't fully
remember the issue on this chip but this was not a generic problem. TDB
with TI if they need a symmetry for the power-down.

Re: tas2783-sdw: questions about the register tables

[Andrey Golovko]

On Fri, Sep 04, 2026 at 08:23:23AM +0000, Pierre-Louis Bossart wrote:

Thank you - that settles most of the list.  Below is what it changes on
my side, and one correction: your reading of the FU23 Mute sent me back
to the bus, and the sync failure turned out to have a different cause
than either of us described.  It is measured down to the mechanism.

> Huh, you can safely ignore Latency Controls. This is something that was
> 'inspired' by USB audio, where this was also ignored. No one knows what
> to do with Latency controls...

Good, that closes the odd part of the question too.  The one Latency the
platform describes here, FU21 0x10, is a DisCo constant that the driver
cannot reach, and the other twenty are placeholders in the defaults
table; if nobody wants a Latency, the answer is not to make them
readable but to stop describing them.  So in the next revision of

  [PATCH v2 2/2] ASoC: tas2783-sdw: do not cache read-only Controls
  Message-ID: <20260814094000.22118-3-andrey.golovko@gmail.com>

the twenty Latency entries leave tas2783_reg_default[] rather than
becoming volatile, and no case is added to tas2783_sdca_mbq_size() for
FU21 0x10.  The same for the XU22 id and version, per your answer on the
Extension Unit.

> all volatile registers should be handled as such with no defaults...

That is exactly the shape of the patch, and the placeholders are wrong
in both directions on this part.  Measured over the bus on an ASUS
ProArt PX13, both amplifiers, Function powered (PDE23 requested and
actual both PS0):

  PDE23 Actual Power State    table 0x3 (PS3)   device 0x0 (PS0)
  SAPU29 Protection Status    table 0x0         device 0x3

With the Controls cached, a regmap read answers the table in both cases,
i.e. the opposite of the device state.  With them marked volatile and
their entries dropped, the read reaches the peripheral and matches the
bus.  I have been running that patch here since mid-August: calibration
values read back byte for byte identical to a kernel without it, and
nothing else in the driver reads those Controls at all.

> that points to an unimplemented register...

It is implemented, and the sync failure turns out not to be about that
Control at all.  Measured on both amplifiers of an ASUS ProArt PX13,
7.2.0-rc6 plus for-next and the read-only Control series:

  - FU23 Mute of channel 0, 0x40400108, reads 0x0 and accepts writes both
    over the bus and through the regmap, with the Function in PS0 and in
    PS3.  The Mute of channel 1 reflects what tas_fu23_event() writes.
    2000 back-to-back writes of either: no error.

  - What misled me in the trace I posted in August [2] is that
    trace_regmap_reg_write() fires only when the write returned 0.  The
    last address in the trace is therefore the last *successful* write,
    not the failing one.  The write that fails is the next one the sync
    performs: the PDE23 Requested Power State, 0x40400608.

  - PDE23 on its own is fine: 12000 writes of PS0, back to back and
    spaced apart, no error.

  - What breaks it is the write immediately before it.  regcache_sync()
    writes, in address order, the registers whose cached value differs
    from the table default - 13 of them here - which ends the firmware
    block with 0x0080042b and then requests the power state.  Replaying
    that sequence with plain sdw_write_no_pm(): the PDE23 write is
    answered COMMAND_IGNORED 268 to 278 times out of 300.  The same
    sequence started at the FU23 Mute, i.e. without the firmware block:
    no errors at all.

  - Narrowed to one register and one delay, 300 iterations each,
    same result on both amplifiers:

      0x0080042b then PDE23, no delay       300/300 ignored
      0x0080042b then PDE23, 50 us          299/300, 300/300
      0x0080042b then PDE23, 200 us         221/300, 227/300
      0x0080042b then PDE23, 1 ms             0/300
      0x0080042b then FU23 Mute               0/300
      0x0080042b then PPU21 0x10              0/300
      0x00800418 then PDE23                   0/300

    So a write to book 0, page 8, register 0x2b - the last byte of the
    32-bit value at 0x28, which both tas2783_init_seq[] and the sync
    write - leaves the Power Domain Entity refusing a power state
    request for something under a millisecond.  Nothing else is refused,
    and no other register of that block has the effect.

  - It does not latch: after the sequence, 500 lone PDE23 writes all
    succeed.

That explains the failure rate too - regcache_sync() aborted in 10 of 20
attempts in one run and 4 of 20 in another, which is what a sub-
millisecond window looks like from the outside.  And it explains why
tas2783_init_seq[] never trips over it: the init writes the same four
registers and then the FU23 Mute, not the power state.

So my August description of this - "the sync dies at the FU23 Mute of
channel 0" - was wrong in both halves, and I would rather correct it
here than leave it in the archive.  The Mute is implemented, and the
sync dies at the power state request that follows the firmware block.

Two questions to TI, then.  What is book 0 page 8 register 0x28, and
does the part require a delay or a status poll after that value is
written?  And is a Power Domain Entity expected to answer
COMMAND_IGNORED to a Requested Power State write while it is busy?  If
it is, then every driver path that writes PDE23 should retry rather than
propagate -ENODATA - including tas_port_prep() as it stands after
119046319e77, which fails the port preparation on the first refusal.

Independently of the answer, the sync should not be writing those
firmware registers at all: they are device state written by the
downloaded firmware, they sit in tas2783_reg_default[] with table
values, and Ville reported earlier that a sync puts the table values
back over what the firmware wrote.  The FU23 Mute entries are a separate
question - what is their reset value, and should they be in the defaults
at all?

> XU stands for eXtension Unit, it's where all the secret-sauce is
> handled. In practice this is what's used for firmware download. As long
> as the firmware download works I wouldn't spend time looking at this.

Understood - firmware download works here, so I will leave XU22 alone.

> this has been addressed in multiple threads. the short story is that we
> stick to this simple_ch_prep_sm mode.

Fine by me; the driver-side fix in 119046319e77 does not depend on it.
It only means a port that fails to prepare stays silent, so I will keep
that in mind when reading future reports rather than proposing to change
the property.

> possibly. In general, ports and functions are not really at the same
> level and there's no reason why the two are linked. [...] TBD with TI if
> they need a symmetry for the power-down.

Then I will leave the power-down where it is and let TI say whether they
want it in POST_DEPREP; the patch is two lines whenever they do.

[2] Message-ID: <20260813213000.13990-1-andrey.golovko@gmail.com>

Thanks,
Andrey