[PATCH] env: sf: add support for env erase

[Harry Waschkeit <harry.waschkeit@men.de>]

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 ...

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.

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 sign-off problem I guess is probably caused by the check not accepting name
in reverse order, isn't it?
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 ;-/
I will take care of that before resending my patch (v2 then, right?).

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@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@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;
>> +               }
>> +       }
>> +
>> +       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;
>> +       }
>> +
>> +       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)
>> +       .erase          = env_sf_erase,
>> +#endif
>>   };
> 


-- 
Harry Waschkeit - Software Engineer