[U-Boot] cfi: Problem with Intel Strata 28F320 flash

[U-Boot] cfi: Problem with Intel Strata 28F320 flash

[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)!

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

commit 54652991caedc39b2ec2e5b49e750669bfcd1e2e
Author: Philippe De Muyter <phdm@macqel.be>
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
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] cfi: Problem with Intel Strata 28F320 flash

[Philippe De Muyter]

Hello Heiko,

On Tue, Mar 08, 2011 at 02:08:26PM +0100, Heiko Schocher wrote:
> 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 ?

> 
> After some debugging, I found out, that, if I revert commit
> 
> commit 54652991caedc39b2ec2e5b49e750669bfcd1e2e
> Author: Philippe De Muyter <phdm@macqel.be>
> 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?

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

Philippe

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

[U-Boot] cfi: Problem with Intel Strata 28F320 flash

[Heiko Schocher]

Hello Philippe,

Philippe De Muyter wrote:
> On Tue, Mar 08, 2011 at 02:08:26PM +0100, Heiko Schocher wrote:
>> 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.

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 ...

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

No, see above comment.

>> After some debugging, I found out, that, if I revert commit
>>
>> commit 54652991caedc39b2ec2e5b49e750669bfcd1e2e
>> Author: Philippe De Muyter <phdm@macqel.be>
>> 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?
> 
> 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

diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
index dd394a8..9d3fdcc 100644
--- a/drivers/mtd/cfi_flash.c
+++ b/drivers/mtd/cfi_flash.c
@@ -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:

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

[U-Boot] cfi: Problem with Intel Strata 28F320 flash

[Detlev Zundel]

Hi Heiko,

> 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
>
> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
> index dd394a8..9d3fdcc 100644
> --- a/drivers/mtd/cfi_flash.c
> +++ b/drivers/mtd/cfi_flash.c
> @@ -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:

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

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

[U-Boot] cfi: Problem with Intel Strata 28F320 flash

[Gerlando Falauto]

Hi all,

On 03/09/2011 02:21 PM, Detlev Zundel wrote:
> Hi Heiko,
>
>> 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

[...]

> 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

[U-Boot] cfi: Problem with Intel Strata 28F320 flash

[Heiko Schocher]

Hello Gerlando,

On 27.07.2012 16:11, Gerlando Falauto wrote:
> Hi all,
>
> On 03/09/2011 02:21 PM, Detlev Zundel wrote:
>> Hi Heiko,
>>
>>> 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
>
> [...]
>
>> 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)?

I don?t know ...

> 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 ...

> 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
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] cfi: Problem with Intel Strata 28F320 flash

[Stefan Roese]

Hi Heiko,

On Monday 30 July 2012 13:07:07 Heiko Schocher wrote:
> > 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 ...
> 
> > 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 ...
> 
> > 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

[U-Boot] cfi: Problem with Intel Strata 28F320 flash

[Stefan Roese]

Hi Heiko,

On Monday 30 July 2012 13:07:07 Heiko Schocher wrote:
> > 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