[PATCH] Wait for SPI WRSR

[Carl-Daniel Hailfinger]

Bugfix, 0.9.3 candidate.

SPI write status register (WRSR) may take longer than 100 ms, and it
makes sense to poll for completion in 10 ms steps.
This patch complements r1115.

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-wrsr_delay/spi25.c
===================================================================
--- flashrom-wrsr_delay/spi25.c	(Revision 1184)
+++ flashrom-wrsr_delay/spi25.c	(Arbeitskopie)
@@ -880,8 +880,14 @@
    	msg_cerr("%s failed during command execution\n",
    		__func__);
    }
-	/* WRSR performs a self-timed erase before the changes take effect. */
+	/* WRSR performs a self-timed erase before the changes take effect.
+	 * This may take 50-85 ms in most cases, and some chips apparently
+	 * allow running RDSR only once. Pick an initial delay of 100 ms,
+	 * then wait in 10 ms steps.
+	 */
    programmer_delay(100 * 1000);
+	while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
+		programmer_delay(10 * 1000);
    return result;
 }

@@ -912,8 +918,14 @@
    	msg_cerr("%s failed during command execution\n",
    		__func__);
    }
-	/* WRSR performs a self-timed erase before the changes take effect. */
+	/* WRSR performs a self-timed erase before the changes take effect.
+	 * This may take 50-85 ms in most cases, and some chips apparently
+	 * allow running RDSR only once. Pick an initial delay of 100 ms,
+	 * then wait in 10 ms steps.
+	 */
    programmer_delay(100 * 1000);
+	while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
+		programmer_delay(10 * 1000);
    return result;
 }

[Joshua Roys]

On 10/04/2010 08:58 AM, Carl-Daniel Hailfinger wrote:
> Bugfix, 0.9.3 candidate.
>
> SPI write status register (WRSR) may take longer than 100 ms, and it
> makes sense to poll for completion in 10 ms steps.
> This patch complements r1115.
>
> Signed-off-by: Carl-Daniel Hailfinger
> * This may take 50-85 ms in most cases, and some chips apparently
> * allow running RDSR only once. Pick an initial delay of 100 ms,
> * then wait in 10 ms steps.
> */
> programmer_delay(100 * 1000);
> while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
> programmer_delay(10 * 1000);

I might say "So, pick an initial delay..." or "Therefore pick an initial
delay..." to make it a little clearer.  With or without,

Acked-by: Joshua Roys

[Carl-Daniel Hailfinger]

On 07.10.2010 23:59, Joshua Roys wrote:
> On 10/04/2010 08:58 AM, Carl-Daniel Hailfinger wrote:
> > Bugfix, 0.9.3 candidate.
> >
> > SPI write status register (WRSR) may take longer than 100 ms, and it
> > makes sense to poll for completion in 10 ms steps.
> > This patch complements r1115.
> >
> > Signed-off-by: Carl-Daniel Hailfinger
> > * This may take 50-85 ms in most cases, and some chips apparently
> > * allow running RDSR only once. Pick an initial delay of 100 ms,
> > * then wait in 10 ms steps.
>
> I might say "So, pick an initial delay..." or "Therefore pick an initial
> delay..." to make it a little clearer.  With or without,
>
> Acked-by: Joshua Roys

SPI write status register (WRSR) may take longer than 100 ms, and it
makes sense to poll for completion in 10 ms steps until 5 s are over.
This patch complements r1115.

Josh, I reused your ack. Any objections?

Signed-off-by: Carl-Daniel Hailfinger 
Acked-by: Joshua Roys

Index: flashrom-wrsr_delay/flash.h
===================================================================
--- flashrom-wrsr_delay/flash.h	(Revision 1200)
+++ flashrom-wrsr_delay/flash.h	(Arbeitskopie)
@@ -35,6 +35,9 @@

#define ERROR_PTR ((void*)-1)

+/* Error codes */
+#define TIMEOUT_ERROR	-101
+
 typedef unsigned long chipaddr;

int register_shutdown(void (*function) (void *data), void *data);
Index: flashrom-wrsr_delay/spi25.c
===================================================================
--- flashrom-wrsr_delay/spi25.c	(Revision 1200)
+++ flashrom-wrsr_delay/spi25.c	(Arbeitskopie)
@@ -856,6 +856,7 @@
 static int spi_write_status_register_ewsr(struct flashchip *flash, int status)
 {
    int result;
+	int i = 0;
    struct spi_command cmds[] = {
    {
    /* WRSR requires either EWSR or WREN depending on chip type. */
@@ -879,15 +880,31 @@
    if (result) {
    	msg_cerr("%s failed during command execution\n",
    		__func__);
+		/* No point in waiting for the command to complete if execution
+		 * failed.
+		 */
+		return result;
    }
-	/* WRSR performs a self-timed erase before the changes take effect. */
+	/* WRSR performs a self-timed erase before the changes take effect.
+	 * This may take 50-85 ms in most cases, and some chips apparently
+	 * allow running RDSR only once. Therefore pick an initial delay of
+	 * 100 ms, then wait in 10 ms steps until a total of 5 s have elapsed.
+	 */
    programmer_delay(100 * 1000);
-	return result;
+	while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP) {
+		if (++i > 490) {
+			msg_cerr("Error: WIP bit after WRSR never cleared\n");
+			return TIMEOUT_ERROR;
+		}
+		programmer_delay(10 * 1000);
+	}
+	return 0;
 }

static int spi_write_status_register_wren(struct flashchip *flash, int status)
 {
    int result;
+	int i = 0;
    struct spi_command cmds[] = {
    {
    /* WRSR requires either EWSR or WREN depending on chip type. */
@@ -911,10 +928,25 @@
    if (result) {
    	msg_cerr("%s failed during command execution\n",
    		__func__);
+		/* No point in waiting for the command to complete if execution
+		 * failed.
+		 */
+		return result;
    }
-	/* WRSR performs a self-timed erase before the changes take effect. */
+	/* WRSR performs a self-timed erase before the changes take effect.
+	 * This may take 50-85 ms in most cases, and some chips apparently
+	 * allow running RDSR only once. Therefore pick an initial delay of
+	 * 100 ms, then wait in 10 ms steps until a total of 5 s have elapsed.
+	 */
    programmer_delay(100 * 1000);
-	return result;
+	while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP) {
+		if (++i > 490) {
+			msg_cerr("Error: WIP bit after WRSR never cleared\n");
+			return TIMEOUT_ERROR;
+		}
+		programmer_delay(10 * 1000);
+	}
+	return 0;
 }

static int spi_write_status_register(struct flashchip *flash, int status)

[Carl-Daniel Hailfinger]

On 08.10.2010 00:36, Carl-Daniel Hailfinger wrote:
> SPI write status register (WRSR) may take longer than 100 ms, and it
> makes sense to poll for completion in 10 ms steps until 5 s are over.
> This patch complements r1115.
>
> Josh, I reused your ack. Any objections?
>
> Signed-off-by: Carl-Daniel Hailfinger 
> Acked-by: Joshua Roys

Thanks for the review.
Michael Karcher gave a thumbs up on IRC for the modified version.

Committed in r1201.

Regards,
Carl-Daniel