| Message ID | 20200219094726.26798-4-rasmus.villemoes@prevas.dk |
|---|---|
| State | Accepted |
| Commit | 3908bc93443be458abe28b9c3797ad912612631a |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | CMD_SAVEENV ifdef cleanup | expand |
Dear Rasmus, In message <20200219094726.26798-4-rasmus.villemoes@prevas.dk> you wrote: > Always compile the env_fat_save() function, and let > CONFIG_IS_ENABLED(SAVEENV) (via the ENV_SAVE_PTR macro) decide whether > it actually ends up being compiled in. Have you tested that this works? How do the sizes of the images differe before and after applying your changes? [Same question for ext4] Best regards, Wolfgang Denk
On 19/02/2020 14.27, Wolfgang Denk wrote: > Dear Rasmus, > > In message <20200219094726.26798-4-rasmus.villemoes@prevas.dk> you wrote: >> Always compile the env_fat_save() function, and let >> CONFIG_IS_ENABLED(SAVEENV) (via the ENV_SAVE_PTR macro) decide whether >> it actually ends up being compiled in. > > Have you tested that this works? How do the sizes of the > images differe before and after applying your changes? With or without these patches, I get $ size u-boot spl/u-boot-spl text data bss dec hex filename 407173 45308 98352 550833 867b1 u-boot 52403 3360 276 56039 dae7 spl/u-boot-spl $ nm spl/u-boot-spl | grep env_fat 0090c5e8 t env_fat_load $ nm u-boot | grep env_fat 17826cb4 t env_fat_load 17826c10 t env_fat_save for a wandboard_defconfig modified by -CONFIG_SPL_FS_EXT4=y +CONFIG_SPL_FS_FAT=y +CONFIG_SPL_ENV_SUPPORT=y +CONFIG_ENV_IS_IN_FAT=y So in the "read-only environment access in SPL" case, everything is the same before and after. Now also enable CONFIG_SPL_SAVEENV and SPL_FAT_WRITE, then with my patches we get $ size u-boot spl/u-boot-spl text data bss dec hex filename 407173 45308 98352 550833 867b1 u-boot 58298 3360 65860 127518 1f21e spl/u-boot-spl $ nm spl/u-boot-spl | grep env_fat 0090c6e0 t env_fat_load 0090c63c t env_fat_save but without, $ size u-boot spl/u-boot-spl text data bss dec hex filename 407173 45308 98352 550833 867b1 u-boot 52659 3360 280 56299 dbeb spl/u-boot-spl $ nm spl/u-boot-spl | grep env_fat 0090c5e8 t env_fat_load So without the fat.c patch, CONFIG_SPL_SAVEENV is effectively ignored. > [Same question for ext4] Actually, the situation for ext4 is even worse than indicated. Just from reading code in ext4.c, env_ext4_save gets built into the SPL if CONFIG_CMD_SAVEENV, whether or not CONFIG_SPL_SAVEENV is set. So I expected my patch to simply reduce the spl image size in the CONFIG_SPL_SAVEENV=n case. But one cannot compare - currently building with CONFIG_CMD_SAVEENV=y CONFIG_SPL_ENV_IS_IN_EXT4=y CONFIG_SPL_SAVEENV=n simply fails the SPL build because env_ext4_save refers to hexport_r, which is only compiled if !(defined(CONFIG_SPL_BUILD) && !defined(CONFIG_SPL_SAVEENV)) - which took me a while to read, it's a little easier if spelled !defined(CONFIG_SPL_BUILD) || defined(CONFIG_SPL_SAVEENV) Anyway, a side-effect of my ext4 patch is that the above combination actually builds, because env_ext4_save is not linked in, so hexport_r isn't needed. And turning on SPL_SAVEENV also works as expected. Rasmus
Dear Rasmus, In message <5265fdd5-3992-4e5f-3235-5586b3b77dd7@prevas.dk> you wrote: > > So without the fat.c patch, CONFIG_SPL_SAVEENV is effectively ignored. OK, but what about bords that don't store the envionment in a file system, but instead for example in (parallel or SPI) NOR flash or in a UBI volume? > Actually, the situation for ext4 is even worse than indicated. ... > Anyway, a side-effect of my ext4 patch is that the above combination > actually builds, because env_ext4_save is not linked in, so hexport_r > isn't needed. And turning on SPL_SAVEENV also works as expected. OK. Thanks. Best regards, Wolfgang Denk
On Fri, Feb 21, 2020 at 05:14:14PM +0100, Wolfgang Denk wrote: > Dear Rasmus, > > In message <5265fdd5-3992-4e5f-3235-5586b3b77dd7@prevas.dk> you wrote: > > > > So without the fat.c patch, CONFIG_SPL_SAVEENV is effectively ignored. > > OK, but what about bords that don't store the envionment in a file > system, but instead for example in (parallel or SPI) NOR flash or in > a UBI volume? I think the intent is that there is no change today but the door is now open for someone that can test / confirm changes there to do so.
On 21/02/2020 17.19, Tom Rini wrote: > On Fri, Feb 21, 2020 at 05:14:14PM +0100, Wolfgang Denk wrote: >> Dear Rasmus, >> >> In message <5265fdd5-3992-4e5f-3235-5586b3b77dd7@prevas.dk> you wrote: >>> >>> So without the fat.c patch, CONFIG_SPL_SAVEENV is effectively ignored. >> >> OK, but what about bords that don't store the envionment in a file >> system, but instead for example in (parallel or SPI) NOR flash or in >> a UBI volume? > > I think the intent is that there is no change today but the door is now > open for someone that can test / confirm changes there to do so. Yes, exactly. I could have just fixed sf.c which is the one I need for my current project, but it turns out that without the ability to say CONFIG_IS_ENABLED(SAVEENV) the changes to sf.c would be significantly uglier, so it seemed better to provide the infrastructure that will also be useful for converting other storage drivers to honour CONFIG_SPL_SAVEENV. Rasmus
On Wed, Feb 19, 2020 at 09:47:41AM +0000, Rasmus Villemoes wrote: > Always compile the env_fat_save() function, and let > CONFIG_IS_ENABLED(SAVEENV) (via the ENV_SAVE_PTR macro) decide whether > it actually ends up being compiled in. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> Applied to u-boot/master, thanks!
diff --git a/env/fat.c b/env/fat.c index 1836556f36..cf2e5e2b26 100644 --- a/env/fat.c +++ b/env/fat.c @@ -26,12 +26,8 @@ # endif #else # define LOADENV -# if defined(CONFIG_CMD_SAVEENV) -# define CMD_SAVEENV -# endif #endif -#ifdef CMD_SAVEENV static int env_fat_save(void) { env_t __aligned(ARCH_DMA_MINALIGN) env_new; @@ -76,7 +72,6 @@ static int env_fat_save(void) return 0; } -#endif /* CMD_SAVEENV */ #ifdef LOADENV static int env_fat_load(void) @@ -135,7 +130,5 @@ U_BOOT_ENV_LOCATION(fat) = { #ifdef LOADENV .load = env_fat_load, #endif -#ifdef CMD_SAVEENV - .save = env_save_ptr(env_fat_save), -#endif + .save = ENV_SAVE_PTR(env_fat_save), };
Always compile the env_fat_save() function, and let CONFIG_IS_ENABLED(SAVEENV) (via the ENV_SAVE_PTR macro) decide whether it actually ends up being compiled in. Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- env/fat.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)