[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");
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@denx.de
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
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@denx.de
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");elseprintf ("Erased %d sectors\n", erased);
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
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");elseprintf ("Erased %d sectors\n", erased);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
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
participants (4)
-
"Seunghyeon Rhee (이승현)" -
Stefan Roese -
Wolfgang Denk -
이승현