intel_adsp: Fix interruption locks in power management code #67863
Conversation
zephyrbot
requested review from
andyross,
dcpleung,
jxstelter,
kv2019i,
lgirdwood,
lyakh,
marc-hb,
marcinszkudlinski,
nashif,
softwarecki and
tmleman
|
Putting DNM label because I found issues in save/restore context path (not caused by this pr) that makes the atomic idle does not work. |
|
DNM removed :) @tmleman can you take a look please ? |
soc/xtensa/intel_adsp/ace/power.c
Outdated
| @@ -144,6 +144,7 @@ static ALWAYS_INLINE void _restore_core_context(void) | |||
| XTENSA_WSR("EXCSAVE2", core_desc[core_id].excsave2); | |||
| XTENSA_WSR("EXCSAVE3", core_desc[core_id].excsave3); | |||
| XTENSA_WUR("THREADPTR", core_desc[core_id].thread_ptr); | |||
| __asm__ volatile("wsr.intenable %0" : : "r"(0x00000000)); | |||
There was a problem hiding this comment.
This step is redundant; the register is cleared in the cpu_early_init procedure.
There was a problem hiding this comment.
yep. Thanks for pointing it.
soc/xtensa/intel_adsp/ace/power.c
Outdated
| @@ -173,7 +174,6 @@ void power_gate_entry(uint32_t core_id) | |||
| soc_cpus_active[core_id] = false; | |||
| sys_cache_data_flush_range(soc_cpus_active, sizeof(soc_cpus_active)); | |||
| k_cpu_idle(); | |||
| z_xt_ints_off(0xffffffff); | |||
There was a problem hiding this comment.
It's true that after power gating, we are not going back here, but we can't guarantee that power gating will happen. If it's not causing any problems, I would recommend keeping it.
There was a problem hiding this comment.
why can't we guarantee ? I am asking because I thought this was deterministic, if it is not I will check the workflow with this possibility in mind (obviously just keeping it here is ok ...)
There was a problem hiding this comment.
The idea of disabling interrupts as a step after WAITI is taken from the reference code. There is no detailed explanation of what exactly can prevent power gating in that case. I think the apprehension about this arises from the fact that firmware does not have full control over all hardware prevents. For example, the HOST can prevent the DSP HP domain from its side without any interaction with firmware.
|
|
||
| /* We don't have the key used to lock interruptions here. | ||
| * Just set PS.INTLEVEL to 0. | ||
| */ | ||
| __asm__ volatile ("rsil a2, 0"); |
There was a problem hiding this comment.
This commit should be part of the previous one. Without restoring the PS value, PS.INTLEVEL == 0
We are arbitrarily setting a value to PS after power gates and losing valid information like OWB, CALLINC and INTLEVEL. We need to properly save/restore them to avoid possible wrong behavior. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>
In pm_state_set we can't just call k_cpu_idle() because this will clear out PS.INTLEVEL. Use k_cpu_atomic_idle instead since Zephyr's expect interruptions to be locked after pm_state_set. Signed-off-by: Flavio Ceolin <flavio.ceolin@intel.com>
|
@RanderWang can you take a look at the changes for CAVS? |
Interruptions must be locked until
pm_state_exit_post_ops.