[PATCH v3 1/1] env: sf: single function env_sf_save()
Harry Waschkeit
harry.waschkeit at men.de
Tue Mar 2 19:09:42 CET 2021
Hello Sean,
On 02.03.21 00:10, Sean Anderson wrote:
> On 3/1/21 11:39 AM, Harry Waschkeit wrote:
>> Hi again,
>>
>> gentle ping for that patch, also in view of subsequently sent patch ...
>>
>> https://lists.denx.de/pipermail/u-boot/2021-February/439797.html
>>
>> ... which saw a competing patch by Patrick Delaunay a week later:
>>
>> https://lists.denx.de/pipermail/u-boot/2021-February/440769.html
>>
>> However, the latter doesn't seem to take embedded environments into account.
>
> Can you give an example of where your patch would work while Patrick's wouldn't?
I didn't dig too deep into Patrick's patch and maybe I simply miss something important, but I don't even see an spi_flash_erase() in his code, so I'm wondering how it could work at all.
What I do see in his code is spi_flash_write() with a CONFIG_ENV_SIZE worth of zeroes to the environment environment offset, but this can only work for an erased flash as far as I know.
And erasing the flash part where the environment is stored should take an environment sizes below sector size into account; the rest of the environment sector could be used otherwise, i.e. CONFIG_ENV_SIZE < CONFIG_ENV_SECT_SIZE.
Function env_sf_save() takes care of all of that, so does the env_sf_erase() in my patch, Patrick's version of env_sf_erase() does not.
(side note: using more than a flash sector for environment and sharing the last one with different data isn't handled at all at the moment, probably because it was never needed)
Best regards,
Harry
>
> Thanks.
>
> --Sean
>
>>
>> Best regards,
>> Harry
>>
>>
>> On 02.02.21 09: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>
>>> Reviewed-by: Stefan Roese <sr at denx.de>
>>> ---
>>> Change in v3:
>>> - no change in patch, only added "reviewed-by" to commit log
>>>
>>> Change in v2:
>>> - remove one more #ifdef, instead take advantage of compiler attribute
>>> __maybe_unused for one variable used only in case of redundant
>>> environments
>>>
>>> env/sf.c | 130 ++++++++++++++++++-------------------------------------
>>> 1 file changed, 41 insertions(+), 89 deletions(-)
>>>
>>> diff --git a/env/sf.c b/env/sf.c
>>> index 937778aa37..199114fd3b 100644
>>> --- a/env/sf.c
>>> +++ b/env/sf.c
>>> @@ -66,13 +66,14 @@ 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;
>>> + __maybe_unused char flag = ENV_REDUND_OBSOLETE;
>>> ret = setup_flash_device();
>>> if (ret)
>>> @@ -81,27 +82,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 +116,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 +157,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 +195,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 +210,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 +244,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];
>>>
>>
>>
>
>
--
Harry Waschkeit - Software Engineer
More information about the U-Boot
mailing list