cfi_cmdset_0002 -- erase suspends broken.

PATCH David Vrabel 4 participants 7 messages Dec 12, 2003 – Jan 27, 2004 Memory technology devices (MTD)

lore

Share & tools

lore

David Vrabel

author

2003-12-12 10:36 UTC #0 lore raw

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

Signature · 10 lines

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.

drivers/mtd/chips/cfi_cmdset_0002.c +16 -7

Index: linux-2.4.21/drivers/mtd/chips/cfi_cmdset_0002.c
===================================================================
@@ -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);

include/linux/mtd/flashchip.h +1 -0

Index: linux-2.4.21/include/linux/mtd/flashchip.h
===================================================================
@@ -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. */

Thayne Harbaugh

2003-12-12 17:17 UTC #1 lore raw

Quoted 3 lines · David Vrabel

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.

Quoted 6 lines

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.

Quoted 4 lines

It's a
work-in-progress as there's still an issue remaining -- JFFS2 reports (for example):

Quoted 8 lines

"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^)

Signature · 2 lines

Thayne Harbaugh
Linux Networx

David Vrabel

author

2003-12-15 15:59 UTC #2 lore raw

In reply to David Vrabel

drivers/mtd/chips/cfi_cmdset_0002.c line 524

Quoted 1 line

drivers/mtd/chips/cfi_cmdset_0002.c +1 -0
+			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

Signature · 12 lines

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
________________________________________________________________________

David Vrabel

author

2004-01-26 10:12 UTC #3 lore raw

Quoted 2 lines · David Vrabel

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

Signature · 10 lines

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.

%patch

drivers/mtd/chips/cfi_cmdset_0002.c +20 -8

Index: linux-2.4.21/drivers/mtd/chips/cfi_cmdset_0002.c
===================================================================
@@ -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);

include/linux/mtd/flashchip.h +1 -0

Index: linux-2.4.21/include/linux/mtd/flashchip.h
===================================================================
@@ -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. */

David Woodhouse

2004-01-26 15:05 UTC #4 lore raw

Quoted 5 lines · David Vrabel

Quoted 3 lines
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?

Quoted 6 lines

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.

Signature · 1 line

dwmw2

David Vrabel

2004-01-27 10:19 UTC #5 lore raw

Quoted 8 lines · David Woodhouse

Quoted 4 lines · David Vrabel
Quoted 2 lines
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.

David Woodhouse

2004-01-27 10:43 UTC #6 lore raw

Quoted 7 lines · 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.

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?

Signature · 1 line

dwmw2