Dear Rasmus,
In message 20200326230200.12617-1-rasmus.villemoes@prevas.dk you wrote:
Currently, CONFIG_SPL_SAVEENV is not very well supported by the various storage backends, as many of them contain variants of some logic that end up not compiling the .save method when CONFIG_SPL_BUILD.
...
As far as I can tell, the only in-tree defconfig that sets both SPL_ENV_IS_IN_SPI_FLASH and SPL_SAVEENV is display5_defconfig, which also happens to be the only one setting SPL_SAVEENV at all. Let's see how these patches affect that:
# avoid differences due to different git commit or wallclock time $ export SOURCE_DATE_EPOCH=1585252702 $ echo 'test' > .scmversion $ export ARCH=arm $ export CROSS_COMPILE=arm-linux-gnueabi- $ git checkout master ; make display5_defconfig ; make -j8 $ cp u-boot u-boot.1 ; cp spl/u-boot-spl u-boot-spl.1 $ git checkout sf-spl-saveenv ; make display5_defconfig ; make -j8 $ cp u-boot u-boot.2 ; cp spl/u-boot-spl u-boot-spl.2 $ size u-boot{,-spl}.{1,2} text data bss dec hex filename 377468 24620 66164 468252 7251c u-boot.1 377468 24620 66164 468252 7251c u-boot.2 58411 2020 116 60547 ec83 u-boot-spl.1 59976 2028 116 62120 f2a8 u-boot-spl.2
Thanks for the additional testing. As you can see here, it is definitely worth the effort.
So U-Boot proper is not affected (the files even yield identical objdump -d output), while the SPL grows by the ~1.5K necessary to implement saving the environment. Borrowing the bloat-o-meter script from linux, we can also see the functions/data items that are now included:
Does this not trigger questions to you? Why is the code growing? It had SPL_ENV_IS_IN_SPI_FLASH and SPL_SAVEENV before!
../linux/scripts/bloat-o-meter u-boot-spl.1 u-boot-spl.2 add/remove: 11/0 grow/shrink: 0/1 up/down: 1340/-24 (1316) Function old new delta hexport_r - 408 +408 env_sf_save - 332 +332 qsort - 144 +144 match_entry - 124 +124 env_export - 100 +100 match_string - 92 +92 strstr - 64 +64 setup_flash_device - 56 +56 cmpkey - 12 +12 env_offset - 4 +4 env_new_offset - 4 +4 env_sf_load 184 160 -24 Total: Before=52986, After=54302, chg +2.48%
To me this triggers at least two questions:
- Why is this code included now, when it was not before? - Iff the code was not included before, why did this not cause problems when trying to save the environment in SPL, which was apparently needed by this board?
Adding Lukasz on Cc:, who maintains this board.
After some initial talk to Lukasz it seems your testing indeed discovered a bug - without your patch SPL_SAVEENV apparently had no effect, and oard testing did not vdetect this failure, because requirements changed during the project the the feature that was once requested got later dropped, but the option was not removed.
Testing is _always_ worth the effort.
Now, to check that other storage backends are not affected, and also that nothing (neither U-Boot or SPL) changes for ENV_IS_IN_SPI_FLASH when CONFIG_SPL_SAVEENV=n, I have repeated the above with am335x_shc_netboot_defconfig (MMC), pengwyn_defconfig (NAND), mccmon6_sd_defconfig (FLASH), ls1046ardb_qspi_spl_defconfig (SPI_FLASH):
$ for c in am335x_shc_netboot_defconfig pengwyn_defconfig mccmon6_sd_defconfig ls1046ardb_qspi_spl_defconfig ; do
...
Actually this would have been easier using tbot, and it would have been possible to cover many more / all boards, but I don't intend to ask more from you. Thanks, both for the additional testing and your patience.
Reviewed-by: Wolfgang Denk wd@denx.de
Best regards,
Wolfgang Denk