Re: [PATCH 3/3] ASoC: tas2783: drop firmware-owned registers from the regmap cache

Ville withdrew this patch partly because its evidence was taken on
v7.1.6, before b627da430357.  I measured the same mechanism on current
broonie/sound for-next today, so the evidence now exists either way.
Four results, one of which is his.

Method: a small out-of-tree module takes the codec's regmap with
dev_get_regmap(), calls regcache_mark_dirty() and regcache_sync(), and
the writes are captured with the regmap:regmap_reg_write tracepoint.
The amplifiers are powered while this runs - a tone is playing on the
hw device - so nothing fails merely because the Function is in PS3.
Machine: ASUS ProArt PX13 HN7306EAC, two TAS2783 aggregated on SDW1,
kernel 6f6fb37f9f9a.

1. regcache_sync() writes firmware-owned registers back to their table
   defaults, exactly as this patch says.  Per amplifier:

     reg=80005c val=1d      reg=800428 val=40
     reg=80006b val=7b      reg=800429 val=0
     reg=800418 val=0       reg=80042a val=0
     reg=800419 val=0       reg=80042b val=0
     reg=80041a val=0
     reg=80041b val=0

   b627da430357 does not cover this: it drops the cache when the
   peripheral re-attaches uninitialized, which is the S0i3 path.  A sync
   with a live cache still restores these defaults over whatever the
   firmware put there.

2. The sync then aborts, on both amplifiers, at

     reg=40400108 val=0

   which is the FU23 Mute of channel 0, and the peripheral answers
   -ENODATA.  That Control is in tas2783_reg_default[] with 0x1 while
   the driver's own init sequence writes 0x00 to it, so the cache
   disagrees with the default and the sync tries to restore it.  Every
   regcache_sync() on this driver stops there.  So today the sync is
   both harmful, per (1), and incomplete: whatever sits above that
   address is never restored at all.

3. The read-only Controls are cached, so they answer from the cache and
   report state the device left long ago.  With the Function in PS0, a
   regmap read of the PDE23 actual power state returns the 0x3
   placeholder from the defaults table - PS3 - while a cache-bypassing
   read of the same Control over the bus answers 0x0.  I have posted a
   patch for that:

     ASoC: tas2783-sdw: do not cache read-only Controls
     Message-ID: <[email protected]>

   It marks them volatile and drops them from the defaults table, which
   is the half of your suggestion, Mark, that fits this part cleanly.
   The caching half is still open, and Ville's point about the two RAM
   ranges stands: page 0 wants caching, PRAM and YRAM do not.

4. Two Controls the driver declares as multi-byte answer -ENODATA from
   the device even in PS0, on both amplifiers: XU22 Control 0x07, two
   bytes, and XU22 Control 0x12, four bytes.  This looks like the
   Cluster Index situation Ville hit - declared by the driver, not
   implemented by the part.

   Separately, those two are unreachable through the regmap in the first
   place: tas2783_sdca_mbq_size() declares registers of one, two and
   four bytes while the regmap is created with val_bits = 8, so
   regmap_sdw_mbq_size() rejects them with -EINVAL before anything
   reaches the bus.  That is measurable as the difference between two
   kernels: -EINVAL with val_bits = 8, -ENODATA from the device with
   val_bits widened.  I have a patch for it but it changes nothing that
   can be heard, and it does not fix the sync - the abort in (2) happens
   at a single-byte Control long before any multi-byte one is reached.

Questions for TI, which the above keeps running into:

  - What are XU22 Controls 0x09 (declared two bytes) and 0x0a (one
    byte)?  SDCA defines XU Control selectors 0x01, 0x06, 0x07, 0x08,
    0x10 and 0x12-0x16; 0x09 and 0x0a are not among them.

  - Should the read-only Controls be in tas2783_reg_default[] at all?
    31 of the entries are readings - latencies, clock valid, actual
    power state, protection status, algorithm ready, firmware download
    status - each with a placeholder that the device never agrees with.

  - The calibration values are written as four consecutive single-byte
    registers, most significant byte first, and mbq_size() declares that
    whole page one byte wide.  Is that the intended layout and width?

  - The BIOS on these machines describes no Smart Amp SDCA function, so
    the driver falls back to its static tables and never gets
    sdca_regmap_writeable() or sdca_regmap_deferrable().  Is that
    expected on shipping hardware?

I can run further probes on this machine; the driver performs no
register reads at all today, so anything read here has to come from
outside it.

Thanks,
Andrey