| Message ID | 20200326230200.12617-3-rasmus.villemoes@prevas.dk |
|---|---|
| State | New |
| Headers | show |
| Series | allow CONFIG_SPL_SAVEENV to work with ENV_IS_IN_SPI_FLASH | expand |
On Fri, Mar 27, 2020 at 12:02:00AM +0100, Rasmus Villemoes wrote: > 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 "<something> ? > env_sf_save : NULL" construction instead of a macro that just eats its > argument and expands to NULL. That way, if <something> is false, > env_sf_save gets eliminated as dead code, but the compiler still sees > the reference to it. > > For <something>, 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 <rasmus.villemoes at prevas.dk> Applied to u-boot/master, thanks!
On 09/05/2020 00.59, Tom Rini wrote: > On Fri, Mar 27, 2020 at 12:02:00AM +0100, Rasmus Villemoes wrote: > > > Applied to u-boot/master, thanks! > Eh, thanks, but you already applied v1 consisting of 5 patches. v1 1/5 corresponded to v2 1/2, while v1 5/5 corresponded to v2 2/2 - v1 3/5 and 4/5 were left out of v2. v1 2/5 was a helper macro I decided wasn't that much of a helper (but it's still needed since v1 3/5 and v1 4/5 were applied). This doesn't mean anything needs fixing up - I'm guessing you rebased the two patches, git saw that v2 1/2 was already applied, and then either you or git saw that most of v2 2/2 was already applied, so the only thing commit 6d3524c2ad does is to replace ENV_SAVE_PTR with its definition. Rasmus
On Sat, May 09, 2020 at 08:56:46PM +0200, Rasmus Villemoes wrote: > On 09/05/2020 00.59, Tom Rini wrote: > > On Fri, Mar 27, 2020 at 12:02:00AM +0100, Rasmus Villemoes wrote: > > > > > > Applied to u-boot/master, thanks! > > > > Eh, thanks, but you already applied v1 consisting of 5 patches. v1 1/5 > corresponded to v2 1/2, while v1 5/5 corresponded to v2 2/2 - v1 3/5 and > 4/5 were left out of v2. v1 2/5 was a helper macro I decided wasn't that > much of a helper (but it's still needed since v1 3/5 and v1 4/5 were > applied). > > This doesn't mean anything needs fixing up - I'm guessing you rebased > the two patches, git saw that v2 1/2 was already applied, and then > either you or git saw that most of v2 2/2 was already applied, so the > only thing commit 6d3524c2ad does is to replace ENV_SAVE_PTR with its > definition. :headdesk: So, 'git am' went through and applied what could be applied and I didn't see a "skipping" message. But at this point, are there any changes that need to still come in? Thanks!
On 09/05/2020 22.54, Tom Rini wrote: > On Sat, May 09, 2020 at 08:56:46PM +0200, Rasmus Villemoes wrote: >> On 09/05/2020 00.59, Tom Rini wrote: >>> On Fri, Mar 27, 2020 at 12:02:00AM +0100, Rasmus Villemoes wrote: >>> >>> >>> Applied to u-boot/master, thanks! >>> >> >> Eh, thanks, but you already applied v1 consisting of 5 patches. v1 1/5 >> corresponded to v2 1/2, while v1 5/5 corresponded to v2 2/2 - v1 3/5 and >> 4/5 were left out of v2. v1 2/5 was a helper macro I decided wasn't that >> much of a helper (but it's still needed since v1 3/5 and v1 4/5 were >> applied). >> >> This doesn't mean anything needs fixing up - I'm guessing you rebased >> the two patches, git saw that v2 1/2 was already applied, and then >> either you or git saw that most of v2 2/2 was already applied, so the >> only thing commit 6d3524c2ad does is to replace ENV_SAVE_PTR with its >> definition. > > :headdesk: > > So, 'git am' went through and applied what could be applied and I didn't > see a "skipping" message. But at this point, are there any changes that > need to still come in? Thanks! No, I think the code should all work. The history is a bit misleading as commit 6d3524c2ad doesn't have any functional change, it was all already done by the v1 patch which is applied as 9e3c94d11. But there's not much to be done about that, I guess. One could revert 6d3524c2ad without any damage and use the commit message to explain things, but I don't know if that's worth the churn. If you think it is, let me know and I'll try to draft a revert commit log which you can then edit to taste. Rasmus
On Mon, May 11, 2020 at 08:49:38AM +0200, Rasmus Villemoes wrote: > On 09/05/2020 22.54, Tom Rini wrote: > > On Sat, May 09, 2020 at 08:56:46PM +0200, Rasmus Villemoes wrote: > >> On 09/05/2020 00.59, Tom Rini wrote: > >>> On Fri, Mar 27, 2020 at 12:02:00AM +0100, Rasmus Villemoes wrote: > >>> > >>> > >>> Applied to u-boot/master, thanks! > >>> > >> > >> Eh, thanks, but you already applied v1 consisting of 5 patches. v1 1/5 > >> corresponded to v2 1/2, while v1 5/5 corresponded to v2 2/2 - v1 3/5 and > >> 4/5 were left out of v2. v1 2/5 was a helper macro I decided wasn't that > >> much of a helper (but it's still needed since v1 3/5 and v1 4/5 were > >> applied). > >> > >> This doesn't mean anything needs fixing up - I'm guessing you rebased > >> the two patches, git saw that v2 1/2 was already applied, and then > >> either you or git saw that most of v2 2/2 was already applied, so the > >> only thing commit 6d3524c2ad does is to replace ENV_SAVE_PTR with its > >> definition. > > > > :headdesk: > > > > So, 'git am' went through and applied what could be applied and I didn't > > see a "skipping" message. But at this point, are there any changes that > > need to still come in? Thanks! > > No, I think the code should all work. The history is a bit misleading as > commit 6d3524c2ad doesn't have any functional change, it was all already > done by the v1 patch which is applied as 9e3c94d11. But there's not much > to be done about that, I guess. One could revert 6d3524c2ad without any > damage and use the commit message to explain things, but I don't know if > that's worth the churn. If you think it is, let me know and I'll try to > draft a revert commit log which you can then edit to taste. I guess we should just leave it be then for now, thanks and sorry for the confusion.
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 <u-boot/crc.h> #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
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 "<something> ? env_sf_save : NULL" construction instead of a macro that just eats its argument and expands to NULL. That way, if <something> is false, env_sf_save gets eliminated as dead code, but the compiler still sees the reference to it. For <something>, 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 <rasmus.villemoes at prevas.dk> --- 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(-)