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.

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

Jump to bottom

zephyr: Add support for automatically calculcating max sectors #2019

Merged
merged 2 commits into from
Aug 13, 2024
Merged

zephyr: Add support for automatically calculcating max sectors #2019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b2636fa
zephyr: Add support for automatically calculcating max sectors
nordicjm Jul 25, 2024
fc6ca4b
docs: release-notes: Add note on zephyr auto max sectors
nordicjm Jul 25, 2024
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
73 changes: 60 additions & 13 deletions boot/zephyr/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,23 +363,66 @@ if(CONFIG_MCUBOOT_BOOT_BANNER)
zephyr_sources(kernel/banner.c)
endif()

if(SYSBUILD)
function(align_up num align result)
math(EXPR out "(((${num}) + ((${align}) - 1)) & ~((${align}) - 1))")
set(${result} "${out}" PARENT_SCOPE)
endfunction()
function(align_up num align result)
math(EXPR out "(((${num}) + ((${align}) - 1)) & ~((${align}) - 1))")
set(${result} "${out}" PARENT_SCOPE)
endfunction()

function(dt_get_parent node)
string(FIND "${${node}}" "/" pos REVERSE)

if(pos EQUAL -1)
message(ERROR "Unable to get parent of node: ${${node}}")
endif()

function(dt_get_parent node)
string(FIND "${${node}}" "/" pos REVERSE)
string(SUBSTRING "${${node}}" 0 ${pos} ${node})
set(${node} "${${node}}" PARENT_SCOPE)
endfunction()

if(CONFIG_BOOT_MAX_IMG_SECTORS_AUTO)
dt_nodelabel(slot0_flash NODELABEL "slot0_partition")
dt_prop(slot0_size PATH "${slot0_flash}" PROPERTY "reg" INDEX 1)
dt_get_parent(slot0_flash)
dt_get_parent(slot0_flash)
dt_prop(erase_size_slot0 PATH "${slot0_flash}" PROPERTY "erase-block-size")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I kinda do not like that we get to the Flash device layer to get this information as I think that we start to pick information that we should not access directly from scripts, but unless the DTS parsing script does not start definitions for info like number of pages per partitions this is the only way. Still this makes changes to dts harder in the future.

Copy link
Member

Choose a reason for hiding this comment

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

The other problem is that I don't believe the code cares what this property is even set to. The flash api directly asks the driver about the erase block size. And that API is needlessly complicated to allow for devices with variable sized sectors, even though the only reasonable thing to do with them in mcuboot is treat the device as if all of the sectors were the size of the largest one.

Copy link
Member

Choose a reason for hiding this comment

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

What this means is that if this property is at a mismatch with the underlying device, the calculation will be wrong. For example, people have reported trying to increase the erase size in the DT, to discover it didn't work, as the code still asked the device what the real sector size is.

I think we should look into changing things so that mcuboot uses this property only to determine the erase-block-size. This will make it easier to support multiple devices by just setting the value to the largest size.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For variably sized sectors, like some stm32 parts, that property is not set and this will give the warning then use the existing default of 128 instead, I did give it a try


if(NOT DEFINED slot0_size)
message(WARNING "Unable to determine size of slot0 partition, cannot calculate minimum sector usage")
elseif(NOT DEFINED erase_size_slot0)
message(WARNING "Unable to determine erase size of slot0 partition, cannot calculate minimum sector usage")
else()
math(EXPR slot_min_sectors "${slot0_size} / ${erase_size_slot0}")
endif()

if(NOT CONFIG_SINGLE_APPLICATION_SLOT)
dt_nodelabel(slot1_flash NODELABEL "slot1_partition")
dt_prop(slot1_size PATH "${slot1_flash}" PROPERTY "reg" INDEX 1)
dt_get_parent(slot1_flash)
dt_get_parent(slot1_flash)
dt_prop(erase_size_slot1 PATH "${slot1_flash}" PROPERTY "erase-block-size")

if(NOT DEFINED slot1_size)
message(WARNING "Unable to determine size of slot1 partition, cannot calculate minimum sector usage")
elseif(NOT DEFINED erase_size_slot1)
message(WARNING "Unable to determine erase size of slot1 partition, cannot calculate minimum sector usage")
else()
math(EXPR slot1_min_sectors "${slot1_size} / ${erase_size_slot1}")

if(pos EQUAL -1)
message(ERROR "Unable to get parent of node: ${${node}}")
if("${slot1_min_sectors}" GREATER "${slot_min_sectors}")
set(slot_min_sectors ${slot1_min_sectors})
endif()
endif()
endif()

string(SUBSTRING "${${node}}" 0 ${pos} ${node})
set(${node} "${${node}}" PARENT_SCOPE)
endfunction()
if(DEFINED slot_min_sectors AND "${slot_min_sectors}" GREATER "0")
zephyr_compile_definitions("MIN_SECTOR_COUNT=${slot_min_sectors}")
message("Calculated maximum number of sectors: ${slot_min_sectors}")
else()
message(WARNING "Unable to calculate minimum number of sector sizes, falling back to 128 sector default. Please disable CONFIG_BOOT_MAX_IMG_SECTORS_AUTO and set CONFIG_BOOT_MAX_IMG_SECTORS to the required value")
endif()
endif()

if(SYSBUILD)
if(CONFIG_SINGLE_APPLICATION_SLOT OR CONFIG_BOOT_FIRMWARE_LOADER OR CONFIG_BOOT_SWAP_USING_SCRATCH OR CONFIG_BOOT_SWAP_USING_MOVE OR CONFIG_BOOT_UPGRADE_ONLY OR CONFIG_BOOT_DIRECT_XIP OR CONFIG_BOOT_RAM_LOAD)
# TODO: RAM LOAD support
dt_nodelabel(slot0_flash NODELABEL "slot0_partition")
Expand Down Expand Up @@ -495,7 +538,11 @@ if(SYSBUILD)
endif()

if(CONFIG_BOOT_SWAP_USING_SCRATCH OR CONFIG_BOOT_SWAP_USING_MOVE)
math(EXPR boot_status_data_size "${CONFIG_BOOT_MAX_IMG_SECTORS} * (3 * ${write_size})")
if(CONFIG_BOOT_MAX_IMG_SECTORS_AUTO AND DEFINED slot_min_sectors AND "${slot_min_sectors}" GREATER "0")
math(EXPR boot_status_data_size "${slot_min_sectors} * (3 * ${write_size})")
else()
math(EXPR boot_status_data_size "${CONFIG_BOOT_MAX_IMG_SECTORS} * (3 * ${write_size})")
endif()
else()
set(boot_status_data_size 0)
endif()
Expand Down
12 changes: 12 additions & 0 deletions boot/zephyr/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,9 +380,21 @@ config BOOT_ENCRYPTION_KEY_FILE
with the public key information will be written in a format expected by
MCUboot.

config BOOT_MAX_IMG_SECTORS_AUTO
bool "Calculate maximum sectors automatically"
default y
help
If this option is enabled then the maximum number of supported sectors per image will
be calculated automatically from the flash erase sizes and size of each partition for
the first image.

If this information is not available, or multiple images are used, then this option
should be disabled and BOOT_MAX_IMG_SECTORS should be set instead

config BOOT_MAX_IMG_SECTORS
int "Maximum number of sectors per image slot"
default 128
depends on !BOOT_MAX_IMG_SECTORS_AUTO
help
This option controls the maximum number of sectors that each of
the two image areas can contain. Smaller values reduce MCUboot's
Expand Down
6 changes: 5 additions & 1 deletion boot/zephyr/include/mcuboot_config/mcuboot_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,11 @@
# endif
#endif

#ifdef CONFIG_BOOT_MAX_IMG_SECTORS
#if defined(CONFIG_BOOT_MAX_IMG_SECTORS_AUTO) && defined(MIN_SECTOR_COUNT)

#define MCUBOOT_MAX_IMG_SECTORS MIN_SECTOR_COUNT

#elif defined(CONFIG_BOOT_MAX_IMG_SECTORS)

#define MCUBOOT_MAX_IMG_SECTORS CONFIG_BOOT_MAX_IMG_SECTORS

Expand Down
5 changes: 5 additions & 0 deletions docs/release-notes.d/zephyr-auto-max-sectors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
- Added support for automatically calculating the maximum number
of sectors that are needed for a build by checking the erase
sizes of the partitions using CMake for Zephyr. This behaviour
can be reverted to the old manual behaviour by disabling
``CONFIG_BOOT_MAX_IMG_SECTORS_AUTO``
Loading