About flash chip writing/erasing, reading chip status.

[Steve Wahl]

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?

I don't want to be a pest.  But I did expect to see some kind of
interest...

* Was I to wordy?

* Did I send to the wrong place?

* Is there a low-level chip "expert" who happens to be away from
  email, who usually replies to stuff like this?

I want to be a good member of the commuity.  I'd love to make the code
we ship match CVS 100% for ease of future maintenance.  And I'd love
to save someone else the trouble of finding this minor "bug".

How can I do better at this?

--> Steve Wahl, Brecis Communications


On Wed, Jan 15, 2003 at 11:37:05AM -0600, Steve Wahl wrote:
> 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.orig	Wed Jan 15 11:13:11 2003
> +++ cfi_cmdset_0002.c	Wed Jan 15 11:25:29 2003
> @@ -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);