[PATCH] MTD NAND: Fix ECC errors in au1550nd.c

[PATCH] MTD NAND: Fix ECC errors in au1550nd.c

[Vitaly Bordug]

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

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.

Signed-off-by: Konstantin Baidarov <kbaidarov@ru.mvista.com>
Signed-off-by: Vitaly Bordug <vbordug@ru.mvista.com>

-- 
Sincerely,
Vitaly



[-- Attachment #2: common_db1550_nand_2.patch --]
[-- Type: text/x-patch, Size: 5619 bytes --]

Index: linux-2.6.10/drivers/mtd/nand/au1550nd.c
===================================================================
--- linux-2.6.10.orig/drivers/mtd/nand/au1550nd.c
+++ 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;

Re: [PATCH] MTD NAND: Fix ECC errors in au1550nd.c

[Thomas Gleixner]

On Mon, 2005-10-03 at 17:22 +0400, Vitaly Bordug wrote:
> 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 ? 

> - 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.

> 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.

> We must manually keep CS in active state 

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

> 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

Re: [PATCH] MTD NAND: Fix ECC errors in au1550nd.c

[Vitaly Bordug]

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.
Thomas Gleixner wrote:
> On Mon, 2005-10-03 at 17:22 +0400, Vitaly Bordug wrote:
> 
>>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 ? 
> 
TC58DVM92A1FT00
> 
>>- 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
> 
>>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.
> 

> 
>>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.
> 
>>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


-- 
Sincerely,
Vitaly

Re: [PATCH] MTD NAND: Fix ECC errors in au1550nd.c

[Vitaly Wool]

Vitaly Bordug wrote:

<snip>

>+
>+			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

Re: [PATCH] MTD NAND: Fix ECC errors in au1550nd.c

[Vitaly Bordug]

Vitaly Wool wrote:
> Vitaly Bordug wrote:
> 
> <snip>
> 
>> +
>> +            if( (command == NAND_CMD_READ0) || (command == 
>> NAND_CMD_READOOB) ){
>> +                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).


-- 
Sincerely,
Vitaly

Re: [PATCH] MTD NAND: Fix ECC errors in au1550nd.c

[Thomas Gleixner]

On Tue, 2005-10-04 at 15:53 +0400, Vitaly Bordug wrote:
> > 
> 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

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

Re: [PATCH] MTD NAND: Fix ECC errors in au1550nd.c

[Vitaly Bordug]

Thomas Gleixner wrote:
> On Tue, 2005-10-04 at 15:53 +0400, Vitaly Bordug wrote:
> 
>>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
> 
> 
> 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?
> tglx
> 
> 
> 
> 
> 
> 


-- 
Sincerely,
Vitaly

Re: [PATCH] MTD NAND: Fix ECC errors in au1550nd.c

[Vitaly Wool]

Vitaly Bordug wrote:

> Thomas Gleixner wrote:
>
>> On Tue, 2005-10-04 at 15:53 +0400, Vitaly Bordug wrote:
>>
>>> 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
>>
>>
>>
>> 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