[PATCH v3 1/1] env: sf: single function env_sf_save()
Stefan Roese
sr at denx.de
Wed Feb 3 07:10:05 CET 2021
On 02.02.21 18:09, Harry Waschkeit wrote:
> 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 ...
No, you don't have to wait. This would be counter-productive. Please
just add the dependancy to this patch in the commit text of the new
one to make this clear. Best would be below the "---" line so that
it will not be included in the git history.
> 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 :-)
Yep, that would have been even better. ;)
Thanks,
Stefan
>> 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
>>
>
>
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