[PATCH 0/3] ASoC: SOF: Intel: Fix IMR boot race caused by early core booting
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(-)
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,
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");
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,
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
participants (2)
-
Mark Brown -
Peter Ujfalusi