Repository navigation
IMR restore feature implementation #2989
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,10 +21,49 @@ | |
| #include <sound/sof.h> | ||
| #include "ext_manifest.h" | ||
| #include "../ops.h" | ||
| #include "../sof-priv.h" | ||
| #include "hda.h" | ||
|
|
||
| #define HDA_CL_STREAM_FORMAT 0x40 | ||
|
|
||
| static void hda_ssp_resume(struct snd_sof_dev *sdev) | ||
| { | ||
| struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata; | ||
| const struct sof_intel_dsp_desc *chip = hda->desc; | ||
| int i; | ||
|
|
||
| /* DSP is powered up, set all SSPs to slave mode */ | ||
| for (i = 0; i < chip->ssp_count; i++) { | ||
| snd_sof_dsp_update_bits_unlocked(sdev, HDA_DSP_BAR, | ||
| chip->ssp_base_offset | ||
| + i * SSP_DEV_MEM_SIZE | ||
| + SSP_SSC1_OFFSET, | ||
| SSP_SET_SLAVE, | ||
| SSP_SET_SLAVE); | ||
| } | ||
| } | ||
|
|
||
| static void hda_sdw_resume(struct snd_sof_dev *sdev) | ||
| { | ||
| /* | ||
| * When a SoundWire link is in clock stop state, a secondary | ||
| * device may trigger in-band wakes for events such as jack | ||
| * insertion or acoustic event detection. This event will lead | ||
| * to a WAKEEN interrupt, handled by the PCI device and routed | ||
| * to PME if the PCI device is in D3. The resume function in | ||
| * audio PCI driver will be invoked by ACPI for PME event and | ||
| * initialize the device and process WAKEEN interrupt. | ||
| * | ||
| * The WAKEEN interrupt should be processed ASAP to prevent an | ||
| * interrupt flood, otherwise other interrupts, such IPC, | ||
| * cannot work normally. The WAKEEN is handled after the ROM | ||
| * is initialized successfully, which ensures power rails are | ||
| * enabled before accessing the SoundWire SHIM registers | ||
| */ | ||
| if (!sdev->first_boot) | ||
| hda_sdw_process_wakeen(sdev); | ||
| } | ||
|
|
||
|
plbossart marked this conversation as resolved.
Resolved
|
||
| static struct hdac_ext_stream *cl_stream_prepare(struct snd_sof_dev *sdev, unsigned int format, | ||
| unsigned int size, struct snd_dma_buffer *dmab, | ||
| int direction) | ||
|
|
@@ -91,7 +130,6 @@ static int cl_dsp_init(struct snd_sof_dev *sdev, int stream_tag) | |
| char *dump_msg; | ||
| u32 flags, j; | ||
| int ret; | ||
|
|
||
| /* step 1: power up corex */ | ||
| ret = hda_dsp_enable_core(sdev, chip->host_managed_cores_mask); | ||
|
|
@@ -101,15 +139,7 @@ static int cl_dsp_init(struct snd_sof_dev *sdev, int stream_tag) | |
| goto err; | ||
| } | ||
|
|
||
| hda_ssp_resume(sdev); | ||
|
Member
There was a problem hiding this comment. the commit message mentions a different name so that needs to be modified. what was wrong with hda_set_ssps_slave()?
Member
There was a problem hiding this comment. this calls SSP_SET_SLAVE... I am all for inclusive terms, but this has nothing to do with resume operations.
Author
There was a problem hiding this comment.
I think Ranjani means "what we need to perform to SSPs when resuming audio device (D3->D0)", like "_ssp_at_resume()". And, use the more generic _ssp_resume() will be friendly to add more SSP related reset/resume/init stuff into it in future when needed. I am fine on both, just let me know the aligned one.
Collaborator
There was a problem hiding this comment. just because we do set_slave doesnt mean we have to name the function that too no? Someday maybe set_slave will get renamed and we wont have to worry about the static function name having the word"slave" in it. But I agree resume is a bad name. This is the loader, so while it is also called during resume, thats not the only thing. Why not call is something lika hda_ssp_cl_init() and hda_sdw_cl_init()
Member
There was a problem hiding this comment. the acronym we came up with is CBP Codec BitClockProvider, so we can call this wrapper hda_ssp_set_cbp() if it also sets the fsync hda_ssp_set_cbp_cfp(). and no wrapper for SoundWire (see comment below) would be my preference.
Member
There was a problem hiding this comment. sound/soc/sof/intel/hda-loader.c: SSP_SET_SLAVE, This should be SSP_SET_CBP_CFP |
||
|
|
||
| /* step 2: purge FW request */ | ||
| snd_sof_dsp_write(sdev, HDA_DSP_BAR, chip->ipc_req, | ||
|
|
@@ -345,6 +375,37 @@ int hda_dsp_cl_boot_firmware_iccmax(struct snd_sof_dev *sdev) | |
| return ret; | ||
| } | ||
|
|
||
| static int hda_dsp_boot_imr(struct snd_sof_dev *sdev) | ||
|
Collaborator
There was a problem hiding this comment. you call it IMR restore support with no mention on it in the code anywhere. The code has only references to D3_PERSISTENT. Can we just stick to that in the commit messages as well so that reader don't get confused. Also I think PERSISTENT_D3 sounds more intuitive than D3_PERSISTENT. D3_PERSISTENT sounds like the D3 state will be persistent.
Collaborator
There was a problem hiding this comment. acutally if I read back my own comment, both of them convey the same message. Maybe we should call it IMR_PERSISTENT_D3 or something similar to say whats persistent in D3
Author
There was a problem hiding this comment. Per my understanding, the IMR D3 persistency is our system behavior and should not be configurable, but either to use the IMR restoring feature or not is configurable. Yes, the naming SOF_IPC_INFO_D3_PERSISTENT is somewhat confusing with respect to what I mentioned above, it should be SOF_IPC_INFO_IMR_RESTORE_SUPPORT, but since the naming is already quite long, and it needs to align to the existed FW side also, I would suggest keeping it at the moment and change it later from both side if needed. |
||
| { | ||
| struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata; | ||
| const struct sof_intel_dsp_desc *chip = hda->desc; | ||
| unsigned long mask; | ||
| u32 j; | ||
| int ret; | ||
|
|
||
| /* power up & unstall/run the cores to run the firmware */ | ||
| ret = hda_dsp_enable_core(sdev, chip->init_core_mask); | ||
| if (ret < 0) { | ||
| dev_err(sdev->dev, "dsp core start failed %d\n", ret); | ||
| return -EIO; | ||
| } | ||
|
|
||
| /* set enabled cores mask and increment ref count for cores in init_core_mask */ | ||
| sdev->enabled_cores_mask |= chip->init_core_mask; | ||
|
Collaborator
There was a problem hiding this comment. @keyonjie paranoia check here: I hope you have checked that this is OK to do i.e just power up the init_core_mask. When we download the fw, we power up all cores and then power down ~init_core_mask cores.
Author
There was a problem hiding this comment.
Yes, the CI test showed me that we actually don't need to power up other Cores not covered by init_core_mask.
Member
There was a problem hiding this comment. @keyonjie you should base your results on the hardware recommended sequences, not experimental results from a small set of devices in the CI pool. That really doesn't inspire confidence if I'm honest.
Author
There was a problem hiding this comment.
That's a good point, I didn't find any hardware-recommended sequences mentioning that we have to power on all DSP cores at the booting, @ranj063 did you? |
||
| mask = sdev->enabled_cores_mask; | ||
| for_each_set_bit(j, &mask, SOF_MAX_DSP_NUM_CORES) | ||
| sdev->dsp_core_ref_count[j]++; | ||
|
Member
There was a problem hiding this comment. @keyonjie I was not able to understand the difference with the first boot where this code is used /* step 1: power up corex */
ret = hda_dsp_enable_core(sdev, chip->host_managed_cores_mask);init_core_mask, enable_cores_mask, host_managed_cores_mask, that looks like a recipe for bugs.
Author
There was a problem hiding this comment.
It could be, but that is not introduced by this PR, no? |
||
|
|
||
| hda_ssp_resume(sdev); | ||
|
|
||
| /* enable IPC interrupts */ | ||
| hda_dsp_ipc_int_enable(sdev); | ||
|
|
||
| hda_sdw_resume(sdev); | ||
|
|
||
| return ret; | ||
| } | ||
|
|
||
| int hda_dsp_cl_boot_firmware(struct snd_sof_dev *sdev) | ||
| { | ||
| struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata; | ||
|
|
@@ -355,6 +416,13 @@ int hda_dsp_cl_boot_firmware(struct snd_sof_dev *sdev) | |
| struct firmware stripped_firmware; | ||
| int ret, ret1, i; | ||
|
|
||
| if ((sdev->fw_ready.flags & SOF_IPC_INFO_D3_PERSISTENT) && | ||
| !(sof_debug_check_flag(SOF_DBG_IGNORE_D3_PERSISTENT)) && | ||
|
Collaborator
There was a problem hiding this comment. this patch has to be squashed with the previous one no? Because we'd enable it by default in the previous one and only disable it based on the module param in this one
Author
There was a problem hiding this comment. my design here is to enable it by default if the FW claims it supports the feature. So this subsequent module param is only adding an interface to disable using the FW feature on the host side, for debugging purposes only. |
||
| !sdev->first_boot) { | ||
| dev_dbg(sdev->dev, "IMR restore supported, booting from IMR directly\n"); | ||
| return hda_dsp_boot_imr(sdev); | ||
|
Member
There was a problem hiding this comment. what happens if we override this IMR_RESTORE configuration. Can we still go ahead and proceed with the default path? This should be an opt-in solution IMHO. Changing the flows will mechanically lead to bugs, so by default users who upgrade the firmware should not see a change on how the DSP is managed.
Author
There was a problem hiding this comment.
Yes, if the driver choose to re-downloading, even the IMR_RESTORE is declared support from the fw_ready, we will still go ahead with today's path.
Um, that's what I commented above, do you suggest a kconfig item or a module param here?
Collaborator
There was a problem hiding this comment. @plbossart This can also fix issues, so not sure if this should be opt-in. If tests look good, we could just make this the default and offer an opt-out in case an issue is hit.
Member
There was a problem hiding this comment. @kv2019i yes, that's what I meant, opt-out/disable similar to pm_runtime or dynamic pipelines.
Author
There was a problem hiding this comment. So let me add a kconfig item and the check will be like this: if ((sdev->fw_ready.flags & SOF_IPC_INFO_IMR_RESTORE) &&
!CONFIG_SND_SOC_SOF_DEBUG_DISABLE_IMR_RESTORE &&
!sdev->first_boot) {
Collaborator
There was a problem hiding this comment. @keyonjie @plbossart I'd vote for a module option. If this causes a regression, user can easily add a modprobe.d option, but having to roll out a custom kernel for a specific system is too difficult. And we have to too many Kconfig options. If there are many, we should have a quirk table in driver to make the default sane for most systems (as the module option is not scalable either).
Member
There was a problem hiding this comment. agree with @kv2019i we already have sof_debug quirks as a module parameter, let's add one bit to disable IMR reload. |
||
| } | ||
|
|
||
| chip_info = desc->chip_info; | ||
|
|
||
| if (plat_data->fw->size <= plat_data->fw_offset) { | ||
|
|
@@ -398,23 +466,7 @@ int hda_dsp_cl_boot_firmware(struct snd_sof_dev *sdev) | |
| goto cleanup; | ||
| } | ||
|
|
||
| hda_sdw_resume(sdev); | ||
|
Member
There was a problem hiding this comment. we could move the !sdev->first_boot inside the helper, no need to add a new definition that's rather misleading.
Author
There was a problem hiding this comment.
checking sdev->first_boot inside hda_sdw_process_wakeen() sounds a bit odd to me, @plbossart do you really want that?
Member
There was a problem hiding this comment. I actually don't see a need for a new wrapper here, just use if (!sdev->first_boot) twice...
Member
There was a problem hiding this comment. actually the imr stuff only happens after the first boot so the test is not needed ... really no need for a wrapper. |
||
|
|
||
| /* | ||
| * Set the boot_iteration to the last attempt, indicating that the | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@keyonjie your commit message is way too specific.
D3_PERSISTENT could also be used for Baytrail where the DSP is not power gated. This bit should really mean that downloading firmware after a D3 transition is not required, whether the DSP remained powered or the memory was power-gated and restored is a platform detail that the kernel does not need to know, does it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also this has to be 3.20, I took 3.19 for SSP changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not good at this naming stuff, since it has gone with this on the FW side as a FW_READY flag, let's just align to it?
For the ABI version, can we use 3.19 for both updates? It is a little bit screwed as the FW side has taken this D3_PERSISTENT as 3.19 and driver side took SSP changes as 3.19.
To fix this, as before can we use one minor version bump for several updates in a bunch? @lgirdwood FYI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@plbossart @keyonjie we can use 3.19 for both 3.19 already went out with a release (and I thin that's not the case, the tagged v1.8 has ABI3.18.1), so 3.19 is for SOF v1.9.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kv2019i I don't think 3.19 is out, I bumped the number in my SSP changes and it's not merged yet.