-
Notifications
You must be signed in to change notification settings - Fork 1.4k
ironside: Add a sample that uses the UICR PERIPHCONF infrastructure #24930
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
Conversation
This sample was already reviewed and approved in #24028. But then it got force-pushed out of the PR for reasons I don't recall. |
@nordicjm , @FrancescoSer , @umapraseeda , @AyturkDuzen : You already reviewed this in #24028. Please review again in this PR. |
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:sdk-nrf: PR head: 62617a44e2355dc096a8fe6f166d76c8e318993c more detailssdk-nrf:
zephyr:
Github labels
List of changed files detected by CI (20)
Outputs:ToolchainVersion: f66cf421f3 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
You can find the documentation preview for this PR here. Preview links for modified nRF Connect SDK documents: https://ncsdoc.z6.web.core.windows.net/PR-24930/nrf/releases_and_maturity/releases/release-notes-changelog.html |
b643866
to
bcc7ec3
Compare
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.
Approved but please implement this small suggestion
|
||
.. code-block:: console | ||
|
||
west build -b nrf54h20dk/nrf54h20/cpuapp samples/secondary_boot --sysbuild |
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.
nit. sysbuild flag should not be needed because it is enabled globally (unless a user has changed their local configuration to disable it but that would break basically everything)
# Create empty marker file at configure-time | ||
file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/is_secondary_firmware.txt "") |
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 should go
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.
Hi,
this was discussed earlier in
#24524 (comment)
There I proposed introducing a Kconfig
IS_SECONDARY_IMAGE
Do you approve of this approach?
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 be doing the same as other things, they should not need to set a Kconfig, a function should be doing that for them (and in this case it's not hard because this is being done as a direct image, not as a variant, so you can get sysbuild to force the value of a Kconfig, so a new Kconfig can be added with a description saying informative only, see https://github.com/nrfconnect/sdk-nrf/blob/main/subsys/mcuboot_ids/Kconfig for an example), so you can do that in the cmake file in the sysbuild folder
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 will look into if what you are proposing is possible.
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.
sysbuild/Kconfig.secondary
Outdated
|
||
config SECONDARY_IMAGE_NAME | ||
string | ||
default "secondary" if SECONDARY_BOOT_GEN_UICR |
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.
names should match i.e. boot_gen_uicr
sysbuild/Kconfig.secondary
Outdated
|
||
config SECONDARY_IMAGE_PATH | ||
string | ||
default "${ZEPHYR_NRF_MODULE_DIR}/samples/ironside_se/secondary_boot_gen_uicr/secondary" if SECONDARY_BOOT_GEN_UICR |
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.
hmm yeah this is a bit of a problem, the names should match so the final folder name should be the name of the image and should be the name in the Kconfig to use it
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.
Sorry I don't understand what you are saying.
Also, I wonder if there has been a misunderstanding from the beginning.
Are you aware that the image in
~/ncs/nrf/samples/ironside_se/secondary_boot_gen_uicr/secondary
is not a reusable image, like mcuboot, NSIB, etc.
But just a demonstration of how a customer could create their own image that is booted in secondary mode?
It is specific to the sample in
~/ncs/nrf/samples/ironside_se/secondary_boot_gen_uicr
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 that's the case then the list should be empty in sysbuild, then appended to by the application - see https://github.com/nrfconnect/sdk-nrf/tree/main/samples/bluetooth/direct_test_mode for an example, then it can be documented how users would extend the list themselves, https://ncsdoc.z6.web.core.windows.net/latest/nrf/app_dev/config_and_build/sysbuild/sysbuild_images.html#adding-custom-images can be used as a base thing to copy
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.

Note that I don't need
~/ncs/nrf/samples/ironside_se/secondary_boot_gen_uicr/secondary
to be available to multiple projects.
I believe that if I use "Adding custom images" as a base, but remove the code related to "letting an image be available to multiple projects", then I will end up with a simple ExternalZephyrProject_Add call.
Would this approach be acceptable?
It is the approach that I believe the customer would prefer as reference/sample code for how he can create a
secondary image.
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.
the point is to configure it for them though right, so they can pick an image (i.e. their own image) and the correct configuration will be applied, so that's not going to work without doing it like 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.
No, it is not desired that they can pick via Kconfig their own image for the primary or for the secondary image.
The goal here is to demonstrate to the customer how they can create their own primary image and their own secondary image.
I have pushed a new revision now which I feel demonstrates this quite well.
9de05ec
to
6446558
Compare
The following west manifest projects have changed revision in this Pull Request:
✅ All manifest checks OK Note: This message is automatically posted and updated by the Manifest GitHub Action. |
6446558
to
fc2268d
Compare
* *Primary Image*: Runs initially on the application core (``cpuapp``), prints a ``Hello World`` message, and includes stub functions for the Secure Domain service calls to boot the secondary image. | ||
|
||
* *Secondary Image*: Runs on the same application core (``cpuapp/secondary``) after the primary image initiates the boot sequence and prints its own ``Hello World`` message. |
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.
* *Primary Image*: Runs initially on the application core (``cpuapp``), prints a ``Hello World`` message, and includes stub functions for the Secure Domain service calls to boot the secondary image. | |
* *Secondary Image*: Runs on the same application core (``cpuapp/secondary``) after the primary image initiates the boot sequence and prints its own ``Hello World`` message. | |
* *Primary Image*: Runs initially on the application core (``cpuapp``), prints a ``Hello World`` message, and includes stub functions for the Secure Domain service calls to boot the secondary image. | |
* *Secondary Image*: Runs on the same application core (``cpuapp/secondary``) after the primary image initiates the boot sequence and prints its own ``Hello World`` message. |
fc2268d
to
410fc80
Compare
c15013b
to
d32f34c
Compare
d32f34c
to
da1d974
Compare
Automatically created by Github Action Signed-off-by: Sebastian Bøe <[email protected]>
Add a sample that uses the UICR PERIPHCONF infrastructure. Signed-off-by: Sebastian Bøe <[email protected]>
da1d974
to
62617a4
Compare
Add a sample that uses the UICR PERIPHCONF infrastructure.