From patchwork Thu Mar 26 23:01:59 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rasmus Villemoes X-Patchwork-Id: 244364 List-Id: U-Boot discussion From: rasmus.villemoes at prevas.dk (Rasmus Villemoes) Date: Fri, 27 Mar 2020 00:01:59 +0100 Subject: [PATCH v2 1/2] env: add SAVEENV as an alias of the CMD_SAVEENV symbol In-Reply-To: <20200326230200.12617-1-rasmus.villemoes@prevas.dk> References: <20200219094726.26798-1-rasmus.villemoes@prevas.dk> <20200326230200.12617-1-rasmus.villemoes@prevas.dk> Message-ID: <20200326230200.12617-2-rasmus.villemoes@prevas.dk> Currently, quite a few storage drivers currently do not honour SPL_SAVEENV. That is, whether or not one enables CONFIG_SPL_SAVEENV, the backend drivers do not provide the .save method. Witness env/fat.c:#ifdef CONFIG_SPL_BUILD ... env/fat.c-#else env/fat.c-# define LOADENV env/fat.c:# if defined(CONFIG_CMD_SAVEENV) env/fat.c:# define CMD_SAVEENV env/fat.c-# endif env/fat.c-#endif env/fat.c- env/fat.c:#ifdef CMD_SAVEENV env/fat.c-static int env_fat_save(void) env/flash.c:#ifndef CONFIG_SPL_BUILD env/flash.c:# if defined(CONFIG_CMD_SAVEENV) && defined(CONFIG_CMD_FLASH) env/flash.c:# define CMD_SAVEENV ... env/flash.c:#ifdef CMD_SAVEENV env/flash.c-static int env_flash_save(void) env/mmc.c:#if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_SPL_BUILD) env/mmc.c-static inline int write_env(struct mmc *mmc, unsigned long size, env/nand.c:#if defined(CONFIG_CMD_SAVEENV) && defined(CONFIG_CMD_NAND) && \ env/nand.c: !defined(CONFIG_SPL_BUILD) env/nand.c:#define CMD_SAVEENV ... env/nand.c:#ifdef CMD_SAVEENV env/nand.c-/* env/nand.c- * The legacy NAND code saved the environment in the first NAND device i.e., env/nand.c- * nand_dev_desc + 0. This is also the behaviour using the new NAND code. env/nand.c- */ env/nand.c-static int writeenv(size_t offset, u_char *buf) env/sf.c:#ifndef CONFIG_SPL_BUILD env/sf.c:#define CMD_SAVEENV env/sf.c-#define INITENV env/sf.c-#endif ... env/sf.c:#ifdef CMD_SAVEENV env/sf.c-static int env_sf_save(void) In all these cases, the mere presence of CONFIG_SPL_BUILD means the save method does not get built. Now, it is currently a bit awkward to write a proper test for whether saving the environment is enabled in the current context; something like #if (!defined(CONFIG_SPL_BUILD) && defined(CONFIG_CMD_SAVEENV)) || \ (defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_SAVEENV)) But we already have a rather elegant mechanism that implicitly does the CONFIG_SPL_BUILD tests, namely CONFIG_IS_ENABLED(). Using that requires that the controlling config symbols follow a strict pattern: FOO for U-Boot proper, SPL_FOO for SPL. This patch introduces CONFIG_SAVEENV as an alias for CONFIG_CMD_SAVEENV. That way, the above can simply be written #if CONFIG_IS_ENABLED(SAVEENV) and moreover, CONFIG_IS_ENABLED(SAVEENV) can also be used in C code, avoiding ifdeffery and providing more compile testing. Signed-off-by: Rasmus Villemoes --- v2: Expand commit message, explain why one needs a new config symbol in order to use CONFIG_IS_ENABLED, and demonstrate how many of the storage drivers don't compile their .save method when CONFIG_SPL_BUILD. The patch itself is the same. env/Kconfig | 3 +++ 1 file changed, 3 insertions(+) diff --git a/env/Kconfig b/env/Kconfig index 0d6f559b39..969308fe6c 100644 --- a/env/Kconfig +++ b/env/Kconfig @@ -3,6 +3,9 @@ menu "Environment" config ENV_SUPPORT def_bool y +config SAVEENV + def_bool y if CMD_SAVEENV + config ENV_IS_NOWHERE bool "Environment is not stored" default y if !ENV_IS_IN_EEPROM && !ENV_IS_IN_EXT4 && \ From patchwork Thu Mar 26 23:02:00 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rasmus Villemoes X-Patchwork-Id: 244366 List-Id: U-Boot discussion From: rasmus.villemoes at prevas.dk (Rasmus Villemoes) Date: Fri, 27 Mar 2020 00:02:00 +0100 Subject: [PATCH v2 2/2] env/sf.c: honour CONFIG_SPL_SAVEENV In-Reply-To: <20200326230200.12617-1-rasmus.villemoes@prevas.dk> References: <20200219094726.26798-1-rasmus.villemoes@prevas.dk> <20200326230200.12617-1-rasmus.villemoes@prevas.dk> Message-ID: <20200326230200.12617-3-rasmus.villemoes@prevas.dk> Deciding whether to compile the env_sf_save() function based solely on CONFIG_SPL_BUILD is wrong: For U-Boot proper, it leads to a build warning in case CONFIG_CMD_SAVEENV=n (because the initialization of the .save member is guarded by CONFIG_CMD_SAVEENV, while the env_sf_save() function is built if !CONFIG_SPL_BUILD - and even without the CONFIG_CMD_SAVEENV guard, the env_save_ptr() macro would just expand to NULL, with no reference to env_sf_save visible to the compiler). And for SPL, when one selects CONFIG_SPL_SAVEENV, one obviously expects to actually be able to save the environment. The compiler warning can be fixed by using a " ? env_sf_save : NULL" construction instead of a macro that just eats its argument and expands to NULL. That way, if is false, env_sf_save gets eliminated as dead code, but the compiler still sees the reference to it. For , we can use CONFIG_IS_ENABLED(SAVEENV), which is true precisely: - For U-Boot proper, when CONFIG_CMD_SAVEENV is set (because CONFIG_SAVEENV is a hidden config symbol that gets set if and only if CONFIG_CMD_SAVEENV is set). - For SPL, when CONFIG_SPL_SAVEENV is set. As a bonus, this also removes quite a few preprocessor conditionals. This has been run-time tested on a mpc8309-derived board to verify that saving the environment does indeed work in SPL with these patches applied. Signed-off-by: Rasmus Villemoes --- v2: Use 'CONFIG_IS_ENABLED(SAVEENV) ? env_sf_save : NULL' directly instead of the dropped ENV_SAVE_PTR macro and expand commit message a bit. env/sf.c | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/env/sf.c b/env/sf.c index 5ef4055219..f41a846294 100644 --- a/env/sf.c +++ b/env/sf.c @@ -21,16 +21,12 @@ #include #ifndef CONFIG_SPL_BUILD -#define CMD_SAVEENV #define INITENV #endif #ifdef CONFIG_ENV_OFFSET_REDUND -#ifdef CMD_SAVEENV static ulong env_offset = CONFIG_ENV_OFFSET; static ulong env_new_offset = CONFIG_ENV_OFFSET_REDUND; -#endif - #endif /* CONFIG_ENV_OFFSET_REDUND */ DECLARE_GLOBAL_DATA_PTR; @@ -69,7 +65,6 @@ static int setup_flash_device(void) } #if defined(CONFIG_ENV_OFFSET_REDUND) -#ifdef CMD_SAVEENV static int env_sf_save(void) { env_t env_new; @@ -148,7 +143,6 @@ static int env_sf_save(void) return ret; } -#endif /* CMD_SAVEENV */ static int env_sf_load(void) { @@ -187,7 +181,6 @@ out: return ret; } #else -#ifdef CMD_SAVEENV static int env_sf_save(void) { u32 saved_size, saved_offset, sector; @@ -247,7 +240,6 @@ static int env_sf_save(void) return ret; } -#endif /* CMD_SAVEENV */ static int env_sf_load(void) { @@ -313,9 +305,7 @@ U_BOOT_ENV_LOCATION(sf) = { .location = ENVL_SPI_FLASH, ENV_NAME("SPI Flash") .load = env_sf_load, -#ifdef CMD_SAVEENV - .save = env_save_ptr(env_sf_save), -#endif + .save = CONFIG_IS_ENABLED(SAVEENV) ? env_sf_save : NULL, #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0) .init = env_sf_init, #endif