[U-Boot] [PATCH] fix flash_sect_erase() to display correct message

["Seunghyeon Rhee (이승현)"]

Dear Wolfgang Denk,

Wolfgang Denk wrote:
> Dear =?UTF-8?B?7J207Iq57ZiE?=,
>
> In message <fa2126d60911130006q3d5a1879pb177a51a4544fb6b at mail.gmail.com> you wrote:
>   
>> flash_sect_erase() displays message "Erased #N sectors" even when
>> there are some protected sectors found and command "erase" fail.
>>
>> Signed-off-by: Seunghyeon Rhee <seunghyeon at lpmtec.com>
>> ---
>>  common/cmd_flash.c |    5 ++++-
>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/common/cmd_flash.c b/common/cmd_flash.c
>> index 3773412..b3d982f 100644
>> --- a/common/cmd_flash.c
>> +++ b/common/cmd_flash.c
>> @@ -451,7 +451,10 @@ int flash_sect_erase (ulong addr_first, ulong addr_last)
>>  				rcode = flash_erase (info, s_first[bank], s_last[bank]);
>>  			}
>>  		}
>> -		printf ("Erased %d sectors\n", erased);
>> +		if (rcode == ERR_PROTECTED)
>> +			printf ("Not erased - protected sector(s) found\n");
>> +		else
>> +			printf ("Erased %d sectors\n", erased);
>>  	} else if (rcode == 0) {
>>  		puts ("Error: start and/or end address"
>>  			" not on sector boundary\n");
>>     
>
> I think this patch is not an improvement. Now it prints "Not erased"
> even when sectors _have_ successfully been earased, which is at least
> as wrong als the old behaviour.
>
>
> Just to see what we are talking about:
>
> Preparation:
> ============
>
> => fli 2
>
> Bank # 2: CFI conformant FLASH (32 x 16)  Size: 4 MB in 35 Sectors
>   AMD Standard command set, Manufacturer ID: 0x04, Device ID: 0x2249
>   Erase timeout: 16384 ms, write timeout: 1 ms
>
>   Sector Start Addresses:
>   40400000        40408000        4040C000        40410000        40420000      
>   40440000        40460000        40480000        404A0000        404C0000      
>   404E0000        40500000        40520000        40540000        40560000      
>   40580000        405A0000        405C0000        405E0000        40600000      
>   40620000        40640000        40660000        40680000        406A0000      
>   406C0000        406E0000        40700000        40720000        40740000      
>   40760000        40780000        407A0000        407C0000        407E0000      
>
> => protect on 2:2-4
> Protect Flash Sectors 2-4 in Bank # 2
> => fli 2           
>
> Bank # 2: CFI conformant FLASH (32 x 16)  Size: 4 MB in 35 Sectors
>   AMD Standard command set, Manufacturer ID: 0x04, Device ID: 0x2249
>   Erase timeout: 16384 ms, write timeout: 1 ms
>
>   Sector Start Addresses:
>   40400000        40408000        4040C000   RO   40410000   RO   40420000   RO 
>   40440000        40460000        40480000        404A0000        404C0000      
>   404E0000        40500000        40520000        40540000        40560000      
>   40580000        405A0000        405C0000        405E0000        40600000      
>   40620000        40640000        40660000        40680000        406A0000      
>   406C0000        406E0000        40700000        40720000        40740000      
>   40760000        40780000        407A0000        407C0000        407E0000      
>
> Case 1:
> =======
>
> => erase 40400000 4047FFFF
> - Warning: 3 protected sectors will not be erased!
> .... done
> Erased 7 sectors
>
> Case 2:
> =======
>
> => erase 40400000 +7FFFF
> - Warning: 3 protected sectors will not be erased!
> .... done
> Erased 7 sectors
>
> Case 3:
> =======
>
> => erase 2:0-6
> Erase Flash Sectors 0-6 in Bank # 2 - Warning: 3 protected sectors will not be erased!
> .... done
>
> Case 4:
> =======
>
> => erase bank 2
> Erase Flash Bank # 2 - Warning: 3 protected sectors will not be erased!
> ................................ done
>
>
> As you can see, we _always_ print a warning message.
>   
Actually, we usually print the warning message but not _always_.
That depends on the flash implementation (*flash.c) of each board.
At least 20 implementations currently do nothing and return with
ERR_PROTECTED if they found any protected sectors. I was porting U-Boot
to my board and found the artifact. Unfortunately (or fortunately in
some respect), I chose smdk2410's flash.c as a template which belongs
to the _irregular_ case.

> You can argument that it is incorrect to print "Erased 7 sectors"  in
> cases  1  and  2,  as  actually  only 7 - 3 = 4 have been erased, but
> printing "Not erased" would definitely be worse.
>
> If you want, and if you can find a clean way to implement it, it
> might make sense to change the output into something like "Erased 4
> (instead of 7 requested) sectors" or the like.
>   
I think we need to first make all of them consistent. My suggestion is:
- display a warning message in flash_erase() that there are some
  protected sectors and erase unprotected sectors like now.
- remove the number indicating how manny sectors are erased from the
  message in flash_sect_erase() or any caller of flash_erase(). A simple
  message like "done" would be enough.
>
> NAK for the patch as is.
>   
Agree, of course.
>
> Best regards,
>
> Wolfgang Denk
>
>   

Best regards,
Seunghyeon Rhee