[PATCH 0/6] ASoC: SOF: ipc4/Intel: Improve and enable IPC error dump
Hi,
On Intel platforms the registers for DSP communications are used differently, the IPC dump information is not correct since important registers are not printed and existing ones are used a bit differently for IPC4.
As a last step, enable the IPC timeout 'handling' and allow the printout of the now usefull IPC dump.
Regards, Peter --- Peter Ujfalusi (6): ASoC: SOF: Intel: cnl: Add separate ops for ipc_dump for IPC4 ASoC: SOF: Intel: hda: Add separate ops for ipc_dump for IPC4 ASoC: SOF: Intel: skl: Use the ipc4 version of the ipc_dump ASoC: SOF: Intel: mtl: Print relevant register in ipc_dump ASoC: SOF: Intel: hda: Only dump firmware registers for IPC3 ASoC: SOF: ipc4: Call snd_sof_handle_fw_exception() in case of timeout
sound/soc/sof/intel/apl.c | 7 ++++++- sound/soc/sof/intel/cnl.c | 28 +++++++++++++++++++++++++++- sound/soc/sof/intel/hda-ipc.h | 1 + sound/soc/sof/intel/hda.c | 21 ++++++++++++++++++++- sound/soc/sof/intel/hda.h | 1 + sound/soc/sof/intel/icl.c | 7 ++++++- sound/soc/sof/intel/mtl.c | 17 ++++++++--------- sound/soc/sof/intel/skl.c | 2 +- sound/soc/sof/intel/tgl.c | 7 ++++++- sound/soc/sof/ipc4.c | 1 + 10 files changed, 77 insertions(+), 15 deletions(-)
The use of the IPC registers are different between IPC3 and IPC4. The ipc_dump needs to use different prints depending on the used IPC protocol.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com --- sound/soc/sof/intel/cnl.c | 28 +++++++++++++++++++++++++++- sound/soc/sof/intel/hda-ipc.h | 1 + sound/soc/sof/intel/icl.c | 7 ++++++- sound/soc/sof/intel/tgl.c | 7 ++++++- 4 files changed, 40 insertions(+), 3 deletions(-)
diff --git a/sound/soc/sof/intel/cnl.c b/sound/soc/sof/intel/cnl.c index 180001d0a38a..1e71f6dc604e 100644 --- a/sound/soc/sof/intel/cnl.c +++ b/sound/soc/sof/intel/cnl.c @@ -332,6 +332,27 @@ void cnl_ipc_dump(struct snd_sof_dev *sdev) hipcida, hipctdr, hipcctl); }
+void cnl_ipc4_dump(struct snd_sof_dev *sdev) +{ + u32 hipcidr, hipcidd, hipcida, hipctdr, hipctdd, hipctda, hipcctl; + + hda_ipc_irq_dump(sdev); + + hipcidr = snd_sof_dsp_read(sdev, HDA_DSP_BAR, CNL_DSP_REG_HIPCIDR); + hipcidd = snd_sof_dsp_read(sdev, HDA_DSP_BAR, CNL_DSP_REG_HIPCIDD); + hipcida = snd_sof_dsp_read(sdev, HDA_DSP_BAR, CNL_DSP_REG_HIPCIDA); + hipctdr = snd_sof_dsp_read(sdev, HDA_DSP_BAR, CNL_DSP_REG_HIPCTDR); + hipctdd = snd_sof_dsp_read(sdev, HDA_DSP_BAR, CNL_DSP_REG_HIPCTDD); + hipctda = snd_sof_dsp_read(sdev, HDA_DSP_BAR, CNL_DSP_REG_HIPCTDA); + hipcctl = snd_sof_dsp_read(sdev, HDA_DSP_BAR, CNL_DSP_REG_HIPCCTL); + + /* dump the IPC regs */ + /* TODO: parse the raw msg */ + dev_err(sdev->dev, + "Host IPC initiator: %#x|%#x|%#x, target: %#x|%#x|%#x, ctl: %#x\n", + hipcidr, hipcidd, hipcida, hipctdr, hipctdd, hipctda, hipcctl); +} + /* cannonlake ops */ struct snd_sof_dsp_ops sof_cnl_ops; EXPORT_SYMBOL_NS(sof_cnl_ops, SND_SOC_SOF_INTEL_HDA_COMMON); @@ -351,6 +372,9 @@ int sof_cnl_ops_init(struct snd_sof_dev *sdev)
/* ipc */ sof_cnl_ops.send_msg = cnl_ipc_send_msg; + + /* debug */ + sof_cnl_ops.ipc_dump = cnl_ipc_dump; }
if (sdev->pdata->ipc_type == SOF_INTEL_IPC4) { @@ -370,6 +394,9 @@ int sof_cnl_ops_init(struct snd_sof_dev *sdev)
/* ipc */ sof_cnl_ops.send_msg = cnl_ipc4_send_msg; + + /* debug */ + sof_cnl_ops.ipc_dump = cnl_ipc4_dump; }
/* set DAI driver ops */ @@ -378,7 +405,6 @@ int sof_cnl_ops_init(struct snd_sof_dev *sdev) /* debug */ sof_cnl_ops.debug_map = cnl_dsp_debugfs; sof_cnl_ops.debug_map_count = ARRAY_SIZE(cnl_dsp_debugfs); - sof_cnl_ops.ipc_dump = cnl_ipc_dump;
/* pre/post fw run */ sof_cnl_ops.post_fw_run = hda_dsp_post_fw_run; diff --git a/sound/soc/sof/intel/hda-ipc.h b/sound/soc/sof/intel/hda-ipc.h index 10fbca5939db..8ec5e9f6f8d7 100644 --- a/sound/soc/sof/intel/hda-ipc.h +++ b/sound/soc/sof/intel/hda-ipc.h @@ -51,5 +51,6 @@ irqreturn_t cnl_ipc_irq_thread(int irq, void *context); int cnl_ipc_send_msg(struct snd_sof_dev *sdev, struct snd_sof_ipc_msg *msg); void cnl_ipc_dump(struct snd_sof_dev *sdev); +void cnl_ipc4_dump(struct snd_sof_dev *sdev);
#endif diff --git a/sound/soc/sof/intel/icl.c b/sound/soc/sof/intel/icl.c index 59ce3132fada..f1018c6db5c2 100644 --- a/sound/soc/sof/intel/icl.c +++ b/sound/soc/sof/intel/icl.c @@ -113,6 +113,9 @@ int sof_icl_ops_init(struct snd_sof_dev *sdev)
/* ipc */ sof_icl_ops.send_msg = cnl_ipc_send_msg; + + /* debug */ + sof_icl_ops.ipc_dump = cnl_ipc_dump; }
if (sdev->pdata->ipc_type == SOF_INTEL_IPC4) { @@ -132,12 +135,14 @@ int sof_icl_ops_init(struct snd_sof_dev *sdev)
/* ipc */ sof_icl_ops.send_msg = cnl_ipc4_send_msg; + + /* debug */ + sof_icl_ops.ipc_dump = cnl_ipc4_dump; }
/* debug */ sof_icl_ops.debug_map = icl_dsp_debugfs; sof_icl_ops.debug_map_count = ARRAY_SIZE(icl_dsp_debugfs); - sof_icl_ops.ipc_dump = cnl_ipc_dump;
/* pre/post fw run */ sof_icl_ops.post_fw_run = icl_dsp_post_fw_run; diff --git a/sound/soc/sof/intel/tgl.c b/sound/soc/sof/intel/tgl.c index 5135e1c7e6cf..c606c3691de7 100644 --- a/sound/soc/sof/intel/tgl.c +++ b/sound/soc/sof/intel/tgl.c @@ -68,6 +68,9 @@ int sof_tgl_ops_init(struct snd_sof_dev *sdev)
/* ipc */ sof_tgl_ops.send_msg = cnl_ipc_send_msg; + + /* debug */ + sof_tgl_ops.ipc_dump = cnl_ipc_dump; }
if (sdev->pdata->ipc_type == SOF_INTEL_IPC4) { @@ -87,6 +90,9 @@ int sof_tgl_ops_init(struct snd_sof_dev *sdev)
/* ipc */ sof_tgl_ops.send_msg = cnl_ipc4_send_msg; + + /* debug */ + sof_tgl_ops.ipc_dump = cnl_ipc4_dump; }
/* set DAI driver ops */ @@ -95,7 +101,6 @@ int sof_tgl_ops_init(struct snd_sof_dev *sdev) /* debug */ sof_tgl_ops.debug_map = tgl_dsp_debugfs; sof_tgl_ops.debug_map_count = ARRAY_SIZE(tgl_dsp_debugfs); - sof_tgl_ops.ipc_dump = cnl_ipc_dump;
/* pre/post fw run */ sof_tgl_ops.post_fw_run = hda_dsp_post_fw_run;
The use of the IPC registers are different between IPC3 and IPC4. The ipc_dump needs to use different prints depending on the used IPC protocol.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com --- sound/soc/sof/intel/apl.c | 7 ++++++- sound/soc/sof/intel/hda.c | 18 ++++++++++++++++++ sound/soc/sof/intel/hda.h | 1 + 3 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/sound/soc/sof/intel/apl.c b/sound/soc/sof/intel/apl.c index 295df44be271..37d12e821c10 100644 --- a/sound/soc/sof/intel/apl.c +++ b/sound/soc/sof/intel/apl.c @@ -45,6 +45,9 @@ int sof_apl_ops_init(struct snd_sof_dev *sdev)
/* ipc */ sof_apl_ops.send_msg = hda_dsp_ipc_send_msg; + + /* debug */ + sof_apl_ops.ipc_dump = hda_ipc_dump; }
if (sdev->pdata->ipc_type == SOF_INTEL_IPC4) { @@ -64,6 +67,9 @@ int sof_apl_ops_init(struct snd_sof_dev *sdev)
/* ipc */ sof_apl_ops.send_msg = hda_dsp_ipc4_send_msg; + + /* debug */ + sof_apl_ops.ipc_dump = hda_ipc4_dump; }
/* set DAI driver ops */ @@ -72,7 +78,6 @@ int sof_apl_ops_init(struct snd_sof_dev *sdev) /* debug */ sof_apl_ops.debug_map = apl_dsp_debugfs; sof_apl_ops.debug_map_count = ARRAY_SIZE(apl_dsp_debugfs); - sof_apl_ops.ipc_dump = hda_ipc_dump;
/* firmware run */ sof_apl_ops.run = hda_dsp_cl_boot_firmware; diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index f7068a7e2e81..ca648d2a9da7 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -662,6 +662,24 @@ void hda_ipc_dump(struct snd_sof_dev *sdev) hipcie, hipct, hipcctl); }
+void hda_ipc4_dump(struct snd_sof_dev *sdev) +{ + u32 hipci, hipcie, hipct, hipcte, hipcctl; + + hda_ipc_irq_dump(sdev); + + hipci = snd_sof_dsp_read(sdev, HDA_DSP_BAR, HDA_DSP_REG_HIPCI); + hipcie = snd_sof_dsp_read(sdev, HDA_DSP_BAR, HDA_DSP_REG_HIPCIE); + hipct = snd_sof_dsp_read(sdev, HDA_DSP_BAR, HDA_DSP_REG_HIPCT); + hipcte = snd_sof_dsp_read(sdev, HDA_DSP_BAR, HDA_DSP_REG_HIPCTE); + hipcctl = snd_sof_dsp_read(sdev, HDA_DSP_BAR, HDA_DSP_REG_HIPCCTL); + + /* dump the IPC regs */ + /* TODO: parse the raw msg */ + dev_err(sdev->dev, "Host IPC initiator: %#x|%#x, target: %#x|%#x, ctl: %#x\n", + hipci, hipcie, hipct, hipcte, hipcctl); +} + static int hda_init(struct snd_sof_dev *sdev) { struct hda_bus *hbus; diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h index 2013a94020c6..c3a9f89b726d 100644 --- a/sound/soc/sof/intel/hda.h +++ b/sound/soc/sof/intel/hda.h @@ -851,6 +851,7 @@ irqreturn_t cnl_ipc4_irq_thread(int irq, void *context); int cnl_ipc4_send_msg(struct snd_sof_dev *sdev, struct snd_sof_ipc_msg *msg); irqreturn_t hda_dsp_ipc4_irq_thread(int irq, void *context); int hda_dsp_ipc4_send_msg(struct snd_sof_dev *sdev, struct snd_sof_ipc_msg *msg); +void hda_ipc4_dump(struct snd_sof_dev *sdev); extern struct sdw_intel_ops sdw_callback;
#endif
The use of the IPC registers are different between IPC3 and IPC4. The ipc_dump needs to use different prints depending on the used IPC protocol.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com --- sound/soc/sof/intel/skl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/sof/intel/skl.c b/sound/soc/sof/intel/skl.c index f05905e00368..eba166fe4d33 100644 --- a/sound/soc/sof/intel/skl.c +++ b/sound/soc/sof/intel/skl.c @@ -87,7 +87,7 @@ int sof_skl_ops_init(struct snd_sof_dev *sdev) /* debug */ sof_skl_ops.debug_map = skl_dsp_debugfs; sof_skl_ops.debug_map_count = ARRAY_SIZE(skl_dsp_debugfs); - sof_skl_ops.ipc_dump = hda_ipc_dump; + sof_skl_ops.ipc_dump = hda_ipc4_dump;
/* firmware run */ sof_skl_ops.run = hda_dsp_cl_boot_firmware_skl;
The use of the IPC registers are different between IPC3 and IPC4. The ipc_dump needs to use different prints depending on the used IPC protocol.
The existing code was printing registers relevant for IPC3, which is not even supported on MTL.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com --- sound/soc/sof/intel/mtl.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/sound/soc/sof/intel/mtl.c b/sound/soc/sof/intel/mtl.c index 5408c34b04ef..61bcaa1556f6 100644 --- a/sound/soc/sof/intel/mtl.c +++ b/sound/soc/sof/intel/mtl.c @@ -698,20 +698,19 @@ static int mtl_dsp_runtime_resume(struct snd_sof_dev *sdev)
static void mtl_ipc_dump(struct snd_sof_dev *sdev) { - u32 hipcctl; - u32 hipcida; - u32 hipctdr; + u32 hipcidr, hipcidd, hipcida, hipctdr, hipctdd, hipctda, hipcctl;
- /* read IPC status */ + hipcidr = snd_sof_dsp_read(sdev, HDA_DSP_BAR, MTL_DSP_REG_HFIPCXIDR); + hipcidd = snd_sof_dsp_read(sdev, HDA_DSP_BAR, MTL_DSP_REG_HFIPCXIDDY); hipcida = snd_sof_dsp_read(sdev, HDA_DSP_BAR, MTL_DSP_REG_HFIPCXIDA); - hipcctl = snd_sof_dsp_read(sdev, HDA_DSP_BAR, MTL_DSP_REG_HFIPCXCTL); hipctdr = snd_sof_dsp_read(sdev, HDA_DSP_BAR, MTL_DSP_REG_HFIPCXTDR); + hipctdd = snd_sof_dsp_read(sdev, HDA_DSP_BAR, MTL_DSP_REG_HFIPCXTDDY); + hipctda = snd_sof_dsp_read(sdev, HDA_DSP_BAR, MTL_DSP_REG_HFIPCXTDA); + hipcctl = snd_sof_dsp_read(sdev, HDA_DSP_BAR, MTL_DSP_REG_HFIPCXCTL);
- /* dump the IPC regs */ - /* TODO: parse the raw msg */ dev_err(sdev->dev, - "error: host status 0x%8.8x dsp status 0x%8.8x mask 0x%8.8x\n", - hipcida, hipctdr, hipcctl); + "Host IPC initiator: %#x|%#x|%#x, target: %#x|%#x|%#x, ctl: %#x\n", + hipcidr, hipcidd, hipcida, hipctdr, hipctdd, hipctda, hipcctl); }
/* Meteorlake ops */
The firmware register dump is IPC3 specific, it is not available for other IPC versions.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com --- sound/soc/sof/intel/hda.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index ca648d2a9da7..e00062f3b21c 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -598,7 +598,8 @@ void hda_dsp_dump(struct snd_sof_dev *sdev, u32 flags) /* print ROM/FW status */ hda_dsp_get_state(sdev, level);
- if (flags & SOF_DBG_DUMP_REGS) { + /* The firmware register dump only available with IPC3 */ + if (flags & SOF_DBG_DUMP_REGS && sdev->pdata->ipc_type == SOF_IPC) { u32 status = snd_sof_dsp_read(sdev, HDA_DSP_BAR, HDA_DSP_SRAM_REG_FW_STATUS); u32 panic = snd_sof_dsp_read(sdev, HDA_DSP_BAR, HDA_DSP_SRAM_REG_FW_TRACEP);
It can help debugging IPC timeout issues (like we do with IPC3) if we dump the IPC and DSP information.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Rander Wang rander.wang@intel.com --- sound/soc/sof/ipc4.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/soc/sof/ipc4.c b/sound/soc/sof/ipc4.c index 0d830020556d..6eaa18e27e5a 100644 --- a/sound/soc/sof/ipc4.c +++ b/sound/soc/sof/ipc4.c @@ -295,6 +295,7 @@ static int ipc4_wait_tx_done(struct snd_sof_ipc *ipc, void *reply_data) if (ret == 0) { dev_err(sdev->dev, "ipc timed out for %#x|%#x\n", ipc4_msg->primary, ipc4_msg->extension); + snd_sof_handle_fw_exception(ipc->sdev, "IPC timeout"); return -ETIMEDOUT; }
On Fri, 23 Sep 2022 16:36:10 +0300, Peter Ujfalusi wrote:
On Intel platforms the registers for DSP communications are used differently, the IPC dump information is not correct since important registers are not printed and existing ones are used a bit differently for IPC4.
As a last step, enable the IPC timeout 'handling' and allow the printout of the now usefull IPC dump.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/6] ASoC: SOF: Intel: cnl: Add separate ops for ipc_dump for IPC4 commit: a996a333ad74d1f26c3831f1edd94a5d16798a0c [2/6] ASoC: SOF: Intel: hda: Add separate ops for ipc_dump for IPC4 commit: 32b97c07c2a3b7cccc0c7e9a5b23970bd9a52c5d [3/6] ASoC: SOF: Intel: skl: Use the ipc4 version of the ipc_dump commit: 6759f35b234aa94e26e122afcd402ba2a39bd9d3 [4/6] ASoC: SOF: Intel: mtl: Print relevant register in ipc_dump commit: d01784ee680c558938baf6c4f184bee2bc612798 [5/6] ASoC: SOF: Intel: hda: Only dump firmware registers for IPC3 commit: 01fb69d09afb896579e00c3dbc3c1aa74613dd86 [6/6] ASoC: SOF: ipc4: Call snd_sof_handle_fw_exception() in case of timeout commit: 4245fdba89b82befee0d963a85f7494c70432ee9
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (2)
-
Mark Brown -
Peter Ujfalusi