* [PATCH] NAND: fix reading/writing OOB for syndrome: next iteration
@ 2006-06-14 15:45 Vitaly Wool
2006-06-14 16:44 ` Thomas Gleixner
0 siblings, 1 reply; 4+ messages in thread
From: Vitaly Wool @ 2006-06-14 15:45 UTC (permalink / raw)
To: tglx; +Cc: linux-mtd
Hi Thomas,
please see the new version inlined.
Notable is that you can't optimize write_oob() parameters as you've proposed im #mtd because buf, offs and length are not always taken from ops.
I've tested this version for syndrome and for software-based ecc and it seemed to work well with both (given that flash_eraseall -j won't work now anyway due to the OOB corruption we've discussed before).
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,166 @@ 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, offs, 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 = chip->ecc.bytes + 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(int, 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 int nand_write_oob_raw(struct mtd_info *mtd, struct nand_chip *chip,
+ const uint8_t *buf, int offs, int length,
+ int page)
+{
+ int status = 0;
+
+ chip->cmdfunc(mtd, NAND_CMD_SEQIN, mtd->writesize + offs,
+ 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);
+
+ return status;
+}
+static int (*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 int (*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 int 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, status = 0, sndcmd = 0;
+
+ chip->cmdfunc(mtd, NAND_CMD_SEQIN,
+ eccsize + offs,
+ page);
+ for (i = 0; i < chip->ecc.steps; i++) {
+ if (offs < portion) {
+ if (sndcmd)
+ chip->cmdfunc(mtd, NAND_CMD_RNDIN,
+ eccsize + i * (eccsize + portion) + offs,
+ -1);
+ else
+ sndcmd = 1;
+ len = min_t(int, length, portion - offs);
+ chip->write_buf(mtd, bufpoi, len);
+ bufpoi += len;
+ length -= len;
+ offs = 0;
+ } 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);
+
+ return status & NAND_STATUS_FAIL ? -EIO : 0;
+}
+
+/**
* nand_do_read_oob - [Intern] NAND read out-of-band
* @mtd: MTD device structure
* @from: offset to read from
@@ -1122,12 +1283,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 +1753,16 @@ 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);
+ status = 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);
- }
-
- /* Send command to program the OOB data */
- chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
+ } else
+ status = chip->ecc.write_oob(mtd, chip, ops->oobbuf,
+ ops->ooboffs, ops->len, page & chip->pagemask);
- status = chip->waitfunc(mtd, chip, FL_WRITING);
+ if (status)
+ return status;
- /* 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 +2410,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 +2427,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 +2444,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 +2455,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
@@ -69,6 +69,7 @@ extern void nand_release (struct mtd_inf
#define NAND_CMD_STATUS 0x70
#define NAND_CMD_STATUS_MULTI 0x71
#define NAND_CMD_SEQIN 0x80
+#define NAND_CMD_RNDIN 0x85
#define NAND_CMD_READID 0x90
#define NAND_CMD_ERASE2 0xd0
#define NAND_CMD_RESET 0xff
@@ -250,6 +251,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);
+ int (*write_oob)(struct mtd_info *mtd,
+ struct nand_chip *chip,
+ const uint8_t *buf,
+ int offs,
+ int length,
+ int page);
};
/**
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] NAND: fix reading/writing OOB for syndrome: next iteration
2006-06-14 15:45 [PATCH] NAND: fix reading/writing OOB for syndrome: next iteration Vitaly Wool
@ 2006-06-14 16:44 ` Thomas Gleixner
2006-06-15 6:08 ` Vitaly Wool
0 siblings, 1 reply; 4+ messages in thread
From: Thomas Gleixner @ 2006-06-14 16:44 UTC (permalink / raw)
To: Vitaly Wool; +Cc: linux-mtd
On Wed, 2006-06-14 at 19:45 +0400, Vitaly Wool wrote:
> Hi Thomas,
>
> please see the new version inlined.
> Notable is that you can't optimize write_oob() parameters as you've proposed im #mtd because buf, offs and length are not always taken from ops.
> I've tested this version for syndrome and for software-based ecc and it seemed to work well with both (given that flash_eraseall -j won't work now anyway due to the OOB corruption we've discussed before).
Grrr. Fix you mail client.
> 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,166 @@ 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) {
*smdcmd ?
s/void nand_.../int nand.../
> + chip->cmdfunc(mtd, NAND_CMD_READOOB, offs, 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;
What is this for ? We can put nand_read_oob_raw into the chip->ecc.xxx
pointer directly
> +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;
> +
Same here
> +/**
> + * 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 = chip->ecc.bytes + 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++;
> + }
> + }
That breaks, if portion is < oobsize / ecc.steps and the offset is in
the last area. I gave you a hint for the correct code before.
> + chip->cmdfunc(mtd, NAND_CMD_READ0,
> + (i + 1) * chip->ecc.size + i * portion + offs,
> + page);
We should use the random in command for the second read, as it is faster
> + toread = min_t(int, length, portion - offs);
> + chip->read_buf(mtd, bufpoi, toread);
> + bufpoi += toread;
> + length -= toread;
> + offs = 0;
> + }
> + if (length > 0)
> + chip->read_buf(mtd, bufpoi, length);
When offset is far enough off, you need to reposition the flash
pointer.
> +}
> +
> +/**
> + * 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 int nand_write_oob_raw(struct mtd_info *mtd, struct nand_chip *chip,
> + const uint8_t *buf, int offs, int length,
> + int page)
> +{
> + int status = 0;
> +
> + chip->cmdfunc(mtd, NAND_CMD_SEQIN, mtd->writesize + offs,
> + 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);
> +
> + return status;
> +}
> +static int (*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 int (*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;
See above
> +/**
> + * 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 int 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, status = 0, sndcmd = 0;
> +
> + chip->cmdfunc(mtd, NAND_CMD_SEQIN,
> + eccsize + offs,
> + page);
One line please.
> + for (i = 0; i < chip->ecc.steps; i++) {
> + if (offs < portion) {
if (offs >= portion) {
offs -= portion;
continue;
}
......
> + if (sndcmd)
> + chip->cmdfunc(mtd, NAND_CMD_RNDIN,
> + eccsize + i * (eccsize + portion) + offs,
> + -1);
Move the calculation into a seperate line please
pos = .....;
chip->...(...., pos, -1);
> + else
> + sndcmd = 1;
> + len = min_t(int, length, portion - offs);
> + chip->write_buf(mtd, bufpoi, len);
> + bufpoi += len;
> + length -= len;
> + offs = 0;
> + } else
> + offs -= portion;
> + }
> + if (length > 0)
> + chip->write_buf(mtd, bufpoi, length);
Needs seperate positioning depending on offset and layout
> + 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);
> +
> + return status & NAND_STATUS_FAIL ? -EIO : 0;
Why is the pageprog command and the status wait in every implementation
and not in the calling code ?
> +}
> +
> +/**
> * nand_do_read_oob - [Intern] NAND read out-of-band
> * @mtd: MTD device structure
> * @from: offset to read from
> @@ -1122,12 +1283,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);
Maybe we should get rid of that direct stuff and just read oobsize into
the buffer and transfer it afterwards. Would make the code simpler and
the number of arguments smaller:
sndcmd = ecc.read_oob(mtd, page, sndcmd);
So we read the full oob into chip->buffers.oobrbuf and transfer it to
the callers buffer then.
> @@ -1596,28 +1753,16 @@ 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);
> + status = 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);
> - }
> -
> - /* Send command to program the OOB data */
> - chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
> + } else
> + status = chip->ecc.write_oob(mtd, chip, ops->oobbuf,
> + ops->ooboffs, ops->len, page & chip->pagemask);
Maybe we should think about the direct comment in read_oob here too. Not
sure though.
tglx
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] NAND: fix reading/writing OOB for syndrome: next iteration
2006-06-14 16:44 ` Thomas Gleixner
@ 2006-06-15 6:08 ` Vitaly Wool
0 siblings, 0 replies; 4+ messages in thread
From: Vitaly Wool @ 2006-06-15 6:08 UTC (permalink / raw)
To: tglx; +Cc: linux-mtd
Thomas Gleixner wrote:
> *smdcmd ?
>
> s/void nand_.../int nand.../
>
Agreed.
>
>> + chip->cmdfunc(mtd, NAND_CMD_READOOB, offs, 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;
>>
>
> What is this for ? We can put nand_read_oob_raw into the chip->ecc.xxx
> pointer directly
>
Agreed.
>> +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 = chip->ecc.bytes + 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++;
>> + }
>> + }
>>
>
> That breaks, if portion is < oobsize / ecc.steps and the offset is in
> the last area. I gave you a hint for the correct code before.
>
Yup, thanks for pointing that out.
>
>> + chip->cmdfunc(mtd, NAND_CMD_READ0,
>> + (i + 1) * chip->ecc.size + i * portion + offs,
>> + page);
>>
>
> We should use the random in command for the second read, as it is faster
>
Ok
>
>> + toread = min_t(int, length, portion - offs);
>> + chip->read_buf(mtd, bufpoi, toread);
>> + bufpoi += toread;
>> + length -= toread;
>> + offs = 0;
>> + }
>> + if (length > 0)
>> + chip->read_buf(mtd, bufpoi, length);
>>
>
> When offset is far enough off, you need to reposition the flash
> pointer.
>
Depends on the previous code
>> + 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);
>> +
>> + return status & NAND_STATUS_FAIL ? -EIO : 0;
>>
>
> Why is the pageprog command and the status wait in every implementation
> and not in the calling code ?
>
It's because the READ/READOOB and SEQIN commands are to be in every
implementation, so it's keeping the integrity.
Having SEQIN in every implementation and PAGEPROG elsewhere would have
been misleading IMHO.
Vitaly
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] NAND: fix reading/writing OOB for syndrome: next iteration
@ 2006-06-14 9:58 Vitaly Wool
0 siblings, 0 replies; 4+ messages in thread
From: Vitaly Wool @ 2006-06-14 9:58 UTC (permalink / raw)
To: tglx; +Cc: linux-mtd
Hi Thomas,
please see the new version inlined.
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,163 @@ 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, offs, 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 = chip->ecc.bytes + 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(int, 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 int nand_write_oob_raw(struct mtd_info *mtd, struct nand_chip *chip,
+ const uint8_t *buf, int offs, int length,
+ int page)
+{
+ int status = 0;
+
+ 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);
+
+ return status;
+}
+static int (*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 int (*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 int 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, status = 0;
+
+ chip->cmdfunc(mtd, NAND_CMD_SEQIN,
+ eccsize + offs,
+ page);
+ for (i = 0; i < chip->ecc.steps; i++) {
+ if (offs < portion) {
+ chip->cmdfunc(mtd, NAND_CMD_RNDIN,
+ eccsize + i * (eccsize + portion) + offs,
+ -1);
+ len = min_t(int, length, portion - offs);
+ chip->write_buf(mtd, bufpoi, len);
+ bufpoi += len;
+ length -= len;
+ offs = 0;
+ } 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);
+
+ return status & NAND_STATUS_FAIL ? -EIO : 0;
+}
+
+/**
* nand_do_read_oob - [Intern] NAND read out-of-band
* @mtd: MTD device structure
* @from: offset to read from
@@ -1122,12 +1279,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 +1749,16 @@ 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);
+ status = 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);
- }
+ } else
+ status = 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);
+ if (status)
+ return status;
- 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 +2406,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 +2423,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 +2440,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 +2451,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
@@ -69,6 +69,7 @@ extern void nand_release (struct mtd_inf
#define NAND_CMD_STATUS 0x70
#define NAND_CMD_STATUS_MULTI 0x71
#define NAND_CMD_SEQIN 0x80
+#define NAND_CMD_RNDIN 0x85
#define NAND_CMD_READID 0x90
#define NAND_CMD_ERASE2 0xd0
#define NAND_CMD_RESET 0xff
@@ -250,6 +251,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);
+ int (*write_oob)(struct mtd_info *mtd,
+ struct nand_chip *chip,
+ const uint8_t *buf,
+ int offs,
+ int length,
+ int page);
};
/**
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2006-06-15 6:07 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-06-14 15:45 [PATCH] NAND: fix reading/writing OOB for syndrome: next iteration Vitaly Wool
2006-06-14 16:44 ` Thomas Gleixner
2006-06-15 6:08 ` Vitaly Wool
-- strict thread matches above, loose matches on Subject: below --
2006-06-14 9:58 Vitaly Wool
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox