About flash chip writing/erasing, reading chip status.

I'm trying to place MTD and JFFS2 on our board, and I'm getting occasional messages like "Warning: DQ5 raised while erase operation was in progress, but erase completed OK" messages.

I think the general method for polling the status of the chips is just a bit wrong, or in other words, this warning is NORMAL operation and should not be printed.

If I understand things correctly, you start an operation like erasing a block, then you get the status by performing a read to that block. If the operation is in progress, data bit 6 toggles, and data bit 5 stays 0. If the operation succeeds, data bits 6 and 5 return the data at that address (bit 6 stops toggling). If the operation fails, bit 6 keeps toggling and bit 5 goes to 1.

The general algorithm used in mtd (recent CVS, cfi_cmdset_0002.c,v 1.61 2002/11/17 13:10:34 spse) goes something like this (timeout condition left out of the loop for brevity)

------------------------------------------------------------

	/* oldstatus and status are two reads from the same address */
	/* in each place it occurs in this code */

	read oldstatus;
	read status;

	while ( ((status & bit6) != (oldstatus & bit6))
		&& ((status & bit5) == 0) )
	{
		read oldstatus;
		read status;
	}

	if ( (status & bit6) != (oldstatus & bit6) )
	{
		/* still toggling, but exited loop. Why? */

		if ( status & bit5 != 0 )
		{
			/* bit5 went high. */
			/* are we still toggling? */
			read oldstatus;
			read status;
			
			if (status == oldstatus)
			{
				/* no longer toggling, operation done */
				printk("Warning: DQ5 raised while ...") ;
			}	
			else
			{
				/* DQ5 went active, reset the chip */
				...
			}
		}

	}

------------------------------------------------------------

[ In the real code, there are some extra reads in the while loop that make the condition I'm about to describe less likely, but it can still happen. ]

Let's say we're looking at an erase operation (the one I seem to run into). Say we manage to just get lucky, and within the while loop, the read of oldstatus returns the last status register value, and status gets the actual data because the chip has returned to read mode. Status's value will be 0xFF, the erased value. Furthermore, suppose we get lucky and the toggle bit's value in the read of oldstatus was a '0' (so in binary, oldstatus would have been 'x00x-xxxx'). So the toggle bit is seen as still toggling in the while loop, but bit5 suddenly goes from 0 to 1.

This will end up printing the warning message erroneously.

I think the "correct" thing to do would be to examine bit 5 from oldstatus, so you're only looking at bit 5 from an earlier read, where you know bit 6 has since toggled. Like this:

	read oldstatus;
	read status;

	while ( ((status & bit6) != (oldstatus & bit6))
		&& ((oldstatus & bit5) == 0) )
	{
		read oldstatus;
		read status;
	}

	if ( (status & bit6) != (oldstatus & bit6) )
	{
		/* still toggling, but exited loop. Why? */

		if ( oldstatus & bit5 != 0 )
		{
			/* bit5 went high. */
			/* NOTE: NO NEED TO CHECK IF WE'RE STILL TOGGLING. */
			/* we know we are, because we checked bit5 in the */
			/* previous read, and a read after it had bit6 */
			/* different. */
				/* DQ5 went active, reset the chip */
				...
		}

	}

I also think there's another error in the original code. The original code sets

dq5 = CMD(1<<5);

which, if I understand things, sets up a word the correct width, with a bit in EACH bit 5 position if you have interleaved chips. Yet the check for bit 5 being set (not being set in this case) is stated as

	((status & dq5) != dq5)

which will only trigger when ALL bit5's are set. I think it should instead be

	((status & dq5) == 0)

which triggers when ANY bit5 gets set. Similarly, tests for

	((status & dq5) == dq5)

should instead be

	((status & dq5) != 0)

I've attached a patch file that changes both these things. Please let me know what you think.

--> Steve Wahl, Brecis Communications.

cfi_cmdset_0002.c renamed from cfi_cmdset_0002.c.orig +36 -45

@@ -520,7 +520,7 @@
 	status = cfi_read(map, adr);
 
 	while( (status & dq6) != (oldstatus & dq6) && 
-	       (status & dq5) != dq5 &&
+	       (oldstatus & dq5) == 0 &&
 	       !time_after(jiffies, timeo) ) {
 
 		if (need_resched()) {

@@ -536,20 +536,17 @@
 
 	if( (status & dq6) != (oldstatus & dq6) ) {
 		/* The erasing didn't stop?? */
-		if( (status & dq5) == dq5 ) {
-			/* When DQ5 raises, we must check once again
-			   if DQ6 is toggling.  If not, the erase has been
-			   completed OK.  If not, reset chip. */
-			oldstatus = cfi_read(map, adr);
-			status = cfi_read(map, adr);
-		    
-			if ( (oldstatus & 0x00FF) == (status & 0x00FF) ) {
-				printk(KERN_WARNING "Warning: DQ5 raised while program operation was in progress, however operation completed OK\n" );
-			} else { 
-				/* DQ5 is active so we can do a reset and stop the erase */
-				cfi_write(map, CMD(0xF0), chip->start);
-				printk(KERN_WARNING "Internal flash device timeout occurred or write operation was performed while flash was programming.\n" );
-			}
+		if( (status & dq5) != 0 ) {
+
+			/* one or more bit5's set. */
+
+			/* no need to re-check if bit 6 is toggling, as */
+			/* we've already seen bit 6 toggle after bit 5 */
+			/* was set. */
+			
+			/* DQ5 is active so we can do a reset and stop the erase */
+			cfi_write(map, CMD(0xF0), chip->start);
+			printk(KERN_WARNING "Internal flash device timeout occurred or write operation was performed while flash was programming.\n" );
 		} else {
 			printk(KERN_WARNING "Waiting for write to complete timed out in do_write_oneword.");        
 

@@ -773,7 +770,7 @@
 	oldstatus = cfi_read(map, adr);
 	status = cfi_read(map, adr);
 	while( ((status & dq6) != (oldstatus & dq6)) && 
-		((status & dq5) != dq5) &&
+		((oldstatus & dq5) == 0) &&
 		!time_after(jiffies, timeo)) {
 		int wait_reps;
 

@@ -805,7 +802,7 @@
 		for(wait_reps = 0;
 		    	(wait_reps < 100) &&
 			((status & dq6) != (oldstatus & dq6)) && 
-			((status & dq5) != dq5);
+			((oldstatus & dq5) == 0);
 			wait_reps++) {
 
 			/* Latency issues. Drop the lock, wait a while and retry */

@@ -822,7 +819,7 @@
 	}
 	if ((status & dq6) != (oldstatus & dq6)) {
 		/* The erasing didn't stop?? */
-		if ((status & dq5) == dq5) {
+		if ((oldstatus & dq5) != 0) {
 			/* dq5 is active so we can do a reset and stop the erase */
 			cfi_write(map, CMD(0xF0), chip->start);
 		}

@@ -897,7 +894,7 @@
 	oldstatus = cfi_read(map, adr);
 	status = cfi_read(map, adr);
 	while( ((status & dq6) != (oldstatus & dq6)) && 
-		((status & dq5) != dq5) &&
+		((oldstatus & dq5) == 0) &&
 		!time_after(jiffies, timeo)) {
 		int wait_reps;
 

@@ -929,7 +926,7 @@
 		for(wait_reps = 0;
 		    	(wait_reps < 100) &&
 			((status & dq6) != (oldstatus & dq6)) && 
-			((status & dq5) != dq5);
+			((oldstatus & dq5) == 0);
 			wait_reps++) {
 
 			/* Latency issues. Drop the lock, wait a while and retry */

@@ -947,34 +944,28 @@
 	if( (status & dq6) != (oldstatus & dq6) ) 
 	{                                       
 		/* The erasing didn't stop?? */
-		if( ( status & dq5 ) == dq5 ) 
+		if( ( oldstatus & dq5 ) != 0 ) 
 		{   			
-			/* When DQ5 raises, we must check once again if DQ6 is toggling.
-               If not, the erase has been completed OK.  If not, reset chip. */
-		    oldstatus   = cfi_read( map, adr );
-		    status      = cfi_read( map, adr );
-		    
-		    if( ( oldstatus & 0x00FF ) == ( status & 0x00FF ) )
-		    {
-                printk( "Warning: DQ5 raised while erase operation was in progress, but erase completed OK\n" ); 		    
-		    } 			
-			else
-            {
-			    /* DQ5 is active so we can do a reset and stop the erase */
-				cfi_write(map, CMD(0xF0), chip->start);
-                printk( KERN_WARNING "Internal flash device timeout occured or write operation was performed while flash was erasing\n" );
-			}
+			/* one or more bit5's set. */
+
+			/* no need to re-check if bit 6 is toggling, as */
+			/* we've already seen bit 6 toggle after bit 5 */
+			/* was set. */
+			
+			/* DQ5 is active so we can do a reset and stop the erase */
+			cfi_write(map, CMD(0xF0), chip->start);
+			printk( KERN_WARNING "Internal flash device timeout occured or write operation was performed while flash was erasing\n" );
 		}
-        else
-        {
-		    printk( "Waiting for erase to complete timed out in do_erase_oneblock.");        
+		else
+		{
+			printk( "Waiting for erase to complete timed out in do_erase_oneblock.");        
 
-		chip->state = FL_READY;
-		wake_up(&chip->wq);
-		cfi_spin_unlock(chip->mutex);
-		DISABLE_VPP(map);
-		return -EIO;
-	}
+			chip->state = FL_READY;
+			wake_up(&chip->wq);
+			cfi_spin_unlock(chip->mutex);
+			DISABLE_VPP(map);
+			return -EIO;
+		}
 	}
 
 	DISABLE_VPP(map);

Like the comment says, this depends on being able to get CMD(1<<5) by doing a left shift of CMD(1<<6).

I therefore think the correct test of bit5 is:

	/* assumes ( (CMD(1<<6) >> 1) == CMD(1<<5) ) for all cases */
	/* I believe this is true from inspection... */

	dq6 = CMD(1<<6);

	do {

		oldstatus = cfi_read(map, adr);
		status = cfi_read(map, addr) ;

		toggling = ( oldstatus ^ status ) & dq6 ;
		dq5s_set = ( oldstatus & (toggling >> 1) ) ;

	} while ( (toggling != 0) && (dq5s_set == 0) && !time_after(xxx));

I'm willing to work this in to my patch. But I want to see what others have to say before I put the work in.

I'm trying to place MTD and JFFS2 on our board, and I'm getting occasional messages like "Warning: DQ5 raised while erase operation was in progress, but erase completed OK" messages.

I think the general method for polling the status of the chips is just a bit wrong, or in other words, this warning is NORMAL operation and should not be printed.

If I understand things correctly, you start an operation like erasing a block, then you get the status by performing a read to that block. If the operation is in progress, data bit 6 toggles, and data bit 5 stays 0. If the operation succeeds, data bits 6 and 5 return the data at that address (bit 6 stops toggling). If the operation fails, bit 6 keeps toggling and bit 5 goes to 1.

The general algorithm used in mtd (recent CVS, cfi_cmdset_0002.c,v 1.61 2002/11/17 13:10:34 spse) goes something like this (timeout condition left out of the loop for brevity)

------------------------------------------------------------

	/* oldstatus and status are two reads from the same address */
	/* in each place it occurs in this code */

	read oldstatus;
	read status;

	while ( ((status & bit6) != (oldstatus & bit6))
		&& ((status & bit5) == 0) )
	{
		read oldstatus;
		read status;
	}

	if ( (status & bit6) != (oldstatus & bit6) )
	{
		/* still toggling, but exited loop. Why? */

		if ( status & bit5 != 0 )
		{
			/* bit5 went high. */
			/* are we still toggling? */
			read oldstatus;
			read status;
			
			if (status == oldstatus)
			{
				/* no longer toggling, operation done */
				printk("Warning: DQ5 raised while ...") ;
			}	
			else
			{
				/* DQ5 went active, reset the chip */
				...
			}
		}

	}

------------------------------------------------------------

[ In the real code, there are some extra reads in the while loop that make the condition I'm about to describe less likely, but it can still happen. ]

Let's say we're looking at an erase operation (the one I seem to run into). Say we manage to just get lucky, and within the while loop, the read of oldstatus returns the last status register value, and status gets the actual data because the chip has returned to read mode. Status's value will be 0xFF, the erased value. Furthermore, suppose we get lucky and the toggle bit's value in the read of oldstatus was a '0' (so in binary, oldstatus would have been 'x00x-xxxx'). So the toggle bit is seen as still toggling in the while loop, but bit5 suddenly goes from 0 to 1.

This will end up printing the warning message erroneously.

I think the "correct" thing to do would be to examine bit 5 from oldstatus, so you're only looking at bit 5 from an earlier read, where you know bit 6 has since toggled. Like this:

	read oldstatus;
	read status;

	while ( ((status & bit6) != (oldstatus & bit6))
		&& ((oldstatus & bit5) == 0) )
	{
		read oldstatus;
		read status;
	}

	if ( (status & bit6) != (oldstatus & bit6) )
	{
		/* still toggling, but exited loop. Why? */

		if ( oldstatus & bit5 != 0 )
		{
			/* bit5 went high. */
			/* NOTE: NO NEED TO CHECK IF WE'RE STILL TOGGLING. */
			/* we know we are, because we checked bit5 in the */
			/* previous read, and a read after it had bit6 */
			/* different. */
				/* DQ5 went active, reset the chip */
				...
		}

	}

I also think there's another error in the original code. The original code sets

dq5 = CMD(1<<5);

which, if I understand things, sets up a word the correct width, with a bit in EACH bit 5 position if you have interleaved chips. Yet the check for bit 5 being set (not being set in this case) is stated as

	((status & dq5) != dq5)

which will only trigger when ALL bit5's are set. I think it should instead be

	((status & dq5) == 0)

which triggers when ANY bit5 gets set. Similarly, tests for

	((status & dq5) == dq5)

should instead be

	((status & dq5) != 0)

I've attached a patch file that changes both these things. Please let me know what you think.

--> Steve Wahl, Brecis Communications.

cfi_cmdset_0002.c renamed from cfi_cmdset_0002.c.orig +36 -45

@@ -520,7 +520,7 @@
 	status = cfi_read(map, adr);
 
 	while( (status & dq6) != (oldstatus & dq6) && 
-	       (status & dq5) != dq5 &&
+	       (oldstatus & dq5) == 0 &&
 	       !time_after(jiffies, timeo) ) {
 
 		if (need_resched()) {

@@ -536,20 +536,17 @@
 
 	if( (status & dq6) != (oldstatus & dq6) ) {
 		/* The erasing didn't stop?? */
-		if( (status & dq5) == dq5 ) {
-			/* When DQ5 raises, we must check once again
-			   if DQ6 is toggling.  If not, the erase has been
-			   completed OK.  If not, reset chip. */
-			oldstatus = cfi_read(map, adr);
-			status = cfi_read(map, adr);
-		    
-			if ( (oldstatus & 0x00FF) == (status & 0x00FF) ) {
-				printk(KERN_WARNING "Warning: DQ5 raised while program operation was in progress, however operation completed OK\n" );
-			} else { 
-				/* DQ5 is active so we can do a reset and stop the erase */
-				cfi_write(map, CMD(0xF0), chip->start);
-				printk(KERN_WARNING "Internal flash device timeout occurred or write operation was performed while flash was programming.\n" );
-			}
+		if( (status & dq5) != 0 ) {
+
+			/* one or more bit5's set. */
+
+			/* no need to re-check if bit 6 is toggling, as */
+			/* we've already seen bit 6 toggle after bit 5 */
+			/* was set. */
+			
+			/* DQ5 is active so we can do a reset and stop the erase */
+			cfi_write(map, CMD(0xF0), chip->start);
+			printk(KERN_WARNING "Internal flash device timeout occurred or write operation was performed while flash was programming.\n" );
 		} else {
 			printk(KERN_WARNING "Waiting for write to complete timed out in do_write_oneword.");        
 

@@ -773,7 +770,7 @@
 	oldstatus = cfi_read(map, adr);
 	status = cfi_read(map, adr);
 	while( ((status & dq6) != (oldstatus & dq6)) && 
-		((status & dq5) != dq5) &&
+		((oldstatus & dq5) == 0) &&
 		!time_after(jiffies, timeo)) {
 		int wait_reps;
 

@@ -805,7 +802,7 @@
 		for(wait_reps = 0;
 		    	(wait_reps < 100) &&
 			((status & dq6) != (oldstatus & dq6)) && 
-			((status & dq5) != dq5);
+			((oldstatus & dq5) == 0);
 			wait_reps++) {
 
 			/* Latency issues. Drop the lock, wait a while and retry */

@@ -822,7 +819,7 @@
 	}
 	if ((status & dq6) != (oldstatus & dq6)) {
 		/* The erasing didn't stop?? */
-		if ((status & dq5) == dq5) {
+		if ((oldstatus & dq5) != 0) {
 			/* dq5 is active so we can do a reset and stop the erase */
 			cfi_write(map, CMD(0xF0), chip->start);
 		}

@@ -897,7 +894,7 @@
 	oldstatus = cfi_read(map, adr);
 	status = cfi_read(map, adr);
 	while( ((status & dq6) != (oldstatus & dq6)) && 
-		((status & dq5) != dq5) &&
+		((oldstatus & dq5) == 0) &&
 		!time_after(jiffies, timeo)) {
 		int wait_reps;
 

@@ -929,7 +926,7 @@
 		for(wait_reps = 0;
 		    	(wait_reps < 100) &&
 			((status & dq6) != (oldstatus & dq6)) && 
-			((status & dq5) != dq5);
+			((oldstatus & dq5) == 0);
 			wait_reps++) {
 
 			/* Latency issues. Drop the lock, wait a while and retry */

@@ -947,34 +944,28 @@
 	if( (status & dq6) != (oldstatus & dq6) ) 
 	{                                       
 		/* The erasing didn't stop?? */
-		if( ( status & dq5 ) == dq5 ) 
+		if( ( oldstatus & dq5 ) != 0 ) 
 		{   			
-			/* When DQ5 raises, we must check once again if DQ6 is toggling.
-               If not, the erase has been completed OK.  If not, reset chip. */
-		    oldstatus   = cfi_read( map, adr );
-		    status      = cfi_read( map, adr );
-		    
-		    if( ( oldstatus & 0x00FF ) == ( status & 0x00FF ) )
-		    {
-                printk( "Warning: DQ5 raised while erase operation was in progress, but erase completed OK\n" ); 		    
-		    } 			
-			else
-            {
-			    /* DQ5 is active so we can do a reset and stop the erase */
-				cfi_write(map, CMD(0xF0), chip->start);
-                printk( KERN_WARNING "Internal flash device timeout occured or write operation was performed while flash was erasing\n" );
-			}
+			/* one or more bit5's set. */
+
+			/* no need to re-check if bit 6 is toggling, as */
+			/* we've already seen bit 6 toggle after bit 5 */
+			/* was set. */
+			
+			/* DQ5 is active so we can do a reset and stop the erase */
+			cfi_write(map, CMD(0xF0), chip->start);
+			printk( KERN_WARNING "Internal flash device timeout occured or write operation was performed while flash was erasing\n" );
 		}
-        else
-        {
-		    printk( "Waiting for erase to complete timed out in do_erase_oneblock.");        
+		else
+		{
+			printk( "Waiting for erase to complete timed out in do_erase_oneblock.");        
 
-		chip->state = FL_READY;
-		wake_up(&chip->wq);
-		cfi_spin_unlock(chip->mutex);
-		DISABLE_VPP(map);
-		return -EIO;
-	}
+			chip->state = FL_READY;
+			wake_up(&chip->wq);
+			cfi_spin_unlock(chip->mutex);
+			DISABLE_VPP(map);
+			return -EIO;
+		}
 	}
 
 	DISABLE_VPP(map);

Dear MTD folk,

Hmm. 5 days and no replies. That seems unusual for this list. Or am I confusing it with the jffs2 developers list?

How can I do better at this?

--> Steve Wahl, Brecis Communications