cfi_cmdset_0002 -- erase suspends broken.

cfi_cmdset_0002 -- erase suspends broken.

[David Vrabel]

[-- Attachment #1: Type: text/plain, Size: 1547 bytes --]

Hi,

The latest cfi_cmdset_0002.c (AMD chips) in CVS has broken erase-suspends.

1. Erase suspend command must be written to the block to be resumed.
2. When erasing DQ2 must a) be read from the erase in progress block b) 
doesn't indicate that an erase has been suspended.  DQ6 is the toggle 
bit to check.

There's a patch attached that fixes these issues.  It's a 
work-in-progress as there's still an issue remaining -- JFFS2 reports 
(for example):

"Newly-erased block contained word 0xff7fffff at offset 0x00ca4540
Newly-erased block contained word 0xffefffff at offset 0x00c90a14
Newly-erased block contained word 0xffffffbf at offset 0x00c82ef4
Newly-erased block contained word 0xfffffbff at offset 0x00df0e20
Newly-erased block contained word 0xfffffffe at offset 0x00de0010"

My 1st thought was that some toggle/status bit were still active but it 
doesn't really make sense.

(Tested with an AMD AM29LV128M part.)

David Vrabel
-- 
David Vrabel, Design Engineer

Arcom, Clifton Road           Tel: +44 (0)1223 411200 ext. 3233
Cambridge CB1 7EA, UK         Web: http://www.arcom.com/


_____________________________________________________________________
The message in this transmission is sent in confidence for the attention of the addressee only and should not be disclosed to any other party. Unauthorised recipients are requested to preserve this confidentiality. Please advise the sender if the addressee is not resident at the receiving end.

This message has been checked for all viruses by MessageLabs Virus Control Centre.

[-- Attachment #2: mtd-cfi_cmdset_0002.c-erase-suspend-fix.patch --]
[-- Type: text/plain, Size: 2882 bytes --]

Index: linux-2.4.21/drivers/mtd/chips/cfi_cmdset_0002.c
===================================================================
--- linux-2.4.21.orig/drivers/mtd/chips/cfi_cmdset_0002.c	2003-12-11 15:03:06.000000000 +0000
+++ linux-2.4.21/drivers/mtd/chips/cfi_cmdset_0002.c	2003-12-12 10:00:53.000000000 +0000
@@ -505,22 +505,29 @@
 		      || (mode == FL_WRITING && (cfip->EraseSuspend & 0x1))))
 			goto sleep;
 
+                oldstatus = cfi_read(map, adr);
+                status = cfi_read(map, adr);
+                if ((oldstatus ^ status) & dq2) {
+                        printk(KERN_ERR "Can't suspend erase -- block in progress\n");
+                        goto sleep;
+                }
+
 		/* Erase suspend */
 		/* FIXME - is there a way to verify suspend? */
-		cfi_write(map, CMD(0xB0), adr);
+		cfi_write(map, CMD(0xB0), chip->in_progress_block_addr);
 		chip->oldstate = FL_ERASING;
 		chip->state = FL_ERASE_SUSPENDING;
 		chip->erase_suspended = 1;
 		for (;;) {
-			oldstatus = cfi_read(map, adr);
-			status = cfi_read(map, adr);
-			if (((oldstatus ^ status) & dq2) == dq2)
+			oldstatus = cfi_read(map, chip->in_progress_block_addr);
+			status = cfi_read(map, chip->in_progress_block_addr);
+			if (((oldstatus ^ status) & dq6) != dq6)
 			        break;
 
 			if (time_after(jiffies, timeo)) {
 				/* Urgh. Resume and pretend we weren't here. */
 				/* FIXME - is there a way to verify resume? */
-				cfi_write(map, CMD(0x30), adr);
+				cfi_write(map, CMD(0x30), chip->in_progress_block_addr);
 				chip->state = FL_ERASING;
 				chip->oldstate = FL_READY;
 				printk(KERN_ERR "Chip not ready after erase "
@@ -562,7 +569,7 @@
 	switch(chip->oldstate) {
 	case FL_ERASING:
 		chip->state = chip->oldstate;
-		cfi_write(map, CMD(0x30), adr);
+		cfi_write(map, CMD(0x30), chip->in_progress_block_addr);
 		chip->oldstate = FL_READY;
 		chip->state = FL_ERASING;
 		break;
@@ -1507,7 +1514,8 @@
 
 	chip->state = FL_ERASING;
 	chip->erase_suspended = 0;
-	
+        chip->in_progress_block_addr = adr;
+
 	cfi_spin_unlock(chip->mutex);
 	set_current_state(TASK_UNINTERRUPTIBLE);
 	schedule_timeout((chip->erase_time*HZ)/(2*1000));
@@ -1739,6 +1747,7 @@
 
 	chip->state = FL_ERASING;
 	chip->erase_suspended = 0;
+        chip->in_progress_block_addr = adr;
 	
 	cfi_spin_unlock(chip->mutex);
 	set_current_state(TASK_UNINTERRUPTIBLE);
Index: linux-2.4.21/include/linux/mtd/flashchip.h
===================================================================
--- linux-2.4.21.orig/include/linux/mtd/flashchip.h	2003-12-11 11:19:59.000000000 +0000
+++ linux-2.4.21/include/linux/mtd/flashchip.h	2003-12-11 15:09:22.000000000 +0000
@@ -61,6 +61,7 @@
 
 	int write_suspended:1;
 	int erase_suspended:1;
+        unsigned long in_progress_block_addr;
 
 	spinlock_t *mutex;
 	spinlock_t _spinlock; /* We do it like this because sometimes they'll be shared. */

Re: cfi_cmdset_0002 -- erase suspends broken.

[Thayne Harbaugh]

On Fri, 2003-12-12 at 03:31, David Vrabel wrote:
> Hi,
> 
> The latest cfi_cmdset_0002.c (AMD chips) in CVS has broken erase-suspends.

I knew there was a possibility of this, but didn't have hardware and
software for a good test.

> 1. Erase suspend command must be written to the block to be resumed.
> 2. When erasing DQ2 must a) be read from the erase in progress block b) 
> doesn't indicate that an erase has been suspended.  DQ6 is the toggle 
> bit to check.
> 
> There's a patch attached that fixes these issues.

Wonderful.  I need to look through it a bit more.

>   It's a 
> work-in-progress as there's still an issue remaining -- JFFS2 reports 
> (for example):
> 
> "Newly-erased block contained word 0xff7fffff at offset 0x00ca4540
> Newly-erased block contained word 0xffefffff at offset 0x00c90a14
> Newly-erased block contained word 0xffffffbf at offset 0x00c82ef4
> Newly-erased block contained word 0xfffffbff at offset 0x00df0e20
> Newly-erased block contained word 0xfffffffe at offset 0x00de0010"
> 
> My 1st thought was that some toggle/status bit were still active but it 
> doesn't really make sense.

I'll be looking for another patch, then. 8^)

-- 
Thayne Harbaugh
Linux Networx

Re: cfi_cmdset_0002 -- erase suspends broken.

[David Vrabel]

> +			if (((oldstatus ^ status) & dq6) != dq6)

This should be
     if (((oldstatus ^ status) & dq6) == 0)
to account for interleaved chips (not that mine are). (Thanks Steve Wohl).

The problem with the occasional bit errors appears to only occur when 
the erase is suspended for a write so I've temporarily disabled that.

It also appears to not be toggle/status bits as the flash reads the same 
after a hard reset.

David Vrabel
-- 
David Vrabel, Design Engineer

Arcom, Clifton Road           Tel: +44 (0)1223 411200 ext. 3233
Cambridge CB1 7EA, UK         Web: http://www.arcom.com/


________________________________________________________________________
This email has been scanned for all viruses by the MessageLabs Email
Security System. For more information on a proactive email security
service working around the clock, around the globe, visit
http://www.messagelabs.com
________________________________________________________________________

Re: cfi_cmdset_0002 -- erase suspends broken.

[David Vrabel]

[-- Attachment #1: Type: text/plain, Size: 928 bytes --]

David Vrabel wrote:
>
> The problem with the occasional bit errors appears to only occur when 
> the erase is suspended for a write so I've temporarily disabled that.

Seems I left the patch off that does this.

David Vrabel
-- 
David Vrabel, Design Engineer

Arcom, Clifton Road           Tel: +44 (0)1223 411200 ext. 3233
Cambridge CB1 7EA, UK         Web: http://www.arcom.com/


_____________________________________________________________________
The message in this transmission is sent in confidence for the attention of the addressee only and should not be disclosed to any other party. Unauthorised recipients are requested to preserve this confidentiality. Please advise the sender if the addressee is not resident at the receiving end.  Email to and from Arcom is automatically monitored for operational and lawful business reasons.

This message has been checked for all viruses by MessageLabs Virus Control Centre.

[-- Attachment #2: mtd-cfi_cmdset_0002.c-erase-suspend-fix.patch --]
[-- Type: text/plain, Size: 3378 bytes --]

%patch
Index: linux-2.4.21/drivers/mtd/chips/cfi_cmdset_0002.c
===================================================================
--- linux-2.4.21.orig/drivers/mtd/chips/cfi_cmdset_0002.c	2003-12-11 15:03:06.000000000 +0000
+++ linux-2.4.21/drivers/mtd/chips/cfi_cmdset_0002.c	2003-12-15 15:34:01.000000000 +0000
@@ -500,27 +500,37 @@
 		return 0;
 
 	case FL_ERASING:
+                if (mode == FL_WRITING) /* FIXME: Erase-suspend-program appears broken. */
+                        goto sleep;
+
 		if (!(mode == FL_READY || mode == FL_POINT
 		      || (mode == FL_WRITING && (cfip->EraseSuspend & 0x2))
 		      || (mode == FL_WRITING && (cfip->EraseSuspend & 0x1))))
 			goto sleep;
 
+                oldstatus = cfi_read(map, adr);
+                status = cfi_read(map, adr);
+                if ((oldstatus ^ status) & dq2) {
+                        printk(KERN_ERR "Can't suspend erase -- block in progress\n");
+                        goto sleep;
+                }
+
 		/* Erase suspend */
 		/* FIXME - is there a way to verify suspend? */
-		cfi_write(map, CMD(0xB0), adr);
+		cfi_write(map, CMD(0xB0), chip->in_progress_block_addr);
 		chip->oldstate = FL_ERASING;
 		chip->state = FL_ERASE_SUSPENDING;
 		chip->erase_suspended = 1;
 		for (;;) {
-			oldstatus = cfi_read(map, adr);
-			status = cfi_read(map, adr);
-			if (((oldstatus ^ status) & dq2) == dq2)
+			oldstatus = cfi_read(map, chip->in_progress_block_addr);
+			status = cfi_read(map, chip->in_progress_block_addr);
+			if (((oldstatus ^ status) & dq6) == 0)
 			        break;
 
 			if (time_after(jiffies, timeo)) {
 				/* Urgh. Resume and pretend we weren't here. */
 				/* FIXME - is there a way to verify resume? */
-				cfi_write(map, CMD(0x30), adr);
+				cfi_write(map, CMD(0x30), chip->in_progress_block_addr);
 				chip->state = FL_ERASING;
 				chip->oldstate = FL_READY;
 				printk(KERN_ERR "Chip not ready after erase "
@@ -534,7 +544,7 @@
 			/* Nobody will touch it while it's in state FL_ERASE_SUSPENDING.
 			   So we can just loop here. */
 		}
-		chip->state = FL_STATUS;
+		chip->state = FL_READY;
 		return 0;
 
 	case FL_POINT:
@@ -562,7 +572,7 @@
 	switch(chip->oldstate) {
 	case FL_ERASING:
 		chip->state = chip->oldstate;
-		cfi_write(map, CMD(0x30), adr);
+		cfi_write(map, CMD(0x30), chip->in_progress_block_addr);
 		chip->oldstate = FL_READY;
 		chip->state = FL_ERASING;
 		break;
@@ -1507,7 +1517,8 @@
 
 	chip->state = FL_ERASING;
 	chip->erase_suspended = 0;
-	
+        chip->in_progress_block_addr = adr;
+
 	cfi_spin_unlock(chip->mutex);
 	set_current_state(TASK_UNINTERRUPTIBLE);
 	schedule_timeout((chip->erase_time*HZ)/(2*1000));
@@ -1739,6 +1750,7 @@
 
 	chip->state = FL_ERASING;
 	chip->erase_suspended = 0;
+        chip->in_progress_block_addr = adr;
 	
 	cfi_spin_unlock(chip->mutex);
 	set_current_state(TASK_UNINTERRUPTIBLE);
Index: linux-2.4.21/include/linux/mtd/flashchip.h
===================================================================
--- linux-2.4.21.orig/include/linux/mtd/flashchip.h	2003-12-11 11:19:59.000000000 +0000
+++ linux-2.4.21/include/linux/mtd/flashchip.h	2003-12-11 15:09:22.000000000 +0000
@@ -61,6 +61,7 @@
 
 	int write_suspended:1;
 	int erase_suspended:1;
+        unsigned long in_progress_block_addr;
 
 	spinlock_t *mutex;
 	spinlock_t _spinlock; /* We do it like this because sometimes they'll be shared. */

Re: cfi_cmdset_0002 -- erase suspends broken.

[David Woodhouse]

On Mon, 2004-01-26 at 10:12 +0000, David Vrabel wrote:
> > The problem with the occasional bit errors appears to only occur 
> > when the erase is suspended for a write so I've temporarily disabled
> > that.
> 
> Seems I left the patch off that does this.

Thanks. Wanna go ahead and commit it?

> The message in this transmission is sent in confidence for the
> attention of the addressee only and should not be disclosed to any
> other party. Unauthorised recipients are requested to preserve this
> confidentiality. Please advise the sender if the addressee is not
> resident at the receiving end.  Email to and from Arcom is
> automatically monitored for operational and lawful business reasons.

Not appropriate for a mailing list. If your management and/or lawyers
are incompetent enough to think this is at _all_ useful or enforceable,
and if they _really_ insist on making their level of competence publicly
known with every mail sent out from your company address, then please
use a different mail account for posting to the list.

You're welcome to use your @infradead.org account for list traffic if
you want, btw.

-- 
dwmw2

Re: cfi_cmdset_0002 -- erase suspends broken.

[David Vrabel]

David Woodhouse wrote:
> On Mon, 2004-01-26 at 10:12 +0000, David Vrabel wrote:
> 
>>>The problem with the occasional bit errors appears to only occur 
>>>when the erase is suspended for a write so I've temporarily disabled
>>>that.
>>
>>Seems I left the patch off that does this.
> 
> Thanks. Wanna go ahead and commit it?

Done.  MTD CVS should now work on AMD chips though performance will be 
poor when used with (for example) JFFS2.

David Vrabel



_____________________________________________________________________
The message in this transmission is sent in confidence for the attention of the addressee only and should not be disclosed to any other party. Unauthorised recipients are requested to preserve this confidentiality. Please advise the sender if the addressee is not resident at the receiving end.  Email to and from Arcom is automatically monitored for operational and lawful business reasons.

This message has been checked for all viruses by MessageLabs Virus Control Centre.

Re: cfi_cmdset_0002 -- erase suspends broken.

[David Woodhouse]

On Tue, 2004-01-27 at 10:19 +0000, David Vrabel wrote:
> _____________________________________________________________________
> The message in this transmission is sent in confidence for the
> attention of the addressee only and should not be disclosed to any
> other party. Unauthorised recipients are requested to preserve this
> confidentiality. Please advise the sender if the addressee is not
> resident at the receiving end.  Email to and from Arcom is
> automatically monitored for operational and lawful business reasons.

I can't find a way to make mailman trap for moderation any messages with
'confidental' and/or 'addressee' in the _text_, only to filter on
headers. So I've added 'Received:.*mail.*.arcom.com' along with
'From:.*dolby.com' to the blocked-for-having-clueless-management list.

Anyone else with similar disclaimers want to step forward now and
volunteer to be added to the list, to prevent them from accidentally
sending such crap in the future and being summarily removed?

-- 
dwmw2