Skip to content
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

bootloaders/riotboot: don't change CPU/pin state #21097

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Dec 19, 2024

Contribution description

A reoccurring issue is that some board sets way too much pin state in board_init() and this carries over to riotboot where it is then frozen forever.

But riotboot has no business to set any hw state, it just needs to compare two values in flash and jump to an address - we don't need to touch any hardware registers for that.

Testing procedure

An app that makes use of riotboot, e.g. examples/suit_update still boots as before.

This was verified on same54-xpro.

Issues/PRs references

@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: cpu Area: CPU/MCU ports labels Dec 19, 2024
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 19, 2024
@riot-ci
Copy link

riot-ci commented Dec 19, 2024

Murdock results

✔️ PASSED

e7205d3 cpu/cortexm_common: add option to skip hardware init

Success Failures Total Runtime
10246 0 10248 17m:35s

Artifacts

@benpicco benpicco marked this pull request as ready for review December 19, 2024 20:56
@benpicco benpicco changed the title cpu/cortexm_common: skip hw init if RIOTBOOT_MINIMAL is set cpu/cortexm_common: skip hw init if RIOTBOOT_MINIMAL is set Dec 19, 2024
@maribu
Copy link
Member

maribu commented Dec 19, 2024

I have some concerns:

  1. The name RIOTBOOT_MINIMAL matches the goal, but not the effect. Something like DISABLE_BOARD_INIT or DISABLE_CPU_INIT would make it a lot more obvious what is happening
  2. Wouldn't it be better to be more fine grained? E.g. being able to disable board_init() and cpu_init() separately.
  3. Are we sure we can drop cpu_init()? Looking through a bit of code I see things like disabling or feeding a WDT. I think a user knowing a specific board well may be able to conclude that cpu_init() is optional. I guess board_init() is pretty likely to not be needed for riotboot.

@benpicco
Copy link
Contributor Author

  1. The name RIOTBOOT_MINIMAL matches the goal, but not the effect. Something like DISABLE_BOARD_INIT or DISABLE_CPU_INIT would make it a lot more obvious what is happening

One the one hand I didn't want to give application developers ideas, but then again this is more descriptive when writing cpu/ code.

  1. Wouldn't it be better to be more fine grained? E.g. being able to disable board_init() and cpu_init() separately.

I don't think there is a use case, but it makes sense from a consistency point of view when going with the descriptive naming.

  1. Are we sure we can drop cpu_init()? Looking through a bit of code I see things like disabling or feeding a WDT. I think a user knowing a specific board well may be able to conclude that cpu_init() is optional. I guess board_init() is pretty likely to not be needed for riotboot.

Yes, for cpu_init() the CPU ran just fine up to that point - in fact we don't want to change anything about the CPU state.
All riotboot does is compare two version numbers in ROM, we don't need to feed any watchdog for that. We want to leave everything very much like it is at boot-up and leave the proper init to the real firmware.

@dylad
Copy link
Member

dylad commented Dec 20, 2024

Maybe we could just provide some hooks to the user (empty by default) so they can do whatever they want if they have a special use case for that.
Otherwise I like the general idea here.

@benpicco benpicco changed the title cpu/cortexm_common: skip hw init if RIOTBOOT_MINIMAL is set bootloaders/riotboot: don't change CPU/pin state Dec 20, 2024
@maribu
Copy link
Member

maribu commented Dec 20, 2024

Looks good to me.

Would you mind adding some documentation with a friendly warning that those two options are experimental for now and users should only use them if they really know what they are doing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants