* [PATCH 0/2] mtd: nand: rework nand_ecc_ctrl interface for OOB
@ 2012-04-16 22:35 Brian Norris
2012-04-16 22:35 ` [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces Brian Norris
` (2 more replies)
0 siblings, 3 replies; 27+ messages in thread
From: Brian Norris @ 2012-04-16 22:35 UTC (permalink / raw)
To: linux-mtd
Cc: Viresh Kumar, Artem Bityutskiy, Nicolas Ferre, Vipin Kumar,
Laurent Pinchart, Florian Fainelli, Jamie Iles, Mike Dunn,
Bastian Hecht, Dmitry Eremin-Solenikov, Kevin Cernekee, Lei Wen,
Axel Lin, Li Yang, Jean-Christophe PLAGNIOL-VILLARD,
Armando Visconti, Liu Shuo, Thomas Gleixner, Scott Branden,
Artem Bityutskiy, Wolfram Sang, Huang Shijie, Shmulik Ladkani,
Jiandong Zheng, Brian Norris, David Woodhouse
Hello,
I made a proposal to change the nand_chip and nand_ecc_ctrl interfaces a few
weeks ago to address a problem I see with a new NAND controller I'm working
with. This new controller does not return OOB data easily when performing page
reads/writes. However, currently these interfaces assume that OOB data is
read/written to/from the nand_chip.oob_poi buffer, even when the high-level
user did not request OOB data. I need to break that assumption with my NAND
driver, and so I have developed the following changes to the NAND interfaces
(see patch descriptions for more info).
You can see the original proposal and a little discussion here:
http://lists.infradead.org/pipermail/linux-mtd/2012-March/040441.html
Note that I could not compile all the affected drivers, since some required
ARCH-specific builds that I am not familiar with.
Artem: can you perform your regular compile tests? I compile-tested as many as
I could.
Others: if you care about your driver, please compile test and review to be
sure I'm doing things safely for you. Because most in-kernel drivers seem to be
perfecly happy using nand_chip.oob_poi for OOB data unconditionally, I have not
struggled to port most of them to take advantage of this full change. However,
I might make an attempt eventually, if I can get the proper amount of review
support. I cannot test most of these drivers properly, and so I *need* your
reviewing and testing to prevent breakage.
Thanks for reviewing!
Brian
Brian Norris (2):
mtd: nand: add OOB argument to NAND {read,write}_page interfaces
mtd: nand: nand_do_{read,write}_ops - pass OOB buffer through
drivers/mtd/nand/atmel_nand.c | 7 +-
drivers/mtd/nand/bcm_umi_bch.c | 10 ++-
drivers/mtd/nand/bcm_umi_nand.c | 2 +-
drivers/mtd/nand/bf5xx_nand.c | 4 +-
drivers/mtd/nand/cafe_nand.c | 26 ++++---
drivers/mtd/nand/denali.c | 8 +-
drivers/mtd/nand/docg4.c | 12 ++--
drivers/mtd/nand/fsl_elbc_nand.c | 7 +-
drivers/mtd/nand/fsl_ifc_nand.c | 4 +-
drivers/mtd/nand/fsmc_nand.c | 3 +-
drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 5 +-
drivers/mtd/nand/nand_base.c | 131 ++++++++++++++++++--------------
drivers/mtd/nand/pxa3xx_nand.c | 4 +-
drivers/mtd/nand/sh_flctl.c | 4 +-
include/linux/mtd/nand.h | 11 ++-
15 files changed, 132 insertions(+), 106 deletions(-)
--
1.7.5.4.2.g519b1
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces 2012-04-16 22:35 [PATCH 0/2] mtd: nand: rework nand_ecc_ctrl interface for OOB Brian Norris @ 2012-04-16 22:35 ` Brian Norris 2012-04-17 7:50 ` Matthieu CASTET ` (2 more replies) 2012-04-16 22:35 ` [PATCH 2/2] mtd: nand: nand_do_{read, write}_ops - pass OOB buffer through Brian Norris 2012-04-25 14:16 ` [PATCH 0/2] mtd: nand: rework nand_ecc_ctrl interface for OOB Artem Bityutskiy 2 siblings, 3 replies; 27+ messages in thread From: Brian Norris @ 2012-04-16 22:35 UTC (permalink / raw) To: linux-mtd Cc: Viresh Kumar, Artem Bityutskiy, Nicolas Ferre, Vipin Kumar, Laurent Pinchart, Florian Fainelli, Jamie Iles, Mike Dunn, Bastian Hecht, Dmitry Eremin-Solenikov, Kevin Cernekee, Lei Wen, Axel Lin, Li Yang, Jean-Christophe PLAGNIOL-VILLARD, Armando Visconti, Liu Shuo, Thomas Gleixner, Scott Branden, Artem Bityutskiy, Wolfram Sang, Huang Shijie, Shmulik Ladkani, Jiandong Zheng, Brian Norris, David Woodhouse New NAND controllers can perform read/write via HW engines which don't expose OOB data in their DMA mode. To reflect this, we should rework the nand_chip / nand_ecc_ctrl interfaces that assume that drivers will always read/write OOB data in the nand_chip.oob_poi buffer. A better interface would pass the appropriate buffer explicitly when OOB data is requested and otherwise pass a NULL pointer, meaning that no OOB data is needed. This patch adds the 'oob' parameter to each relevant {read,write}_page interface; all 'oob' parameters are left unused and NULL for now. The next patch will set the parameter properly. Perhaps it should be utilized eventually on some of these drivers, but this is a slow process of hacking each driver function that uses these interfaces. I renamed many 'oob' buffers to 'oobbuf' so that we can use 'oob' as an argument to these interface functions. I think the consistency can help in the long run. Note: I couldn't compile-test all of these easily, as some had ARCH dependencies. Signed-off-by: Brian Norris <computersforpeace@gmail.com> --- drivers/mtd/nand/atmel_nand.c | 7 ++- drivers/mtd/nand/bcm_umi_bch.c | 10 ++- drivers/mtd/nand/bcm_umi_nand.c | 2 +- drivers/mtd/nand/bf5xx_nand.c | 4 +- drivers/mtd/nand/cafe_nand.c | 26 ++++--- drivers/mtd/nand/denali.c | 8 +- drivers/mtd/nand/docg4.c | 12 ++-- drivers/mtd/nand/fsl_elbc_nand.c | 7 +- drivers/mtd/nand/fsl_ifc_nand.c | 4 +- drivers/mtd/nand/fsmc_nand.c | 3 +- drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 5 +- drivers/mtd/nand/nand_base.c | 123 ++++++++++++++++++-------------- drivers/mtd/nand/pxa3xx_nand.c | 4 +- drivers/mtd/nand/sh_flctl.c | 4 +- include/linux/mtd/nand.h | 11 ++-- 15 files changed, 127 insertions(+), 103 deletions(-) diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c index 2165576..95838a6 100644 --- a/drivers/mtd/nand/atmel_nand.c +++ b/drivers/mtd/nand/atmel_nand.c @@ -324,18 +324,21 @@ static int atmel_nand_calculate(struct mtd_info *mtd, * mtd: mtd info structure * chip: nand chip info structure * buf: buffer to store read data + * oob: buffer to store read OOB data */ static int atmel_nand_read_page(struct mtd_info *mtd, - struct nand_chip *chip, uint8_t *buf, int page) + struct nand_chip *chip, uint8_t *buf, uint8_t *oob, int page) { int eccsize = chip->ecc.size; int eccbytes = chip->ecc.bytes; uint32_t *eccpos = chip->ecc.layout->eccpos; uint8_t *p = buf; - uint8_t *oob = chip->oob_poi; uint8_t *ecc_pos; int stat; + if (!oob) + oob = chip->oob_poi; + /* * Errata: ALE is incorrectly wired up to the ECC controller * on the AP7000, so it will include the address cycles in the diff --git a/drivers/mtd/nand/bcm_umi_bch.c b/drivers/mtd/nand/bcm_umi_bch.c index a930666..3f4e845 100644 --- a/drivers/mtd/nand/bcm_umi_bch.c +++ b/drivers/mtd/nand/bcm_umi_bch.c @@ -22,9 +22,9 @@ /* ---- Private Function Prototypes -------------------------------------- */ static int bcm_umi_bch_read_page_hwecc(struct mtd_info *mtd, - struct nand_chip *chip, uint8_t *buf, int page); + struct nand_chip *chip, uint8_t *buf, uint8_t *oob, int page); static void bcm_umi_bch_write_page_hwecc(struct mtd_info *mtd, - struct nand_chip *chip, const uint8_t *buf); + struct nand_chip *chip, const uint8_t *buf, const uint8_t *oob); /* ---- Private Variables ------------------------------------------------ */ @@ -103,11 +103,12 @@ static struct nand_ecclayout nand_hw_eccoob_4096 = { * @mtd: mtd info structure * @chip: nand chip info structure * @buf: buffer to store read data +* @oob: buffer to store read OOB data * ***************************************************************************/ static int bcm_umi_bch_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, uint8_t * buf, - int page) + uint8_t *oob, int page) { int sectorIdx = 0; int eccsize = chip->ecc.size; @@ -188,10 +189,11 @@ static int bcm_umi_bch_read_page_hwecc(struct mtd_info *mtd, * @mtd: mtd info structure * @chip: nand chip info structure * @buf: data buffer +* @oob: OOB buffer * ***************************************************************************/ static void bcm_umi_bch_write_page_hwecc(struct mtd_info *mtd, - struct nand_chip *chip, const uint8_t *buf) + struct nand_chip *chip, const uint8_t *buf, const uint8_t *oob) { int sectorIdx = 0; int eccsize = chip->ecc.size; diff --git a/drivers/mtd/nand/bcm_umi_nand.c b/drivers/mtd/nand/bcm_umi_nand.c index 6908cdd..ac7a027 100644 --- a/drivers/mtd/nand/bcm_umi_nand.c +++ b/drivers/mtd/nand/bcm_umi_nand.c @@ -341,7 +341,7 @@ static int bcm_umi_nand_verify_buf(struct mtd_info *mtd, const u_char * buf, * for MLC parts which may have permanently stuck bits. */ struct nand_chip *chip = mtd->priv; - int ret = chip->ecc.read_page(mtd, chip, readbackbuf, 0); + int ret = chip->ecc.read_page(mtd, chip, readbackbuf, NULL, 0); if (ret < 0) return -EFAULT; else { diff --git a/drivers/mtd/nand/bf5xx_nand.c b/drivers/mtd/nand/bf5xx_nand.c index d7b86b9..d25ab1a 100644 --- a/drivers/mtd/nand/bf5xx_nand.c +++ b/drivers/mtd/nand/bf5xx_nand.c @@ -558,7 +558,7 @@ static void bf5xx_nand_dma_write_buf(struct mtd_info *mtd, } static int bf5xx_nand_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip, - uint8_t *buf, int page) + uint8_t *buf, uint8_t *oob, int page) { bf5xx_nand_read_buf(mtd, buf, mtd->writesize); bf5xx_nand_read_buf(mtd, chip->oob_poi, mtd->oobsize); @@ -567,7 +567,7 @@ static int bf5xx_nand_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip } static void bf5xx_nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, - const uint8_t *buf) + const uint8_t *buf, const uint8_t *oob) { bf5xx_nand_write_buf(mtd, buf, mtd->writesize); bf5xx_nand_write_buf(mtd, chip->oob_poi, mtd->oobsize); diff --git a/drivers/mtd/nand/cafe_nand.c b/drivers/mtd/nand/cafe_nand.c index 2973d97..e4103c5 100644 --- a/drivers/mtd/nand/cafe_nand.c +++ b/drivers/mtd/nand/cafe_nand.c @@ -375,12 +375,13 @@ static int cafe_nand_read_oob(struct mtd_info *mtd, struct nand_chip *chip, * @mtd: mtd info structure * @chip: nand chip info structure * @buf: buffer to store read data + * @oob: buffer to store OOB data * * The hw generator calculates the error syndrome automatically. Therefor * we need a special oob layout and handling. */ static int cafe_nand_read_page(struct mtd_info *mtd, struct nand_chip *chip, - uint8_t *buf, int page) + uint8_t *buf, uint8_t *oob, int page) { struct cafe_priv *cafe = mtd->priv; @@ -394,7 +395,7 @@ static int cafe_nand_read_page(struct mtd_info *mtd, struct nand_chip *chip, if (checkecc && cafe_readl(cafe, NAND_ECC_RESULT) & (1<<18)) { unsigned short syn[8], pat[4]; int pos[4]; - u8 *oob = chip->oob_poi; + u8 *oobbuf = chip->oob_poi; int i, n; for (i=0; i<8; i+=2) { @@ -422,14 +423,14 @@ static int cafe_nand_read_page(struct mtd_info *mtd, struct nand_chip *chip, buf[0] ^= pat[i]; } else if (p == 1365) { buf[2047] ^= pat[i] >> 4; - oob[0] ^= pat[i] << 4; + oobbuf[0] ^= pat[i] << 4; } else if (p > 1365) { if ((p & 1) == 1) { - oob[3*p/2 - 2048] ^= pat[i] >> 4; - oob[3*p/2 - 2047] ^= pat[i] << 4; + oobbuf[3*p/2 - 2048] ^= pat[i] >> 4; + oobbuf[3*p/2 - 2047] ^= pat[i] << 4; } else { - oob[3*p/2 - 2049] ^= pat[i] >> 8; - oob[3*p/2 - 2048] ^= pat[i]; + oobbuf[3*p/2 - 2049] ^= pat[i] >> 8; + oobbuf[3*p/2 - 2048] ^= pat[i]; } } else if ((p & 1) == 1) { buf[3*p/2] ^= pat[i] >> 4; @@ -518,7 +519,9 @@ static struct nand_bbt_descr cafe_bbt_mirror_descr_512 = { static void cafe_nand_write_page_lowlevel(struct mtd_info *mtd, - struct nand_chip *chip, const uint8_t *buf) + struct nand_chip *chip, + const uint8_t *buf, + const uint8_t *oob) { struct cafe_priv *cafe = mtd->priv; @@ -530,16 +533,17 @@ static void cafe_nand_write_page_lowlevel(struct mtd_info *mtd, } static int cafe_nand_write_page(struct mtd_info *mtd, struct nand_chip *chip, - const uint8_t *buf, int page, int cached, int raw) + const uint8_t *buf, const uint8_t *oob, + int page, int cached, int raw) { int status; chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page); if (unlikely(raw)) - chip->ecc.write_page_raw(mtd, chip, buf); + chip->ecc.write_page_raw(mtd, chip, buf, oob); else - chip->ecc.write_page(mtd, chip, buf); + chip->ecc.write_page(mtd, chip, buf, oob); /* * Cached progamming disabled for now, Not sure if its worth the diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c index a1048c7..d078470 100644 --- a/drivers/mtd/nand/denali.c +++ b/drivers/mtd/nand/denali.c @@ -1084,7 +1084,7 @@ static void write_page(struct mtd_info *mtd, struct nand_chip *chip, * by write_page above. * */ static void denali_write_page(struct mtd_info *mtd, struct nand_chip *chip, - const uint8_t *buf) + const uint8_t *buf, const uint8_t *oob) { /* for regular page writes, we let HW handle all the ECC * data written to the device. */ @@ -1096,7 +1096,7 @@ static void denali_write_page(struct mtd_info *mtd, struct nand_chip *chip, * write_page() function above. */ static void denali_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, - const uint8_t *buf) + const uint8_t *buf, const uint8_t *oob) { /* for raw page writes, we want to disable ECC and simply write whatever data is in the buffer. */ @@ -1119,7 +1119,7 @@ static int denali_read_oob(struct mtd_info *mtd, struct nand_chip *chip, } static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip, - uint8_t *buf, int page) + uint8_t *buf, uint8_t *oob, int page) { struct denali_nand_info *denali = mtd_to_denali(mtd); @@ -1171,7 +1171,7 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip, } static int denali_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip, - uint8_t *buf, int page) + uint8_t *buf, uint8_t *oob, int page) { struct denali_nand_info *denali = mtd_to_denali(mtd); diff --git a/drivers/mtd/nand/docg4.c b/drivers/mtd/nand/docg4.c index b082026..7ff99c9 100644 --- a/drivers/mtd/nand/docg4.c +++ b/drivers/mtd/nand/docg4.c @@ -786,13 +786,13 @@ static int read_page(struct mtd_info *mtd, struct nand_chip *nand, static int docg4_read_page_raw(struct mtd_info *mtd, struct nand_chip *nand, - uint8_t *buf, int page) + uint8_t *buf, uint8_t *oob, int page) { return read_page(mtd, nand, buf, page, false); } static int docg4_read_page(struct mtd_info *mtd, struct nand_chip *nand, - uint8_t *buf, int page) + uint8_t *buf, uint8_t *oob, int page) { return read_page(mtd, nand, buf, page, true); } @@ -952,13 +952,13 @@ static void write_page(struct mtd_info *mtd, struct nand_chip *nand, } static void docg4_write_page_raw(struct mtd_info *mtd, struct nand_chip *nand, - const uint8_t *buf) + const uint8_t *buf, const uint8_t *oob) { return write_page(mtd, nand, buf, false); } static void docg4_write_page(struct mtd_info *mtd, struct nand_chip *nand, - const uint8_t *buf) + const uint8_t *buf, const uint8_t *oob) { return write_page(mtd, nand, buf, true); } @@ -1002,7 +1002,7 @@ static int __init read_factory_bbt(struct mtd_info *mtd) return -ENOMEM; read_page_prologue(mtd, g4_addr); - status = docg4_read_page(mtd, nand, buf, DOCG4_FACTORY_BBT_PAGE); + status = docg4_read_page(mtd, nand, buf, NULL, DOCG4_FACTORY_BBT_PAGE); if (status) goto exit; @@ -1079,7 +1079,7 @@ static int docg4_block_markbad(struct mtd_info *mtd, loff_t ofs) /* write first page of block */ write_page_prologue(mtd, g4_addr); - docg4_write_page(mtd, nand, buf); + docg4_write_page(mtd, nand, buf, nand->oob_poi); ret = pageprog(mtd); if (!ret) mtd->ecc_stats.badblocks++; diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c index 80b5264..b7d9bfd 100644 --- a/drivers/mtd/nand/fsl_elbc_nand.c +++ b/drivers/mtd/nand/fsl_elbc_nand.c @@ -740,7 +740,7 @@ static int fsl_elbc_chip_init_tail(struct mtd_info *mtd) static int fsl_elbc_read_page(struct mtd_info *mtd, struct nand_chip *chip, - uint8_t *buf, + uint8_t *buf, uint8_t *oob, int page) { fsl_elbc_read_buf(mtd, buf, mtd->writesize); @@ -755,9 +755,8 @@ static int fsl_elbc_read_page(struct mtd_info *mtd, /* ECC will be calculated automatically, and errors will be detected in * waitfunc. */ -static void fsl_elbc_write_page(struct mtd_info *mtd, - struct nand_chip *chip, - const uint8_t *buf) +static void fsl_elbc_write_page(struct mtd_info *mtd, struct nand_chip *chip, + const uint8_t *buf, const uint8_t *oob) { fsl_elbc_write_buf(mtd, buf, mtd->writesize); fsl_elbc_write_buf(mtd, chip->oob_poi, mtd->oobsize); diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c index c30ac7b..f8a6e3e 100644 --- a/drivers/mtd/nand/fsl_ifc_nand.c +++ b/drivers/mtd/nand/fsl_ifc_nand.c @@ -684,7 +684,7 @@ static int fsl_ifc_wait(struct mtd_info *mtd, struct nand_chip *chip) static int fsl_ifc_read_page(struct mtd_info *mtd, struct nand_chip *chip, - uint8_t *buf, int page) + uint8_t *buf, uint8_t *oob, int page) { struct fsl_ifc_mtd *priv = chip->priv; struct fsl_ifc_ctrl *ctrl = priv->ctrl; @@ -706,7 +706,7 @@ static int fsl_ifc_read_page(struct mtd_info *mtd, */ static void fsl_ifc_write_page(struct mtd_info *mtd, struct nand_chip *chip, - const uint8_t *buf) + const uint8_t *buf, const uint8_t *oob) { fsl_ifc_write_buf(mtd, buf, mtd->writesize); fsl_ifc_write_buf(mtd, chip->oob_poi, mtd->oobsize); diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c index 1b8330e..8bb731b 100644 --- a/drivers/mtd/nand/fsmc_nand.c +++ b/drivers/mtd/nand/fsmc_nand.c @@ -692,6 +692,7 @@ static void fsmc_write_buf_dma(struct mtd_info *mtd, const uint8_t *buf, * @mtd: mtd info structure * @chip: nand chip info structure * @buf: buffer to store read data + * @oob: buffer to store read OOB data * @page: page number to read * * This routine is needed for fsmc version 8 as reading from NAND chip has to be @@ -701,7 +702,7 @@ static void fsmc_write_buf_dma(struct mtd_info *mtd, const uint8_t *buf, * max of 8 bits) */ static int fsmc_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, - uint8_t *buf, int page) + uint8_t *buf, uint8_t *oob, int page) { struct fsmc_nand_data *host = container_of(mtd, struct fsmc_nand_data, mtd); diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c index 75b1dde..949c761 100644 --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c @@ -841,7 +841,7 @@ static void block_mark_swapping(struct gpmi_nand_data *this, } static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip, - uint8_t *buf, int page) + uint8_t *buf, uint8_t *oob, int page) { struct gpmi_nand_data *this = chip->priv; struct bch_geometry *nfc_geo = &this->bch_geometry; @@ -928,7 +928,8 @@ exit_nfc: } static void gpmi_ecc_write_page(struct mtd_info *mtd, - struct nand_chip *chip, const uint8_t *buf) + struct nand_chip *chip, const uint8_t *buf, + const uint8_t *oob) { struct gpmi_nand_data *this = chip->priv; struct bch_geometry *nfc_geo = &this->bch_geometry; diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 47b19c0..95ba987 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -1066,12 +1066,13 @@ EXPORT_SYMBOL(nand_lock); * @mtd: mtd info structure * @chip: nand chip info structure * @buf: buffer to store read data + * @oob: buffer to store read OOB data * @page: page number to read * * Not for syndrome calculating ECC controllers, which use a special oob layout. */ static int nand_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip, - uint8_t *buf, int page) + uint8_t *buf, uint8_t *oob, int page) { chip->read_buf(mtd, buf, mtd->writesize); chip->read_buf(mtd, chip->oob_poi, mtd->oobsize); @@ -1083,17 +1084,18 @@ static int nand_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip, * @mtd: mtd info structure * @chip: nand chip info structure * @buf: buffer to store read data + * @oob: buffer to store read OOB * @page: page number to read * * We need a special oob layout and handling even when OOB isn't used. */ static int nand_read_page_raw_syndrome(struct mtd_info *mtd, struct nand_chip *chip, - uint8_t *buf, int page) + uint8_t *buf, uint8_t *oob, int page) { int eccsize = chip->ecc.size; int eccbytes = chip->ecc.bytes; - uint8_t *oob = chip->oob_poi; + uint8_t *oobbuf = chip->oob_poi; int steps, size; for (steps = chip->ecc.steps; steps > 0; steps--) { @@ -1101,22 +1103,22 @@ static int nand_read_page_raw_syndrome(struct mtd_info *mtd, buf += eccsize; if (chip->ecc.prepad) { - chip->read_buf(mtd, oob, chip->ecc.prepad); - oob += chip->ecc.prepad; + chip->read_buf(mtd, oobbuf, chip->ecc.prepad); + oobbuf += chip->ecc.prepad; } - chip->read_buf(mtd, oob, eccbytes); - oob += eccbytes; + chip->read_buf(mtd, oobbuf, eccbytes); + oobbuf += eccbytes; if (chip->ecc.postpad) { - chip->read_buf(mtd, oob, chip->ecc.postpad); - oob += chip->ecc.postpad; + chip->read_buf(mtd, oobbuf, chip->ecc.postpad); + oobbuf += chip->ecc.postpad; } } - size = mtd->oobsize - (oob - chip->oob_poi); + size = mtd->oobsize - (oobbuf - chip->oob_poi); if (size) - chip->read_buf(mtd, oob, size); + chip->read_buf(mtd, oobbuf, size); return 0; } @@ -1129,7 +1131,7 @@ static int nand_read_page_raw_syndrome(struct mtd_info *mtd, * @page: page number to read */ static int nand_read_page_swecc(struct mtd_info *mtd, struct nand_chip *chip, - uint8_t *buf, int page) + uint8_t *buf, uint8_t *oob, int page) { int i, eccsize = chip->ecc.size; int eccbytes = chip->ecc.bytes; @@ -1139,7 +1141,7 @@ static int nand_read_page_swecc(struct mtd_info *mtd, struct nand_chip *chip, uint8_t *ecc_code = chip->buffers->ecccode; uint32_t *eccpos = chip->ecc.layout->eccpos; - chip->ecc.read_page_raw(mtd, chip, buf, page); + chip->ecc.read_page_raw(mtd, chip, buf, oob, page); for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) chip->ecc.calculate(mtd, p, &ecc_calc[i]); @@ -1257,12 +1259,13 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip, * @mtd: mtd info structure * @chip: nand chip info structure * @buf: buffer to store read data + * @oob: buffer to store read OOB * @page: page number to read * * Not for syndrome calculating ECC controllers which need a special oob layout. */ static int nand_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, - uint8_t *buf, int page) + uint8_t *buf, uint8_t *oob, int page) { int i, eccsize = chip->ecc.size; int eccbytes = chip->ecc.bytes; @@ -1302,6 +1305,7 @@ static int nand_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, * @mtd: mtd info structure * @chip: nand chip info structure * @buf: buffer to store read data + * @oob: buffer to store read OOB * @page: page number to read * * Hardware ECC for large page chips, require OOB to be read first. For this @@ -1311,7 +1315,7 @@ static int nand_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, * the data area, by overwriting the NAND manufacturer bad block markings. */ static int nand_read_page_hwecc_oob_first(struct mtd_info *mtd, - struct nand_chip *chip, uint8_t *buf, int page) + struct nand_chip *chip, uint8_t *buf, uint8_t *oob, int page) { int i, eccsize = chip->ecc.size; int eccbytes = chip->ecc.bytes; @@ -1350,19 +1354,20 @@ static int nand_read_page_hwecc_oob_first(struct mtd_info *mtd, * @mtd: mtd info structure * @chip: nand chip info structure * @buf: buffer to store read data + * @oob: buffer to store read OOB * @page: page number to read * * The hw generator calculates the error syndrome automatically. Therefore we * need a special oob layout and handling. */ static int nand_read_page_syndrome(struct mtd_info *mtd, struct nand_chip *chip, - uint8_t *buf, int page) + uint8_t *buf, uint8_t *oob, int page) { int i, eccsize = chip->ecc.size; int eccbytes = chip->ecc.bytes; int eccsteps = chip->ecc.steps; uint8_t *p = buf; - uint8_t *oob = chip->oob_poi; + uint8_t *oobbuf = chip->oob_poi; for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) { int stat; @@ -1371,23 +1376,23 @@ static int nand_read_page_syndrome(struct mtd_info *mtd, struct nand_chip *chip, chip->read_buf(mtd, p, eccsize); if (chip->ecc.prepad) { - chip->read_buf(mtd, oob, chip->ecc.prepad); - oob += chip->ecc.prepad; + chip->read_buf(mtd, oobbuf, chip->ecc.prepad); + oobbuf += chip->ecc.prepad; } chip->ecc.hwctl(mtd, NAND_ECC_READSYN); - chip->read_buf(mtd, oob, eccbytes); - stat = chip->ecc.correct(mtd, p, oob, NULL); + chip->read_buf(mtd, oobbuf, eccbytes); + stat = chip->ecc.correct(mtd, p, oobbuf, NULL); if (stat < 0) mtd->ecc_stats.failed++; else mtd->ecc_stats.corrected += stat; - oob += eccbytes; + oobbuf += eccbytes; if (chip->ecc.postpad) { - chip->read_buf(mtd, oob, chip->ecc.postpad); + chip->read_buf(mtd, oobbuf, chip->ecc.postpad); oob += chip->ecc.postpad; } } @@ -1500,14 +1505,14 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from, /* Now read the page into the buffer */ if (unlikely(ops->mode == MTD_OPS_RAW)) - ret = chip->ecc.read_page_raw(mtd, chip, - bufpoi, page); + ret = chip->ecc.read_page_raw(mtd, chip, bufpoi, + NULL, page); else if (!aligned && NAND_SUBPAGE_READ(chip) && !oob) ret = chip->ecc.read_subpage(mtd, chip, col, bytes, bufpoi); else ret = chip->ecc.read_page(mtd, chip, bufpoi, - page); + NULL, page); if (ret < 0) { if (!aligned) /* Invalidate page cache */ @@ -1919,11 +1924,12 @@ out: * @mtd: mtd info structure * @chip: nand chip info structure * @buf: data buffer + * @oob: OOB buffer * * Not for syndrome calculating ECC controllers, which use a special oob layout. */ static void nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, - const uint8_t *buf) + const uint8_t *buf, const uint8_t *oob) { chip->write_buf(mtd, buf, mtd->writesize); chip->write_buf(mtd, chip->oob_poi, mtd->oobsize); @@ -1934,16 +1940,18 @@ static void nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, * @mtd: mtd info structure * @chip: nand chip info structure * @buf: data buffer + * @oob: OOB buffer * * We need a special oob layout and handling even when ECC isn't checked. */ static void nand_write_page_raw_syndrome(struct mtd_info *mtd, struct nand_chip *chip, - const uint8_t *buf) + const uint8_t *buf, + const uint8_t *oob) { int eccsize = chip->ecc.size; int eccbytes = chip->ecc.bytes; - uint8_t *oob = chip->oob_poi; + uint8_t *oobbuf = chip->oob_poi; int steps, size; for (steps = chip->ecc.steps; steps > 0; steps--) { @@ -1951,31 +1959,32 @@ static void nand_write_page_raw_syndrome(struct mtd_info *mtd, buf += eccsize; if (chip->ecc.prepad) { - chip->write_buf(mtd, oob, chip->ecc.prepad); - oob += chip->ecc.prepad; + chip->write_buf(mtd, oobbuf, chip->ecc.prepad); + oobbuf += chip->ecc.prepad; } - chip->read_buf(mtd, oob, eccbytes); - oob += eccbytes; + chip->read_buf(mtd, oobbuf, eccbytes); + oobbuf += eccbytes; if (chip->ecc.postpad) { - chip->write_buf(mtd, oob, chip->ecc.postpad); - oob += chip->ecc.postpad; + chip->write_buf(mtd, oobbuf, chip->ecc.postpad); + oobbuf += chip->ecc.postpad; } } - size = mtd->oobsize - (oob - chip->oob_poi); + size = mtd->oobsize - (oobbuf - chip->oob_poi); if (size) - chip->write_buf(mtd, oob, size); + chip->write_buf(mtd, oobbuf, size); } /** * nand_write_page_swecc - [REPLACEABLE] software ECC based page write function * @mtd: mtd info structure * @chip: nand chip info structure * @buf: data buffer + * @oob: OOB buffer */ static void nand_write_page_swecc(struct mtd_info *mtd, struct nand_chip *chip, - const uint8_t *buf) + const uint8_t *buf, const uint8_t *oob) { int i, eccsize = chip->ecc.size; int eccbytes = chip->ecc.bytes; @@ -1991,7 +2000,7 @@ static void nand_write_page_swecc(struct mtd_info *mtd, struct nand_chip *chip, for (i = 0; i < chip->ecc.total; i++) chip->oob_poi[eccpos[i]] = ecc_calc[i]; - chip->ecc.write_page_raw(mtd, chip, buf); + chip->ecc.write_page_raw(mtd, chip, buf, oob); } /** @@ -1999,9 +2008,10 @@ static void nand_write_page_swecc(struct mtd_info *mtd, struct nand_chip *chip, * @mtd: mtd info structure * @chip: nand chip info structure * @buf: data buffer + * @oob: OOB buffer */ static void nand_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, - const uint8_t *buf) + const uint8_t *buf, const uint8_t *oob) { int i, eccsize = chip->ecc.size; int eccbytes = chip->ecc.bytes; @@ -2027,18 +2037,20 @@ static void nand_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, * @mtd: mtd info structure * @chip: nand chip info structure * @buf: data buffer + * @oob: OOB data buffer * * The hw generator calculates the error syndrome automatically. Therefore we * need a special oob layout and handling. */ static void nand_write_page_syndrome(struct mtd_info *mtd, - struct nand_chip *chip, const uint8_t *buf) + struct nand_chip *chip, const uint8_t *buf, + const uint8_t *oob) { int i, eccsize = chip->ecc.size; int eccbytes = chip->ecc.bytes; int eccsteps = chip->ecc.steps; const uint8_t *p = buf; - uint8_t *oob = chip->oob_poi; + uint8_t *oobbuf = chip->oob_poi; for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) { @@ -2046,24 +2058,24 @@ static void nand_write_page_syndrome(struct mtd_info *mtd, chip->write_buf(mtd, p, eccsize); if (chip->ecc.prepad) { - chip->write_buf(mtd, oob, chip->ecc.prepad); - oob += chip->ecc.prepad; + chip->write_buf(mtd, oobbuf, chip->ecc.prepad); + oobbuf += chip->ecc.prepad; } - chip->ecc.calculate(mtd, p, oob); - chip->write_buf(mtd, oob, eccbytes); - oob += eccbytes; + chip->ecc.calculate(mtd, p, oobbuf); + chip->write_buf(mtd, oobbuf, eccbytes); + oobbuf += eccbytes; if (chip->ecc.postpad) { - chip->write_buf(mtd, oob, chip->ecc.postpad); - oob += chip->ecc.postpad; + chip->write_buf(mtd, oobbuf, chip->ecc.postpad); + oobbuf += chip->ecc.postpad; } } /* Calculate remaining oob bytes */ - i = mtd->oobsize - (oob - chip->oob_poi); + i = mtd->oobsize - (oobbuf - chip->oob_poi); if (i) - chip->write_buf(mtd, oob, i); + chip->write_buf(mtd, oobbuf, i); } /** @@ -2076,16 +2088,17 @@ static void nand_write_page_syndrome(struct mtd_info *mtd, * @raw: use _raw version of write_page */ static int nand_write_page(struct mtd_info *mtd, struct nand_chip *chip, - const uint8_t *buf, int page, int cached, int raw) + const uint8_t *buf, const uint8_t *oob, int page, + int cached, int raw) { int status; chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page); if (unlikely(raw)) - chip->ecc.write_page_raw(mtd, chip, buf); + chip->ecc.write_page_raw(mtd, chip, buf, oob); else - chip->ecc.write_page(mtd, chip, buf); + chip->ecc.write_page(mtd, chip, buf, oob); /* * Cached progamming disabled for now. Not sure if it's worth the @@ -2264,7 +2277,7 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to, memset(chip->oob_poi, 0xff, mtd->oobsize); } - ret = chip->write_page(mtd, chip, wbuf, page, cached, + ret = chip->write_page(mtd, chip, wbuf, NULL, page, cached, (ops->mode == MTD_OPS_RAW)); if (ret) break; diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c index def50ca..92d2eae 100644 --- a/drivers/mtd/nand/pxa3xx_nand.c +++ b/drivers/mtd/nand/pxa3xx_nand.c @@ -682,14 +682,14 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command, } static void pxa3xx_nand_write_page_hwecc(struct mtd_info *mtd, - struct nand_chip *chip, const uint8_t *buf) + struct nand_chip *chip, const uint8_t *buf, const uint8_t *oob) { chip->write_buf(mtd, buf, mtd->writesize); chip->write_buf(mtd, chip->oob_poi, mtd->oobsize); } static int pxa3xx_nand_read_page_hwecc(struct mtd_info *mtd, - struct nand_chip *chip, uint8_t *buf, int page) + struct nand_chip *chip, uint8_t *buf, uint8_t *oob, int page) { struct pxa3xx_nand_host *host = mtd->priv; struct pxa3xx_nand_info *info = host->info_data; diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c index e9b2b26..d1b2731 100644 --- a/drivers/mtd/nand/sh_flctl.c +++ b/drivers/mtd/nand/sh_flctl.c @@ -344,7 +344,7 @@ static void set_cmd_regs(struct mtd_info *mtd, uint32_t cmd, uint32_t flcmcdr_va } static int flctl_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, - uint8_t *buf, int page) + uint8_t *buf, uint8_t *oob, int page) { int i, eccsize = chip->ecc.size; int eccbytes = chip->ecc.bytes; @@ -366,7 +366,7 @@ static int flctl_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, } static void flctl_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, - const uint8_t *buf) + const uint8_t *buf, const uint8_t *oob) { int i, eccsize = chip->ecc.size; int eccbytes = chip->ecc.bytes; diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h index 1482340..be9ee1f 100644 --- a/include/linux/mtd/nand.h +++ b/include/linux/mtd/nand.h @@ -363,15 +363,15 @@ struct nand_ecc_ctrl { int (*correct)(struct mtd_info *mtd, uint8_t *dat, uint8_t *read_ecc, uint8_t *calc_ecc); int (*read_page_raw)(struct mtd_info *mtd, struct nand_chip *chip, - uint8_t *buf, int page); + uint8_t *buf, uint8_t *oob, int page); void (*write_page_raw)(struct mtd_info *mtd, struct nand_chip *chip, - const uint8_t *buf); + const uint8_t *buf, const uint8_t *oob); int (*read_page)(struct mtd_info *mtd, struct nand_chip *chip, - uint8_t *buf, int page); + uint8_t *buf, uint8_t *oob, int page); int (*read_subpage)(struct mtd_info *mtd, struct nand_chip *chip, uint32_t offs, uint32_t len, uint8_t *buf); void (*write_page)(struct mtd_info *mtd, struct nand_chip *chip, - const uint8_t *buf); + const uint8_t *buf, const uint8_t *oob); int (*write_oob_raw)(struct mtd_info *mtd, struct nand_chip *chip, int page); int (*read_oob_raw)(struct mtd_info *mtd, struct nand_chip *chip, @@ -505,7 +505,8 @@ struct nand_chip { int (*errstat)(struct mtd_info *mtd, struct nand_chip *this, int state, int status, int page); int (*write_page)(struct mtd_info *mtd, struct nand_chip *chip, - const uint8_t *buf, int page, int cached, int raw); + const uint8_t *buf, const uint8_t *oob, int page, + int cached, int raw); int chip_delay; unsigned int options; -- 1.7.5.4.2.g519b1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces 2012-04-16 22:35 ` [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces Brian Norris @ 2012-04-17 7:50 ` Matthieu CASTET 2012-04-18 3:44 ` Brian Norris 2012-04-17 14:29 ` [PATCH 1/2] mtd: nand: add OOB argument to NAND {read,write}_page interfaces Shmulik Ladkani 2012-04-18 7:56 ` Bastian Hecht 2 siblings, 1 reply; 27+ messages in thread From: Matthieu CASTET @ 2012-04-17 7:50 UTC (permalink / raw) To: Brian Norris Cc: Viresh Kumar, Artem Bityutskiy, Nicolas Ferre, Vipin Kumar, linux-mtd, Laurent Pinchart, Florian Fainelli, Lei Wen, Jamie Iles, Mike Dunn, Dmitry Eremin-Solenikov, Kevin Cernekee, Bastian Hecht, Axel Lin, Li Yang, Jean-Christophe PLAGNIOL-VILLARD, Armando Visconti, Liu Shuo, Thomas Gleixner, Scott Branden, Artem Bityutskiy, Wolfram Sang, Huang Shijie, Shmulik Ladkani, Jiandong Zheng, David Woodhouse Brian Norris a écrit : > New NAND controllers can perform read/write via HW engines which don't expose > OOB data in their DMA mode. To reflect this, we should rework the nand_chip / > nand_ecc_ctrl interfaces that assume that drivers will always read/write OOB > data in the nand_chip.oob_poi buffer. A better interface would pass the > appropriate buffer explicitly when OOB data is requested and otherwise pass a > NULL pointer, meaning that no OOB data is needed. If I understand correctly you propose that these driver will fetch oob (via pio mode) only when needed ? Do you have an example of such controller ? Matthieu ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces 2012-04-17 7:50 ` Matthieu CASTET @ 2012-04-18 3:44 ` Brian Norris 2012-04-19 16:50 ` Mike Dunn 0 siblings, 1 reply; 27+ messages in thread From: Brian Norris @ 2012-04-18 3:44 UTC (permalink / raw) To: Matthieu CASTET Cc: Viresh Kumar, Artem Bityutskiy, Nicolas Ferre, Vipin Kumar, linux-mtd, Laurent Pinchart, Florian Fainelli, Lei Wen, Jamie Iles, Mike Dunn, Dmitry Eremin-Solenikov, Kevin Cernekee, Bastian Hecht, Axel Lin, Li Yang, Jean-Christophe PLAGNIOL-VILLARD, Armando Visconti, Thomas Gleixner, Scott Branden, Artem Bityutskiy, Wolfram Sang, Huang Shijie, Shmulik Ladkani, Jiandong Zheng, David Woodhouse Hi Matthieu, On Tue, Apr 17, 2012 at 12:50 AM, Matthieu CASTET <matthieu.castet@parrot.com> wrote: > Brian Norris a écrit : >> New NAND controllers can perform read/write via HW engines which don't expose >> OOB data in their DMA mode. To reflect this, we should rework the nand_chip / >> nand_ecc_ctrl interfaces that assume that drivers will always read/write OOB >> data in the nand_chip.oob_poi buffer. A better interface would pass the >> appropriate buffer explicitly when OOB data is requested and otherwise pass a >> NULL pointer, meaning that no OOB data is needed. > > If I understand correctly you propose that these driver will fetch oob (via pio > mode) only when needed ? Sure, that's the idea. > Do you have an example of such controller ? Yes. I'm working with a new ASIC which has a DMA mode as well as a PIO mode. For DMA transactions (the preferred interface), I fill out a 64-byte descriptor (format specified below) then point the controller to that DRAM address via a register. The "command" field only supports page program, page read, and block erase, though. So for instance, if I want to perform a page program, I can only specify a single buffer (via dram_addr/dram_addr_ext) for the in-band data; the OOB data is automatically filled with 0xFF + HW ECC (calculated on the fly, internally, by HW controller). And on page read, I only receive the in-band data in my DRAM buffer. Now, in future revisions of this ASIC, it may be possible to access OOB via DMA as well, but even if this happens, it doesn't make a lot of sense on this hardware to *always* pull OOB data. As mentioned previously, most normal applications (i.e., UBI(FS)) don't need to access this OOB data at all, so it seems silly to go to extra work to have the DMA controller return it to the MTD/NAND layers. I'm not familiar with the OOB/ECC schemes on enough other hardware to determine whether other drivers could make use of this same optimization. It would require hardware with internal buffers for error correction and an access mode that easily returns page data only... Brian struct nand_dma_desc { __le32 next_desc; __le32 next_desc_ext; u8 type_E; u8 irq_swap; u8 __pad1; u8 command; __le32 dram_addr; __le32 dram_addr_ext; __le32 tfr_len; __le32 total_len; __le32 flash_addr; __le32 flash_addr_ext; __le32 cs; u32 __pad[5]; u8 valid; u8 ecc_status; u8 flash_status; u8 dma_status; } __attribute__((packed)); ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces 2012-04-18 3:44 ` Brian Norris @ 2012-04-19 16:50 ` Mike Dunn 2012-04-19 22:06 ` Brian Norris 0 siblings, 1 reply; 27+ messages in thread From: Mike Dunn @ 2012-04-19 16:50 UTC (permalink / raw) To: computersforpeace; +Cc: linux-mtd Hi Brian, On 04/17/2012 08:44 PM, Brian Norris wrote: > Now, in future revisions of this ASIC, it may be possible to access > OOB via DMA as well, but even if this happens, it doesn't make a lot > of sense on this hardware to *always* pull OOB data. No, it doesn't. In fact, I'm not aware of any code within or on top of mtd that does anything with the oob data when a page is read. If oob is needed, mtd_read_oob() is used. Coincidentally, I recxently discovered that my docg4 driver is technically broken because I don't fill the chip->oob_poi buffer when I read a page, but it never caused a problem with UBI/ubifs. And the mtdutils are fine because mtdchar requires use of an ioctl for oob access, and the handler for this ioctl uses mtd_read_oob(). > As mentioned > previously, most normal applications (i.e., UBI(FS)) don't need to > access this OOB data at all, so it seems silly to go to extra work to > have the DMA controller return it to the MTD/NAND layers. I'm not > familiar with the OOB/ECC schemes on enough other hardware to > determine whether other drivers could make use of this same > optimization. It would require hardware with internal buffers for > error correction and an access mode that easily returns page data > only... The Freescale nand controllers might fall into this category. Hardware handles error detction *and* correction, so there's no need to read the oob at all if it's not needed. And fsl_ifc_nand was just mainlined, BTW. Thanks, Mike ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces 2012-04-19 16:50 ` Mike Dunn @ 2012-04-19 22:06 ` Brian Norris 2012-04-20 1:10 ` Jon Povey ` (2 more replies) 0 siblings, 3 replies; 27+ messages in thread From: Brian Norris @ 2012-04-19 22:06 UTC (permalink / raw) To: Mike Dunn; +Cc: linux-mtd, Shmulik Ladkani, prabhakar Hi Mike, On Thu, Apr 19, 2012 at 9:50 AM, Mike Dunn <mikedunn@newsguy.com> wrote: > On 04/17/2012 08:44 PM, Brian Norris wrote: > >> Now, in future revisions of this ASIC, it may be possible to access >> OOB via DMA as well, but even if this happens, it doesn't make a lot >> of sense on this hardware to *always* pull OOB data. > > > No, it doesn't. In fact, I'm not aware of any code within or on top of mtd that > does anything with the oob data when a page is read. If oob is needed, > mtd_read_oob() is used. I guess this may not be an issue for page read, but I know one use for write_page data+OOB. MLC NAND, for instance, requires that you write *once* to a page, so I introduced ioctl(MEMWRITE) which generically allows page, OOB, or both to be written. This trickles down to the nand_ecc_ctrl.write_page function, I think. There are probably other cases that I'm not really thinking of right now. > Coincidentally, I recxently discovered that my docg4 > driver is technically broken because I don't fill the chip->oob_poi buffer when > I read a page, but it never caused a problem with UBI/ubifs. If, as you claim, chip->oob_poi is never used on page read, then why do you claim this is "broken" for docg4? (Note that I am not 100% sure of your claim. There are *potential* users through the mtd_read_oob() API, which can specify both oobbuf and datbuf.) > And the mtdutils > are fine because mtdchar requires use of an ioctl for oob access, and the > handler for this ioctl uses mtd_read_oob(). > >> As mentioned >> previously, most normal applications (i.e., UBI(FS)) don't need to >> access this OOB data at all, so it seems silly to go to extra work to >> have the DMA controller return it to the MTD/NAND layers. I'm not >> familiar with the OOB/ECC schemes on enough other hardware to >> determine whether other drivers could make use of this same >> optimization. It would require hardware with internal buffers for >> error correction and an access mode that easily returns page data >> only... > > > The Freescale nand controllers might fall into this category. Hardware handles > error detction *and* correction, so there's no need to read the oob at all if > it's not needed. And fsl_ifc_nand was just mainlined, BTW. Right, I noticed the new driver (merge in 3.4-rc, right?) but didn't study the details. From your description, it sounds like it could use this change. CC'ing Prabhakar, author of fsl_ifc_nand.c. And have you seen the thread (on patch 2/2) in which Shmulik suggests using a boolean (has_oob) argument instead of a buffer (oob) argument? Unless there are objections, I plan to rewrite v2 under his suggestion. Thanks for your thoughts! Brian P.S. It seems, Mike, like you dropped the CC list. This may or may not be intentional, but either way I suppose this discussion isn't particularly important for all the CC'd members... ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces 2012-04-19 22:06 ` Brian Norris @ 2012-04-20 1:10 ` Jon Povey 2012-04-20 16:25 ` Mike Dunn 2012-04-20 16:17 ` Mike Dunn 2012-04-22 7:58 ` Shmulik Ladkani 2 siblings, 1 reply; 27+ messages in thread From: Jon Povey @ 2012-04-20 1:10 UTC (permalink / raw) To: Brian Norris, Mike Dunn; +Cc: linux-mtd, Shmulik Ladkani, prabhakar linux-mtd-bounces@lists.infradead.org wrote: > {read, write}_page interfaces > > Hi Mike, > > On Thu, Apr 19, 2012 at 9:50 AM, Mike Dunn > <mikedunn@newsguy.com> wrote: >> On 04/17/2012 08:44 PM, Brian Norris wrote: >> >>> Now, in future revisions of this ASIC, it may be possible to access >>> OOB via DMA as well, but even if this happens, it doesn't make a lot >>> of sense on this hardware to *always* pull OOB data. >> >> >> No, it doesn't. In fact, I'm not aware of any code within or on top >> of mtd that does anything with the oob data when a page is read. If >> oob is needed, mtd_read_oob() is used. > > I guess this may not be an issue for page read, but I know one use for > write_page data+OOB. MLC NAND, for instance, requires that you write > *once* to a page, so I introduced ioctl(MEMWRITE) which generically > allows page, OOB, or both to be written. This trickles down to the > nand_ecc_ctrl.write_page function, I think. There are probably other > cases that I'm not really thinking of right now. Ability to write raw OOB including userland software-generated ECC is needed for Linux to be able to update the bootloader on certain TI DaVinci chips at least. This is because the ROM bootloader in the chip has a wacky/bugged idea of OOB layout. If you can do that you can also write MTD partitions in a different layout from the one you are supporting, e.g. as part of a firmware update to a kernel with a different filesystem and OOB layout. I have used this to good effect. I can think of a slightly obscure use for reading OOB on these TI chips: Once booted to Linux, you could read and check in software the ECC for the ROM bootloader looking for bitflips, and rewrite the bootloader if it was starting to degrade. The bootloader has no way to rewrite or report ECC corrections, you see. I have not done this myself. In theory nobody should ever need to worry about OOB+ECC but being able to fiddle with it has its uses and I hope that ability doesn't go away. -- Jon Povey jon.povey@racelogic.co.uk Racelogic is a limited company registered in England. Registered number 2743719 . Registered Office Unit 10, Swan Business Centre, Osier Way, Buckingham, Bucks, MK18 1TB . The information contained in this electronic mail transmission is intended by Racelogic Ltd for the use of the named individual or entity to which it is directed and may contain information that is confidential or privileged. If you have received this electronic mail transmission in error, please delete it from your system without copying or forwarding it, and notify the sender of the error by reply email so that the sender's address records can be corrected. The views expressed by the sender of this communication do not necessarily represent those of Racelogic Ltd. Please note that Racelogic reserves the right to monitor e-mail communications passing through its network ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces 2012-04-20 1:10 ` Jon Povey @ 2012-04-20 16:25 ` Mike Dunn 2012-04-20 19:19 ` Brian Norris 0 siblings, 1 reply; 27+ messages in thread From: Mike Dunn @ 2012-04-20 16:25 UTC (permalink / raw) To: Jon Povey; +Cc: Brian Norris, linux-mtd, Shmulik Ladkani, prabhakar On 04/19/2012 06:10 PM, Jon Povey wrote: > > In theory nobody should ever need to worry about OOB+ECC but being able > to fiddle with it has its uses and I hope that ability doesn't go away. I think we're just trying to figure out how to make it optional. Thanks, Mike ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces 2012-04-20 16:25 ` Mike Dunn @ 2012-04-20 19:19 ` Brian Norris 0 siblings, 0 replies; 27+ messages in thread From: Brian Norris @ 2012-04-20 19:19 UTC (permalink / raw) To: Mike Dunn, Jon Povey; +Cc: linux-mtd, Shmulik Ladkani, prabhakar On Fri, Apr 20, 2012 at 9:25 AM, Mike Dunn <mikedunn@newsguy.com> wrote: > On 04/19/2012 06:10 PM, Jon Povey wrote: >> >> In theory nobody should ever need to worry about OOB+ECC but being able >> to fiddle with it has its uses and I hope that ability doesn't go away. > > > I think we're just trying to figure out how to make it optional. Right, optional. Brian ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces 2012-04-19 22:06 ` Brian Norris 2012-04-20 1:10 ` Jon Povey @ 2012-04-20 16:17 ` Mike Dunn 2012-04-22 7:58 ` Shmulik Ladkani 2 siblings, 0 replies; 27+ messages in thread From: Mike Dunn @ 2012-04-20 16:17 UTC (permalink / raw) To: Brian Norris; +Cc: linux-mtd, Shmulik Ladkani, prabhakar Hi Brian, On 04/19/2012 03:06 PM, Brian Norris wrote: > Hi Mike, > > On Thu, Apr 19, 2012 at 9:50 AM, Mike Dunn <mikedunn@newsguy.com> wrote: >> On 04/17/2012 08:44 PM, Brian Norris wrote: >> >>> Now, in future revisions of this ASIC, it may be possible to access >>> OOB via DMA as well, but even if this happens, it doesn't make a lot >>> of sense on this hardware to *always* pull OOB data. >> >> >> No, it doesn't. In fact, I'm not aware of any code within or on top of mtd that >> does anything with the oob data when a page is read. If oob is needed, >> mtd_read_oob() is used. > > I guess this may not be an issue for page read, but I know one use for > write_page data+OOB. MLC NAND, for instance, requires that you write > *once* to a page, so I introduced ioctl(MEMWRITE) which generically > allows page, OOB, or both to be written. This trickles down to the > nand_ecc_ctrl.write_page function, I think. There are probably other > cases that I'm not really thinking of right now. I was just thinking of read. > >> Coincidentally, I recxently discovered that my docg4 >> driver is technically broken because I don't fill the chip->oob_poi buffer when >> I read a page, but it never caused a problem with UBI/ubifs. > > If, as you claim, chip->oob_poi is never used on page read, then why > do you claim this is "broken" for docg4? (Note that I am not 100% sure > of your claim. There are *potential* users through the mtd_read_oob() > API, which can specify both oobbuf and datbuf.) Broken because it's *supposed* to fill it, creating the *potential* users, as you point out. My point was just to illustrate that you are correct: the need to read oob along with the page data need not be assumed. > >> And the mtdutils >> are fine because mtdchar requires use of an ioctl for oob access, and the >> handler for this ioctl uses mtd_read_oob(). >> >>> As mentioned >>> previously, most normal applications (i.e., UBI(FS)) don't need to >>> access this OOB data at all, so it seems silly to go to extra work to >>> have the DMA controller return it to the MTD/NAND layers. I'm not >>> familiar with the OOB/ECC schemes on enough other hardware to >>> determine whether other drivers could make use of this same >>> optimization. It would require hardware with internal buffers for >>> error correction and an access mode that easily returns page data >>> only... >> >> >> The Freescale nand controllers might fall into this category. Hardware handles >> error detction *and* correction, so there's no need to read the oob at all if >> it's not needed. And fsl_ifc_nand was just mainlined, BTW. > > Right, I noticed the new driver (merge in 3.4-rc, right?) but didn't > study the details. From your description, it sounds like it could use > this change. CC'ing Prabhakar, author of fsl_ifc_nand.c. > > And have you seen the thread (on patch 2/2) in which Shmulik suggests > using a boolean (has_oob) argument instead of a buffer (oob) argument? > Unless there are objections, I plan to rewrite v2 under his > suggestion. Yes (after sending my post). I think it's a very good suggestion. Keeps the code from becoming even more confusing than it already is. > > Thanks for your thoughts! > > Brian > > P.S. It seems, Mike, like you dropped the CC list. This may or may not > be intentional, but either way I suppose this discussion isn't > particularly important for all the CC'd members... Done because my smtp server has a recipient limit of 20 :( Otherwise I would have "replied all". Thanks, Mike ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces 2012-04-19 22:06 ` Brian Norris 2012-04-20 1:10 ` Jon Povey 2012-04-20 16:17 ` Mike Dunn @ 2012-04-22 7:58 ` Shmulik Ladkani 2012-04-23 9:14 ` Bastian Hecht 2 siblings, 1 reply; 27+ messages in thread From: Shmulik Ladkani @ 2012-04-22 7:58 UTC (permalink / raw) To: Brian Norris, Mike Dunn; +Cc: linux-mtd, prabhakar On Thu, 19 Apr 2012 15:06:33 -0700 Brian Norris <computersforpeace@gmail.com> wrote: > On Thu, Apr 19, 2012 at 9:50 AM, Mike Dunn <mikedunn@newsguy.com> wrote: > > On 04/17/2012 08:44 PM, Brian Norris wrote: > > > >> Now, in future revisions of this ASIC, it may be possible to access > >> OOB via DMA as well, but even if this happens, it doesn't make a lot > >> of sense on this hardware to *always* pull OOB data. > > > > > > No, it doesn't. In fact, I'm not aware of any code within or on top of mtd that > > does anything with the oob data when a page is read. If oob is needed, > > mtd_read_oob() is used. > > I guess this may not be an issue for page read, but I know one use for > write_page data+OOB. MLC NAND, for instance, requires that you write > *once* to a page, so I introduced ioctl(MEMWRITE) which generically > allows page, OOB, or both to be written. This trickles down to the > nand_ecc_ctrl.write_page function, I think. There are probably other > cases that I'm not really thinking of right now. From MTD user's perspective, mtd_read() and mtd_write() interfaces do not expose the OOB to the user at all. OTHO, mtd_read_oob() and mtd_write_oob() allow the user to read/write either the OOB on its own (NULL 'ops->datbuf'), or the page data along with its accompanying OOB (non-null 'ops->datbuf' and 'ops->oobbuf'). There are few users of the mtd_read_oob/mtd_write_oob interfaces that provide a non-null 'ops->datbuf': - nand_bbt.c - MEMWRITE mtdchar ioctl - mtdswap.c - sm_ftl.c - yaffs2 (out-of-tree) Regards, Shmulik ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces 2012-04-22 7:58 ` Shmulik Ladkani @ 2012-04-23 9:14 ` Bastian Hecht 2012-04-23 17:14 ` Mike Dunn 2012-04-24 6:02 ` Shmulik Ladkani 0 siblings, 2 replies; 27+ messages in thread From: Bastian Hecht @ 2012-04-23 9:14 UTC (permalink / raw) To: Shmulik Ladkani; +Cc: linux-mtd, Brian Norris, Mike Dunn, prabhakar 2012/4/22 Shmulik Ladkani <shmulik.ladkani@gmail.com>: > On Thu, 19 Apr 2012 15:06:33 -0700 Brian Norris <computersforpeace@gmail.com> wrote: >> On Thu, Apr 19, 2012 at 9:50 AM, Mike Dunn <mikedunn@newsguy.com> wrote: >> > On 04/17/2012 08:44 PM, Brian Norris wrote: >> I guess this may not be an issue for page read, but I know one use for >> write_page data+OOB. MLC NAND, for instance, requires that you write >> *once* to a page, so I introduced ioctl(MEMWRITE) which generically >> allows page, OOB, or both to be written. This trickles down to the >> nand_ecc_ctrl.write_page function, I think. There are probably other >> cases that I'm not really thinking of right now. > > From MTD user's perspective, mtd_read() and mtd_write() interfaces > do not expose the OOB to the user at all. > > OTHO, mtd_read_oob() and mtd_write_oob() allow the user to read/write > either the OOB on its own (NULL 'ops->datbuf'), or the page data along > with its accompanying OOB (non-null 'ops->datbuf' and 'ops->oobbuf'). > > There are few users of the mtd_read_oob/mtd_write_oob interfaces that > provide a non-null 'ops->datbuf': > - nand_bbt.c > - MEMWRITE mtdchar ioctl > - mtdswap.c > - sm_ftl.c > - yaffs2 (out-of-tree) Following this thread, I wondered how mtd_write_oob is meant to work. The sh_flctl code simply writes the OOB data 1 to 1 into the oob area of the nand chip when using hardware ECC. So when reading this, it should result in a corrupted page. Is the driver meant to OR the ECC code to the buffer? Then the driver would need a page read and write it back applying the oob data, no? Best regards, Bastian Hecht > Regards, > Shmulik > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces 2012-04-23 9:14 ` Bastian Hecht @ 2012-04-23 17:14 ` Mike Dunn 2012-04-24 6:02 ` Shmulik Ladkani 1 sibling, 0 replies; 27+ messages in thread From: Mike Dunn @ 2012-04-23 17:14 UTC (permalink / raw) To: Bastian Hecht; +Cc: Brian Norris, linux-mtd, Shmulik Ladkani, prabhakar On 04/23/2012 02:14 AM, Bastian Hecht wrote: > > Following this thread, I wondered how mtd_write_oob is meant to work. > The sh_flctl code simply writes the OOB data 1 to 1 into the oob area > of the nand chip when using hardware ECC. So when reading this, it > should result in a corrupted page. Is the driver meant to OR the ECC > code to the buffer? Then the driver would need a page read and write > it back applying the oob data, no? Good question. I would guess that one-to-one placement is probably only appropriate for MTD_OPS_RAW. Mike ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces 2012-04-23 9:14 ` Bastian Hecht 2012-04-23 17:14 ` Mike Dunn @ 2012-04-24 6:02 ` Shmulik Ladkani 2012-04-25 13:17 ` Bastian Hecht 1 sibling, 1 reply; 27+ messages in thread From: Shmulik Ladkani @ 2012-04-24 6:02 UTC (permalink / raw) To: Bastian Hecht; +Cc: linux-mtd, Brian Norris, Mike Dunn, prabhakar Hi Bastian, On Mon, 23 Apr 2012 11:14:39 +0200 Bastian Hecht <hechtb@googlemail.com> wrote: > Following this thread, I wondered how mtd_write_oob is meant to work. > The sh_flctl code simply writes the OOB data 1 to 1 into the oob area > of the nand chip when using hardware ECC. So when reading this, it > should result in a corrupted page. Is the driver meant to OR the ECC > code to the buffer? Then the driver would need a page read and write > it back applying the oob data, no? The beahvior is dependent on mtd_oob_ops->mode. If mode is MTD_OPS_AUTO_OOB, 'nand_fill_oob' places the user provided 'oobbuf' into the "free" locations within the OOB layout (as defined by 'chip->ecc.layout->oobfree'). Implementor of 'chip->ecc.write_page' is expected to take care of ECC caclulation and fill the ECC bytes (driver's or HW responsibility). If the mode is MTD_OPS_RAW, user provided 'oobbuf' should be transferred as-is, without error correction ('chip->ecc.write_page_raw' will be invoked). If the mode is MTD_OPS_PLACE_OOB, then 'nand_fill_oob' copies user's 'oobbuf' onto 'oob_poi'. As with MTD_OPS_AUTO_OOB, implementor of 'chip->ecc.write_page' is expected to take care of ECC calculation; the ECC locations are assumed to be overwritten (ignoring user's bytes at the ECC locations). This is according to the nand_base.c default methods. For example, you can follow nand_do_write_ops, nand_write_page, nand_write_page_swecc. Regards, Shmulik ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces 2012-04-24 6:02 ` Shmulik Ladkani @ 2012-04-25 13:17 ` Bastian Hecht 0 siblings, 0 replies; 27+ messages in thread From: Bastian Hecht @ 2012-04-25 13:17 UTC (permalink / raw) To: Shmulik Ladkani; +Cc: linux-mtd, Brian Norris, Mike Dunn, prabhakar 2012/4/24 Shmulik Ladkani <shmulik.ladkani@gmail.com>: > Hi Bastian, > > On Mon, 23 Apr 2012 11:14:39 +0200 Bastian Hecht <hechtb@googlemail.com> wrote: >> Following this thread, I wondered how mtd_write_oob is meant to work. >> The sh_flctl code simply writes the OOB data 1 to 1 into the oob area >> of the nand chip when using hardware ECC. So when reading this, it >> should result in a corrupted page. Is the driver meant to OR the ECC >> code to the buffer? Then the driver would need a page read and write >> it back applying the oob data, no? Hello Schmulik, > The beahvior is dependent on mtd_oob_ops->mode. > > If mode is MTD_OPS_AUTO_OOB, 'nand_fill_oob' places the user provided > 'oobbuf' into the "free" locations within the OOB layout (as defined by > 'chip->ecc.layout->oobfree'). > Implementor of 'chip->ecc.write_page' is expected to take care of ECC > caclulation and fill the ECC bytes (driver's or HW responsibility). > > If the mode is MTD_OPS_RAW, user provided 'oobbuf' should be transferred > as-is, without error correction ('chip->ecc.write_page_raw' will be > invoked). Ahaa. Thanks for elaborating this, now I understand how things are supposed to work. You helped me to catch a flaw in the code, thanks! I have to introduce a flag or similar to turn off the on-the-fly ECC generation when the _raw functions are used. Best regards, Bastian Hecht > If the mode is MTD_OPS_PLACE_OOB, then 'nand_fill_oob' copies user's > 'oobbuf' onto 'oob_poi'. > As with MTD_OPS_AUTO_OOB, implementor of 'chip->ecc.write_page' is > expected to take care of ECC calculation; the ECC locations are assumed > to be overwritten (ignoring user's bytes at the ECC locations). > > This is according to the nand_base.c default methods. > For example, you can follow nand_do_write_ops, nand_write_page, > nand_write_page_swecc. > > Regards, > Shmulik ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] mtd: nand: add OOB argument to NAND {read,write}_page interfaces 2012-04-16 22:35 ` [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces Brian Norris 2012-04-17 7:50 ` Matthieu CASTET @ 2012-04-17 14:29 ` Shmulik Ladkani 2012-04-18 4:11 ` [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces Brian Norris 2012-04-18 7:56 ` Bastian Hecht 2 siblings, 1 reply; 27+ messages in thread From: Shmulik Ladkani @ 2012-04-17 14:29 UTC (permalink / raw) To: Brian Norris Cc: Viresh Kumar, Artem Bityutskiy, Nicolas Ferre, Vipin Kumar, linux-mtd, Laurent Pinchart, Florian Fainelli, Jamie Iles, Mike Dunn, Bastian Hecht, Dmitry Eremin-Solenikov, Kevin Cernekee, Lei Wen, Axel Lin, Li Yang, Jean-Christophe PLAGNIOL-VILLARD, Armando Visconti, Liu Shuo, Thomas Gleixner, Scott Branden, Artem Bityutskiy, Wolfram Sang, Huang Shijie, Jiandong Zheng, David Woodhouse Hi Brian, I have no inputs regarding the concept, others probably know better. Only verified execution in nand_base.c and nand.h, see below. On Mon, 16 Apr 2012 15:35:54 -0700 Brian Norris <computersforpeace@gmail.com> wrote: > static int nand_read_page_syndrome(struct mtd_info *mtd, struct nand_chip *chip, > - uint8_t *buf, int page) > + uint8_t *buf, uint8_t *oob, int page) > { > int i, eccsize = chip->ecc.size; > int eccbytes = chip->ecc.bytes; > int eccsteps = chip->ecc.steps; > uint8_t *p = buf; > - uint8_t *oob = chip->oob_poi; > + uint8_t *oobbuf = chip->oob_poi; > > for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) { > int stat; > @@ -1371,23 +1376,23 @@ static int nand_read_page_syndrome(struct mtd_info *mtd, struct nand_chip *chip, > chip->read_buf(mtd, p, eccsize); > > if (chip->ecc.prepad) { > - chip->read_buf(mtd, oob, chip->ecc.prepad); > - oob += chip->ecc.prepad; > + chip->read_buf(mtd, oobbuf, chip->ecc.prepad); > + oobbuf += chip->ecc.prepad; > } > > chip->ecc.hwctl(mtd, NAND_ECC_READSYN); > - chip->read_buf(mtd, oob, eccbytes); > - stat = chip->ecc.correct(mtd, p, oob, NULL); > + chip->read_buf(mtd, oobbuf, eccbytes); > + stat = chip->ecc.correct(mtd, p, oobbuf, NULL); > > if (stat < 0) > mtd->ecc_stats.failed++; > else > mtd->ecc_stats.corrected += stat; > > - oob += eccbytes; > + oobbuf += eccbytes; > > if (chip->ecc.postpad) { > - chip->read_buf(mtd, oob, chip->ecc.postpad); > + chip->read_buf(mtd, oobbuf, chip->ecc.postpad); > oob += chip->ecc.postpad; Missed convertion of 'oob' in the last line above. s/oob/oobbuf/ Same problem in the following lines of 'nand_read_page_syndrome'. Should be: @@ -1393,9 +1393,9 @@ static int nand_read_page_syndrome(struct mtd_info *mtd, struct nand_chip *chip, } /* Calculate remaining oob bytes */ - i = mtd->oobsize - (oob - chip->oob_poi); + i = mtd->oobsize - (oobbuf - chip->oob_poi); if (i) - chip->read_buf(mtd, oob, i); + chip->read_buf(mtd, oobbuf, i); return 0; } > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > index 1482340..be9ee1f 100644 > --- a/include/linux/mtd/nand.h > +++ b/include/linux/mtd/nand.h > @@ -363,15 +363,15 @@ struct nand_ecc_ctrl { > int (*correct)(struct mtd_info *mtd, uint8_t *dat, uint8_t *read_ecc, > uint8_t *calc_ecc); > int (*read_page_raw)(struct mtd_info *mtd, struct nand_chip *chip, > - uint8_t *buf, int page); > + uint8_t *buf, uint8_t *oob, int page); > void (*write_page_raw)(struct mtd_info *mtd, struct nand_chip *chip, > - const uint8_t *buf); > + const uint8_t *buf, const uint8_t *oob); > int (*read_page)(struct mtd_info *mtd, struct nand_chip *chip, > - uint8_t *buf, int page); > + uint8_t *buf, uint8_t *oob, int page); > int (*read_subpage)(struct mtd_info *mtd, struct nand_chip *chip, > uint32_t offs, uint32_t len, uint8_t *buf); No need to pass 'oob' to 'read_subpage'? Regards, Shmulik ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces 2012-04-17 14:29 ` [PATCH 1/2] mtd: nand: add OOB argument to NAND {read,write}_page interfaces Shmulik Ladkani @ 2012-04-18 4:11 ` Brian Norris 0 siblings, 0 replies; 27+ messages in thread From: Brian Norris @ 2012-04-18 4:11 UTC (permalink / raw) To: Shmulik Ladkani Cc: Viresh Kumar, Artem Bityutskiy, Nicolas Ferre, Vipin Kumar, linux-mtd, Laurent Pinchart, Florian Fainelli, Jamie Iles, Mike Dunn, Bastian Hecht, Dmitry Eremin-Solenikov, Kevin Cernekee, Lei Wen, Axel Lin, Li Yang, Jean-Christophe PLAGNIOL-VILLARD, Armando Visconti, Thomas Gleixner, Scott Branden, Artem Bityutskiy, Wolfram Sang, Huang Shijie, Jiandong Zheng, David Woodhouse Hi Shmulik, On Tue, Apr 17, 2012 at 7:29 AM, Shmulik Ladkani <shmulik.ladkani@gmail.com> wrote: > Only verified execution in nand_base.c and nand.h, see below. Thanks, this is helpful! > On Mon, 16 Apr 2012 15:35:54 -0700 Brian Norris <computersforpeace@gmail.com> wrote: >> @@ -1371,23 +1376,23 @@ static int nand_read_page_syndrome(struct mtd_info *mtd, struct nand_chip *chip, >> chip->read_buf(mtd, p, eccsize); >> >> if (chip->ecc.prepad) { >> - chip->read_buf(mtd, oob, chip->ecc.prepad); >> - oob += chip->ecc.prepad; >> + chip->read_buf(mtd, oobbuf, chip->ecc.prepad); >> + oobbuf += chip->ecc.prepad; >> } >> >> chip->ecc.hwctl(mtd, NAND_ECC_READSYN); >> - chip->read_buf(mtd, oob, eccbytes); >> - stat = chip->ecc.correct(mtd, p, oob, NULL); >> + chip->read_buf(mtd, oobbuf, eccbytes); >> + stat = chip->ecc.correct(mtd, p, oobbuf, NULL); >> >> if (stat < 0) >> mtd->ecc_stats.failed++; >> else >> mtd->ecc_stats.corrected += stat; >> >> - oob += eccbytes; >> + oobbuf += eccbytes; >> >> if (chip->ecc.postpad) { >> - chip->read_buf(mtd, oob, chip->ecc.postpad); >> + chip->read_buf(mtd, oobbuf, chip->ecc.postpad); >> oob += chip->ecc.postpad; > > Missed convertion of 'oob' in the last line above. > s/oob/oobbuf/ Yes, good catch. Thanks. > Same problem in the following lines of 'nand_read_page_syndrome'. > Should be: > > @@ -1393,9 +1393,9 @@ static int nand_read_page_syndrome(struct mtd_info *mtd, struct nand_chip *chip, > } > > /* Calculate remaining oob bytes */ > - i = mtd->oobsize - (oob - chip->oob_poi); > + i = mtd->oobsize - (oobbuf - chip->oob_poi); > if (i) > - chip->read_buf(mtd, oob, i); > + chip->read_buf(mtd, oobbuf, i); > > return 0; > } > >> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h >> index 1482340..be9ee1f 100644 >> --- a/include/linux/mtd/nand.h >> +++ b/include/linux/mtd/nand.h >> @@ -363,15 +363,15 @@ struct nand_ecc_ctrl { >> int (*correct)(struct mtd_info *mtd, uint8_t *dat, uint8_t *read_ecc, >> uint8_t *calc_ecc); >> int (*read_page_raw)(struct mtd_info *mtd, struct nand_chip *chip, >> - uint8_t *buf, int page); >> + uint8_t *buf, uint8_t *oob, int page); >> void (*write_page_raw)(struct mtd_info *mtd, struct nand_chip *chip, >> - const uint8_t *buf); >> + const uint8_t *buf, const uint8_t *oob); >> int (*read_page)(struct mtd_info *mtd, struct nand_chip *chip, >> - uint8_t *buf, int page); >> + uint8_t *buf, uint8_t *oob, int page); >> int (*read_subpage)(struct mtd_info *mtd, struct nand_chip *chip, >> uint32_t offs, uint32_t len, uint8_t *buf); > > No need to pass 'oob' to 'read_subpage'? No. It seems like 'read_subpage' is already assumed not to read OOB data; it only has one user, in nand_base.c, and this user has the condition '!oob'. (My driver simply avoids subpage read/write entirely, BTW, so I'm not too familiar with subpages.) Thanks a lot for the review! This lets me know I should take a closer look at my patch submissions...this pair of patches went through a lot of revision before sending, so the 'oob' vs. 'oobbuf' thing got a little convoluted. Brian ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces 2012-04-16 22:35 ` [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces Brian Norris 2012-04-17 7:50 ` Matthieu CASTET 2012-04-17 14:29 ` [PATCH 1/2] mtd: nand: add OOB argument to NAND {read,write}_page interfaces Shmulik Ladkani @ 2012-04-18 7:56 ` Bastian Hecht 2012-04-18 9:37 ` Bastian Hecht 2 siblings, 1 reply; 27+ messages in thread From: Bastian Hecht @ 2012-04-18 7:56 UTC (permalink / raw) To: Brian Norris Cc: Viresh Kumar, Artem Bityutskiy, Nicolas Ferre, Vipin Kumar, linux-mtd, Laurent Pinchart, Florian Fainelli, Jamie Iles, Mike Dunn, Bastian Hecht, Dmitry Eremin-Solenikov, Kevin Cernekee, Lei Wen, Axel Lin, Li Yang, Jean-Christophe PLAGNIOL-VILLARD, Armando Visconti, Liu Shuo, Thomas Gleixner, Scott Branden, Artem Bityutskiy, Wolfram Sang, Huang Shijie, Shmulik Ladkani, Jiandong Zheng, David Woodhouse Hello Brian, I'm currently working on the hardware ECC part of the SH Mobile flctl and I like the patchset as it makes things more explicit when OOB data is requested or not and such things. It's cleaner and better, thanks! Best regards, Bastian Hecht 2012/4/17 Brian Norris <computersforpeace@gmail.com>: > New NAND controllers can perform read/write via HW engines which don't expose > OOB data in their DMA mode. To reflect this, we should rework the nand_chip / > nand_ecc_ctrl interfaces that assume that drivers will always read/write OOB > data in the nand_chip.oob_poi buffer. A better interface would pass the > appropriate buffer explicitly when OOB data is requested and otherwise pass a > NULL pointer, meaning that no OOB data is needed. > > This patch adds the 'oob' parameter to each relevant {read,write}_page > interface; all 'oob' parameters are left unused and NULL for now. The next > patch will set the parameter properly. Perhaps it should be utilized eventually > on some of these drivers, but this is a slow process of hacking each driver > function that uses these interfaces. > > I renamed many 'oob' buffers to 'oobbuf' so that we can use 'oob' as an > argument to these interface functions. I think the consistency can help in the > long run. > > Note: I couldn't compile-test all of these easily, as some had ARCH > dependencies. > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > --- > drivers/mtd/nand/atmel_nand.c | 7 ++- > drivers/mtd/nand/bcm_umi_bch.c | 10 ++- > drivers/mtd/nand/bcm_umi_nand.c | 2 +- > drivers/mtd/nand/bf5xx_nand.c | 4 +- > drivers/mtd/nand/cafe_nand.c | 26 ++++--- > drivers/mtd/nand/denali.c | 8 +- > drivers/mtd/nand/docg4.c | 12 ++-- > drivers/mtd/nand/fsl_elbc_nand.c | 7 +- > drivers/mtd/nand/fsl_ifc_nand.c | 4 +- > drivers/mtd/nand/fsmc_nand.c | 3 +- > drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 5 +- > drivers/mtd/nand/nand_base.c | 123 ++++++++++++++++++-------------- > drivers/mtd/nand/pxa3xx_nand.c | 4 +- > drivers/mtd/nand/sh_flctl.c | 4 +- > include/linux/mtd/nand.h | 11 ++-- > 15 files changed, 127 insertions(+), 103 deletions(-) > > diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c > index 2165576..95838a6 100644 > --- a/drivers/mtd/nand/atmel_nand.c > +++ b/drivers/mtd/nand/atmel_nand.c > @@ -324,18 +324,21 @@ static int atmel_nand_calculate(struct mtd_info *mtd, > * mtd: mtd info structure > * chip: nand chip info structure > * buf: buffer to store read data > + * oob: buffer to store read OOB data > */ > static int atmel_nand_read_page(struct mtd_info *mtd, > - struct nand_chip *chip, uint8_t *buf, int page) > + struct nand_chip *chip, uint8_t *buf, uint8_t *oob, int page) > { > int eccsize = chip->ecc.size; > int eccbytes = chip->ecc.bytes; > uint32_t *eccpos = chip->ecc.layout->eccpos; > uint8_t *p = buf; > - uint8_t *oob = chip->oob_poi; > uint8_t *ecc_pos; > int stat; > > + if (!oob) > + oob = chip->oob_poi; > + > /* > * Errata: ALE is incorrectly wired up to the ECC controller > * on the AP7000, so it will include the address cycles in the > diff --git a/drivers/mtd/nand/bcm_umi_bch.c b/drivers/mtd/nand/bcm_umi_bch.c > index a930666..3f4e845 100644 > --- a/drivers/mtd/nand/bcm_umi_bch.c > +++ b/drivers/mtd/nand/bcm_umi_bch.c > @@ -22,9 +22,9 @@ > > /* ---- Private Function Prototypes -------------------------------------- */ > static int bcm_umi_bch_read_page_hwecc(struct mtd_info *mtd, > - struct nand_chip *chip, uint8_t *buf, int page); > + struct nand_chip *chip, uint8_t *buf, uint8_t *oob, int page); > static void bcm_umi_bch_write_page_hwecc(struct mtd_info *mtd, > - struct nand_chip *chip, const uint8_t *buf); > + struct nand_chip *chip, const uint8_t *buf, const uint8_t *oob); > > /* ---- Private Variables ------------------------------------------------ */ > > @@ -103,11 +103,12 @@ static struct nand_ecclayout nand_hw_eccoob_4096 = { > * @mtd: mtd info structure > * @chip: nand chip info structure > * @buf: buffer to store read data > +* @oob: buffer to store read OOB data > * > ***************************************************************************/ > static int bcm_umi_bch_read_page_hwecc(struct mtd_info *mtd, > struct nand_chip *chip, uint8_t * buf, > - int page) > + uint8_t *oob, int page) > { > int sectorIdx = 0; > int eccsize = chip->ecc.size; > @@ -188,10 +189,11 @@ static int bcm_umi_bch_read_page_hwecc(struct mtd_info *mtd, > * @mtd: mtd info structure > * @chip: nand chip info structure > * @buf: data buffer > +* @oob: OOB buffer > * > ***************************************************************************/ > static void bcm_umi_bch_write_page_hwecc(struct mtd_info *mtd, > - struct nand_chip *chip, const uint8_t *buf) > + struct nand_chip *chip, const uint8_t *buf, const uint8_t *oob) > { > int sectorIdx = 0; > int eccsize = chip->ecc.size; > diff --git a/drivers/mtd/nand/bcm_umi_nand.c b/drivers/mtd/nand/bcm_umi_nand.c > index 6908cdd..ac7a027 100644 > --- a/drivers/mtd/nand/bcm_umi_nand.c > +++ b/drivers/mtd/nand/bcm_umi_nand.c > @@ -341,7 +341,7 @@ static int bcm_umi_nand_verify_buf(struct mtd_info *mtd, const u_char * buf, > * for MLC parts which may have permanently stuck bits. > */ > struct nand_chip *chip = mtd->priv; > - int ret = chip->ecc.read_page(mtd, chip, readbackbuf, 0); > + int ret = chip->ecc.read_page(mtd, chip, readbackbuf, NULL, 0); > if (ret < 0) > return -EFAULT; > else { > diff --git a/drivers/mtd/nand/bf5xx_nand.c b/drivers/mtd/nand/bf5xx_nand.c > index d7b86b9..d25ab1a 100644 > --- a/drivers/mtd/nand/bf5xx_nand.c > +++ b/drivers/mtd/nand/bf5xx_nand.c > @@ -558,7 +558,7 @@ static void bf5xx_nand_dma_write_buf(struct mtd_info *mtd, > } > > static int bf5xx_nand_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip, > - uint8_t *buf, int page) > + uint8_t *buf, uint8_t *oob, int page) > { > bf5xx_nand_read_buf(mtd, buf, mtd->writesize); > bf5xx_nand_read_buf(mtd, chip->oob_poi, mtd->oobsize); > @@ -567,7 +567,7 @@ static int bf5xx_nand_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip > } > > static void bf5xx_nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, > - const uint8_t *buf) > + const uint8_t *buf, const uint8_t *oob) > { > bf5xx_nand_write_buf(mtd, buf, mtd->writesize); > bf5xx_nand_write_buf(mtd, chip->oob_poi, mtd->oobsize); > diff --git a/drivers/mtd/nand/cafe_nand.c b/drivers/mtd/nand/cafe_nand.c > index 2973d97..e4103c5 100644 > --- a/drivers/mtd/nand/cafe_nand.c > +++ b/drivers/mtd/nand/cafe_nand.c > @@ -375,12 +375,13 @@ static int cafe_nand_read_oob(struct mtd_info *mtd, struct nand_chip *chip, > * @mtd: mtd info structure > * @chip: nand chip info structure > * @buf: buffer to store read data > + * @oob: buffer to store OOB data > * > * The hw generator calculates the error syndrome automatically. Therefor > * we need a special oob layout and handling. > */ > static int cafe_nand_read_page(struct mtd_info *mtd, struct nand_chip *chip, > - uint8_t *buf, int page) > + uint8_t *buf, uint8_t *oob, int page) > { > struct cafe_priv *cafe = mtd->priv; > > @@ -394,7 +395,7 @@ static int cafe_nand_read_page(struct mtd_info *mtd, struct nand_chip *chip, > if (checkecc && cafe_readl(cafe, NAND_ECC_RESULT) & (1<<18)) { > unsigned short syn[8], pat[4]; > int pos[4]; > - u8 *oob = chip->oob_poi; > + u8 *oobbuf = chip->oob_poi; > int i, n; > > for (i=0; i<8; i+=2) { > @@ -422,14 +423,14 @@ static int cafe_nand_read_page(struct mtd_info *mtd, struct nand_chip *chip, > buf[0] ^= pat[i]; > } else if (p == 1365) { > buf[2047] ^= pat[i] >> 4; > - oob[0] ^= pat[i] << 4; > + oobbuf[0] ^= pat[i] << 4; > } else if (p > 1365) { > if ((p & 1) == 1) { > - oob[3*p/2 - 2048] ^= pat[i] >> 4; > - oob[3*p/2 - 2047] ^= pat[i] << 4; > + oobbuf[3*p/2 - 2048] ^= pat[i] >> 4; > + oobbuf[3*p/2 - 2047] ^= pat[i] << 4; > } else { > - oob[3*p/2 - 2049] ^= pat[i] >> 8; > - oob[3*p/2 - 2048] ^= pat[i]; > + oobbuf[3*p/2 - 2049] ^= pat[i] >> 8; > + oobbuf[3*p/2 - 2048] ^= pat[i]; > } > } else if ((p & 1) == 1) { > buf[3*p/2] ^= pat[i] >> 4; > @@ -518,7 +519,9 @@ static struct nand_bbt_descr cafe_bbt_mirror_descr_512 = { > > > static void cafe_nand_write_page_lowlevel(struct mtd_info *mtd, > - struct nand_chip *chip, const uint8_t *buf) > + struct nand_chip *chip, > + const uint8_t *buf, > + const uint8_t *oob) > { > struct cafe_priv *cafe = mtd->priv; > > @@ -530,16 +533,17 @@ static void cafe_nand_write_page_lowlevel(struct mtd_info *mtd, > } > > static int cafe_nand_write_page(struct mtd_info *mtd, struct nand_chip *chip, > - const uint8_t *buf, int page, int cached, int raw) > + const uint8_t *buf, const uint8_t *oob, > + int page, int cached, int raw) > { > int status; > > chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page); > > if (unlikely(raw)) > - chip->ecc.write_page_raw(mtd, chip, buf); > + chip->ecc.write_page_raw(mtd, chip, buf, oob); > else > - chip->ecc.write_page(mtd, chip, buf); > + chip->ecc.write_page(mtd, chip, buf, oob); > > /* > * Cached progamming disabled for now, Not sure if its worth the > diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c > index a1048c7..d078470 100644 > --- a/drivers/mtd/nand/denali.c > +++ b/drivers/mtd/nand/denali.c > @@ -1084,7 +1084,7 @@ static void write_page(struct mtd_info *mtd, struct nand_chip *chip, > * by write_page above. > * */ > static void denali_write_page(struct mtd_info *mtd, struct nand_chip *chip, > - const uint8_t *buf) > + const uint8_t *buf, const uint8_t *oob) > { > /* for regular page writes, we let HW handle all the ECC > * data written to the device. */ > @@ -1096,7 +1096,7 @@ static void denali_write_page(struct mtd_info *mtd, struct nand_chip *chip, > * write_page() function above. > */ > static void denali_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, > - const uint8_t *buf) > + const uint8_t *buf, const uint8_t *oob) > { > /* for raw page writes, we want to disable ECC and simply write > whatever data is in the buffer. */ > @@ -1119,7 +1119,7 @@ static int denali_read_oob(struct mtd_info *mtd, struct nand_chip *chip, > } > > static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip, > - uint8_t *buf, int page) > + uint8_t *buf, uint8_t *oob, int page) > { > struct denali_nand_info *denali = mtd_to_denali(mtd); > > @@ -1171,7 +1171,7 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip, > } > > static int denali_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip, > - uint8_t *buf, int page) > + uint8_t *buf, uint8_t *oob, int page) > { > struct denali_nand_info *denali = mtd_to_denali(mtd); > > diff --git a/drivers/mtd/nand/docg4.c b/drivers/mtd/nand/docg4.c > index b082026..7ff99c9 100644 > --- a/drivers/mtd/nand/docg4.c > +++ b/drivers/mtd/nand/docg4.c > @@ -786,13 +786,13 @@ static int read_page(struct mtd_info *mtd, struct nand_chip *nand, > > > static int docg4_read_page_raw(struct mtd_info *mtd, struct nand_chip *nand, > - uint8_t *buf, int page) > + uint8_t *buf, uint8_t *oob, int page) > { > return read_page(mtd, nand, buf, page, false); > } > > static int docg4_read_page(struct mtd_info *mtd, struct nand_chip *nand, > - uint8_t *buf, int page) > + uint8_t *buf, uint8_t *oob, int page) > { > return read_page(mtd, nand, buf, page, true); > } > @@ -952,13 +952,13 @@ static void write_page(struct mtd_info *mtd, struct nand_chip *nand, > } > > static void docg4_write_page_raw(struct mtd_info *mtd, struct nand_chip *nand, > - const uint8_t *buf) > + const uint8_t *buf, const uint8_t *oob) > { > return write_page(mtd, nand, buf, false); > } > > static void docg4_write_page(struct mtd_info *mtd, struct nand_chip *nand, > - const uint8_t *buf) > + const uint8_t *buf, const uint8_t *oob) > { > return write_page(mtd, nand, buf, true); > } > @@ -1002,7 +1002,7 @@ static int __init read_factory_bbt(struct mtd_info *mtd) > return -ENOMEM; > > read_page_prologue(mtd, g4_addr); > - status = docg4_read_page(mtd, nand, buf, DOCG4_FACTORY_BBT_PAGE); > + status = docg4_read_page(mtd, nand, buf, NULL, DOCG4_FACTORY_BBT_PAGE); > if (status) > goto exit; > > @@ -1079,7 +1079,7 @@ static int docg4_block_markbad(struct mtd_info *mtd, loff_t ofs) > > /* write first page of block */ > write_page_prologue(mtd, g4_addr); > - docg4_write_page(mtd, nand, buf); > + docg4_write_page(mtd, nand, buf, nand->oob_poi); > ret = pageprog(mtd); > if (!ret) > mtd->ecc_stats.badblocks++; > diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c > index 80b5264..b7d9bfd 100644 > --- a/drivers/mtd/nand/fsl_elbc_nand.c > +++ b/drivers/mtd/nand/fsl_elbc_nand.c > @@ -740,7 +740,7 @@ static int fsl_elbc_chip_init_tail(struct mtd_info *mtd) > > static int fsl_elbc_read_page(struct mtd_info *mtd, > struct nand_chip *chip, > - uint8_t *buf, > + uint8_t *buf, uint8_t *oob, > int page) > { > fsl_elbc_read_buf(mtd, buf, mtd->writesize); > @@ -755,9 +755,8 @@ static int fsl_elbc_read_page(struct mtd_info *mtd, > /* ECC will be calculated automatically, and errors will be detected in > * waitfunc. > */ > -static void fsl_elbc_write_page(struct mtd_info *mtd, > - struct nand_chip *chip, > - const uint8_t *buf) > +static void fsl_elbc_write_page(struct mtd_info *mtd, struct nand_chip *chip, > + const uint8_t *buf, const uint8_t *oob) > { > fsl_elbc_write_buf(mtd, buf, mtd->writesize); > fsl_elbc_write_buf(mtd, chip->oob_poi, mtd->oobsize); > diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c > index c30ac7b..f8a6e3e 100644 > --- a/drivers/mtd/nand/fsl_ifc_nand.c > +++ b/drivers/mtd/nand/fsl_ifc_nand.c > @@ -684,7 +684,7 @@ static int fsl_ifc_wait(struct mtd_info *mtd, struct nand_chip *chip) > > static int fsl_ifc_read_page(struct mtd_info *mtd, > struct nand_chip *chip, > - uint8_t *buf, int page) > + uint8_t *buf, uint8_t *oob, int page) > { > struct fsl_ifc_mtd *priv = chip->priv; > struct fsl_ifc_ctrl *ctrl = priv->ctrl; > @@ -706,7 +706,7 @@ static int fsl_ifc_read_page(struct mtd_info *mtd, > */ > static void fsl_ifc_write_page(struct mtd_info *mtd, > struct nand_chip *chip, > - const uint8_t *buf) > + const uint8_t *buf, const uint8_t *oob) > { > fsl_ifc_write_buf(mtd, buf, mtd->writesize); > fsl_ifc_write_buf(mtd, chip->oob_poi, mtd->oobsize); > diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c > index 1b8330e..8bb731b 100644 > --- a/drivers/mtd/nand/fsmc_nand.c > +++ b/drivers/mtd/nand/fsmc_nand.c > @@ -692,6 +692,7 @@ static void fsmc_write_buf_dma(struct mtd_info *mtd, const uint8_t *buf, > * @mtd: mtd info structure > * @chip: nand chip info structure > * @buf: buffer to store read data > + * @oob: buffer to store read OOB data > * @page: page number to read > * > * This routine is needed for fsmc version 8 as reading from NAND chip has to be > @@ -701,7 +702,7 @@ static void fsmc_write_buf_dma(struct mtd_info *mtd, const uint8_t *buf, > * max of 8 bits) > */ > static int fsmc_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, > - uint8_t *buf, int page) > + uint8_t *buf, uint8_t *oob, int page) > { > struct fsmc_nand_data *host = container_of(mtd, > struct fsmc_nand_data, mtd); > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > index 75b1dde..949c761 100644 > --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > @@ -841,7 +841,7 @@ static void block_mark_swapping(struct gpmi_nand_data *this, > } > > static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip, > - uint8_t *buf, int page) > + uint8_t *buf, uint8_t *oob, int page) > { > struct gpmi_nand_data *this = chip->priv; > struct bch_geometry *nfc_geo = &this->bch_geometry; > @@ -928,7 +928,8 @@ exit_nfc: > } > > static void gpmi_ecc_write_page(struct mtd_info *mtd, > - struct nand_chip *chip, const uint8_t *buf) > + struct nand_chip *chip, const uint8_t *buf, > + const uint8_t *oob) > { > struct gpmi_nand_data *this = chip->priv; > struct bch_geometry *nfc_geo = &this->bch_geometry; > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index 47b19c0..95ba987 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -1066,12 +1066,13 @@ EXPORT_SYMBOL(nand_lock); > * @mtd: mtd info structure > * @chip: nand chip info structure > * @buf: buffer to store read data > + * @oob: buffer to store read OOB data > * @page: page number to read > * > * Not for syndrome calculating ECC controllers, which use a special oob layout. > */ > static int nand_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip, > - uint8_t *buf, int page) > + uint8_t *buf, uint8_t *oob, int page) > { > chip->read_buf(mtd, buf, mtd->writesize); > chip->read_buf(mtd, chip->oob_poi, mtd->oobsize); > @@ -1083,17 +1084,18 @@ static int nand_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip, > * @mtd: mtd info structure > * @chip: nand chip info structure > * @buf: buffer to store read data > + * @oob: buffer to store read OOB > * @page: page number to read > * > * We need a special oob layout and handling even when OOB isn't used. > */ > static int nand_read_page_raw_syndrome(struct mtd_info *mtd, > struct nand_chip *chip, > - uint8_t *buf, int page) > + uint8_t *buf, uint8_t *oob, int page) > { > int eccsize = chip->ecc.size; > int eccbytes = chip->ecc.bytes; > - uint8_t *oob = chip->oob_poi; > + uint8_t *oobbuf = chip->oob_poi; > int steps, size; > > for (steps = chip->ecc.steps; steps > 0; steps--) { > @@ -1101,22 +1103,22 @@ static int nand_read_page_raw_syndrome(struct mtd_info *mtd, > buf += eccsize; > > if (chip->ecc.prepad) { > - chip->read_buf(mtd, oob, chip->ecc.prepad); > - oob += chip->ecc.prepad; > + chip->read_buf(mtd, oobbuf, chip->ecc.prepad); > + oobbuf += chip->ecc.prepad; > } > > - chip->read_buf(mtd, oob, eccbytes); > - oob += eccbytes; > + chip->read_buf(mtd, oobbuf, eccbytes); > + oobbuf += eccbytes; > > if (chip->ecc.postpad) { > - chip->read_buf(mtd, oob, chip->ecc.postpad); > - oob += chip->ecc.postpad; > + chip->read_buf(mtd, oobbuf, chip->ecc.postpad); > + oobbuf += chip->ecc.postpad; > } > } > > - size = mtd->oobsize - (oob - chip->oob_poi); > + size = mtd->oobsize - (oobbuf - chip->oob_poi); > if (size) > - chip->read_buf(mtd, oob, size); > + chip->read_buf(mtd, oobbuf, size); > > return 0; > } > @@ -1129,7 +1131,7 @@ static int nand_read_page_raw_syndrome(struct mtd_info *mtd, > * @page: page number to read > */ > static int nand_read_page_swecc(struct mtd_info *mtd, struct nand_chip *chip, > - uint8_t *buf, int page) > + uint8_t *buf, uint8_t *oob, int page) > { > int i, eccsize = chip->ecc.size; > int eccbytes = chip->ecc.bytes; > @@ -1139,7 +1141,7 @@ static int nand_read_page_swecc(struct mtd_info *mtd, struct nand_chip *chip, > uint8_t *ecc_code = chip->buffers->ecccode; > uint32_t *eccpos = chip->ecc.layout->eccpos; > > - chip->ecc.read_page_raw(mtd, chip, buf, page); > + chip->ecc.read_page_raw(mtd, chip, buf, oob, page); > > for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) > chip->ecc.calculate(mtd, p, &ecc_calc[i]); > @@ -1257,12 +1259,13 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip, > * @mtd: mtd info structure > * @chip: nand chip info structure > * @buf: buffer to store read data > + * @oob: buffer to store read OOB > * @page: page number to read > * > * Not for syndrome calculating ECC controllers which need a special oob layout. > */ > static int nand_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, > - uint8_t *buf, int page) > + uint8_t *buf, uint8_t *oob, int page) > { > int i, eccsize = chip->ecc.size; > int eccbytes = chip->ecc.bytes; > @@ -1302,6 +1305,7 @@ static int nand_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, > * @mtd: mtd info structure > * @chip: nand chip info structure > * @buf: buffer to store read data > + * @oob: buffer to store read OOB > * @page: page number to read > * > * Hardware ECC for large page chips, require OOB to be read first. For this > @@ -1311,7 +1315,7 @@ static int nand_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, > * the data area, by overwriting the NAND manufacturer bad block markings. > */ > static int nand_read_page_hwecc_oob_first(struct mtd_info *mtd, > - struct nand_chip *chip, uint8_t *buf, int page) > + struct nand_chip *chip, uint8_t *buf, uint8_t *oob, int page) > { > int i, eccsize = chip->ecc.size; > int eccbytes = chip->ecc.bytes; > @@ -1350,19 +1354,20 @@ static int nand_read_page_hwecc_oob_first(struct mtd_info *mtd, > * @mtd: mtd info structure > * @chip: nand chip info structure > * @buf: buffer to store read data > + * @oob: buffer to store read OOB > * @page: page number to read > * > * The hw generator calculates the error syndrome automatically. Therefore we > * need a special oob layout and handling. > */ > static int nand_read_page_syndrome(struct mtd_info *mtd, struct nand_chip *chip, > - uint8_t *buf, int page) > + uint8_t *buf, uint8_t *oob, int page) > { > int i, eccsize = chip->ecc.size; > int eccbytes = chip->ecc.bytes; > int eccsteps = chip->ecc.steps; > uint8_t *p = buf; > - uint8_t *oob = chip->oob_poi; > + uint8_t *oobbuf = chip->oob_poi; > > for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) { > int stat; > @@ -1371,23 +1376,23 @@ static int nand_read_page_syndrome(struct mtd_info *mtd, struct nand_chip *chip, > chip->read_buf(mtd, p, eccsize); > > if (chip->ecc.prepad) { > - chip->read_buf(mtd, oob, chip->ecc.prepad); > - oob += chip->ecc.prepad; > + chip->read_buf(mtd, oobbuf, chip->ecc.prepad); > + oobbuf += chip->ecc.prepad; > } > > chip->ecc.hwctl(mtd, NAND_ECC_READSYN); > - chip->read_buf(mtd, oob, eccbytes); > - stat = chip->ecc.correct(mtd, p, oob, NULL); > + chip->read_buf(mtd, oobbuf, eccbytes); > + stat = chip->ecc.correct(mtd, p, oobbuf, NULL); > > if (stat < 0) > mtd->ecc_stats.failed++; > else > mtd->ecc_stats.corrected += stat; > > - oob += eccbytes; > + oobbuf += eccbytes; > > if (chip->ecc.postpad) { > - chip->read_buf(mtd, oob, chip->ecc.postpad); > + chip->read_buf(mtd, oobbuf, chip->ecc.postpad); > oob += chip->ecc.postpad; > } > } > @@ -1500,14 +1505,14 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from, > > /* Now read the page into the buffer */ > if (unlikely(ops->mode == MTD_OPS_RAW)) > - ret = chip->ecc.read_page_raw(mtd, chip, > - bufpoi, page); > + ret = chip->ecc.read_page_raw(mtd, chip, bufpoi, > + NULL, page); > else if (!aligned && NAND_SUBPAGE_READ(chip) && !oob) > ret = chip->ecc.read_subpage(mtd, chip, > col, bytes, bufpoi); > else > ret = chip->ecc.read_page(mtd, chip, bufpoi, > - page); > + NULL, page); > if (ret < 0) { > if (!aligned) > /* Invalidate page cache */ > @@ -1919,11 +1924,12 @@ out: > * @mtd: mtd info structure > * @chip: nand chip info structure > * @buf: data buffer > + * @oob: OOB buffer > * > * Not for syndrome calculating ECC controllers, which use a special oob layout. > */ > static void nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, > - const uint8_t *buf) > + const uint8_t *buf, const uint8_t *oob) > { > chip->write_buf(mtd, buf, mtd->writesize); > chip->write_buf(mtd, chip->oob_poi, mtd->oobsize); > @@ -1934,16 +1940,18 @@ static void nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, > * @mtd: mtd info structure > * @chip: nand chip info structure > * @buf: data buffer > + * @oob: OOB buffer > * > * We need a special oob layout and handling even when ECC isn't checked. > */ > static void nand_write_page_raw_syndrome(struct mtd_info *mtd, > struct nand_chip *chip, > - const uint8_t *buf) > + const uint8_t *buf, > + const uint8_t *oob) > { > int eccsize = chip->ecc.size; > int eccbytes = chip->ecc.bytes; > - uint8_t *oob = chip->oob_poi; > + uint8_t *oobbuf = chip->oob_poi; > int steps, size; > > for (steps = chip->ecc.steps; steps > 0; steps--) { > @@ -1951,31 +1959,32 @@ static void nand_write_page_raw_syndrome(struct mtd_info *mtd, > buf += eccsize; > > if (chip->ecc.prepad) { > - chip->write_buf(mtd, oob, chip->ecc.prepad); > - oob += chip->ecc.prepad; > + chip->write_buf(mtd, oobbuf, chip->ecc.prepad); > + oobbuf += chip->ecc.prepad; > } > > - chip->read_buf(mtd, oob, eccbytes); > - oob += eccbytes; > + chip->read_buf(mtd, oobbuf, eccbytes); > + oobbuf += eccbytes; > > if (chip->ecc.postpad) { > - chip->write_buf(mtd, oob, chip->ecc.postpad); > - oob += chip->ecc.postpad; > + chip->write_buf(mtd, oobbuf, chip->ecc.postpad); > + oobbuf += chip->ecc.postpad; > } > } > > - size = mtd->oobsize - (oob - chip->oob_poi); > + size = mtd->oobsize - (oobbuf - chip->oob_poi); > if (size) > - chip->write_buf(mtd, oob, size); > + chip->write_buf(mtd, oobbuf, size); > } > /** > * nand_write_page_swecc - [REPLACEABLE] software ECC based page write function > * @mtd: mtd info structure > * @chip: nand chip info structure > * @buf: data buffer > + * @oob: OOB buffer > */ > static void nand_write_page_swecc(struct mtd_info *mtd, struct nand_chip *chip, > - const uint8_t *buf) > + const uint8_t *buf, const uint8_t *oob) > { > int i, eccsize = chip->ecc.size; > int eccbytes = chip->ecc.bytes; > @@ -1991,7 +2000,7 @@ static void nand_write_page_swecc(struct mtd_info *mtd, struct nand_chip *chip, > for (i = 0; i < chip->ecc.total; i++) > chip->oob_poi[eccpos[i]] = ecc_calc[i]; > > - chip->ecc.write_page_raw(mtd, chip, buf); > + chip->ecc.write_page_raw(mtd, chip, buf, oob); > } > > /** > @@ -1999,9 +2008,10 @@ static void nand_write_page_swecc(struct mtd_info *mtd, struct nand_chip *chip, > * @mtd: mtd info structure > * @chip: nand chip info structure > * @buf: data buffer > + * @oob: OOB buffer > */ > static void nand_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, > - const uint8_t *buf) > + const uint8_t *buf, const uint8_t *oob) > { > int i, eccsize = chip->ecc.size; > int eccbytes = chip->ecc.bytes; > @@ -2027,18 +2037,20 @@ static void nand_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, > * @mtd: mtd info structure > * @chip: nand chip info structure > * @buf: data buffer > + * @oob: OOB data buffer > * > * The hw generator calculates the error syndrome automatically. Therefore we > * need a special oob layout and handling. > */ > static void nand_write_page_syndrome(struct mtd_info *mtd, > - struct nand_chip *chip, const uint8_t *buf) > + struct nand_chip *chip, const uint8_t *buf, > + const uint8_t *oob) > { > int i, eccsize = chip->ecc.size; > int eccbytes = chip->ecc.bytes; > int eccsteps = chip->ecc.steps; > const uint8_t *p = buf; > - uint8_t *oob = chip->oob_poi; > + uint8_t *oobbuf = chip->oob_poi; > > for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) { > > @@ -2046,24 +2058,24 @@ static void nand_write_page_syndrome(struct mtd_info *mtd, > chip->write_buf(mtd, p, eccsize); > > if (chip->ecc.prepad) { > - chip->write_buf(mtd, oob, chip->ecc.prepad); > - oob += chip->ecc.prepad; > + chip->write_buf(mtd, oobbuf, chip->ecc.prepad); > + oobbuf += chip->ecc.prepad; > } > > - chip->ecc.calculate(mtd, p, oob); > - chip->write_buf(mtd, oob, eccbytes); > - oob += eccbytes; > + chip->ecc.calculate(mtd, p, oobbuf); > + chip->write_buf(mtd, oobbuf, eccbytes); > + oobbuf += eccbytes; > > if (chip->ecc.postpad) { > - chip->write_buf(mtd, oob, chip->ecc.postpad); > - oob += chip->ecc.postpad; > + chip->write_buf(mtd, oobbuf, chip->ecc.postpad); > + oobbuf += chip->ecc.postpad; > } > } > > /* Calculate remaining oob bytes */ > - i = mtd->oobsize - (oob - chip->oob_poi); > + i = mtd->oobsize - (oobbuf - chip->oob_poi); > if (i) > - chip->write_buf(mtd, oob, i); > + chip->write_buf(mtd, oobbuf, i); > } > > /** > @@ -2076,16 +2088,17 @@ static void nand_write_page_syndrome(struct mtd_info *mtd, > * @raw: use _raw version of write_page > */ > static int nand_write_page(struct mtd_info *mtd, struct nand_chip *chip, > - const uint8_t *buf, int page, int cached, int raw) > + const uint8_t *buf, const uint8_t *oob, int page, > + int cached, int raw) > { > int status; > > chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page); > > if (unlikely(raw)) > - chip->ecc.write_page_raw(mtd, chip, buf); > + chip->ecc.write_page_raw(mtd, chip, buf, oob); > else > - chip->ecc.write_page(mtd, chip, buf); > + chip->ecc.write_page(mtd, chip, buf, oob); > > /* > * Cached progamming disabled for now. Not sure if it's worth the > @@ -2264,7 +2277,7 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to, > memset(chip->oob_poi, 0xff, mtd->oobsize); > } > > - ret = chip->write_page(mtd, chip, wbuf, page, cached, > + ret = chip->write_page(mtd, chip, wbuf, NULL, page, cached, > (ops->mode == MTD_OPS_RAW)); > if (ret) > break; > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c > index def50ca..92d2eae 100644 > --- a/drivers/mtd/nand/pxa3xx_nand.c > +++ b/drivers/mtd/nand/pxa3xx_nand.c > @@ -682,14 +682,14 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command, > } > > static void pxa3xx_nand_write_page_hwecc(struct mtd_info *mtd, > - struct nand_chip *chip, const uint8_t *buf) > + struct nand_chip *chip, const uint8_t *buf, const uint8_t *oob) > { > chip->write_buf(mtd, buf, mtd->writesize); > chip->write_buf(mtd, chip->oob_poi, mtd->oobsize); > } > > static int pxa3xx_nand_read_page_hwecc(struct mtd_info *mtd, > - struct nand_chip *chip, uint8_t *buf, int page) > + struct nand_chip *chip, uint8_t *buf, uint8_t *oob, int page) > { > struct pxa3xx_nand_host *host = mtd->priv; > struct pxa3xx_nand_info *info = host->info_data; > diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c > index e9b2b26..d1b2731 100644 > --- a/drivers/mtd/nand/sh_flctl.c > +++ b/drivers/mtd/nand/sh_flctl.c > @@ -344,7 +344,7 @@ static void set_cmd_regs(struct mtd_info *mtd, uint32_t cmd, uint32_t flcmcdr_va > } > > static int flctl_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, > - uint8_t *buf, int page) > + uint8_t *buf, uint8_t *oob, int page) > { > int i, eccsize = chip->ecc.size; > int eccbytes = chip->ecc.bytes; > @@ -366,7 +366,7 @@ static int flctl_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, > } > > static void flctl_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, > - const uint8_t *buf) > + const uint8_t *buf, const uint8_t *oob) > { > int i, eccsize = chip->ecc.size; > int eccbytes = chip->ecc.bytes; > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > index 1482340..be9ee1f 100644 > --- a/include/linux/mtd/nand.h > +++ b/include/linux/mtd/nand.h > @@ -363,15 +363,15 @@ struct nand_ecc_ctrl { > int (*correct)(struct mtd_info *mtd, uint8_t *dat, uint8_t *read_ecc, > uint8_t *calc_ecc); > int (*read_page_raw)(struct mtd_info *mtd, struct nand_chip *chip, > - uint8_t *buf, int page); > + uint8_t *buf, uint8_t *oob, int page); > void (*write_page_raw)(struct mtd_info *mtd, struct nand_chip *chip, > - const uint8_t *buf); > + const uint8_t *buf, const uint8_t *oob); > int (*read_page)(struct mtd_info *mtd, struct nand_chip *chip, > - uint8_t *buf, int page); > + uint8_t *buf, uint8_t *oob, int page); > int (*read_subpage)(struct mtd_info *mtd, struct nand_chip *chip, > uint32_t offs, uint32_t len, uint8_t *buf); > void (*write_page)(struct mtd_info *mtd, struct nand_chip *chip, > - const uint8_t *buf); > + const uint8_t *buf, const uint8_t *oob); > int (*write_oob_raw)(struct mtd_info *mtd, struct nand_chip *chip, > int page); > int (*read_oob_raw)(struct mtd_info *mtd, struct nand_chip *chip, > @@ -505,7 +505,8 @@ struct nand_chip { > int (*errstat)(struct mtd_info *mtd, struct nand_chip *this, int state, > int status, int page); > int (*write_page)(struct mtd_info *mtd, struct nand_chip *chip, > - const uint8_t *buf, int page, int cached, int raw); > + const uint8_t *buf, const uint8_t *oob, int page, > + int cached, int raw); > > int chip_delay; > unsigned int options; > -- > 1.7.5.4.2.g519b1 > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces 2012-04-18 7:56 ` Bastian Hecht @ 2012-04-18 9:37 ` Bastian Hecht 2012-04-18 16:22 ` Brian Norris 0 siblings, 1 reply; 27+ messages in thread From: Bastian Hecht @ 2012-04-18 9:37 UTC (permalink / raw) To: Brian Norris Cc: Viresh Kumar, Artem Bityutskiy, Nicolas Ferre, Vipin Kumar, linux-mtd, Laurent Pinchart, Florian Fainelli, Jamie Iles, Mike Dunn, Bastian Hecht, Dmitry Eremin-Solenikov, Kevin Cernekee, Lei Wen, Axel Lin, Li Yang, Jean-Christophe PLAGNIOL-VILLARD, Armando Visconti, Liu Shuo, Thomas Gleixner, Scott Branden, Artem Bityutskiy, Wolfram Sang, Huang Shijie, Shmulik Ladkani, Jiandong Zheng, David Woodhouse 2012/4/18 Bastian Hecht <hechtb@googlemail.com>: > Hello Brian, > > I'm currently working on the hardware ECC part of the SH Mobile flctl > and I like the patchset as it makes things more explicit when OOB data > is requested or not and such things. It's cleaner and better, thanks! To be a bit more of a concrete help, if you can give me a use case to test the oob reads/writes I can modify my driver to work with your patches. Right now I rely on nandwrite, nanddump, nandtest, the kernel test modules and ubi. With none of them I produced an error so far although the current implementation of the hardware ECC completely ignores OOB data other than ECC. Best regards, Bastian Hecht > Best regards, > > Bastian Hecht > > > 2012/4/17 Brian Norris <computersforpeace@gmail.com>: >> New NAND controllers can perform read/write via HW engines which don't expose >> OOB data in their DMA mode. To reflect this, we should rework the nand_chip / >> nand_ecc_ctrl interfaces that assume that drivers will always read/write OOB >> data in the nand_chip.oob_poi buffer. A better interface would pass the >> appropriate buffer explicitly when OOB data is requested and otherwise pass a >> NULL pointer, meaning that no OOB data is needed. >> >> This patch adds the 'oob' parameter to each relevant {read,write}_page >> interface; all 'oob' parameters are left unused and NULL for now. The next >> patch will set the parameter properly. Perhaps it should be utilized eventually >> on some of these drivers, but this is a slow process of hacking each driver >> function that uses these interfaces. >> >> I renamed many 'oob' buffers to 'oobbuf' so that we can use 'oob' as an >> argument to these interface functions. I think the consistency can help in the >> long run. >> >> Note: I couldn't compile-test all of these easily, as some had ARCH >> dependencies. >> >> Signed-off-by: Brian Norris <computersforpeace@gmail.com> >> --- >> drivers/mtd/nand/atmel_nand.c | 7 ++- >> drivers/mtd/nand/bcm_umi_bch.c | 10 ++- >> drivers/mtd/nand/bcm_umi_nand.c | 2 +- >> drivers/mtd/nand/bf5xx_nand.c | 4 +- >> drivers/mtd/nand/cafe_nand.c | 26 ++++--- >> drivers/mtd/nand/denali.c | 8 +- >> drivers/mtd/nand/docg4.c | 12 ++-- >> drivers/mtd/nand/fsl_elbc_nand.c | 7 +- >> drivers/mtd/nand/fsl_ifc_nand.c | 4 +- >> drivers/mtd/nand/fsmc_nand.c | 3 +- >> drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 5 +- >> drivers/mtd/nand/nand_base.c | 123 ++++++++++++++++++-------------- >> drivers/mtd/nand/pxa3xx_nand.c | 4 +- >> drivers/mtd/nand/sh_flctl.c | 4 +- >> include/linux/mtd/nand.h | 11 ++-- >> 15 files changed, 127 insertions(+), 103 deletions(-) >> >> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c >> index 2165576..95838a6 100644 >> --- a/drivers/mtd/nand/atmel_nand.c >> +++ b/drivers/mtd/nand/atmel_nand.c >> @@ -324,18 +324,21 @@ static int atmel_nand_calculate(struct mtd_info *mtd, >> * mtd: mtd info structure >> * chip: nand chip info structure >> * buf: buffer to store read data >> + * oob: buffer to store read OOB data >> */ >> static int atmel_nand_read_page(struct mtd_info *mtd, >> - struct nand_chip *chip, uint8_t *buf, int page) >> + struct nand_chip *chip, uint8_t *buf, uint8_t *oob, int page) >> { >> int eccsize = chip->ecc.size; >> int eccbytes = chip->ecc.bytes; >> uint32_t *eccpos = chip->ecc.layout->eccpos; >> uint8_t *p = buf; >> - uint8_t *oob = chip->oob_poi; >> uint8_t *ecc_pos; >> int stat; >> >> + if (!oob) >> + oob = chip->oob_poi; >> + >> /* >> * Errata: ALE is incorrectly wired up to the ECC controller >> * on the AP7000, so it will include the address cycles in the >> diff --git a/drivers/mtd/nand/bcm_umi_bch.c b/drivers/mtd/nand/bcm_umi_bch.c >> index a930666..3f4e845 100644 >> --- a/drivers/mtd/nand/bcm_umi_bch.c >> +++ b/drivers/mtd/nand/bcm_umi_bch.c >> @@ -22,9 +22,9 @@ >> >> /* ---- Private Function Prototypes -------------------------------------- */ >> static int bcm_umi_bch_read_page_hwecc(struct mtd_info *mtd, >> - struct nand_chip *chip, uint8_t *buf, int page); >> + struct nand_chip *chip, uint8_t *buf, uint8_t *oob, int page); >> static void bcm_umi_bch_write_page_hwecc(struct mtd_info *mtd, >> - struct nand_chip *chip, const uint8_t *buf); >> + struct nand_chip *chip, const uint8_t *buf, const uint8_t *oob); >> >> /* ---- Private Variables ------------------------------------------------ */ >> >> @@ -103,11 +103,12 @@ static struct nand_ecclayout nand_hw_eccoob_4096 = { >> * @mtd: mtd info structure >> * @chip: nand chip info structure >> * @buf: buffer to store read data >> +* @oob: buffer to store read OOB data >> * >> ***************************************************************************/ >> static int bcm_umi_bch_read_page_hwecc(struct mtd_info *mtd, >> struct nand_chip *chip, uint8_t * buf, >> - int page) >> + uint8_t *oob, int page) >> { >> int sectorIdx = 0; >> int eccsize = chip->ecc.size; >> @@ -188,10 +189,11 @@ static int bcm_umi_bch_read_page_hwecc(struct mtd_info *mtd, >> * @mtd: mtd info structure >> * @chip: nand chip info structure >> * @buf: data buffer >> +* @oob: OOB buffer >> * >> ***************************************************************************/ >> static void bcm_umi_bch_write_page_hwecc(struct mtd_info *mtd, >> - struct nand_chip *chip, const uint8_t *buf) >> + struct nand_chip *chip, const uint8_t *buf, const uint8_t *oob) >> { >> int sectorIdx = 0; >> int eccsize = chip->ecc.size; >> diff --git a/drivers/mtd/nand/bcm_umi_nand.c b/drivers/mtd/nand/bcm_umi_nand.c >> index 6908cdd..ac7a027 100644 >> --- a/drivers/mtd/nand/bcm_umi_nand.c >> +++ b/drivers/mtd/nand/bcm_umi_nand.c >> @@ -341,7 +341,7 @@ static int bcm_umi_nand_verify_buf(struct mtd_info *mtd, const u_char * buf, >> * for MLC parts which may have permanently stuck bits. >> */ >> struct nand_chip *chip = mtd->priv; >> - int ret = chip->ecc.read_page(mtd, chip, readbackbuf, 0); >> + int ret = chip->ecc.read_page(mtd, chip, readbackbuf, NULL, 0); >> if (ret < 0) >> return -EFAULT; >> else { >> diff --git a/drivers/mtd/nand/bf5xx_nand.c b/drivers/mtd/nand/bf5xx_nand.c >> index d7b86b9..d25ab1a 100644 >> --- a/drivers/mtd/nand/bf5xx_nand.c >> +++ b/drivers/mtd/nand/bf5xx_nand.c >> @@ -558,7 +558,7 @@ static void bf5xx_nand_dma_write_buf(struct mtd_info *mtd, >> } >> >> static int bf5xx_nand_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip, >> - uint8_t *buf, int page) >> + uint8_t *buf, uint8_t *oob, int page) >> { >> bf5xx_nand_read_buf(mtd, buf, mtd->writesize); >> bf5xx_nand_read_buf(mtd, chip->oob_poi, mtd->oobsize); >> @@ -567,7 +567,7 @@ static int bf5xx_nand_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip >> } >> >> static void bf5xx_nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, >> - const uint8_t *buf) >> + const uint8_t *buf, const uint8_t *oob) >> { >> bf5xx_nand_write_buf(mtd, buf, mtd->writesize); >> bf5xx_nand_write_buf(mtd, chip->oob_poi, mtd->oobsize); >> diff --git a/drivers/mtd/nand/cafe_nand.c b/drivers/mtd/nand/cafe_nand.c >> index 2973d97..e4103c5 100644 >> --- a/drivers/mtd/nand/cafe_nand.c >> +++ b/drivers/mtd/nand/cafe_nand.c >> @@ -375,12 +375,13 @@ static int cafe_nand_read_oob(struct mtd_info *mtd, struct nand_chip *chip, >> * @mtd: mtd info structure >> * @chip: nand chip info structure >> * @buf: buffer to store read data >> + * @oob: buffer to store OOB data >> * >> * The hw generator calculates the error syndrome automatically. Therefor >> * we need a special oob layout and handling. >> */ >> static int cafe_nand_read_page(struct mtd_info *mtd, struct nand_chip *chip, >> - uint8_t *buf, int page) >> + uint8_t *buf, uint8_t *oob, int page) >> { >> struct cafe_priv *cafe = mtd->priv; >> >> @@ -394,7 +395,7 @@ static int cafe_nand_read_page(struct mtd_info *mtd, struct nand_chip *chip, >> if (checkecc && cafe_readl(cafe, NAND_ECC_RESULT) & (1<<18)) { >> unsigned short syn[8], pat[4]; >> int pos[4]; >> - u8 *oob = chip->oob_poi; >> + u8 *oobbuf = chip->oob_poi; >> int i, n; >> >> for (i=0; i<8; i+=2) { >> @@ -422,14 +423,14 @@ static int cafe_nand_read_page(struct mtd_info *mtd, struct nand_chip *chip, >> buf[0] ^= pat[i]; >> } else if (p == 1365) { >> buf[2047] ^= pat[i] >> 4; >> - oob[0] ^= pat[i] << 4; >> + oobbuf[0] ^= pat[i] << 4; >> } else if (p > 1365) { >> if ((p & 1) == 1) { >> - oob[3*p/2 - 2048] ^= pat[i] >> 4; >> - oob[3*p/2 - 2047] ^= pat[i] << 4; >> + oobbuf[3*p/2 - 2048] ^= pat[i] >> 4; >> + oobbuf[3*p/2 - 2047] ^= pat[i] << 4; >> } else { >> - oob[3*p/2 - 2049] ^= pat[i] >> 8; >> - oob[3*p/2 - 2048] ^= pat[i]; >> + oobbuf[3*p/2 - 2049] ^= pat[i] >> 8; >> + oobbuf[3*p/2 - 2048] ^= pat[i]; >> } >> } else if ((p & 1) == 1) { >> buf[3*p/2] ^= pat[i] >> 4; >> @@ -518,7 +519,9 @@ static struct nand_bbt_descr cafe_bbt_mirror_descr_512 = { >> >> >> static void cafe_nand_write_page_lowlevel(struct mtd_info *mtd, >> - struct nand_chip *chip, const uint8_t *buf) >> + struct nand_chip *chip, >> + const uint8_t *buf, >> + const uint8_t *oob) >> { >> struct cafe_priv *cafe = mtd->priv; >> >> @@ -530,16 +533,17 @@ static void cafe_nand_write_page_lowlevel(struct mtd_info *mtd, >> } >> >> static int cafe_nand_write_page(struct mtd_info *mtd, struct nand_chip *chip, >> - const uint8_t *buf, int page, int cached, int raw) >> + const uint8_t *buf, const uint8_t *oob, >> + int page, int cached, int raw) >> { >> int status; >> >> chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page); >> >> if (unlikely(raw)) >> - chip->ecc.write_page_raw(mtd, chip, buf); >> + chip->ecc.write_page_raw(mtd, chip, buf, oob); >> else >> - chip->ecc.write_page(mtd, chip, buf); >> + chip->ecc.write_page(mtd, chip, buf, oob); >> >> /* >> * Cached progamming disabled for now, Not sure if its worth the >> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c >> index a1048c7..d078470 100644 >> --- a/drivers/mtd/nand/denali.c >> +++ b/drivers/mtd/nand/denali.c >> @@ -1084,7 +1084,7 @@ static void write_page(struct mtd_info *mtd, struct nand_chip *chip, >> * by write_page above. >> * */ >> static void denali_write_page(struct mtd_info *mtd, struct nand_chip *chip, >> - const uint8_t *buf) >> + const uint8_t *buf, const uint8_t *oob) >> { >> /* for regular page writes, we let HW handle all the ECC >> * data written to the device. */ >> @@ -1096,7 +1096,7 @@ static void denali_write_page(struct mtd_info *mtd, struct nand_chip *chip, >> * write_page() function above. >> */ >> static void denali_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, >> - const uint8_t *buf) >> + const uint8_t *buf, const uint8_t *oob) >> { >> /* for raw page writes, we want to disable ECC and simply write >> whatever data is in the buffer. */ >> @@ -1119,7 +1119,7 @@ static int denali_read_oob(struct mtd_info *mtd, struct nand_chip *chip, >> } >> >> static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip, >> - uint8_t *buf, int page) >> + uint8_t *buf, uint8_t *oob, int page) >> { >> struct denali_nand_info *denali = mtd_to_denali(mtd); >> >> @@ -1171,7 +1171,7 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip, >> } >> >> static int denali_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip, >> - uint8_t *buf, int page) >> + uint8_t *buf, uint8_t *oob, int page) >> { >> struct denali_nand_info *denali = mtd_to_denali(mtd); >> >> diff --git a/drivers/mtd/nand/docg4.c b/drivers/mtd/nand/docg4.c >> index b082026..7ff99c9 100644 >> --- a/drivers/mtd/nand/docg4.c >> +++ b/drivers/mtd/nand/docg4.c >> @@ -786,13 +786,13 @@ static int read_page(struct mtd_info *mtd, struct nand_chip *nand, >> >> >> static int docg4_read_page_raw(struct mtd_info *mtd, struct nand_chip *nand, >> - uint8_t *buf, int page) >> + uint8_t *buf, uint8_t *oob, int page) >> { >> return read_page(mtd, nand, buf, page, false); >> } >> >> static int docg4_read_page(struct mtd_info *mtd, struct nand_chip *nand, >> - uint8_t *buf, int page) >> + uint8_t *buf, uint8_t *oob, int page) >> { >> return read_page(mtd, nand, buf, page, true); >> } >> @@ -952,13 +952,13 @@ static void write_page(struct mtd_info *mtd, struct nand_chip *nand, >> } >> >> static void docg4_write_page_raw(struct mtd_info *mtd, struct nand_chip *nand, >> - const uint8_t *buf) >> + const uint8_t *buf, const uint8_t *oob) >> { >> return write_page(mtd, nand, buf, false); >> } >> >> static void docg4_write_page(struct mtd_info *mtd, struct nand_chip *nand, >> - const uint8_t *buf) >> + const uint8_t *buf, const uint8_t *oob) >> { >> return write_page(mtd, nand, buf, true); >> } >> @@ -1002,7 +1002,7 @@ static int __init read_factory_bbt(struct mtd_info *mtd) >> return -ENOMEM; >> >> read_page_prologue(mtd, g4_addr); >> - status = docg4_read_page(mtd, nand, buf, DOCG4_FACTORY_BBT_PAGE); >> + status = docg4_read_page(mtd, nand, buf, NULL, DOCG4_FACTORY_BBT_PAGE); >> if (status) >> goto exit; >> >> @@ -1079,7 +1079,7 @@ static int docg4_block_markbad(struct mtd_info *mtd, loff_t ofs) >> >> /* write first page of block */ >> write_page_prologue(mtd, g4_addr); >> - docg4_write_page(mtd, nand, buf); >> + docg4_write_page(mtd, nand, buf, nand->oob_poi); >> ret = pageprog(mtd); >> if (!ret) >> mtd->ecc_stats.badblocks++; >> diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c >> index 80b5264..b7d9bfd 100644 >> --- a/drivers/mtd/nand/fsl_elbc_nand.c >> +++ b/drivers/mtd/nand/fsl_elbc_nand.c >> @@ -740,7 +740,7 @@ static int fsl_elbc_chip_init_tail(struct mtd_info *mtd) >> >> static int fsl_elbc_read_page(struct mtd_info *mtd, >> struct nand_chip *chip, >> - uint8_t *buf, >> + uint8_t *buf, uint8_t *oob, >> int page) >> { >> fsl_elbc_read_buf(mtd, buf, mtd->writesize); >> @@ -755,9 +755,8 @@ static int fsl_elbc_read_page(struct mtd_info *mtd, >> /* ECC will be calculated automatically, and errors will be detected in >> * waitfunc. >> */ >> -static void fsl_elbc_write_page(struct mtd_info *mtd, >> - struct nand_chip *chip, >> - const uint8_t *buf) >> +static void fsl_elbc_write_page(struct mtd_info *mtd, struct nand_chip *chip, >> + const uint8_t *buf, const uint8_t *oob) >> { >> fsl_elbc_write_buf(mtd, buf, mtd->writesize); >> fsl_elbc_write_buf(mtd, chip->oob_poi, mtd->oobsize); >> diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c >> index c30ac7b..f8a6e3e 100644 >> --- a/drivers/mtd/nand/fsl_ifc_nand.c >> +++ b/drivers/mtd/nand/fsl_ifc_nand.c >> @@ -684,7 +684,7 @@ static int fsl_ifc_wait(struct mtd_info *mtd, struct nand_chip *chip) >> >> static int fsl_ifc_read_page(struct mtd_info *mtd, >> struct nand_chip *chip, >> - uint8_t *buf, int page) >> + uint8_t *buf, uint8_t *oob, int page) >> { >> struct fsl_ifc_mtd *priv = chip->priv; >> struct fsl_ifc_ctrl *ctrl = priv->ctrl; >> @@ -706,7 +706,7 @@ static int fsl_ifc_read_page(struct mtd_info *mtd, >> */ >> static void fsl_ifc_write_page(struct mtd_info *mtd, >> struct nand_chip *chip, >> - const uint8_t *buf) >> + const uint8_t *buf, const uint8_t *oob) >> { >> fsl_ifc_write_buf(mtd, buf, mtd->writesize); >> fsl_ifc_write_buf(mtd, chip->oob_poi, mtd->oobsize); >> diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c >> index 1b8330e..8bb731b 100644 >> --- a/drivers/mtd/nand/fsmc_nand.c >> +++ b/drivers/mtd/nand/fsmc_nand.c >> @@ -692,6 +692,7 @@ static void fsmc_write_buf_dma(struct mtd_info *mtd, const uint8_t *buf, >> * @mtd: mtd info structure >> * @chip: nand chip info structure >> * @buf: buffer to store read data >> + * @oob: buffer to store read OOB data >> * @page: page number to read >> * >> * This routine is needed for fsmc version 8 as reading from NAND chip has to be >> @@ -701,7 +702,7 @@ static void fsmc_write_buf_dma(struct mtd_info *mtd, const uint8_t *buf, >> * max of 8 bits) >> */ >> static int fsmc_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, >> - uint8_t *buf, int page) >> + uint8_t *buf, uint8_t *oob, int page) >> { >> struct fsmc_nand_data *host = container_of(mtd, >> struct fsmc_nand_data, mtd); >> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c >> index 75b1dde..949c761 100644 >> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c >> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c >> @@ -841,7 +841,7 @@ static void block_mark_swapping(struct gpmi_nand_data *this, >> } >> >> static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip, >> - uint8_t *buf, int page) >> + uint8_t *buf, uint8_t *oob, int page) >> { >> struct gpmi_nand_data *this = chip->priv; >> struct bch_geometry *nfc_geo = &this->bch_geometry; >> @@ -928,7 +928,8 @@ exit_nfc: >> } >> >> static void gpmi_ecc_write_page(struct mtd_info *mtd, >> - struct nand_chip *chip, const uint8_t *buf) >> + struct nand_chip *chip, const uint8_t *buf, >> + const uint8_t *oob) >> { >> struct gpmi_nand_data *this = chip->priv; >> struct bch_geometry *nfc_geo = &this->bch_geometry; >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c >> index 47b19c0..95ba987 100644 >> --- a/drivers/mtd/nand/nand_base.c >> +++ b/drivers/mtd/nand/nand_base.c >> @@ -1066,12 +1066,13 @@ EXPORT_SYMBOL(nand_lock); >> * @mtd: mtd info structure >> * @chip: nand chip info structure >> * @buf: buffer to store read data >> + * @oob: buffer to store read OOB data >> * @page: page number to read >> * >> * Not for syndrome calculating ECC controllers, which use a special oob layout. >> */ >> static int nand_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip, >> - uint8_t *buf, int page) >> + uint8_t *buf, uint8_t *oob, int page) >> { >> chip->read_buf(mtd, buf, mtd->writesize); >> chip->read_buf(mtd, chip->oob_poi, mtd->oobsize); >> @@ -1083,17 +1084,18 @@ static int nand_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip, >> * @mtd: mtd info structure >> * @chip: nand chip info structure >> * @buf: buffer to store read data >> + * @oob: buffer to store read OOB >> * @page: page number to read >> * >> * We need a special oob layout and handling even when OOB isn't used. >> */ >> static int nand_read_page_raw_syndrome(struct mtd_info *mtd, >> struct nand_chip *chip, >> - uint8_t *buf, int page) >> + uint8_t *buf, uint8_t *oob, int page) >> { >> int eccsize = chip->ecc.size; >> int eccbytes = chip->ecc.bytes; >> - uint8_t *oob = chip->oob_poi; >> + uint8_t *oobbuf = chip->oob_poi; >> int steps, size; >> >> for (steps = chip->ecc.steps; steps > 0; steps--) { >> @@ -1101,22 +1103,22 @@ static int nand_read_page_raw_syndrome(struct mtd_info *mtd, >> buf += eccsize; >> >> if (chip->ecc.prepad) { >> - chip->read_buf(mtd, oob, chip->ecc.prepad); >> - oob += chip->ecc.prepad; >> + chip->read_buf(mtd, oobbuf, chip->ecc.prepad); >> + oobbuf += chip->ecc.prepad; >> } >> >> - chip->read_buf(mtd, oob, eccbytes); >> - oob += eccbytes; >> + chip->read_buf(mtd, oobbuf, eccbytes); >> + oobbuf += eccbytes; >> >> if (chip->ecc.postpad) { >> - chip->read_buf(mtd, oob, chip->ecc.postpad); >> - oob += chip->ecc.postpad; >> + chip->read_buf(mtd, oobbuf, chip->ecc.postpad); >> + oobbuf += chip->ecc.postpad; >> } >> } >> >> - size = mtd->oobsize - (oob - chip->oob_poi); >> + size = mtd->oobsize - (oobbuf - chip->oob_poi); >> if (size) >> - chip->read_buf(mtd, oob, size); >> + chip->read_buf(mtd, oobbuf, size); >> >> return 0; >> } >> @@ -1129,7 +1131,7 @@ static int nand_read_page_raw_syndrome(struct mtd_info *mtd, >> * @page: page number to read >> */ >> static int nand_read_page_swecc(struct mtd_info *mtd, struct nand_chip *chip, >> - uint8_t *buf, int page) >> + uint8_t *buf, uint8_t *oob, int page) >> { >> int i, eccsize = chip->ecc.size; >> int eccbytes = chip->ecc.bytes; >> @@ -1139,7 +1141,7 @@ static int nand_read_page_swecc(struct mtd_info *mtd, struct nand_chip *chip, >> uint8_t *ecc_code = chip->buffers->ecccode; >> uint32_t *eccpos = chip->ecc.layout->eccpos; >> >> - chip->ecc.read_page_raw(mtd, chip, buf, page); >> + chip->ecc.read_page_raw(mtd, chip, buf, oob, page); >> >> for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) >> chip->ecc.calculate(mtd, p, &ecc_calc[i]); >> @@ -1257,12 +1259,13 @@ static int nand_read_subpage(struct mtd_info *mtd, struct nand_chip *chip, >> * @mtd: mtd info structure >> * @chip: nand chip info structure >> * @buf: buffer to store read data >> + * @oob: buffer to store read OOB >> * @page: page number to read >> * >> * Not for syndrome calculating ECC controllers which need a special oob layout. >> */ >> static int nand_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, >> - uint8_t *buf, int page) >> + uint8_t *buf, uint8_t *oob, int page) >> { >> int i, eccsize = chip->ecc.size; >> int eccbytes = chip->ecc.bytes; >> @@ -1302,6 +1305,7 @@ static int nand_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, >> * @mtd: mtd info structure >> * @chip: nand chip info structure >> * @buf: buffer to store read data >> + * @oob: buffer to store read OOB >> * @page: page number to read >> * >> * Hardware ECC for large page chips, require OOB to be read first. For this >> @@ -1311,7 +1315,7 @@ static int nand_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, >> * the data area, by overwriting the NAND manufacturer bad block markings. >> */ >> static int nand_read_page_hwecc_oob_first(struct mtd_info *mtd, >> - struct nand_chip *chip, uint8_t *buf, int page) >> + struct nand_chip *chip, uint8_t *buf, uint8_t *oob, int page) >> { >> int i, eccsize = chip->ecc.size; >> int eccbytes = chip->ecc.bytes; >> @@ -1350,19 +1354,20 @@ static int nand_read_page_hwecc_oob_first(struct mtd_info *mtd, >> * @mtd: mtd info structure >> * @chip: nand chip info structure >> * @buf: buffer to store read data >> + * @oob: buffer to store read OOB >> * @page: page number to read >> * >> * The hw generator calculates the error syndrome automatically. Therefore we >> * need a special oob layout and handling. >> */ >> static int nand_read_page_syndrome(struct mtd_info *mtd, struct nand_chip *chip, >> - uint8_t *buf, int page) >> + uint8_t *buf, uint8_t *oob, int page) >> { >> int i, eccsize = chip->ecc.size; >> int eccbytes = chip->ecc.bytes; >> int eccsteps = chip->ecc.steps; >> uint8_t *p = buf; >> - uint8_t *oob = chip->oob_poi; >> + uint8_t *oobbuf = chip->oob_poi; >> >> for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) { >> int stat; >> @@ -1371,23 +1376,23 @@ static int nand_read_page_syndrome(struct mtd_info *mtd, struct nand_chip *chip, >> chip->read_buf(mtd, p, eccsize); >> >> if (chip->ecc.prepad) { >> - chip->read_buf(mtd, oob, chip->ecc.prepad); >> - oob += chip->ecc.prepad; >> + chip->read_buf(mtd, oobbuf, chip->ecc.prepad); >> + oobbuf += chip->ecc.prepad; >> } >> >> chip->ecc.hwctl(mtd, NAND_ECC_READSYN); >> - chip->read_buf(mtd, oob, eccbytes); >> - stat = chip->ecc.correct(mtd, p, oob, NULL); >> + chip->read_buf(mtd, oobbuf, eccbytes); >> + stat = chip->ecc.correct(mtd, p, oobbuf, NULL); >> >> if (stat < 0) >> mtd->ecc_stats.failed++; >> else >> mtd->ecc_stats.corrected += stat; >> >> - oob += eccbytes; >> + oobbuf += eccbytes; >> >> if (chip->ecc.postpad) { >> - chip->read_buf(mtd, oob, chip->ecc.postpad); >> + chip->read_buf(mtd, oobbuf, chip->ecc.postpad); >> oob += chip->ecc.postpad; >> } >> } >> @@ -1500,14 +1505,14 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from, >> >> /* Now read the page into the buffer */ >> if (unlikely(ops->mode == MTD_OPS_RAW)) >> - ret = chip->ecc.read_page_raw(mtd, chip, >> - bufpoi, page); >> + ret = chip->ecc.read_page_raw(mtd, chip, bufpoi, >> + NULL, page); >> else if (!aligned && NAND_SUBPAGE_READ(chip) && !oob) >> ret = chip->ecc.read_subpage(mtd, chip, >> col, bytes, bufpoi); >> else >> ret = chip->ecc.read_page(mtd, chip, bufpoi, >> - page); >> + NULL, page); >> if (ret < 0) { >> if (!aligned) >> /* Invalidate page cache */ >> @@ -1919,11 +1924,12 @@ out: >> * @mtd: mtd info structure >> * @chip: nand chip info structure >> * @buf: data buffer >> + * @oob: OOB buffer >> * >> * Not for syndrome calculating ECC controllers, which use a special oob layout. >> */ >> static void nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, >> - const uint8_t *buf) >> + const uint8_t *buf, const uint8_t *oob) >> { >> chip->write_buf(mtd, buf, mtd->writesize); >> chip->write_buf(mtd, chip->oob_poi, mtd->oobsize); >> @@ -1934,16 +1940,18 @@ static void nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, >> * @mtd: mtd info structure >> * @chip: nand chip info structure >> * @buf: data buffer >> + * @oob: OOB buffer >> * >> * We need a special oob layout and handling even when ECC isn't checked. >> */ >> static void nand_write_page_raw_syndrome(struct mtd_info *mtd, >> struct nand_chip *chip, >> - const uint8_t *buf) >> + const uint8_t *buf, >> + const uint8_t *oob) >> { >> int eccsize = chip->ecc.size; >> int eccbytes = chip->ecc.bytes; >> - uint8_t *oob = chip->oob_poi; >> + uint8_t *oobbuf = chip->oob_poi; >> int steps, size; >> >> for (steps = chip->ecc.steps; steps > 0; steps--) { >> @@ -1951,31 +1959,32 @@ static void nand_write_page_raw_syndrome(struct mtd_info *mtd, >> buf += eccsize; >> >> if (chip->ecc.prepad) { >> - chip->write_buf(mtd, oob, chip->ecc.prepad); >> - oob += chip->ecc.prepad; >> + chip->write_buf(mtd, oobbuf, chip->ecc.prepad); >> + oobbuf += chip->ecc.prepad; >> } >> >> - chip->read_buf(mtd, oob, eccbytes); >> - oob += eccbytes; >> + chip->read_buf(mtd, oobbuf, eccbytes); >> + oobbuf += eccbytes; >> >> if (chip->ecc.postpad) { >> - chip->write_buf(mtd, oob, chip->ecc.postpad); >> - oob += chip->ecc.postpad; >> + chip->write_buf(mtd, oobbuf, chip->ecc.postpad); >> + oobbuf += chip->ecc.postpad; >> } >> } >> >> - size = mtd->oobsize - (oob - chip->oob_poi); >> + size = mtd->oobsize - (oobbuf - chip->oob_poi); >> if (size) >> - chip->write_buf(mtd, oob, size); >> + chip->write_buf(mtd, oobbuf, size); >> } >> /** >> * nand_write_page_swecc - [REPLACEABLE] software ECC based page write function >> * @mtd: mtd info structure >> * @chip: nand chip info structure >> * @buf: data buffer >> + * @oob: OOB buffer >> */ >> static void nand_write_page_swecc(struct mtd_info *mtd, struct nand_chip *chip, >> - const uint8_t *buf) >> + const uint8_t *buf, const uint8_t *oob) >> { >> int i, eccsize = chip->ecc.size; >> int eccbytes = chip->ecc.bytes; >> @@ -1991,7 +2000,7 @@ static void nand_write_page_swecc(struct mtd_info *mtd, struct nand_chip *chip, >> for (i = 0; i < chip->ecc.total; i++) >> chip->oob_poi[eccpos[i]] = ecc_calc[i]; >> >> - chip->ecc.write_page_raw(mtd, chip, buf); >> + chip->ecc.write_page_raw(mtd, chip, buf, oob); >> } >> >> /** >> @@ -1999,9 +2008,10 @@ static void nand_write_page_swecc(struct mtd_info *mtd, struct nand_chip *chip, >> * @mtd: mtd info structure >> * @chip: nand chip info structure >> * @buf: data buffer >> + * @oob: OOB buffer >> */ >> static void nand_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, >> - const uint8_t *buf) >> + const uint8_t *buf, const uint8_t *oob) >> { >> int i, eccsize = chip->ecc.size; >> int eccbytes = chip->ecc.bytes; >> @@ -2027,18 +2037,20 @@ static void nand_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, >> * @mtd: mtd info structure >> * @chip: nand chip info structure >> * @buf: data buffer >> + * @oob: OOB data buffer >> * >> * The hw generator calculates the error syndrome automatically. Therefore we >> * need a special oob layout and handling. >> */ >> static void nand_write_page_syndrome(struct mtd_info *mtd, >> - struct nand_chip *chip, const uint8_t *buf) >> + struct nand_chip *chip, const uint8_t *buf, >> + const uint8_t *oob) >> { >> int i, eccsize = chip->ecc.size; >> int eccbytes = chip->ecc.bytes; >> int eccsteps = chip->ecc.steps; >> const uint8_t *p = buf; >> - uint8_t *oob = chip->oob_poi; >> + uint8_t *oobbuf = chip->oob_poi; >> >> for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) { >> >> @@ -2046,24 +2058,24 @@ static void nand_write_page_syndrome(struct mtd_info *mtd, >> chip->write_buf(mtd, p, eccsize); >> >> if (chip->ecc.prepad) { >> - chip->write_buf(mtd, oob, chip->ecc.prepad); >> - oob += chip->ecc.prepad; >> + chip->write_buf(mtd, oobbuf, chip->ecc.prepad); >> + oobbuf += chip->ecc.prepad; >> } >> >> - chip->ecc.calculate(mtd, p, oob); >> - chip->write_buf(mtd, oob, eccbytes); >> - oob += eccbytes; >> + chip->ecc.calculate(mtd, p, oobbuf); >> + chip->write_buf(mtd, oobbuf, eccbytes); >> + oobbuf += eccbytes; >> >> if (chip->ecc.postpad) { >> - chip->write_buf(mtd, oob, chip->ecc.postpad); >> - oob += chip->ecc.postpad; >> + chip->write_buf(mtd, oobbuf, chip->ecc.postpad); >> + oobbuf += chip->ecc.postpad; >> } >> } >> >> /* Calculate remaining oob bytes */ >> - i = mtd->oobsize - (oob - chip->oob_poi); >> + i = mtd->oobsize - (oobbuf - chip->oob_poi); >> if (i) >> - chip->write_buf(mtd, oob, i); >> + chip->write_buf(mtd, oobbuf, i); >> } >> >> /** >> @@ -2076,16 +2088,17 @@ static void nand_write_page_syndrome(struct mtd_info *mtd, >> * @raw: use _raw version of write_page >> */ >> static int nand_write_page(struct mtd_info *mtd, struct nand_chip *chip, >> - const uint8_t *buf, int page, int cached, int raw) >> + const uint8_t *buf, const uint8_t *oob, int page, >> + int cached, int raw) >> { >> int status; >> >> chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page); >> >> if (unlikely(raw)) >> - chip->ecc.write_page_raw(mtd, chip, buf); >> + chip->ecc.write_page_raw(mtd, chip, buf, oob); >> else >> - chip->ecc.write_page(mtd, chip, buf); >> + chip->ecc.write_page(mtd, chip, buf, oob); >> >> /* >> * Cached progamming disabled for now. Not sure if it's worth the >> @@ -2264,7 +2277,7 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to, >> memset(chip->oob_poi, 0xff, mtd->oobsize); >> } >> >> - ret = chip->write_page(mtd, chip, wbuf, page, cached, >> + ret = chip->write_page(mtd, chip, wbuf, NULL, page, cached, >> (ops->mode == MTD_OPS_RAW)); >> if (ret) >> break; >> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c >> index def50ca..92d2eae 100644 >> --- a/drivers/mtd/nand/pxa3xx_nand.c >> +++ b/drivers/mtd/nand/pxa3xx_nand.c >> @@ -682,14 +682,14 @@ static void pxa3xx_nand_cmdfunc(struct mtd_info *mtd, unsigned command, >> } >> >> static void pxa3xx_nand_write_page_hwecc(struct mtd_info *mtd, >> - struct nand_chip *chip, const uint8_t *buf) >> + struct nand_chip *chip, const uint8_t *buf, const uint8_t *oob) >> { >> chip->write_buf(mtd, buf, mtd->writesize); >> chip->write_buf(mtd, chip->oob_poi, mtd->oobsize); >> } >> >> static int pxa3xx_nand_read_page_hwecc(struct mtd_info *mtd, >> - struct nand_chip *chip, uint8_t *buf, int page) >> + struct nand_chip *chip, uint8_t *buf, uint8_t *oob, int page) >> { >> struct pxa3xx_nand_host *host = mtd->priv; >> struct pxa3xx_nand_info *info = host->info_data; >> diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c >> index e9b2b26..d1b2731 100644 >> --- a/drivers/mtd/nand/sh_flctl.c >> +++ b/drivers/mtd/nand/sh_flctl.c >> @@ -344,7 +344,7 @@ static void set_cmd_regs(struct mtd_info *mtd, uint32_t cmd, uint32_t flcmcdr_va >> } >> >> static int flctl_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, >> - uint8_t *buf, int page) >> + uint8_t *buf, uint8_t *oob, int page) >> { >> int i, eccsize = chip->ecc.size; >> int eccbytes = chip->ecc.bytes; >> @@ -366,7 +366,7 @@ static int flctl_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, >> } >> >> static void flctl_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, >> - const uint8_t *buf) >> + const uint8_t *buf, const uint8_t *oob) >> { >> int i, eccsize = chip->ecc.size; >> int eccbytes = chip->ecc.bytes; >> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h >> index 1482340..be9ee1f 100644 >> --- a/include/linux/mtd/nand.h >> +++ b/include/linux/mtd/nand.h >> @@ -363,15 +363,15 @@ struct nand_ecc_ctrl { >> int (*correct)(struct mtd_info *mtd, uint8_t *dat, uint8_t *read_ecc, >> uint8_t *calc_ecc); >> int (*read_page_raw)(struct mtd_info *mtd, struct nand_chip *chip, >> - uint8_t *buf, int page); >> + uint8_t *buf, uint8_t *oob, int page); >> void (*write_page_raw)(struct mtd_info *mtd, struct nand_chip *chip, >> - const uint8_t *buf); >> + const uint8_t *buf, const uint8_t *oob); >> int (*read_page)(struct mtd_info *mtd, struct nand_chip *chip, >> - uint8_t *buf, int page); >> + uint8_t *buf, uint8_t *oob, int page); >> int (*read_subpage)(struct mtd_info *mtd, struct nand_chip *chip, >> uint32_t offs, uint32_t len, uint8_t *buf); >> void (*write_page)(struct mtd_info *mtd, struct nand_chip *chip, >> - const uint8_t *buf); >> + const uint8_t *buf, const uint8_t *oob); >> int (*write_oob_raw)(struct mtd_info *mtd, struct nand_chip *chip, >> int page); >> int (*read_oob_raw)(struct mtd_info *mtd, struct nand_chip *chip, >> @@ -505,7 +505,8 @@ struct nand_chip { >> int (*errstat)(struct mtd_info *mtd, struct nand_chip *this, int state, >> int status, int page); >> int (*write_page)(struct mtd_info *mtd, struct nand_chip *chip, >> - const uint8_t *buf, int page, int cached, int raw); >> + const uint8_t *buf, const uint8_t *oob, int page, >> + int cached, int raw); >> >> int chip_delay; >> unsigned int options; >> -- >> 1.7.5.4.2.g519b1 >> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces 2012-04-18 9:37 ` Bastian Hecht @ 2012-04-18 16:22 ` Brian Norris 2012-04-19 9:26 ` Bastian Hecht 0 siblings, 1 reply; 27+ messages in thread From: Brian Norris @ 2012-04-18 16:22 UTC (permalink / raw) To: Bastian Hecht Cc: Viresh Kumar, Artem Bityutskiy, Nicolas Ferre, Vipin Kumar, linux-mtd, Laurent Pinchart, Florian Fainelli, Jamie Iles, Mike Dunn, Bastian Hecht, Dmitry Eremin-Solenikov, Kevin Cernekee, Lei Wen, Axel Lin, Li Yang, Jean-Christophe PLAGNIOL-VILLARD, Armando Visconti, Liu Shuo, Thomas Gleixner, Scott Branden, Artem Bityutskiy, Wolfram Sang, Huang Shijie, Shmulik Ladkani, Jiandong Zheng, David Woodhouse Hi Bastian, On 4/18/2012 2:37 AM, Bastian Hecht wrote: > 2012/4/18 Bastian Hecht<hechtb@googlemail.com>: >> I'm currently working on the hardware ECC part of the SH Mobile flctl >> and I like the patchset as it makes things more explicit when OOB data >> is requested or not and such things. It's cleaner and better, thanks! Glad it will help! It's good to know other drivers will see a benefit. > To be a bit more of a concrete help, if you can give me a use case to > test the oob reads/writes I can modify my driver to work with your > patches. Right now I rely on nandwrite, nanddump, nandtest, the kernel > test modules and ubi. With none of them I produced an error so far > although the current implementation of the hardware ECC completely > ignores OOB data other than ECC. Well, for me the big "use case" is that I can support my new DMA controller (that doesn't read/write OOB) without the rest of MTD/NAND choking on anything or returning junk data. So I mostly make sure that nanddump/nandwrite work both with and without OOB (the -o flag) or ECC (the -n flag); that the kernel test modules work; and that UBI/UBIFS work on top of MTD. I don't know if there are any more specific tests you would need to run; are you looking for something else? Brian ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces 2012-04-18 16:22 ` Brian Norris @ 2012-04-19 9:26 ` Bastian Hecht 0 siblings, 0 replies; 27+ messages in thread From: Bastian Hecht @ 2012-04-19 9:26 UTC (permalink / raw) To: Brian Norris Cc: Viresh Kumar, Artem Bityutskiy, Nicolas Ferre, Vipin Kumar, linux-mtd, Laurent Pinchart, Florian Fainelli, Jamie Iles, Mike Dunn, Bastian Hecht, Dmitry Eremin-Solenikov, Kevin Cernekee, Lei Wen, Axel Lin, Li Yang, Jean-Christophe PLAGNIOL-VILLARD, Armando Visconti, Liu Shuo, Thomas Gleixner, Scott Branden, Artem Bityutskiy, Wolfram Sang, Huang Shijie, Shmulik Ladkani, Jiandong Zheng, David Woodhouse Hi Brian, 2012/4/18 Brian Norris <computersforpeace@gmail.com>: > Hi Bastian, > > On 4/18/2012 2:37 AM, Bastian Hecht wrote: >> >> 2012/4/18 Bastian Hecht<hechtb@googlemail.com>: >> >>> I'm currently working on the hardware ECC part of the SH Mobile flctl >>> and I like the patchset as it makes things more explicit when OOB data >>> is requested or not and such things. It's cleaner and better, thanks! > > > Glad it will help! It's good to know other drivers will see a benefit. > > >> To be a bit more of a concrete help, if you can give me a use case to >> test the oob reads/writes I can modify my driver to work with your >> patches. Right now I rely on nandwrite, nanddump, nandtest, the kernel >> test modules and ubi. With none of them I produced an error so far >> although the current implementation of the hardware ECC completely >> ignores OOB data other than ECC. > > > Well, for me the big "use case" is that I can support my new DMA controller > (that doesn't read/write OOB) without the rest of MTD/NAND choking on > anything or returning junk data. So I mostly make sure that > nanddump/nandwrite work both with and without OOB (the -o flag) or ECC (the > -n flag); that the kernel test modules work; and that UBI/UBIFS work on top > of MTD. Ah very nice! You made me look at the nandwrite -o thing once again. I used it before without success, there was no write at all when I issued it, but now ( - I compiled the git version of mtd-utils and replaced the debian prehistoric prepackaged one with it - ) it works! Now I can finish my work on the flctl ECC part. Thanks! > I don't know if there are any more specific tests you would need to run; are > you looking for something else? Nope, thanks. cheers, Bastian Hecht > Brian ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 2/2] mtd: nand: nand_do_{read, write}_ops - pass OOB buffer through 2012-04-16 22:35 [PATCH 0/2] mtd: nand: rework nand_ecc_ctrl interface for OOB Brian Norris 2012-04-16 22:35 ` [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces Brian Norris @ 2012-04-16 22:35 ` Brian Norris 2012-04-18 11:52 ` [PATCH 2/2] mtd: nand: nand_do_{read,write}_ops " Shmulik Ladkani 2012-04-25 14:16 ` [PATCH 0/2] mtd: nand: rework nand_ecc_ctrl interface for OOB Artem Bityutskiy 2 siblings, 1 reply; 27+ messages in thread From: Brian Norris @ 2012-04-16 22:35 UTC (permalink / raw) To: linux-mtd Cc: Viresh Kumar, Artem Bityutskiy, Nicolas Ferre, Vipin Kumar, Laurent Pinchart, Florian Fainelli, Jamie Iles, Mike Dunn, Bastian Hecht, Dmitry Eremin-Solenikov, Kevin Cernekee, Lei Wen, Axel Lin, Li Yang, Jean-Christophe PLAGNIOL-VILLARD, Armando Visconti, Liu Shuo, Thomas Gleixner, Scott Branden, Artem Bityutskiy, Wolfram Sang, Huang Shijie, Shmulik Ladkani, Jiandong Zheng, Brian Norris, David Woodhouse Now that we have a function parameter for the OOB buffer, we can pass the OOB buffer as an argument to the nand_ecc_ctrl functions. This allows drivers to know when OOB data must be returned to the upper layers and when it is simply needed for internal calculations, potentially saving time for NAND HW/SW that can simply avoid reading the OOB data. Signed-off-by: Brian Norris <computersforpeace@gmail.com> --- drivers/mtd/nand/nand_base.c | 14 ++++++++------ 1 files changed, 8 insertions(+), 6 deletions(-) diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 95ba987..a206f43 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -1475,7 +1475,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from, uint32_t max_oobsize = ops->mode == MTD_OPS_AUTO_OOB ? mtd->oobavail : mtd->oobsize; - uint8_t *bufpoi, *oob, *buf; + uint8_t *bufpoi, *oobpoi, *oob, *buf; stats = mtd->ecc_stats; @@ -1489,6 +1489,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from, buf = ops->datbuf; oob = ops->oobbuf; + oobpoi = oob ? chip->oob_poi : NULL; while (1) { bytes = min(mtd->writesize - col, readlen); @@ -1506,13 +1507,13 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from, /* Now read the page into the buffer */ if (unlikely(ops->mode == MTD_OPS_RAW)) ret = chip->ecc.read_page_raw(mtd, chip, bufpoi, - NULL, page); + oobpoi, page); else if (!aligned && NAND_SUBPAGE_READ(chip) && !oob) ret = chip->ecc.read_subpage(mtd, chip, col, bytes, bufpoi); else ret = chip->ecc.read_page(mtd, chip, bufpoi, - NULL, page); + oobpoi, page); if (ret < 0) { if (!aligned) /* Invalidate page cache */ @@ -1535,7 +1536,6 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from, buf += bytes; if (unlikely(oob)) { - int toread = min(oobreadlen, max_oobsize); if (toread) { @@ -2256,7 +2256,7 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to, while (1) { int bytes = mtd->writesize; int cached = writelen > bytes && page != blockmask; - uint8_t *wbuf = buf; + uint8_t *wbuf = buf, *oobpoi; /* Partial page write? */ if (unlikely(column || writelen < (mtd->writesize - 1))) { @@ -2272,12 +2272,14 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to, size_t len = min(oobwritelen, oobmaxlen); oob = nand_fill_oob(mtd, oob, len, ops); oobwritelen -= len; + oobpoi = chip->oob_poi; } else { + oobpoi = NULL; /* We still need to erase leftover OOB data */ memset(chip->oob_poi, 0xff, mtd->oobsize); } - ret = chip->write_page(mtd, chip, wbuf, NULL, page, cached, + ret = chip->write_page(mtd, chip, wbuf, oobpoi, page, cached, (ops->mode == MTD_OPS_RAW)); if (ret) break; -- 1.7.5.4.2.g519b1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] mtd: nand: nand_do_{read,write}_ops - pass OOB buffer through 2012-04-16 22:35 ` [PATCH 2/2] mtd: nand: nand_do_{read, write}_ops - pass OOB buffer through Brian Norris @ 2012-04-18 11:52 ` Shmulik Ladkani 2012-04-18 16:13 ` [PATCH 2/2] mtd: nand: nand_do_{read, write}_ops " Brian Norris 0 siblings, 1 reply; 27+ messages in thread From: Shmulik Ladkani @ 2012-04-18 11:52 UTC (permalink / raw) To: Brian Norris Cc: Viresh Kumar, Artem Bityutskiy, Nicolas Ferre, Vipin Kumar, linux-mtd, Laurent Pinchart, Florian Fainelli, Jamie Iles, Mike Dunn, Bastian Hecht, Dmitry Eremin-Solenikov, Kevin Cernekee, Lei Wen, Axel Lin, Li Yang, Jean-Christophe PLAGNIOL-VILLARD, Armando Visconti, Liu Shuo, Thomas Gleixner, Scott Branden, Artem Bityutskiy, Wolfram Sang, Huang Shijie, Jiandong Zheng, David Woodhouse Hi Brian, On Mon, 16 Apr 2012 15:35:55 -0700 Brian Norris <computersforpeace@gmail.com> wrote: > Now that we have a function parameter for the OOB buffer, we can pass the OOB > buffer as an argument to the nand_ecc_ctrl functions. This allows drivers to > know when OOB data must be returned to the upper layers and when it is simply > needed for internal calculations, potentially saving time for NAND HW/SW that > can simply avoid reading the OOB data. I think for consistency sake, existing chip->ecc.{read,write}_page_xxx methods do need to be ported to support the new 'oob' parameter. > @@ -2272,12 +2272,14 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to, > size_t len = min(oobwritelen, oobmaxlen); > oob = nand_fill_oob(mtd, oob, len, ops); > oobwritelen -= len; > + oobpoi = chip->oob_poi; > } else { > + oobpoi = NULL; > /* We still need to erase leftover OOB data */ > memset(chip->oob_poi, 0xff, mtd->oobsize); > } > > - ret = chip->write_page(mtd, chip, wbuf, NULL, page, cached, > + ret = chip->write_page(mtd, chip, wbuf, oobpoi, page, cached, > (ops->mode == MTD_OPS_RAW)); > if (ret) > break; The 'write_page' interface is problematic, as the meaning of 'oob' parameter is a bit inconsistent: - A NULL 'oob' actually states "no OOB buffer to write" - Your driver instructs HW to write the page (ECC taken care of by HW) - However default chip->ecc.write_page_xxx methods do need a temp buffer for OOB ECC calculation (hence will probably use the internal chip->oob_poi buffer) - But when non-null 'oob' is passed to the default methods, they should probably use the given 'oob' buffer (and not a temp buffer) (This is same for the read interface.) So the 'oob' parameter is more of a boolean than an actual buffer to be used by the various ecc.{read,write}_page implementors. Any reason not to pass a boolean instead? Regards, Shmulik ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] mtd: nand: nand_do_{read, write}_ops - pass OOB buffer through 2012-04-18 11:52 ` [PATCH 2/2] mtd: nand: nand_do_{read,write}_ops " Shmulik Ladkani @ 2012-04-18 16:13 ` Brian Norris 2012-04-18 19:43 ` [PATCH 2/2] mtd: nand: nand_do_{read,write}_ops " Shmulik Ladkani 0 siblings, 1 reply; 27+ messages in thread From: Brian Norris @ 2012-04-18 16:13 UTC (permalink / raw) To: Shmulik Ladkani Cc: Viresh Kumar, Artem Bityutskiy, Nicolas Ferre, Vipin Kumar, linux-mtd, Laurent Pinchart, Florian Fainelli, Jamie Iles, Mike Dunn, Bastian Hecht, Dmitry Eremin-Solenikov, Kevin Cernekee, Lei Wen, Axel Lin, Li Yang, Jean-Christophe PLAGNIOL-VILLARD, Armando Visconti, Liu Shuo, Thomas Gleixner, Scott Branden, Artem Bityutskiy, Wolfram Sang, Huang Shijie, Jiandong Zheng, David Woodhouse On 4/18/2012 4:52 AM, Shmulik Ladkani wrote: > On Mon, 16 Apr 2012 15:35:55 -0700 Brian Norris<computersforpeace@gmail.com> wrote: >> Now that we have a function parameter for the OOB buffer, we can pass the OOB >> buffer as an argument to the nand_ecc_ctrl functions. This allows drivers to >> know when OOB data must be returned to the upper layers and when it is simply >> needed for internal calculations, potentially saving time for NAND HW/SW that >> can simply avoid reading the OOB data. > > I think for consistency sake, existing chip->ecc.{read,write}_page_xxx > methods do need to be ported to support the new 'oob' parameter. OK, but it's difficult to tell sometimes what is and isn't needed; some drivers might expect OOB data in chip->oob_poi unconditionally so they can perform correction, whereas others might fill up buffers that won't be used in the end. >> @@ -2272,12 +2272,14 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to, >> size_t len = min(oobwritelen, oobmaxlen); >> oob = nand_fill_oob(mtd, oob, len, ops); >> oobwritelen -= len; >> + oobpoi = chip->oob_poi; >> } else { >> + oobpoi = NULL; >> /* We still need to erase leftover OOB data */ >> memset(chip->oob_poi, 0xff, mtd->oobsize); >> } >> >> - ret = chip->write_page(mtd, chip, wbuf, NULL, page, cached, >> + ret = chip->write_page(mtd, chip, wbuf, oobpoi, page, cached, >> (ops->mode == MTD_OPS_RAW)); >> if (ret) >> break; > > The 'write_page' interface is problematic, as the meaning of 'oob' > parameter is a bit inconsistent: > - A NULL 'oob' actually states "no OOB buffer to write" > - Your driver instructs HW to write the page (ECC taken care of by HW) > - However default chip->ecc.write_page_xxx methods do need a temp buffer > for OOB ECC calculation (hence will probably use the internal > chip->oob_poi buffer) Right, this is a trouble spot for 'porting them to support the new oob parameter', since many driver-users still need a buffer even when OOB is not needed for the higher levels. > - But when non-null 'oob' is passed to the default methods, they should > probably use the given 'oob' buffer (and not a temp buffer) Yes. This gets strange and potentially ugly, with code snippets like below. > (This is same for the read interface.) > > So the 'oob' parameter is more of a boolean than an actual buffer to be > used by the various ecc.{read,write}_page implementors. Yes, I suppose so. I naturally used 'oob' as a buffer, since that's very straightforward and logical from a 'layers' perspective and because my driver doesn't need any buffer when oob is not required. But I see that it essentially would become a boolean flag for many of the other interfaces, and so a boolean can work just as well. > Any reason not to pass a boolean instead? Only reason I'm thinking of: a cleaner interface. To me, the interface is rather non-obvious and ugly when data is constantly shuttled back and forth behind the scenes (i.e., not via function arguments or ret values) by using chip->oob_poi. However, this sense of "ugliness" competes with the ugliness of needing a buffer even when the interface might otherwise say "no OOB." Many {read,write}_page functions would need something like: uint8_t *oobbuf = oob ? oob : chip->oob_poi; which is not pretty. I'm open to either way, I guess, but I'm now leaning a little toward 'oob' as a boolean. Brian ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] mtd: nand: nand_do_{read,write}_ops - pass OOB buffer through 2012-04-18 16:13 ` [PATCH 2/2] mtd: nand: nand_do_{read, write}_ops " Brian Norris @ 2012-04-18 19:43 ` Shmulik Ladkani 0 siblings, 0 replies; 27+ messages in thread From: Shmulik Ladkani @ 2012-04-18 19:43 UTC (permalink / raw) To: Brian Norris Cc: Viresh Kumar, Artem Bityutskiy, Nicolas Ferre, Vipin Kumar, linux-mtd, Laurent Pinchart, Florian Fainelli, Jamie Iles, Mike Dunn, Bastian Hecht, Dmitry Eremin-Solenikov, Kevin Cernekee, Lei Wen, Axel Lin, Li Yang, Jean-Christophe PLAGNIOL-VILLARD, Armando Visconti, Thomas Gleixner, Scott Branden, Artem Bityutskiy, Wolfram Sang, Huang Shijie, Jiandong Zheng, David Woodhouse Hi Brian, On Wed, 18 Apr 2012 09:13:38 -0700 Brian Norris <computersforpeace@gmail.com> wrote: > > So the 'oob' parameter is more of a boolean than an actual buffer to be > > used by the various ecc.{read,write}_page implementors. > > Yes, I suppose so. I naturally used 'oob' as a buffer, since that's very > straightforward and logical from a 'layers' perspective and because my > driver doesn't need any buffer when oob is not required. But I see that > it essentially would become a boolean flag for many of the other > interfaces, and so a boolean can work just as well. > > > Any reason not to pass a boolean instead? > > Only reason I'm thinking of: a cleaner interface. > > To me, the interface is rather non-obvious and ugly when data is > constantly shuttled back and forth behind the scenes (i.e., not via > function arguments or ret values) by using chip->oob_poi. > > However, this sense of "ugliness" competes with the ugliness of needing > a buffer even when the interface might otherwise say "no OOB." Many > {read,write}_page functions would need something like: > > uint8_t *oobbuf = oob ? oob : chip->oob_poi; > > which is not pretty. > > I'm open to either way, I guess, but I'm now leaning a little toward > 'oob' as a boolean. Yes, both approaches aren't perfect. However a 'int has_user_oob' (or 'has_oob') has a precise, consistent meaning: the mtd user explicitly provided an OOB buffer (or in the read case, the mtd user expects the OOB to be returned). (need help with the boolean's name, though) Also, I think it will result in less changes (makes the whole s/oob/oobbuf/ collateral damage of 1st patch irrelevant). So IMO a boolean is better. Regards, Shmulik ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 0/2] mtd: nand: rework nand_ecc_ctrl interface for OOB 2012-04-16 22:35 [PATCH 0/2] mtd: nand: rework nand_ecc_ctrl interface for OOB Brian Norris 2012-04-16 22:35 ` [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces Brian Norris 2012-04-16 22:35 ` [PATCH 2/2] mtd: nand: nand_do_{read, write}_ops - pass OOB buffer through Brian Norris @ 2012-04-25 14:16 ` Artem Bityutskiy 2012-04-25 18:26 ` Brian Norris 2 siblings, 1 reply; 27+ messages in thread From: Artem Bityutskiy @ 2012-04-25 14:16 UTC (permalink / raw) To: Brian Norris Cc: Viresh Kumar, Artem Bityutskiy, Nicolas Ferre, Vipin Kumar, linux-mtd, Laurent Pinchart, Florian Fainelli, Jamie Iles, Mike Dunn, Bastian Hecht, Dmitry Eremin-Solenikov, Kevin Cernekee, Lei Wen, Axel Lin, Li Yang, Jean-Christophe PLAGNIOL-VILLARD, Armando Visconti, Liu Shuo, Thomas Gleixner, Scott Branden, Wolfram Sang, Huang Shijie, Shmulik Ladkani, Jiandong Zheng, David Woodhouse [-- Attachment #1: Type: text/plain, Size: 636 bytes --] On Mon, 2012-04-16 at 15:35 -0700, Brian Norris wrote: > Note that I could not compile all the affected drivers, since some required > ARCH-specific builds that I am not familiar with. > > Artem: can you perform your regular compile tests? I compile-tested as many as > I could. Yes, I compile-test for several architecture/platforms, but not sure I cover everything. I'll check your patches once I reach that point in my inbox queue. I am currently having paternity leave, and I'll be very busy once I return back to work, but I am slowly processing my inbox anyway in background. -- Best Regards, Artem Bityutskiy [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 490 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 0/2] mtd: nand: rework nand_ecc_ctrl interface for OOB 2012-04-25 14:16 ` [PATCH 0/2] mtd: nand: rework nand_ecc_ctrl interface for OOB Artem Bityutskiy @ 2012-04-25 18:26 ` Brian Norris 0 siblings, 0 replies; 27+ messages in thread From: Brian Norris @ 2012-04-25 18:26 UTC (permalink / raw) To: dedekind1; +Cc: linux-mtd On Wed, Apr 25, 2012 at 7:16 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > On Mon, 2012-04-16 at 15:35 -0700, Brian Norris wrote: >> Note that I could not compile all the affected drivers, since some required >> ARCH-specific builds that I am not familiar with. >> >> Artem: can you perform your regular compile tests? I compile-tested as many as >> I could. > > Yes, I compile-test for several architecture/platforms, but not sure I > cover everything. I'll check your patches once I reach that point in my > inbox queue. I am currently having paternity leave, and I'll be very > busy once I return back to work, but I am slowly processing my inbox > anyway in background. Right, no problem. We're all busy, and I know you get around to it eventually :) I just wanted to make a special note of it, since this sort of patch series touches more territory than I'm used to, and I'm almost guaranteed to miss something. Brian ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2012-04-25 18:26 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-16 22:35 [PATCH 0/2] mtd: nand: rework nand_ecc_ctrl interface for OOB Brian Norris
2012-04-16 22:35 ` [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces Brian Norris
2012-04-17 7:50 ` Matthieu CASTET
2012-04-18 3:44 ` Brian Norris
2012-04-19 16:50 ` Mike Dunn
2012-04-19 22:06 ` Brian Norris
2012-04-20 1:10 ` Jon Povey
2012-04-20 16:25 ` Mike Dunn
2012-04-20 19:19 ` Brian Norris
2012-04-20 16:17 ` Mike Dunn
2012-04-22 7:58 ` Shmulik Ladkani
2012-04-23 9:14 ` Bastian Hecht
2012-04-23 17:14 ` Mike Dunn
2012-04-24 6:02 ` Shmulik Ladkani
2012-04-25 13:17 ` Bastian Hecht
2012-04-17 14:29 ` [PATCH 1/2] mtd: nand: add OOB argument to NAND {read,write}_page interfaces Shmulik Ladkani
2012-04-18 4:11 ` [PATCH 1/2] mtd: nand: add OOB argument to NAND {read, write}_page interfaces Brian Norris
2012-04-18 7:56 ` Bastian Hecht
2012-04-18 9:37 ` Bastian Hecht
2012-04-18 16:22 ` Brian Norris
2012-04-19 9:26 ` Bastian Hecht
2012-04-16 22:35 ` [PATCH 2/2] mtd: nand: nand_do_{read, write}_ops - pass OOB buffer through Brian Norris
2012-04-18 11:52 ` [PATCH 2/2] mtd: nand: nand_do_{read,write}_ops " Shmulik Ladkani
2012-04-18 16:13 ` [PATCH 2/2] mtd: nand: nand_do_{read, write}_ops " Brian Norris
2012-04-18 19:43 ` [PATCH 2/2] mtd: nand: nand_do_{read,write}_ops " Shmulik Ladkani
2012-04-25 14:16 ` [PATCH 0/2] mtd: nand: rework nand_ecc_ctrl interface for OOB Artem Bityutskiy
2012-04-25 18:26 ` Brian Norris
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.