[PATCH v2 0/2] allow CONFIG_SPL_SAVEENV to work with ENV_IS_IN_SPI_FLASH

[Rasmus Villemoes]

On 27/03/2020 17.31, Wolfgang Denk wrote:
> Dear Rasmus,
> 
>>
>> # 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!

No, it does not trigger questions, or at least, none that I can't answer
myself. It's just -ffunction-sections, -fdata-sections,
-Wl,--gc-sections at work, working as intended. When nothing references
env_export (and nothing was referencing it when env_sf_save did not get
compiled in), the linker discards it, along with everything that
env_export was the sole user of.

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

See above. It gets compiled, but discarded (in current master, that is).

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

Yes, that was one thing I did think about, but it wouldn't be the first
time somebody enabled a config option that wasn't actually needed.

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

That is exactly what I have been saying (or trying to say) all along.

SPL_SAVEENV is, for many/most backends, completely ignored. The
compiler/linker flags then ensure that the binary doesn't carry a lot of
excess baggage that does get _compiled_ with SPL_SAVEENV, e.g.
env_export(), so while SPL_SAVEENV did not have the intended effect, it
also did not (due to the garbage collection) have any ill effect in
terms of needless bloat.

 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.

That's what I figured, because once you _do_ try to save the environment
from the SPL, you quickly realize that doesn't work at all. Which is of
course how I discovered the bug in sf.c (and which is repeated in
various forms in the other backends).

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

Can you provide a pointer? Sounds like something I could use going forward.

> Thanks, both for the additional testing and your patience.

Likewise.

> Reviewed-by: Wolfgang Denk <wd at denx.de>

Thanks,
Rasmus