Add idle relocated TCM multicore test for nRF54H20 #25737
Conversation
NordicBuilder
added
doc-required
|
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. |
There was a problem hiding this comment.
Pull request overview
This PR adds a new multicore idle test sample for the nRF54H20 platform that demonstrates firmware relocation to the radio core's TCM (Tightly Coupled Memory). The implementation introduces a radio loader pattern where firmware is copied from MRAM to TCM at runtime for optimal performance. The PR also extends the sysbuild infrastructure to support building the radio TCM loader with MCUboot integration.
Key Changes:
- Introduces a two-stage boot process for the radio core using a loader that copies firmware from MRAM to TCM
- Adds sysbuild infrastructure (
radioloader.cmake,Kconfig.radioloader) to integrate the radio loader automatically - Implements automatic firmware relocation using Zephyr's
CONFIG_BUILD_OUTPUT_ADJUST_LMAfeature via devicetree chosen nodes
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| west.yml | Updates Zephyr revision to pull request branch |
| sysbuild/radioloader.cmake | Adds radio loader sysbuild integration |
| sysbuild/Kconfig.radioloader | Defines Kconfig options for radio loader |
| sysbuild/Kconfig.netcore | Adds custom radio option to netcore choices |
| sysbuild/Kconfig.sysbuild | Includes radioloader Kconfig file |
| sysbuild/CMakeLists.txt | Includes radioloader CMake file |
| samples/nrf54h20/radio_loader/src/main.c | Implements loader that copies firmware from MRAM to TCM |
| samples/nrf54h20/radio_loader/CMakeLists.txt | Build configuration for radio loader |
| samples/nrf54h20/idle_relocated_tcm/* | New sample demonstrating multicore idle with TCM relocation |
| cmake/sysbuild/sign_nrf54h20.cmake | Updates to support fw-to-relocate chosen node |
| cmake/modules/kconfig.cmake | Adds support for fw-to-relocate in variant image configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -137,11 +142,13 @@ config NETCORE_IMAGE_NAME | |||
| default "rpc_host" if NETCORE_RPC_HOST | |||
| default "802154_rpmsg" if NETCORE_802154_RPMSG | |||
| default "ipc_radio" if NETCORE_IPC_RADIO | |||
| default "remote_rad" if CUSTOM_RADIO | |||
| help | |||
| Name of netcore image. | |||
|
|
|||
| config NETCORE_IMAGE_PATH | |||
| string | |||
There was a problem hiding this comment.
Duplicate string type declaration. Line 150 should be removed as the type and prompt are correctly defined on line 151.
| string |
There was a problem hiding this comment.
I think that string "..." if OPTION will be enough.
i've seen similar thing here: https://github.com/nrfconnect/sdk-nrf/blob/main/subsys/dult/Kconfig#L181
CI InformationTo view the history of this post, click the 'edited' button above Inputs:Sources:sdk-nrf: PR head: 5d0da1b2cb47a78ff3fde291d95c77fcfffa0c70 more detailssdk-nrf:
Github labels
List of changed files detected by CI (35)Outputs:ToolchainVersion: 964ddb2c70 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
| sysbuild: true | ||
| tags: | ||
| - ci_build | ||
| - ci_tests_benchmarks_multicore |
There was a problem hiding this comment.
wrong CI tag, it must follow path where tests/sample is located e.g. ci_samples_nrf54h20
This means, new tags should be created.
mkapala-nordic
left a comment
mkapala-nordic
left a comment
There was a problem hiding this comment.
Some initial comments
| @@ -0,0 +1,70 @@ | |||
| /* | |||
There was a problem hiding this comment.
is there a reason why the optimized radio_loader configuration (prj.conf) is not already present here?
it would be nice to have it optimized from the start or to have reference how to configure it here.
There was a problem hiding this comment.
I removed it as it will never be used anyway as you have to place it in app/sysbuild/radio_loader/prj.conf - I don't want to duplicate configuration - but if you think it is worth doing I can add it back there.
There was a problem hiding this comment.
I assume that there is no way to source this prj.conf always and just overlying it in the app if needed?
Do we need to modify prj.conf file in the app or the DTS overlay is enough?
Can't we apply only the overlay for it in the same way as sysbuild/radio_loader_secondary_app.overlay?
|
Rebased |
NordicBuilder
removed
the
changelog-entry-required
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
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-25737/nrf/releases_and_maturity/releases/release-notes-changelog.html |
Memory footprint analysis revealed the following potential issuesapplications.nrf_desktop.zdebug.usb_next[nrf52840gmouse/nrf52840]: ROM size increased by 640[B] in comparison to the main[b708336] branch. - link (cc: @nrfconnect/ncs-si-bluebagel) Note: This message is automatically posted and updated by the CI (latest/sdk-nrf/PR-25737/13) |
Update sdk-zephyr revision and enhance Kconfig handling. If the 'fw-to-relocate' chosen node exists, the firmware will: - automatically calculate the Load Memory Address (LMA) adjustment for firmware relocation. - Modified Kconfig handling in cmake modules to accommodate a new property "fw-to-relocate". - Ensured backward compatibility by falling back to "zephyr,code-partition" if ther is not property "fw-to-relocate". JIRA: NCSDK-36461 Signed-off-by: Jan Zyczkowski <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dt_chosen(fw_to_relocate_property PROPERTY "fw-to-relocate") | ||
| if("${line}" MATCHES "^CONFIG_FLASH_LOAD_OFFSET=.*$") | ||
| string(REGEX REPLACE "CONFIG_FLASH_LOAD_OFFSET=(.*)" "CONFIG_FLASH_LOAD_OFFSET=${code_partition_offset}" line ${line}) | ||
| # Change the CONFIG_FLASH_LOAD_OFFSET value only if the fw_to_relocate_property is empty - meaning that the firmware is not being relocated. | ||
| if("${fw_to_relocate_property}" STREQUAL "") | ||
| string(REGEX REPLACE "CONFIG_FLASH_LOAD_OFFSET=(.*)" "CONFIG_FLASH_LOAD_OFFSET=${code_partition_offset}" line ${line}) | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
The dt_chosen() call for fw_to_relocate_property is executed inside the loop for every line in dotconfig_content. This is inefficient as the chosen node value doesn't change between iterations. Move the dt_chosen(fw_to_relocate_property PROPERTY \"fw-to-relocate\") call outside the loop, before line 50.
It is very simple actually. You can disable CONFIG_XIP for image that is relocated to RAM memory. Then Zephyr build system will build image which is automatically linked against the |
| #. Radio Loader - A small bootloader that runs on the radio core, copies firmware from MRAM to TCM, and jumps to it. | ||
| #. Remote Firmware - The actual application that runs from TCM after being loaded. |
There was a problem hiding this comment.
| #. Radio Loader - A small bootloader that runs on the radio core, copies firmware from MRAM to TCM, and jumps to it. | |
| #. Remote Firmware - The actual application that runs from TCM after being loaded. | |
| * Radio Loader - A small bootloader that runs on the radio core, copies firmware from MRAM to TCM, and jumps to it. | |
| * Remote Firmware - The actual application that runs from TCM after being loaded. |
| #. TCM regions: | ||
|
|
||
| .. code-block:: devicetree | ||
| cpurad_ram0: sram@23000000 { | ||
| compatible = "mmio-sram"; | ||
| reg = <0x23000000 0x20000>; /* 128 KB for code */ | ||
| }; | ||
| cpurad_data_ram: sram@23020000 { | ||
| compatible = "mmio-sram"; | ||
| reg = <0x23020000 0x10000>; /* 64 KB for data */ | ||
| }; |
There was a problem hiding this comment.
| #. TCM regions: | |
| .. code-block:: devicetree | |
| cpurad_ram0: sram@23000000 { | |
| compatible = "mmio-sram"; | |
| reg = <0x23000000 0x20000>; /* 128 KB for code */ | |
| }; | |
| cpurad_data_ram: sram@23020000 { | |
| compatible = "mmio-sram"; | |
| reg = <0x23020000 0x10000>; /* 64 KB for data */ | |
| }; | |
| #. TCM regions: | |
| .. code-block:: devicetree | |
| cpurad_ram0: sram@23000000 { | |
| compatible = "mmio-sram"; | |
| reg = <0x23000000 0x20000>; /* 128 KB for code */ | |
| }; | |
| cpurad_data_ram: sram@23020000 { | |
| compatible = "mmio-sram"; | |
| reg = <0x23020000 0x10000>; /* 64 KB for data */ | |
| }; |
This doesn't render as expected.
| #. Build the firmware for the secondary app slot, increase the version number in the :file:`prj.conf` file (uncomment the line): | ||
|
|
||
| .. code-block:: kconfig | ||
| CONFIG_MCUBOOT_IMGTOOL_SIGN_VERSION="0.0.1+0" |
There was a problem hiding this comment.
| #. Build the firmware for the secondary app slot, increase the version number in the :file:`prj.conf` file (uncomment the line): | |
| .. code-block:: kconfig | |
| CONFIG_MCUBOOT_IMGTOOL_SIGN_VERSION="0.0.1+0" | |
| #. Build the firmware for the secondary app slot, increase the version number in the :file:`prj.conf` file (uncomment the line): | |
| .. code-block:: kconfig | |
| CONFIG_MCUBOOT_IMGTOOL_SIGN_VERSION="0.0.1+0" |
| * - zephyr,loaded-fw-dst: Destination region in RAM | ||
| */ | ||
|
|
||
| #define LOADED_FW_NVM_NODE DT_CHOSEN(zephyr_loaded_fw_src) |
There was a problem hiding this comment.
How this application recognized if the image should be copied from primary or secondary slot?
There was a problem hiding this comment.
the secondary slot image has dts.overlay which sets chosen node to correct address.
There was a problem hiding this comment.
In a follow-up PR we'll update the Radio image to log the address of the slot it loaded from to demonstrate the correct one was used.
There was a problem hiding this comment.
You can just detect slot here apart of making overlays for secondary slot images. As reference you can check how application core boots radio core on nRF54h20
There was a problem hiding this comment.
Do you mean to use the cpuapp_slot1_partition node like here: https://github.com/nrfconnect/sdk-zephyr/blob/main/soc/nordic/nrf54h/soc.c#L213?
| memcpy((void *)LOADED_FW_RAM_ADDR, (void *)LOADED_FW_NVM_ADDR, LOADED_FW_NVM_SIZE); | ||
|
|
||
| /* Extract reset handler from ARM Cortex-M vector table (entry 1) */ | ||
| uint32_t *vector_table = (uint32_t *)LOADED_FW_RAM_ADDR; |
There was a problem hiding this comment.
What about MCUboot header, in some cases it is added at the beginning of the image and vector table is shifted? For example 800 bytes is reserved for nRF54H20
There was a problem hiding this comment.
Don't understand.
here we have MCUboot in direct-xip with merged slots so header is at the beginning of first image - in this case app - image
There was a problem hiding this comment.
Handling split slot required changes here to handle it, see draft PR 75048fa#diff-b40b584714f96c16fd14e46fe04bec2174736e6b2339c583d99fbc90eee41379R50
| BOARD ${SB_CONFIG_NRF_RADIO_LOADER_BOARD} | ||
| BOARD_REVISION ${BOARD_REVISION} | ||
| ) | ||
|
|
There was a problem hiding this comment.
Is it no needed?
| set_target_properties(${radio_loader} PROPERTIES | |
| IMAGE_CONF_SCRIPT ${ZEPHYR_BASE}/share/sysbuild/image_configurations/MAIN_image_default.cmake | |
| ) |
There was a problem hiding this comment.
It is only needed in case of split slot variant (see draft PR 75048fa#diff-11840703ce5ba60c7bd4cde5441eccec624524bcf41f016843ac38c0bdbb9711R22)
grochu
force-pushed
the
issue36461_TCM
branch
from
a8f3051 to
e8f35b5
Compare
This PR will remain in this default configuration. The optimization removing the |
|
Pr not ready, moved out of 3.2.0 scope |
KAGA164
left a comment
KAGA164
left a comment
There was a problem hiding this comment.
Approving for speed-up integration. I would like to have support for splitted slots and simplified version with disabled CONFIG_XIP in NCS3.2.0
grochu
force-pushed
the
issue36461_TCM
branch
from
e8f35b5 to
7d464dc
Compare
|
|
||
| The overlay defines: | ||
|
|
||
| #. TCM regions: |
There was a problem hiding this comment.
| #. TCM regions: | |
| #. TCM regions: |
Introduce a new sample demonstrating a multicore idle test with firmware relocated to the radio core's TCM. The test showcases the radio loader pattern, where firmware is loaded from MRAM to TCM at runtime, ensuring efficient execution. JIRA: NCSDK-36461 Signed-off-by: Jan Zyczkowski <[email protected]>
grochu
force-pushed
the
issue36461_TCM
branch
from
7d464dc to
5d0da1b
Compare
tejlmand
left a comment
tejlmand
left a comment
There was a problem hiding this comment.
Besides the misuse of kconf.cmake, then several warnings are seens when building this example:
CMake Warning:
Manually-specified variables were not used by the project:
EXTRA_KCONFIG_TARGETS
FORCED_CONF_FILE
and
warning: UPDATEABLE_IMAGE_NUMBER (defined at
/projects/github/review/nrf/modules/../samples/common/mcumgr_bt_ota_dfu/Kconfig:87,
subsys/dfu/Kconfig:96) was assigned the value '1' but got the value ''. Check these unsatisfied
dependencies: (((BOARD_THINGY53_NRF5340_CPUAPP || BOARD_THINGY53_NRF5340_CPUAPP_NS) &&
SOC_SERIES_NRF53X && NCS_SAMPLE_MCUMGR_BT_OTA_DFU) || (!MCUBOOT && IMG_MANAGER)) (=n). See
http://docs.zephyrproject.org/latest/kconfig.html#CONFIG_UPDATEABLE_IMAGE_NUMBER and/or look up
UPDATEABLE_IMAGE_NUMBER in the menuconfig/guiconfig interface. The Application Development Primer,
Setting Configuration Values, and Kconfig - Tips and Best Practices sections of the manual might be
helpful too.
warning: MCUBOOT_UPDATE_FOOTER_SIZE (defined at subsys/dfu/Kconfig:55) was assigned the value '0x50'
but got the value ''. Check these unsatisfied dependencies: MCUBOOT_IMG_MANAGER (=n), IMG_MANAGER
(=n). See http://docs.zephyrproject.org/latest/kconfig.html#CONFIG_MCUBOOT_UPDATE_FOOTER_SIZE and/or
look up MCUBOOT_UPDATE_FOOTER_SIZE in the menuconfig/guiconfig interface. The Application
Development Primer, Setting Configuration Values, and Kconfig - Tips and Best Practices sections of
the manual might be helpful too.
warning: MCUBOOT_BOOTLOADER_USES_SHA512 (defined at modules/Kconfig.mcuboot:370) was assigned the
value 'y' but got the value 'n'. Check these unsatisfied dependencies: BOOTLOADER_MCUBOOT (=n). See
http://docs.zephyrproject.org/latest/kconfig.html#CONFIG_MCUBOOT_BOOTLOADER_USES_SHA512 and/or look
up MCUBOOT_BOOTLOADER_USES_SHA512 in the menuconfig/guiconfig interface. The Application Development
Primer, Setting Configuration Values, and Kconfig - Tips and Best Practices sections of the manual
might be helpful too.
the EXTRA_KCONFIG_TARGETS and FORCED_CONF_FILE warnings are probably unrelated to this PR, but the Kconfig errors seems related to this PR.
| dt_chosen(code_partition PROPERTY "fw-to-relocate") | ||
| if("${code_partition}" STREQUAL "") | ||
| dt_chosen(code_partition PROPERTY "zephyr,code-partition") | ||
| endif() | ||
|
|
There was a problem hiding this comment.
but you're using devicetree to adjust Kconfig behind the scenes.
This file is kconfig.cmake, and kconfig.cmake is for kconfig handling and not intended for a devicetree handling / adjustments.
And before you start replying that devicetree is already done in this file, then I would like to direct to this PR #23562 .
This commit introduces a (probably temporary) way of fixing passing Kconfigs to a variant image.
...
Note: there were attempts to make a non-hack, clean fix in zephyr:
...
A quick solution for 3.1.0 is needed, as we already get complains about the issue
Starting to manipulate n-kconfig settings depending on featureA, featureB, featureC, ... etc is not scalable, and definitely not making it easier for NCS customers to adjust the solution to their needs without patching NCS itself.
Please take a look at the original implemetation for variant image e4be060 and notice how clean that is as it doesn't need special treatment for xyz settings.
just because something was introduced as a quick fix for 3.1 doesn't mean it's a proper way to do things.
@ahasztag why wasn't #23562 followed up after 3.1 was done ?
|
Will be substituted by #25891 |
11 participants
Add idle relocated TCM multicore test for nRF54H20
Introduce a new sample demonstrating a multicore idle test
with firmware relocated to the radio core's TCM.
The test showcases the radio loader pattern, where firmware
is loaded from MRAM to TCM at runtime, ensuring efficient execution.
Add changes in sysbuild required to allow building radio TCM loader with sysbuild and mcuboot.