-
Notifications
You must be signed in to change notification settings - Fork 7.8k
STM32WBAX PM rework to support STOP/STDBY modes with radio activity #92847
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
The following west manifest projects have changed revision in this Pull Request:
⛔ DNM label due to: 1 project with PR revision Note: This message is automatically posted and updated by the Manifest GitHub Action. |
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.
Hello @asm5878, thanks a lot for this draft PR, very interesting proposal. I added few preliminary comments, and I would like also to involve @HoZHel and @FrancescoCiminoST
/* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
* Copyright (c) 2023 STMicroelectronics | ||
*/ | ||
|
||
/ { | ||
/* Change min residency time to ease power consumption measurement */ | ||
cpus { | ||
cpu0: cpu@0 { | ||
cpu-power-states = <&stop0 &stop1 &standby>; | ||
}; | ||
|
||
power-states { | ||
standby: state2 { | ||
compatible = "zephyr,power-state"; | ||
power-state-name = "suspend-to-ram"; | ||
substate-id = <1>; | ||
min-residency-us = <10000>; | ||
exit-latency-us = <1000>; | ||
}; | ||
}; | ||
}; | ||
|
||
}; | ||
|
||
&lptim1 { | ||
status = "okay"; | ||
}; |
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.
In my opinion, all these changes should be moved in the dtsi file of the device since they are not board specific.
In particular, the standby power state should be added where stop 0 and stop 1 are defined.
LPTIM should be selected when CONFIG_PM is set.
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.
Agreed and for LPTIM we could use PM config options to enable/disable LPTIM (board or soc dts )
/* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
* Copyright (c) 2023 STMicroelectronics | ||
*/ | ||
|
||
/ { | ||
/* Change min residency time to ease power consumption measurement */ | ||
cpus { | ||
cpu0: cpu@0 { | ||
cpu-power-states = <&stop0 &stop1 &standby>; | ||
}; | ||
|
||
power-states { | ||
standby: state2 { | ||
compatible = "zephyr,power-state"; | ||
power-state-name = "suspend-to-ram"; | ||
substate-id = <1>; | ||
min-residency-us = <10000>; | ||
exit-latency-us = <1000>; | ||
}; | ||
}; | ||
}; | ||
}; | ||
|
||
&lptim1 { | ||
status = "okay"; | ||
}; |
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.
Similar considerations done for nucleo_wba55cg.overlay
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.
Same comment as before
|
||
CONFIG_PM=y | ||
CONFIG_PM_DEVICE=y | ||
CONFIG_PM_DEVICE_RUNTIME=y | ||
CONFIG_PM_DEVICE_SYSTEM_MANAGED=y | ||
CONFIG_PM_S2RAM=y |
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 think these should be moved to a specific file related to the relevant NUCLEO, if we want to enable power management by default.
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.
We could just leave CONFIG_PM
to be selected by user in each sample and use it to conditionally enable all the rest of configs in nucleo_wba65ri_defconfig
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.
CONFIG_PM_S2RAM
could be imply
ed by CONFIG_PM
at SoC level.
Or even better: imply
only if the corresponding state is status = "okay"
in DTS. (but not sure that status = "disabled"
on a power state really disables it)
soc/st/stm32/stm32wbax/power.c
Outdated
#include "linklayer_plat.h" | ||
#include "ll_sys.h" |
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.
This code is not needed, and it should be removed, including the enclosing #ifdef CONFIG_BT_STM32WBA
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.
Agreed, for sure an error during the rebase.
#ifdef CONFIG_BT_STM32WBA | ||
#include "ll_sys.h" | ||
#endif |
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 consider this code experimental and eventually to be removed as it is device specific.
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.
yes, this file must stay untouched compared to upstream
subsys/pm/pm.c
Outdated
#ifdef CONFIG_BT_STM32WBA | ||
{ | ||
uint32_t radio_remaining_time, cmd_status; | ||
int32_t radio_ticks; | ||
cmd_status = |
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 understand this logic, but all these changes in this file should ideally be reverted and an approach using
zephyr/subsys/pm/policy/policy_events.c
Line 62 in e3ed029
void pm_policy_event_register(struct pm_policy_event *evt, int64_t uptime_ticks) |
zephyr/subsys/pm/policy/policy_events.c
Line 79 in e3ed029
void pm_policy_event_unregister(struct pm_policy_event *evt) |
should be used
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.
Same comment as before, this file will stay untouched and in hci_stm32wba.c we need to understand when the next radio event has been set and inform zephyr when this event will occur (or not)
west.yml
Outdated
revision: 126cbbee19208b7aaca5ad8b287cf104e8bc837a | ||
revision: pull/294/head |
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.
To be reverted before marking this PR ready for review
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.
Indeed when PR 294 will be approved we will have a new commit to be used
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.
zephyrproject-rtos/hal_stm32#294 won't be approved before review of current PR has made good progress.
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.
Agreed it's reasonable
drivers/bluetooth/hci/hci_stm32wba.c
Outdated
@@ -462,11 +514,22 @@ static DEVICE_API(bt_hci, drv) = { | |||
.send = bt_hci_stm32wba_send, | |||
}; | |||
|
|||
|
|||
#if defined (CONFIG_PM_DEVICE) |
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.
#if defined (CONFIG_PM_DEVICE) | |
#if defined(CONFIG_PM_DEVICE) |
|
||
CONFIG_PM=y | ||
CONFIG_PM_DEVICE=y | ||
CONFIG_PM_DEVICE_RUNTIME=y | ||
CONFIG_PM_DEVICE_SYSTEM_MANAGED=y | ||
CONFIG_PM_S2RAM=y |
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.
CONFIG_PM_S2RAM
could be imply
ed by CONFIG_PM
at SoC level.
Or even better: imply
only if the corresponding state is status = "okay"
in DTS. (but not sure that status = "disabled"
on a power state really disables it)
soc/st/stm32/stm32wbax/power.c
Outdated
/* MCU enters in low-power modes */ | ||
basepri = __get_BASEPRI(); | ||
__set_BASEPRI(0); | ||
__ISB(); | ||
__DSB(); |
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.
This seems to be mostly identical to arch_irq_lock()
(except we set BASEPRI
to 0):
zephyr/include/zephyr/arch/arm/asm_inline_gcc.h
Lines 44 to 73 in 732a3a5
static ALWAYS_INLINE unsigned int arch_irq_lock(void) | |
{ | |
unsigned int key; | |
#if defined(CONFIG_ARMV6_M_ARMV8_M_BASELINE) | |
#if CONFIG_MP_MAX_NUM_CPUS == 1 || defined(CONFIG_ARMV8_M_BASELINE) | |
key = __get_PRIMASK(); | |
__disable_irq(); | |
#else | |
#error "Cortex-M0 and Cortex-M0+ require SoC specific support for cross core synchronisation." | |
#endif | |
#elif defined(CONFIG_ARMV7_M_ARMV8_M_MAINLINE) | |
key = __get_BASEPRI(); | |
__set_BASEPRI_MAX(_EXC_IRQ_DEFAULT_PRIO); | |
__ISB(); | |
#elif defined(CONFIG_ARMV7_R) || defined(CONFIG_AARCH32_ARMV8_R) \ | |
|| defined(CONFIG_ARMV7_A) | |
__asm__ volatile( | |
"mrs %0, cpsr;" | |
"and %0, #" STRINGIFY(I_BIT) ";" | |
"cpsid i;" | |
: "=r" (key) | |
: | |
: "memory", "cc"); | |
#else | |
#error Unknown ARM architecture | |
#endif /* CONFIG_ARMV6_M_ARMV8_M_BASELINE */ | |
return key; | |
} |
Q: do we really need to duplicate this code here? Can't we just call irq_lock()
/irq_unlock()
?
(but also see remark lower in file, only __DSB()
+ __WFI()
should be needed)
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 think the reasoning here is the following:
- When executing these power management procedures, we do not want any interrupt, regardless of his priority to preempt us
- We would like any possible pending interrupt to transform the Wfi in a nop and skip the power mode entry
For 1), we globally disable interrupts with __disable_irq
For 2), we unmask all interrupt priorities
The original code of Zephyr using irq_lock
, allows effectively to preempt this critical function by high priority irq (priority lower than _EXC_IRQ_DEFAULT_PRIO
).
In turn, since interrupt handlers are calling the power management routines both in direct or regular case, in the middle of this critical routine, not only the high priority interrupt is executed, but also the pm_system_resume
is executed effectively re-enabling all the interrupts.
See
zephyr/arch/arm/core/cortex_m/isr_wrapper.c
Lines 39 to 66 in 553fc84
#ifdef CONFIG_PM | |
/* | |
* All non-ZLI interrupts are disabled when handling idle wakeup. For | |
* tickless idle, this ensures that the calculation and programming of | |
* the device for the next timer deadline is not interrupted. For | |
* non-tickless idle, this ensures that the clearing of the kernel idle | |
* state is not interrupted. In each case, pm_system_resume is called | |
* with non-ZLI interrupts disabled. | |
*/ | |
/* | |
* Disable non-ZLI interrupts to prevent nesting while exiting idle | |
* state. This is only necessary for the Cortex-M because it is the only | |
* ARM architecture variant that automatically enables interrupts when | |
* entering an ISR. | |
*/ | |
unsigned int key = arch_irq_lock(); | |
/* is this a wakeup from idle ? */ | |
/* requested idle duration, in ticks */ | |
if (_kernel.idle != 0) { | |
/* clear kernel idle state */ | |
_kernel.idle = 0; | |
pm_system_resume(); | |
} | |
/* re-enable interrupts */ | |
arch_irq_unlock(key); | |
#endif /* CONFIG_PM */ |
As such, we took inspiration from
zephyr/soc/nordic/nrf54h/power.c
Lines 183 to 208 in 553fc84
void pm_state_set(enum pm_state state, uint8_t substate_id) | |
{ | |
if (state == PM_STATE_SUSPEND_TO_IDLE) { | |
__disable_irq(); | |
sys_trace_idle(); | |
s2idle_enter(substate_id); | |
/* Resume here. */ | |
s2idle_exit(substate_id); | |
sys_trace_idle_exit(); | |
__enable_irq(); | |
} | |
#if defined(CONFIG_PM_S2RAM) | |
else if (state == PM_STATE_SUSPEND_TO_RAM) { | |
__disable_irq(); | |
sys_trace_idle(); | |
s2ram_enter(); | |
/* On resuming or error we return exactly *HERE* */ | |
s2ram_exit(); | |
sys_trace_idle_exit(); | |
__enable_irq(); | |
} | |
#endif | |
else { | |
k_cpu_idle(); | |
} | |
} |
This implementation will remove the feature that allows high priority irq to preempt cpu in these critical routines, but I believe this feature would not work during s2ram procedure execution where being interrupted is a system crash guaranteed.
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.
Thanks for the detailed explanation. As you say yourself, IRQ_ZERO_LATENCY
is not used on STM32 platforms. Since it is (unless I'm misunderstanding things) the only way to have interrupts with priority higher than _EXC_IRQ_DEFAULT_PRIO
, we should not be affected.
This does raise the point that despite the arch_irq_lock()
specification being:
Lines 223 to 226 in 1ee39b9
* This routine disables all interrupts on the CPU. It returns an unsigned | |
* integer "lock-out key", which is an architecture-dependent indicator of | |
* whether interrupts were locked prior to the call. The lock-out key must be | |
* passed to irq_unlock() to re-enable interrupts. |
(Emphasis mine: This routine disables all interrupts on the CPU)
on ARM platforms, this is not respected: ZLI might occur under irq_lock()
... but this is documented, and I would argue it's fine because ZLI should not interact with the OS?
Regardless, adding a __disable_irq()
in pm_state_set()
is at best a band-aid: there remains a gigantic window in pm_system_suspend()
(between arch_irq_lock()
in idle.c
and pm_state_set()
in SoC code) where an interrupt could occur. The fix, if any, would be introducing a new API set to distinguish between disable ALL interrupts
and disable <specified subset> interrupts
(e.g., <specified subset> = all non-ZLI
)
WRT my other remark, I still think __ISB()
is not required.
ARMv8-M Architecture Reference Manual says:
Between any change to special purpose registers which require synchronization or memory-mapped registers and
a subsequent Context synchronization event, it is IMPLEMENTATION DEFINED whether an indirect read of the register by the PE uses the old or new values, and may vary with each use of the unsynchronized value.
and
Except for writes to the CONTROL register, any change to a special-purpose register by a CPS or MSR instruction
is guaranteed:
• Not to affect that CPS or MSR instruction, or any instruction preceding it in program order.
• To be visible to all instructions that appear in program order after the CPS or MSR.
(this latter paragraph is found verbatim in the ARMv7-M Architecture Reference Manual)
My understanding of these paragraphs is that writing to BASEPRI
should not require any synchronization (ISB
), regardless of what the comment in idle.c
may say.
soc/st/stm32/stm32wbax/power.c
Outdated
basepri = __get_BASEPRI(); | ||
__set_BASEPRI(0); | ||
__ISB(); | ||
__DSB(); |
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.
Same remark: can't we use irq_lock()
+ __WFI()
+ irq_unlock()
?
(but also see remark lower in file, only __DSB()
+ __WFI()
should be needed)
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.
See #92847 (comment)
soc/st/stm32/stm32wbax/power.c
Outdated
__WFI(); | ||
|
||
/* MCU exits from low power mode and set back the basepri */ | ||
__set_BASEPRI(basepri); | ||
|
||
return 0; |
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.
- If we fall here we should return an error.
- Could use this routine for entry in Stop mode too (in this case, ignore return value)
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 think you are right in both point
LOG_DBG("Suspend to RAM needs CONFIG_PM_S2RAM to be enabled"); | ||
#endif | ||
case PM_STATE_SUSPEND_TO_RAM: | ||
__disable_irq(); |
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.
pm_system_suspend
which calls pm_state_set
can only be called by the idle thread, which acquires IRQ lock before calling it.
I think this should not be needed? (nor any IRQ lock call in lower layers)
Lines 46 to 74 in 732a3a5
/* Note weird API: k_cpu_idle() is called with local | |
* CPU interrupts masked, and returns with them | |
* unmasked. It does not take a spinlock or other | |
* higher level construct. | |
*/ | |
(void) arch_irq_lock(); | |
#ifdef CONFIG_PM | |
_kernel.idle = z_get_next_timeout_expiry(); | |
/* | |
* Call the suspend hook function of the soc interface | |
* to allow entry into a low power state. The function | |
* returns false if low power state was not entered, in | |
* which case, kernel does normal idle processing. | |
* | |
* This function is entered with interrupts disabled. | |
* If a low power state was entered, then the hook | |
* function should enable interrupts before exiting. | |
* This is because the kernel does not do its own idle | |
* processing in those cases i.e. skips k_cpu_idle(). | |
* The kernel's idle processing re-enables interrupts | |
* which is essential for the kernel's scheduling | |
* logic. | |
*/ | |
if (k_is_pre_kernel() || !pm_system_suspend(_kernel.idle)) { | |
k_cpu_idle(); | |
} | |
#else |
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.
See #92847 (comment)
soc/st/stm32/stm32wbax/power.c
Outdated
*/ | ||
z_arm_configure_static_mpu_regions(); | ||
#endif /* CONFIG_ARM_MPU */ | ||
z_arm_fault_init(); |
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.
z_arm_fault_init(); |
This should not be needed since SCB.CCR
is restored by scb_resume
.
Power management enhancements to support suspend to ram with BLE activity. ST system clock module is no more used. Draft commit still to be refined. Signed-off-by: Alessandro Manganaro <[email protected]>
e119b24
to
a58e6c7
Compare
|
} else { | ||
scm_setwaitstates(RUN); | ||
/* Apply waitsates for HSE32 configuration */ | ||
__HAL_FLASH_SET_LATENCY(FLASH_LATENCY_0); |
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.
Setting latency to 0 will lead to a hard fault, when running on higher clock rates (e.g. PLL at 100MHz)
Latency should only be changed to 1 while entering LP Modes if it is zero before, if not keep it as it is...
stm32_clock_control_init(NULL); | ||
} else { | ||
/* Apply waitsates for HSE32 configuration */ | ||
__HAL_FLASH_SET_LATENCY(FLASH_LATENCY_0); |
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.
can we get here when leaving standby mode? If yes, setting latency to 0 may be the wrong value for th current clock. (see comment for set_mode_stop_exit() below)
Power management has been split between rf driver and system.
SCM is no more used.
This PR is clearly a draft version to highlight where we are are on STDBY topic (working but intensive tests still to be done) and to start a public discussion on open topics we have.