[PATCH] env: sf: single function env_sf_save()
Harry Waschkeit
harry.waschkeit at men.de
Mon Feb 1 11:46:16 CET 2021
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 at 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 at 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
>
--
Harry Waschkeit - Software Engineer
duagon Germany GmbH
Neuwieder Straße 1-7 90411 Nuernberg
Geschaeftsfuehrer: Dr. Michael Goldbach - Matthias Kamolz - Kalina Scott - Handelsregister AG Nuernberg HRB 5540
More information about the U-Boot
mailing list