diff mbox series

[3/5] env/fat.c: remove private CMD_SAVEENV logic

Message ID 20200219094726.26798-4-rasmus.villemoes@prevas.dk
State Accepted
Commit 3908bc93443be458abe28b9c3797ad912612631a
Headers show
Series CMD_SAVEENV ifdef cleanup | expand

Commit Message

Rasmus Villemoes Feb. 19, 2020, 9:47 a.m. UTC
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 at prevas.dk>
---
 env/fat.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

Comments

Wolfgang Denk Feb. 19, 2020, 1:27 p.m. UTC | #1
Dear Rasmus,

In message <20200219094726.26798-4-rasmus.villemoes at 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
Rasmus Villemoes Feb. 20, 2020, 2:22 p.m. UTC | #2
On 19/02/2020 14.27, Wolfgang Denk wrote:
> Dear Rasmus,
> 
> In message <20200219094726.26798-4-rasmus.villemoes at 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
Wolfgang Denk Feb. 21, 2020, 4:14 p.m. UTC | #3
Dear Rasmus,

In message <5265fdd5-3992-4e5f-3235-5586b3b77dd7 at 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
Tom Rini Feb. 21, 2020, 4:19 p.m. UTC | #4
On Fri, Feb 21, 2020 at 05:14:14PM +0100, Wolfgang Denk wrote:
> Dear Rasmus,
> 
> In message <5265fdd5-3992-4e5f-3235-5586b3b77dd7 at 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.
Rasmus Villemoes Feb. 24, 2020, 2:49 p.m. UTC | #5
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 at 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
Tom Rini April 24, 2020, 5:08 p.m. UTC | #6
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 at prevas.dk>

Applied to u-boot/master, thanks!
diff mbox series

Patch

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),
 };