diff mbox series

env: sf: add support for env erase

Message ID 66c89baf-b52a-81cb-2846-112a7925a256@men.de
State Needs Review / ACK
Delegated to: Tom Rini
Headers show
Series env: sf: add support for env erase | expand

Commit Message

Harry Waschkeit Oct. 8, 2020, 5:27 p.m. UTC
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>
---
  env/sf.c | 130 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 128 insertions(+), 2 deletions(-)

Comments

Sean Anderson Oct. 9, 2020, 1:55 p.m. UTC | #1
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 Oct. 9, 2020, 4:43 p.m. UTC | #2
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
>>   };
>
Sean Anderson Oct. 9, 2020, 5 p.m. UTC | #3
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@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@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;
>>> +               }
>>> +       }

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
Harry Waschkeit Oct. 14, 2020, 4:10 p.m. UTC | #4

On 10/9/20 7:00 PM, Sean Anderson wrote:
> 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 :)

Yeah, guess so ... ;-/

> I suggest configuring git send-email. If you are going to be making more
> patch series, also check out tools/patman.

I'll definitely have a look at that, sooner or later.

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

Ok, got it.

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

Hmm, that's strange, I tried that one but the complaints remained.
Chances are I did something wrong so I will have a look at kconfig.h to get also
around that.

>>
>> 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@address>".

This will be the easiest part then :-)

>> 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@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;
>>>> +               }
>>>> +       }
> 
> 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?

Funnily enough, I did think about that for a short moment but cowardly
didn't dare restructuring such a central file with my first U-Boot patch
ever ...

But yeah, I fully agree, code replication of non-trivial things deserve
appropriate refactoring, I'll give that a try in my next patch.

>>>> +
>>>> +       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?

Sure, when I am at it anyway for the other path :-)

Actually, the two paths (w/ and w/o redundancy) look to me as if
they probably could be cleaned up even more so that no separate
implementations for these functions would be necessary, but I am
not sure how welcome that would be ...

I don't want to give the impression being a know-it-all, you know,
just want to help a little bit improving things ;-)

>>>> +
>>>> +       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?

Good catch, will have another thorough look at that.

>>>> +       .erase          = env_sf_erase,
>>>> +#endif
>>>>    };
>>>
>>
>>
> 
> --Sean
>
diff mbox series

Patch

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
  };