[PATCH] env: sf: add support for env erase
Sean Anderson
seanga2 at gmail.com
Fri Oct 9 19:00:20 CEST 2020
On 10/9/20 12:43 PM, Harry Waschkeit wrote:
> Hi Sean,
>
> thanks for your try and sorry for the inconvenience my beginner's mistakes have
> caused :-(
>
> It is definitely no good idea to use copy&paste with patch data, I should have
> guessed that beforehand ...
You *can* do it, you just have to configure your mail client correctly.
However, it gets pretty tedious when you have a lot of patches :)
I suggest configuring git send-email. If you are going to be making more
patch series, also check out tools/patman.
>
> Anyway, concerning the complaints from checkpatch.pl: I indeed ran checkpatch.pl
> on my patch prior to sending it - the real one, not the one as pasted into the
> mail ;-/
>
> The alignment warnings simply result from the fact that I adhered to the style
> used in that file already, you can easily verify that by running checkpatch.pl
> on the complete file.
Please keep new code in the correct style. For example, you have
> + ret = spi_flash_read(env_flash, saved_offset,
> + saved_size, saved_buffer);
which is aligned properly, but later on you have
>>> + ret = spi_flash_read(env_flash, saved_offset,
>>> + saved_size, saved_buffer);
which is not.
>
> For the warnings with "IS_ENABLED(CONFIG...)" I don't see what I could do to
> get around them: all three occurrences are about compiling functions into the
> code depending on CONFIG_CMD_ERASEENV.
> Two times it is the new function env_sf_erase(), one variant for normal and the
> other for redundand environment handling.
> The third time it is used to define this new method into the structure
> U_BOOT_ENV_LOCATION(sf).
The macro IS_ENABLED can be used both in C code and in preprocessor
directives. See include/linux/kconfig.h for details.
>
> The sign-off problem I guess is probably caused by the check not accepting name
> in reverse order, isn't it?
Yes. The format is "First Last <email at address>".
> Anyway, I will change my user.name to match the order in the mail address so
> next patch is hopefully correct.
>
> So please let me know what else I should do beside sending a properly formatted
> patch ;-/
See below.
> I will take care of that before resending my patch (v2 then, right?).
Yes.
>
> On 10/9/20 3:55 PM, Sean Anderson wrote:
>> On 10/8/20 1:27 PM, Harry Waschkeit wrote:
>>> Command "env erase" didn't work even though CONFIG_CMD_ERASEENV was
>>> defined, because serial flash environment routines didn't implement
>>> erase method.
>>>
>>> Signed-off-by: Waschkeit, Harry <Harry.Waschkeit at men.de>
>>> ---
>>
>> Hi Harry,
>>
>> I wanted to test out your patch, but I couldn't get it to apply. It
>> appears that your mail program has replaced the tabs with spaces, so git
>> can't figure out how to apply it. I tried to fix it by performing the
>> substitutions
>>
>> s/^\(.\) /\1\t/g
>> s/ /\t/g
>>
>> but it still wouldn't apply. In addition, checkpatch has a few warnings:
>>
>>> $ scripts/checkpatch.pl env-sf-add-support-for-env-erase.patch
>>> WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible
>>> #152: FILE: env/sf.c:149:
>>> +#ifdef CONFIG_CMD_ERASEENV
>>>
>>> WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible
>>> #240: FILE: env/sf.c:318:
>>> +#ifdef CONFIG_CMD_ERASEENV
>>>
>>> CHECK: Alignment should match open parenthesis
>>> #260: FILE: env/sf.c:338:
>>> + ret = spi_flash_read(env_flash, saved_offset,
>>> + saved_size, saved_buffer);
>>>
>>> CHECK: Alignment should match open parenthesis
>>> #269: FILE: env/sf.c:347:
>>> + ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,
>>> + sector * CONFIG_ENV_SECT_SIZE);
>>>
>>> CHECK: Alignment should match open parenthesis
>>> #276: FILE: env/sf.c:354:
>>> + ret = spi_flash_write(env_flash, saved_offset,
>>> + saved_size, saved_buffer);
>>>
>>> WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible
>>> #307: FILE: env/sf.c:437:
>>> +#if defined(CONFIG_CMD_ERASEENV) && defined(CONFIG_ENV_ADDR)
>>>
>>> WARNING: Missing Signed-off-by: line by nominal patch author 'Harry Waschkeit <harry.waschkeit at men.de>'
>>>
>>> total: 0 errors, 4 warnings, 3 checks, 158 lines checked
>>>
>>> NOTE: For some of the reported defects, checkpatch may be able to
>>> mechanically convert to the typical style using --fix or --fix-inplace.
>>>
>>> env-sf-add-support-for-env-erase.patch has style problems, please review.
>>>
>>> NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX MULTISTATEMENT_MACRO_USE_DO_WHILE NETWORKING_BLOCK_COMMENT_STYLE PREFER_ETHER_ADDR_COPY USLEEP_RANGE
>>>
>>> NOTE: If any of the errors are false positives, please report
>>> them to the maintainer, see CHECKPATCH in MAINTAINERS.
>>
>> Please fix these issues and resend, thanks!
>>
>> --Sean
>>
>>> env/sf.c | 130 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>> 1 file changed, 128 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/env/sf.c b/env/sf.c
>>> index 937778aa37..9cda192a73 100644
>>> --- a/env/sf.c
>>> +++ b/env/sf.c
>>> @@ -146,6 +146,78 @@ static int env_sf_save(void)
>>> return ret;
>>> }
>>> +#ifdef CONFIG_CMD_ERASEENV
>>> +static int env_sf_erase(void)
>>> +{
>>> + char *saved_buffer = NULL;
>>> + u32 saved_size, saved_offset, sector;
>>> + ulong offset;
>>> + ulong offsets[2] = { CONFIG_ENV_OFFSET, CONFIG_ENV_OFFSET_REDUND };
>>> + int i;
>>> + int ret;
>>> +
>>> + ret = setup_flash_device();
>>> + if (ret)
>>> + return ret;
>>> +
>>> + /* get temporary storage if sector is larger than env (i.e. embedded) */
>>> + if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>> + saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>> + saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
>>> + if (!saved_buffer) {
>>> + ret = -ENOMEM;
>>> + goto done;
>>> + }
>>> + }
Can this logic be split out into a separate function, since it is shared
with env_sf_save? Perhaps make a function like env_sf_do_erase() and
call it from env_sf_save and env_sf_erase?
>>> +
>>> + sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>> +
>>> + /* simply erase both environments, retaining non-env data (if any) */
>>> + for (i = 0; i < ARRAY_SIZE(offsets); i++) {
>>> + offset = offsets[i];
>>> +
>>> + if (saved_buffer) {
>>> + saved_offset = offset + CONFIG_ENV_SIZE;
>>> + ret = spi_flash_read(env_flash, saved_offset,
>>> + saved_size, saved_buffer);
>>> + if (ret)
>>> + goto done;
>>> + }
>>> +
>>> + if (i)
>>> + puts("Redund:");
>>> +
>>> + puts("Erasing SPI flash...");
>>> + ret = spi_flash_erase(env_flash, offset,
>>> + sector * CONFIG_ENV_SECT_SIZE);
>>> + if (ret)
>>> + goto done;
>>> +
>>> + if (saved_buffer) {
>>> + puts("Writing non-environment data to SPI flash...");
>>> + ret = spi_flash_write(env_flash, saved_offset,
>>> + saved_size, saved_buffer);
>>> + if (ret)
>>> + goto done;
>>> + }
>>> +
>>> + puts("done\n");
>>> + }
>>> +
>>> + /* here we know that both env sections are cleared */
>>> + env_new_offset = CONFIG_ENV_OFFSET;
>>> + env_offset = CONFIG_ENV_OFFSET_REDUND;
>>> +
>>> + gd->env_valid = ENV_INVALID;
>>> +
>>> + done:
>>> + if (saved_buffer)
>>> + free(saved_buffer);
>>> +
>>> + return ret;
>>> +}
>>> +#endif /* CONFIG_CMD_ERASEENV */
>>> +
>>> static int env_sf_load(void)
>>> {
>>> int ret;
>>> @@ -182,7 +254,7 @@ out:
>>> return ret;
>>> }
>>> -#else
>>> +#else /* #if defined(CONFIG_ENV_OFFSET_REDUND) */
>>> static int env_sf_save(void)
>>> {
>>> u32 saved_size, saved_offset, sector;
>>> @@ -243,6 +315,57 @@ static int env_sf_save(void)
>>> return ret;
>>> }
>>> +#ifdef CONFIG_CMD_ERASEENV
>>> +static int env_sf_erase(void)
>>> +{
>>> + u32 saved_size, saved_offset, sector;
>>> + char *saved_buffer = NULL;
>>> + int ret = 1;
>>> +
>>> + 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;
>>> + }
Same thing here; can this be a separate function?
>>> +
>>> + 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;
>>> +
>>> + if (saved_buffer) {
>>> + puts("Writing non-environment data to SPI flash...");
>>> + ret = spi_flash_write(env_flash, saved_offset,
>>> + saved_size, saved_buffer);
>>> + if (ret)
>>> + goto done;
>>> + }
>>> +
>>> + puts("done\n");
>>> +
>>> + done:
>>> + if (saved_buffer)
>>> + free(saved_buffer);
>>> +
>>> + return ret;
>>> +}
>>> +#endif /* CONFIG_CMD_ERASEENV */
>>> +
>>> static int env_sf_load(void)
>>> {
>>> int ret;
>>> @@ -277,7 +400,7 @@ out:
>>> return ret;
>>> }
>>> -#endif
>>> +#endif /* #if defined(CONFIG_ENV_OFFSET_REDUND) #else */
>>> #if CONFIG_ENV_ADDR != 0x0
>>> __weak void *env_sf_get_env_addr(void)
>>> @@ -311,4 +434,7 @@ U_BOOT_ENV_LOCATION(sf) = {
>>> #if defined(INITENV) && (CONFIG_ENV_ADDR != 0x0)
>>> .init = env_sf_init,
>>> #endif
>>> +#if defined(CONFIG_CMD_ERASEENV) && defined(CONFIG_ENV_ADDR)
Why does this depend on ENV_ADDR, when above we only depend on
CMD_ERASEENV?
>>> + .erase = env_sf_erase,
>>> +#endif
>>> };
>>
>
>
--Sean
More information about the U-Boot
mailing list