[PATCH] env: sf: single function env_sf_save()
Stefan Roese
sr at denx.de
Fri Jan 29 08:16:23 CET 2021
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
--
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