* [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