-
Notifications
You must be signed in to change notification settings - Fork 1.4k
nRF Desktop: nRF54LM20: execute the app code from RAM #25661
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
base: main
Are you sure you want to change the base?
Conversation
CI InformationTo view the history of this post, click the 'edited' button above Inputs:Sources:sdk-nrf: PR head: e4d08c06c05b6817b33717c97a9e1dd0904ab1c4 more detailssdk-nrf:
Github labels
List of changed files detected by CI (19)Outputs:ToolchainVersion: df3cc9d822 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
25781ba to
8daea40
Compare
|
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-25661/nrf/applications/nrf_desktop/doc/dfu.html |
Memory footprint analysis revealed the following potential issuesapplications.nrf_desktop.zdebug.uart[nrf54lm20dk/nrf54lm20a/cpuapp]: RAM size increased by 344725[B] in comparison to the main[3524638] branch. - link (cc: @nrfconnect/ncs-si-bluebagel) Note: This message is automatically posted and updated by the CI (latest/sdk-nrf/PR-25661/4) |
|
|
||
| &cpuapp_rram { | ||
| /* Application does not use cpuflpr core. Assign whole RRAM to cpuapp. */ | ||
| reg = < 0x0 DT_SIZE_K(2036) >; |
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.
<value value> style, no space after < and before >
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.
fixed
applications/nrf_desktop/configuration/nrf54lm20dk_nrf54lm20a_cpuapp/images/mcuboot/app.overlay
Show resolved
Hide resolved
8daea40 to
729480a
Compare
|
Pushed an update with the following additions:
|
729480a to
e4d08c0
Compare
| * RAM due to the dependency on the nrfutil device tool and its KMU | ||
| * provisioning functionality. | ||
| */ | ||
| nrf_kmu_reserved_push_area: memory@0 { |
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.
reserved memory is for hardware only, so KMU usage is fine, DMA usage is fine...
| * both the MCUboot image and the application code. Currently, it is used | ||
| * to share the image metadata between the bootloader and the application. | ||
| */ | ||
| cpuapp_sram_retained_mem_region: memory@400 { |
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 though is not, this is normal RAM, it needs to be a child of the sram node, see the retention doc in zephyr for an example
| #address-cells = <1>; | ||
| #size-cells = <1>; | ||
| ranges = <0x0 0x7a400 DT_SIZE_K(20)>; | ||
| }; |
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.
so it can be moved here
| SYMBOLS __start_config_channel_modules __stop_config_channel_modules | ||
| KEEP SORT NAME) | ||
| if(CONFIG_DESKTOP_CONFIG_CHANNEL_DFU_MCUBOOT_RAM_LOAD) | ||
| zephyr_linker_sources(RODATA linker/config_channel_dfu_ram_load.ld) |
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.
Should we add zephyr_linker_section/zephyr_iterable_section too? This commit suggests that it's needed for IAR: b937d94
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 don't think we have to, as we just perform validation as part of this linker section without creating any new symbols
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.
Could you please check if the assertion is properly triggered for LLVM? (to ensure no silent errors in the future)
| validate if the executable RAM region of the application image does not overlap with | ||
| the RAM region of the MCUboot image. | ||
|
|
||
| config DESKTOP_CONFIG_CHANNEL_DFU_MCUBOOT_UNSPECIFIED |
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.
Consider listing the swap mode explicitly too. Then we might e.g. emit an error/warning log in case MCUboot mode unsupported by the module is selected
|
|
||
| config DESKTOP_CONFIG_CHANNEL_DFU_MCUBOOT_RAM_LOAD | ||
| bool "Adjust the DFU module to be compatible with the RAM load mode of MCUboot" | ||
| depends on USE_DT_CODE_PARTITION |
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.
Dependency on disabled Partition Manager might be even more relevant here (we might need a build assert here)
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.
btw. Would it work also for other platforms (e.g. nRF52)? We might limit support to nRF54LM20 (temporarily) if needed
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 think so
| prompt "Adjust the DFU module to be compatible with the specific mode of MCUboot" | ||
| depends on BOOTLOADER_MCUBOOT | ||
| default y if MCUBOOT_BOOTLOADER_MODE_DIRECT_XIP | ||
| default DESKTOP_CONFIG_CHANNEL_DFU_MCUBOOT_DIRECT_XIP if MCUBOOT_BOOTLOADER_MODE_DIRECT_XIP |
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.
Consider using depends on for bootloader modes instead of setting default (that should allow to skip the newly introduced build asserts)
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.
Or maybe we could even use MCUBOOT_BOOTLOADER_MODE_XYZ options directly?
| * Currently, the revert feature is only supported by the MCUboot bootloader that | ||
| * is not configured for the standard direct-xip or RAM load mode. | ||
| */ | ||
| #define DFU_REVERT_FEATURE_IS_SUPPORTED \ |
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.
MCUBOOT_REVERT_FEATURE_IS_SUPPORTED (as we refer to MCUboot's revert feature)
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.
Or maybe even MCUBOOT_IMAGE_CONFIRM_NEEDED (to indicate that we need to confirm the image after it's uploaded)
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.
We also need to set the test bit with the boot_request_upgrade API
I will stick to the REVERT variant, it is easily associated with most mcuboot modes
CONFIG_MCUBOOT_BOOTLOADER_MODE_DIRECT_XIP_WITH_REVERT
CONFIG_MCUBOOT_BOOTLOADER_MODE_RAM_LOAD_WITH_REVERT
| */ | ||
| cpuapp_sram_retained_mem_region: memory@400 { | ||
| compatible = "zephyr,memory-region", "mmio-sram"; | ||
| reg = <0x400 DT_SIZE_K(1)>; |
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.
Can we explain why we have a gap between nrf_kmu_reserved_push_area and RetainedMem?
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.
Also: maybe we could set size to 0x100 for now and expand when needed (currently part of the retained RAM seems unused)
| * this region, but the ROM section of the application image must never | ||
| * fill this region. This DTS node must be assigned to the MCUboot image | ||
| * as its chosen SRAM DTS node. The usage of this SRAM region should not | ||
| * be close to the maximum size of this region (100%), as the System Heap |
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.
Would be good to check what it the System Heap used for in MCUboot. Maybe we could simply disabled it: CONFIG_COMMON_LIBC_MALLOC=n?
Similarly for the application image (nRF Desktop relies on k_malloc)
|
|
||
| slot0_partition: partition@c000 { | ||
| label = "image-0"; | ||
| reg = <0xc000 DT_SIZE_K(984)>; |
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.
Does it make sense to use partition size greater than DK_SIZE_K(489) here? Maybe we could leave part of the memory unused (as it would not be possible to boot bigger image from SRAM anyway)?
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.
Discussed offline, lowering partition size here would not trigger build errors related to partition memory overflows anyway (build system reports FLASH usage of 0).
| @@ -0,0 +1,3 @@ | |||
| /* Validate that the executable RAM region does not overlap with the MCUboot RAM region. */ | |||
| ASSERT(__rom_region_end <= CONFIG_DESKTOP_CONFIG_CHANNEL_DFU_MCUBOOT_RAM_LOAD_MCUBOOT_RAM_START_ADDR, | |||
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 am worried that the assertion might not always be valid (e.g. if user specifies MCUboot's RAM earlier in memory).
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.
Can we at least add a brief comment explaining our assumptions, please?
| config DESKTOP_CONFIG_CHANNEL_DFU_MCUBOOT_RAM_LOAD | ||
| bool "Adjust the DFU module to be compatible with the RAM load mode of MCUboot" | ||
| depends on USE_DT_CODE_PARTITION | ||
| depends on $(dt_nodelabel_exists,cpuapp_sram_mcuboot_ram_region) |
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.
Consider using BUILD_ASSERT instead of dependency here (now you might end up with accidentally set CONFIG_DESKTOP_CONFIG_CHANNEL_DFU_MCUBOOT_UNSPECIFIED)
| bool "Adjust the DFU module to be compatible with the RAM load mode of MCUboot" | ||
| depends on USE_DT_CODE_PARTITION | ||
| depends on $(dt_nodelabel_exists,cpuapp_sram_mcuboot_ram_region) | ||
| select EXPERIMENTAL |
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.
Reminder: Make sure that some sort of experimental warning is emitted to the end-user if we remove these app-specific options.
|
|
||
| SB_CONFIG_BOOTLOADER_MCUBOOT=y | ||
| SB_CONFIG_MCUBOOT_MODE_DIRECT_XIP=y | ||
| SB_CONFIG_MCUBOOT_MODE_RAM_LOAD=y |
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 wonder if we shouldn't leave one configuration that uses SB_CONFIG_MCUBOOT_MODE_DIRECT_XIP to have both DFU methods support for nRF54LM20. Thanks to that we could have configuration that is not "experimental" (but has report rate issues in USB HS mode).
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.
We can discuss it offline
b0749a4 to
f7436ee
Compare
Added support for the RAM load mode of the MCUboot bootloader to the nRF Desktop application DFU module. Added a linker script with the assert statement that prevents the executable RAM region of the application image to overflow the standard RAM region of the MCUboot image. Ref: NCSDK-35506 Signed-off-by: Kamil Piszczek <Kamil.Piszczek@nordicsemi.no>
Updated the nRF54LM20 DK configuration in the nRF Desktop application to increase the USB High Speed performance by executing application code from RAM. Due to this change, the application can now consistently maintain the 8000Hz report rate in the mouse release configuration. To execute code from RAM, the MCUboot mode has been changed from the direct-xip mode to the RAM load mode. The bootloader now copies the newer application image from one of two RRAM slots into the RAM region and then it starts the application from RAM. To support RAM load mode of the MCUboot bootloader, the partitioning method has been changed from the Partition Manager (PM) to devicetree (DTS). Currently, the PM is not properly supported in the RAM load mode. Ref: NCSDK-35506 Signed-off-by: Aleksander Strzebonski <aleksander.strzebonski@nordicsemi.no> Signed-off-by: Kamil Piszczek <Kamil.Piszczek@nordicsemi.no>
alstrzebonski
left a comment
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.
LGTM - one comment to discuss tomorrow.
Side PRs:
CONFIG_BOOT_IMAGE_EXECUTABLE_RAM_STARTandCONFIG_BOOT_IMAGE_EXECUTABLE_RAM_SIZEaccording to the DTS description:[nrf noup] boot: zephyr: support code-sram label to define exe region sdk-mcuboot#575
Other potential improvements: