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

JordanYates · 2024-07-19T04:55:39Z

Remove the arbitrary disabling of MULTITHREADING, SPI_NOR, NORDIC_QSPI_NOR, etc on Nordic platforms.
The stated reason for disabling these options is to save flash space, however all of these platforms compile just fine with it enabled (with the possible exception of nRF51 series).

The cost of disabling these options in the MCUboot repository is that it becomes painful for upstream users to define alternate flash partitioning schemes, for example moving the secondary image onto external flash. The big annoyance is that these overlays cannot be changed by symbol defaults in the upstream repository. Without this change the options to makes this work are:

Fork MCUboot (annoying to maintain)
Custom sysbuild MCUboot configuration file for every application that wants the alternate layout (copy pasting files everywhere)
Override arguments from the commands line (a lot of typing and chances for mistakes)

The benefits of disabling these options in the MCUboot repository is that you have more free ROM in the bootloader?

If users want to take advantage of reduced ROM footprints, they are already required to modify the devicetree in order to reduce the boot partition size. The additional maintenance burden of a Kconfig overlay is negligible. Furthermore, there are two separate ways to configure individual boards upstream to get the same behavior without the downsides:

Firstly, the devices can simply be disabled in devicetree.

Secondly, the defaults can be configured in the board Kconfig file:

configdefault NORDIC_QSPI_NOR
    default n if MCUBOOT

Flash Comparison

west build -b nrf52840dk/nrf52840 bootloader/mcuboot/boot/zephyr/ -p
Pre PR:

[271/271] Linking C executable zephyr/zephyr.elf
Memory region         Used Size  Region Size  %age Used
           FLASH:       32830 B        48 KB     66.79%
             RAM:       23808 B       256 KB      9.08%
        IDT_LIST:          0 GB        32 KB      0.00%

Post PR:

[287/287] Linking C executable zephyr/zephyr.elf
Memory region         Used Size  Region Size  %age Used
           FLASH:       40104 B        48 KB     81.59%
             RAM:       24640 B       256 KB      9.40%
        IDT_LIST:          0 GB        32 KB      0.00%

No argument that the FLASH goes up, but there is no consequence to this...

The documentation on the `MULTITHREADING` symbol states that ``` If you know for sure that your hardware will work, you can default it to n here. ``` Defaulting the entire Nordic user base to `n` here is a huge statement that all Nordic hardware will work without `MULTITHREADING`, which is somewhat undermined by all of the board overlays that have to turn it back on again in order to get basic features working (USB recovery, images on SPI or QSPI flash). Disable it again in the `nrf52_minimal_footprint.conf` sample, as that is obviously intended to showcase the minimal footprint possible. Leave it disabled for the `SOC_SERIES_NRF51X`, as the default boot sector partitions for that SoC are small enough to require it. Signed-off-by: Jordan Yates <jordan@embeint.com>

Remove configuration overrides that were only specified to compile with `MULTITHREADING=n`. Signed-off-by: Jordan Yates <jordan@embeint.com>

Set valid defaults for `BOOT_MAX_IMG_SECTORS` based on the size of the primary image that MCUboot is expecting. Signed-off-by: Jordan Yates <jordan@embeint.com>

`NORDIC_QSPI_NOR_FLASH_LAYOUT_PAGE_SIZE` needs to be set to 4096 for every board that uses QSPI for external images, so set the default in a single location. Signed-off-by: Jordan Yates <jordan@embeint.com>

Remove explicit control over `NORDIC_QSPI_NOR` from board overlays (including some boards that didn't have QPI flash anyway). The new behaviour is that if the QSPI flash node exists in devicetree, it will be compiled into the bootloader. Options for disabling this include: * Setting the devicetree node to `status="disabled"` for MCUboot * Adding a default in the board definition ``` configdefault NORDIC_QSPI_NOR default n if MCUBOOT ``` Explicitly disabling the symbol in the MCUboot repository makes it very painful for users of the board to setup an alternate flash partition scheme (secondary on QSPI flash), as it requires maintaining a fork of MCUboot or board specific overrides at compile-time. Signed-off-by: Jordan Yates <jordan@embeint.com>

nordicjm · 2024-07-19T06:06:09Z

Custom sysbuild MCUboot configuration file for every application that wants the alternate layout (copy pasting files everywhere)

Not true as said, you can have one folder and just set the application config dir

No argument that the FLASH goes up, but there is no consequence to this...

There is a big consequence to it, go and enable serial recovery in that 81% flash usage build and watch as it no longer builds

JordanYates · 2024-07-19T06:32:58Z

Not true as said, you can have one folder and just set the application config dir

Fair point, it does require periodic checking to ensure configs don't drift from upsteam, but is much better than copy-pasting files.

There is a big consequence to it, go and enable serial recovery in that 81% flash usage build and watch as it no longer builds

That doesn't appear to be the case:

west build -b nrf52840dk/nrf52840 bootloader/mcuboot/boot/zephyr/ -p -- -DCONFIG_MCUBOOT_SERIAL=y -DCONFIG_CONSOLE=n
...
[3/301] Generating include/generated/zephyr/version.h
-- Zephyr version: 3.7.0-rc3 (/home/jordan/code/zephyr/zephyr), build: v3.7.0-rc3-37-g4bc3fd272dcb
[301/301] Linking C executable zephyr/zephyr.elf
Memory region         Used Size  Region Size  %age Used
           FLASH:       46560 B        48 KB     94.73%
             RAM:       28160 B       256 KB     10.74%
        IDT_LIST:          0 GB        32 KB      0.00%

And even if it was, enabling that feature is still a non-default option, which is being specified in a file somewhere, which is a natural location to add a CONFIG_MULTITHREADING=n or any other options needed to make that non-default option work.

My gripe essentially comes down to explicitly disabling options which are generically useful (on some boards only), when the build works fine without them being explicitly disabled, and there are a multitude of ways to get the same behavior from only modifying files in the Zephyr repo.

Take actinius_icarus_bee_nrf9160.conf as an example.

# Disable Zephyr console
CONFIG_CONSOLE=n

# MCUboot serial recovery
CONFIG_MCUBOOT_SERIAL=y

Why is this a file that exists in the MCUboot repo? There is no benefit as far as I can tell.
It has no relevance to the MCUboot project, and is in an extremely inconvenient location to modify if required.
It can just as easily be done in the upstream Zephyr configuration with

configdefault CONSOLE
  n if MCUBOOT
configdefault MCUBOOT_SERIAL
  y if MCUBOOT

nordicjm · 2024-07-19T06:42:47Z

My gripe essentially comes down to explicitly disabling options which are generically useful (on some boards only), when the build works fine without them being explicitly disabled, and there are a multitude of ways to get the same behavior from only modifying files in the Zephyr repo.

They are essentially manufacturer defaults, doesn't mean people have to stick with them but just the defaults

It can just as easily be done in the upstream Zephyr configuration with

Actually it cannot because that Kconfig does not exist in zephyr and compliance will throw an error

JordanYates · 2024-07-19T06:55:21Z

They are essentially manufacturer defaults, doesn't mean people have to stick with them but just the defaults

Defaults that are extremely annoying to alter.
As far as I can see this is the exact same situation that lead to the introduction of the external build system options for modules.

build:
  cmake-ext: True
  kconfig-ext: True

Its a PITA to change things in modules, and there is no inherent value in the defaults being in MCUboot vs Zephyr (where the boards are actually defined).

Actually it cannot because that Kconfig does not exist in zephyr and compliance will throw an error

The current behavior of the compliance script is not a real argument against this proposal.

If there was agreement that specifying the default configuration for a build when built as a bootloader was useful, it is trivial to add config MCUBOOT as a non selectable option somewhere in the Zephyr tree (Nordic already does that in your downstream).

butok · 2024-07-19T15:01:49Z

boot/zephyr/Kconfig

@@ -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


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

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

nvlsianpu · 2024-07-25T12:24:59Z

boot/zephyr/Kconfig

@@ -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


I'm against.
Nordic have low footprint policy,

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...

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

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.

butok · 2024-07-25T12:34:06Z

boot/zephyr/Kconfig

@@ -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


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

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...

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 :(

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.

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.

So moving from:

Works for 256kB image, 4kB sectors

to:

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

isn't valuable?

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.

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

JordanYates added 5 commits July 19, 2024 14:39

boot: zephyr: boards: remove MULTITHREADINGcasualties

fb7ac6c

Remove configuration overrides that were only specified to compile with `MULTITHREADING=n`. Signed-off-by: Jordan Yates <jordan@embeint.com>

boot: zephyr: automatically configure BOOT_MAX_IMG_SECTORS

f3faa8e

Set valid defaults for `BOOT_MAX_IMG_SECTORS` based on the size of the primary image that MCUboot is expecting. Signed-off-by: Jordan Yates <jordan@embeint.com>

boot: zephyr: valid NRF QSPI defaults

b1d5fe2

`NORDIC_QSPI_NOR_FLASH_LAYOUT_PAGE_SIZE` needs to be set to 4096 for every board that uses QSPI for external images, so set the default in a single location. Signed-off-by: Jordan Yates <jordan@embeint.com>

JordanYates mentioned this pull request Jul 19, 2024

mcuboot: nordic development kits fail to compile zephyrproject-rtos/zephyr#73301

Closed

butok reviewed Jul 19, 2024

View reviewed changes

de-nordic added the area: zephyr Affects the Zephyr port label Jul 24, 2024

de-nordic requested review from nvlsianpu, nordicjm, de-nordic and michalek-no July 24, 2024 11:46

nvlsianpu reviewed Jul 25, 2024

View reviewed changes

butok reviewed Jul 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

JordanYates commented Jul 19, 2024

nordicjm commented Jul 19, 2024

JordanYates commented Jul 19, 2024

nordicjm commented Jul 19, 2024 •

edited

Loading

JordanYates commented Jul 19, 2024

butok Jul 19, 2024

sigvartmh Jul 26, 2024

nvlsianpu Jul 25, 2024

JordanYates Jul 25, 2024

sigvartmh Jul 26, 2024

JordanYates Jul 26, 2024

butok Jul 25, 2024

nordicjm Jul 25, 2024

butok Jul 25, 2024

JordanYates Jul 25, 2024

butok Jul 25, 2024

JordanYates Jul 25, 2024

butok Jul 25, 2024

nordicjm Jul 25, 2024

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

Are you sure you want to change the base?

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

Conversation

JordanYates commented Jul 19, 2024

Flash Comparison

nordicjm commented Jul 19, 2024

JordanYates commented Jul 19, 2024

nordicjm commented Jul 19, 2024 • edited Loading

JordanYates commented Jul 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nordicjm commented Jul 19, 2024 •

edited

Loading