Skip to contentSkip to conversation

MTD NAND: Fix ECC errors in au1550nd.c

PATCH In review Vitaly Bordug 3 participants 8 messages Oct 3, 2005 – Oct 5, 2005

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 MTD NAND: Fix ECC errors in au1550nd.c

David,
This patch fixes ECC errors by automatic CS assertion in the driver. We still use manual drive of CS only during READ CYCLE and READ OOB CYCLE, while overriding CS right after third address byte latch and finishing override after first data byte read.

From the Nand read page cycle timings (Toshiba Datasheet) - we have to
keep CS active from third address byte latch to the end of the cycle. It seems that static controller drives CS to low after fourth address byte latch - this interrupts the read cycle.
We must manually keep CS in active state from third address byte latch to second data byte read, not during all read page cycles. And when we drive CS manualy - we have to disable interrupts to prevent simultaneous CS-s activation (NAND, PCMCIA,NOR CS).

Always at your disposal for clarifications.

Signature · 2 lines
Sincerely,
Vitaly
drivers/mtd/nand/au1550nd.c +184 -0
Index: linux-2.6.10/drivers/mtd/nand/au1550nd.c
===================================================================
@@ -19,6 +19,8 @@
 #include <linux/mtd/partitions.h>
 #include <asm/io.h>
 
+#include <linux/interrupt.h>
+
 /* fixme: this is ugly */
 #if LINUX_VERSION_CODE > KERNEL_VERSION(2, 6, 0)
 #include <asm/mach-au1x00/au1000.h>
@@ -81,6 +83,47 @@ const static struct mtd_partition partit
 
 
 /**
+ * au_nand_select_chip_override - [DEFAULT] control CE line
+ * @mtd:	MTD device structure
+ * @chip:	chipnumber to select, -1 for deselect
+ *
+ * Default select function for 1 chip devices.
+ */
+static void au_nand_select_chip_override(struct mtd_info *mtd, int chip)
+{
+	struct nand_chip *this = mtd->priv;
+	switch(chip) {
+	case -1:
+		this->hwcontrol(mtd, NAND_CTL_CLRNCE);
+		break;
+	case 0:
+		this->hwcontrol(mtd, NAND_CTL_SETNCE);
+		break;
+
+	default:
+		BUG();
+	}
+}
+
+
+/*
+ * Wait for the ready pin, after a command
+ * The timeout is catched later.
+ */
+static void au_nand_wait_ready(struct mtd_info *mtd)
+{
+	struct nand_chip *this = mtd->priv;
+	unsigned long	timeo = jiffies + 2;
+
+	/* wait until command is processed or timeout occures */
+	do {
+		if (this->dev_ready(mtd))
+			return;
+	} while (time_before(jiffies, timeo));
+}
+
+
+/**
  * au_read_byte -  read one byte from the chip
  * @mtd:	MTD device structure
  *
@@ -336,6 +379,144 @@ int au1550_device_ready(struct mtd_info 
 	return ret;
 }
 
+/**
+ * au_select_chip - Do not drive CS manual by default.
+ *	Nand flash controller do that automatically.
+ 	We use manual CS drive only in NAND_CMD_READ0 and NAND_CMD_READOOB.
+ * @mtd:	MTD device structure
+ * @chip:	chipnumber to select, -1 for deselect
+ *
+ * Default select function for 1 chip devices.
+ */
+static void au_select_chip(struct mtd_info *mtd, int chip)
+{
+
+}
+
+/**
+ * nand_command - [DEFAULT] Send command to NAND device
+ * @mtd:	MTD device structure
+ * @command:	the command to be sent
+ * @column:	the column address for this command, -1 if none
+ * @page_addr:	the page address for this command, -1 if none
+ *
+ * Send command to NAND device. This function is used for small page
+ * devices (256/512 Bytes per page)
+ */
+static void au_command (struct mtd_info *mtd, unsigned command, int column, int page_addr)
+{
+	register struct nand_chip *this = mtd->priv;
+	unsigned long g_cpu_flags_cmd;
+
+	/* Begin command latch cycle */
+	this->hwcontrol(mtd, NAND_CTL_SETCLE);
+	/*
+	 * Write out the command to the device.
+	 */
+	if (command == NAND_CMD_SEQIN) {
+		int readcmd;
+
+		if (column >= mtd->oobblock) {
+			/* OOB area */
+			column -= mtd->oobblock;
+			readcmd = NAND_CMD_READOOB;
+		} else if (column < 256) {
+			/* First 256 bytes --> READ0 */
+			readcmd = NAND_CMD_READ0;
+		} else {
+			column -= 256;
+			readcmd = NAND_CMD_READ1;
+		}
+		this->write_byte(mtd, readcmd);
+	}
+	this->write_byte(mtd, command);
+
+	/* Set ALE and clear CLE to start address cycle */
+	this->hwcontrol(mtd, NAND_CTL_CLRCLE);
+
+	if (column != -1 || page_addr != -1) {
+		this->hwcontrol(mtd, NAND_CTL_SETALE);
+
+		/* Serially input address */
+		if (column != -1) {
+			/* Adjust columns for 16 bit buswidth */
+			if (this->options & NAND_BUSWIDTH_16)
+				column >>= 1;
+			this->write_byte(mtd, column);
+		}
+		if (page_addr != -1) {
+			this->write_byte(mtd, (unsigned char) (page_addr & 0xff));
+			this->write_byte(mtd, (unsigned char) ((page_addr >> 8) & 0xff));
+
+			if( (command == NAND_CMD_READ0) || (command == NAND_CMD_READOOB) ){
+				save_flags(g_cpu_flags_cmd);
+				cli();
+				au_nand_select_chip_override( mtd, 0 );
+			}
+
+
+			/* One more address cycle for devices > 32MiB */
+			if (this->chipsize > (32 << 20))
+				this->write_byte(mtd, (unsigned char) ((page_addr >> 16) & 0x0f));
+		}
+		/* Latch in address */
+		this->hwcontrol(mtd, NAND_CTL_CLRALE);
+	}
+
+	/*
+	 * program and erase have their own busy handlers
+	 * status and sequential in needs no delay
+	*/
+	switch (command) {
+
+	case NAND_CMD_PAGEPROG:
+	case NAND_CMD_ERASE1:
+	case NAND_CMD_ERASE2:
+	case NAND_CMD_SEQIN:
+	case NAND_CMD_STATUS:
+		return;
+
+	case NAND_CMD_RESET:
+		if (this->dev_ready)
+			break;
+		udelay(this->chip_delay);
+		this->hwcontrol(mtd, NAND_CTL_SETCLE);
+		this->write_byte(mtd, NAND_CMD_STATUS);
+		this->hwcontrol(mtd, NAND_CTL_CLRCLE);
+		while ( !(this->read_byte(mtd) & NAND_STATUS_READY));
+		return;
+
+	/* This applies to read commands */
+	default:
+		/*
+		 * If we don't have access to the busy pin, we apply the given
+		 * command delay
+		*/
+		if (!this->dev_ready) {
+			udelay (this->chip_delay);
+
+			if( (command == NAND_CMD_READ0) || (command == NAND_CMD_READOOB) )
+				goto au1550_nand_restore_flags;
+			return;
+		}
+	}
+	/* Apply this short delay always to ensure that we do wait tWB in
+	 * any case on any machine. */
+	ndelay (100);
+
+	au_nand_wait_ready(mtd);
+
+	if( (command == NAND_CMD_READ0) || (command == NAND_CMD_READOOB) )
+		goto au1550_nand_restore_flags;
+
+	return;
+
+au1550_nand_restore_flags:
+	au_nand_select_chip_override( mtd, -1 );
+	restore_flags(g_cpu_flags_cmd);
+}
+
+
 /*
  * Main initialization routine
  */
@@ -431,6 +612,9 @@ int __init au1550_init (void)
 	this->read_buf = (!nand_width) ? au_read_buf16 : au_read_buf;
 	this->verify_buf = (!nand_width) ? au_verify_buf16 : au_verify_buf;
 
+	this->select_chip = au_select_chip;
+	this->cmdfunc = au_command;
+
 	/* Scan to find existence of the device */
 	if (nand_scan (au1550_mtd, 1)) {
 		retval = -ENXIO;
#1 lore raw
Quoted 7 lines · Vitaly Bordug

David,
This patch fixes ECC errors by automatic CS assertion in the driver. We still use manual drive of CS only during READ CYCLE and READ OOB CYCLE, while overriding CS right after third address byte latch and finishing override after first data byte read.

From the Nand read page cycle timings (Toshiba Datasheet)

Come on. I know those data sheets quite well and there is more than one. Which chip type (Manufacturer part Nr) are you using ?

Quoted 2 lines

- we have to
keep CS active from third address byte latch to the end of the cycle

Thats a known issue and the controller has worked with those chips before.

Quoted 2 lines

It seems that static controller drives CS to low after fourth address byte latch - this interrupts the read cycle.

It seems ?

"drives CS to low ?" CS is active low.

Quoted 1 line

We must manually keep CS in active state

So whats active state: low or high or high-Z ?

Quoted 4 lines

from third address byte latch
to second data byte read, not during all read page cycles. And when we drive CS manualy - we have to disable interrupts to prevent simultaneous CS-s activation (NAND, PCMCIA,NOR CS).

Oh well. Disable interrupts with cli().

Have your Makefiles a builtin -Wignore ?

tglx

#2 lore raw

Thomas, thanks for reviewing this.
I asked the intern who did this work to clarify some points more preciously.

My comments and understanding of the situation is below.

Quoted 8 lines · Thomas Gleixner
Quoted 6 lines · Vitaly Bordug

David,
This patch fixes ECC errors by automatic CS assertion in the driver. We still use manual drive of CS only during READ CYCLE and READ OOB CYCLE, while overriding CS right after third address byte latch and finishing override after first data byte read.

Quoted 6 lines
Quoted 1 line

From the Nand read page cycle timings (Toshiba Datasheet)

Come on. I know those data sheets quite well and there is more than one. Which chip type (Manufacturer part Nr) are you using ?

TC58DVM92A1FT00

Quoted 8 lines
Quoted 3 lines

- we have to
keep CS active from third address byte latch to the end of the cycle

Thats a known issue and the controller has worked with those chips before.

Actually, yes, a sort of. The issue was in some bytes lost during read cycles (ECC errors reported) when PCMCIA (board AMD Alchemy 1550) is active. This does not take place when 4 CS drives happen - as shown on the timing diagram - e.g.
cs: \__/\__/\__/\__/\_<CS low>______ [works fine]
^
asserted by SoC controller
unchanged code:
cs:\______________/\_[PCMCIA driver steals bytes ]...

	           ^
	    asserted by SoC controller
Quoted 7 lines
Quoted 3 lines

It seems that static controller drives CS to low after fourth address byte latch - this interrupts the read cycle.

It seems ?

Quoted 9 lines

"drives CS to low ?" CS is active low.

Quoted 1 line

We must manually keep CS in active state

So whats active state: low or high or high-Z ?

The developer of this code will provide detailed description of the situation.

Quoted 11 lines
Quoted 5 lines

from third address byte latch
to second data byte read, not during all read page cycles. And when we drive CS manualy - we have to disable interrupts to prevent simultaneous CS-s activation (NAND, PCMCIA,NOR CS).

Oh well. Disable interrupts with cli().

Have your Makefiles a builtin -Wignore ?

Heh, how could I miss this crap... Really funny

Signature · 2 lines
Sincerely,
Vitaly
#3 lore raw
Quoted 10 lines · Vitaly Bordug
Quoted 1 line

Actually, yes, a sort of. The issue was in some bytes lost during read cycles (ECC errors reported) when PCMCIA (board AMD Alchemy 1550) is active. This does not take place when 4 CS drives happen - as shown on the timing diagram - e.g.
cs: \__/\__/\__/\__/\_<CS low>______ [works fine]
^
asserted by SoC controller
unchanged code:
cs:\______________/\_[PCMCIA driver steals bytes ]...

Quoted 2 lines
	           ^
	    asserted by SoC controller

Right. Thats a known problem with those FLASH types. You have to hold CS low until the busy pin goes high.

Hmm, you have to protect against interrupts during this time I guess ? If not you can use the existing controller lock implementation to protect against concurrent access.

tglx

#4 lore raw
Quoted 12 lines · Thomas Gleixner
Quoted 8 lines · Vitaly Bordug

Actually, yes, a sort of. The issue was in some bytes lost during read cycles (ECC errors reported) when PCMCIA (board AMD Alchemy 1550) is active. This does not take place when 4 CS drives happen - as shown on the timing diagram - e.g.
cs: \__/\__/\__/\__/\_<CS low>______ [works fine]
^
asserted by SoC controller
unchanged code:
cs:\______________/\_[PCMCIA driver steals bytes ]...

	           ^
	    asserted by SoC controller
Quoted 7 lines

Right. Thats a known problem with those FLASH types. You have to hold CS low until the busy pin goes high.

Hmm, you have to protect against interrupts during this time I guess ? If not you can use the existing controller lock implementation to protect against concurrent access.

Yes. That's because making a global locking mechanism across 3 different divers looks more ugly than just local_irq_save() for a short period in one callback.

Is this acceptable or we need to do it other way?

Quoted 7 lines

tglx

Signature · 2 lines
Sincerely,
Vitaly
#5 lore raw
Quoted 15 lines · Vitaly Bordug
Quoted 13 lines · Thomas Gleixner
Quoted 8 lines · Vitaly Bordug

Actually, yes, a sort of. The issue was in some bytes lost during read cycles (ECC errors reported) when PCMCIA (board AMD Alchemy 1550) is active. This does not take place when 4 CS drives happen - as shown on the timing diagram - e.g.
cs: \__/\__/\__/\__/\_<CS low>______ [works fine]
^
asserted by SoC controller
unchanged code:
cs:\______________/\_[PCMCIA driver steals bytes ]...
^
asserted by SoC controller

Quoted 8 lines
Quoted 7 lines

Right. Thats a known problem with those FLASH types. You have to hold CS low until the busy pin goes high.

Hmm, you have to protect against interrupts during this time I guess ? If not you can use the existing controller lock implementation to protect against concurrent access.

Yes. That's because making a global locking mechanism across 3 different divers looks more ugly than just local_irq_save() for a short period in one callback.

Enabling interrupts in callback looks quite dangerous to me. I'm also afraid that real-time responsiveness of that system is not gonna be satisfactory.
So why not implement mutex-based bus_lock()/bus_unlock() routines somewhere in arch/mips/somewhere and call those from wherever they are needed to prevent concurrent access? It'a a question due to the fact that I haven't seen evidence that you really need to disable interrupts, if that's not the case, please elaborate :)

Vitaly

#6 lore raw
In reply to Vitaly Bordug

Vitaly Bordug wrote:

<snip>

drivers/mtd/nand/au1550nd.c int au1550_device_ready(struct mtd_info · line 453

Quoted 6 lines
drivers/mtd/nand/au1550nd.c +4 -0
+
+			if( (command == NAND_CMD_READ0) || (command == NAND_CMD_READOOB) ){
+				save_flags(g_cpu_flags_cmd);
+				cli();

Don't you want to sti() it somewhere?

Vitaly

#7 lore raw
Quoted 4 lines · Vitaly Wool

Vitaly Bordug wrote:

<snip>

Quoted 9 lines

drivers/mtd/nand/au1550nd.c int au1550_device_ready(struct mtd_info · line 453

Quoted 7 lines
(quoted diff) +2 -0
+
+            if( (command == NAND_CMD_READ0) || (command == 

NAND_CMD_READOOB) ){

drivers/mtd/nand/au1550nd.c +2 -0
+                save_flags(g_cpu_flags_cmd);
+                cli();

Don't you want to sti() it somewhere?

Yes, I understand that saving flags / clearing interrupts in one mtd chip callback and conditionally restoring in another callback seems dangerous. I already asked the developer of this code to clarify this point (this is excusable in case of the h/w demands of this chip unfortunately require something ugly).

Signature · 2 lines
Sincerely,
Vitaly