Re: [U-Boot] [PATCH 2/2] MTD:NAND: ADD new ECC mode NAND_ECC_HW_OOB_FIRST
Sorry for the late comments. We have been trying to use this code with the associated davinci 4-bit ecc patches and have some questions (inline).
.....
uint8_t *ecc_code = chip->buffers->ecccode;uint32_t *eccpos = chip->ecc.layout->eccpos;uint8_t *ecc_calc = chip->buffers->ecccalc;/* Read the OOB area first */chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
What about chips that do not support the NAND_CMD_READOOB? Do I need to provide my own read routine for that case?
+ chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);for (i = 0; i < chip->ecc.total; i++)ecc_code[i] = chip->oob_poi[eccpos[i]];for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {int stat;chip->ecc.hwctl(mtd, NAND_ECC_READ);chip->read_buf(mtd, p, eccsize);chip->ecc.calculate(mtd, p, &ecc_calc[i]);
Here you calculate ecc then never use the result?
+
stat = chip->ecc.correct(mtd, p, &ecc_code[i], NULL);if (stat < 0)mtd->ecc_stats.failed++;elsemtd->ecc_stats.corrected += stat;}return 0;+}
John Rigby wrote:
Sorry for the late comments. We have been trying to use this code with the associated davinci 4-bit ecc patches and have some questions (inline).
..... + uint8_t *ecc_code = chip->buffers->ecccode; + uint32_t *eccpos = chip->ecc.layout->eccpos; + uint8_t *ecc_calc = chip->buffers->ecccalc; + + /* Read the OOB area first */ + chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);What about chips that do not support the NAND_CMD_READOOB? Do I need to provide my own read routine for that case?
cmdfunc is supposed to fix that up. This is already the case with existing code.
+ chip->read_buf(mtd, chip->oob_poi, mtd->oobsize); + chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page); + + for (i = 0; i < chip->ecc.total; i++) + ecc_code[i] = chip->oob_poi[eccpos[i]]; + + for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) { + int stat; + + chip->ecc.hwctl(mtd, NAND_ECC_READ); + chip->read_buf(mtd, p, eccsize); + chip->ecc.calculate(mtd, p, &ecc_calc[i]);Here you calculate ecc then never use the result?
Hmm, that looks wrong, both here and in the davinci driver. Are the two calls to nand_davinci_4bit_readecc reading different things? Does the calculate function have any side effects beyond producing data that is never used?
-Scott
John Rigby wrote:
Sorry for the late comments. We have been trying to use this code with the associated davinci 4-bit ecc patches and have some questions
We use this internally and it works. Are you having any issues because we don't see any!! J-C has to apply a patch and you will need that for this to work properly. That patch updates the DM355 Config
(inline).
..... + uint8_t *ecc_code = chip->buffers->ecccode; + uint32_t *eccpos = chip->ecc.layout->eccpos; + uint8_t *ecc_calc = chip->buffers->ecccalc; + + /* Read the OOB area first */ + chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);What about chips that do not support the NAND_CMD_READOOB? Do I need to provide my own read routine for that case?
cmdfunc is supposed to fix that up. This is already the case with existing code.
+ chip->read_buf(mtd, chip->oob_poi, mtd->oobsize); + chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page); + + for (i = 0; i < chip->ecc.total; i++) + ecc_code[i] = chip->oob_poi[eccpos[i]]; + + for (i = 0; eccsteps; eccsteps--, i += eccbytes, p +=eccsize) {
+ int stat; + + chip->ecc.hwctl(mtd, NAND_ECC_READ); + chip->read_buf(mtd, p, eccsize); + chip->ecc.calculate(mtd, p, &ecc_calc[i]);Here you calculate ecc then never use the result?
Hmm, that looks wrong, both here and in the davinci driver. Are the two calls to nand_davinci_4bit_readecc reading different things? Does the calculate function have any side effects beyond producing data that is never used?
Have you reads the patch description. Maybe that might help a bit
This patch adds the new mode NAND_ECC_HW_OOB_FIRST in the nand code to support 4-bit ECC on TI DaVinci devices with large page (up to 2K) NAND chips. This ECC mode is similar to NAND_ECC_HW, with the exception of read_page API that first reads the OOB area, reads the data in chunks, feeds the ECC from OOB area to the ECC hw engine and perform any correction on the data as per the ECC status reported by the engine.
-Scott _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Scott answered my question about chips that don't support NAND_CMD_READOOB, we need to take care of it in cmdfunc.
I still don't see why the calculate method is called. The results are ignored.
On Tue, Sep 1, 2009 at 10:03 AM, Paulraj, Sandeep s-paulraj@ti.com wrote:
John Rigby wrote:
Sorry for the late comments. We have been trying to use this code with the associated davinci 4-bit ecc patches and have some questions
We use this internally and it works. Are you having any issues because we don't see any!! J-C has to apply a patch and you will need that for this to work properly. That patch updates the DM355 Config
(inline).
..... + uint8_t *ecc_code = chip->buffers->ecccode; + uint32_t *eccpos = chip->ecc.layout->eccpos; + uint8_t *ecc_calc = chip->buffers->ecccalc; + + /* Read the OOB area first */ + chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);What about chips that do not support the NAND_CMD_READOOB? Do I need to provide my own read routine for that case?
cmdfunc is supposed to fix that up. This is already the case with existing code.
+ chip->read_buf(mtd, chip->oob_poi, mtd->oobsize); + chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page); + + for (i = 0; i < chip->ecc.total; i++) + ecc_code[i] = chip->oob_poi[eccpos[i]]; + + for (i = 0; eccsteps; eccsteps--, i += eccbytes, p +=eccsize) {
+ int stat; + + chip->ecc.hwctl(mtd, NAND_ECC_READ); + chip->read_buf(mtd, p, eccsize); + chip->ecc.calculate(mtd, p, &ecc_calc[i]);Here you calculate ecc then never use the result?
Hmm, that looks wrong, both here and in the davinci driver. Are the two calls to nand_davinci_4bit_readecc reading different things? Does the calculate function have any side effects beyond producing data that is never used?
Have you reads the patch description. Maybe that might help a bit
This patch adds the new mode NAND_ECC_HW_OOB_FIRST in the nand code to support 4-bit ECC on TI DaVinci devices with large page (up to 2K) NAND chips. This ECC mode is similar to NAND_ECC_HW, with the exception of read_page API that first reads the OOB area, reads the data in chunks, feeds the ECC from OOB area to the ECC hw engine and perform any correction on the data as per the ECC status reported by the engine.
-Scott _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Paulraj, Sandeep wrote:
John Rigby wrote:
Sorry for the late comments. We have been trying to use this code with the associated davinci 4-bit ecc patches and have some questions
We use this internally and it works. Are you having any issues because we don't see any!!
Calm down, just because it works doesn't mean nobody should ask questions about the code.
Here you calculate ecc then never use the result?
Hmm, that looks wrong, both here and in the davinci driver. Are the two calls to nand_davinci_4bit_readecc reading different things? Does the calculate function have any side effects beyond producing data that is never used?
Have you reads the patch description. Maybe that might help a bit
This patch adds the new mode NAND_ECC_HW_OOB_FIRST in the nand code to support 4-bit ECC on TI DaVinci devices with large page (up to 2K) NAND chips. This ECC mode is similar to NAND_ECC_HW, with the exception of read_page API that first reads the OOB area, reads the data in chunks, feeds the ECC from OOB area to the ECC hw engine and perform any correction on the data as per the ECC status reported by the engine.
Yes, I've read that. That doesn't explain why calculate_ecc is producing data, *after* everything has been read, that is never consumed by anything (hardware or software) AFAICT. It doesn't explain why the "generic" code is passing NULL to ecc.correct.
-Scott
Hi Scott and Sandeep,
As long as we're looking at these patches again I had a question about patch 1 of 2 in this series.
The following part of the patch added a definition for:
chip->ecc.read_page = nand_read_page_hwecc_oob_first
but since there aren't any "break" statements until "case NAND_ECC_SOFT:" it looks like the chip->ecc.read_page definition ends up being set to:
chip->ecc.read_page = nand_read_page_swecc.
Maybe this is explains why things seem to be working???
Best regards, Matt
+ case NAND_ECC_HW_OOB_FIRST: + /* Similar to NAND_ECC_HW, but a separate read_page handle */ + if (!chip->ecc.calculate || !chip->ecc.correct || + !chip->ecc.hwctl) { + printk(KERN_WARNING "No ECC functions supplied, " + "Hardware ECC not possible\n"); + BUG(); + } + if (!chip->ecc.read_page) + chip->ecc.read_page = nand_read_page_hwecc_oob_first; +
On Tuesday 01 September 2009 01:19:09 pm Scott Wood wrote:
Paulraj, Sandeep wrote:
John Rigby wrote:
Sorry for the late comments. We have been trying to use this code with the associated davinci 4-bit ecc patches and have some questions
We use this internally and it works. Are you having any issues because we don't see any!!
Calm down, just because it works doesn't mean nobody should ask questions about the code.
Here you calculate ecc then never use the result?
Hmm, that looks wrong, both here and in the davinci driver. Are the two calls to nand_davinci_4bit_readecc reading different things? Does the calculate function have any side effects beyond producing data that is never used?
Have you reads the patch description. Maybe that might help a bit
This patch adds the new mode NAND_ECC_HW_OOB_FIRST in the nand code to support 4-bit ECC on TI DaVinci devices with large page (up to 2K) NAND chips. This ECC mode is similar to NAND_ECC_HW, with the exception of read_page API that first reads the OOB area, reads the data in chunks, feeds the ECC from OOB area to the ECC hw engine and perform any correction on the data as per the ECC status reported by the engine.
Yes, I've read that. That doesn't explain why calculate_ecc is producing data, *after* everything has been read, that is never consumed by anything (hardware or software) AFAICT. It doesn't explain why the "generic" code is passing NULL to ecc.correct.
-Scott _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Matt Waddel wrote:
Hi Scott and Sandeep,
As long as we're looking at these patches again I had a question about patch 1 of 2 in this series.
The following part of the patch added a definition for:
chip->ecc.read_page = nand_read_page_hwecc_oob_first
but since there aren't any "break" statements until "case NAND_ECC_SOFT:" it looks like the chip->ecc.read_page definition ends up being set to:
chip->ecc.read_page = nand_read_page_swecc.
Maybe this is explains why things seem to be working???
No, that's OK -- it stops before that on the "if (mtd->writesize >= chip->ecc.size) break;" line (or if that condition is not true, it prints a warning that it is falling back on soft ecc).
-Scott
participants (4)
-
John Rigby -
Matt Waddel -
Paulraj, Sandeep -
Scott Wood