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

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

[Baojun Xu]

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.

Signed-off-by: Baojun Xu <baojun.xu@ti.com>
---
v5:
- Add fw_version field for PRAM status check.
- Adjust register range from pages 1–127, 253 to pages 0–1, 253.
- Include PRAM and YRAM ranges in tas2783_sdca_mbq_size.
- Set max_register to the end of PRAM.
- Add PRAM status check before firmware download to skip reloading if PRAM
  content is retained.
- Add a retry mechanism after download failure to handle register write
  issues on initial power-up.
- Call regcache_drop_region() after firmware download to ensure correct
  firmware version reading.
- Read firmware version after download for subsequent comparison.
- Remove the "separate two monos to stereo" workaround as it causes missing
  audio on the right channel; stereo configuration should be handled by the
  DisCo table.
- Add AMP reset in the driver remove path.
- Update PRAM_ADDR_END from 0x7f to 0x80 in tas2783.h.
- Add address definition for the firmware version register in tas2783.h.
v4:
 - Since first_hw_init is only required for download reduction, remove it
   when this feature is not enabled.
v3:
 - Updated description about memory page download reduce.
 - Removed the logic that skips memory page downloads.
v2:
 - Update register address from 0x07 to 7 and keep the line within the
   80-character limit.‌
 - ‌Remove stray/unnecessary changes.‌
 - ‌Change variable type to unsigned int to comply with API requirements.‌
 - ‌Replace usleep_range() with fsleep().‌
 - ‌Set idle_bias_on to 0 to enable low-power mode.
 - Reactivate the AMP after resume.
---
 sound/soc/codecs/tas2783-sdw.c | 100 +++++++++++++++++++--------------
 sound/soc/codecs/tas2783.h     |   4 +-
 2 files changed, 62 insertions(+), 42 deletions(-)

diff --git a/sound/soc/codecs/tas2783-sdw.c b/sound/soc/codecs/tas2783-sdw.c
index 85ab3fd83c7b..1a88a15bcb02 100644
--- a/sound/soc/codecs/tas2783-sdw.c
+++ b/sound/soc/codecs/tas2783-sdw.c
@@ -97,6 +97,7 @@ struct tas2783_prv {
 	u8 rca_binaryname[64];
 	u8 dev_name[32];
 	bool hw_init;
+	unsigned int fw_version;
 	/* wq for firmware download */
 	wait_queue_head_t fw_wait;
 	bool fw_dl_task_done;
@@ -315,8 +316,8 @@ static int tas2783_sdca_mbq_size(struct device *dev, u32 reg)
 	case 0x300 ... 0x340: /* Data port 3. */
 	case 0x400 ... 0x440: /* Data port 4. */
 	case 0x500 ... 0x540: /* Data port 5. */
-	case 0x800000 ... 0x803fff: /* Page 0 ~ 127. */
-	case 0x807e80 ... 0x807eff: /* Page 253. */
+	case TASDEV_REG_SDW(0, 0, 0) ... TASDEV_REG_SDW(0x00, 0x01, 0x80):
+	case TASDEV_REG_SDW(0, 0xfd, 0) ... TASDEV_REG_SDW(0, 0xfd, 0x80):
 	case SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_UDMPU23,
 			  TAS2783_SDCA_CTL_UDMPU_CLUSTER, 0):
 	case SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_FU21, TAS2783_SDCA_CTL_FU_MUTE,
@@ -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;
 
 	default:
@@ -517,7 +520,7 @@ static const struct regmap_config tas_regmap = {
 	.volatile_reg = tas2783_volatile_register,
 	.reg_defaults = tas2783_reg_default,
 	.num_reg_defaults = ARRAY_SIZE(tas2783_reg_default),
-	.max_register = 0x41008000 + TASDEV_REG_SDW(0xa1, 0x60, 0x7f),
+	.max_register = 0x41000000 + PRAM_ADDR_END,
 	.cache_type = REGCACHE_MAPLE,
 	.use_single_read = true,
 	.use_single_write = true,
@@ -745,6 +748,7 @@ static void tas2783_fw_ready(const struct firmware *fmw, void *context)
 	const u8 *buf = NULL;
 	s32  img_sz, ret = 0, cur_file = 0;
 	s32 offset = 0;
+	u32 val[4], fw_version;
 
 	struct tas_fw_hdr *hdr __free(kfree) = kzalloc_obj(*hdr);
 	struct tas_fw_file *file __free(kfree) = kzalloc_obj(*file);
@@ -786,6 +790,11 @@ static void tas2783_fw_ready(const struct firmware *fmw, void *context)
 	}
 
 	mutex_lock(&tas_dev->pde_lock);
+	ret = regmap_bulk_read(tas_dev->regmap, TAS2783_FW_VERSION, &val, 4);
+	fw_version = (val[0] << 24) | (val[1] << 16) | (val[2] << 8) | val[3];
+	dev_dbg(tas_dev->dev, "Get Firmware version: %08x == %08x?, err=%d",
+		fw_version, tas_dev->fw_version, ret);
+
 	while (offset < (img_sz - FW_FL_HDR)) {
 		offset += tas_fw_get_next_file(&buf[offset], file);
 		dev_dbg(tas_dev->dev,
@@ -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,
@@ -801,17 +817,34 @@ static void tas2783_fw_ready(const struct firmware *fmw, void *context)
 		if (ret < 0) {
 			dev_err(tas_dev->dev,
 				"FW download failed: %d", ret);
-			break;
+			/*
+			 * We do retry here for some special case of download
+			 * 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;
+			}
 		}
 		cur_file++;
 	}
 	mutex_unlock(&tas_dev->pde_lock);
+	regcache_drop_region(tas_dev->regmap, 0, UINT_MAX);
 
 	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];
 	}
 
 out:
@@ -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--);
 	}
 
@@ -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;
@@ -1074,7 +1083,7 @@ static const struct snd_soc_component_driver soc_codec_driver_tasdevice = {
 	.num_dapm_widgets = ARRAY_SIZE(tas_dapm_widgets),
 	.dapm_routes = tas_audio_map,
 	.num_dapm_routes = ARRAY_SIZE(tas_audio_map),
-	.idle_bias_on = 1,
+	.idle_bias_on = 0,
 	.endianness = 1,
 };
 
@@ -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;
 	s32 ret;
 
 	if (tas_dev->hw_init)
 		return 0;
 
-	tas_dev->fw_dl_success = false;
+	ret = regmap_read(tas_dev->regmap, TASDEV_REG_SDW(0, 0, 7), &val);
 
-	ret = regmap_write(tas_dev->regmap, TAS2783_SW_RESET, 0x1);
-	if (ret) {
-		dev_err(dev, "sw reset failed, err=%d", ret);
-		return ret;
-	}
-	usleep_range(2000, 2200);
+	/* Check if the AMP is in reset status. */
+	if (val == 0x20) {
+		tas_dev->fw_dl_success = false;
 
-	tas_dev->fw_use_fallback = false;
-	ret = tas_fw_load(tas_dev, slave);
-	if (!ret && tas_dev->fw_use_fallback)
-		ret = tas_fw_load(tas_dev, slave);
+		ret = regmap_write(tas_dev->regmap, TAS2783_SW_RESET, 0x1);
+		if (ret) {
+			dev_err(dev, "sw reset failed, err=%d", ret);
+			return ret;
+		}
+		fsleep(2200);
 
+		tas_dev->fw_use_fallback = false;
+		ret = tas_fw_load(tas_dev, slave);
+		if (!ret && tas_dev->fw_use_fallback)
+			ret = tas_fw_load(tas_dev, slave);
+	}
 	if (!ret) {
 		if (tas_dev->sa_func_data)
 			ret = sdca_regmap_write_init(dev, tas_dev->regmap,
@@ -1234,6 +1248,8 @@ static s32 tas_io_init(struct device *dev, struct sdw_slave *slave)
 			ret = regmap_multi_reg_write(tas_dev->regmap, tas2783_init_seq,
 						     ARRAY_SIZE(tas2783_init_seq));
 
+		/* Re-active AMP after resume. */
+		regmap_write(tas_dev->regmap, TASDEV_REG_SDW(0, 0, 2), 0);
 		if (ret)
 			dev_err(tas_dev->dev,
 				"init writes failed, err=%d", ret);
@@ -1413,6 +1429,7 @@ static s32 tas_sdw_probe(struct sdw_slave *peripheral,
 	tas_dev->dev = dev;
 	tas_dev->sdw_peripheral = peripheral;
 	tas_dev->hw_init = false;
+	tas_dev->fw_version = 0;
 	mutex_init(&tas_dev->calib_lock);
 	mutex_init(&tas_dev->pde_lock);
 
@@ -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);
diff --git a/sound/soc/codecs/tas2783.h b/sound/soc/codecs/tas2783.h
index d5996c73526c..2f034617e356 100644
--- a/sound/soc/codecs/tas2783.h
+++ b/sound/soc/codecs/tas2783.h
@@ -35,10 +35,12 @@
 #define TAS2783_AMP_LEVEL_MASK	GENMASK(5, 1)
 
 #define PRAM_ADDR_START		TASDEV_REG_SDW(0x8c, 0x01, 0x8)
-#define PRAM_ADDR_END		TASDEV_REG_SDW(0x8c, 0xff, 0x7f)
+#define PRAM_ADDR_END		TASDEV_REG_SDW(0x8c, 0xff, 0x80)
 #define YRAM_ADDR_START		TASDEV_REG_SDW(0x00, 0x02, 0x8)
 #define YRAM_ADDR_END		TASDEV_REG_SDW(0x00, 0x37, 0x7f)
 
+#define TAS2783_FW_VERSION	TASDEV_REG_SDW(0x00, 0x20, 0x3c)
+
 /* Calibration data */
 #define TAS2783_CAL_R0		TASDEV_REG_SDW(0, 0x16, 0x4C)
 #define TAS2783_CAL_INVR0	TASDEV_REG_SDW(0, 0x16, 0x5C)
-- 
2.25.1

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

[Mark Brown]

[-- 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 --]

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

[Xu, Baojun]

Hi,

> From: Mark Brown <broonie@kernel.org>
> Sent: 21 September 2026 16:45
> To: Xu, Baojun
> Cc: tiwai@suse.de; andriy.shevchenko@linux.intel.com; 13916275206@139.com; alsa-devel@alsa-project.org; Ding, Shenghao; linux-sound@vger.kernel.org; linux-kernel@vger.kernel.org; Yi, Ken; Lo, Henry; Chen, Robin; Holalu Yogendra, Niranjan; pin-hao.huang@hp.com; Syed.SabaKareem@amd.com
> 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

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

[Mark Brown]

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

On Mon, Sep 21, 2026 at 09:22:23AM +0000, Xu, Baojun wrote:

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

As I said on the previous version can you guarantee that the driver is
being probed with the device having been reset?  What happens if
something earlier in the boot process left it running, or the driver was
removed and reprobed?  The logic only works currently if the driver did
the download that it's skipping.

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

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

[Xu, Baojun]

> 
> ________________________________________
> From: Mark Brown <broonie@kernel.org>
> Sent: 21 September 2026 17:27
> To: Xu, Baojun
> Cc: tiwai@suse.de; andriy.shevchenko@linux.intel.com; 13916275206@139.com; alsa-devel@alsa-project.org; Ding, Shenghao; linux-sound@vger.kernel.org; linux-kernel@vger.kernel.org; Yi, Ken; Lo, Henry; Chen, Robin; Holalu Yogendra, Niranjan; pin-hao.huang@hp.com; Syed.SabaKareem@amd.com
> Subject: Re: [EXTERNAL] Re: [PATCH v5] ASoC: tas2783-sdw: add firmware download status check
> 
> On Mon, Sep 21, 2026 at 09:22:23AM +0000, Xu, Baojun wrote:
> 
> > > > +     /* 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.
> 
> As I said on the previous version can you guarantee that the driver is
> being probed with the device having been reset?  What happens if
> something earlier in the boot process left it running, or the driver was
> removed and reprobed?  The logic only works currently if the driver did
> the download that it's skipping.
> 
Previously, a local variable tracked the AMP reset state to determine if a
firmware download was required. However, we observed that the AMP resets
during SoundWire bus stop/start sequences, causing the variable to become
stale. To improve stability, the driver now directly reads the register
value instead of relying on the cached variable.
This version has been verified to work correctly on the current platform.
Given the customer's urgent need, could you please help apply this patch?
This will allow the customer to proceed with further testing immediately.
Version 6 will be released shortly.

Best Regards
Jim

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

[Mark Brown]

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

On Mon, Sep 21, 2026 at 10:08:54AM +0000, Xu, Baojun wrote:

> Previously, a local variable tracked the AMP reset state to determine if a
> firmware download was required. However, we observed that the AMP resets
> during SoundWire bus stop/start sequences, causing the variable to become
> stale. To improve stability, the driver now directly reads the register
> value instead of relying on the cached variable.

It should just need an assignment to set the download flag when you
decide to skip?

> This version has been verified to work correctly on the current platform.

Are you sure there are no users with other systems who might be broken
by skipping the download?

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