Skip to content

sysbuild: mcuboot: direct_xip: enable custom overlay for secondary slot #24561

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+14 −1
Open

sysbuild: mcuboot: direct_xip: enable custom overlay for secondary slot #24561

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions sysbuild/Kconfig.mcuboot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,14 @@ config MCUBOOT_BUILD_DIRECT_XIP_VARIANT
help
Will build the alternative slot (variant) image of the main application.

config MCUBOOT_BUILD_DIRECT_XIP_VARIANT_CUSTOM_OVERLAY
string "Use a custom DTS overlay for the secondary slot image"
depends on MCUBOOT_BUILD_DIRECT_XIP_VARIANT || !PARTITION_MANAGER
help
When this value is not populated, the default DTS overlay is used for the secondary slot.
Use this Kconfig option when you want to set a custom overlay file for the secondary slot
image.
Comment on lines +21 to +27
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not acceptable. If you want to append a new file then doing that should work via command line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't want to use a CLI parameter to override the DTS overlay file because the build command becomes complex (actually, you need to replace overlays for two secondary variants of the following images: the default application image and IPC radio).

west build -p -b nrf54h20dk/nrf54h20/cpuapp -- -Dmcuboot_secondary_app_DTC_OVERLAY_FILE=secondary.overlay -Dipc_radio_secondary_app_DTC_OVERLAY_FILE=secondary.overlay

There is also a guideline/requirement from PMT to make the default build command work in a given project. So the following command should work without any extra params:

west build -p -b nrf54h20dk/nrf54h20/cpuapp

Since there is already a mechanism that injects the overlay file from an arbitrary location ( nrf/subsys/mcuboot/mcuboot_secondary_app.overlay), can't we simply introduce the possibility to change the path and pass your own DTS overlay?

Also, right now, you can't remove the nrf/subsys/mcuboot/mcuboot_secondary_app.overlay overlay, which is added to your secondary images automatically.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kapi-no but the sample already builds out-of-the-box, correct ?

afaict the mcuboot_secondary_app.overlay is always applied, so what you're tryin (correct me if i'm wrong) is for users to choose their own overlay.

For this, as @nordicjm points out, sysbuild already supports the use of -D<image>_DTC_OVERLAY_FILE=<user-custom-file>.overlay, so why not change the code into dropping add_overlay_dts(${image} "${secondary_overlay}") if custom overlay for image is set ?

Instead of inventing Kconfig setting for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kapi-no but the sample already builds out-of-the-box, correct ?

In the simple MCUboot direct-xip use case scenario, where we have a default application image (built for slot0) and its secondary counterpart (built for slot1) -> yes.

However, when each slot holds two images (merged binary configuration for the nRF54H20): the default application image and the IPC radio image -> the default overlay configuration from the nrf/subsys/mcuboot/mcuboot_secondary_app.overlay is broken.

See the following comment for more context: #24494 (comment)

afaict the mcuboot_secondary_app.overlay is always applied, so what you're tryin (correct me if i'm wrong) is for users to choose their own overlay.

Yes

For this, as @nordicjm points out, sysbuild already supports the use of -D<image>_DTC_OVERLAY_FILE=<user-custom-file>.overlay, so why not change the code into dropping add_overlay_dts(${image} "${secondary_overlay}") if custom overlay for image is set ?

Instead of inventing Kconfig setting for this.

We always strive to have the default build command working for each supported target. Requiring extra parameters breaks this requirement.

There is also a guideline/requirement from PMT to make the default build command work in a given project. So the following command should work without any extra params:

west build -p -b nrf54h20dk/nrf54h20/cpuapp

Copy link
Contributor

Choose a reason for hiding this comment

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

afaict the mcuboot_secondary_app.overlay is always applied, so what you're tryin (correct me if i'm wrong) is for users to choose their own overlay.

Yes

For this, as @nordicjm points out, sysbuild already supports the use of -D_DTC_OVERLAY_FILE=.overlay, so why not change the code into dropping add_overlay_dts(${image} "${secondary_overlay}") if custom overlay for image is set ?

Instead of inventing Kconfig setting for this.

We always strive to have the default build command working for each supported target. Requiring extra parameters breaks this requirement.

but ensuring that a default build command for a given target always work is not necessarily the same as supporting custom user provided files.

Is the nrf54h20dk/nrf54h20/cpuapp not working with the currently used / default applied mcuboot_secondary_app.overlay file ?

In short, does west build -p -b nrf54h20dk/nrf54h20/cpuapp --sysbuild fail today ?

There is no Jira task reference provided, so I'm a bit unsure exactly what you're trying to solve here.

  • Working sample out-of-the-box for west build -p -b nrf54h20dk/nrf54h20/cpuapp --sysbuild ?
    If this is not working, then we should have an issue describe what and why this is not working.
  • Ability for our downstream users / customers to provide their own overlays.
    Of course a user wanting to provide their own overlay must be expected to do some work of their own.
    Even with the use if a Kconfig proposol in this PR they would need to set the value somehow, for example:
    -DMCUBOOT_BUILD_DIRECT_XIP_VARIANT_CUSTOM_OVERLAY=<file>.overlay

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally, the ROM footprint statistics for the nRF54H20 images in the merged binary approach are incorrect. More details can be found in the following ticket:

NCSDK-35489

The code partition for the application image is defined in such a way that it also contains the radio partition.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand NCSDK-35489 then the issue exists for all samples building for nRF54h20dk, so isn't it simply the original mcuboot_secondary_app.overlay intended for the nrf54h20dk which is wrong and should be fixed ?
ef85447

Secondly, if a sample wants to provide custom overlays for images and have those applied per default, then that is also possible. Can even be board specific: https://docs.zephyrproject.org/latest/build/sysbuild/index.html#zephyr-application-configuration

If none of the existing solutions are suitable, then I believe it will be more efficient if you set up a meeting and invite relevant parties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand NCSDK-35489 then the issue exists for all samples building for nRF54h20dk, so isn't it simply the original mcuboot_secondary_app.overlay intended for the nrf54h20dk which is wrong and should be fixed ?

Yes, at least with the merged binary approach. I haven't tested the DFU solution with isolated binaries, as we want to use one version number for the image set (application + radio image) to minimize complexity in our project.

Secondly, if a sample wants to provide custom overlays for images and have those applied per default, then that is also possible. Can even be board specific: https://docs.zephyrproject.org/latest/build/sysbuild/index.html#zephyr-application-configuration

If none of the existing solutions are suitable, then I believe it will be more efficient if you set up a meeting and invite relevant parties.

Well, I am aware of the application configuration directory solution. We use it in the nRF Desktop app where I initially want to use the suggested change in this PR. I know I can create the configuration from scratch for both the mcuboot_secondary_app and the ipc_radio_secondary_app images. However, this requires copying over 1000 lines of configuration files (at least for the mcuboot_secondary_app) to change one DTS node. And even then, the default file - nrf/subsys/mcuboot/mcuboot_secondary_app.overlay - will be applied.


menuconfig MCUBOOT_HARDWARE_DOWNGRADE_PREVENTION
bool "Downgrade prevention using hardware security counters"
depends on (SOC_NRF5340_CPUAPP || SOC_SERIES_NRF91X || SOC_SERIES_NRF54LX)
Expand Down
7 changes: 6 additions & 1 deletion sysbuild/mcuboot.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,12 @@ if(SB_CONFIG_MCUBOOT_BUILD_DIRECT_XIP_VARIANT)
)
else()
UpdateableImage_Add(APPLICATION ${image} GROUP "VARIANT")
set(secondary_overlay "${ZEPHYR_NRF_MODULE_DIR}/subsys/mcuboot/mcuboot_secondary_app.overlay")

string(CONFIGURE "${SB_CONFIG_MCUBOOT_BUILD_DIRECT_XIP_VARIANT_CUSTOM_OVERLAY}" secondary_overlay)
if("${secondary_overlay}" STREQUAL "")
set(secondary_overlay "${ZEPHYR_NRF_MODULE_DIR}/subsys/mcuboot/mcuboot_secondary_app.overlay")
endif()

add_overlay_dts(${image} "${secondary_overlay}")
endif()

Expand Down