| Message ID | d4639ffb4c3a7ec8391e3c2eea7d13b32d55094e.1409822589.git.baruch@tkos.co.il |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Stefan Roese |
| Headers | show |
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
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
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
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
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
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,
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(-)