| Message ID | 20210128072108.32657-1-Harry.Waschkeit@men.de |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | env: sf: single function env_sf_save() | expand |
Hi Harry, On 28.01.21 08:21, Harry Waschkeit wrote: > Instead of implementing redundant environments in two very similar > functions env_sf_save(), handle redundancy in one function, placing the > few differences in appropriate pre-compiler sections depending on config > option CONFIG_ENV_OFFSET_REDUND. > > Additionally, several checkpatch complaints were addressed. > > This patch is in preparation for adding support for env erase. > > Signed-off-by: Harry Waschkeit <Harry.Waschkeit@men.de> I like this idea very much, thanks for working on this. One comment below.. > --- > env/sf.c | 132 ++++++++++++++++++------------------------------------- > 1 file changed, 43 insertions(+), 89 deletions(-) > > diff --git a/env/sf.c b/env/sf.c > index 937778aa37..c60bd1deed 100644 > --- a/env/sf.c > +++ b/env/sf.c > @@ -66,13 +66,16 @@ static int setup_flash_device(void) > return 0; > } > > -#if defined(CONFIG_ENV_OFFSET_REDUND) > static int env_sf_save(void) > { > env_t env_new; > - char *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE; > + char *saved_buffer = NULL; > u32 saved_size, saved_offset, sector; > + ulong offset; > int ret; > +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) > + char flag = ENV_REDUND_OBSOLETE; > +#endif No more #idef's if possible please. See below... > > ret = setup_flash_device(); > if (ret) > @@ -81,27 +84,33 @@ static int env_sf_save(void) > ret = env_export(&env_new); > if (ret) > return -EIO; > - env_new.flags = ENV_REDUND_ACTIVE; > > - if (gd->env_valid == ENV_VALID) { > - env_new_offset = CONFIG_ENV_OFFSET_REDUND; > - env_offset = CONFIG_ENV_OFFSET; > - } else { > - env_new_offset = CONFIG_ENV_OFFSET; > - env_offset = CONFIG_ENV_OFFSET_REDUND; > - } > +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) > + env_new.flags = ENV_REDUND_ACTIVE; > + > + if (gd->env_valid == ENV_VALID) { > + env_new_offset = CONFIG_ENV_OFFSET_REDUND; > + env_offset = CONFIG_ENV_OFFSET; > + } else { > + env_new_offset = CONFIG_ENV_OFFSET; > + env_offset = CONFIG_ENV_OFFSET_REDUND; > + } > + offset = env_new_offset; > +#else > + offset = CONFIG_ENV_OFFSET; > +#endif Can you please try to add this without adding #ifdef's but using if (CONFIG_IS_ENABLE(SYS_REDUNDAND_ENVIRONMENT)) instead? Thanks, Stefan > > /* Is the sector larger than the env (i.e. embedded) */ > if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { > saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE; > - saved_offset = env_new_offset + CONFIG_ENV_SIZE; > + saved_offset = offset + CONFIG_ENV_SIZE; > saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size); > if (!saved_buffer) { > ret = -ENOMEM; > goto done; > } > - ret = spi_flash_read(env_flash, saved_offset, > - saved_size, saved_buffer); > + ret = spi_flash_read(env_flash, saved_offset, saved_size, > + saved_buffer); > if (ret) > goto done; > } > @@ -109,35 +118,39 @@ static int env_sf_save(void) > sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE); > > puts("Erasing SPI flash..."); > - ret = spi_flash_erase(env_flash, env_new_offset, > - sector * CONFIG_ENV_SECT_SIZE); > + ret = spi_flash_erase(env_flash, offset, > + sector * CONFIG_ENV_SECT_SIZE); > if (ret) > goto done; > > puts("Writing to SPI flash..."); > > - ret = spi_flash_write(env_flash, env_new_offset, > - CONFIG_ENV_SIZE, &env_new); > + ret = spi_flash_write(env_flash, offset, > + CONFIG_ENV_SIZE, &env_new); > if (ret) > goto done; > > if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { > - ret = spi_flash_write(env_flash, saved_offset, > - saved_size, saved_buffer); > + ret = spi_flash_write(env_flash, saved_offset, saved_size, > + saved_buffer); > if (ret) > goto done; > } > > - ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags), > - sizeof(env_new.flags), &flag); > - if (ret) > - goto done; > +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) > + ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags), > + sizeof(env_new.flags), &flag); > + if (ret) > + goto done; > > - puts("done\n"); > + puts("done\n"); > > - gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND; > + gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND; > > - printf("Valid environment: %d\n", (int)gd->env_valid); > + printf("Valid environment: %d\n", (int)gd->env_valid); > +#else > + puts("done\n"); > +#endif > > done: > if (saved_buffer) > @@ -146,6 +159,7 @@ static int env_sf_save(void) > return ret; > } > > +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) > static int env_sf_load(void) > { > int ret; > @@ -183,66 +197,6 @@ out: > return ret; > } > #else > -static int env_sf_save(void) > -{ > - u32 saved_size, saved_offset, sector; > - char *saved_buffer = NULL; > - int ret = 1; > - env_t env_new; > - > - ret = setup_flash_device(); > - if (ret) > - return ret; > - > - /* Is the sector larger than the env (i.e. embedded) */ > - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { > - saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE; > - saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE; > - saved_buffer = malloc(saved_size); > - if (!saved_buffer) > - goto done; > - > - ret = spi_flash_read(env_flash, saved_offset, > - saved_size, saved_buffer); > - if (ret) > - goto done; > - } > - > - ret = env_export(&env_new); > - if (ret) > - goto done; > - > - sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE); > - > - puts("Erasing SPI flash..."); > - ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET, > - sector * CONFIG_ENV_SECT_SIZE); > - if (ret) > - goto done; > - > - puts("Writing to SPI flash..."); > - ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET, > - CONFIG_ENV_SIZE, &env_new); > - if (ret) > - goto done; > - > - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { > - ret = spi_flash_write(env_flash, saved_offset, > - saved_size, saved_buffer); > - if (ret) > - goto done; > - } > - > - ret = 0; > - puts("done\n"); > - > - done: > - if (saved_buffer) > - free(saved_buffer); > - > - return ret; > -} > - > static int env_sf_load(void) > { > int ret; > @@ -258,8 +212,8 @@ static int env_sf_load(void) > if (ret) > goto out; > > - ret = spi_flash_read(env_flash, > - CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf); > + ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, > + buf); > if (ret) { > env_set_default("spi_flash_read() failed", 0); > goto err_read; > @@ -292,7 +246,7 @@ static int env_sf_init(void) > env_t *env_ptr = (env_t *)env_sf_get_env_addr(); > > if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) { > - gd->env_addr = (ulong)&(env_ptr->data); > + gd->env_addr = (ulong)&env_ptr->data; > gd->env_valid = 1; > } else { > gd->env_addr = (ulong)&default_environment[0]; > Viele Grüße, Stefan
Hi Stefan, thanks a lot for your prompt reply :-) And sorry that I didn't manage to continue on that for such a long time ... On 28.01.21 09:50, Stefan Roese wrote: > Hi Harry, > > On 28.01.21 08:21, Harry Waschkeit wrote: >> Instead of implementing redundant environments in two very similar >> functions env_sf_save(), handle redundancy in one function, placing the >> few differences in appropriate pre-compiler sections depending on config >> option CONFIG_ENV_OFFSET_REDUND. >> >> Additionally, several checkpatch complaints were addressed. >> >> This patch is in preparation for adding support for env erase. >> >> Signed-off-by: Harry Waschkeit <Harry.Waschkeit@men.de> > > I like this idea very much, thanks for working on this. One comment > below.. > >> --- >> env/sf.c | 132 ++++++++++++++++++------------------------------------- >> 1 file changed, 43 insertions(+), 89 deletions(-) >> >> diff --git a/env/sf.c b/env/sf.c >> index 937778aa37..c60bd1deed 100644 >> --- a/env/sf.c >> +++ b/env/sf.c >> @@ -66,13 +66,16 @@ static int setup_flash_device(void) >> return 0; >> } >> -#if defined(CONFIG_ENV_OFFSET_REDUND) >> static int env_sf_save(void) >> { >> env_t env_new; >> - char *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE; >> + char *saved_buffer = NULL; >> u32 saved_size, saved_offset, sector; >> + ulong offset; >> int ret; >> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) >> + char flag = ENV_REDUND_OBSOLETE; >> +#endif > > No more #idef's if possible please. See below... As checkpatch.pl throws warnings for each such #if[def] occurrence, I already tried to get rid of them. But I can't see how this shall be possible in a structure declaration. I'm eager to learn how to do that if possible, though :-) > >> ret = setup_flash_device(); >> if (ret) >> @@ -81,27 +84,33 @@ static int env_sf_save(void) >> ret = env_export(&env_new); >> if (ret) >> return -EIO; >> - env_new.flags = ENV_REDUND_ACTIVE; >> - if (gd->env_valid == ENV_VALID) { >> - env_new_offset = CONFIG_ENV_OFFSET_REDUND; >> - env_offset = CONFIG_ENV_OFFSET; >> - } else { >> - env_new_offset = CONFIG_ENV_OFFSET; >> - env_offset = CONFIG_ENV_OFFSET_REDUND; >> - } >> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) >> + env_new.flags = ENV_REDUND_ACTIVE; >> + >> + if (gd->env_valid == ENV_VALID) { >> + env_new_offset = CONFIG_ENV_OFFSET_REDUND; >> + env_offset = CONFIG_ENV_OFFSET; >> + } else { >> + env_new_offset = CONFIG_ENV_OFFSET; >> + env_offset = CONFIG_ENV_OFFSET_REDUND; >> + } >> + offset = env_new_offset; >> +#else >> + offset = CONFIG_ENV_OFFSET; >> +#endif > > Can you please try to add this without adding #ifdef's but using > if (CONFIG_IS_ENABLE(SYS_REDUNDAND_ENVIRONMENT)) instead? I tried that, too, but it was doomed to fail, because element flags in env_t structure is only defined if redundant environment is enabled. Even though an "if (CONFIG_IS_ENABLED(...))" would statically evaluate to '0' without active redundancy in environments, the parser sees the syntax error of the non-existing structure element. So, I don't see a chance for avoiding the #if construct here, too. Let me know if I miss something :-) Cheers, Harry > Thanks, > Stefan > >> /* Is the sector larger than the env (i.e. embedded) */ >> if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >> saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE; >> - saved_offset = env_new_offset + CONFIG_ENV_SIZE; >> + saved_offset = offset + CONFIG_ENV_SIZE; >> saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size); >> if (!saved_buffer) { >> ret = -ENOMEM; >> goto done; >> } >> - ret = spi_flash_read(env_flash, saved_offset, >> - saved_size, saved_buffer); >> + ret = spi_flash_read(env_flash, saved_offset, saved_size, >> + saved_buffer); >> if (ret) >> goto done; >> } >> @@ -109,35 +118,39 @@ static int env_sf_save(void) >> sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE); >> puts("Erasing SPI flash..."); >> - ret = spi_flash_erase(env_flash, env_new_offset, >> - sector * CONFIG_ENV_SECT_SIZE); >> + ret = spi_flash_erase(env_flash, offset, >> + sector * CONFIG_ENV_SECT_SIZE); >> if (ret) >> goto done; >> puts("Writing to SPI flash..."); >> - ret = spi_flash_write(env_flash, env_new_offset, >> - CONFIG_ENV_SIZE, &env_new); >> + ret = spi_flash_write(env_flash, offset, >> + CONFIG_ENV_SIZE, &env_new); >> if (ret) >> goto done; >> if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >> - ret = spi_flash_write(env_flash, saved_offset, >> - saved_size, saved_buffer); >> + ret = spi_flash_write(env_flash, saved_offset, saved_size, >> + saved_buffer); >> if (ret) >> goto done; >> } >> - ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags), >> - sizeof(env_new.flags), &flag); >> - if (ret) >> - goto done; >> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) >> + ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags), >> + sizeof(env_new.flags), &flag); >> + if (ret) >> + goto done; >> - puts("done\n"); >> + puts("done\n"); >> - gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND; >> + gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND; >> - printf("Valid environment: %d\n", (int)gd->env_valid); >> + printf("Valid environment: %d\n", (int)gd->env_valid); >> +#else >> + puts("done\n"); >> +#endif >> done: >> if (saved_buffer) >> @@ -146,6 +159,7 @@ static int env_sf_save(void) >> return ret; >> } >> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) >> static int env_sf_load(void) >> { >> int ret; >> @@ -183,66 +197,6 @@ out: >> return ret; >> } >> #else >> -static int env_sf_save(void) >> -{ >> - u32 saved_size, saved_offset, sector; >> - char *saved_buffer = NULL; >> - int ret = 1; >> - env_t env_new; >> - >> - ret = setup_flash_device(); >> - if (ret) >> - return ret; >> - >> - /* Is the sector larger than the env (i.e. embedded) */ >> - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >> - saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE; >> - saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE; >> - saved_buffer = malloc(saved_size); >> - if (!saved_buffer) >> - goto done; >> - >> - ret = spi_flash_read(env_flash, saved_offset, >> - saved_size, saved_buffer); >> - if (ret) >> - goto done; >> - } >> - >> - ret = env_export(&env_new); >> - if (ret) >> - goto done; >> - >> - sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE); >> - >> - puts("Erasing SPI flash..."); >> - ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET, >> - sector * CONFIG_ENV_SECT_SIZE); >> - if (ret) >> - goto done; >> - >> - puts("Writing to SPI flash..."); >> - ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET, >> - CONFIG_ENV_SIZE, &env_new); >> - if (ret) >> - goto done; >> - >> - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >> - ret = spi_flash_write(env_flash, saved_offset, >> - saved_size, saved_buffer); >> - if (ret) >> - goto done; >> - } >> - >> - ret = 0; >> - puts("done\n"); >> - >> - done: >> - if (saved_buffer) >> - free(saved_buffer); >> - >> - return ret; >> -} >> - >> static int env_sf_load(void) >> { >> int ret; >> @@ -258,8 +212,8 @@ static int env_sf_load(void) >> if (ret) >> goto out; >> - ret = spi_flash_read(env_flash, >> - CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf); >> + ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, >> + buf); >> if (ret) { >> env_set_default("spi_flash_read() failed", 0); >> goto err_read; >> @@ -292,7 +246,7 @@ static int env_sf_init(void) >> env_t *env_ptr = (env_t *)env_sf_get_env_addr(); >> if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) { >> - gd->env_addr = (ulong)&(env_ptr->data); >> + gd->env_addr = (ulong)&env_ptr->data; >> gd->env_valid = 1; >> } else { >> gd->env_addr = (ulong)&default_environment[0]; >> > > > Viele Grüße, > Stefan >
Hi Harry, On 28.01.21 11:00, Harry Waschkeit wrote: > Hi Stefan, > > thanks a lot for your prompt reply :-) > > And sorry that I didn't manage to continue on that for such a long time ... > > > On 28.01.21 09:50, Stefan Roese wrote: >> Hi Harry, >> >> On 28.01.21 08:21, Harry Waschkeit wrote: >>> Instead of implementing redundant environments in two very similar >>> functions env_sf_save(), handle redundancy in one function, placing the >>> few differences in appropriate pre-compiler sections depending on config >>> option CONFIG_ENV_OFFSET_REDUND. >>> >>> Additionally, several checkpatch complaints were addressed. >>> >>> This patch is in preparation for adding support for env erase. >>> >>> Signed-off-by: Harry Waschkeit <Harry.Waschkeit@men.de> >> >> I like this idea very much, thanks for working on this. One comment >> below.. >> >>> --- >>> env/sf.c | 132 ++++++++++++++++++------------------------------------- >>> 1 file changed, 43 insertions(+), 89 deletions(-) >>> >>> diff --git a/env/sf.c b/env/sf.c >>> index 937778aa37..c60bd1deed 100644 >>> --- a/env/sf.c >>> +++ b/env/sf.c >>> @@ -66,13 +66,16 @@ static int setup_flash_device(void) >>> return 0; >>> } >>> -#if defined(CONFIG_ENV_OFFSET_REDUND) >>> static int env_sf_save(void) >>> { >>> env_t env_new; >>> - char *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE; >>> + char *saved_buffer = NULL; >>> u32 saved_size, saved_offset, sector; >>> + ulong offset; >>> int ret; >>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) >>> + char flag = ENV_REDUND_OBSOLETE; >>> +#endif >> >> No more #idef's if possible please. See below... > > As checkpatch.pl throws warnings for each such #if[def] occurrence, I > already tried to get rid of them. > But I can't see how this shall be possible in a structure declaration. > I'm eager to learn how to do that if possible, though :-) Ah, right. It's probably not possible for this for a struct declaration. But the variable above can be included always, right? >> >>> ret = setup_flash_device(); >>> if (ret) >>> @@ -81,27 +84,33 @@ static int env_sf_save(void) >>> ret = env_export(&env_new); >>> if (ret) >>> return -EIO; >>> - env_new.flags = ENV_REDUND_ACTIVE; >>> - if (gd->env_valid == ENV_VALID) { >>> - env_new_offset = CONFIG_ENV_OFFSET_REDUND; >>> - env_offset = CONFIG_ENV_OFFSET; >>> - } else { >>> - env_new_offset = CONFIG_ENV_OFFSET; >>> - env_offset = CONFIG_ENV_OFFSET_REDUND; >>> - } >>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) >>> + env_new.flags = ENV_REDUND_ACTIVE; >>> + >>> + if (gd->env_valid == ENV_VALID) { >>> + env_new_offset = CONFIG_ENV_OFFSET_REDUND; >>> + env_offset = CONFIG_ENV_OFFSET; >>> + } else { >>> + env_new_offset = CONFIG_ENV_OFFSET; >>> + env_offset = CONFIG_ENV_OFFSET_REDUND; >>> + } >>> + offset = env_new_offset; >>> +#else >>> + offset = CONFIG_ENV_OFFSET; >>> +#endif >> >> Can you please try to add this without adding #ifdef's but using >> if (CONFIG_IS_ENABLE(SYS_REDUNDAND_ENVIRONMENT)) instead? > > I tried that, too, but it was doomed to fail, because element flags in > env_t structure is only defined if redundant environment is enabled. Too bad we didn't include "flag" for non-redundant env in the beginning. Now its too late. > Even though an "if (CONFIG_IS_ENABLED(...))" would statically evaluate > to '0' without active redundancy in environments, the parser sees the > syntax error of the non-existing structure element. > > So, I don't see a chance for avoiding the #if construct here, too. > Let me know if I miss something :-) I agree its not easy or perhaps even impossible. One idea would be to introduce a union in the struct with "flags" also defines in the non-redundant case but not being used here. But this would result in very non-intuitive code AFAICT. So better not go this way. Perhaps somebody else has a quick idea on how to handle this without introduncing #ifdef's here? Thanks, Stefan > Cheers, > Harry > >> Thanks, >> Stefan >> >>> /* Is the sector larger than the env (i.e. embedded) */ >>> if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >>> saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE; >>> - saved_offset = env_new_offset + CONFIG_ENV_SIZE; >>> + saved_offset = offset + CONFIG_ENV_SIZE; >>> saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size); >>> if (!saved_buffer) { >>> ret = -ENOMEM; >>> goto done; >>> } >>> - ret = spi_flash_read(env_flash, saved_offset, >>> - saved_size, saved_buffer); >>> + ret = spi_flash_read(env_flash, saved_offset, saved_size, >>> + saved_buffer); >>> if (ret) >>> goto done; >>> } >>> @@ -109,35 +118,39 @@ static int env_sf_save(void) >>> sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE); >>> puts("Erasing SPI flash..."); >>> - ret = spi_flash_erase(env_flash, env_new_offset, >>> - sector * CONFIG_ENV_SECT_SIZE); >>> + ret = spi_flash_erase(env_flash, offset, >>> + sector * CONFIG_ENV_SECT_SIZE); >>> if (ret) >>> goto done; >>> puts("Writing to SPI flash..."); >>> - ret = spi_flash_write(env_flash, env_new_offset, >>> - CONFIG_ENV_SIZE, &env_new); >>> + ret = spi_flash_write(env_flash, offset, >>> + CONFIG_ENV_SIZE, &env_new); >>> if (ret) >>> goto done; >>> if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >>> - ret = spi_flash_write(env_flash, saved_offset, >>> - saved_size, saved_buffer); >>> + ret = spi_flash_write(env_flash, saved_offset, saved_size, >>> + saved_buffer); >>> if (ret) >>> goto done; >>> } >>> - ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, >>> flags), >>> - sizeof(env_new.flags), &flag); >>> - if (ret) >>> - goto done; >>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) >>> + ret = spi_flash_write(env_flash, env_offset + >>> offsetof(env_t, flags), >>> + sizeof(env_new.flags), &flag); >>> + if (ret) >>> + goto done; >>> - puts("done\n"); >>> + puts("done\n"); >>> - gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : >>> ENV_REDUND; >>> + gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : >>> ENV_REDUND; >>> - printf("Valid environment: %d\n", (int)gd->env_valid); >>> + printf("Valid environment: %d\n", (int)gd->env_valid); >>> +#else >>> + puts("done\n"); >>> +#endif >>> done: >>> if (saved_buffer) >>> @@ -146,6 +159,7 @@ static int env_sf_save(void) >>> return ret; >>> } >>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) >>> static int env_sf_load(void) >>> { >>> int ret; >>> @@ -183,66 +197,6 @@ out: >>> return ret; >>> } >>> #else >>> -static int env_sf_save(void) >>> -{ >>> - u32 saved_size, saved_offset, sector; >>> - char *saved_buffer = NULL; >>> - int ret = 1; >>> - env_t env_new; >>> - >>> - ret = setup_flash_device(); >>> - if (ret) >>> - return ret; >>> - >>> - /* Is the sector larger than the env (i.e. embedded) */ >>> - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >>> - saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE; >>> - saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE; >>> - saved_buffer = malloc(saved_size); >>> - if (!saved_buffer) >>> - goto done; >>> - >>> - ret = spi_flash_read(env_flash, saved_offset, >>> - saved_size, saved_buffer); >>> - if (ret) >>> - goto done; >>> - } >>> - >>> - ret = env_export(&env_new); >>> - if (ret) >>> - goto done; >>> - >>> - sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE); >>> - >>> - puts("Erasing SPI flash..."); >>> - ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET, >>> - sector * CONFIG_ENV_SECT_SIZE); >>> - if (ret) >>> - goto done; >>> - >>> - puts("Writing to SPI flash..."); >>> - ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET, >>> - CONFIG_ENV_SIZE, &env_new); >>> - if (ret) >>> - goto done; >>> - >>> - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >>> - ret = spi_flash_write(env_flash, saved_offset, >>> - saved_size, saved_buffer); >>> - if (ret) >>> - goto done; >>> - } >>> - >>> - ret = 0; >>> - puts("done\n"); >>> - >>> - done: >>> - if (saved_buffer) >>> - free(saved_buffer); >>> - >>> - return ret; >>> -} >>> - >>> static int env_sf_load(void) >>> { >>> int ret; >>> @@ -258,8 +212,8 @@ static int env_sf_load(void) >>> if (ret) >>> goto out; >>> - ret = spi_flash_read(env_flash, >>> - CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf); >>> + ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, >>> + buf); >>> if (ret) { >>> env_set_default("spi_flash_read() failed", 0); >>> goto err_read; >>> @@ -292,7 +246,7 @@ static int env_sf_init(void) >>> env_t *env_ptr = (env_t *)env_sf_get_env_addr(); >>> if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) { >>> - gd->env_addr = (ulong)&(env_ptr->data); >>> + gd->env_addr = (ulong)&env_ptr->data; >>> gd->env_valid = 1; >>> } else { >>> gd->env_addr = (ulong)&default_environment[0]; >>> >> >> >> Viele Grüße, >> Stefan >> > > Viele Grüße, Stefan
On 28.01.21 11:11, Stefan Roese wrote: > Hi Harry, > > On 28.01.21 11:00, Harry Waschkeit wrote: >> Hi Stefan, >> >> thanks a lot for your prompt reply :-) >> >> And sorry that I didn't manage to continue on that for such a long time ... >> >> >> On 28.01.21 09:50, Stefan Roese wrote: >>> Hi Harry, >>> >>> On 28.01.21 08:21, Harry Waschkeit wrote: >>>> Instead of implementing redundant environments in two very similar >>>> functions env_sf_save(), handle redundancy in one function, placing the >>>> few differences in appropriate pre-compiler sections depending on config >>>> option CONFIG_ENV_OFFSET_REDUND. >>>> >>>> Additionally, several checkpatch complaints were addressed. >>>> >>>> This patch is in preparation for adding support for env erase. >>>> >>>> Signed-off-by: Harry Waschkeit <Harry.Waschkeit@men.de> >>> >>> I like this idea very much, thanks for working on this. One comment >>> below.. >>> >>>> --- >>>> env/sf.c | 132 ++++++++++++++++++------------------------------------- >>>> 1 file changed, 43 insertions(+), 89 deletions(-) >>>> >>>> diff --git a/env/sf.c b/env/sf.c >>>> index 937778aa37..c60bd1deed 100644 >>>> --- a/env/sf.c >>>> +++ b/env/sf.c >>>> @@ -66,13 +66,16 @@ static int setup_flash_device(void) >>>> return 0; >>>> } >>>> -#if defined(CONFIG_ENV_OFFSET_REDUND) >>>> static int env_sf_save(void) >>>> { >>>> env_t env_new; >>>> - char *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE; >>>> + char *saved_buffer = NULL; >>>> u32 saved_size, saved_offset, sector; >>>> + ulong offset; >>>> int ret; >>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) >>>> + char flag = ENV_REDUND_OBSOLETE; >>>> +#endif >>> >>> No more #idef's if possible please. See below... >> >> As checkpatch.pl throws warnings for each such #if[def] occurrence, I already tried to get rid of them. >> But I can't see how this shall be possible in a structure declaration. >> I'm eager to learn how to do that if possible, though :-) > > Ah, right. It's probably not possible for this for a struct declaration. > But the variable above can be included always, right? Sure it could, but then we would have to live with a compiler warning about an unused variable I think. >>> >>>> ret = setup_flash_device(); >>>> if (ret) >>>> @@ -81,27 +84,33 @@ static int env_sf_save(void) >>>> ret = env_export(&env_new); >>>> if (ret) >>>> return -EIO; >>>> - env_new.flags = ENV_REDUND_ACTIVE; >>>> - if (gd->env_valid == ENV_VALID) { >>>> - env_new_offset = CONFIG_ENV_OFFSET_REDUND; >>>> - env_offset = CONFIG_ENV_OFFSET; >>>> - } else { >>>> - env_new_offset = CONFIG_ENV_OFFSET; >>>> - env_offset = CONFIG_ENV_OFFSET_REDUND; >>>> - } >>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) >>>> + env_new.flags = ENV_REDUND_ACTIVE; >>>> + >>>> + if (gd->env_valid == ENV_VALID) { >>>> + env_new_offset = CONFIG_ENV_OFFSET_REDUND; >>>> + env_offset = CONFIG_ENV_OFFSET; >>>> + } else { >>>> + env_new_offset = CONFIG_ENV_OFFSET; >>>> + env_offset = CONFIG_ENV_OFFSET_REDUND; >>>> + } >>>> + offset = env_new_offset; >>>> +#else >>>> + offset = CONFIG_ENV_OFFSET; >>>> +#endif >>> >>> Can you please try to add this without adding #ifdef's but using >>> if (CONFIG_IS_ENABLE(SYS_REDUNDAND_ENVIRONMENT)) instead? >> >> I tried that, too, but it was doomed to fail, because element flags in env_t structure is only defined if redundant environment is enabled. > > Too bad we didn't include "flag" for non-redundant env in the beginning. > Now its too late. That would probably have been the best solution to avoid #if's, at least in that file. But keep in mind: as a last consequence of this approach you would end up in structures bearing variables for all eventualities, however unlikely these are needed, blowing up the memory footprint for no reason. In my opinion such things need to be thought through thoroughly :-) >> Even though an "if (CONFIG_IS_ENABLED(...))" would statically evaluate to '0' without active redundancy in environments, the parser sees the syntax error of the non-existing structure element. >> >> So, I don't see a chance for avoiding the #if construct here, too. >> Let me know if I miss something :-) > > I agree its not easy or perhaps even impossible. One idea would be > to introduce a union in the struct with "flags" also defines in the > non-redundant case but not being used here. But this would result > in very non-intuitive code AFAICT. So better not go this way. That would feel very strange to me, too. > Perhaps somebody else has a quick idea on how to handle this without > introduncing #ifdef's here? It would be possible to introduce a macro like env_set_flags(envp, flag) in env.h that only evaluates to something in case of active redundancy, sure. But that's not even half of a solution, because also variables env_offset and env_new_offset which are set in that section are only defined if necessary, i.e. if CONFIG_SYS_REDUNDAND_ENVIRONMENT is set (btw also protected by #if while not possible otherwise). The trade-off here is between code duplication - which I removed by combining two very similar separate functions for the two situations w/ and w/o redundancy in one - and in-line pre-compiler protected regions within one function. While I fully agree that the latter should be avoided as far as possible, it is not avoidable here afaics. And avoiding code duplication here outweighs the few pre-compiler occurrences in my eyes, but that's just my € 0,02 :-) Viele Grüße, Harry > > Thanks, > Stefan > >> Cheers, >> Harry >> >>> Thanks, >>> Stefan >>> >>>> /* Is the sector larger than the env (i.e. embedded) */ >>>> if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >>>> saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE; >>>> - saved_offset = env_new_offset + CONFIG_ENV_SIZE; >>>> + saved_offset = offset + CONFIG_ENV_SIZE; >>>> saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size); >>>> if (!saved_buffer) { >>>> ret = -ENOMEM; >>>> goto done; >>>> } >>>> - ret = spi_flash_read(env_flash, saved_offset, >>>> - saved_size, saved_buffer); >>>> + ret = spi_flash_read(env_flash, saved_offset, saved_size, >>>> + saved_buffer); >>>> if (ret) >>>> goto done; >>>> } >>>> @@ -109,35 +118,39 @@ static int env_sf_save(void) >>>> sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE); >>>> puts("Erasing SPI flash..."); >>>> - ret = spi_flash_erase(env_flash, env_new_offset, >>>> - sector * CONFIG_ENV_SECT_SIZE); >>>> + ret = spi_flash_erase(env_flash, offset, >>>> + sector * CONFIG_ENV_SECT_SIZE); >>>> if (ret) >>>> goto done; >>>> puts("Writing to SPI flash..."); >>>> - ret = spi_flash_write(env_flash, env_new_offset, >>>> - CONFIG_ENV_SIZE, &env_new); >>>> + ret = spi_flash_write(env_flash, offset, >>>> + CONFIG_ENV_SIZE, &env_new); >>>> if (ret) >>>> goto done; >>>> if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >>>> - ret = spi_flash_write(env_flash, saved_offset, >>>> - saved_size, saved_buffer); >>>> + ret = spi_flash_write(env_flash, saved_offset, saved_size, >>>> + saved_buffer); >>>> if (ret) >>>> goto done; >>>> } >>>> - ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags), >>>> - sizeof(env_new.flags), &flag); >>>> - if (ret) >>>> - goto done; >>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) >>>> + ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags), >>>> + sizeof(env_new.flags), &flag); >>>> + if (ret) >>>> + goto done; >>>> - puts("done\n"); >>>> + puts("done\n"); >>>> - gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND; >>>> + gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND; >>>> - printf("Valid environment: %d\n", (int)gd->env_valid); >>>> + printf("Valid environment: %d\n", (int)gd->env_valid); >>>> +#else >>>> + puts("done\n"); >>>> +#endif >>>> done: >>>> if (saved_buffer) >>>> @@ -146,6 +159,7 @@ static int env_sf_save(void) >>>> return ret; >>>> } >>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) >>>> static int env_sf_load(void) >>>> { >>>> int ret; >>>> @@ -183,66 +197,6 @@ out: >>>> return ret; >>>> } >>>> #else >>>> -static int env_sf_save(void) >>>> -{ >>>> - u32 saved_size, saved_offset, sector; >>>> - char *saved_buffer = NULL; >>>> - int ret = 1; >>>> - env_t env_new; >>>> - >>>> - ret = setup_flash_device(); >>>> - if (ret) >>>> - return ret; >>>> - >>>> - /* Is the sector larger than the env (i.e. embedded) */ >>>> - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >>>> - saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE; >>>> - saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE; >>>> - saved_buffer = malloc(saved_size); >>>> - if (!saved_buffer) >>>> - goto done; >>>> - >>>> - ret = spi_flash_read(env_flash, saved_offset, >>>> - saved_size, saved_buffer); >>>> - if (ret) >>>> - goto done; >>>> - } >>>> - >>>> - ret = env_export(&env_new); >>>> - if (ret) >>>> - goto done; >>>> - >>>> - sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE); >>>> - >>>> - puts("Erasing SPI flash..."); >>>> - ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET, >>>> - sector * CONFIG_ENV_SECT_SIZE); >>>> - if (ret) >>>> - goto done; >>>> - >>>> - puts("Writing to SPI flash..."); >>>> - ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET, >>>> - CONFIG_ENV_SIZE, &env_new); >>>> - if (ret) >>>> - goto done; >>>> - >>>> - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >>>> - ret = spi_flash_write(env_flash, saved_offset, >>>> - saved_size, saved_buffer); >>>> - if (ret) >>>> - goto done; >>>> - } >>>> - >>>> - ret = 0; >>>> - puts("done\n"); >>>> - >>>> - done: >>>> - if (saved_buffer) >>>> - free(saved_buffer); >>>> - >>>> - return ret; >>>> -} >>>> - >>>> static int env_sf_load(void) >>>> { >>>> int ret; >>>> @@ -258,8 +212,8 @@ static int env_sf_load(void) >>>> if (ret) >>>> goto out; >>>> - ret = spi_flash_read(env_flash, >>>> - CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf); >>>> + ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, >>>> + buf); >>>> if (ret) { >>>> env_set_default("spi_flash_read() failed", 0); >>>> goto err_read; >>>> @@ -292,7 +246,7 @@ static int env_sf_init(void) >>>> env_t *env_ptr = (env_t *)env_sf_get_env_addr(); >>>> if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) { >>>> - gd->env_addr = (ulong)&(env_ptr->data); >>>> + gd->env_addr = (ulong)&env_ptr->data; >>>> gd->env_valid = 1; >>>> } else { >>>> gd->env_addr = (ulong)&default_environment[0]; >>>> >>> >>> >>> Viele Grüße, >>> Stefan >>> >> >> > > > Viele Grüße, > Stefan >
On 28.01.21 12:21, Harry Waschkeit wrote: <snip> >>> Even though an "if (CONFIG_IS_ENABLED(...))" would statically >>> evaluate to '0' without active redundancy in environments, the parser >>> sees the syntax error of the non-existing structure element. >>> >>> So, I don't see a chance for avoiding the #if construct here, too. >>> Let me know if I miss something :-) >> >> I agree its not easy or perhaps even impossible. One idea would be >> to introduce a union in the struct with "flags" also defines in the >> non-redundant case but not being used here. But this would result >> in very non-intuitive code AFAICT. So better not go this way. > > That would feel very strange to me, too. > >> Perhaps somebody else has a quick idea on how to handle this without >> introduncing #ifdef's here? > > It would be possible to introduce a macro like env_set_flags(envp, flag) > in env.h that only evaluates to something in case of active redundancy, > sure. > > But that's not even half of a solution, because also variables > env_offset and env_new_offset which are set in that section are only > defined if necessary, i.e. if CONFIG_SYS_REDUNDAND_ENVIRONMENT is set > (btw also protected by #if while not possible otherwise). > > The trade-off here is between code duplication - which I removed by > combining two very similar separate functions for the two situations w/ > and w/o redundancy in one - and in-line pre-compiler protected regions > within one function. > > While I fully agree that the latter should be avoided as far as > possible, it is not avoidable here afaics. > > And avoiding code duplication here outweighs the few pre-compiler > occurrences in my eyes, but that's just my € 0,02 :-) I also agree (now). If nobody else comes up with a better idea, then please proceed with the current implementation to remove the code duplication. Thanks, Stefan > > Viele Grüße, > Harry > >> >> Thanks, >> Stefan >> >>> Cheers, >>> Harry >>> >>>> Thanks, >>>> Stefan >>>> >>>>> /* Is the sector larger than the env (i.e. embedded) */ >>>>> if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >>>>> saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE; >>>>> - saved_offset = env_new_offset + CONFIG_ENV_SIZE; >>>>> + saved_offset = offset + CONFIG_ENV_SIZE; >>>>> saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size); >>>>> if (!saved_buffer) { >>>>> ret = -ENOMEM; >>>>> goto done; >>>>> } >>>>> - ret = spi_flash_read(env_flash, saved_offset, >>>>> - saved_size, saved_buffer); >>>>> + ret = spi_flash_read(env_flash, saved_offset, saved_size, >>>>> + saved_buffer); >>>>> if (ret) >>>>> goto done; >>>>> } >>>>> @@ -109,35 +118,39 @@ static int env_sf_save(void) >>>>> sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE); >>>>> puts("Erasing SPI flash..."); >>>>> - ret = spi_flash_erase(env_flash, env_new_offset, >>>>> - sector * CONFIG_ENV_SECT_SIZE); >>>>> + ret = spi_flash_erase(env_flash, offset, >>>>> + sector * CONFIG_ENV_SECT_SIZE); >>>>> if (ret) >>>>> goto done; >>>>> puts("Writing to SPI flash..."); >>>>> - ret = spi_flash_write(env_flash, env_new_offset, >>>>> - CONFIG_ENV_SIZE, &env_new); >>>>> + ret = spi_flash_write(env_flash, offset, >>>>> + CONFIG_ENV_SIZE, &env_new); >>>>> if (ret) >>>>> goto done; >>>>> if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >>>>> - ret = spi_flash_write(env_flash, saved_offset, >>>>> - saved_size, saved_buffer); >>>>> + ret = spi_flash_write(env_flash, saved_offset, saved_size, >>>>> + saved_buffer); >>>>> if (ret) >>>>> goto done; >>>>> } >>>>> - ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, >>>>> flags), >>>>> - sizeof(env_new.flags), &flag); >>>>> - if (ret) >>>>> - goto done; >>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) >>>>> + ret = spi_flash_write(env_flash, env_offset + >>>>> offsetof(env_t, flags), >>>>> + sizeof(env_new.flags), &flag); >>>>> + if (ret) >>>>> + goto done; >>>>> - puts("done\n"); >>>>> + puts("done\n"); >>>>> - gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : >>>>> ENV_REDUND; >>>>> + gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : >>>>> ENV_REDUND; >>>>> - printf("Valid environment: %d\n", (int)gd->env_valid); >>>>> + printf("Valid environment: %d\n", (int)gd->env_valid); >>>>> +#else >>>>> + puts("done\n"); >>>>> +#endif >>>>> done: >>>>> if (saved_buffer) >>>>> @@ -146,6 +159,7 @@ static int env_sf_save(void) >>>>> return ret; >>>>> } >>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) >>>>> static int env_sf_load(void) >>>>> { >>>>> int ret; >>>>> @@ -183,66 +197,6 @@ out: >>>>> return ret; >>>>> } >>>>> #else >>>>> -static int env_sf_save(void) >>>>> -{ >>>>> - u32 saved_size, saved_offset, sector; >>>>> - char *saved_buffer = NULL; >>>>> - int ret = 1; >>>>> - env_t env_new; >>>>> - >>>>> - ret = setup_flash_device(); >>>>> - if (ret) >>>>> - return ret; >>>>> - >>>>> - /* Is the sector larger than the env (i.e. embedded) */ >>>>> - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >>>>> - saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE; >>>>> - saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE; >>>>> - saved_buffer = malloc(saved_size); >>>>> - if (!saved_buffer) >>>>> - goto done; >>>>> - >>>>> - ret = spi_flash_read(env_flash, saved_offset, >>>>> - saved_size, saved_buffer); >>>>> - if (ret) >>>>> - goto done; >>>>> - } >>>>> - >>>>> - ret = env_export(&env_new); >>>>> - if (ret) >>>>> - goto done; >>>>> - >>>>> - sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE); >>>>> - >>>>> - puts("Erasing SPI flash..."); >>>>> - ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET, >>>>> - sector * CONFIG_ENV_SECT_SIZE); >>>>> - if (ret) >>>>> - goto done; >>>>> - >>>>> - puts("Writing to SPI flash..."); >>>>> - ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET, >>>>> - CONFIG_ENV_SIZE, &env_new); >>>>> - if (ret) >>>>> - goto done; >>>>> - >>>>> - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >>>>> - ret = spi_flash_write(env_flash, saved_offset, >>>>> - saved_size, saved_buffer); >>>>> - if (ret) >>>>> - goto done; >>>>> - } >>>>> - >>>>> - ret = 0; >>>>> - puts("done\n"); >>>>> - >>>>> - done: >>>>> - if (saved_buffer) >>>>> - free(saved_buffer); >>>>> - >>>>> - return ret; >>>>> -} >>>>> - >>>>> static int env_sf_load(void) >>>>> { >>>>> int ret; >>>>> @@ -258,8 +212,8 @@ static int env_sf_load(void) >>>>> if (ret) >>>>> goto out; >>>>> - ret = spi_flash_read(env_flash, >>>>> - CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf); >>>>> + ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, >>>>> CONFIG_ENV_SIZE, >>>>> + buf); >>>>> if (ret) { >>>>> env_set_default("spi_flash_read() failed", 0); >>>>> goto err_read; >>>>> @@ -292,7 +246,7 @@ static int env_sf_init(void) >>>>> env_t *env_ptr = (env_t *)env_sf_get_env_addr(); >>>>> if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) { >>>>> - gd->env_addr = (ulong)&(env_ptr->data); >>>>> + gd->env_addr = (ulong)&env_ptr->data; >>>>> gd->env_valid = 1; >>>>> } else { >>>>> gd->env_addr = (ulong)&default_environment[0]; >>>>> >>>> >>>> >>>> Viele Grüße, >>>> Stefan >>>> >>> >>> >> >> >> Viele Grüße, >> Stefan >> > > Viele Grüße, Stefan
Hi again Stefan, On 29.01.21 08:16, Stefan Roese wrote: > On 28.01.21 12:21, Harry Waschkeit wrote: > > <snip> > >>>> Even though an "if (CONFIG_IS_ENABLED(...))" would statically evaluate to '0' without active redundancy in environments, the parser sees the syntax error of the non-existing structure element. >>>> >>>> So, I don't see a chance for avoiding the #if construct here, too. >>>> Let me know if I miss something :-) >>> >>> I agree its not easy or perhaps even impossible. One idea would be >>> to introduce a union in the struct with "flags" also defines in the >>> non-redundant case but not being used here. But this would result >>> in very non-intuitive code AFAICT. So better not go this way. >> >> That would feel very strange to me, too. >> >>> Perhaps somebody else has a quick idea on how to handle this without >>> introduncing #ifdef's here? >> >> It would be possible to introduce a macro like env_set_flags(envp, flag) in env.h that only evaluates to something in case of active redundancy, sure. >> >> But that's not even half of a solution, because also variables env_offset and env_new_offset which are set in that section are only defined if necessary, i.e. if CONFIG_SYS_REDUNDAND_ENVIRONMENT is set (btw also protected by #if while not possible otherwise). >> >> The trade-off here is between code duplication - which I removed by combining two very similar separate functions for the two situations w/ and w/o redundancy in one - and in-line pre-compiler protected regions within one function. >> >> While I fully agree that the latter should be avoided as far as possible, it is not avoidable here afaics. >> >> And avoiding code duplication here outweighs the few pre-compiler occurrences in my eyes, but that's just my € 0,02 :-) > > I also agree (now). If nobody else comes up with a better idea, then > please proceed with the current implementation to remove the code > duplication. sorry for the newbie question, I'm not familiar (yet) with the normal procedure and I don't want to keep that ball from rolling ;-/ As far as I understood you finally agreed on my patch, but you didn't give it a "reviewed-by" all the same; so do you still expect me to change my patch in some way or are you waiting for a "reviewed-by" from someone else to give that a go? Why I am also asking: I have another small patch in he pipe that bases on the changed sources and that adds "env erase" support on top of that :-) Thanks, Harry > > Thanks, > Stefan > >> >> Viele Grüße, >> Harry >> >>> >>> Thanks, >>> Stefan >>> >>>> Cheers, >>>> Harry >>>> >>>>> Thanks, >>>>> Stefan >>>>> >>>>>> /* Is the sector larger than the env (i.e. embedded) */ >>>>>> if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >>>>>> saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE; >>>>>> - saved_offset = env_new_offset + CONFIG_ENV_SIZE; >>>>>> + saved_offset = offset + CONFIG_ENV_SIZE; >>>>>> saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size); >>>>>> if (!saved_buffer) { >>>>>> ret = -ENOMEM; >>>>>> goto done; >>>>>> } >>>>>> - ret = spi_flash_read(env_flash, saved_offset, >>>>>> - saved_size, saved_buffer); >>>>>> + ret = spi_flash_read(env_flash, saved_offset, saved_size, >>>>>> + saved_buffer); >>>>>> if (ret) >>>>>> goto done; >>>>>> } >>>>>> @@ -109,35 +118,39 @@ static int env_sf_save(void) >>>>>> sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE); >>>>>> puts("Erasing SPI flash..."); >>>>>> - ret = spi_flash_erase(env_flash, env_new_offset, >>>>>> - sector * CONFIG_ENV_SECT_SIZE); >>>>>> + ret = spi_flash_erase(env_flash, offset, >>>>>> + sector * CONFIG_ENV_SECT_SIZE); >>>>>> if (ret) >>>>>> goto done; >>>>>> puts("Writing to SPI flash..."); >>>>>> - ret = spi_flash_write(env_flash, env_new_offset, >>>>>> - CONFIG_ENV_SIZE, &env_new); >>>>>> + ret = spi_flash_write(env_flash, offset, >>>>>> + CONFIG_ENV_SIZE, &env_new); >>>>>> if (ret) >>>>>> goto done; >>>>>> if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >>>>>> - ret = spi_flash_write(env_flash, saved_offset, >>>>>> - saved_size, saved_buffer); >>>>>> + ret = spi_flash_write(env_flash, saved_offset, saved_size, >>>>>> + saved_buffer); >>>>>> if (ret) >>>>>> goto done; >>>>>> } >>>>>> - ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags), >>>>>> - sizeof(env_new.flags), &flag); >>>>>> - if (ret) >>>>>> - goto done; >>>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) >>>>>> + ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags), >>>>>> + sizeof(env_new.flags), &flag); >>>>>> + if (ret) >>>>>> + goto done; >>>>>> - puts("done\n"); >>>>>> + puts("done\n"); >>>>>> - gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND; >>>>>> + gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND; >>>>>> - printf("Valid environment: %d\n", (int)gd->env_valid); >>>>>> + printf("Valid environment: %d\n", (int)gd->env_valid); >>>>>> +#else >>>>>> + puts("done\n"); >>>>>> +#endif >>>>>> done: >>>>>> if (saved_buffer) >>>>>> @@ -146,6 +159,7 @@ static int env_sf_save(void) >>>>>> return ret; >>>>>> } >>>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) >>>>>> static int env_sf_load(void) >>>>>> { >>>>>> int ret; >>>>>> @@ -183,66 +197,6 @@ out: >>>>>> return ret; >>>>>> } >>>>>> #else >>>>>> -static int env_sf_save(void) >>>>>> -{ >>>>>> - u32 saved_size, saved_offset, sector; >>>>>> - char *saved_buffer = NULL; >>>>>> - int ret = 1; >>>>>> - env_t env_new; >>>>>> - >>>>>> - ret = setup_flash_device(); >>>>>> - if (ret) >>>>>> - return ret; >>>>>> - >>>>>> - /* Is the sector larger than the env (i.e. embedded) */ >>>>>> - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >>>>>> - saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE; >>>>>> - saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE; >>>>>> - saved_buffer = malloc(saved_size); >>>>>> - if (!saved_buffer) >>>>>> - goto done; >>>>>> - >>>>>> - ret = spi_flash_read(env_flash, saved_offset, >>>>>> - saved_size, saved_buffer); >>>>>> - if (ret) >>>>>> - goto done; >>>>>> - } >>>>>> - >>>>>> - ret = env_export(&env_new); >>>>>> - if (ret) >>>>>> - goto done; >>>>>> - >>>>>> - sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE); >>>>>> - >>>>>> - puts("Erasing SPI flash..."); >>>>>> - ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET, >>>>>> - sector * CONFIG_ENV_SECT_SIZE); >>>>>> - if (ret) >>>>>> - goto done; >>>>>> - >>>>>> - puts("Writing to SPI flash..."); >>>>>> - ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET, >>>>>> - CONFIG_ENV_SIZE, &env_new); >>>>>> - if (ret) >>>>>> - goto done; >>>>>> - >>>>>> - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >>>>>> - ret = spi_flash_write(env_flash, saved_offset, >>>>>> - saved_size, saved_buffer); >>>>>> - if (ret) >>>>>> - goto done; >>>>>> - } >>>>>> - >>>>>> - ret = 0; >>>>>> - puts("done\n"); >>>>>> - >>>>>> - done: >>>>>> - if (saved_buffer) >>>>>> - free(saved_buffer); >>>>>> - >>>>>> - return ret; >>>>>> -} >>>>>> - >>>>>> static int env_sf_load(void) >>>>>> { >>>>>> int ret; >>>>>> @@ -258,8 +212,8 @@ static int env_sf_load(void) >>>>>> if (ret) >>>>>> goto out; >>>>>> - ret = spi_flash_read(env_flash, >>>>>> - CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf); >>>>>> + ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, >>>>>> + buf); >>>>>> if (ret) { >>>>>> env_set_default("spi_flash_read() failed", 0); >>>>>> goto err_read; >>>>>> @@ -292,7 +246,7 @@ static int env_sf_init(void) >>>>>> env_t *env_ptr = (env_t *)env_sf_get_env_addr(); >>>>>> if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) { >>>>>> - gd->env_addr = (ulong)&(env_ptr->data); >>>>>> + gd->env_addr = (ulong)&env_ptr->data; >>>>>> gd->env_valid = 1; >>>>>> } else { >>>>>> gd->env_addr = (ulong)&default_environment[0]; >>>>>> >>>>> >>>>> >>>>> Viele Grüße, >>>>> Stefan >>>>> >>>> >>>> >>> >>> >>> Viele Grüße, >>> Stefan >>> >> >> > > > Viele Grüße, > Stefan >
Hi Harry, On 29.01.21 18:18, Harry Waschkeit wrote: > Hi again Stefan, > > On 29.01.21 08:16, Stefan Roese wrote: >> On 28.01.21 12:21, Harry Waschkeit wrote: >> >> <snip> >> >>>>> Even though an "if (CONFIG_IS_ENABLED(...))" would statically >>>>> evaluate to '0' without active redundancy in environments, the >>>>> parser sees the syntax error of the non-existing structure element. >>>>> >>>>> So, I don't see a chance for avoiding the #if construct here, too. >>>>> Let me know if I miss something :-) >>>> >>>> I agree its not easy or perhaps even impossible. One idea would be >>>> to introduce a union in the struct with "flags" also defines in the >>>> non-redundant case but not being used here. But this would result >>>> in very non-intuitive code AFAICT. So better not go this way. >>> >>> That would feel very strange to me, too. >>> >>>> Perhaps somebody else has a quick idea on how to handle this without >>>> introduncing #ifdef's here? >>> >>> It would be possible to introduce a macro like env_set_flags(envp, >>> flag) in env.h that only evaluates to something in case of active >>> redundancy, sure. >>> >>> But that's not even half of a solution, because also variables >>> env_offset and env_new_offset which are set in that section are only >>> defined if necessary, i.e. if CONFIG_SYS_REDUNDAND_ENVIRONMENT is set >>> (btw also protected by #if while not possible otherwise). >>> >>> The trade-off here is between code duplication - which I removed by >>> combining two very similar separate functions for the two situations >>> w/ and w/o redundancy in one - and in-line pre-compiler protected >>> regions within one function. >>> >>> While I fully agree that the latter should be avoided as far as >>> possible, it is not avoidable here afaics. >>> >>> And avoiding code duplication here outweighs the few pre-compiler >>> occurrences in my eyes, but that's just my € 0,02 :-) >> >> I also agree (now). If nobody else comes up with a better idea, then >> please proceed with the current implementation to remove the code >> duplication. > > sorry for the newbie question, I'm not familiar (yet) with the normal > procedure and I don't want to keep that ball from rolling ;-/ No Problem. Please double-check if not 100% sure. ;) > As far as I understood you finally agreed on my patch, but you didn't > give it a "reviewed-by" all the same; so do you still expect me to > change my patch in some way or are you waiting for a "reviewed-by" from > someone else to give that a go? > > Why I am also asking: I have another small patch in he pipe that bases > on the changed sources and that adds "env erase" support on top of that :-) I didn't do a thorough review yet. Let me try to do this quickly... Thanks, Stefan > Thanks, > Harry > >> >> Thanks, >> Stefan >> >>> >>> Viele Grüße, >>> Harry >>> >>>> >>>> Thanks, >>>> Stefan >>>> >>>>> Cheers, >>>>> Harry >>>>> >>>>>> Thanks, >>>>>> Stefan >>>>>> >>>>>>> /* Is the sector larger than the env (i.e. embedded) */ >>>>>>> if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >>>>>>> saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE; >>>>>>> - saved_offset = env_new_offset + CONFIG_ENV_SIZE; >>>>>>> + saved_offset = offset + CONFIG_ENV_SIZE; >>>>>>> saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size); >>>>>>> if (!saved_buffer) { >>>>>>> ret = -ENOMEM; >>>>>>> goto done; >>>>>>> } >>>>>>> - ret = spi_flash_read(env_flash, saved_offset, >>>>>>> - saved_size, saved_buffer); >>>>>>> + ret = spi_flash_read(env_flash, saved_offset, saved_size, >>>>>>> + saved_buffer); >>>>>>> if (ret) >>>>>>> goto done; >>>>>>> } >>>>>>> @@ -109,35 +118,39 @@ static int env_sf_save(void) >>>>>>> sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE); >>>>>>> puts("Erasing SPI flash..."); >>>>>>> - ret = spi_flash_erase(env_flash, env_new_offset, >>>>>>> - sector * CONFIG_ENV_SECT_SIZE); >>>>>>> + ret = spi_flash_erase(env_flash, offset, >>>>>>> + sector * CONFIG_ENV_SECT_SIZE); >>>>>>> if (ret) >>>>>>> goto done; >>>>>>> puts("Writing to SPI flash..."); >>>>>>> - ret = spi_flash_write(env_flash, env_new_offset, >>>>>>> - CONFIG_ENV_SIZE, &env_new); >>>>>>> + ret = spi_flash_write(env_flash, offset, >>>>>>> + CONFIG_ENV_SIZE, &env_new); >>>>>>> if (ret) >>>>>>> goto done; >>>>>>> if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >>>>>>> - ret = spi_flash_write(env_flash, saved_offset, >>>>>>> - saved_size, saved_buffer); >>>>>>> + ret = spi_flash_write(env_flash, saved_offset, saved_size, >>>>>>> + saved_buffer); >>>>>>> if (ret) >>>>>>> goto done; >>>>>>> } >>>>>>> - ret = spi_flash_write(env_flash, env_offset + >>>>>>> offsetof(env_t, flags), >>>>>>> - sizeof(env_new.flags), &flag); >>>>>>> - if (ret) >>>>>>> - goto done; >>>>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) >>>>>>> + ret = spi_flash_write(env_flash, env_offset + >>>>>>> offsetof(env_t, flags), >>>>>>> + sizeof(env_new.flags), &flag); >>>>>>> + if (ret) >>>>>>> + goto done; >>>>>>> - puts("done\n"); >>>>>>> + puts("done\n"); >>>>>>> - gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : >>>>>>> ENV_REDUND; >>>>>>> + gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID >>>>>>> : ENV_REDUND; >>>>>>> - printf("Valid environment: %d\n", (int)gd->env_valid); >>>>>>> + printf("Valid environment: %d\n", (int)gd->env_valid); >>>>>>> +#else >>>>>>> + puts("done\n"); >>>>>>> +#endif >>>>>>> done: >>>>>>> if (saved_buffer) >>>>>>> @@ -146,6 +159,7 @@ static int env_sf_save(void) >>>>>>> return ret; >>>>>>> } >>>>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) >>>>>>> static int env_sf_load(void) >>>>>>> { >>>>>>> int ret; >>>>>>> @@ -183,66 +197,6 @@ out: >>>>>>> return ret; >>>>>>> } >>>>>>> #else >>>>>>> -static int env_sf_save(void) >>>>>>> -{ >>>>>>> - u32 saved_size, saved_offset, sector; >>>>>>> - char *saved_buffer = NULL; >>>>>>> - int ret = 1; >>>>>>> - env_t env_new; >>>>>>> - >>>>>>> - ret = setup_flash_device(); >>>>>>> - if (ret) >>>>>>> - return ret; >>>>>>> - >>>>>>> - /* Is the sector larger than the env (i.e. embedded) */ >>>>>>> - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >>>>>>> - saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE; >>>>>>> - saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE; >>>>>>> - saved_buffer = malloc(saved_size); >>>>>>> - if (!saved_buffer) >>>>>>> - goto done; >>>>>>> - >>>>>>> - ret = spi_flash_read(env_flash, saved_offset, >>>>>>> - saved_size, saved_buffer); >>>>>>> - if (ret) >>>>>>> - goto done; >>>>>>> - } >>>>>>> - >>>>>>> - ret = env_export(&env_new); >>>>>>> - if (ret) >>>>>>> - goto done; >>>>>>> - >>>>>>> - sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE); >>>>>>> - >>>>>>> - puts("Erasing SPI flash..."); >>>>>>> - ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET, >>>>>>> - sector * CONFIG_ENV_SECT_SIZE); >>>>>>> - if (ret) >>>>>>> - goto done; >>>>>>> - >>>>>>> - puts("Writing to SPI flash..."); >>>>>>> - ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET, >>>>>>> - CONFIG_ENV_SIZE, &env_new); >>>>>>> - if (ret) >>>>>>> - goto done; >>>>>>> - >>>>>>> - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >>>>>>> - ret = spi_flash_write(env_flash, saved_offset, >>>>>>> - saved_size, saved_buffer); >>>>>>> - if (ret) >>>>>>> - goto done; >>>>>>> - } >>>>>>> - >>>>>>> - ret = 0; >>>>>>> - puts("done\n"); >>>>>>> - >>>>>>> - done: >>>>>>> - if (saved_buffer) >>>>>>> - free(saved_buffer); >>>>>>> - >>>>>>> - return ret; >>>>>>> -} >>>>>>> - >>>>>>> static int env_sf_load(void) >>>>>>> { >>>>>>> int ret; >>>>>>> @@ -258,8 +212,8 @@ static int env_sf_load(void) >>>>>>> if (ret) >>>>>>> goto out; >>>>>>> - ret = spi_flash_read(env_flash, >>>>>>> - CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf); >>>>>>> + ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, >>>>>>> CONFIG_ENV_SIZE, >>>>>>> + buf); >>>>>>> if (ret) { >>>>>>> env_set_default("spi_flash_read() failed", 0); >>>>>>> goto err_read; >>>>>>> @@ -292,7 +246,7 @@ static int env_sf_init(void) >>>>>>> env_t *env_ptr = (env_t *)env_sf_get_env_addr(); >>>>>>> if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) { >>>>>>> - gd->env_addr = (ulong)&(env_ptr->data); >>>>>>> + gd->env_addr = (ulong)&env_ptr->data; >>>>>>> gd->env_valid = 1; >>>>>>> } else { >>>>>>> gd->env_addr = (ulong)&default_environment[0]; >>>>>>> >>>>>> >>>>>> >>>>>> Viele Grüße, >>>>>> Stefan >>>>>> >>>>> >>>>> >>>> >>>> >>>> Viele Grüße, >>>> Stefan >>>> >>> >>> >> >> >> Viele Grüße, >> Stefan >> Viele Grüße, Stefan
Hi Harry, On 28.01.21 08:21, Harry Waschkeit wrote: > Instead of implementing redundant environments in two very similar > functions env_sf_save(), handle redundancy in one function, placing the > few differences in appropriate pre-compiler sections depending on config > option CONFIG_ENV_OFFSET_REDUND. > > Additionally, several checkpatch complaints were addressed. > > This patch is in preparation for adding support for env erase. > > Signed-off-by: Harry Waschkeit <Harry.Waschkeit@men.de> > --- > env/sf.c | 132 ++++++++++++++++++------------------------------------- > 1 file changed, 43 insertions(+), 89 deletions(-) Nice diffstat. ;) > > diff --git a/env/sf.c b/env/sf.c > index 937778aa37..c60bd1deed 100644 > --- a/env/sf.c > +++ b/env/sf.c > @@ -66,13 +66,16 @@ static int setup_flash_device(void) > return 0; > } > > -#if defined(CONFIG_ENV_OFFSET_REDUND) > static int env_sf_save(void) > { > env_t env_new; > - char *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE; > + char *saved_buffer = NULL; > u32 saved_size, saved_offset, sector; > + ulong offset; > int ret; > +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) > + char flag = ENV_REDUND_OBSOLETE; > +#endif I would prefer this to the #ifdef here: __maybe_unused char flag = ENV_REDUND_OBSOLETE; Looks good aside from this: Reviewed-by: Stefan Roese <sr@denx.de> Thanks, Stefan > > ret = setup_flash_device(); > if (ret) > @@ -81,27 +84,33 @@ static int env_sf_save(void) > ret = env_export(&env_new); > if (ret) > return -EIO; > - env_new.flags = ENV_REDUND_ACTIVE; > > - if (gd->env_valid == ENV_VALID) { > - env_new_offset = CONFIG_ENV_OFFSET_REDUND; > - env_offset = CONFIG_ENV_OFFSET; > - } else { > - env_new_offset = CONFIG_ENV_OFFSET; > - env_offset = CONFIG_ENV_OFFSET_REDUND; > - } > +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) > + env_new.flags = ENV_REDUND_ACTIVE; > + > + if (gd->env_valid == ENV_VALID) { > + env_new_offset = CONFIG_ENV_OFFSET_REDUND; > + env_offset = CONFIG_ENV_OFFSET; > + } else { > + env_new_offset = CONFIG_ENV_OFFSET; > + env_offset = CONFIG_ENV_OFFSET_REDUND; > + } > + offset = env_new_offset; > +#else > + offset = CONFIG_ENV_OFFSET; > +#endif > > /* Is the sector larger than the env (i.e. embedded) */ > if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { > saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE; > - saved_offset = env_new_offset + CONFIG_ENV_SIZE; > + saved_offset = offset + CONFIG_ENV_SIZE; > saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size); > if (!saved_buffer) { > ret = -ENOMEM; > goto done; > } > - ret = spi_flash_read(env_flash, saved_offset, > - saved_size, saved_buffer); > + ret = spi_flash_read(env_flash, saved_offset, saved_size, > + saved_buffer); > if (ret) > goto done; > } > @@ -109,35 +118,39 @@ static int env_sf_save(void) > sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE); > > puts("Erasing SPI flash..."); > - ret = spi_flash_erase(env_flash, env_new_offset, > - sector * CONFIG_ENV_SECT_SIZE); > + ret = spi_flash_erase(env_flash, offset, > + sector * CONFIG_ENV_SECT_SIZE); > if (ret) > goto done; > > puts("Writing to SPI flash..."); > > - ret = spi_flash_write(env_flash, env_new_offset, > - CONFIG_ENV_SIZE, &env_new); > + ret = spi_flash_write(env_flash, offset, > + CONFIG_ENV_SIZE, &env_new); > if (ret) > goto done; > > if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { > - ret = spi_flash_write(env_flash, saved_offset, > - saved_size, saved_buffer); > + ret = spi_flash_write(env_flash, saved_offset, saved_size, > + saved_buffer); > if (ret) > goto done; > } > > - ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags), > - sizeof(env_new.flags), &flag); > - if (ret) > - goto done; > +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) > + ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags), > + sizeof(env_new.flags), &flag); > + if (ret) > + goto done; > > - puts("done\n"); > + puts("done\n"); > > - gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND; > + gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND; > > - printf("Valid environment: %d\n", (int)gd->env_valid); > + printf("Valid environment: %d\n", (int)gd->env_valid); > +#else > + puts("done\n"); > +#endif > > done: > if (saved_buffer) > @@ -146,6 +159,7 @@ static int env_sf_save(void) > return ret; > } > > +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) > static int env_sf_load(void) > { > int ret; > @@ -183,66 +197,6 @@ out: > return ret; > } > #else > -static int env_sf_save(void) > -{ > - u32 saved_size, saved_offset, sector; > - char *saved_buffer = NULL; > - int ret = 1; > - env_t env_new; > - > - ret = setup_flash_device(); > - if (ret) > - return ret; > - > - /* Is the sector larger than the env (i.e. embedded) */ > - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { > - saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE; > - saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE; > - saved_buffer = malloc(saved_size); > - if (!saved_buffer) > - goto done; > - > - ret = spi_flash_read(env_flash, saved_offset, > - saved_size, saved_buffer); > - if (ret) > - goto done; > - } > - > - ret = env_export(&env_new); > - if (ret) > - goto done; > - > - sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE); > - > - puts("Erasing SPI flash..."); > - ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET, > - sector * CONFIG_ENV_SECT_SIZE); > - if (ret) > - goto done; > - > - puts("Writing to SPI flash..."); > - ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET, > - CONFIG_ENV_SIZE, &env_new); > - if (ret) > - goto done; > - > - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { > - ret = spi_flash_write(env_flash, saved_offset, > - saved_size, saved_buffer); > - if (ret) > - goto done; > - } > - > - ret = 0; > - puts("done\n"); > - > - done: > - if (saved_buffer) > - free(saved_buffer); > - > - return ret; > -} > - > static int env_sf_load(void) > { > int ret; > @@ -258,8 +212,8 @@ static int env_sf_load(void) > if (ret) > goto out; > > - ret = spi_flash_read(env_flash, > - CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf); > + ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, > + buf); > if (ret) { > env_set_default("spi_flash_read() failed", 0); > goto err_read; > @@ -292,7 +246,7 @@ static int env_sf_init(void) > env_t *env_ptr = (env_t *)env_sf_get_env_addr(); > > if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) { > - gd->env_addr = (ulong)&(env_ptr->data); > + gd->env_addr = (ulong)&env_ptr->data; > gd->env_valid = 1; > } else { > gd->env_addr = (ulong)&default_environment[0]; > Viele Grüße, Stefan
On 30.01.21 10:07, Stefan Roese wrote: > Hi Harry, Hi Stefan, > On 28.01.21 08:21, Harry Waschkeit wrote: >> Instead of implementing redundant environments in two very similar >> functions env_sf_save(), handle redundancy in one function, placing the >> few differences in appropriate pre-compiler sections depending on config >> option CONFIG_ENV_OFFSET_REDUND. >> >> Additionally, several checkpatch complaints were addressed. >> >> This patch is in preparation for adding support for env erase. >> >> Signed-off-by: Harry Waschkeit <Harry.Waschkeit@men.de> >> --- >> env/sf.c | 132 ++++++++++++++++++------------------------------------- >> 1 file changed, 43 insertions(+), 89 deletions(-) > > Nice diffstat. ;) > >> >> diff --git a/env/sf.c b/env/sf.c >> index 937778aa37..c60bd1deed 100644 >> --- a/env/sf.c >> +++ b/env/sf.c >> @@ -66,13 +66,16 @@ static int setup_flash_device(void) >> return 0; >> } >> -#if defined(CONFIG_ENV_OFFSET_REDUND) >> static int env_sf_save(void) >> { >> env_t env_new; >> - char *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE; >> + char *saved_buffer = NULL; >> u32 saved_size, saved_offset, sector; >> + ulong offset; >> int ret; >> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) >> + char flag = ENV_REDUND_OBSOLETE; >> +#endif > > I would prefer this to the #ifdef here: > > __maybe_unused char flag = ENV_REDUND_OBSOLETE; Ok, will change that and send a v2. > Looks good aside from this: > > Reviewed-by: Stefan Roese <sr@denx.de> Thanks for your review! :-) Cheers, Harry > Thanks, > Stefan > >> ret = setup_flash_device(); >> if (ret) >> @@ -81,27 +84,33 @@ static int env_sf_save(void) >> ret = env_export(&env_new); >> if (ret) >> return -EIO; >> - env_new.flags = ENV_REDUND_ACTIVE; >> - if (gd->env_valid == ENV_VALID) { >> - env_new_offset = CONFIG_ENV_OFFSET_REDUND; >> - env_offset = CONFIG_ENV_OFFSET; >> - } else { >> - env_new_offset = CONFIG_ENV_OFFSET; >> - env_offset = CONFIG_ENV_OFFSET_REDUND; >> - } >> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) >> + env_new.flags = ENV_REDUND_ACTIVE; >> + >> + if (gd->env_valid == ENV_VALID) { >> + env_new_offset = CONFIG_ENV_OFFSET_REDUND; >> + env_offset = CONFIG_ENV_OFFSET; >> + } else { >> + env_new_offset = CONFIG_ENV_OFFSET; >> + env_offset = CONFIG_ENV_OFFSET_REDUND; >> + } >> + offset = env_new_offset; >> +#else >> + offset = CONFIG_ENV_OFFSET; >> +#endif >> /* Is the sector larger than the env (i.e. embedded) */ >> if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >> saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE; >> - saved_offset = env_new_offset + CONFIG_ENV_SIZE; >> + saved_offset = offset + CONFIG_ENV_SIZE; >> saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size); >> if (!saved_buffer) { >> ret = -ENOMEM; >> goto done; >> } >> - ret = spi_flash_read(env_flash, saved_offset, >> - saved_size, saved_buffer); >> + ret = spi_flash_read(env_flash, saved_offset, saved_size, >> + saved_buffer); >> if (ret) >> goto done; >> } >> @@ -109,35 +118,39 @@ static int env_sf_save(void) >> sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE); >> puts("Erasing SPI flash..."); >> - ret = spi_flash_erase(env_flash, env_new_offset, >> - sector * CONFIG_ENV_SECT_SIZE); >> + ret = spi_flash_erase(env_flash, offset, >> + sector * CONFIG_ENV_SECT_SIZE); >> if (ret) >> goto done; >> puts("Writing to SPI flash..."); >> - ret = spi_flash_write(env_flash, env_new_offset, >> - CONFIG_ENV_SIZE, &env_new); >> + ret = spi_flash_write(env_flash, offset, >> + CONFIG_ENV_SIZE, &env_new); >> if (ret) >> goto done; >> if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >> - ret = spi_flash_write(env_flash, saved_offset, >> - saved_size, saved_buffer); >> + ret = spi_flash_write(env_flash, saved_offset, saved_size, >> + saved_buffer); >> if (ret) >> goto done; >> } >> - ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags), >> - sizeof(env_new.flags), &flag); >> - if (ret) >> - goto done; >> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) >> + ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags), >> + sizeof(env_new.flags), &flag); >> + if (ret) >> + goto done; >> - puts("done\n"); >> + puts("done\n"); >> - gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND; >> + gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND; >> - printf("Valid environment: %d\n", (int)gd->env_valid); >> + printf("Valid environment: %d\n", (int)gd->env_valid); >> +#else >> + puts("done\n"); >> +#endif >> done: >> if (saved_buffer) >> @@ -146,6 +159,7 @@ static int env_sf_save(void) >> return ret; >> } >> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) >> static int env_sf_load(void) >> { >> int ret; >> @@ -183,66 +197,6 @@ out: >> return ret; >> } >> #else >> -static int env_sf_save(void) >> -{ >> - u32 saved_size, saved_offset, sector; >> - char *saved_buffer = NULL; >> - int ret = 1; >> - env_t env_new; >> - >> - ret = setup_flash_device(); >> - if (ret) >> - return ret; >> - >> - /* Is the sector larger than the env (i.e. embedded) */ >> - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >> - saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE; >> - saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE; >> - saved_buffer = malloc(saved_size); >> - if (!saved_buffer) >> - goto done; >> - >> - ret = spi_flash_read(env_flash, saved_offset, >> - saved_size, saved_buffer); >> - if (ret) >> - goto done; >> - } >> - >> - ret = env_export(&env_new); >> - if (ret) >> - goto done; >> - >> - sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE); >> - >> - puts("Erasing SPI flash..."); >> - ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET, >> - sector * CONFIG_ENV_SECT_SIZE); >> - if (ret) >> - goto done; >> - >> - puts("Writing to SPI flash..."); >> - ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET, >> - CONFIG_ENV_SIZE, &env_new); >> - if (ret) >> - goto done; >> - >> - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { >> - ret = spi_flash_write(env_flash, saved_offset, >> - saved_size, saved_buffer); >> - if (ret) >> - goto done; >> - } >> - >> - ret = 0; >> - puts("done\n"); >> - >> - done: >> - if (saved_buffer) >> - free(saved_buffer); >> - >> - return ret; >> -} >> - >> static int env_sf_load(void) >> { >> int ret; >> @@ -258,8 +212,8 @@ static int env_sf_load(void) >> if (ret) >> goto out; >> - ret = spi_flash_read(env_flash, >> - CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf); >> + ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, >> + buf); >> if (ret) { >> env_set_default("spi_flash_read() failed", 0); >> goto err_read; >> @@ -292,7 +246,7 @@ static int env_sf_init(void) >> env_t *env_ptr = (env_t *)env_sf_get_env_addr(); >> if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) { >> - gd->env_addr = (ulong)&(env_ptr->data); >> + gd->env_addr = (ulong)&env_ptr->data; >> gd->env_valid = 1; >> } else { >> gd->env_addr = (ulong)&default_environment[0]; >> > > > Viele Grüße, > Stefan >
diff --git a/env/sf.c b/env/sf.c index 937778aa37..c60bd1deed 100644 --- a/env/sf.c +++ b/env/sf.c @@ -66,13 +66,16 @@ static int setup_flash_device(void) return 0; } -#if defined(CONFIG_ENV_OFFSET_REDUND) static int env_sf_save(void) { env_t env_new; - char *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE; + char *saved_buffer = NULL; u32 saved_size, saved_offset, sector; + ulong offset; int ret; +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) + char flag = ENV_REDUND_OBSOLETE; +#endif ret = setup_flash_device(); if (ret) @@ -81,27 +84,33 @@ static int env_sf_save(void) ret = env_export(&env_new); if (ret) return -EIO; - env_new.flags = ENV_REDUND_ACTIVE; - if (gd->env_valid == ENV_VALID) { - env_new_offset = CONFIG_ENV_OFFSET_REDUND; - env_offset = CONFIG_ENV_OFFSET; - } else { - env_new_offset = CONFIG_ENV_OFFSET; - env_offset = CONFIG_ENV_OFFSET_REDUND; - } +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) + env_new.flags = ENV_REDUND_ACTIVE; + + if (gd->env_valid == ENV_VALID) { + env_new_offset = CONFIG_ENV_OFFSET_REDUND; + env_offset = CONFIG_ENV_OFFSET; + } else { + env_new_offset = CONFIG_ENV_OFFSET; + env_offset = CONFIG_ENV_OFFSET_REDUND; + } + offset = env_new_offset; +#else + offset = CONFIG_ENV_OFFSET; +#endif /* Is the sector larger than the env (i.e. embedded) */ if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE; - saved_offset = env_new_offset + CONFIG_ENV_SIZE; + saved_offset = offset + CONFIG_ENV_SIZE; saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size); if (!saved_buffer) { ret = -ENOMEM; goto done; } - ret = spi_flash_read(env_flash, saved_offset, - saved_size, saved_buffer); + ret = spi_flash_read(env_flash, saved_offset, saved_size, + saved_buffer); if (ret) goto done; } @@ -109,35 +118,39 @@ static int env_sf_save(void) sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE); puts("Erasing SPI flash..."); - ret = spi_flash_erase(env_flash, env_new_offset, - sector * CONFIG_ENV_SECT_SIZE); + ret = spi_flash_erase(env_flash, offset, + sector * CONFIG_ENV_SECT_SIZE); if (ret) goto done; puts("Writing to SPI flash..."); - ret = spi_flash_write(env_flash, env_new_offset, - CONFIG_ENV_SIZE, &env_new); + ret = spi_flash_write(env_flash, offset, + CONFIG_ENV_SIZE, &env_new); if (ret) goto done; if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { - ret = spi_flash_write(env_flash, saved_offset, - saved_size, saved_buffer); + ret = spi_flash_write(env_flash, saved_offset, saved_size, + saved_buffer); if (ret) goto done; } - ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags), - sizeof(env_new.flags), &flag); - if (ret) - goto done; +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) + ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags), + sizeof(env_new.flags), &flag); + if (ret) + goto done; - puts("done\n"); + puts("done\n"); - gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND; + gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND; - printf("Valid environment: %d\n", (int)gd->env_valid); + printf("Valid environment: %d\n", (int)gd->env_valid); +#else + puts("done\n"); +#endif done: if (saved_buffer) @@ -146,6 +159,7 @@ static int env_sf_save(void) return ret; } +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT) static int env_sf_load(void) { int ret; @@ -183,66 +197,6 @@ out: return ret; } #else -static int env_sf_save(void) -{ - u32 saved_size, saved_offset, sector; - char *saved_buffer = NULL; - int ret = 1; - env_t env_new; - - ret = setup_flash_device(); - if (ret) - return ret; - - /* Is the sector larger than the env (i.e. embedded) */ - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { - saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE; - saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE; - saved_buffer = malloc(saved_size); - if (!saved_buffer) - goto done; - - ret = spi_flash_read(env_flash, saved_offset, - saved_size, saved_buffer); - if (ret) - goto done; - } - - ret = env_export(&env_new); - if (ret) - goto done; - - sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE); - - puts("Erasing SPI flash..."); - ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET, - sector * CONFIG_ENV_SECT_SIZE); - if (ret) - goto done; - - puts("Writing to SPI flash..."); - ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET, - CONFIG_ENV_SIZE, &env_new); - if (ret) - goto done; - - if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) { - ret = spi_flash_write(env_flash, saved_offset, - saved_size, saved_buffer); - if (ret) - goto done; - } - - ret = 0; - puts("done\n"); - - done: - if (saved_buffer) - free(saved_buffer); - - return ret; -} - static int env_sf_load(void) { int ret; @@ -258,8 +212,8 @@ static int env_sf_load(void) if (ret) goto out; - ret = spi_flash_read(env_flash, - CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf); + ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, + buf); if (ret) { env_set_default("spi_flash_read() failed", 0); goto err_read; @@ -292,7 +246,7 @@ static int env_sf_init(void) env_t *env_ptr = (env_t *)env_sf_get_env_addr(); if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) { - gd->env_addr = (ulong)&(env_ptr->data); + gd->env_addr = (ulong)&env_ptr->data; gd->env_valid = 1; } else { gd->env_addr = (ulong)&default_environment[0];
Instead of implementing redundant environments in two very similar functions env_sf_save(), handle redundancy in one function, placing the few differences in appropriate pre-compiler sections depending on config option CONFIG_ENV_OFFSET_REDUND. Additionally, several checkpatch complaints were addressed. This patch is in preparation for adding support for env erase. Signed-off-by: Harry Waschkeit <Harry.Waschkeit@men.de> --- env/sf.c | 132 ++++++++++++++++++------------------------------------- 1 file changed, 43 insertions(+), 89 deletions(-)