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

boot: zephyr: common defaults, remove arbitrary disables #2015

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

boot: zephyr: common defaults, remove arbitrary disables #2015

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f8c03cf
boot: zephyr: don't disable `MULTITHREADING`
JordanYates Jul 19, 2024
fb7ac6c
boot: zephyr: boards: remove `MULTITHREADING`casualties
JordanYates Jul 19, 2024
f3faa8e
boot: zephyr: automatically configure `BOOT_MAX_IMG_SECTORS`
JordanYates Jul 19, 2024
b1d5fe2
boot: zephyr: valid NRF QSPI defaults
JordanYates Jul 19, 2024
dad039d
boot: zephyr: remove explicit `NORDIC_QSPI_NOR` control
JordanYates Jul 19, 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
17 changes: 15 additions & 2 deletions boot/zephyr/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,12 +382,16 @@ config BOOT_ENCRYPTION_KEY_FILE

config BOOT_MAX_IMG_SECTORS
int "Maximum number of sectors per image slot"
default 512 if $(dt_nodelabel_reg_size_int,slot0_partition) > 1048576
default 256 if $(dt_nodelabel_reg_size_int,slot0_partition) > 524288
Copy link
Contributor

Choose a reason for hiding this comment

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

This default value calculation is a good idea.
If you add it as a separate PR, it should be no complains.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this really does not work well, for example a 2MiB stm32 device with 128 or 256KiB sectors here needs a value of 12 or 6 really, but would have a value of 512...

Copy link
Contributor

Choose a reason for hiding this comment

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

this really does not work well, for example a 2MiB stm32 device with 128 or 256KiB sectors here needs a value of 12 or 6 really, but would have a value of 512...

You are right. Sector size is missed :(

Copy link
Author

Choose a reason for hiding this comment

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

this really does not work well, for example a 2MiB stm32 device with 128 or 256KiB sectors here needs a value of 12 or 6 really, but would have a value of 512...

Does it work better than the current solution? Sector size is not exposed to the build or configuration system.

Copy link
Contributor

Choose a reason for hiding this comment

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

this really does not work well, for example a 2MiB stm32 device with 128 or 256KiB sectors here needs a value of 12 or 6 really, but would have a value of 512...

Does it work better than the current solution? Sector size is not exposed to the build or configuration system.

Unfortunately without a sector size it has no big sense.

Copy link
Author

Choose a reason for hiding this comment

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

So moving from:

  • Works for 256kB image, 4kB sectors

to:

  • Works for 256kB, 512kB, 1MB images, 4kB sectors

isn't valuable?

Copy link
Contributor

Choose a reason for hiding this comment

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

So moving from:

  • Works for 256kB image, 4kB sectors

to:

  • Works for 256kB, 512kB, 1MB images, 4kB sectors

isn't valuable?

Platforms have different sector sizes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've created #2019 though I've tested it on one device - nrf52840dk, it gets 118 which is about right. Haven't actually flashed it to test. It will also probably be slightly wrong if someone has a larger slot0 than slot1 using swap using move, but that would be 2 sectors it'd be out by

default 128
help
This option controls the maximum number of sectors that each of
the two image areas can contain. Smaller values reduce MCUboot's
memory usage; larger values allow it to support larger images.
If unsure, leave at the default value.
If unsure, leave at the default value. The required condition is
that (IMAGE_SIZE <= (BOOT_MAX_IMG_SECTORS * SECTOR_SIZE)). The
defaults are configured for a SECTOR_SIZE of 4096.

config BOOT_SHARE_BACKEND_AVAILABLE
bool
Expand Down Expand Up @@ -748,8 +752,8 @@ comment "Zephyr configuration options"
config MULTITHREADING
default y if BOOT_SERIAL_CDC_ACM #usb driver requires MULTITHREADING
default y if BOOT_USB_DFU_GPIO || BOOT_USB_DFU_WAIT
default n if SOC_FAMILY_NORDIC_NRF
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm against.
Nordic have low footprint policy,

Copy link
Author

Choose a reason for hiding this comment

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

I feel like I'm going in circles on this.
Every single platform tested in CI builds fine with this default removed.
Therefore removing this default has exactly 0% effect on the space available for any application.
IF downstream boards are optimizing their boot partitions to the kB and this causes an overflow, turn the option back off.
By contrast, leaving this option disabled means that MCUboot fails to compile as soon as you move a partition to external SPI or QSPI flash. IMO the latter is a larger problem than 66% vs 82% of the boot partition being used...

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it better that a user knows the impact of their choice other than having a default which increase the footprint by default?

Copy link
Author

Choose a reason for hiding this comment

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

Zephyr is overflowing with defaults that are not 100% size optimized in the name of making more situations work by default. Search for any Kconfig with the range property and I can almost guarantee that it is not choosing the lowest footprint option. None of this stops the user trying to save every byte from overriding defaults.

default n if SOC_FAMILY_ESPRESSIF_ESP32 && MCUBOOT
default n if SOC_SERIES_NRF51X # Minimal flash available
default y

config LOG_PROCESS_THREAD
Expand Down Expand Up @@ -782,4 +786,13 @@ config MCUBOOT_VERIFY_IMG_ADDRESS
also be useful when BOOT_DIRECT_XIP is enabled, to ensure that the image
linked at the correct address is loaded.

# When used with MCUboot, the external flash page size must match
# the internal SoC flash page size, which is 4096 for Nordic MCUs.
configdefault NORDIC_QSPI_NOR_FLASH_LAYOUT_PAGE_SIZE
Copy link
Contributor

Choose a reason for hiding this comment

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

Can NORDIC specific configuration moved from the generic to nordic-specific files?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah maybe this could moved into a specific Kconfig.nrf file which is included when using a nRF file?

default 4096
# Use a slightly larger default value for stack writes that is unlikely
# to lead to any stack overflows.
configdefault NORDIC_QSPI_NOR_STACK_WRITE_BUFFER_SIZE
default 32

source "Kconfig.zephyr"
5 changes: 0 additions & 5 deletions boot/zephyr/boards/actinius_icarus_bee_nrf9160.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
# Disable Zephyr console
CONFIG_CONSOLE=n

# MCUBoot settings
CONFIG_BOOT_MAX_IMG_SECTORS=256

# MCUboot serial recovery
CONFIG_MCUBOOT_SERIAL=y

CONFIG_MULTITHREADING=y
5 changes: 0 additions & 5 deletions boot/zephyr/boards/actinius_icarus_nrf9160.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
# Disable Zephyr console
CONFIG_CONSOLE=n

# MCUBoot settings
CONFIG_BOOT_MAX_IMG_SECTORS=256

# MCUboot serial recovery
CONFIG_MCUBOOT_SERIAL=y

CONFIG_MULTITHREADING=y
5 changes: 0 additions & 5 deletions boot/zephyr/boards/actinius_icarus_som_dk_nrf9160.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
# Disable Zephyr console
CONFIG_CONSOLE=n

# MCUBoot settings
CONFIG_BOOT_MAX_IMG_SECTORS=256

# MCUboot serial recovery
CONFIG_MCUBOOT_SERIAL=y

CONFIG_MULTITHREADING=y
5 changes: 0 additions & 5 deletions boot/zephyr/boards/actinius_icarus_som_nrf9160.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
# Disable Zephyr console
CONFIG_CONSOLE=n

# MCUBoot settings
CONFIG_BOOT_MAX_IMG_SECTORS=256

# MCUboot serial recovery
CONFIG_MCUBOOT_SERIAL=y

CONFIG_MULTITHREADING=y
6 changes: 0 additions & 6 deletions boot/zephyr/boards/bl5340_dvk_nrf5340_cpuapp.conf
View file
Open in desktop

This file was deleted.

6 changes: 0 additions & 6 deletions boot/zephyr/boards/circuitdojo_feather_nrf9160.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,6 @@
# Disable Zephyr console
CONFIG_CONSOLE=n

# Multithreading
CONFIG_MULTITHREADING=y

# MCUBoot settings
CONFIG_BOOT_MAX_IMG_SECTORS=256

# MCUboot serial recovery
CONFIG_MCUBOOT_SERIAL=y
CONFIG_BOOT_SERIAL_DETECT_DELAY=450
Expand Down
6 changes: 0 additions & 6 deletions boot/zephyr/boards/conexio_stratus.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,6 @@ CONFIG_CONSOLE=n
CONFIG_CONSOLE_HANDLER=n
CONFIG_UART_CONSOLE=n

# Multithreading
CONFIG_MULTITHREADING=y

# MCUBoot settings
CONFIG_BOOT_MAX_IMG_SECTORS=256

# MCUboot serial recovery
CONFIG_MCUBOOT_SERIAL=y
CONFIG_BOOT_SERIAL_DETECT_DELAY=450
Expand Down
2 changes: 0 additions & 2 deletions boot/zephyr/boards/nrf52840dk_nrf52840.conf
View file
Open in desktop

This file was deleted.

3 changes: 0 additions & 3 deletions boot/zephyr/boards/nrf52840dk_qspi_nor.conf
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions boot/zephyr/boards/nrf52840dk_qspi_secondary_boot.conf
View file
Open in desktop

This file was deleted.

5 changes: 0 additions & 5 deletions boot/zephyr/boards/nrf52840dongle_nrf52840.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,7 @@ CONFIG_GPIO=y
CONFIG_MCUBOOT_SERIAL=y
CONFIG_BOOT_SERIAL_CDC_ACM=y

# Required by USB
CONFIG_MULTITHREADING=y

# USB
CONFIG_USB_DEVICE_STACK=y
CONFIG_USB_DEVICE_REMOTE_WAKEUP=n
CONFIG_USB_DEVICE_PRODUCT="MCUBOOT"

CONFIG_NORDIC_QSPI_NOR=n
3 changes: 3 additions & 0 deletions boot/zephyr/boards/nrf52_minimal_footprint.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ CONFIG_THREAD_STACK_INFO=n
# CONFIG_TICKLESS_KERNEL is not set
CONFIG_FLASH=y

# Multithreading
CONFIG_MULTITHREADING=n

CONFIG_CONSOLE=n
CONFIG_DEBUG=n
CONFIG_EARLY_CONSOLE=n
Expand Down
5 changes: 0 additions & 5 deletions boot/zephyr/boards/nrf54l15pdk_nrf54l15_cpuapp.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,4 @@
#
# SPDX-License-Identifier: Apache-2.0
#
CONFIG_BOOT_MAX_IMG_SECTORS=256

# Ensure that the qspi driver is disabled by default
CONFIG_NORDIC_QSPI_NOR=n

CONFIG_BOOT_WATCHDOG_FEED=n
1 change: 0 additions & 1 deletion boot/zephyr/boards/nrf9161dk_nrf9161_0_7_0.conf
View file
Open in desktop

This file was deleted.

6 changes: 0 additions & 6 deletions boot/zephyr/boards/pinnacle_100_dvk_nrf52840.conf
View file
Open in desktop

This file was deleted.

6 changes: 0 additions & 6 deletions boot/zephyr/boards/sparkfun_thing_plus_nrf9160.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,6 @@
# Disable Zephyr console
CONFIG_CONSOLE=n

# Multithreading
CONFIG_MULTITHREADING=y

# MCUBoot settings
CONFIG_BOOT_MAX_IMG_SECTORS=256

# MCUboot serial recovery
CONFIG_MCUBOOT_SERIAL=y
CONFIG_BOOT_SERIAL_DETECT_DELAY=450
Expand Down
1 change: 0 additions & 1 deletion boot/zephyr/boards/thingy52_nrf52832.conf
View file
Open in desktop

This file was deleted.

3 changes: 0 additions & 3 deletions boot/zephyr/boards/thingy53_nrf5340_cpuapp.conf
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion boot/zephyr/sample.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ tests:
tags: bootloader_mcuboot
sample.bootloader.mcuboot.qspi_nor_slot:
extra_args: DTC_OVERLAY_FILE="./boards/nrf52840dk_qspi_nor_secondary.overlay;app.overlay"
OVERLAY_CONFIG="./boards/nrf52840dk_qspi_nor.conf;./boards/nrf52840dk_qspi_secondary_boot.conf"
platform_allow: nrf52840dk/nrf52840
integration_platforms:
- nrf52840dk/nrf52840
Expand Down