[PATCH] Debug excessive toggle bit delay

[Carl-Daniel Hailfinger]

Today's systems are quite capable of running 2^32 reads in short time.
Some chips may be slow enough to fall off the loop here. This patch is
an attempt at debugging a problem we're seeing on the Winbond W39V040C
where the standard toggle bit algorithm seems to fail for block erases.

Note: This probably needs a few tweaks before it can be committed.

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-detect_excessive_toggle/jedec.c
===================================================================
--- flashrom-detect_excessive_toggle/jedec.c	(Revision 803)
+++ flashrom-detect_excessive_toggle/jedec.c	(Arbeitskopie)
@@ -33,7 +33,7 @@
    return (val ^ (val >> 1)) & 0x1;
 }

-void toggle_ready_jedec(chipaddr dst)
+void toggle_ready_jedec_common(chipaddr dst, int delay)
 {
    unsigned int i = 0;
    uint8_t tmp1, tmp2;
@@ -41,14 +41,31 @@
    tmp1 = chip_readb(dst) & 0x40;

while (i++ < 0xFFFFFFF) {
+		if (delay)
+			programmer_delay(delay);
    	tmp2 = chip_readb(dst) & 0x40;
    	if (tmp1 == tmp2) {
    		break;
    	}
    	tmp1 = tmp2;
    }
+	if (i > 0x10000)
+		printf_debug("%s: excessive toggle, i=0x%x\n", __func__, i);
 }

+void toggle_ready_jedec(chipaddr dst)
+{
+	toggle_ready_jedec_common(dst, 0);
+}
+
+/* Some chips require a minimum delay between toggle bit reads.
+ * The Winbond W39V040C wants 50 ms between reads on sector erase toggle.
+ */
+void toggle_ready_jedec_slow(chipaddr dst)
+{
+	toggle_ready_jedec_common(dst, 50 * 1000);
+}
+
 void data_polling_jedec(chipaddr dst, uint8_t data)
 {
    unsigned int i = 0;
@@ -62,6 +79,8 @@
    		break;
    	}
    }
+	if (i > 0x10000)
+		printf_debug("%s: excessive data poll, i=0x%x\n", __func__, i);
 }

void start_program_jedec(chipaddr bios)

[Carl-Daniel Hailfinger]

On 16.12.2009 01:08, Carl-Daniel Hailfinger wrote:
> Today's systems are quite capable of running 2^32 reads in short time.
> Some chips may be slow enough to fall off the loop here. This patch is
> an attempt at debugging a problem we're seeing on the Winbond W39V040C
> where the standard toggle bit algorithm seems to fail for block erases.
>
> Note: This probably needs a few tweaks before it can be committed.

Next try. Use slow toggle for all erase stuff.

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-detect_excessive_toggle/jedec.c
===================================================================
--- flashrom-detect_excessive_toggle/jedec.c	(Revision 803)
+++ flashrom-detect_excessive_toggle/jedec.c	(Arbeitskopie)
@@ -33,7 +33,7 @@
    return (val ^ (val >> 1)) & 0x1;
 }

-void toggle_ready_jedec(chipaddr dst)
+void toggle_ready_jedec_common(chipaddr dst, int delay)
 {
    unsigned int i = 0;
    uint8_t tmp1, tmp2;
@@ -41,14 +41,31 @@
    tmp1 = chip_readb(dst) & 0x40;

while (i++ < 0xFFFFFFF) {
+		if (delay)
+			programmer_delay(delay);
    	tmp2 = chip_readb(dst) & 0x40;
    	if (tmp1 == tmp2) {
    		break;
    	}
    	tmp1 = tmp2;
    }
+	if (i > 0x10000)
+		printf_debug("%s: excessive toggle, i=0x%x\n", __func__, i);
 }

+void toggle_ready_jedec(chipaddr dst)
+{
+	toggle_ready_jedec_common(dst, 0);
+}
+
+/* Some chips require a minimum delay between toggle bit reads.
+ * The Winbond W39V040C wants 50 ms between reads on sector erase toggle.
+ */
+void toggle_ready_jedec_slow(chipaddr dst)
+{
+	toggle_ready_jedec_common(dst, 50 * 1000);
+}
+
 void data_polling_jedec(chipaddr dst, uint8_t data)
 {
    unsigned int i = 0;
@@ -62,6 +79,8 @@
    		break;
    	}
    }
+	if (i > 0x10000)
+		printf_debug("%s: excessive data poll, i=0x%x\n", __func__, i);
 }

void start_program_jedec(chipaddr bios)
@@ -178,7 +197,7 @@
    programmer_delay(10);

/* wait for Toggle bit ready         */
-	toggle_ready_jedec(bios);
+	toggle_ready_jedec_slow(bios);

if (check_erased_range(flash, page, pagesize)) {
    	fprintf(stderr,"ERASE FAILED!\n");
@@ -207,7 +226,7 @@
    programmer_delay(10);

/* wait for Toggle bit ready         */
-	toggle_ready_jedec(bios);
+	toggle_ready_jedec_slow(bios);

if (check_erased_range(flash, block, blocksize)) {
    	fprintf(stderr,"ERASE FAILED!\n");
@@ -236,7 +255,7 @@
    chip_writeb(0x10, bios + 0x5555);
    programmer_delay(10);

-	toggle_ready_jedec(bios);
+	toggle_ready_jedec_slow(bios);

if (check_erased_range(flash, 0, total_size)) {
    	fprintf(stderr,"ERASE FAILED!\n");

[PATCH] Use slow toggle for erase, debug excessive delay

[Carl-Daniel Hailfinger]

Today's systems are quite capable of running 2^32 reads in short time.
Some chips may be slow enough to fall off the loop here. Other chips may
be slow enough to cause delays of more than 2^20 read cycles.

If the JEDEC Toggle Bit algorithm needs more than 2^20 loops, it is a
good sign we should have used delays between toggle bit reads.

The Winbond W39V040C requires a 50 ms delay between toggle bit reads
during erase according to the datasheet. Turns out a 2 ms delay is
sufficient. Use a safety factor of 4 and default all erase operations to
8 ms delay between toggle reads. This is short enough not to have a
substantial negative impact on erase times, and should improve reliability.

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-detect_excessive_toggle/jedec.c
===================================================================
--- flashrom-detect_excessive_toggle/jedec.c	(Revision 803)
+++ flashrom-detect_excessive_toggle/jedec.c	(Arbeitskopie)
@@ -33,7 +33,7 @@
    return (val ^ (val >> 1)) & 0x1;
 }

-void toggle_ready_jedec(chipaddr dst)
+void toggle_ready_jedec_common(chipaddr dst, int delay)
 {
    unsigned int i = 0;
    uint8_t tmp1, tmp2;
@@ -41,14 +41,35 @@
    tmp1 = chip_readb(dst) & 0x40;

while (i++ < 0xFFFFFFF) {
+		if (delay)
+			programmer_delay(delay);
    	tmp2 = chip_readb(dst) & 0x40;
    	if (tmp1 == tmp2) {
    		break;
    	}
    	tmp1 = tmp2;
    }
+	if (i > 0x100000)
+		printf_debug("%s: excessive loops, i=0x%x\n", __func__, i);
 }

+void toggle_ready_jedec(chipaddr dst)
+{
+	toggle_ready_jedec_common(dst, 0);
+}
+
+/* Some chips require a minimum delay between toggle bit reads.
+ * The Winbond W39V040C wants 50 ms between reads on sector erase toggle,
+ * but experiments show that 2 ms are already enough. Pick a safety factor
+ * of 4 and use an 8 ms delay.
+ * Given that erase is slow on all chips, it is recommended to use 
+ * toggle_ready_jedec_slow in erase functions.
+ */
+void toggle_ready_jedec_slow(chipaddr dst)
+{
+	toggle_ready_jedec_common(dst, 8 * 1000);
+}
+
 void data_polling_jedec(chipaddr dst, uint8_t data)
 {
    unsigned int i = 0;
@@ -62,6 +83,8 @@
    		break;
    	}
    }
+	if (i > 0x100000)
+		printf_debug("%s: excessive loops, i=0x%x\n", __func__, i);
 }

void start_program_jedec(chipaddr bios)
@@ -178,7 +201,7 @@
    programmer_delay(10);

/* wait for Toggle bit ready         */
-	toggle_ready_jedec(bios);
+	toggle_ready_jedec_slow(bios);

if (check_erased_range(flash, page, pagesize)) {
    	fprintf(stderr,"ERASE FAILED!\n");
@@ -207,7 +230,7 @@
    programmer_delay(10);

/* wait for Toggle bit ready         */
-	toggle_ready_jedec(bios);
+	toggle_ready_jedec_slow(bios);

if (check_erased_range(flash, block, blocksize)) {
    	fprintf(stderr,"ERASE FAILED!\n");
@@ -236,7 +259,7 @@
    chip_writeb(0x10, bios + 0x5555);
    programmer_delay(10);

-	toggle_ready_jedec(bios);
+	toggle_ready_jedec_slow(bios);

if (check_erased_range(flash, 0, total_size)) {
    	fprintf(stderr,"ERASE FAILED!\n");

[PATCH] Use slow toggle for erase, debug excessive delay

[Javier Ortega Conde (Malkavian)]

¡Hello!:

As I have been asked by Carl-Daniel I send this email...

Tested on "Asus M2N-E" mainboard with Winbond W39V040C
Acked-by: Javier Ortega Conde (aka Malkavian) <malkavian666 AT gmail DOT 
com>

My rom BIOS was fucked but with the inestimable help of Carl-Daniel at irc 
channel, he wrote a patch using my logs and now my mainboard is safe 
(well, I haven't rebooted yet, but seems safe).

[PATCH] Use slow toggle for erase, debug excessive delay

[Carl-Daniel Hailfinger]

Hi Javier,

On 16.12.2009 03:04, Javier Ortega Conde (Malkavian) wrote:
> As I have been asked by Carl-Daniel I send this email...
>
> Tested on "Asus M2N-E" mainboard with Winbond W39V040C
> Acked-by: Javier Ortega Conde (aka Malkavian) <malkavian666 AT gmail DOT 
> com>

Thanks!

If you enter the flashrom directory and run
svn revert -R .
svn up
you should have a flashrom tree which works fine for you.
> My rom BIOS was fucked but with the inestimable help of Carl-Daniel at irc 
> channel, he wrote a patch using my logs and now my mainboard is safe 
> (well, I haven't rebooted yet, but seems safe).

Yes, should be safe now.

Regards,
Carl-Daniel

[PATCH] Use slow toggle for erase, debug excessive delay

[Michael Karcher]

Am Mittwoch, den 16.12.2009, 02:26 +0100 schrieb Carl-Daniel Hailfinger:
> Today's systems are quite capable of running 2^32 reads in short time.
> Some chips may be slow enough to fall off the loop here. Other chips may
> be slow enough to cause delays of more than 2^20 read cycles.
>
> If the JEDEC Toggle Bit algorithm needs more than 2^20 loops, it is a
> good sign we should have used delays between toggle bit reads.
>
> The Winbond W39V040C requires a 50 ms delay between toggle bit reads
> during erase according to the datasheet. Turns out a 2 ms delay is
> sufficient. Use a safety factor of 4 and default all erase operations to
> 8 ms delay between toggle reads. This is short enough not to have a
> substantial negative impact on erase times, and should improve reliability.
>
> Signed-off-by: Carl-Daniel Hailfinger

While your patch looks OK, I somehow dislike your comment. The main
problem addressed is not falling out of the loop, but the Winbond chip
being slow on toggling, so that you read two times the same value of the
toggle bit while the operation is still in progress, unless you wait for
some ms between reads. Looks like the toggling of the toggle bit is
implemented in "software" in that chip instead of being implemented in
hardware driven by the read instructions.

2^20 iterations should be around 1 second on ISA, and i doubt that LPC
or FWH are much faster. For LPC, 15 LPC clocks are needed for
read/write. As an example, I checked the maximum LPC clock of the
W39V040C which is 33MHz. So 2^20 iterations still are 0.5 seconds.

Still, as the code is OK:
Acked-By: Michael Karcher

Regards,
  Michael Karcher

[PATCH] Use slow toggle for erase, debug excessive delay

[Carl-Daniel Hailfinger]

On 16.12.2009 12:53, Michael Karcher wrote:
> Am Mittwoch, den 16.12.2009, 02:26 +0100 schrieb Carl-Daniel Hailfinger:
> > The Winbond W39V040C requires a 50 ms delay between toggle bit reads
> > during erase according to the datasheet. Turns out a 2 ms delay is
> > sufficient. Use a safety factor of 4 and default all erase operations to
> > 8 ms delay between toggle reads. This is short enough not to have a
> > substantial negative impact on erase times, and should improve reliability.
>
> While your patch looks OK, I somehow dislike your comment.

I have reworked the changelog to address your concerns. Thanks for the
detailed review.
> Acked-By: Michael Karcher

Thanks, r807.

Regards,
Carl-Daniel