additional error checks for AG-AND erase/write

The Renesas AG-AND chips support additional error checking on erase and write operations beyond just checking the operation status. I think the logic is that since ECC can correct 2-bit errors on read, if only a single bit error occurs on and erase or write the operation should not be considered FAIL. Even if a single bit error occurs on read (in addition to the single bit error on write), it will still be corrected and no data will be lost.

I'm looking at how to implement the additional status checks, and am considering adding an optional callback routine to nand_base that, if defined, would be called in the event of an erase or write error before returning a FAIL status. The nand_write routine would need to be modified to perform the callback routine as follows:

<pseudocode>
command(SEQIN) // begin auto page programming enable_hwecc(WRITE)
write_buffer(data)
calculate the ECC // from FPGA write_buffer(ECC) command(PAGE_PROGRAM)
+ if READY && (status & 0x01) // Program Fail (I/O1=1)
+ && error_status_callback // and there is a callback
+ status = error_status_callback(page)
+ endif

    if READY && (status & 0x01)  // Program Fail (I/O1=1)
      return error
    endif

// The callback routine itself would need to perform the following:

error_status_callback(page)
status = read_error_status
if !(status & 0x20) // ECC not available (I/O6=0)
return error
else
ReadECCcheck(page) // Read the data and check the status
if (!1_bit_error) // if Not 1 bit error
return error
endif
endif
return ok
</pseudocode>

One problem I see is that in order to determine if there is a 1-bit error, we must perform a "read" of the page in question, but the erase and write routines both hold a "lock" on the device through 'nand_get_device'. This would prevent the read from proceeding until the erase or write completed (deadlock).

Is there a better place to include additional status checks, or a more appropriate method of implementing this? I would appreciate any suggestions.

Thank you,

d.marlin

Hmm, have you other information ?

When an error occurs in program page, block replacement including corresponding page should be done.
...
When an error occurs in erase operation, future access to this bad block is prohibited.

An error on erase is definitely a criteria for putting the block on the bad list. Actually JFFS2 tries to erase it again before sorting it out.

The single bit programming error does not make a lot of sense to me, as you always have to do RS error correction when you read the page, which results in a nice performance penalty. And its a clear sign that something went wrong. We try to reuse the block, see above.

tglx

Yes. Please see the flowcharts on pages 35 and 36 of the HN29V1G91T-30 data sheet. They show the additional checks I referred to.

I would not disagree, but the Renesas considers this a "feature" of their chips that they would like to see supported. It could be that they are trying to avoid marking blocks bad, since doing so makes 128KiB unusable (JFFS2 virtual erase block size), and they think it is worth the performance hit. I'm just trying to figure out if it is practical to implement in the current driver.

I have hacked nand_base.c to implement part of the logic (extra status checks and read), but these are not really acceptable changes (just something to test with), and I haven't worked out where to check for single bit errors yet (probably in read_ecc). I'm still trying to find a "less invasive" approach that might be acceptable.

d.marlin

Oops, I missed them. So you have to go through the page, read it back, do the RS error correction and check, whether the number of corrected bit errors = 1 or not. Interesting.

Hmm. Everything is possible. It's in the error path anyway so it does not hurt anybody. What we can do to avoid the deadlock is:

Rename nand_read_ecc to do_nand_read_ecc and add an flag argument. Look at the erase function, where we have the additional argument for allowbbt.
flag bits: 0-7 tolerable errors
8 do not get chip
or something like that.
Create a new wrapper function nand_read_ecc which calls do_nand_read_ecc with flags = 0xff. Depending on this argument we grab the device and release it on the end and react on the error check.
The same call can be done from erase in the failure path.

To make this actually work we should add a member like int (*ext_errchk) (.....)
which is checked in the error path

if (this->ext_errchk) {
	res = this->ext_errchk(....);
}

This is a board supplied function, as the board driver knows how much errors are acceptable. We return the number of corrected bits anyway in the correct_data() function, but we check only for -1 at the moment. An additional check does not hurt here, as we are in the slow path again. We check the return value of correct_data() for
(res == -1 || res > flags & 0xff) which ensures in the normal case this tolerance will never bite us, as error correction of 256 bits is unreal.

Hope that helps. I think this will make the code not uglier than it is anyway and keeps it nicely dependend on the board drivers featurelist.

tglx

Thomas Gleixner wrote:

	:

Attached is a patch that I think implements what you described. Please let me know if I've missed anything or gone astray in the details.

Note: I changed a few literals to defined symbols in 'nand_base.c'. Please let me know if you would prefer this in a separate patch (or not at all).

Thank you,

d.marlin
=========

Index: include/linux/mtd/nand.h
===================================================================
RCS file: /home/cvs/mtd/include/linux/mtd/nand.h,v retrieving revision 1.69

diff -u -r1.69 nand.h
@@ -50,6 +50,8 @@
  *			update of nand_chip structure description
  *  01-17-2005 dmarlin	added extended commands for AG-AND device and added option 
  * 			for BBT_AUTO_REFRESH.
+ *  01-20-2005 dmarlin	added optional pointer to hardware specific callback for 
+ *			extra error status checks.
  */
 #ifndef __LINUX_MTD_NAND_H
 #define __LINUX_MTD_NAND_H

@@ -164,7 +166,7 @@
 
 /*
  * Constants for Hardware ECC
-*/
+ */
 /* Reset Hardware ECC for read */
 #define NAND_ECC_READ		0
 /* Reset Hardware ECC for write */

@@ -172,6 +174,10 @@
 /* Enable Hardware ECC before syndrom is read back from flash */
 #define NAND_ECC_READSYN	2
 
+/* Bit mask for flags passed to do_nand_read_ecc */
+#define NAND_GET_DEVICE		0x80
+
+
 /* Option constants for bizarre disfunctionality and real
 *  features
 */

@@ -308,6 +314,8 @@
  * @badblock_pattern:	[REPLACEABLE] bad block scan pattern used for initial bad block scan 
  * @controller:		[OPTIONAL] a pointer to a hardware controller structure which is shared among multiple independend devices
  * @priv:		[OPTIONAL] pointer to private chip date
+ * @errstat:		[OPTIONAL] hardware specific function to perform additional error status checks 
+ *			(determine if errors are correctable)
  */
  
 struct nand_chip {

@@ -363,6 +371,7 @@
 	struct nand_bbt_descr	*badblock_pattern;
 	struct nand_hw_control  *controller;
 	void		*priv;
+	int		(*errstat)(struct mtd_info *mtd, struct nand_chip *this, int state, int status, int page);
 };
 
 /*

@@ -484,6 +493,9 @@
 extern int nand_default_bbt (struct mtd_info *mtd);
 extern int nand_isbad_bbt (struct mtd_info *mtd, loff_t offs, int allowbbt);
 extern int nand_erase_nand (struct mtd_info *mtd, struct erase_info *instr, int allowbbt);
+extern int do_nand_read_ecc (struct mtd_info *mtd, loff_t from, size_t len,
+                             size_t * retlen, u_char * buf, u_char * oob_buf,
+                             struct nand_oobinfo *oobsel, int flags);
 
 /*
 * Constants for oob configuration

Index: drivers/mtd/nand/nand_base.c
===================================================================
RCS file: /home/cvs/mtd/drivers/mtd/nand/nand_base.c,v retrieving revision 1.128

Looks good.

If it's not too much work it would be nice when you can do this change first.

	:

Sure. Would you like it as a patch, or should I just commit it?

Will do.

As I was looking over this, it occurred to me that the new name (do_nand_read_ecc) will be visible external to 'nand_base.c', but the function does not begin with 'nand'. Would it be more appropriate to call it 'nand_do_read_ecc'? Just trying to be consistent.

Thank you,

d.marlin

Commit. That's why you have CVS write access :)

Makes sense. If you want to load your driver as a module, you also have to export the symbol.

tglx