[PATCH v3 1/1] env: sf: single function env_sf_save()
Harry Waschkeit
harry.waschkeit at men.de
Tue Feb 2 18:09:16 CET 2021
On 02.02.21 15:54, Stefan Roese wrote:
> On 02.02.21 15:43, Harry Waschkeit wrote:
>>
>> On 02.02.21 10:30, Stefan Roese wrote:
>>> 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
>>>
>>> JFYI:
>>> No need to re-send this patch with this added RB tag, as I already did
>>> send a new RB to the last mail as reply. Patchwork collects these tags
>>> when sent to the list. So you only need to include them, if you send
>>> a new patch version.
>>
>> thanks for this hint, obviously I step into it all the time ...
>>
>> Ok, lesson learnt, let's see what I can do wrong next time ... ;-/
>>
>> Back on topic: I guess that was all I needed to do so that the patch will get merged when its time comes.
>
> Yes, now we (you) need a bit of patience, so that other people might
> review this patch as well. And usually it will get handled after some
> time (depending on the development stage of U-Boot, merge window open
> or shortly before release etc).
Ok, no problem with that.
That also means that I should wait with submission of the other patch (adding "env erase" support) until this one is merged.
Otherwise the patch would need to be significantly different ...
Hmm, I guess it would have been better to not send that patch standalone but instead as a first one in a two-patch series where the second one adds the new functionality on top of the clean-up.
Anyway, something to keep in mind for next time :-)
> It does not hurt of course to check this from time to time and
> "trigger" the maintainer of the subsystem or the custodian this
> patch is delegated to nothing is happening here for a too long
> time (like more than 1 month).
Alright, good to know :-)
Thanks,
Harry
> Thanks,
> Stefan
>
>> If not, please let me know.
>>
>> Thanks,
>> Harry
>>
>>> Thanks,
>>> Stefan
>>>
>>>> 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];
>>>>
>>>
>>>
>>> Viele Grüße,
>>> Stefan
>>>
>>
>>
>
>
> Viele Grüße,
> Stefan
>
--
Harry Waschkeit - Software Engineer
More information about the U-Boot
mailing list