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