ASoC: Intel: avs: Fixes and cleanups

broonie · broonie · commit fd688ce125a4 · 2025-01-09T15:55:24.000Z
Merge series from Cezary Rojewski &lt;cezary.rojewski@intel.com&gt;:

A set of loosely connected changes, fixing few outstanding issues as
well as improving readability of the existing code.

The fixes lead the series, first five patches. The goal is to make sure
proper read() is used when accessing the registers, probe() and remove()
sequences for HDAudio streaming are synced, minimal AudioDSP firmware
version points to correct values and recent additions to the topology
are parsed properly.

The only patch that points to 'new functionality' is:
  ASoC: Intel: avs: Update ASRC definition

as with the struct definition updates, one can utilize the ASRC module
in both streaming directions now (previously limited to Capture).
Everything else either improves the logging or provides comments vital
for long-term maintenance of the code.
diff --git a/sound/soc/intel/avs/apl.c b/sound/soc/intel/avs/apl.c
@@ -125,7 +125,7 @@ int avs_apl_coredump(struct avs_dev *adev, union avs_notify_msg *msg)
 	struct avs_apl_log_buffer_layout layout;
 	void __iomem *addr, *buf;
 	size_t dump_size;
-	u16 offset = 0;
+	u32 offset = 0;
 	u8 *dump, *pos;
 
 	dump_size = AVS_FW_REGS_SIZE + msg->ext.coredump.stack_dump_size;
diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c
@@ -829,22 +829,22 @@ static const struct avs_spec jsl_desc = {
 	.hipc = &cnl_hipc_spec,
 };
 
-#define AVS_TGL_BASED_SPEC(sname)		\
+#define AVS_TGL_BASED_SPEC(sname, min)		\
 static const struct avs_spec sname##_desc = {	\
 	.name = #sname,				\
-	.min_fw_version = { 10,	29, 0, 5646 },	\
+	.min_fw_version = { 10,	min, 0, 5646 },	\
 	.dsp_ops = &avs_tgl_dsp_ops,		\
 	.core_init_mask = 1,			\
 	.attributes = AVS_PLATATTR_IMR,		\
 	.sram = &apl_sram_spec,			\
 	.hipc = &cnl_hipc_spec,			\
 }
 
-AVS_TGL_BASED_SPEC(lkf);
-AVS_TGL_BASED_SPEC(tgl);
-AVS_TGL_BASED_SPEC(ehl);
-AVS_TGL_BASED_SPEC(adl);
-AVS_TGL_BASED_SPEC(adl_n);
+AVS_TGL_BASED_SPEC(lkf, 28);
+AVS_TGL_BASED_SPEC(tgl, 29);
+AVS_TGL_BASED_SPEC(ehl, 30);
+AVS_TGL_BASED_SPEC(adl, 35);
+AVS_TGL_BASED_SPEC(adl_n, 35);
 
 static const struct pci_device_id avs_ids[] = {
 	{ PCI_DEVICE_DATA(INTEL, HDA_SKL_LP, &skl_desc) },
@@ -902,3 +902,13 @@ MODULE_AUTHOR("Cezary Rojewski <cezary.rojewski@intel.com>");
 MODULE_AUTHOR("Amadeusz Slawinski <amadeuszx.slawinski@linux.intel.com>");
 MODULE_DESCRIPTION("Intel cAVS sound driver");
 MODULE_LICENSE("GPL");
+MODULE_FIRMWARE("intel/skl/dsp_basefw.bin");
+MODULE_FIRMWARE("intel/apl/dsp_basefw.bin");
+MODULE_FIRMWARE("intel/cnl/dsp_basefw.bin");
+MODULE_FIRMWARE("intel/icl/dsp_basefw.bin");
+MODULE_FIRMWARE("intel/jsl/dsp_basefw.bin");
+MODULE_FIRMWARE("intel/lkf/dsp_basefw.bin");
+MODULE_FIRMWARE("intel/tgl/dsp_basefw.bin");
+MODULE_FIRMWARE("intel/ehl/dsp_basefw.bin");
+MODULE_FIRMWARE("intel/adl/dsp_basefw.bin");
+MODULE_FIRMWARE("intel/adl_n/dsp_basefw.bin");
diff --git a/sound/soc/intel/avs/debugfs.c b/sound/soc/intel/avs/debugfs.c
@@ -10,6 +10,7 @@
 #include <linux/kfifo.h>
 #include <linux/wait.h>
 #include <linux/sched/signal.h>
+#include <linux/string_helpers.h>
 #include <sound/soc.h>
 #include "avs.h"
 #include "messages.h"
diff --git a/sound/soc/intel/avs/ipc.c b/sound/soc/intel/avs/ipc.c
@@ -184,10 +184,11 @@ static void avs_dsp_receive_rx(struct avs_dev *adev, u64 header)
 {
 	struct avs_ipc *ipc = adev->ipc;
 	union avs_reply_msg msg = AVS_MSG(header);
-	u64 reg;
+	u32 sts, lec;
 
-	reg = readq(avs_sram_addr(adev, AVS_FW_REGS_WINDOW));
-	trace_avs_ipc_reply_msg(header, reg);
+	sts = snd_hdac_adsp_readl(adev, AVS_FW_REG_STATUS(adev));
+	lec = snd_hdac_adsp_readl(adev, AVS_FW_REG_ERROR(adev));
+	trace_avs_ipc_reply_msg(header, sts, lec);
 
 	ipc->rx.header = header;
 	/* Abort copying payload if request processing was unsuccessful. */
@@ -209,10 +210,11 @@ static void avs_dsp_process_notification(struct avs_dev *adev, u64 header)
 	union avs_notify_msg msg = AVS_MSG(header);
 	size_t data_size = 0;
 	void *data = NULL;
-	u64 reg;
+	u32 sts, lec;
 
-	reg = readq(avs_sram_addr(adev, AVS_FW_REGS_WINDOW));
-	trace_avs_ipc_notify_msg(header, reg);
+	sts = snd_hdac_adsp_readl(adev, AVS_FW_REG_STATUS(adev));
+	lec = snd_hdac_adsp_readl(adev, AVS_FW_REG_ERROR(adev));
+	trace_avs_ipc_notify_msg(header, sts, lec);
 
 	/* Ignore spurious notifications until handshake is established. */
 	if (!adev->ipc->ready && msg.notify_msg_type != AVS_NOTIFY_FW_READY) {
@@ -367,13 +369,16 @@ static void avs_ipc_msg_init(struct avs_ipc *ipc, struct avs_ipc_msg *reply)
 static void avs_dsp_send_tx(struct avs_dev *adev, struct avs_ipc_msg *tx, bool read_fwregs)
 {
 	const struct avs_spec *const spec = adev->spec;
-	u64 reg = ULONG_MAX;
+	u32 sts = UINT_MAX;
+	u32 lec = UINT_MAX;
 
 	tx->header |= spec->hipc->req_busy_mask;
-	if (read_fwregs)
-		reg = readq(avs_sram_addr(adev, AVS_FW_REGS_WINDOW));
+	if (read_fwregs) {
+		sts = snd_hdac_adsp_readl(adev, AVS_FW_REG_STATUS(adev));
+		lec = snd_hdac_adsp_readl(adev, AVS_FW_REG_ERROR(adev));
+	}
 
-	trace_avs_request(tx, reg);
+	trace_avs_request(tx, sts, lec);
 
 	if (tx->size)
 		memcpy_toio(avs_downlink_addr(adev), tx->data, tx->size);
diff --git a/sound/soc/intel/avs/loader.c b/sound/soc/intel/avs/loader.c
@@ -167,7 +167,8 @@ int avs_cldma_load_basefw(struct avs_dev *adev, struct firmware *fw)
 				       (reg & AVS_ROM_INIT_DONE) == AVS_ROM_INIT_DONE,
 				       AVS_ROM_INIT_POLLING_US, SKL_ROM_INIT_TIMEOUT_US);
 	if (ret < 0) {
-		dev_err(adev->dev, "rom init timeout: %d\n", ret);
+		dev_err(adev->dev, "rom init failed: %d, status: 0x%08x, lec: 0x%08x\n",
+			ret, reg, snd_hdac_adsp_readl(adev, AVS_FW_REG_ERROR(adev)));
 		avs_dsp_core_disable(adev, AVS_MAIN_CORE_MASK);
 		return ret;
 	}
@@ -180,7 +181,8 @@ int avs_cldma_load_basefw(struct avs_dev *adev, struct firmware *fw)
 				       AVS_FW_INIT_POLLING_US, AVS_FW_INIT_TIMEOUT_US);
 	hda_cldma_stop(cl);
 	if (ret < 0) {
-		dev_err(adev->dev, "transfer fw failed: %d\n", ret);
+		dev_err(adev->dev, "transfer fw failed: %d, status: 0x%08x, lec: 0x%08x\n",
+			ret, reg, snd_hdac_adsp_readl(adev, AVS_FW_REG_ERROR(adev)));
 		avs_dsp_core_disable(adev, AVS_MAIN_CORE_MASK);
 		return ret;
 	}
@@ -308,12 +310,13 @@ avs_hda_init_rom(struct avs_dev *adev, unsigned int dma_id, bool purge)
 	}
 
 	/* await ROM init */
-	ret = snd_hdac_adsp_readq_poll(adev, spec->sram->rom_status_offset, reg,
+	ret = snd_hdac_adsp_readl_poll(adev, spec->sram->rom_status_offset, reg,
 				       (reg & 0xF) == AVS_ROM_INIT_DONE ||
 				       (reg & 0xF) == APL_ROM_FW_ENTERED,
 				       AVS_ROM_INIT_POLLING_US, APL_ROM_INIT_TIMEOUT_US);
 	if (ret < 0) {
-		dev_err(adev->dev, "rom init timeout: %d\n", ret);
+		dev_err(adev->dev, "rom init failed: %d, status: 0x%08x, lec: 0x%08x\n",
+			ret, reg, snd_hdac_adsp_readl(adev, AVS_FW_REG_ERROR(adev)));
 		goto err;
 	}
 
@@ -337,15 +340,15 @@ static int avs_imr_load_basefw(struct avs_dev *adev)
 
 	/* DMA id ignored when flashing from IMR as no transfer occurs. */
 	ret = avs_hda_init_rom(adev, 0, false);
-	if (ret < 0) {
-		dev_err(adev->dev, "rom init failed: %d\n", ret);
+	if (ret < 0)
 		return ret;
-	}
 
 	ret = wait_for_completion_timeout(&adev->fw_ready,
 					  msecs_to_jiffies(AVS_FW_INIT_TIMEOUT_MS));
 	if (!ret) {
-		dev_err(adev->dev, "firmware ready timeout\n");
+		dev_err(adev->dev, "firmware ready timeout, status: 0x%08x, lec: 0x%08x\n",
+			snd_hdac_adsp_readl(adev, AVS_FW_REG_STATUS(adev)),
+			snd_hdac_adsp_readl(adev, AVS_FW_REG_ERROR(adev)));
 		avs_dsp_core_disable(adev, AVS_MAIN_CORE_MASK);
 		return -ETIMEDOUT;
 	}
@@ -392,7 +395,7 @@ int avs_hda_load_basefw(struct avs_dev *adev, struct firmware *fw)
 		ret = avs_hda_init_rom(adev, dma_id, true);
 		if (!ret)
 			break;
-		dev_info(adev->dev, "#%d rom init fail: %d\n", i + 1, ret);
+		dev_info(adev->dev, "#%d rom init failed: %d\n", i + 1, ret);
 	}
 	if (ret < 0)
 		goto cleanup_resources;
@@ -404,7 +407,8 @@ int avs_hda_load_basefw(struct avs_dev *adev, struct firmware *fw)
 				       AVS_FW_INIT_POLLING_US, AVS_FW_INIT_TIMEOUT_US);
 	snd_hdac_dsp_trigger(hstream, false);
 	if (ret < 0) {
-		dev_err(adev->dev, "transfer fw failed: %d\n", ret);
+		dev_err(adev->dev, "transfer fw failed: %d, status: 0x%08x, lec: 0x%08x\n",
+			ret, reg, snd_hdac_adsp_readl(adev, AVS_FW_REG_ERROR(adev)));
 		avs_dsp_core_disable(adev, AVS_MAIN_CORE_MASK);
 	}
 
@@ -584,7 +588,9 @@ static int avs_dsp_load_basefw(struct avs_dev *adev)
 	ret = wait_for_completion_timeout(&adev->fw_ready,
 					  msecs_to_jiffies(AVS_FW_INIT_TIMEOUT_MS));
 	if (!ret) {
-		dev_err(adev->dev, "firmware ready timeout\n");
+		dev_err(adev->dev, "firmware ready timeout, status: 0x%08x, lec: 0x%08x\n",
+			snd_hdac_adsp_readl(adev, AVS_FW_REG_STATUS(adev)),
+			snd_hdac_adsp_readl(adev, AVS_FW_REG_ERROR(adev)));
 		avs_dsp_core_disable(adev, AVS_MAIN_CORE_MASK);
 		ret = -ETIMEDOUT;
 		goto release_fw;
@@ -675,16 +681,12 @@ int avs_dsp_first_boot_firmware(struct avs_dev *adev)
 	}
 
 	ret = avs_ipc_get_hw_config(adev, &adev->hw_cfg);
-	if (ret) {
-		dev_err(adev->dev, "get hw cfg failed: %d\n", ret);
+	if (ret)
 		return AVS_IPC_RET(ret);
-	}
 
 	ret = avs_ipc_get_fw_config(adev, &adev->fw_cfg);
-	if (ret) {
-		dev_err(adev->dev, "get fw cfg failed: %d\n", ret);
+	if (ret)
 		return AVS_IPC_RET(ret);
-	}
 
 	adev->core_refs = devm_kcalloc(adev->dev, adev->hw_cfg.dsp_cores,
 				       sizeof(*adev->core_refs), GFP_KERNEL);
diff --git a/sound/soc/intel/avs/messages.c b/sound/soc/intel/avs/messages.c
@@ -400,10 +400,12 @@ int avs_ipc_get_fw_config(struct avs_dev *adev, struct avs_fw_cfg *cfg)
 				       AVS_BASEFW_FIRMWARE_CONFIG, NULL, 0,
 				       &payload, &payload_size);
 	if (ret)
-		return ret;
+		goto err;
 	/* Non-zero payload expected for FIRMWARE_CONFIG. */
-	if (!payload_size)
-		return -EREMOTEIO;
+	if (!payload_size) {
+		ret = -EREMOTEIO;
+		goto err;
+	}
 
 	while (offset < payload_size) {
 		tlv = (struct avs_tlv *)(payload + offset);
@@ -502,6 +504,9 @@ int avs_ipc_get_fw_config(struct avs_dev *adev, struct avs_fw_cfg *cfg)
 
 	/* No longer needed, free it as it's owned by the get_large_config() caller. */
 	kfree(payload);
+err:
+	if (ret)
+		dev_err(adev->dev, "get fw cfg failed: %d\n", ret);
 	return ret;
 }
 
@@ -517,10 +522,12 @@ int avs_ipc_get_hw_config(struct avs_dev *adev, struct avs_hw_cfg *cfg)
 				       AVS_BASEFW_HARDWARE_CONFIG, NULL, 0,
 				       &payload, &payload_size);
 	if (ret)
-		return ret;
+		goto err;
 	/* Non-zero payload expected for HARDWARE_CONFIG. */
-	if (!payload_size)
-		return -EREMOTEIO;
+	if (!payload_size) {
+		ret = -EREMOTEIO;
+		goto err;
+	}
 
 	while (offset < payload_size) {
 		tlv = (struct avs_tlv *)(payload + offset);
@@ -590,6 +597,9 @@ int avs_ipc_get_hw_config(struct avs_dev *adev, struct avs_hw_cfg *cfg)
 exit:
 	/* No longer needed, free it as it's owned by the get_large_config() caller. */
 	kfree(payload);
+err:
+	if (ret)
+		dev_err(adev->dev, "get hw cfg failed: %d\n", ret);
 	return ret;
 }
 
diff --git a/sound/soc/intel/avs/messages.h b/sound/soc/intel/avs/messages.h
@@ -859,8 +859,7 @@ static_assert(sizeof(struct avs_aec_cfg) == 92);
 struct avs_asrc_cfg {
 	struct avs_modcfg_base base;
 	u32 out_freq;
-	u32 rsvd0:1;
-	u32 mode:1;
+	u32 mode:2;
 	u32 rsvd2:2;
 	u32 disable_jitter_buffer:1;
 	u32 rsvd3:27;
diff --git a/sound/soc/intel/avs/pcm.c b/sound/soc/intel/avs/pcm.c
@@ -161,6 +161,7 @@ static int avs_dai_be_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_dpcm *dpcm;
 
 	be = snd_soc_substream_to_rtd(substream);
+	/* dpcm_fe_dai_open() guarantees the list is not empty at this point. */
 	for_each_dpcm_fe(be, substream->stream, dpcm) {
 		fe = dpcm->fe;
 		fe_hw_params = &fe->dpcm[substream->stream].hw_params;
@@ -576,6 +577,7 @@ static int avs_dai_fe_hw_params(struct snd_pcm_substream *substream,
 	hdac_stream(host_stream)->format_val = 0;
 
 	fe = snd_soc_substream_to_rtd(substream);
+	/* dpcm_fe_dai_open() guarantees the list is not empty at this point. */
 	for_each_dpcm_be(fe, substream->stream, dpcm) {
 		be = dpcm->be;
 		be_hw_params = &be->dpcm[substream->stream].hw_params;
@@ -1564,6 +1566,7 @@ static int avs_component_hda_probe(struct snd_soc_component *component)
 		if (ret < 0) {
 			dev_err(component->dev, "create widgets failed: %d\n",
 				ret);
+			snd_soc_unregister_dai(dai);
 			goto exit;
 		}
 	}
@@ -1578,8 +1581,8 @@ static int avs_component_hda_probe(struct snd_soc_component *component)
 
 static void avs_component_hda_remove(struct snd_soc_component *component)
 {
-	avs_component_hda_unregister_dais(component);
 	avs_component_remove(component);
+	avs_component_hda_unregister_dais(component);
 }
 
 static int avs_component_hda_open(struct snd_soc_component *component,
diff --git a/sound/soc/intel/avs/registers.h b/sound/soc/intel/avs/registers.h
@@ -74,7 +74,7 @@
 /* Constants used when accessing SRAM, space shared with firmware */
 #define AVS_FW_REG_BASE(adev)		((adev)->spec->sram->base_offset)
 #define AVS_FW_REG_STATUS(adev)		(AVS_FW_REG_BASE(adev) + 0x0)
-#define AVS_FW_REG_ERROR_CODE(adev)	(AVS_FW_REG_BASE(adev) + 0x4)
+#define AVS_FW_REG_ERROR(adev)		(AVS_FW_REG_BASE(adev) + 0x4)
 
 #define AVS_WINDOW_CHUNK_SIZE		SZ_4K
 #define AVS_FW_REGS_SIZE		AVS_WINDOW_CHUNK_SIZE
diff --git a/sound/soc/intel/avs/topology.c b/sound/soc/intel/avs/topology.c
@@ -1466,7 +1466,7 @@ avs_tplg_path_template_create(struct snd_soc_component *comp, struct avs_tplg *o
 
 static const struct avs_tplg_token_parser mod_init_config_parsers[] = {
 	{
-		.token = AVS_TKN_MOD_INIT_CONFIG_ID_U32,
+		.token = AVS_TKN_INIT_CONFIG_ID_U32,
 		.type = SND_SOC_TPLG_TUPLE_TYPE_WORD,
 		.offset = offsetof(struct avs_tplg_init_config, id),
 		.parse = avs_parse_word_token,
@@ -1519,7 +1519,7 @@ static int avs_tplg_parse_initial_configs(struct snd_soc_component *comp,
 		esize = le32_to_cpu(tuples->size) + le32_to_cpu(tmp->size);
 
 		ret = parse_dictionary_entries(comp, tuples, esize, config, 1, sizeof(*config),
-					       AVS_TKN_MOD_INIT_CONFIG_ID_U32,
+					       AVS_TKN_INIT_CONFIG_ID_U32,
 					       mod_init_config_parsers,
 					       ARRAY_SIZE(mod_init_config_parsers));
 
diff --git a/sound/soc/intel/avs/trace.h b/sound/soc/intel/avs/trace.h
