cfi: Problem with Intel Strata 28F320 flash

U-Boot Heiko Schocher 5 participants 8 messages Mar 8, 2011 – Aug 3, 2012 Memory technology devices (MTD)

Share & tools

Heiko Schocher

author

2011-03-08 13:08 UTC #0 lore raw

Hello,

as I am actual trying to get the keymile boards in sync with actual mainline u-boot, I faced the following Problem with an Intel Strata Flash on the mgcoge (mpc8247 based board):

I couldn;t unprotect/erase/write/protect some (not all!) Flash sectors.

For Example, I could do this without errors on the sectors where u-boot sits (First three sectors), but not with the environment (next two sectors)!

After some debugging, I found out, that, if I revert commit

commit 54652991caedc39b2ec2e5b49e750669bfcd1e2e

Author: Philippe De Muyter <[email protected]>
Date:   Tue Aug 17 18:40:25 2010 +0200

Work around bug in Numonyx P33/P30 256-Mbit 65nm flash chips.

I have "ported" U-boot to a in house made board with Numonyx Axcell P33/P30 256-Mbit 65nm flash chips.

After some time :( searching for bugs in our board or soft, we have discovered that those chips have a small but annoying bug, documented in "Numonyx Axcell P33/P30 256-Mbit Specification Update"
[...]

It works again fine, and without problems ... did somebody faced similiar issues with the cfi driver? Some Ideas?

bye,
Heiko

Signature · 2 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Philippe De Muyter

2011-03-08 14:26 UTC #1 lore raw

Hello Heiko,

Quoted 11 lines · Heiko Schocher

Hello,
as I am actual trying to get the keymile boards in sync with actual mainline u-boot, I faced the following Problem with an Intel Strata Flash on the mgcoge (mpc8247 based board):
I couldn;t unprotect/erase/write/protect some (not all!) Flash sectors.
For Example, I could do this without errors on the sectors where u-boot sits (First three sectors), but not with the environment (next two sectors)!

That's exactly the problem I had, but triggered by another valid command sequence.

So it is also a flash bug.

Have you searched with google for your chip plus "errata" or "specification update" as they call that ?

Quoted 12 lines

After some debugging, I found out, that, if I revert commit
commit 54652991caedc39b2ec2e5b49e750669bfcd1e2e
Author: Philippe De Muyter <[email protected]>
Date:   Tue Aug 17 18:40:25 2010 +0200
Work around bug in Numonyx P33/P30 256-Mbit 65nm flash chips.
I have "ported" U-boot to a in house made board with Numonyx Axcell P33/P30 256-Mbit 65nm flash chips.

Quoted 7 lines

After some time :( searching for bugs in our board or soft, we have discovered that those chips have a small but annoying bug, documented in "Numonyx Axcell P33/P30 256-Mbit Specification Update"
[...]
It works again fine, and without problems ... did somebody faced similiar issues with the cfi driver? Some Ideas?

It seems like we'll need to check the flash chip type before issuing one command sequence or another.

Philippe

Signature · 1 line

Philippe De Muyter +32 2 6101532 Macq SA rue de l'Aeronef 2 B-1140 Bruxelles

Heiko Schocher

author

2011-03-09 06:41 UTC #2 lore raw

Hello Philippe,

Quoted 9 lines · Philippe De Muyter

Quoted 8 lines · Heiko Schocher
Hello,
as I am actual trying to get the keymile boards in sync with actual mainline u-boot, I faced the following Problem with an Intel Strata Flash on the mgcoge (mpc8247 based board):
I couldn;t unprotect/erase/write/protect some (not all!) Flash sectors.

Quoted 8 lines

Quoted 3 lines
For Example, I could do this without errors on the sectors where u-boot sits (First three sectors), but not with the environment (next two sectors)!
That's exactly the problem I had, but triggered by another valid command sequence.
So it is also a flash bug.

Hmm.. I wouldn;t call it bug, because the sequence which is run without 54652991caedc39b2ec2e5b49e750669bfcd1e2e, is exactly the sequence, which is described in the datasheet for the 29Fxxx chips ...

Quoted 2 lines

Have you searched with google for your chip plus "errata" or "specification update" as they call that ?

No, see above comment.

Quoted 16 lines

Quoted 16 lines
After some debugging, I found out, that, if I revert commit
commit 54652991caedc39b2ec2e5b49e750669bfcd1e2e
Author: Philippe De Muyter <[email protected]>
Date:   Tue Aug 17 18:40:25 2010 +0200
Work around bug in Numonyx P33/P30 256-Mbit 65nm flash chips.
I have "ported" U-boot to a in house made board with Numonyx Axcell P33/P30 256-Mbit 65nm flash chips.
After some time :( searching for bugs in our board or soft, we have discovered that those chips have a small but annoying bug, documented in "Numonyx Axcell P33/P30 256-Mbit Specification Update"
[...]

Quoted 5 lines

Quoted 2 lines
It works again fine, and without problems ... did somebody faced similiar issues with the cfi driver? Some Ideas?
It seems like we'll need to check the flash chip type before issuing one command sequence or another.

Maybe a way to go ... more comments?

Below a patch, which introduces a function, which checks for "protection bugfixes", and if no bugfix is found the old code is executed. Just a fast RFC patch.

bye,
Heiko

drivers/mtd/cfi_flash.c +41 ~~-22~~

index dd394a8..9d3fdcc 100644
@@ -1376,6 +1376,38 @@ int write_buff (flash_info_t * info, uchar * src, ulong addr, ulong cnt)
  */
 #ifdef CONFIG_SYS_FLASH_PROTECTION
 
+static int cfi_protect_bugfix(flash_info_t * info, long sector, int prot)
+{
+	if ((info->manufacturer_id == 0x89) && (info->device_id == 0x8922)) {
+		/*
+		 * see errata called
+		 * "Numonyx Axcell P33/P30 Specification Update" :)
+		 */
+		flash_write_cmd (info, sector, 0, FLASH_CMD_READ_ID);
+		if (!flash_isequal (info, sector, FLASH_OFFSET_PROTECT,
+				    prot)) {
+			/*
+			 * cmd must come before FLASH_CMD_PROTECT + 20us
+			 * Disable interrupts which might cause a timeout here.
+			 */
+			int flag = disable_interrupts ();
+			unsigned short cmd;
+				if (prot)
+				cmd = FLASH_CMD_PROTECT_SET;
+			else
+				cmd = FLASH_CMD_PROTECT_CLEAR;
+				flash_write_cmd (info, sector, 0,
+					  FLASH_CMD_PROTECT);
+			flash_write_cmd (info, sector, 0, cmd);
+			/* re-enable interrupts if necessary */
+			if (flag)
+				enable_interrupts ();
+		}
+		return 1;
+	}
+	return 0;
+}
+
 int flash_real_protect (flash_info_t * info, long sector, int prot)
 {
 	int retcode = 0;

@@ -1384,31 +1416,18 @@ int flash_real_protect (flash_info_t * info, long sector, int prot)
 		case CFI_CMDSET_INTEL_PROG_REGIONS:
 		case CFI_CMDSET_INTEL_STANDARD:
 		case CFI_CMDSET_INTEL_EXTENDED:
-			/*
-			 * see errata called
-			 * "Numonyx Axcell P33/P30 Specification Update" :)
-			 */
-			flash_write_cmd (info, sector, 0, FLASH_CMD_READ_ID);
-			if (!flash_isequal (info, sector, FLASH_OFFSET_PROTECT,
-					    prot)) {
-				/*
-				 * cmd must come before FLASH_CMD_PROTECT + 20us
-				 * Disable interrupts which might cause a timeout here.
-				 */
-				int flag = disable_interrupts ();
-				unsigned short cmd;
-
+			if (!cfi_protect_bugfix(info, sector, prot)) {
+				flash_write_cmd (info, sector, 0,
+					 FLASH_CMD_CLEAR_STATUS);
+				flash_write_cmd (info, sector, 0,
+					FLASH_CMD_PROTECT);
 				if (prot)
-					cmd = FLASH_CMD_PROTECT_SET;
+					flash_write_cmd (info, sector, 0,
+						FLASH_CMD_PROTECT_SET);
 				else
-					cmd = FLASH_CMD_PROTECT_CLEAR;
+					flash_write_cmd (info, sector, 0,
+						FLASH_CMD_PROTECT_CLEAR);
 
-				flash_write_cmd (info, sector, 0,
-						  FLASH_CMD_PROTECT);
-				flash_write_cmd (info, sector, 0, cmd);
-				/* re-enable interrupts if necessary */
-				if (flag)
-					enable_interrupts ();
 			}
 			break;
 		case CFI_CMDSET_AMD_EXTENDED:

Signature · 2 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Detlev Zundel

2011-03-09 13:21 UTC #3 lore raw

Hi Heiko,

Quoted 83 lines

Maybe a way to go ... more comments?

Below a patch, which introduces a function, which checks for "protection bugfixes", and if no bugfix is found the old code is executed. Just a fast RFC patch.

bye,
Heiko

drivers/mtd/cfi_flash.c +41 ~~-16~~

index dd394a8..9d3fdcc 100644
@@ -1376,6 +1376,38 @@ int write_buff (flash_info_t * info, uchar * src, ulong addr, ulong cnt)
  */
 #ifdef CONFIG_SYS_FLASH_PROTECTION
 
+static int cfi_protect_bugfix(flash_info_t * info, long sector, int prot)
+{
+	if ((info->manufacturer_id == 0x89) && (info->device_id == 0x8922)) {
+		/*
+		 * see errata called
+		 * "Numonyx Axcell P33/P30 Specification Update" :)
+		 */
+		flash_write_cmd (info, sector, 0, FLASH_CMD_READ_ID);
+		if (!flash_isequal (info, sector, FLASH_OFFSET_PROTECT,
+				    prot)) {
+			/*
+			 * cmd must come before FLASH_CMD_PROTECT + 20us
+			 * Disable interrupts which might cause a timeout here.
+			 */
+			int flag = disable_interrupts ();
+			unsigned short cmd;
+				if (prot)
+				cmd = FLASH_CMD_PROTECT_SET;
+			else
+				cmd = FLASH_CMD_PROTECT_CLEAR;
+				flash_write_cmd (info, sector, 0,
+					  FLASH_CMD_PROTECT);
+			flash_write_cmd (info, sector, 0, cmd);
+			/* re-enable interrupts if necessary */
+			if (flag)
+				enable_interrupts ();
+		}
+		return 1;
+	}
+	return 0;
+}
+
 int flash_real_protect (flash_info_t * info, long sector, int prot)
 {
 	int retcode = 0;

@@ -1384,31 +1416,18 @@ int flash_real_protect (flash_info_t * info, long sector, int prot)
 		case CFI_CMDSET_INTEL_PROG_REGIONS:
 		case CFI_CMDSET_INTEL_STANDARD:
 		case CFI_CMDSET_INTEL_EXTENDED:
-			/*
-			 * see errata called
-			 * "Numonyx Axcell P33/P30 Specification Update" :)
-			 */
-			flash_write_cmd (info, sector, 0, FLASH_CMD_READ_ID);
-			if (!flash_isequal (info, sector, FLASH_OFFSET_PROTECT,
-					    prot)) {
-				/*
-				 * cmd must come before FLASH_CMD_PROTECT + 20us
-				 * Disable interrupts which might cause a timeout here.
-				 */
-				int flag = disable_interrupts ();
-				unsigned short cmd;
-
+			if (!cfi_protect_bugfix(info, sector, prot)) {
+				flash_write_cmd (info, sector, 0,
+					 FLASH_CMD_CLEAR_STATUS);
+				flash_write_cmd (info, sector, 0,
+					FLASH_CMD_PROTECT);
 				if (prot)
-					cmd = FLASH_CMD_PROTECT_SET;
+					flash_write_cmd (info, sector, 0,
+						FLASH_CMD_PROTECT_SET);
 				else
-					cmd = FLASH_CMD_PROTECT_CLEAR;
+					flash_write_cmd (info, sector, 0,
+						FLASH_CMD_PROTECT_CLEAR);

drivers/mtd/cfi_flash.c int flash_real_protect (flash_info_t * info, long sector, int prot) · line 1433

Quoted 10 lines

drivers/mtd/cfi_flash.c +0 -6

 
-				flash_write_cmd (info, sector, 0,
-						  FLASH_CMD_PROTECT);
-				flash_write_cmd (info, sector, 0, cmd);
-				/* re-enable interrupts if necessary */
-				if (flag)
-					enable_interrupts ();
 			}
 			break;
 		case CFI_CMDSET_AMD_EXTENDED:

Can't we introduce a field "chip_quirk" in flash_info_t, and upon flash enumeration check for the specific chip version and for example put a CFI_QUIRK_PROTECT in it for this chip. The core code can then check for this flag and call functions appropriately.

In other words, why not introduce a generic infrastructure for handling chip quirks that may need different handling for other functions also.

Cheers
Detlev

Signature · 7 lines

Programming X-Windows is like trying to find the square root of pi
using roman numerals.
                                          -- The UNIX Haters Handbook
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

Gerlando Falauto

2012-07-27 14:11 UTC #4 lore raw

Hi all,

Quoted 10 lines · Detlev Zundel

Hi Heiko,
Quoted 8 lines
Maybe a way to go ... more comments?
Below a patch, which introduces a function, which checks for "protection bugfixes", and if no bugfix is found the old code is executed. Just a fast RFC patch.
bye,
Heiko

[...]

Quoted 11 lines

Can't we introduce a field "chip_quirk" in flash_info_t, and upon flash enumeration check for the specific chip version and for example put a CFI_QUIRK_PROTECT in it for this chip. The core code can then check for this flag and call functions appropriately.
In other words, why not introduce a generic infrastructure for handling chip quirks that may need different handling for other functions also.
Cheers
Detlev

Have there been (since the original posting) other instances of flash parts requiring quirks (like the original one introduced by Philippe De Muyter for the Numonyx chip)?
Is there any ongoing activity on this or any other reason to suggest it might be necessary to introduce such generic infrastructure (like the one in linux mtd, the way I understand it)?

If that's not the case, wouldn't Heicho's original patch in this thread
(http://patchwork.ozlabs.org/patch/86063/) just be good enough for the purpose?

Thanks,
Gerlando

Heiko Schocher

author

2012-07-30 11:07 UTC #5 lore raw

Hello Gerlando,

Quoted 21 lines · Gerlando Falauto

Hi all,
Quoted 10 lines · Detlev Zundel
Hi Heiko,
Quoted 8 lines
Maybe a way to go ... more comments?
Below a patch, which introduces a function, which checks for "protection bugfixes", and if no bugfix is found the old code is executed. Just a fast RFC patch.
bye,
Heiko
[...]
Quoted 5 lines
Can't we introduce a field "chip_quirk" in flash_info_t, and upon flash enumeration check for the specific chip version and for example put a CFI_QUIRK_PROTECT in it for this chip. The core code can then check for this flag and call functions appropriately.

Quoted 8 lines

Quoted 6 lines
In other words, why not introduce a generic infrastructure for handling chip quirks that may need different handling for other functions also.
Cheers
Detlev
Have there been (since the original posting) other instances of flash parts requiring quirks (like the original one introduced by Philippe De Muyter for the Numonyx chip)?

I don?t know ...

Quoted 1 line

Is there any ongoing activity on this or any other reason to suggest it might be necessary to introduce such generic infrastructure (like the one in linux mtd, the way I understand it)?

I have no current activity on this ...

Quoted 2 lines

If that's not the case, wouldn't Heicho's original patch in this thread
(http://patchwork.ozlabs.org/patch/86063/) just be good enough for the purpose?

I am here on Detlevs side, but if it is currently only one usecase here, maybe "my" patch is enough ... Stefan? Detlev?

bye,
Heiko

Signature · 2 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Stefan Roese

2012-07-30 11:11 UTC #6 lore raw

Hi Heiko,

Quoted 10 lines · On Monday 30 July 2012 13:07:07 Heiko Schocher wrote:

Quoted 3 lines
Have there been (since the original posting) other instances of flash parts requiring quirks (like the original one introduced by Philippe De Muyter for the Numonyx chip)?
I don?t know ...
Quoted 3 lines
Is there any ongoing activity on this or any other reason to suggest it might be necessary to introduce such generic infrastructure (like the one in linux mtd, the way I understand it)?

Quoted 8 lines

I have no current activity on this ...
Quoted 3 lines
If that's not the case, wouldn't Heicho's original patch in this thread
(http://patchwork.ozlabs.org/patch/86063/) just be good enough for the purpose?
I am here on Detlevs side, but if it is currently only one usecase here, maybe "my" patch is enough ... Stefan? Detlev?

I'm just back from vacation and I'll look into this in a few days. Promise!

Viele Gr??e,
Stefan

--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office at denx.de

Stefan Roese

2012-08-03 08:01 UTC #7 lore raw

In reply to Heiko Schocher

Hi Heiko,

Quoted 5 lines · On Monday 30 July 2012 13:07:07 Heiko Schocher wrote:

Quoted 3 lines
If that's not the case, wouldn't Heicho's original patch in this thread
(http://patchwork.ozlabs.org/patch/86063/) just be good enough for the purpose?
I am here on Detlevs side, but if it is currently only one usecase here, maybe "my" patch is enough ... Stefan? Detlev?

After looking into this issue again, I'm in favor to accepting Heiko's patch to fix this problem. Detlev's suggestion would have been better of course. But since we really only have one use-case right now, it seems a bit unfair to request/demand the installation of such a quirk infrastructure for this fix.

Heiko, could you please send an updated patch with a complete patch description? And also please don't use magic numbers like 0x89 but INTEL_MANUFACT.

Thanks,
Stefan