Re: [EXTERNAL] Re: [PATCH v5] ASoC: tas2783-sdw: add firmware download status check

[Xu, Baojun]

Hi,

> From: Mark Brown <broonie@xxxxxxxxxx>
> Sent: 21 September 2026 16:45
> To: Xu, Baojun
> Cc: tiwai@xxxxxxx; andriy.shevchenko@xxxxxxxxxxxxxxx; 13916275206@xxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; Ding, Shenghao; linux-sound@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Yi, Ken; Lo, Henry; Chen, Robin; Holalu Yogendra, Niranjan; pin-hao.huang@xxxxxx; Syed.SabaKareem@xxxxxxx
> Subject: [EXTERNAL] Re: [PATCH v5] ASoC: tas2783-sdw: add firmware download status check
> 
> On Mon, Sep 21, 2026 at 02:40:53PM +0800, Baojun Xu wrote:
> > Currently, the firmware download is unnecessarily triggered on every
> > system resume from suspend, causing significant wake-up latency. However,
> > this step is redundant if the AMP remains powered on.
> > Furthermore, PRAM access is skipped if the firmware version read from
> > registers matches the expected value, indicating that the memory content
> > was retained across the AMP reset.
> 
> > @@ -431,6 +432,8 @@ static int tas2783_sdca_mbq_size(struct device *dev, u32 reg)
> > 
> >        case SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_XU22, 0x12, 0):
> >        case SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_XU22, 0x13, 0):
> > +     case PRAM_ADDR_START ... PRAM_ADDR_END:
> > +     case YRAM_ADDR_START ... YRAM_ADDR_END:
> >                return 4;
> 
> Are you sure these are MBQ registers?

Not, will remove in next version.

> 
> > @@ -794,6 +803,13 @@ static void tas2783_fw_ready(const struct firmware *fmw, void *context)
> >                        file->version, file->length,
> >                        file->dest_addr, file->fw_data);
> > 
> > +             if (tas_dev->fw_version == fw_version &&
> > +                 file->dest_addr >= PRAM_ADDR_START &&
> > +                 (file->dest_addr + file->length) <= PRAM_ADDR_END) {
> > +                     cur_file++;
> > +                     dev_dbg(tas_dev->dev, "Ignore PRAM block");
> > +                     continue;
> > +             }
> >                ret = sdw_nwrite_no_pm(tas_dev->sdw_peripheral,
> >                                       file->dest_addr,
> >                                       file->length,
> 
> This could skip blocks unintentionally if the verison happens to be 0.

It will be non-zero value after reset, and the firmware version is also non-zero value.

> 
> > +                      * failed after Power-On.
> > +                      */
> > +                     ret = sdw_nwrite_no_pm(tas_dev->sdw_peripheral,
> > +                                            file->dest_addr,
> > +                                            file->length,
> > +                                            file->fw_data);
> > +                     if (ret < 0) {
> > +                             dev_err(tas_dev->dev,
> > +                                     "FW download failed again: %d", ret);
> > +                             break;
> > +                     }
> 
> This logs an error and gives up on the download...

Yes, re-try will work for this case. If re-try still failed, should be other
fatal problem, will quit with failed.

> 
> >        if (cur_file == 0) {
> >                dev_err(tas_dev->dev, "fw with no files");
> >                ret = -EINVAL;
> >        } else {
> >                tas2783_update_calibdata(tas_dev);
> > +             ret = regmap_bulk_read(tas_dev->regmap, TAS2783_FW_VERSION,
> > +                                     &val, 4);
> > +             tas_dev->fw_version = (val[0] << 24) | (val[1] << 16) |
> > +                                    (val[2] << 8) | val[3];
> 
> ...then we do another read and overwrite the return value, potentially
> we might have a successful read.
Yes, read it with correct value after firmware downloaded. And save it for
compare in next firmware download.

> 
> > @@ -951,7 +984,7 @@ static s32 tas_sdw_hw_params(struct snd_pcm_substream *substream,
> >                                                        TAS2783_SDCA_POW_STATE_ON);
> >                        if (!ret)
> >                                break;
> > -                     usleep_range(2000, 2200);
> > +                     fsleep(2200);
> >                } while (retry--);
> >        }
> 
> This should be a separate patch.

Yes, will remove it in next version.
> 
> > @@ -962,30 +995,6 @@ static s32 tas_sdw_hw_params(struct snd_pcm_substream *substream,
> >        snd_sdw_params_to_config(substream, params,
> >                                 &stream_config, &port_config);
> > 
> > -     /*
> > -      * The two mono amps each render one channel of the stereo stream:
> > -      * snd_sdw_params_to_config() hands every codec the full mask for
> > -      * playback, which leaves the pair in mirror mode and one channel
> > -      * unreproduced.  Claim a single channel instead, keyed off the
> > -      * machine-assigned component prefix rather than the SoundWire
> > -      * address, which is board-specific: soc_sdw_ti_amp.c names the amps
> > -      * tas2783-1..4.
> > -      *
> > -      * Which side an amp then renders does not follow from the bit that
> > -      * is set - sdw_compute_slave_ports() advances the payload offset by
> > -      * the popcount of ch_mask and never looks at which bit it is - but
> > -      * from the amp's position in the codec order of the DAI link, which
> > -      * on these boards matches the prefix numbering.
> > -      */
> > -     if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
> > -         params_channels(params) == 2 && component->name_prefix) {
> > -             const char *idx_str = strrchr(component->name_prefix, '-');
> > -             unsigned long idx;
> > -
> > -             if (idx_str && !kstrtoul(idx_str + 1, 10, &idx) && idx)
> > -                     port_config.ch_mask = (idx & 1) ? BIT(0) : BIT(1);
> > -     }
> > -
> >        /* port 1 for playback */
> >        if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> >                port_config.num = 1;
> 
> This looks like a rebasing mistake?
It should be different on 4-AMPs project, I will create separate patch for those issues.
> 
> > @@ -1207,25 +1216,30 @@ static s32 tas_fw_load(struct tas2783_prv *tas_dev, struct sdw_slave *slave)
> >  static s32 tas_io_init(struct device *dev, struct sdw_slave *slave)
> >  {
> >        struct tas2783_prv *tas_dev = dev_get_drvdata(dev);
> > +     uint val;
> 
> unsigned int.
OK, will update in next version.
> 
> > +     /* Check if the AMP is in reset status. */
> > +     if (val == 0x20) {
> > +             tas_dev->fw_dl_success = false;
> > 
> 
> I'm still not seeing where fw_dl_success gets set to true if the
> download was skipped.
If download was skipped, it should be true.
> 
> > @@ -1437,6 +1454,7 @@ static void tas_sdw_remove(struct sdw_slave *peripheral)
> >        struct tas2783_prv *tas_dev = dev_get_drvdata(&peripheral->dev);
> > 
> >        pm_runtime_disable(tas_dev->dev);
> > +     regmap_write(tas_dev->regmap, TAS2783_SW_RESET, 0x1);
> >        tas_remove(tas_dev);
> >        mutex_destroy(&tas_dev->calib_lock);
> >        mutex_destroy(&tas_dev->pde_lock);
> 
> We need to make sure the regmap is not in cache only mode to do that
> reset, or make the register volatile.
Will remove it in next version.

Best Regards
Jim