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