[PATCH 0/3] ASoC: SOF: Intel: Fix IMR boot race caused by early core booting

[PATCH 0/3] ASoC: SOF: Intel: Fix IMR boot race caused by early core booting

[Peter Ujfalusi]

Hi,

After the revisited IMR sequence patch
2a68ff846164 ("ASoC: SOF: Intel: hda: Revisit IMR boot sequence")

we have started to see random boot issues manifesting in ghost reply messages from
the DSP for a never sent message.
It turned out that an earlier commit:
d416519982cb ("ASoC: SOF: hda: don't use the core op for power up/power down")

replaced the core power up step with a full blown core power up + enable call.
The result was that we have booted the core before we would place the message
to the ROM code and based on luck and timing it got received by the (IMR) booted
firmware or in between ROM and firmware boot.

Mark, can you queue this series for 5.19 as the 2a68ff846164 is in rc1 to make
sure that we fix this random issue?

Regards,
Peter
---
Peter Ujfalusi (3):
  ASoC: SOF: Intel: hda-dsp: Expose hda_dsp_core_power_up()
  ASoC: SOF: Intel: hda-loader: Make sure that the fw load sequence is
    followed
  ASoC: SOF: Intel: hda-loader: Clarify the cl_dsp_init() flow

 sound/soc/sof/intel/hda-dsp.c    | 10 +++++++++-
 sound/soc/sof/intel/hda-loader.c | 10 +++++-----
 sound/soc/sof/intel/hda.h        |  1 +
 3 files changed, 15 insertions(+), 6 deletions(-)

-- 
2.36.1

[PATCH 1/3] ASoC: SOF: Intel: hda-dsp: Expose hda_dsp_core_power_up()

[Peter Ujfalusi]

The hda_dsp_core_power_up() needs to be exposed so that it can be used in
hda-loader.c to correct the boot flow.
The first step must not unstall the core, it should only power up the
core(s).

Add sanity check for the core_mask while exposing it to be safe.

Complements: 2a68ff846164 ("ASoC: SOF: Intel: hda: Revisit IMR boot sequence")
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
---
 sound/soc/sof/intel/hda-dsp.c | 10 +++++++++-
 sound/soc/sof/intel/hda.h     |  1 +
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c
index 000ea906670c..e24eea725acb 100644
--- a/sound/soc/sof/intel/hda-dsp.c
+++ b/sound/soc/sof/intel/hda-dsp.c
@@ -181,12 +181,20 @@ int hda_dsp_core_run(struct snd_sof_dev *sdev, unsigned int core_mask)
  * Power Management.
  */
 
-static int hda_dsp_core_power_up(struct snd_sof_dev *sdev, unsigned int core_mask)
+int hda_dsp_core_power_up(struct snd_sof_dev *sdev, unsigned int core_mask)
 {
+	struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
+	const struct sof_intel_dsp_desc *chip = hda->desc;
 	unsigned int cpa;
 	u32 adspcs;
 	int ret;
 
+	/* restrict core_mask to host managed cores mask */
+	core_mask &= chip->host_managed_cores_mask;
+	/* return if core_mask is not valid */
+	if (!core_mask)
+		return 0;
+
 	/* update bits */
 	snd_sof_dsp_update_bits(sdev, HDA_DSP_BAR, HDA_DSP_REG_ADSPCS,
 				HDA_DSP_ADSPCS_SPA_MASK(core_mask),
diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
index 3e0f7b0c586a..0f57ef5d9b8e 100644
--- a/sound/soc/sof/intel/hda.h
+++ b/sound/soc/sof/intel/hda.h
@@ -497,6 +497,7 @@ struct sof_intel_hda_stream {
  */
 int hda_dsp_probe(struct snd_sof_dev *sdev);
 int hda_dsp_remove(struct snd_sof_dev *sdev);
+int hda_dsp_core_power_up(struct snd_sof_dev *sdev, unsigned int core_mask);
 int hda_dsp_core_run(struct snd_sof_dev *sdev, unsigned int core_mask);
 int hda_dsp_enable_core(struct snd_sof_dev *sdev, unsigned int core_mask);
 int hda_dsp_core_reset_power_down(struct snd_sof_dev *sdev,
-- 
2.36.1

[PATCH 2/3] ASoC: SOF: Intel: hda-loader: Make sure that the fw load sequence is followed

[Peter Ujfalusi]

The hda_dsp_enable_core() is powering up _and_ unstall the core in one
call while the first step of the firmware loading  must not unstall the
core.
The core can be unstalled only after the set cpb_cfp and the configuration
of the IPC register for the ROM_CONTROL message.

Complements: 2a68ff846164 ("ASoC: SOF: Intel: hda: Revisit IMR boot sequence")
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
---
 sound/soc/sof/intel/hda-loader.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/sof/intel/hda-loader.c b/sound/soc/sof/intel/hda-loader.c
index 64290125d7cd..103e62bcfa82 100644
--- a/sound/soc/sof/intel/hda-loader.c
+++ b/sound/soc/sof/intel/hda-loader.c
@@ -110,7 +110,7 @@ static int cl_dsp_init(struct snd_sof_dev *sdev, int stream_tag, bool imr_boot)
 	int ret;
 
 	/* step 1: power up corex */
-	ret = hda_dsp_enable_core(sdev, chip->host_managed_cores_mask);
+	ret = hda_dsp_core_power_up(sdev, chip->host_managed_cores_mask);
 	if (ret < 0) {
 		if (hda->boot_iteration == HDA_FW_BOOT_ATTEMPTS)
 			dev_err(sdev->dev, "error: dsp core 0/1 power up failed\n");
-- 
2.36.1

[PATCH 3/3] ASoC: SOF: Intel: hda-loader: Clarify the cl_dsp_init() flow

[Peter Ujfalusi]

Update the comment for the cl_dsp_init() to clarify what is done by the
function and use the chip->init_core_mask instead of BIT(0) when
unstalling/running the init core.

Complements: 2a68ff846164 ("ASoC: SOF: Intel: hda: Revisit IMR boot sequence")
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
---
 sound/soc/sof/intel/hda-loader.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/sof/intel/hda-loader.c b/sound/soc/sof/intel/hda-loader.c
index 103e62bcfa82..d3ec5996a9a3 100644
--- a/sound/soc/sof/intel/hda-loader.c
+++ b/sound/soc/sof/intel/hda-loader.c
@@ -95,9 +95,9 @@ struct hdac_ext_stream *hda_cl_stream_prepare(struct snd_sof_dev *sdev, unsigned
 }
 
 /*
- * first boot sequence has some extra steps. core 0 waits for power
- * status on core 1, so power up core 1 also momentarily, keep it in
- * reset/stall and then turn it off
+ * first boot sequence has some extra steps.
+ * power on all host managed cores and only unstall/run the boot core to boot the
+ * DSP then turn off all non boot cores (if any) is powered on.
  */
 static int cl_dsp_init(struct snd_sof_dev *sdev, int stream_tag, bool imr_boot)
 {
@@ -127,7 +127,7 @@ static int cl_dsp_init(struct snd_sof_dev *sdev, int stream_tag, bool imr_boot)
 	snd_sof_dsp_write(sdev, HDA_DSP_BAR, chip->ipc_req, ipc_hdr);
 
 	/* step 3: unset core 0 reset state & unstall/run core 0 */
-	ret = hda_dsp_core_run(sdev, BIT(0));
+	ret = hda_dsp_core_run(sdev, chip->init_core_mask);
 	if (ret < 0) {
 		if (hda->boot_iteration == HDA_FW_BOOT_ATTEMPTS)
 			dev_err(sdev->dev,
-- 
2.36.1

Re: [PATCH 0/3] ASoC: SOF: Intel: Fix IMR boot race caused by early core booting

[Mark Brown]

On Thu, 9 Jun 2022 11:59:46 +0300, Peter Ujfalusi wrote:
> After the revisited IMR sequence patch
> 2a68ff846164 ("ASoC: SOF: Intel: hda: Revisit IMR boot sequence")
> 
> we have started to see random boot issues manifesting in ghost reply messages from
> the DSP for a never sent message.
> It turned out that an earlier commit:
> d416519982cb ("ASoC: SOF: hda: don't use the core op for power up/power down")
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/3] ASoC: SOF: Intel: hda-dsp: Expose hda_dsp_core_power_up()
      commit: 537b4a0c8b9490d762e70c0ecec38144c83d0c37
[2/3] ASoC: SOF: Intel: hda-loader: Make sure that the fw load sequence is followed
      commit: fcb3c775f7073410965ce9414ddb2a1f339c502b
[3/3] ASoC: SOF: Intel: hda-loader: Clarify the cl_dsp_init() flow
      commit: 4643e10a17e549467420aaeeb35c9b3480716618

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