[PATCH v3 1/1] env: sf: single function env_sf_save()
Sean Anderson
seanga2 at gmail.com
Wed Mar 3 00:06:29 CET 2021
On 3/2/21 1:09 PM, Harry Waschkeit wrote:
> 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.
Writing to SPI flash can only set bits to 0. To set bits to 1 you must
erase them. Conceptually, write(buf) does
flash[i] &= buf[i];
And erase() does
flash[i] = -1;
So writing 0s always succeeds, and we are certain to invalidate the
environment (as long as our hash has a non-zero value for an all-zero
input).
> 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.
env_sf_save always erases the environment before flashing. So your patch
effectively erases env twice before writing to it.
> 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.
Right, but typically we have a write granularity of one byte for SPI
flashes. So you can clear only the environment, and let enf_sf_save deal
with other data in the sector.
--Sean
> 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];
>>>>
>>>
>>>
>>
>>
>
>
More information about the U-Boot
mailing list