Patchwork [8/9] mtd: sh_flctl: Use user oob data in hardware ECC mode

login
register
mail settings
Submitter Bastian Hecht
Date April 20, 2012, 9:13 a.m.
Message ID <1334913230-23615-9-git-send-email-hechtb@gmail.com>
Download mbox | patch
Permalink /patch/153977/
State New
Headers show

Comments

Bastian Hecht - April 20, 2012, 9:13 a.m.
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(-)
Laurent Pinchart - April 21, 2012, 3 p.m.
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>
Bastian Hecht - April 23, 2012, 9:05 a.m.
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
>
Bastian Hecht - April 23, 2012, 9:36 a.m.
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
>>
Laurent Pinchart - April 24, 2012, 9:33 a.m.
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.
Brian Norris - April 25, 2012, 4:01 a.m.
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
Bastian Hecht - April 25, 2012, 1:26 p.m.
>> 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

Patch

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);