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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5006 bytes --]

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?

> @@ -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.

> +			 * 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...

>  	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.

> @@ -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.

> @@ -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?

> @@ -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.

> +	/* 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.

> @@ -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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]