[U-Boot] [PATCH CFI flash]: Workaround for Numonyx Axcell P33/P30 256-Mbit 65nm bug

[U-Boot] [PATCH CFI flash]: Workaround for Numonyx Axcell P33/P30 256-Mbit 65nm bug

[Philippe De Muyter]

Hello Wolfgang & list,

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 states :
When customer uses [...] block unlock, the block lock status might be
altered inadvertently. Lock status might be set to either 01h or 03h
unexpectedly (00h as expected data), which leads to program/erase failure
on certain blocks.

A working workaround is given, which I have applied and tested with success.

Signed-off-by: Philippe De Muyter <phdm@macqel.be>

---
 drivers/mtd/cfi_flash.c |   27 ++++++++++++++++++++-------
 1 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
index 3267c5d..0931137 100644
--- a/drivers/mtd/cfi_flash.c
+++ b/drivers/mtd/cfi_flash.c
@@ -1347,16 +1347,29 @@ int flash_real_protect (flash_info_t * info, long sector, int prot)
 	switch (info->vendor) {
 		case CFI_CMDSET_INTEL_PROG_REGIONS:
 		case CFI_CMDSET_INTEL_STANDARD:
-		case CFI_CMDSET_INTEL_EXTENDED:
-			flash_write_cmd (info, sector, 0,
-					 FLASH_CMD_CLEAR_STATUS);
-			flash_write_cmd (info, sector, 0, FLASH_CMD_PROTECT);
+		case CFI_CMDSET_INTEL_EXTENDED: {
+			unsigned short cmd;
+
 			if (prot)
-				flash_write_cmd (info, sector, 0,
-					FLASH_CMD_PROTECT_SET);
+				cmd = FLASH_CMD_PROTECT_SET;
 			else
+				cmd = FLASH_CMD_PROTECT_CLEAR;
+			/* see errata
+			   "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 20us after FLASH_CMD_PROTECT */
+				/* Disable interrupts which might cause a timeout here */
+				int flag = disable_interrupts ();
 				flash_write_cmd (info, sector, 0,
-					FLASH_CMD_PROTECT_CLEAR);
+						  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:
 		case CFI_CMDSET_AMD_STANDARD:
-- 
1.6.3.3



-- 
Philippe De Muyter  phdm at macqel dot be  Tel +32 27029044
Macq Electronique SA  rue de l'Aeronef 2  B-1140 Bruxelles  Fax +32 27029077

[U-Boot] [PATCH CFI flash]: Workaround for Numonyx Axcell P33/P30 256-Mbit 65nm bug

[Wolfgang Denk]

Dear Philippe De Muyter,

In message <20100623115701.GA17625@frolo.macqel> you wrote:
> Hello Wolfgang & list,

You might want to put the CFI custodian on Cc:

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

Heh ;-)

> +		case CFI_CMDSET_INTEL_EXTENDED: {
> +			unsigned short cmd;
> +
>  			if (prot)
> -				flash_write_cmd (info, sector, 0,
> -					FLASH_CMD_PROTECT_SET);
> +				cmd = FLASH_CMD_PROTECT_SET;
>  			else
> +				cmd = FLASH_CMD_PROTECT_CLEAR;
> +			/* see errata
> +			   "Numonyx Axcell P33/P30 Specification Update" :) */

Incorrect multiline comment style.

> +			flash_write_cmd (info, sector, 0, FLASH_CMD_READ_ID);
> +			if (!flash_isequal (info, sector, FLASH_OFFSET_PROTECT,
> +					    prot)) {
> +				/* cmd must come before 20us after FLASH_CMD_PROTECT */
> +				/* Disable interrupts which might cause a timeout here */

Incorrect multiline comment style.

And please move the declaration above the comment.


> +				int flag = disable_interrupts ();
>  				flash_write_cmd (info, sector, 0,
> -					FLASH_CMD_PROTECT_CLEAR);
> +						  FLASH_CMD_PROTECT);
> +				flash_write_cmd (info, sector, 0, cmd);
> +				/* re-enable interrupts if necessary */
> +				if (flag)
> +					enable_interrupts ();
> +			}
> +			}

Incorrect indentation.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
IMPORTANT NOTICE TO PURCHASERS: The Entire Physical Universe,  Inclu-
ding  This Product, May One Day Collapse Back into an Infinitesimally
Small Space. Should  Another  Universe  Subsequently  Re-emerge,  the
Existence of This Product in That Universe Cannot Be Guaranteed.

[U-Boot] [PATCHv2 CFI flash]: Workaround for Numonyx Axcell P33/P30 256-Mbit 65nm bug

[Philippe De Muyter]

Hello Wolfgang & list,

This is a revised patch, with comments and indentation fixed, I hope.

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 states :
When customer uses [...] block unlock, the block lock status might be
altered inadvertently. Lock status might be set to either 01h or 03h
unexpectedly (00h as expected data), which leads to program/erase failure
on certain blocks.

A working workaround is given, which I have applied and tested with success.

Signed-off-by: Philippe De Muyter <phdm@macqel.be>

---
 drivers/mtd/cfi_flash.c |   27 ++++++++++++++++++++-------
 1 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
index 3267c5d..4bf0807 100644
--- a/drivers/mtd/cfi_flash.c
+++ b/drivers/mtd/cfi_flash.c
@@ -1347,17 +1347,34 @@ int flash_real_protect (flash_info_t * info, long sector, int prot)
 	switch (info->vendor) {
 		case CFI_CMDSET_INTEL_PROG_REGIONS:
 		case CFI_CMDSET_INTEL_STANDARD:
-		case CFI_CMDSET_INTEL_EXTENDED:
-			flash_write_cmd (info, sector, 0,
-					 FLASH_CMD_CLEAR_STATUS);
-			flash_write_cmd (info, sector, 0, FLASH_CMD_PROTECT);
+		case CFI_CMDSET_INTEL_EXTENDED: {
+			unsigned short cmd;
+
 			if (prot)
-				flash_write_cmd (info, sector, 0,
-					FLASH_CMD_PROTECT_SET);
+				cmd = FLASH_CMD_PROTECT_SET;
 			else
+				cmd = FLASH_CMD_PROTECT_CLEAR;
+			/*
+			 * 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)) {
+				int flag = disable_interrupts ();
+				/*
+				 * cmd must come before FLASH_CMD_PROTECT + 20us
+				 * Disable interrupts which might cause a timeout here.
+				 */
 				flash_write_cmd (info, sector, 0,
-					FLASH_CMD_PROTECT_CLEAR);
+						  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:
 		case CFI_CMDSET_AMD_STANDARD:
 			/* U-Boot only checks the first byte */
-- 
1.6.3.3



-- 
Philippe De Muyter  phdm at macqel dot be  Tel +32 27029044
Macq Electronique SA  rue de l'Aeronef 2  B-1140 Bruxelles  Fax +32 27029077

[U-Boot] [PATCHv2 CFI flash]: Workaround for Numonyx Axcell P33/P30 256-Mbit 65nm bug

[Wolfgang Denk]

Dear Stefan,

In message <20100623131040.GA23209@frolo.macqel> Philippe De Muyter wrote:
> Hello Wolfgang & list,
> 
> This is a revised patch, with comments and indentation fixed, I hope.
> 
> 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 states :
> When customer uses [...] block unlock, the block lock status might be
> altered inadvertently. Lock status might be set to either 01h or 03h
> unexpectedly (00h as expected data), which leads to program/erase failure
> on certain blocks.
> 
> A working workaround is given, which I have applied and tested with success.
> 
> Signed-off-by: Philippe De Muyter <phdm@macqel.be>
> 
> ---
>  drivers/mtd/cfi_flash.c |   27 ++++++++++++++++++++-------
>  1 files changed, 20 insertions(+), 7 deletions(-)

I didn't see comments from you?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
There are no data that cannot be plotted on a straight  line  if  the
axis are chosen correctly.

[U-Boot] [PATCHv2 CFI flash]: Workaround for Numonyx Axcell P33/P30 256-Mbit 65nm bug

[Joakim Tjernlund]

>
> Dear Stefan,
>
> In message <20100623131040.GA23209@frolo.macqel> Philippe De Muyter wrote:
> > Hello Wolfgang & list,
> >
> > This is a revised patch, with comments and indentation fixed, I hope.
> >
> > 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 states :
> > When customer uses [...] block unlock, the block lock status might be
> > altered inadvertently. Lock status might be set to either 01h or 03h
> > unexpectedly (00h as expected data), which leads to program/erase failure
> > on certain blocks.
> >
> > A working workaround is given, which I have applied and tested with success.
> >
> > Signed-off-by: Philippe De Muyter <phdm@macqel.be>
> >
> > ---
> >  drivers/mtd/cfi_flash.c |   27 ++++++++++++++++++++-------
> >  1 files changed, 20 insertions(+), 7 deletions(-)
>
> I didn't see comments from you?
>
> Best regards,
>
> Wolfgang Denk

Doesn't the Linux kernel need the same fix? Would be great if
you(Philippe) could provide one. I too have such chips but I have
never seen this problem so I guess I have been lucky so far :)

      Jocke

[U-Boot] [PATCHv2 CFI flash]: Workaround for Numonyx Axcell P33/P30 256-Mbit 65nm bug

[Philippe De Muyter]

Hello Joakim,

On Mon, Aug 09, 2010 at 10:32:25AM +0200, Joakim Tjernlund wrote:
> >
> > Dear Stefan,
> >
> > In message <20100623131040.GA23209@frolo.macqel> Philippe De Muyter wrote:
> > > Hello Wolfgang & list,
> > >
> > > This is a revised patch, with comments and indentation fixed, I hope.
> > >
> > > 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 states :
> > > When customer uses [...] block unlock, the block lock status might be
> > > altered inadvertently. Lock status might be set to either 01h or 03h
> > > unexpectedly (00h as expected data), which leads to program/erase failure
> > > on certain blocks.
> > >
> > > A working workaround is given, which I have applied and tested with success.
> > >
> > > Signed-off-by: Philippe De Muyter <phdm@macqel.be>
> > >
> > > ---
> > >  drivers/mtd/cfi_flash.c |   27 ++++++++++++++++++++-------
> > >  1 files changed, 20 insertions(+), 7 deletions(-)
> >
> > I didn't see comments from you?
> >
> > Best regards,
> >
> > Wolfgang Denk
> 
> Doesn't the Linux kernel need the same fix? Would be great if
> you(Philippe) could provide one. I too have such chips but I have
> never seen this problem so I guess I have been lucky so far :)
> 
>       Jocke

I use linux on those boards, of course, so I also thought that I'd need
to fix linux, but I didn't have to.  On the boards I had, the bug was
easily repeatable with u-boot or the bdm-tools's bdm-based flash programmer
I used.  The same blocks always gave the same error, while other blocks
always worked perfectly.  From linux, I tested with eraseall from mtd-utils
also on the bad blocks and I never had any problem, so linux probably
already does the erase sequence the way numonyx testers do it.  The key
point is that the CFI unlock command sequence must come just before the
CFI erase command sequence, not long before.

Philippe

[U-Boot] [PATCHv2 CFI flash]: Workaround for Numonyx Axcell P33/P30 256-Mbit 65nm bug

[Philippe De Muyter]

On Mon, Aug 09, 2010 at 02:57:27PM +0200, Philippe De Muyter wrote:
> Hello Joakim,
> 
> On Mon, Aug 09, 2010 at 10:32:25AM +0200, Joakim Tjernlund wrote:
> > >
> > > Dear Stefan,
> > >
> > > In message <20100623131040.GA23209@frolo.macqel> Philippe De Muyter wrote:
> > > > Hello Wolfgang & list,
> > > >
> > > > This is a revised patch, with comments and indentation fixed, I hope.
> > > >
> > > > 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 states :
> > > > When customer uses [...] block unlock, the block lock status might be
> > > > altered inadvertently. Lock status might be set to either 01h or 03h
> > > > unexpectedly (00h as expected data), which leads to program/erase failure
> > > > on certain blocks.
> > > >
> > > > A working workaround is given, which I have applied and tested with success.
> > > >
> > > > Signed-off-by: Philippe De Muyter <phdm@macqel.be>
> > > >
> > > > ---
> > > >  drivers/mtd/cfi_flash.c |   27 ++++++++++++++++++++-------
> > > >  1 files changed, 20 insertions(+), 7 deletions(-)
> > >
> > > I didn't see comments from you?
> > >
> > > Best regards,
> > >
> > > Wolfgang Denk
> > 
> > Doesn't the Linux kernel need the same fix? Would be great if
> > you(Philippe) could provide one. I too have such chips but I have
> > never seen this problem so I guess I have been lucky so far :)
> > 
> >       Jocke
> 
> I use linux on those boards, of course, so I also thought that I'd need
> to fix linux, but I didn't have to.  On the boards I had, the bug was
> easily repeatable with u-boot or the bdm-tools's bdm-based flash programmer
> I used.  The same blocks always gave the same error, while other blocks
> always worked perfectly.  From linux, I tested with eraseall from mtd-utils
> also on the bad blocks and I never had any problem, so linux probably
> already does the erase sequence the way numonyx testers do it.  The key
> point is that the CFI unlock command sequence must come just before the
> CFI erase command sequence, not long before.

Sorry, bad memory :(
The unlock command must be preceded by a read lock command, with
no more than 20 us between.

> 
> Philippe

-- 
Philippe De Muyter  phdm at macqel dot be  Tel +32 27029044
Macq Electronique SA  rue de l'Aeronef 2  B-1140 Bruxelles  Fax +32 27029077

[U-Boot] [PATCHv2 CFI flash]: Workaround for Numonyx Axcell P33/P30 256-Mbit 65nm bug

[Joakim Tjernlund]

Philippe De Muyter <phdm@macqel.be> wrote on 2010/08/09 14:57:27:
>
> Hello Joakim,
>
> On Mon, Aug 09, 2010 at 10:32:25AM +0200, Joakim Tjernlund wrote:
> > >
> > > Dear Stefan,
> > >
> > > In message <20100623131040.GA23209@frolo.macqel> Philippe De Muyter wrote:
> > > > Hello Wolfgang & list,
> > > >
> > > > This is a revised patch, with comments and indentation fixed, I hope.
> > > >
> > > > 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 states :
> > > > When customer uses [...] block unlock, the block lock status might be
> > > > altered inadvertently. Lock status might be set to either 01h or 03h
> > > > unexpectedly (00h as expected data), which leads to program/erase failure
> > > > on certain blocks.
> > > >
> > > > A working workaround is given, which I have applied and tested with success.
> > > >
> > > > Signed-off-by: Philippe De Muyter <phdm@macqel.be>
> > > >
> > > > ---
> > > >  drivers/mtd/cfi_flash.c |   27 ++++++++++++++++++++-------
> > > >  1 files changed, 20 insertions(+), 7 deletions(-)
> > >
> > > I didn't see comments from you?
> > >
> > > Best regards,
> > >
> > > Wolfgang Denk
> >
> > Doesn't the Linux kernel need the same fix? Would be great if
> > you(Philippe) could provide one. I too have such chips but I have
> > never seen this problem so I guess I have been lucky so far :)
> >
> >       Jocke
>
> I use linux on those boards, of course, so I also thought that I'd need
> to fix linux, but I didn't have to.  On the boards I had, the bug was
> easily repeatable with u-boot or the bdm-tools's bdm-based flash programmer
> I used.  The same blocks always gave the same error, while other blocks
> always worked perfectly.  From linux, I tested with eraseall from mtd-utils
> also on the bad blocks and I never had any problem, so linux probably
> already does the erase sequence the way numonyx testers do it.  The key
> point is that the CFI unlock command sequence must come just before the
> CFI erase command sequence, not long before.

hmm, the problem isn't erase(as I understand it). It is the unlock/lock sequence
that does. I suspect that the auto unlock procedure in cfi_cmd0001 could suffer
from this problem. Do you use auto unlock? There is nothing that
protects the timing for unlock, is there?

       Jocke

[U-Boot] [PATCHv2 CFI flash]: Workaround for Numonyx Axcell P33/P30 256-Mbit 65nm bug

[Stefan Roese]

Hi Philippe,

first sorry for the late review (thanks Wolfgang for reminding).

On Saturday 07 August 2010 01:11:02 Wolfgang Denk wrote:
> Dear Stefan,
> 
> In message <20100623131040.GA23209@frolo.macqel> Philippe De Muyter wrote:
> > Hello Wolfgang & list,
> > 
> > This is a revised patch, with comments and indentation fixed, I hope.
> > 

Minor note: Such commentary lines should be moved below the "---" line
below. They shouldn't appear in the git history.

<snip>

> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
> index 3267c5d..4bf0807 100644
> --- a/drivers/mtd/cfi_flash.c
> +++ b/drivers/mtd/cfi_flash.c
> @@ -1347,17 +1347,34 @@ int flash_real_protect (flash_info_t * info, long
> sector, int prot)
> 
>         switch (info->vendor) {
>         
>                 case CFI_CMDSET_INTEL_PROG_REGIONS:
> 
>                 case CFI_CMDSET_INTEL_STANDARD:
> -               case CFI_CMDSET_INTEL_EXTENDED:
> -                       flash_write_cmd (info, sector, 0,
> -                                        FLASH_CMD_CLEAR_STATUS);
> -                       flash_write_cmd (info, sector, 0, FLASH_CMD_PROTECT);
> +               case CFI_CMDSET_INTEL_EXTENDED: {
> +                       unsigned short cmd;
> +
> 
>                         if (prot)
> 
> -                               flash_write_cmd (info, sector, 0,
> -                                       FLASH_CMD_PROTECT_SET);
> +                               cmd = FLASH_CMD_PROTECT_SET;
> 
>                         else
> 
> +                               cmd = FLASH_CMD_PROTECT_CLEAR;

Please move this variable declaration of "cmd" and assignment further down
(see below). You can remove the "{" "}" of this block then.

> +                       /*
> +                        * 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)) {
> +                               int flag = disable_interrupts ();

Declare and assign "cmd" here please.

Please change and resubmit. Thanks.

Cheers,
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] [PATCHv2 CFI flash]: Workaround for Numonyx Axcell P33/P30 256-Mbit 65nm bug

[Philippe De Muyter]

Hi Stefan,

On Mon, Aug 09, 2010 at 03:52:03PM +0200, Stefan Roese wrote:
> Hi Philippe,
> 
> first sorry for the late review (thanks Wolfgang for reminding).
> 
> On Saturday 07 August 2010 01:11:02 Wolfgang Denk wrote:
> > Dear Stefan,
> > 
> > In message <20100623131040.GA23209@frolo.macqel> Philippe De Muyter wrote:
> > > Hello Wolfgang & list,
> > > 
> > > This is a revised patch, with comments and indentation fixed, I hope.
> > > 
> 
> Minor note: Such commentary lines should be moved below the "---" line
> below. They shouldn't appear in the git history.
> 
> <snip>
> 
> > diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
> > index 3267c5d..4bf0807 100644
> > --- a/drivers/mtd/cfi_flash.c
> > +++ b/drivers/mtd/cfi_flash.c
> > @@ -1347,17 +1347,34 @@ int flash_real_protect (flash_info_t * info, long
> > sector, int prot)
> > 
> >         switch (info->vendor) {
> >         
> >                 case CFI_CMDSET_INTEL_PROG_REGIONS:
> > 
> >                 case CFI_CMDSET_INTEL_STANDARD:
> > -               case CFI_CMDSET_INTEL_EXTENDED:
> > -                       flash_write_cmd (info, sector, 0,
> > -                                        FLASH_CMD_CLEAR_STATUS);
> > -                       flash_write_cmd (info, sector, 0, FLASH_CMD_PROTECT);
> > +               case CFI_CMDSET_INTEL_EXTENDED: {
> > +                       unsigned short cmd;
> > +
> > 
> >                         if (prot)
> > 
> > -                               flash_write_cmd (info, sector, 0,
> > -                                       FLASH_CMD_PROTECT_SET);
> > +                               cmd = FLASH_CMD_PROTECT_SET;
> > 
> >                         else
> > 
> > +                               cmd = FLASH_CMD_PROTECT_CLEAR;
> 
> Please move this variable declaration of "cmd" and assignment further down
> (see below). You can remove the "{" "}" of this block then.
> 

Here is the excerpt from the errata :

Workaround:  If the interval between 60h and its subsequent command
	     can be guaranteed within 20?s, Option I is recommended,
	     otherwise Option II (involves hardware) should be selected.
Option I: The table below lists the detail command sequences:
Command
	      Data bus           Address bus       Remarks
Sequence
  1              90h            Block Address
						   Read Lock Status
  2             Read         Block Address + 02h
 (2)(3)                                      (1)
3                60h           Block Address
 (2)(3)                                      (1)   Lock/Unlock/RCR Configuration
4           D0h/01h/03h        Block Address
Notes:
(1) Block Address refers to RCR configuration data only when the 60h
    command sequence is used to set RCR register combined with 03h
    subsequent command.
(2) For the third and fourth command sequences, the Block Address must
    be the same.
(3) The interval between 60h command and its subsequent D0h/01h/2Fh/03h
    commands should be less than 20?s.

Because of requirement (3), I choosed to minimize the number of instructions
between the `read lock status' and the `unlock' commands, hence the
initialisation of `cmd' moved before the `read lock status' (hidden
in `flash_isequal').  

> > +                       /*
> > +                        * 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)) {
> > +                               int flag = disable_interrupts ();
> 
> Declare and assign "cmd" here please.
> 
> Please change and resubmit. Thanks.

If you still prefer it changed, speak :)

Best regards

Philippe

[U-Boot] [PATCHv2 CFI flash]: Workaround for Numonyx Axcell P33/P30 256-Mbit 65nm bug

[Stefan Roese]

Hi Philippe,

On Monday 09 August 2010 17:46:03 Philippe De Muyter wrote:
> > Please move this variable declaration of "cmd" and assignment further
> > down (see below). You can remove the "{" "}" of this block then.
> 
> Here is the excerpt from the errata :
> 
> Workaround:  If the interval between 60h and its subsequent command
> 	     can be guaranteed within 20?s, Option I is recommended,
> 	     otherwise Option II (involves hardware) should be selected.
> Option I: The table below lists the detail command sequences:
> Command
> 	      Data bus           Address bus       Remarks
> Sequence
>   1              90h            Block Address
> 						   Read Lock Status
>   2             Read         Block Address + 02h
>  (2)(3)                                      (1)
> 3                60h           Block Address
>  (2)(3)                                      (1)   Lock/Unlock/RCR
> Configuration 4           D0h/01h/03h        Block Address
> Notes:
> (1) Block Address refers to RCR configuration data only when the 60h
>     command sequence is used to set RCR register combined with 03h
>     subsequent command.
> (2) For the third and fourth command sequences, the Block Address must
>     be the same.
> (3) The interval between 60h command and its subsequent D0h/01h/2Fh/03h
>     commands should be less than 20?s.
> 
> Because of requirement (3), I choosed to minimize the number of
> instructions between the `read lock status' and the `unlock' commands,
> hence the initialisation of `cmd' moved before the `read lock status'
> (hidden in `flash_isequal').

From my understanding, "only" the last 2 operations need to be in max.
20?s interval. If this is the case, then I would prefer this code
version:

		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)) {
				int flag = disable_interrupts ();
				unsigned short cmd;

				if (prot)
					cmd = FLASH_CMD_PROTECT_SET;
				else
					cmd = FLASH_CMD_PROTECT_CLEAR;

				/*
				 * cmd must come before FLASH_CMD_PROTECT + 20us
				 * Disable interrupts which might cause a timeout here.
				 */
				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 ();
			}

> > > +                       /*
> > > +                        * 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)) {
> > > +                               int flag = disable_interrupts ();
> > 
> > Declare and assign "cmd" here please.
> > 
> > Please change and resubmit. Thanks.
> 
> If you still prefer it changed, speak :)

Yes, please let me know if this patch version also fixes the bug. If
this is the case, I would prefer that you change it accordingly.

Thanks.

Cheers,
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] [PATCHv2 CFI flash]: Workaround for Numonyx Axcell P33/P30 256-Mbit 65nm bug

[Philippe De Muyter]

Hi Stefan,

On Tue, Aug 10, 2010 at 01:20:42PM +0200, Stefan Roese wrote:
> Hi Philippe,
> 
> On Monday 09 August 2010 17:46:03 Philippe De Muyter wrote:
> > > Please move this variable declaration of "cmd" and assignment further
> > > down (see below). You can remove the "{" "}" of this block then.
> > 
> > Here is the excerpt from the errata :
> > 
> > Workaround:  If the interval between 60h and its subsequent command
> > 	     can be guaranteed within 20?s, Option I is recommended,
> > 	     otherwise Option II (involves hardware) should be selected.
> > Option I: The table below lists the detail command sequences:
> > Command
> > 	      Data bus           Address bus       Remarks
> > Sequence
> >   1              90h            Block Address
> > 						   Read Lock Status
> >   2             Read         Block Address + 02h
> >  (2)(3)                                      (1)
> > 3                60h           Block Address
> >  (2)(3)                                      (1)   Lock/Unlock/RCR
> > Configuration 4           D0h/01h/03h        Block Address
> > Notes:
> > (1) Block Address refers to RCR configuration data only when the 60h
> >     command sequence is used to set RCR register combined with 03h
> >     subsequent command.
> > (2) For the third and fourth command sequences, the Block Address must
> >     be the same.
> > (3) The interval between 60h command and its subsequent D0h/01h/2Fh/03h
> >     commands should be less than 20?s.
> > 
> > Because of requirement (3), I choosed to minimize the number of
> > instructions between the `read lock status' and the `unlock' commands,
> > hence the initialisation of `cmd' moved before the `read lock status'
> > (hidden in `flash_isequal').

Yes, you're right.  I had misread the doc :(

> 
> From my understanding, "only" the last 2 operations need to be in max.
> 20?s interval. If this is the case, then I would prefer this code
> version:

I prefer it too.

> 
> 		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)) {
> 				int flag = disable_interrupts ();
> 				unsigned short cmd;
> 
> 				if (prot)
> 					cmd = FLASH_CMD_PROTECT_SET;
> 				else
> 					cmd = FLASH_CMD_PROTECT_CLEAR;
> 
> 				/*
> 				 * cmd must come before FLASH_CMD_PROTECT + 20us
> 				 * Disable interrupts which might cause a timeout here.
> 				 */

Should the above comment not stay closer to the disable_interrupts () call ?

> 				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 ();
> 			}
> 
> > > > +                       /*
> > > > +                        * 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)) {
> > > > +                               int flag = disable_interrupts ();
> > > 
> > > Declare and assign "cmd" here please.
> > > 
> > > Please change and resubmit. Thanks.
> > 
> > If you still prefer it changed, speak :)
> 
> Yes, please let me know if this patch version also fixes the bug. If
> this is the case, I would prefer that you change it accordingly.

I surmise it does, but I currently do not have a board to test.  I'll
get new boards (with the same bug) soon, and I'll let you know then.

Best regards

Philippe

-- 
Philippe De Muyter  phdm at macqel dot be  Tel +32 27029044
Macq Electronique SA  rue de l'Aeronef 2  B-1140 Bruxelles  Fax +32 27029077

[U-Boot] [PATCHv2 CFI flash]: Workaround for Numonyx Axcell P33/P30 256-Mbit 65nm bug

[Stefan Roese]

Hi Philippe,

On Tuesday 10 August 2010 15:31:12 Philippe De Muyter wrote:
> > > Because of requirement (3), I choosed to minimize the number of
> > > instructions between the `read lock status' and the `unlock' commands,
> > > hence the initialisation of `cmd' moved before the `read lock status'
> > > (hidden in `flash_isequal').
> 
> Yes, you're right.  I had misread the doc :(
> 
> > From my understanding, "only" the last 2 operations need to be in max.
> > 20?s interval. If this is the case, then I would prefer this code
> > version:
>
> I prefer it too.

Good! :)
 
> > 		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)) {
> > 				
> > 				int flag = disable_interrupts ();
> > 				unsigned short cmd;
> > 				
> > 				if (prot)
> > 				
> > 					cmd = FLASH_CMD_PROTECT_SET;
> > 				
> > 				else
> > 				
> > 					cmd = FLASH_CMD_PROTECT_CLEAR;
> > 				
> > 				/*
> > 				 * cmd must come before FLASH_CMD_PROTECT + 20us
> > 				 * Disable interrupts which might cause a timeout here.
> > 				 */
> 
> Should the above comment not stay closer to the disable_interrupts () call?

Perhaps yes. But a comment before the following 2 instructions would also
be good. Because this is the "hot path" that needs to be protected against
interrupts.
 
> > Yes, please let me know if this patch version also fixes the bug. If
> > this is the case, I would prefer that you change it accordingly.
> 
> I surmise it does, but I currently do not have a board to test.  I'll
> get new boards (with the same bug) soon, and I'll let you know then.

Thanks.

Cheers,
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] [PATCHv2 CFI flash]: Workaround for Numonyx Axcell P33/P30 256-Mbit 65nm bug

[Stefan Roese]

Hi Philippe,

one more thing:

On Monday 09 August 2010 17:46:03 Philippe De Muyter wrote:
> Here is the excerpt from the errata :
> 
> Workaround:  If the interval between 60h and its subsequent command
> 	     can be guaranteed within 20?s, Option I is recommended,
> 	     otherwise Option II (involves hardware) should be selected.
> Option I: The table below lists the detail command sequences:
> Command
> 	      Data bus           Address bus       Remarks
> Sequence
>   1              90h            Block Address
> 						   Read Lock Status
>   2             Read         Block Address + 02h
>  (2)(3)                                      (1)
> 3                60h           Block Address
>  (2)(3)                                      (1)   Lock/Unlock/RCR
> Configuration 4           D0h/01h/03h        Block Address
> Notes:
> (1) Block Address refers to RCR configuration data only when the 60h
>     command sequence is used to set RCR register combined with 03h
>     subsequent command.
> (2) For the third and fourth command sequences, the Block Address must
>     be the same.
> (3) The interval between 60h command and its subsequent D0h/01h/2Fh/03h
>     commands should be less than 20?s.

Please add this workaround text from the errata to the commit text in your 
next patch version. This makes things easier - no need to search for the 
errata.

Thanks.
 
Cheers,
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] [PATCHv2 CFI flash]: Workaround for Numonyx Axcell?P33/P30 256-Mbit 65nm bug

[Philippe De Muyter]

On Tue, Aug 10, 2010 at 05:28:59PM +0200, Stefan Roese wrote:
> Hi Philippe,
> 
> one more thing:
> 
> On Monday 09 August 2010 17:46:03 Philippe De Muyter wrote:
> > Here is the excerpt from the errata :
> > 
> > Workaround:  If the interval between 60h and its subsequent command
> > 	     can be guaranteed within 20?s, Option I is recommended,
> > 	     otherwise Option II (involves hardware) should be selected.
> > Option I: The table below lists the detail command sequences:
> > Command
> > 	      Data bus           Address bus       Remarks
> > Sequence
> >   1              90h            Block Address
> > 						   Read Lock Status
> >   2             Read         Block Address + 02h
> >  (2)(3)                                      (1)
> > 3                60h           Block Address
> >  (2)(3)                                      (1)   Lock/Unlock/RCR
> > Configuration 4           D0h/01h/03h        Block Address
> > Notes:
> > (1) Block Address refers to RCR configuration data only when the 60h
> >     command sequence is used to set RCR register combined with 03h
> >     subsequent command.
> > (2) For the third and fourth command sequences, the Block Address must
> >     be the same.
> > (3) The interval between 60h command and its subsequent D0h/01h/2Fh/03h
> >     commands should be less than 20?s.
> 
> Please add this workaround text from the errata to the commit text in your 
> next patch version. This makes things easier - no need to search for the 
> errata.

OK

Philippe