-
Notifications
You must be signed in to change notification settings - Fork 8k
STM32 Bootloader support with retained ram boot mode infrastructure #84272
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?
STM32 Bootloader support with retained ram boot mode infrastructure #84272
Conversation
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
Would like if we could re-open this... I've dug into this, and have this working reliably now on WB55RG, F072, and F411 hardware here. Key things:
Notes: Currently, I'm forcing off the RETENTION and RETAINED_MEM mutex Kconfig symbols to test this, but based on @henrikbrixandersen advice I think the more correct way is to update those existing portions of the code to skip the mutex logic if |
b34e786
to
87e9014
Compare
Actually, I only suggested to skip the mutex locking in the retention subsystem when called in pre-kernel mode. I don't think it will always be safe to call these APIs in interrupt context due to the nature of the various backends. I've opened a simple PR for this, which I actually prepared at ZDS, but never got around to pushing: #97012 |
return 0; | ||
} | ||
|
||
SYS_INIT(bootloader_check_boot_init, EARLY, 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.
Won't you run into initialisation order issues here? The retention subsystem (and its backend drivers) are in post-kernel.
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.
So.... Yes, this is challenging. The stm32 bootloader, based on my research, can be persnickety if you try to jump to it with various things already init'd, like clocks, etc.
To be as robust as possible, I wanted to do this super early after a reset. The side effect is consuming device(s) that aren't technically in init'd yet, but we get lucky that the RAM based retained memory driver I'm using for my testing manages to work in this scenario.
Some options:
- Move this stm32 bootloader code later, and enhance the code to properly reset/unload things before jumping.
- Move retention/retained memory bits earlier.
- Leave as is and it happens to work.
There's no real clear winner here, afaict
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.
You should use soc hook instead
zephyr/include/zephyr/platform/hooks.h
Line 30 in 4ef1163
void soc_reset_hook(void); |
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.
Some options:
- Move this stm32 bootloader code later, and enhance the code to properly reset/unload things before jumping.
That's certainly an option, but it might be complicated to deinitialize everything.
- Move retention/retained memory bits earlier.
I think this would be a good solution, but I doubt we can move it all the way to the EARLY
initialisation level. @nordicjm: Any thoughts on this one?
- Leave as is and it happens to work.
This is not an option. We cannot disregard the whole device model and initialisation order and rely on something that just happens to work.
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.
Some options:
- Move this stm32 bootloader code later, and enhance the code to properly reset/unload things before jumping.
That's certainly an option, but it might be complicated to deinitialize everything.
Yeah. Complicated, but possibly what we have to do.
- Move retention/retained memory bits earlier.
I think this would be a good solution, but I doubt we can move it all the way to the
EARLY
initialisation level. @nordicjm: Any thoughts on this one?
The concern I have here, some platforms might need other clocks/whatever initialized for their retained mem driver implementation to work, and would be complicated by having this earlier.
Perhaps we could have the level for retained memory/retention configurable as well?
- Leave as is and it happens to work.
This is not an option. We cannot disregard the whole device model and initialisation order and rely on something that just happens to work.
Fair. Did say no good options...
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.
It should not be moved to earlier than POST_KERNEL
to allow for drivers to init, e.g. it might need i2c. There already is driver deinit functionality and some drivers have it so makes sense to use that
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.
So far, the cleanest, but perhaps annoying to get right for all platforms, would be to move this init hook to a later level/priority that is correct from a sequence standpoint, and see what de-init is really needed. I will experiment with this.
If device PM is enabled, we could potentially leverage that to shut down any devices that are already loaded? Lacking that, I'm not sure how best to do this generically without just making the STM32 LL calls ourselves like LL_RCC_DeInit
, etc.
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 should not use PM for this, but rather device_deinit()
: https://docs.zephyrproject.org/apidoc/latest/group__device__model.html
We may need to implement support for this in various STM32 drivers first, though.
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.
Ok, based on some discussions in Discord, and suggestions from @erwango I've tried the second option and pushed here for folks to consider.
In particular:
- Add new Kconfig options to force the retained mem (Just Zephyr RAM driver right now) and retention init level to
PRE_KERNEL_1
. select
those options when enabling theSTM32_BOOTLOADER
Kconfig, and then default the init priorities for those two to sequence them properly before the STM32 bootloaderSYS_INIT
call.- I did need to add some small code additions to disable the MPU before jumping into the bootloader if it's enabled, since that gets enabled in between
EARLY
andPRE_KERNEL_1
stages, and the other drivers can at best be moved to the start of thePRE_KERNEL_1
init level. I used a non-published ARM arch API to do so, but I'd love some feedback on this specific aspect. - Manually implemented the same changes from retention: skip mutex lock/unlock when in pre-kernel #97012 here just for easier testing. It makes the same change found there in the retention subsystem to the Zephyr RAM retained mem driver. This allows the STM32 bootloader to check the boot mode earlier in the init sequence without the mutex locks causing issues.
I have tested the current branch with this diff:
diff --git a/samples/hello_world/src/main.c b/samples/hello_world/src/main.c
index c550ab461cb..ea956f252ca 100644
--- a/samples/hello_world/src/main.c
+++ b/samples/hello_world/src/main.c
@@ -5,10 +5,18 @@
*/
#include <stdio.h>
+#include <zephyr/retention/bootmode.h>
+#include <zephyr/sys/reboot.h>
int main(void)
{
printf("Hello World! %s\n", CONFIG_BOARD_TARGET);
+ bootmode_set(BOOT_MODE_TYPE_BOOTLOADER);
+ printf("Set the boot mode\n");
+
+ k_sleep(K_SECONDS(10));
+ sys_reboot(0);
+
return 0;
}
and built with:
west build -p -d build/wb55-bootloader -b nucleo_wb55rg samples/hello_world -- -DCONFIG_REBOOT=y -DCONFIG_RETAINED_MEM=y -DCONFIG_RETENTION=y -DCONFIG_RETENTION_BOOT_MODE=y -DCONFIG_STM32_BOOTLOADER=y
Which on my WG55RG Nucleo kit "just works" and restarts into the ROM bootloader as expected.
I've similarly tested on an F411 target, will try f072 soon as I have a sec.
|
||
description: ST STM32 ROM Bootloader Details | ||
|
||
compatible: "st,stm32-bootloader" |
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.
Not needed and not a good use of DT, use a Zephyr memory-region instead.
return 0; | ||
} | ||
|
||
SYS_INIT(bootloader_check_boot_init, EARLY, 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.
You should use soc hook instead
zephyr/include/zephyr/platform/hooks.h
Line 30 in 4ef1163
void soc_reset_hook(void); |
87e9014
to
dfe31af
Compare
dfe31af
to
5a1852b
Compare
drivers/retained_mem/Kconfig
Outdated
config RETAINED_MEM_INIT_LEVEL_PRE_KERNEL_1 | ||
bool "Retained memory devices init during PRE_KERNEL_1" | ||
help | ||
Retained memory devices initialization during PRE_KERNEL_1 level |
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 really do not agree with this, this just makes blind assumptions about drivers which is impossible depending upon the driver
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 it depends on the driver, that is just that some driver may not be compatible, but I don't see it as a major issue.
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 add a new Kconfig symbol, RETAINED_MEM_SUPPORTS_PRE_KERNEL_INIT
that any compatible drivers could select, and have this symbol depend on that. Likewise, the retention config would also depend on this symbol, so the entire "do init early" could only be enabled with a driver that allows it.
Would that address your concerns?
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.
A system can potentially have multiple, different retained memory drivers, some which support being initialised early; some which does not.
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.
Ok, so given the concerns here (and valid point from @henrikbrixandersen on different retained mem drivers), I've gone back to trying option #1 as discussed in #84272 (comment) , having inadvertently solved at least some of the "deinit" issues by adding the code to disable the MPU before jumping.
Moving the retained mem, retention, and new bootloader check to early in POST_KERNEL is now working, with one additional "cleanup" before the jump which was required (on the targets I can test here), which was disabling SysTick as well.
If that's all the cleanup we need, in order for this to be reliable enough on enough STM32 targets, then I would suggest we don't need to complicate this with invoking device_deinit
on a bunch of devices, but I'm happy to implement that if we decide this is the high level approach we want to take, and it warrants that additional logic.
subsys/retention/Kconfig
Outdated
config RETENTION_INIT_LEVEL_PRE_KERNEL_1 | ||
bool "Retention device init during PRE_KERNEL_1" | ||
help | ||
Retention device initialization during PRE_KERNEL_1 level |
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.
Likewise with this
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.
Do not agree with changing init level of retention/retained memory
@rjayles Could you please have a look at this PR? |
Add the ROM bootloader devicetree details for a stm32f072 SoC. Signed-off-by: Peter Johanson <[email protected]>
Add the STM32 bootloader details to the relevant STM32WB .dtsi file. Signed-off-by: Peter Johanson <[email protected]>
Setup a section of retained memory for boot mode retention on the Nucleo WB55RG board. Signed-off-by: Peter Johanson <[email protected]>
Add the ROM bootloader devicetree details for a stm32f411 SoC. Signed-off-by: Peter Johanson <[email protected]>
For early boot mode checking during initalization, skip mutex lock/unlock if in pre-kernel state for the retention and retained mem zephyr ram driver. Signed-off-by: Peter Johanson <[email protected]>
Add jump to ROM bootloader for STM32 when bootmode is set via retention boot mode feature. Signed-off-by: Peter Johanson <[email protected]>
5a1852b
to
741435c
Compare
|
This is the initial work to support entering the STM32 built-in bootloader when the boot mode is set with the retained mem APIs. The PR assumes that a given platform will have the ROM bootloader addresss specified in the DTS using the values from AN2606 as a reference for the given target.
The code in question is working great on f072, but probably imperfect; I'd love some testing input to try to get it more bulletproof on other STM32 targets.
My targets stm32f072 devices work perfectly with this change. My stm32f401 and stm32f411 devices do not, they reset and attempt to jump to the bootloader but fail to enumerate. Sadly, neither of my f4 devices have SWD pins accessible, so I've not been able to further diagnose for those targets. Given that I'm only including the DTS changes to enable this for f072, pending further work.