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

[Wolfgang Denk]

Dear Rasmus,

In message <20200326230200.12617-1-rasmus.villemoes at 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 at denx.de>

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I will also, for an appropriate fee, certify that  your  keyboard  is
object-oriented,  and  that  the bits on your hard disk are template-
compatible.            - Jeffrey S. Haemer in <411akr$3ga at cygnus.com>