[PATCH] NAND: fix reading/writing OOB for syndrome

[PATCH] NAND: fix reading/writing OOB for syndrome

[Vitaly Wool]

Folks,

being unable to read/write OOB for the NAND flash with 'syndrome' controller (standard read/write oob functions presume OOB is contiguous which is wrong in this case), I took a clumsy attempt to implement this functionality. :)

Signed-off-by: Vitaly Wool <vwool@ru.mvista.com>

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 77406fc..fbfd9fb 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1083,6 +1083,116 @@ static int nand_read(struct mtd_info *mt
 	return ret;
 }
 
+
+/**
+ * nand_read_oob_raw - [REPLACABLE] the most common OOB data read function
+ * @mtd:	mtd info structure
+ * @chip:	nand chip info structure
+ * @buf:	buffer to store read data
+ * @offs:	offset to start writing from
+ * @length:	length of the OOB data to read
+ * @page:	page number to read
+ */
+static void nand_read_oob_raw(struct mtd_info *mtd, struct nand_chip *chip, uint8_t *buf, int offs, int length, int page)
+{
+	chip->read_buf(mtd, buf, length);
+}
+static void (*nand_read_oob_swecc)(struct mtd_info *mtd, struct nand_chip *chip, uint8_t *buf, int offs, int length, int page) = &nand_read_oob_raw;
+static void (*nand_read_oob_hwecc)(struct mtd_info *mtd, struct nand_chip *chip, uint8_t *buf, int offs, int length, int page) = &nand_read_oob_raw;
+
+/**
+ * nand_read_oob_syndrome - [REPLACABLE] OOB data read function for HW ECC
+ * 					 with syndromes
+ * @mtd:	mtd info structure
+ * @chip:	nand chip info structure
+ * @buf:	buffer to store read data
+ * @offs:	offset to start reading from
+ * @length:	length of the OOB data to read
+ * @page:	page number to read
+ */
+static void nand_read_oob_syndrome(struct mtd_info *mtd, struct nand_chip *chip, uint8_t *buf, int offs, int length, int page)
+{
+	int portion = mtd->oobsize / chip->ecc.steps;
+	uint8_t *bufpoi = buf;
+	int i;
+
+	/* 
+	 * We presume here that the chip driver is capable of
+	 * emulating NAND_CMD_READOOB properly
+	 */
+	for (i = 0 ; i < chip->ecc.steps ; i++) {
+		if (offs) {
+			while (portion < offs) {
+				offs -= portion;
+				i++;
+			}
+		}
+		chip->read_buf(mtd, bufpoi, portion - offs);
+		bufpoi += portion - offs;
+		offs = 0;
+		chip->cmdfunc(mtd, NAND_CMD_READOOB, (i + 1) * portion, page);
+	}
+}
+		
+/**
+ * nand_write_oob_raw - [REPLACABLE] the most common OOB data write function
+ * @mtd:	mtd info structure
+ * @chip:	nand chip info structure
+ * @buf:	buffer where to write data from
+ * @offs:	offset to start writing from
+ * @length:	length of the OOB data to write
+ * @page:	page number to write
+ */
+static void nand_write_oob_raw(struct mtd_info *mtd, struct nand_chip *chip, const uint8_t *buf, int offs, int length, int page)
+{
+	chip->write_buf(mtd, buf, length);
+}
+static void (*nand_write_oob_swecc)(struct mtd_info *mtd, struct nand_chip *chip, const uint8_t *buf, int offs, int length, int page) = nand_write_oob_raw;
+static void (*nand_write_oob_hwecc)(struct mtd_info *mtd, struct nand_chip *chip, const uint8_t *buf, int offs, int length, int page) = nand_write_oob_raw;
+
+/**
+ * nand_write_oob_syndrome - [REPLACABLE] OOB data write function for HW ECC
+ * 					  with syndrome
+ * @mtd:	mtd info structure
+ * @chip:	nand chip info structure
+ * @buf:	buffer where to write data from
+ * @offs:	offset to start writing from
+ * @length:	length of the OOB data to write
+ * @page:	page number to write
+ */
+static void nand_write_oob_syndrome(struct mtd_info *mtd, struct nand_chip *chip, const uint8_t *buf, int offs, int length, int page)
+{
+	int portion = mtd->oobsize / chip->ecc.steps;
+	int eccsize = chip->ecc.size;
+	const uint8_t *bufpoi = buf;
+	int i, len;
+
+	for (i = 0 ; i < chip->ecc.steps ; i++) {
+		if (offs < portion) {
+			int status;
+			chip->cmdfunc(mtd, NAND_CMD_SEQIN, 
+				eccsize + i * (eccsize + portion) + offs, page);
+			len = min_t(int, length, portion - offs);
+			chip->write_buf(mtd, bufpoi, len);
+			bufpoi += len;
+			length -= len;
+			offs = 0;
+
+			if (i < chip->ecc.steps - 1) {
+				chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
+				status = chip->waitfunc(mtd, chip, FL_WRITING);
+
+				/* See if device thinks it succeeded */
+				if (status & NAND_STATUS_FAIL)
+					DEBUG(MTD_DEBUG_LEVEL0, "%s: "
+					      "Failed write, page 0x%08x\n",
+					      __FUNCTION__, page);
+			}
+		} else
+			offs -= portion;
+	}
+}
+
 /**
  * nand_do_read_oob - [Intern] NAND read out-of-band
  * @mtd:	MTD device structure
@@ -1127,7 +1237,7 @@ static int nand_do_read_oob(struct mtd_i
 			sndcmd = 0;
 		}
 
-		chip->read_buf(mtd, bufpoi, bytes);
+		chip->ecc.read_oob(mtd, chip, bufpoi, col, bytes, page);
 
 		if (unlikely(!direct))
 			buf = nand_transfer_oob(chip, buf, ops);
@@ -1598,13 +1708,13 @@ static int nand_do_write_oob(struct mtd_
 		nand_fill_oob(chip, ops->oobbuf, ops);
 		chip->cmdfunc(mtd, NAND_CMD_SEQIN, mtd->writesize,
 			      page & chip->pagemask);
-		chip->write_buf(mtd, chip->oob_poi, mtd->oobsize);
+		chip->ecc.write_oob(mtd, chip, chip->oob_poi, 0, mtd->oobsize, page & chip->pagemask);
 		memset(chip->oob_poi, 0xff, mtd->oobsize);
 	} else {
 		chip->cmdfunc(mtd, NAND_CMD_SEQIN,
 			      mtd->writesize + ops->ooboffs,
 			      page & chip->pagemask);
-		chip->write_buf(mtd, ops->oobbuf, ops->len);
+		chip->ecc.write_oob(mtd, chip, ops->oobbuf, ops->ooboffs, ops->len, page & chip->pagemask);
 	}
 
 	/* Send command to program the OOB data */
@@ -2265,6 +2375,10 @@ int nand_scan(struct mtd_info *mtd, int 
 			chip->ecc.read_page = nand_read_page_hwecc;
 		if (!chip->ecc.write_page)
 			chip->ecc.write_page = nand_write_page_hwecc;
+		if (!chip->ecc.read_oob)
+			chip->ecc.read_oob = nand_read_oob_hwecc;
+		if (!chip->ecc.write_oob)
+			chip->ecc.write_oob = nand_write_oob_hwecc;
 
 	case NAND_ECC_HW_SYNDROME:
 		if (!chip->ecc.calculate || !chip->ecc.correct ||
@@ -2278,6 +2392,10 @@ int nand_scan(struct mtd_info *mtd, int 
 			chip->ecc.read_page = nand_read_page_syndrome;
 		if (!chip->ecc.write_page)
 			chip->ecc.write_page = nand_write_page_syndrome;
+		if (!chip->ecc.read_oob)
+			chip->ecc.read_oob = nand_read_oob_syndrome;
+		if (!chip->ecc.write_oob)
+			chip->ecc.write_oob = nand_write_oob_syndrome;
 
 		if (mtd->writesize >= chip->ecc.size)
 			break;
@@ -2291,6 +2409,8 @@ int nand_scan(struct mtd_info *mtd, int 
 		chip->ecc.correct = nand_correct_data;
 		chip->ecc.read_page = nand_read_page_swecc;
 		chip->ecc.write_page = nand_write_page_swecc;
+		chip->ecc.read_oob = nand_read_oob_swecc;
+		chip->ecc.write_oob = nand_write_oob_swecc;
 		chip->ecc.size = 256;
 		chip->ecc.bytes = 3;
 		break;
@@ -2300,6 +2420,8 @@ int nand_scan(struct mtd_info *mtd, int 
 		       "This is not recommended !!\n");
 		chip->ecc.read_page = nand_read_page_raw;
 		chip->ecc.write_page = nand_write_page_raw;
+		chip->ecc.read_oob = nand_read_oob_raw;
+		chip->ecc.write_oob = nand_write_oob_raw;
 		chip->ecc.size = mtd->writesize;
 		chip->ecc.bytes = 0;
 		break;
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index bf2ce68..ef6adfa 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -250,6 +250,19 @@ struct nand_ecc_ctrl {
 	void			(*write_page)(struct mtd_info *mtd,
 					      struct nand_chip *chip,
 					      const uint8_t *buf);
+	void			(*read_oob)(struct mtd_info *mtd,
+					    struct nand_chip *chip,
+					    uint8_t *buf,
+					    int offs,
+					    int length,
+					    int page);
+	void			(*write_oob)(struct mtd_info *mtd,
+					     struct nand_chip *chip,
+					     const uint8_t *buf,
+					     int offs,
+					     int length,
+					     int page);
+					    
 };
 
 /**

[PATCH] NAND: fix reading/writing OOB for syndrome

[Vitaly Wool]

Folks,

being unable to read/write OOB for the NAND flash with 'syndrome' controller (standard read/write oob functions presume OOB is contiguous which is wrong in this case), I took a clumsy attempt to implement this functionality. :)

This is the second attempt, basically it means that it was cleaned up a bit.

Index: linux-2.6/drivers/mtd/nand/nand_base.c
===================================================================
--- linux-2.6.orig/drivers/mtd/nand/nand_base.c
+++ linux-2.6/drivers/mtd/nand/nand_base.c
@@ -1084,6 +1082,135 @@ static int nand_read(struct mtd_info *mt
 }
 
 /**
+ * nand_read_oob_raw - [REPLACABLE] the most common OOB data read function
+ * @mtd:	mtd info structure
+ * @chip:	nand chip info structure
+ * @buf:	buffer to store read data
+ * @offs:	offset to start writing from
+ * @length:	length of the OOB data to read
+ * @page:	page number to read
+ */
+static void nand_read_oob_raw(struct mtd_info *mtd, struct nand_chip *chip,
+			      uint8_t *buf, int offs, int length, int page)
+{
+	chip->read_buf(mtd, buf, length);
+}
+static void (*nand_read_oob_swecc) (struct mtd_info *mtd,
+				    struct nand_chip *chip, uint8_t *buf,
+				    int offs, int length, int page) =
+    &nand_read_oob_raw;
+static void (*nand_read_oob_hwecc) (struct mtd_info *mtd,
+				    struct nand_chip *chip, uint8_t *buf,
+				    int offs, int length, int page) =
+    &nand_read_oob_raw;
+
+/**
+ * nand_read_oob_syndrome - [REPLACABLE] OOB data read function for HW ECC
+ * 					 with syndromes
+ * @mtd:	mtd info structure
+ * @chip:	nand chip info structure
+ * @buf:	buffer to store read data
+ * @offs:	offset to start reading from
+ * @length:	length of the OOB data to read
+ * @page:	page number to read
+ */
+static void nand_read_oob_syndrome(struct mtd_info *mtd, struct nand_chip *chip,
+				   uint8_t *buf, int offs, int length,
+				   int page)
+{
+	int portion = mtd->oobsize / chip->ecc.steps;
+	uint8_t *bufpoi = buf;
+	int i;
+
+	/*
+	 * We presume here that the chip driver is capable of
+	 * emulating NAND_CMD_READOOB properly
+	 */
+	for (i = 0; i < chip->ecc.steps; i++) {
+		if (offs) {
+			while (portion < offs) {
+				offs -= portion;
+				i++;
+			}
+		}
+		chip->read_buf(mtd, bufpoi, portion - offs);
+		bufpoi += portion - offs;
+		offs = 0;
+		chip->cmdfunc(mtd, NAND_CMD_READOOB, (i + 1) * portion, page);
+	}
+}
+
+/**
+ * nand_write_oob_raw - [REPLACABLE] the most common OOB data write function
+ * @mtd:	mtd info structure
+ * @chip:	nand chip info structure
+ * @buf:	buffer where to write data from
+ * @offs:	offset to start writing from
+ * @length:	length of the OOB data to write
+ * @page:	page number to write
+ */
+static void nand_write_oob_raw(struct mtd_info *mtd, struct nand_chip *chip,
+			       const uint8_t *buf, int offs, int length,
+			       int page)
+{
+	chip->write_buf(mtd, buf, length);
+}
+static void (*nand_write_oob_swecc) (struct mtd_info *mtd,
+				     struct nand_chip *chip,
+				     const uint8_t *buf, int offs, int length,
+				     int page) = nand_write_oob_raw;
+static void (*nand_write_oob_hwecc) (struct mtd_info *mtd,
+				     struct nand_chip *chip,
+				     const uint8_t *buf, int offs, int length,
+				     int page) = nand_write_oob_raw;
+
+/**
+ * nand_write_oob_syndrome - [REPLACABLE] OOB data write function for HW ECC
+ * 					  with syndrome
+ * @mtd:	mtd info structure
+ * @chip:	nand chip info structure
+ * @buf:	buffer where to write data from
+ * @offs:	offset to start writing from
+ * @length:	length of the OOB data to write
+ * @page:	page number to write
+ */
+static void nand_write_oob_syndrome(struct mtd_info *mtd,
+				    struct nand_chip *chip, const uint8_t *buf,
+				    int offs, int length, int page)
+{
+	int portion = mtd->oobsize / chip->ecc.steps;
+	int eccsize = chip->ecc.size;
+	const uint8_t *bufpoi = buf;
+	int i, len;
+
+	for (i = 0; i < chip->ecc.steps; i++) {
+		if (offs < portion) {
+			int status;
+			chip->cmdfunc(mtd, NAND_CMD_SEQIN,
+				      eccsize + i * (eccsize + portion) + offs,
+				      page);
+			len = min_t(int, length, portion - offs);
+			chip->write_buf(mtd, bufpoi, len);
+			bufpoi += len;
+			length -= len;
+			offs = 0;
+
+			if (i < chip->ecc.steps - 1) {
+				chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
+				status = chip->waitfunc(mtd, chip, FL_WRITING);
+
+				/* See if device thinks it succeeded */
+				if (status & NAND_STATUS_FAIL)
+					DEBUG(MTD_DEBUG_LEVEL0, "%s: "
+					      "Failed write, page 0x%08x\n",
+					      __FUNCTION__, page);
+			}
+		} else
+			offs -= portion;
+	}
+}
+
+/**
  * nand_do_read_oob - [Intern] NAND read out-of-band
  * @mtd:	MTD device structure
  * @from:	offset to read from
@@ -1127,7 +1254,7 @@ static int nand_do_read_oob(struct mtd_i
 			sndcmd = 0;
 		}
 
-		chip->read_buf(mtd, bufpoi, bytes);
+		chip->ecc.read_oob(mtd, chip, bufpoi, col, bytes, page);
 
 		if (unlikely(!direct))
 			buf = nand_transfer_oob(chip, buf, ops);
@@ -1598,13 +1726,15 @@ static int nand_do_write_oob(struct mtd_
 		nand_fill_oob(chip, ops->oobbuf, ops);
 		chip->cmdfunc(mtd, NAND_CMD_SEQIN, mtd->writesize,
 			      page & chip->pagemask);
-		chip->write_buf(mtd, chip->oob_poi, mtd->oobsize);
+		chip->ecc.write_oob(mtd, chip, chip->oob_poi, 0, mtd->oobsize,
+				    page & chip->pagemask);
 		memset(chip->oob_poi, 0xff, mtd->oobsize);
 	} else {
 		chip->cmdfunc(mtd, NAND_CMD_SEQIN,
 			      mtd->writesize + ops->ooboffs,
 			      page & chip->pagemask);
-		chip->write_buf(mtd, ops->oobbuf, ops->len);
+		chip->ecc.write_oob(mtd, chip, ops->oobbuf, ops->ooboffs,
+				    ops->len, page & chip->pagemask);
 	}
 
 	/* Send command to program the OOB data */
@@ -2265,6 +2395,10 @@ int nand_scan(struct mtd_info *mtd, int 
 			chip->ecc.read_page = nand_read_page_hwecc;
 		if (!chip->ecc.write_page)
 			chip->ecc.write_page = nand_write_page_hwecc;
+		if (!chip->ecc.read_oob)
+			chip->ecc.read_oob = nand_read_oob_hwecc;
+		if (!chip->ecc.write_oob)
+			chip->ecc.write_oob = nand_write_oob_hwecc;
 
 	case NAND_ECC_HW_SYNDROME:
 		if (!chip->ecc.calculate || !chip->ecc.correct ||
@@ -2278,6 +2412,10 @@ int nand_scan(struct mtd_info *mtd, int 
 			chip->ecc.read_page = nand_read_page_syndrome;
 		if (!chip->ecc.write_page)
 			chip->ecc.write_page = nand_write_page_syndrome;
+		if (!chip->ecc.read_oob)
+			chip->ecc.read_oob = nand_read_oob_syndrome;
+		if (!chip->ecc.write_oob)
+			chip->ecc.write_oob = nand_write_oob_syndrome;
 
 		if (mtd->writesize >= chip->ecc.size)
 			break;
@@ -2291,6 +2429,8 @@ int nand_scan(struct mtd_info *mtd, int 
 		chip->ecc.correct = nand_correct_data;
 		chip->ecc.read_page = nand_read_page_swecc;
 		chip->ecc.write_page = nand_write_page_swecc;
+		chip->ecc.read_oob = nand_read_oob_swecc;
+		chip->ecc.write_oob = nand_write_oob_swecc;
 		chip->ecc.size = 256;
 		chip->ecc.bytes = 3;
 		break;
@@ -2300,6 +2440,8 @@ int nand_scan(struct mtd_info *mtd, int 
 		       "This is not recommended !!\n");
 		chip->ecc.read_page = nand_read_page_raw;
 		chip->ecc.write_page = nand_write_page_raw;
+		chip->ecc.read_oob = nand_read_oob_raw;
+		chip->ecc.write_oob = nand_write_oob_raw;
 		chip->ecc.size = mtd->writesize;
 		chip->ecc.bytes = 0;
 		break;
Index: linux-2.6/include/linux/mtd/nand.h
===================================================================
--- linux-2.6.orig/include/linux/mtd/nand.h
+++ linux-2.6/include/linux/mtd/nand.h
@@ -250,6 +250,18 @@ struct nand_ecc_ctrl {
 	void			(*write_page)(struct mtd_info *mtd,
 					      struct nand_chip *chip,
 					      const uint8_t *buf);
+	void			(*read_oob)(struct mtd_info *mtd,
+					    struct nand_chip *chip,
+					    uint8_t *buf,
+					    int offs,
+					    int length,
+					    int page);
+	void			(*write_oob)(struct mtd_info *mtd,
+					     struct nand_chip *chip,
+					     const uint8_t *buf,
+					     int offs,
+					     int length,
+					     int page);
 };
 
 /**

Re: [PATCH] NAND: fix reading/writing OOB for syndrome

[Thomas Gleixner]

On Tue, 2006-06-13 at 19:01 +0400, Vitaly Wool wrote:
> +/**
> + * nand_read_oob_syndrome - [REPLACABLE] OOB data read function for HW ECC
> + * 					 with syndromes
> + * @mtd:	mtd info structure
> + * @chip:	nand chip info structure
> + * @buf:	buffer to store read data
> + * @offs:	offset to start reading from
> + * @length:	length of the OOB data to read
> + * @page:	page number to read
> + */
> +static void nand_read_oob_syndrome(struct mtd_info *mtd, struct nand_chip *chip,
> +				   uint8_t *buf, int offs, int length,
> +				   int page)
> +{
> +	int portion = mtd->oobsize / chip->ecc.steps;
> +	uint8_t *bufpoi = buf;
> +	int i;
> +
> +	/*
> +	 * We presume here that the chip driver is capable of
> +	 * emulating NAND_CMD_READOOB properly
> +	 */

???? You want to read in the data area of the FLASH !

> +	for (i = 0; i < chip->ecc.steps; i++) {
> +		if (offs) {
> +			while (portion < offs) {
> +				offs -= portion;
> +				i++;

		if (offs >= portion) {
			offs -= portion;
			continue;
		}

> +		}
> +		chip->read_buf(mtd, bufpoi, portion - offs);
> +		bufpoi += portion - offs;
> +		offs = 0;
> +		chip->cmdfunc(mtd, NAND_CMD_READOOB, (i + 1) * portion, page);

		CMD_READOOB is simply wrong 

Also it ignores the prepad and postpad parts.



> +	}
> +}
> +
> +/**
> + * nand_write_oob_raw - [REPLACABLE] the most common OOB data write function
> + * @mtd:	mtd info structure
> + * @chip:	nand chip info structure
> + * @buf:	buffer where to write data from
> + * @offs:	offset to start writing from
> + * @length:	length of the OOB data to write
> + * @page:	page number to write
> + */
> +static void nand_write_oob_raw(struct mtd_info *mtd, struct nand_chip *chip,
> +			       const uint8_t *buf, int offs, int length,
> +			       int page)
> +{
> +	chip->write_buf(mtd, buf, length);
> +}
> +static void (*nand_write_oob_swecc) (struct mtd_info *mtd,
> +				     struct nand_chip *chip,
> +				     const uint8_t *buf, int offs, int length,
> +				     int page) = nand_write_oob_raw;
> +static void (*nand_write_oob_hwecc) (struct mtd_info *mtd,
> +				     struct nand_chip *chip,
> +				     const uint8_t *buf, int offs, int length,
> +				     int page) = nand_write_oob_raw;
> +
> +/**
> + * nand_write_oob_syndrome - [REPLACABLE] OOB data write function for HW ECC
> + * 					  with syndrome
> + * @mtd:	mtd info structure
> + * @chip:	nand chip info structure
> + * @buf:	buffer where to write data from
> + * @offs:	offset to start writing from
> + * @length:	length of the OOB data to write
> + * @page:	page number to write
> + */
> +static void nand_write_oob_syndrome(struct mtd_info *mtd,
> +				    struct nand_chip *chip, const uint8_t *buf,
> +				    int offs, int length, int page)
> +{
> +	int portion = mtd->oobsize / chip->ecc.steps;
> +	int eccsize = chip->ecc.size;
> +	const uint8_t *bufpoi = buf;
> +	int i, len;
> +
> +	for (i = 0; i < chip->ecc.steps; i++) {
> +		if (offs < portion) {
> +			int status;
> +			chip->cmdfunc(mtd, NAND_CMD_SEQIN,
> +				      eccsize + i * (eccsize + portion) + offs,
> +				      page);
> +			len = min_t(int, length, portion - offs);
> +			chip->write_buf(mtd, bufpoi, len);
> +			bufpoi += len;
> +			length -= len;
> +			offs = 0;
> +
> +			if (i < chip->ecc.steps - 1) {
> +				chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
> +				status = chip->waitfunc(mtd, chip, FL_WRITING);
> +
> +				/* See if device thinks it succeeded */
> +				if (status & NAND_STATUS_FAIL)
> +					DEBUG(MTD_DEBUG_LEVEL0, "%s: "
> +					      "Failed write, page 0x%08x\n",
> +					      __FUNCTION__, page);
> +			}
> +		} else
> +			offs -= portion;
> +	}
> +}
> +

What about the prepad and postpad parts ?

> +/**
>   * nand_do_read_oob - [Intern] NAND read out-of-band
>   * @mtd:	MTD device structure
>   * @from:	offset to read from
> @@ -1127,7 +1254,7 @@ static int nand_do_read_oob(struct mtd_i
>  			sndcmd = 0;
>  		}
>  
> -		chip->read_buf(mtd, bufpoi, bytes);
> +		chip->ecc.read_oob(mtd, chip, bufpoi, col, bytes, page);
>  
>  		if (unlikely(!direct))
>  			buf = nand_transfer_oob(chip, buf, ops);

The direct part should go away and done inside the read functions.

> @@ -1598,13 +1726,15 @@ static int nand_do_write_oob(struct mtd_
>  		nand_fill_oob(chip, ops->oobbuf, ops);
>  		chip->cmdfunc(mtd, NAND_CMD_SEQIN, mtd->writesize,
>  			      page & chip->pagemask);

You put it unconditionally into oob area !!!

> -		chip->write_buf(mtd, chip->oob_poi, mtd->oobsize);
> +		chip->ecc.write_oob(mtd, chip, chip->oob_poi, 0, mtd->oobsize,
> +				    page & chip->pagemask);
>  		memset(chip->oob_poi, 0xff, mtd->oobsize);
>  	} else {
>  		chip->cmdfunc(mtd, NAND_CMD_SEQIN,
>  			      mtd->writesize + ops->ooboffs,
>  			      page & chip->pagemask);

Same here !

> -		chip->write_buf(mtd, ops->oobbuf, ops->len);
> +		chip->ecc.write_oob(mtd, chip, ops->oobbuf, ops->ooboffs,
> +				    ops->len, page & chip->pagemask);
>  	}
>  

	tglx
	

Re: [PATCH] NAND: fix reading/writing OOB for syndrome

[Vitaly Wool]

On Tue, 13 Jun 2006 17:34:55 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> ???? You want to read in the data area of the FLASH !

No, it becomes a requirement to the NAND chip driver to convert NAND_CMD_READOOB to what's appropriate for the chip.
If we require the chip driver to be able to convert NAND_CMD_READOOB with offset 0, why not require it to convert it properly for other offsets?
 
> > +		chip->cmdfunc(mtd, NAND_CMD_READOOB, (i + 1) * portion, page);
> 
> 		CMD_READOOB is simply wrong 
> 
> Also it ignores the prepad and postpad parts.

See above.
I read all the OOB by portions for the (data, oob+ecc) x eccsteps layout.
I don't distinguish between spare OOB and ECC so I don't need to take prepad/postpad into account.
We'll just need another couple of funcs for syndromes that imply ( (data, ecc) x eccsteps, oob) layout.
 
> > +	for (i = 0; i < chip->ecc.steps; i++) {
> > +		if (offs < portion) {
> > +			int status;
> > +			chip->cmdfunc(mtd, NAND_CMD_SEQIN,
> > +				      eccsize + i * (eccsize + portion) + offs,
> > +				      page);
> > +			len = min_t(int, length, portion - offs);
> > +			chip->write_buf(mtd, bufpoi, len);
> > +			bufpoi += len;
> > +			length -= len;
> > +			offs = 0;
> > +
> > +			if (i < chip->ecc.steps - 1) {
> > +				chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
> > +				status = chip->waitfunc(mtd, chip, FL_WRITING);
> > +
> > +				/* See if device thinks it succeeded */
> > +				if (status & NAND_STATUS_FAIL)
> > +					DEBUG(MTD_DEBUG_LEVEL0, "%s: "
> > +					      "Failed write, page 0x%08x\n",
> > +					      __FUNCTION__, page);
> > +			}
> > +		} else
> > +			offs -= portion;
> > +	}
> > +}
> > +
> 
> What about the prepad and postpad parts ?

See above.
 
> > +/**
> >   * nand_do_read_oob - [Intern] NAND read out-of-band
> >   * @mtd:	MTD device structure
> >   * @from:	offset to read from
> > @@ -1127,7 +1254,7 @@ static int nand_do_read_oob(struct mtd_i
> >  			sndcmd = 0;
> >  		}
> >  
> > -		chip->read_buf(mtd, bufpoi, bytes);
> > +		chip->ecc.read_oob(mtd, chip, bufpoi, col, bytes, page);
> >  
> >  		if (unlikely(!direct))
> >  			buf = nand_transfer_oob(chip, buf, ops);
> 
> The direct part should go away and done inside the read functions.

Hmmm... Probably.

 
> > @@ -1598,13 +1726,15 @@ static int nand_do_write_oob(struct mtd_
> >  		nand_fill_oob(chip, ops->oobbuf, ops);
> >  		chip->cmdfunc(mtd, NAND_CMD_SEQIN, mtd->writesize,
> >  			      page & chip->pagemask);
> 
> You put it unconditionally into oob area !!!

Can you please elaborate what's wrong there?

> > -		chip->write_buf(mtd, chip->oob_poi, mtd->oobsize);
> > +		chip->ecc.write_oob(mtd, chip, chip->oob_poi, 0, mtd->oobsize,
> > +				    page & chip->pagemask);
> >  		memset(chip->oob_poi, 0xff, mtd->oobsize);
> >  	} else {
> >  		chip->cmdfunc(mtd, NAND_CMD_SEQIN,
> >  			      mtd->writesize + ops->ooboffs,
> >  			      page & chip->pagemask);
> 
> Same here !

And there.


Vitaly

Re: [PATCH] NAND: fix reading/writing OOB for syndrome

[Thomas Gleixner]

On Tue, 2006-06-13 at 19:48 +0400, Vitaly Wool wrote:
> On Tue, 13 Jun 2006 17:34:55 +0200
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > ???? You want to read in the data area of the FLASH !
> 
> No, it becomes a requirement to the NAND chip driver to convert NAND_CMD_READOOB to what's appropriate for the chip.
> If we require the chip driver to be able to convert NAND_CMD_READOOB with offset 0, why not require it to convert it properly for other offsets?

Thats completely bogus. We will put such crap into the command
functions. Command functions handle raw flash commands and nothing else.
The fixup for NAND_CMD_READOOB is just a conveniance to keep small and
large page compatible. 

> > > +		chip->cmdfunc(mtd, NAND_CMD_READOOB, (i + 1) * portion, page);
> > 
> > 		CMD_READOOB is simply wrong 
> > 
> > Also it ignores the prepad and postpad parts.
> 
> See above.
> I read all the OOB by portions for the (data, oob+ecc) x eccsteps layout.
> I don't distinguish between spare OOB and ECC so I don't need to take prepad/postpad into account.
> We'll just need another couple of funcs for syndromes that imply ( (data, ecc) x eccsteps, oob) layout.

Ah, and we need to make both command functions aware of that crap ? Only
over my dead body !
 
	tglx

Re: [PATCH] NAND: fix reading/writing OOB for syndrome

[Vitaly Wool]

On Tue, 13 Jun 2006 17:52:53 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Tue, 2006-06-13 at 19:48 +0400, Vitaly Wool wrote:
> > On Tue, 13 Jun 2006 17:34:55 +0200
> > Thomas Gleixner <tglx@linutronix.de> wrote:
> > 
> > > ???? You want to read in the data area of the FLASH !
> > 
> > No, it becomes a requirement to the NAND chip driver to convert NAND_CMD_READOOB to what's appropriate for the chip.
> > If we require the chip driver to be able to convert NAND_CMD_READOOB with offset 0, why not require it to convert it properly for other offsets?
> 
> Thats completely bogus. We will put such crap into the command
> functions. Command functions handle raw flash commands and nothing else.
> The fixup for NAND_CMD_READOOB is just a conveniance to keep small and
> large page compatible. 

Hmm, agreed. Not a big thing to change though.

> > I read all the OOB by portions for the (data, oob+ecc) x eccsteps layout.
> > I don't distinguish between spare OOB and ECC so I don't need to take prepad/postpad into account.
> > We'll just need another couple of funcs for syndromes that imply ( (data, ecc) x eccsteps, oob) layout.
> 
> Ah, and we need to make both command functions aware of that crap ? Only
> over my dead body !

Not it's not that dramatic. :)
After some speculation, I do agree that handling prepad and postpad there is necessary. 

Vitaly

Re: [PATCH] NAND: fix reading/writing OOB for syndrome

[Vitaly Wool]

Okay,

here's the new one :)

Index: linux-2.6/drivers/mtd/nand/nand_base.c
===================================================================
--- linux-2.6.orig/drivers/mtd/nand/nand_base.c
+++ linux-2.6/drivers/mtd/nand/nand_base.c
@@ -1084,6 +1084,168 @@ static int nand_read(struct mtd_info *mt
 }
 
 /**
+ * nand_read_oob_raw - [REPLACABLE] the most common OOB data read function
+ * @mtd:	mtd info structure
+ * @chip:	nand chip info structure
+ * @buf:	buffer to store read data
+ * @offs:	offset to start writing from
+ * @length:	length of the OOB data to read
+ * @page:	page number to read
+ * @sndcmd:	pointer to the flag whether to issue read command or not
+ */
+static void nand_read_oob_raw(struct mtd_info *mtd, struct nand_chip *chip,
+			      uint8_t *buf, int offs, int length, int page,
+			      int *sndcmd)
+{
+	if (*sndcmd) {
+		chip->cmdfunc(mtd, NAND_CMD_READOOB, col, page);
+		*sndcmd = 0;
+	}
+	chip->read_buf(mtd, buf, length);
+}
+static void (*nand_read_oob_swecc) (struct mtd_info *mtd,
+				    struct nand_chip *chip, uint8_t *buf,
+				    int offs, int length, int page,
+				    int *sndcmd) =
+    &nand_read_oob_raw;
+static void (*nand_read_oob_hwecc) (struct mtd_info *mtd,
+				    struct nand_chip *chip, uint8_t *buf,
+				    int offs, int length, int page,
+				    int *sndcmd) =
+    &nand_read_oob_raw;
+
+/**
+ * nand_read_oob_syndrome - [REPLACABLE] OOB data read function for HW ECC
+ * 					 with syndromes
+ * @mtd:	mtd info structure
+ * @chip:	nand chip info structure
+ * @buf:	buffer to store read data
+ * @offs:	offset to start reading from
+ * @length:	length of the OOB data to read
+ * @page:	page number to read
+ * @sndcmd:	pointer to the flag whether to issue read command or not
+ */
+static void nand_read_oob_syndrome(struct mtd_info *mtd, struct nand_chip *chip,
+				   uint8_t *buf, int offs, int length,
+				   int page, int *sndcmd)
+{
+	int portion = mtd->eccbytes + chip->ecc.prepad + chip->ecc.postpad;
+	uint8_t *bufpoi = buf;
+	int i, toread;
+
+	for (i = 0; i < chip->ecc.steps; i++) {
+		if (offs) {
+			while (portion < offs) {
+				offs -= portion;
+				i++;
+			}
+		}
+		chip->cmdfunc(mtd, NAND_CMD_READ0,
+				(i + 1) * chip->ecc.size + i * portion + offs,
+				page);
+		toread = min_t(length, portion - offs);
+		chip->read_buf(mtd, bufpoi, toread);
+		bufpoi += toread;
+		length -= toread;
+		offs = 0;
+	}
+	if (length > 0)
+		chip->read_buf(mtd, bufpoi, length);
+}
+
+/**
+ * nand_write_oob_raw - [REPLACABLE] the most common OOB data write function
+ * @mtd:	mtd info structure
+ * @chip:	nand chip info structure
+ * @buf:	buffer where to write data from
+ * @offs:	offset to start writing from
+ * @length:	length of the OOB data to write
+ * @page:	page number to write
+ */
+static void nand_write_oob_raw(struct mtd_info *mtd, struct nand_chip *chip,
+			       const uint8_t *buf, int offs, int length,
+			       int page)
+{
+	chip->cmdfunc(mtd, NAND_CMD_SEQIN, mtd->writesize,
+			page & chip->pagemask);
+	chip->write_buf(mtd, buf, length);
+	/* Send command to program the OOB data */
+	chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
+
+	status = chip->waitfunc(mtd, chip, FL_WRITING);
+
+	/* See if device thinks it succeeded */
+	if (status & NAND_STATUS_FAIL) {
+		DEBUG(MTD_DEBUG_LEVEL0, "nand_write_oob: "
+		      "Failed write, page 0x%08x\n", page);
+	}
+}
+static void (*nand_write_oob_swecc) (struct mtd_info *mtd,
+				     struct nand_chip *chip,
+				     const uint8_t *buf, int offs, int length,
+				     int page) = nand_write_oob_raw;
+static void (*nand_write_oob_hwecc) (struct mtd_info *mtd,
+				     struct nand_chip *chip,
+				     const uint8_t *buf, int offs, int length,
+				     int page) = nand_write_oob_raw;
+
+/**
+ * nand_write_oob_syndrome - [REPLACABLE] OOB data write function for HW ECC
+ * 					  with syndrome
+ * @mtd:	mtd info structure
+ * @chip:	nand chip info structure
+ * @buf:	buffer where to write data from
+ * @offs:	offset to start writing from
+ * @length:	length of the OOB data to write
+ * @page:	page number to write
+ */
+static void nand_write_oob_syndrome(struct mtd_info *mtd,
+				    struct nand_chip *chip, const uint8_t *buf,
+				    int offs, int length, int page)
+{
+	int portion = chip->ecc.bytes + chip->ecc.prepad + chip->ecc.postpad;
+	int eccsize = chip->ecc.size;
+	const uint8_t *bufpoi = buf;
+	int i, len;
+
+	for (i = 0; i < chip->ecc.steps; i++) {
+		if (offs < portion) {
+			int status;
+			chip->cmdfunc(mtd, NAND_CMD_SEQIN,
+				      eccsize + i * (eccsize + portion) + offs,
+				      page);
+			len = min_t(int, length, portion - offs);
+			chip->write_buf(mtd, bufpoi, len);
+			bufpoi += len;
+			length -= len;
+			offs = 0;
+
+			if (i < chip->ecc.steps - 1 && length > 0) {
+				chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
+				status = chip->waitfunc(mtd, chip, FL_WRITING);
+
+				/* See if device thinks it succeeded */
+				if (status & NAND_STATUS_FAIL)
+					DEBUG(MTD_DEBUG_LEVEL0, "%s: "
+					      "Failed write, page 0x%08x\n",
+					      __FUNCTION__, page);
+			}
+		} else
+			offs -= portion;
+	}
+	if (length > 0)
+		chip->write_buf(mtd, bufpoi, length);
+	chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
+	status = chip->waitfunc(mtd, chip, FL_WRITING);
+
+	/* See if device thinks it succeeded */
+	if (status & NAND_STATUS_FAIL)
+		DEBUG(MTD_DEBUG_LEVEL0, "%s: "
+		      "Failed write, page 0x%08x\n",
+		      __FUNCTION__, page);
+}
+
+/**
  * nand_do_read_oob - [Intern] NAND read out-of-band
  * @mtd:	MTD device structure
  * @from:	offset to read from
@@ -1122,12 +1284,8 @@ static int nand_do_read_oob(struct mtd_i
 		bytes = direct ? ops->ooblen : mtd->oobsize;
 		bufpoi = direct ? buf : chip->buffers.oobrbuf;
 
-		if (likely(sndcmd)) {
-			chip->cmdfunc(mtd, NAND_CMD_READOOB, col, page);
-			sndcmd = 0;
-		}
-
-		chip->read_buf(mtd, bufpoi, bytes);
+		chip->ecc.read_oob(mtd, chip, bufpoi, col,
+				  bytes, page, &sndcmd);
 
 		if (unlikely(!direct))
 			buf = nand_transfer_oob(chip, buf, ops);
@@ -1596,28 +1754,17 @@ static int nand_do_write_oob(struct mtd_
 		chip->oob_poi = chip->buffers.oobwbuf;
 		memset(chip->oob_poi, 0xff, mtd->oobsize);
 		nand_fill_oob(chip, ops->oobbuf, ops);
-		chip->cmdfunc(mtd, NAND_CMD_SEQIN, mtd->writesize,
-			      page & chip->pagemask);
-		chip->write_buf(mtd, chip->oob_poi, mtd->oobsize);
+		chip->ecc.write_oob(mtd, chip, chip->oob_poi, 0, mtd->oobsize,
+				    page & chip->pagemask);
 		memset(chip->oob_poi, 0xff, mtd->oobsize);
 	} else {
 		chip->cmdfunc(mtd, NAND_CMD_SEQIN,
 			      mtd->writesize + ops->ooboffs,
 			      page & chip->pagemask);
-		chip->write_buf(mtd, ops->oobbuf, ops->len);
+		chip->ecc.write_oob(mtd, chip, ops->oobbuf, ops->ooboffs,
+				    ops->len, page & chip->pagemask);
 	}
 
-	/* Send command to program the OOB data */
-	chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
-
-	status = chip->waitfunc(mtd, chip, FL_WRITING);
-
-	/* See if device thinks it succeeded */
-	if (status & NAND_STATUS_FAIL) {
-		DEBUG(MTD_DEBUG_LEVEL0, "nand_write_oob: "
-		      "Failed write, page 0x%08x\n", page);
-		return -EIO;
-	}
 	ops->retlen = ops->len;
 
 #ifdef CONFIG_MTD_NAND_VERIFY_WRITE
@@ -2265,6 +2412,10 @@ int nand_scan(struct mtd_info *mtd, int 
 			chip->ecc.read_page = nand_read_page_hwecc;
 		if (!chip->ecc.write_page)
 			chip->ecc.write_page = nand_write_page_hwecc;
+		if (!chip->ecc.read_oob)
+			chip->ecc.read_oob = nand_read_oob_hwecc;
+		if (!chip->ecc.write_oob)
+			chip->ecc.write_oob = nand_write_oob_hwecc;
 
 	case NAND_ECC_HW_SYNDROME:
 		if (!chip->ecc.calculate || !chip->ecc.correct ||
@@ -2278,6 +2429,10 @@ int nand_scan(struct mtd_info *mtd, int 
 			chip->ecc.read_page = nand_read_page_syndrome;
 		if (!chip->ecc.write_page)
 			chip->ecc.write_page = nand_write_page_syndrome;
+		if (!chip->ecc.read_oob)
+			chip->ecc.read_oob = nand_read_oob_syndrome;
+		if (!chip->ecc.write_oob)
+			chip->ecc.write_oob = nand_write_oob_syndrome;
 
 		if (mtd->writesize >= chip->ecc.size)
 			break;
@@ -2291,6 +2446,8 @@ int nand_scan(struct mtd_info *mtd, int 
 		chip->ecc.correct = nand_correct_data;
 		chip->ecc.read_page = nand_read_page_swecc;
 		chip->ecc.write_page = nand_write_page_swecc;
+		chip->ecc.read_oob = nand_read_oob_swecc;
+		chip->ecc.write_oob = nand_write_oob_swecc;
 		chip->ecc.size = 256;
 		chip->ecc.bytes = 3;
 		break;
@@ -2300,6 +2457,8 @@ int nand_scan(struct mtd_info *mtd, int 
 		       "This is not recommended !!\n");
 		chip->ecc.read_page = nand_read_page_raw;
 		chip->ecc.write_page = nand_write_page_raw;
+		chip->ecc.read_oob = nand_read_oob_raw;
+		chip->ecc.write_oob = nand_write_oob_raw;
 		chip->ecc.size = mtd->writesize;
 		chip->ecc.bytes = 0;
 		break;
Index: linux-2.6/include/linux/mtd/nand.h
===================================================================
--- linux-2.6.orig/include/linux/mtd/nand.h
+++ linux-2.6/include/linux/mtd/nand.h
@@ -250,6 +250,19 @@ struct nand_ecc_ctrl {
 	void			(*write_page)(struct mtd_info *mtd,
 					      struct nand_chip *chip,
 					      const uint8_t *buf);
+	void			(*read_oob)(struct mtd_info *mtd,
+					    struct nand_chip *chip,
+					    uint8_t *buf,
+					    int offs,
+					    int length,
+					    int page,
+					    int *sndcmd);
+	void			(*write_oob)(struct mtd_info *mtd,
+					     struct nand_chip *chip,
+					     const uint8_t *buf,
+					     int offs,
+					     int length,
+					     int page);
 };
 
 /**