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

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

[이승현]

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@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");
-- 
1.6.2.5



-------
Seunghyeon Rhee, Ph.D. / Director
LPM Technology Inc.
T +82-70-8255-6007  F +82-2-6442-6462
M +82-10-2790-0657

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

[Stefan Roese]

On Friday 13 November 2009 09:06:32 ??? wrote:
> flash_sect_erase() displays message "Erased #N sectors" even when
> there are some protected sectors found and command "erase" fail.

Applied to u-boot-cfi-flash/master. Thanks.
 
Cheers,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office at denx.de

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

[Wolfgang Denk]

Dear Stefan Roese,

In message <200911171614.17710.sr@denx.de> you wrote:
> > flash_sect_erase() displays message "Erased #N sectors" even when
> > there are some protected sectors found and command "erase" fail.
> 
> Applied to u-boot-cfi-flash/master. Thanks.

Sory, but you were too fast for me. Please put this commit on hold.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
You get a wonderful view from the point of no return.
                                    - Terry Pratchett, _Making_Money_

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

[Stefan Roese]

Hi Wolfgang,

On Tuesday 17 November 2009 22:22:23 Wolfgang Denk wrote:
> > > flash_sect_erase() displays message "Erased #N sectors" even when
> > > there are some protected sectors found and command "erase" fail.
> >
> > Applied to u-boot-cfi-flash/master. Thanks.
> 
> Sory, but you were too fast for me. Please put this commit on hold.

Yes, removed from master again.
 
Cheers,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office at denx.de

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

[Wolfgang Denk]

Dear =?UTF-8?B?7J207Iq57ZiE?=,

In message <fa2126d60911130006q3d5a1879pb177a51a4544fb6b@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@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.

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.


NAK for the patch as is.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The manager will be continually amazed that policies he took for com-
mon knowledge are totally unknown by some member of his  team.  Since
his fundamental job is to keep everybody going in the same direction,
his chief daily task will be communication, not decision-making.
                              - Fred Brooks, "The Mythical Man Month"

[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@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@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

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

[Wolfgang Denk]

Dear =?UTF-8?B?IlNldW5naHllb24gUmhlZSAo7J207Iq57ZiEKSI=?=,

In message <4B035EE8.90702@lpmtec.com> you wrote:
> 
> > 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.
...
> > 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.

Ah! So you are talking about a custom flash driver.

> At least 20 implementations currently do nothing and return with
> ERR_PROTECTED if they found any protected sectors. I was porting U-Boot

Well, in this case we should consider this flash driver incomplete or
broken and fix that one first.

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

So I suggest you dump your copy of that driver code, and use the CFI
driver instead - it seems the smdk2410 flash driver supports only CFI
conformant flashes anyway, so it could/should be removed.

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

OK, then please start by removing obsolete and redundant driver code
and use the CFI driver wherever possible.

> - display a warning message in flash_erase() that there are some
>   protected sectors and erase unprotected sectors like now.

OK.

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

I disagree. I find the number of erased sectors a pretty useful
information (for example when it comes to calculate ease timings).

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Alles Gescheite ist schon gedacht worden, man mu? nur versuchen,
es noch einmal zu denken.          -- Goethe, Maximen und Reflexionen