* [U-Boot] [PATCH] cfi_flash: don't hide write/erase errors @ 2014-09-04 9:23 Baruch Siach 2014-10-06 6:19 ` Baruch Siach 2014-10-06 16:28 ` Edward L Swarthout 0 siblings, 2 replies; 6+ messages in thread From: Baruch Siach @ 2014-09-04 9:23 UTC (permalink / raw) To: u-boot Partially revert commit 0d01f66d235118 (CFI: cfi_flash write fix for AMD legacy). flash_full_status_check() used to skip status register parsing when flash_status_check() returns OK. This is wrong since flash_status_check() must return OK for other status bits to be valid. Cc: Ed Swarthout <Ed.Swarthout@freescale.com> Signed-off-by: Baruch Siach <baruch@tkos.co.il> --- drivers/mtd/cfi_flash.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index c4b5bc1de553..9b3175d87fbd 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -593,7 +593,7 @@ static int flash_full_status_check (flash_info_t * info, flash_sect_t sector, case CFI_CMDSET_INTEL_PROG_REGIONS: case CFI_CMDSET_INTEL_EXTENDED: case CFI_CMDSET_INTEL_STANDARD: - if ((retcode != ERR_OK) + if ((retcode == ERR_OK) && !flash_isequal (info, sector, 0, FLASH_STATUS_DONE)) { retcode = ERR_INVAL; printf ("Flash %s error at address %lx\n", prompt, -- 2.1.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] cfi_flash: don't hide write/erase errors 2014-09-04 9:23 [U-Boot] [PATCH] cfi_flash: don't hide write/erase errors Baruch Siach @ 2014-10-06 6:19 ` Baruch Siach 2014-10-06 8:20 ` Stefan Roese 2014-10-06 16:28 ` Edward L Swarthout 1 sibling, 1 reply; 6+ messages in thread From: Baruch Siach @ 2014-10-06 6:19 UTC (permalink / raw) To: u-boot Hi Stefan, On Thu, Sep 04, 2014 at 12:23:09PM +0300, Baruch Siach wrote: > Partially revert commit 0d01f66d235118 (CFI: cfi_flash write fix for AMD > legacy). > > flash_full_status_check() used to skip status register parsing when > flash_status_check() returns OK. This is wrong since flash_status_check() > must return OK for other status bits to be valid. > > Cc: Ed Swarthout <Ed.Swarthout@freescale.com> > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > --- > drivers/mtd/cfi_flash.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c > index c4b5bc1de553..9b3175d87fbd 100644 > --- a/drivers/mtd/cfi_flash.c > +++ b/drivers/mtd/cfi_flash.c > @@ -593,7 +593,7 @@ static int flash_full_status_check (flash_info_t * info, flash_sect_t sector, > case CFI_CMDSET_INTEL_PROG_REGIONS: > case CFI_CMDSET_INTEL_EXTENDED: > case CFI_CMDSET_INTEL_STANDARD: > - if ((retcode != ERR_OK) > + if ((retcode == ERR_OK) > && !flash_isequal (info, sector, 0, FLASH_STATUS_DONE)) { > retcode = ERR_INVAL; > printf ("Flash %s error at address %lx\n", prompt, Ping? baruch -- http://baruch.siach.name/blog/ ~. .~ Tk Open Systems =}------------------------------------------------ooO--U--Ooo------------{= - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il - ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] cfi_flash: don't hide write/erase errors 2014-10-06 6:19 ` Baruch Siach @ 2014-10-06 8:20 ` Stefan Roese 2014-10-06 11:32 ` Baruch Siach 0 siblings, 1 reply; 6+ messages in thread From: Stefan Roese @ 2014-10-06 8:20 UTC (permalink / raw) To: u-boot Hi Baruch, On 06.10.2014 08:19, Baruch Siach wrote: > On Thu, Sep 04, 2014 at 12:23:09PM +0300, Baruch Siach wrote: >> Partially revert commit 0d01f66d235118 (CFI: cfi_flash write fix for AMD >> legacy). >> >> flash_full_status_check() used to skip status register parsing when >> flash_status_check() returns OK. This is wrong since flash_status_check() >> must return OK for other status bits to be valid. >> >> Cc: Ed Swarthout <Ed.Swarthout@freescale.com> >> Signed-off-by: Baruch Siach <baruch@tkos.co.il> >> --- >> drivers/mtd/cfi_flash.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c >> index c4b5bc1de553..9b3175d87fbd 100644 >> --- a/drivers/mtd/cfi_flash.c >> +++ b/drivers/mtd/cfi_flash.c >> @@ -593,7 +593,7 @@ static int flash_full_status_check (flash_info_t * info, flash_sect_t sector, >> case CFI_CMDSET_INTEL_PROG_REGIONS: >> case CFI_CMDSET_INTEL_EXTENDED: >> case CFI_CMDSET_INTEL_STANDARD: >> - if ((retcode != ERR_OK) >> + if ((retcode == ERR_OK) >> && !flash_isequal (info, sector, 0, FLASH_STATUS_DONE)) { >> retcode = ERR_INVAL; >> printf ("Flash %s error at address %lx\n", prompt, > > Ping? Sorry, I forgot about this one. I have to admit that I'm a bit hesitant here. Since your patch changes the behavior thats present for about than 6 years. You're the first encountering some problems here. And I'm not that actively using CFI NOR flash anymore as well, so my knowledge is a bit "rusty" here as well. Could you please summarize again, what the real problem with this compare is. What is the error exactly in your case (which flash chip is used and which command was issued?)? Thanks, Stefan ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] cfi_flash: don't hide write/erase errors 2014-10-06 8:20 ` Stefan Roese @ 2014-10-06 11:32 ` Baruch Siach 2014-10-06 12:09 ` Stefan Roese 0 siblings, 1 reply; 6+ messages in thread From: Baruch Siach @ 2014-10-06 11:32 UTC (permalink / raw) To: u-boot Hi Stefan, On Mon, Oct 06, 2014 at 10:20:43AM +0200, Stefan Roese wrote: > On 06.10.2014 08:19, Baruch Siach wrote: > >On Thu, Sep 04, 2014 at 12:23:09PM +0300, Baruch Siach wrote: > >>Partially revert commit 0d01f66d235118 (CFI: cfi_flash write fix for AMD > >>legacy). > >> > >>flash_full_status_check() used to skip status register parsing when > >>flash_status_check() returns OK. This is wrong since flash_status_check() > >>must return OK for other status bits to be valid. > >> > >>Cc: Ed Swarthout <Ed.Swarthout@freescale.com> > >>Signed-off-by: Baruch Siach <baruch@tkos.co.il> > >>--- > >> drivers/mtd/cfi_flash.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >>diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c > >>index c4b5bc1de553..9b3175d87fbd 100644 > >>--- a/drivers/mtd/cfi_flash.c > >>+++ b/drivers/mtd/cfi_flash.c > >>@@ -593,7 +593,7 @@ static int flash_full_status_check (flash_info_t * info, flash_sect_t sector, > >> case CFI_CMDSET_INTEL_PROG_REGIONS: > >> case CFI_CMDSET_INTEL_EXTENDED: > >> case CFI_CMDSET_INTEL_STANDARD: > >>- if ((retcode != ERR_OK) > >>+ if ((retcode == ERR_OK) > >> && !flash_isequal (info, sector, 0, FLASH_STATUS_DONE)) { > >> retcode = ERR_INVAL; > >> printf ("Flash %s error at address %lx\n", prompt, > > > >Ping? > > Sorry, I forgot about this one. > > I have to admit that I'm a bit hesitant here. Since your patch changes the > behavior thats present for about than 6 years. You're the first encountering > some problems here. And I'm not that actively using CFI NOR flash anymore as > well, so my knowledge is a bit "rusty" here as well. > > Could you please summarize again, what the real problem with this compare > is. What is the error exactly in your case (which flash chip is used and > which command was issued?)? The Micron StrataFlash datasheet says this on Status Register bit 7 (Device status): 0 = Device is busy; SR[9,8,6:1] are invalid, SR[0] is valid 1 = Device is ready; SR[9:8], SR[6:1] are valid This is was the original code behaviour, since your commit 79b4cda076069d04122f. This commit log explicitly says: * Changes/fixes for drivers/cfi_flash.c: We *should* check if there are any error bits if the previous call returned ERR_OK (Otherwise we will have output an error message in flash_status_check() already.) The original code would only check for error bits if flash_status_check() returns ERR_TIMEOUT. Patch by Marcus Hall, 23 Aug 2005 Currently U-Boot is just silent about NOR flash write errors. baruch -- http://baruch.siach.name/blog/ ~. .~ Tk Open Systems =}------------------------------------------------ooO--U--Ooo------------{= - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il - ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] cfi_flash: don't hide write/erase errors 2014-10-06 11:32 ` Baruch Siach @ 2014-10-06 12:09 ` Stefan Roese 0 siblings, 0 replies; 6+ messages in thread From: Stefan Roese @ 2014-10-06 12:09 UTC (permalink / raw) To: u-boot On 06.10.2014 13:32, Baruch Siach wrote: >> Could you please summarize again, what the real problem with this compare >> is. What is the error exactly in your case (which flash chip is used and >> which command was issued?)? > > The Micron StrataFlash datasheet says this on Status Register bit 7 (Device > status): > > 0 = Device is busy; SR[9,8,6:1] are invalid, SR[0] is valid > 1 = Device is ready; SR[9:8], SR[6:1] are valid > > This is was the original code behaviour, since your commit > 79b4cda076069d04122f. This commit log explicitly says: > > * Changes/fixes for drivers/cfi_flash.c: > We *should* check if there are any error bits if the previous call > returned ERR_OK (Otherwise we will have output an error message in > flash_status_check() already.) The original code would only check for > error bits if flash_status_check() returns ERR_TIMEOUT. > Patch by Marcus Hall, 23 Aug 2005 > > Currently U-Boot is just silent about NOR flash write errors. Okay. Thanks for the summary. I'll prepare a pull request. Thanks, Stefan ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] cfi_flash: don't hide write/erase errors 2014-09-04 9:23 [U-Boot] [PATCH] cfi_flash: don't hide write/erase errors Baruch Siach 2014-10-06 6:19 ` Baruch Siach @ 2014-10-06 16:28 ` Edward L Swarthout 1 sibling, 0 replies; 6+ messages in thread From: Edward L Swarthout @ 2014-10-06 16:28 UTC (permalink / raw) To: u-boot From: Baruch Siach <baruch@tkos.co.il> > Partially revert commit 0d01f66d235118 (CFI: cfi_flash write fix for AMD > legacy). > > flash_full_status_check() used to skip status register parsing when > flash_status_check() returns OK. This is wrong since flash_status_check() > must return OK for other status bits to be valid. > Cc: Ed Swarthout <Ed.Swarthout@freescale.com> > Signed-off-by: Baruch Siach <baruch@tkos.co.il> I don't have access to the error case I thought I was fixing with this part and my change does look incorrect, so Acked-by: Ed Swarthout <Ed.Swarthout@freescale.com> Ed ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-10-06 16:28 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-09-04 9:23 [U-Boot] [PATCH] cfi_flash: don't hide write/erase errors Baruch Siach 2014-10-06 6:19 ` Baruch Siach 2014-10-06 8:20 ` Stefan Roese 2014-10-06 11:32 ` Baruch Siach 2014-10-06 12:09 ` Stefan Roese 2014-10-06 16:28 ` Edward L Swarthout
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox