Skip to contentSkip to conversation

cfi_flash: don't hide write/erase errors

PATCH New Baruch Siach 3 participants 6 messages Sep 4, 2014 – Oct 6, 2014 Memory technology devices (MTD)

lore
Share & tools
lore

Baruch Siach

author
#0 lore raw

PATCH 1/1 cfi_flash: don't hide write/erase errors

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 <[email protected]>
Signed-off-by Baruch Siach <[email protected]>
---
 drivers/mtd/cfi_flash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/mtd/cfi_flash.c +1 -1
index c4b5bc1de553..9b3175d87fbd 100644
@@ -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,
Signature · 1 line
2.1.0
author
#1 lore raw

PATCH 1/1 cfi_flash: don't hide write/erase errors

Hi Stefan,

Quoted 18 lines · Baruch Siach

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 <[email protected]>
Signed-off-by Baruch Siach <[email protected]>
---
 drivers/mtd/cfi_flash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/mtd/cfi_flash.c +0 -0
index c4b5bc1de553..9b3175d87fbd 100644

drivers/mtd/cfi_flash.c static int flash_full_status_check (flash_info_t * info, flash_sect_t sector, · line 599

Quoted 8 lines
drivers/mtd/cfi_flash.c +1 -1
@@ -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

Signature · 3 lines
     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 -
#2 lore raw

PATCH 1/1 cfi_flash: don't hide write/erase errors

Hi Baruch,

Quoted 14 lines · Baruch Siach
Quoted 13 lines · Baruch Siach

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 <[email protected]>
Signed-off-by Baruch Siach <[email protected]>
---
  drivers/mtd/cfi_flash.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 15 lines

drivers/mtd/cfi_flash.c static int flash_full_status_check (flash_info_t * info, flash_sect_t sector, · line 599

Quoted 13 lines
drivers/mtd/cfi_flash.c +1 -1
index c4b5bc1de553..9b3175d87fbd 100644
@@ -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

author
#3 lore raw

PATCH 1/1 cfi_flash: don't hide write/erase errors

Hi Stefan,

Quoted 33 lines · Stefan Roese
Quoted 14 lines · Baruch Siach
Quoted 13 lines · Baruch Siach

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 <[email protected]>
Signed-off-by Baruch Siach <[email protected]>
---
 drivers/mtd/cfi_flash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 15 lines

drivers/mtd/cfi_flash.c static int flash_full_status_check (flash_info_t * info, flash_sect_t sector, · line 599

Quoted 13 lines
drivers/mtd/cfi_flash.c +1 -1
index c4b5bc1de553..9b3175d87fbd 100644
@@ -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.

Quoted 8 lines

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

Signature · 3 lines
     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 -
#4 lore raw

PATCH 1/1 cfi_flash: don't hide write/erase errors

Quoted 13 lines · Baruch Siach
Quoted 3 lines

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:

Quoted 8 lines

* 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

#5 lore raw

PATCH 1/1 cfi_flash: don't hide write/erase errors

In reply to Baruch Siach

From: Baruch Siach <[email protected]>

Quoted 9 lines

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 <[email protected]>
Signed-off-by Baruch Siach <[email protected]>

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 <[email protected]>

Ed