Dear Wolfgang Denk, Wolfgang Denk wrote: > Dear =?UTF-8?B?7J207Iq57ZiE?=, > > In message <fa2126d60911130006q3d5a1879pb177a51a4544f...@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 <seunghy...@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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot