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

Fix nordic boards #1984

Closed
wants to merge 4 commits into from
Closed

Fix nordic boards #1984

wants to merge 4 commits into from

Conversation

nordicjm
Copy link
Collaborator

Fixes building MCUboot for this board

Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
Fixes building MCUboot for this board

Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
Fixes building MCUboot for this board

Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
@JordanYates
Copy link

JordanYates commented Jun 22, 2024

Not doubting this fixes the issue of these 3 boards, but it doesn't seem like it resolves the root issue.
Put another way, if I duplicated the board with a new name the same issue would appear again.

If the bootloader is failing to compile because USB requires multithreading, doesn't that mean either MCUboot shouldn't be disabling multithreading OR the board shouldn't always be enabling USB?

@nordicjm
Copy link
Collaborator Author

Put another way, if I duplicated the board with a new name the same issue would appear again.

That's the same as if you duplicated any board for any sample in the whole of zephyr that has an overlay and you didn't copy the overlay. If you duplicate a board and want to run a sample that worked on the original board, you need to copy the overlay. The bug here was that there boards should have had overlays to begin with which this PR addresses.

If the bootloader is failing to compile because USB requires multithreading, doesn't that mean either MCUboot shouldn't be disabling multithreading OR the board shouldn't always be enabling USB?

The USB only refers to thingy53 since the rest are GPIO/SPI NOR drivers, and for thingy53 the configuration was added originally then it was removed and moved to ncs:

#1122
#1515

The original addition was wrong because it never worked in zephyr anyway and should have contained the overlay in this PR.

@JordanYates
Copy link

That's the same as if you duplicated any board for any sample in the whole of zephyr that has an overlay and you didn't copy the overlay. If you duplicate a board and want to run a sample that worked on the original board, you need to copy the overlay.

Agreed.

The bug here was that there boards should have had overlays to begin with which this PR addresses.

IMO this comes down to which situation we prefer:

  • MULTITHREADING=n, every board that has external flash requires a patch to mcuboot
  • MULTITHREADING=y, boards don't need patches to work, optimizations can still be enabled for those who want them.

I am just making the argument that already exists on the option:

mcuboot/boot/zephyr/Kconfig

Lines 745 to 748 in 9ae634f

# If you know for sure that your hardware will work, you can default
# it to n here. Otherwise, having it on by default makes the most
# hardware work.
config MULTITHREADING

We are not in the first situation. We are instead sure that hardware does NOT work, as evidenced by the issue existing.
If individual boards want to disable multithreading to get ROM savings, it is trivial to do so in the board definition.

@nordicjm
Copy link
Collaborator Author

The actual issue is that some drivers need semaphore/mutexes which thus need multithreading, some can use them optionally if the option is enabled, and some don't use it at all. Drivers started selecting things instead of depending upon them, so the question really should be why do drivers that require multithreading not select it?

Copy link
Member

@d3zd3z d3zd3z left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks find, but there should be a release note stub.

Adds a note that building for these boards has been fixed

Signed-off-by: Jamie McCrae <jamie.mccrae@nordicsemi.no>
@nordicjm nordicjm requested a review from d3zd3z June 28, 2024 08:57
@nordicjm nordicjm mentioned this pull request Jul 10, 2024
@nordicjm nordicjm closed this Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: zephyr Affects the Zephyr port
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mcuboot: nordic development kits fail to compile
4 participants