[PATCH] env: sf: single function env_sf_save()
Stefan Roese
sr at denx.de
Thu Jan 28 09:50:51 CET 2021
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 at 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
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de
More information about the U-Boot
mailing list