| Message ID | 1334913230-23615-9-git-send-email-hechtb@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
Hi Bastian, Thanks for the patch. On Friday 20 April 2012 11:13:49 Bastian Hecht wrote: > In hardware ecc mode, the flctl now writes and reads the oob data > provided by the user. Additionally the ECC is now returned in normal > page reads, not only when using the explicit NAND_CMD_READOOB command. For my information again, what's the purpose of returning OOB data if the caller hasn't requested it ? What are those data then used for ? > Signed-off-by: Bastian Hecht <hechtb@gmail.com>
Hello Laurent, 2012/4/21 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > Hi Bastian, > > Thanks for the patch. > > On Friday 20 April 2012 11:13:49 Bastian Hecht wrote: >> In hardware ecc mode, the flctl now writes and reads the oob data >> provided by the user. Additionally the ECC is now returned in normal >> page reads, not only when using the explicit NAND_CMD_READOOB command. > > For my information again, what's the purpose of returning OOB data if the > caller hasn't requested it ? What are those data then used for ? There is an active discussion going on whether to pass a boolean to nand_{read,write} that indicates if we need oob data or not. I assume this to make it into the mainline then I can adapt this to the flctl driver. The data can be used by file systems or bad block marking or any other organizational needs like wear leveling and so on. Best regards, Bastian Hecht >> Signed-off-by: Bastian Hecht <hechtb@gmail.com> > > -- > Regards, > > Laurent Pinchart >
2012/4/23 Bastian Hecht <hechtb@googlemail.com>: > Hello Laurent, > > 2012/4/21 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: >> Hi Bastian, >> >> Thanks for the patch. >> >> On Friday 20 April 2012 11:13:49 Bastian Hecht wrote: >>> In hardware ecc mode, the flctl now writes and reads the oob data >>> provided by the user. Additionally the ECC is now returned in normal >>> page reads, not only when using the explicit NAND_CMD_READOOB command. >> >> For my information again, what's the purpose of returning OOB data if the >> caller hasn't requested it ? What are those data then used for ? > > There is an active discussion going on whether to pass a boolean to > nand_{read,write} that indicates if we need oob data or not. I assume > this to make it into the mainline then I can adapt this to the flctl > driver. The data can be used by file systems or bad block marking or > any other organizational needs like wear leveling and so on. I'm unsure if I missed your point here - we just don't know if we need it or not. The discussion I mentioned primarily takes place here at the mtd mailing list: [PATCH 1/2] mtd: nand: add OOB argument to NAND {read,write}_page interfaces http://lists.infradead.org/pipermail/linux-mtd/2012-April/040714.html Now I'm confused as well whether I should skip the read oob part of the patch. I'll skip the read part of the patch until a decision is made, I think. Best regards, Bastian Hecht > Best regards, > > Bastian Hecht > > >>> Signed-off-by: Bastian Hecht <hechtb@gmail.com> >> >> -- >> Regards, >> >> Laurent Pinchart >>
Hi Bastian, On Monday 23 April 2012 11:36:29 Bastian Hecht wrote: > 2012/4/23 Bastian Hecht <hechtb@googlemail.com>: > > 2012/4/21 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: > >> On Friday 20 April 2012 11:13:49 Bastian Hecht wrote: > >>> In hardware ecc mode, the flctl now writes and reads the oob data > >>> provided by the user. Additionally the ECC is now returned in normal > >>> page reads, not only when using the explicit NAND_CMD_READOOB command. > >> > >> For my information again, what's the purpose of returning OOB data if the > >> caller hasn't requested it ? What are those data then used for ? > > > > There is an active discussion going on whether to pass a boolean to > > nand_{read,write} that indicates if we need oob data or not. I assume > > this to make it into the mainline then I can adapt this to the flctl > > driver. The data can be used by file systems or bad block marking or > > any other organizational needs like wear leveling and so on. > > I'm unsure if I missed your point here - we just don't know if we need > it or not. The discussion I mentioned primarily takes place here at > the mtd mailing list: > > [PATCH 1/2] mtd: nand: add OOB argument to NAND {read,write}_page interfaces > http://lists.infradead.org/pipermail/linux-mtd/2012-April/040714.html > > Now I'm confused as well whether I should skip the read oob part of the > patch. I'll skip the read part of the patch until a decision is made, I > think. My point was just that it was pointless to read/write OOB data if the caller doesn't use them. It's an optimization: reading OOB data won't hurt regardless of what the caller does with it, but it will use CPU time and power for no reason. Adding an OOB argument to the {read,write}_page function would make this explicit.
On Tue, Apr 24, 2012 at 2:33 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Monday 23 April 2012 11:36:29 Bastian Hecht wrote: >> 2012/4/23 Bastian Hecht <hechtb@googlemail.com>: >> > 2012/4/21 Laurent Pinchart <laurent.pinchart@ideasonboard.com>: >> >> On Friday 20 April 2012 11:13:49 Bastian Hecht wrote: >> >>> In hardware ecc mode, the flctl now writes and reads the oob data >> >>> provided by the user. Additionally the ECC is now returned in normal >> >>> page reads, not only when using the explicit NAND_CMD_READOOB command. >> >> >> >> For my information again, what's the purpose of returning OOB data if the >> >> caller hasn't requested it ? What are those data then used for ? >> > >> > There is an active discussion going on whether to pass a boolean to >> > nand_{read,write} that indicates if we need oob data or not. I assume >> > this to make it into the mainline then I can adapt this to the flctl >> > driver. The data can be used by file systems or bad block marking or >> > any other organizational needs like wear leveling and so on. >> >> I'm unsure if I missed your point here - we just don't know if we need >> it or not. The discussion I mentioned primarily takes place here at >> the mtd mailing list: > > My point was just that it was pointless to read/write OOB data if the caller > doesn't use them. It's an optimization: reading OOB data won't hurt regardless > of what the caller does with it, but it will use CPU time and power for no > reason. Adding an OOB argument to the {read,write}_page function would make > this explicit. Right, it is pointless and should be changed fairly soon, if my patches go through. But until the additional argument is added, you cannot guarantee that the interface wasn't expecting both data+OOB to be read. For instance, the mtd_read_oob interface may call nand_do_read_ops() with non-null datbuf and oobbuf. We have just such a case in mtdswap.c and nand_bbt.c, I think. See Shmulik's comments here (some of which only apply to mtd_write_oob): http://lists.infradead.org/pipermail/linux-mtd/2012-April/040800.html Brian
>> My point was just that it was pointless to read/write OOB data if the caller >> doesn't use them. It's an optimization: reading OOB data won't hurt regardless >> of what the caller does with it, but it will use CPU time and power for no >> reason. Adding an OOB argument to the {read,write}_page function would make >> this explicit. > > Right, it is pointless and should be changed fairly soon, if my > patches go through. But until the additional argument is added, you > cannot guarantee that the interface wasn't expecting both data+OOB to > be read. For instance, the mtd_read_oob interface may call > nand_do_read_ops() with non-null datbuf and oobbuf. We have just such > a case in mtdswap.c and nand_bbt.c, I think. See Shmulik's comments > here (some of which only apply to mtd_write_oob): > > http://lists.infradead.org/pipermail/linux-mtd/2012-April/040800.html > > Brian So after some back and forth, I'll leave the 8/9 patch as it is to ensure compliance to the nand base code and wait for Brian's patches to make it into the mainline. Best regards, Bastian Hecht
diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c index 408e013..2766ce4 100644 --- a/drivers/mtd/nand/sh_flctl.c +++ b/drivers/mtd/nand/sh_flctl.c @@ -319,6 +319,19 @@ static void write_fiforeg(struct sh_flctl *flctl, int rlen, int offset) } } +static void write_ec_fiforeg(struct sh_flctl *flctl, int rlen, int offset) +{ + int i, len_4align; + unsigned long *data = (unsigned long *)&flctl->done_buff[offset]; + void *fifo_addr = (void *)FLECFIFO(flctl); + + len_4align = (rlen + 3) / 4; + for (i = 0; i < len_4align; i++) { + wait_wecfifo_ready(flctl); + writel(cpu_to_be32(data[i]), fifo_addr); + } +} + static void set_cmd_regs(struct mtd_info *mtd, uint32_t cmd, uint32_t flcmcdr_val) { struct sh_flctl *flctl = mtd_to_flctl(mtd); @@ -385,6 +398,7 @@ static int flctl_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, uint8_t *buf, int page) { chip->read_buf(mtd, buf, mtd->writesize); + chip->read_buf(mtd, chip->oob_poi, mtd->oobsize); return 0; } @@ -392,6 +406,7 @@ static void flctl_write_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, const uint8_t *buf) { chip->write_buf(mtd, buf, mtd->writesize); + chip->write_buf(mtd, chip->oob_poi, mtd->oobsize); } static void execmd_read_page_sector(struct mtd_info *mtd, int page_addr) @@ -467,7 +482,7 @@ static void execmd_read_oob(struct mtd_info *mtd, int page_addr) static void execmd_write_page_sector(struct mtd_info *mtd) { struct sh_flctl *flctl = mtd_to_flctl(mtd); - int i, page_addr = flctl->seqin_page_addr; + int page_addr = flctl->seqin_page_addr; int sector, page_sectors; page_sectors = flctl->page_size ? 4 : 1; @@ -483,11 +498,7 @@ static void execmd_write_page_sector(struct mtd_info *mtd) for (sector = 0; sector < page_sectors; sector++) { write_fiforeg(flctl, 512, 512 * sector); - - for (i = 0; i < 4; i++) { - wait_wecfifo_ready(flctl); /* wait for write ready */ - writel(0xFFFFFFFF, FLECFIFO(flctl)); - } + write_ec_fiforeg(flctl, 16, mtd->writesize + 16 * sector); } wait_completion(flctl);
In hardware ecc mode, the flctl now writes and reads the oob data provided by the user. Additionally the ECC is now returned in normal page reads, not only when using the explicit NAND_CMD_READOOB command. Signed-off-by: Bastian Hecht <hechtb@gmail.com> --- drivers/mtd/nand/sh_flctl.c | 23 +++++++++++++++++------ 1 files changed, 17 insertions(+), 6 deletions(-)