| Message ID | 20171028074351.13118-1-prasannatsmkumar@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Boris Brezillon |
| Headers | show |
| Series | [RFC] NAND: Optimize NAND_ECC_HW_OOB_FIRST read | expand |
Hi PrasannaKumar, On Sat, 28 Oct 2017 13:13:51 +0530 PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com> wrote: > From: Lars-Peter Clausen <lars@metafoo.de> > > Avoid sending unnecessary READ commands to the chip. > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> > Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com> > --- > This patch is taken from git://projects.qi-hardware.com/qi-kernel.git > branch jz-3.16. From [1] and [2] it can be seen that the patch author > thinks this can be sent upstream but it never happened so far. This > patch is used in OpenWRT as seen from [3]. Sounds reasonable, but it's likely to conflict with something I'd like to queue for 4.16 [1]. I'll rebase this patch on nand/next once the ->exec_op() series is merged. Don't hesitate to ping me if I forget. Regards, Boris [1]http://patchwork.ozlabs.org/project/linux-mtd/list/?series=8923 > > I have only compile tested the patch. > > 1. https://www.mail-archive.com/discussion@lists.en.qi-hardware.com/msg04635.html > 2. https://www.mail-archive.com/discussion@lists.en.qi-hardware.com/msg04639.html > 3. https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/xburst/patches-3.18/002-NAND-Optimize-NAND_ECC_HW_OOB_FIRST-read.patch;h=046da51912de3cd4444779df5de13d2c1999719a;hb=c03d4317a6bc891cb4a5e89cbdd77f37c23aff86 > > drivers/mtd/nand/nand_base.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index 12edaae..4bf3bdb 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -1680,9 +1680,15 @@ static int nand_read_page_hwecc_oob_first(struct mtd_info *mtd, > unsigned int max_bitflips = 0; > > /* Read the OOB area first */ > - chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page); > - chip->read_buf(mtd, chip->oob_poi, mtd->oobsize); > - chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page); > + if (mtd->writesize > 512) { > + chip->cmdfunc(mtd, NAND_CMD_READ0, mtd->writesize, page); > + chip->read_buf(mtd, chip->oob_poi, mtd->oobsize); > + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, 0, -1); > + } else { > + chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page); > + chip->read_buf(mtd, chip->oob_poi, mtd->oobsize); > + chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page); > + } > > ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code, chip->oob_poi, 0, > chip->ecc.total); > @@ -1902,8 +1908,10 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from, > __func__, buf); > > read_retry: > - if (nand_standard_page_accessors(&chip->ecc)) > + if (nand_standard_page_accessors(&chip->ecc) && > + chip->ecc.mode != NAND_ECC_HW_OOB_FIRST) { > chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page); > + } > > /* > * Now read the page into the buffer. Absent an error,
Hi Boris, On 30 October 2017 at 14:04, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > Hi PrasannaKumar, > > On Sat, 28 Oct 2017 13:13:51 +0530 > PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com> wrote: > >> From: Lars-Peter Clausen <lars@metafoo.de> >> >> Avoid sending unnecessary READ commands to the chip. >> >> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> >> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com> >> --- >> This patch is taken from git://projects.qi-hardware.com/qi-kernel.git >> branch jz-3.16. From [1] and [2] it can be seen that the patch author >> thinks this can be sent upstream but it never happened so far. This >> patch is used in OpenWRT as seen from [3]. > > Sounds reasonable, but it's likely to conflict with something I'd like > to queue for 4.16 [1]. I'll rebase this patch on nand/next once the > ->exec_op() series is merged. Don't hesitate to ping me if I forget. > > Regards, > > Boris > > [1]http://patchwork.ozlabs.org/project/linux-mtd/list/?series=8923 Okay. I will remind you if required. There is another patch used in OpenWRT at [1]. Do you mind to have a look at it and take that if it is useful? I don't see a point in me sending it as a RFC as I am not contributing anything to it other than noticing its existence. 1. https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/xburst/patches-3.18/003-NAND-Add-support-for-subpage-reads-for-NAND_ECC_HW_O.patch;h=974eb7a5db9a09347fa6137461f030575f9a4328;hb=c03d4317a6bc891cb4a5e89cbdd77f37c23aff86 Thanks, PrasannaKumar
On Mon, 30 Oct 2017 18:17:50 +0530 PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com> wrote: > Hi Boris, > > On 30 October 2017 at 14:04, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > Hi PrasannaKumar, > > > > On Sat, 28 Oct 2017 13:13:51 +0530 > > PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com> wrote: > > > >> From: Lars-Peter Clausen <lars@metafoo.de> > >> > >> Avoid sending unnecessary READ commands to the chip. > >> > >> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> > >> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com> > >> --- > >> This patch is taken from git://projects.qi-hardware.com/qi-kernel.git > >> branch jz-3.16. From [1] and [2] it can be seen that the patch author > >> thinks this can be sent upstream but it never happened so far. This > >> patch is used in OpenWRT as seen from [3]. > > > > Sounds reasonable, but it's likely to conflict with something I'd like > > to queue for 4.16 [1]. I'll rebase this patch on nand/next once the > > ->exec_op() series is merged. Don't hesitate to ping me if I forget. > > > > Regards, > > > > Boris > > > > [1]http://patchwork.ozlabs.org/project/linux-mtd/list/?series=8923 > > Okay. I will remind you if required. > > There is another patch used in OpenWRT at [1]. Do you mind to have a > look at it and take that if it is useful? I don't see a point in me > sending it as a RFC as I am not contributing anything to it other than > noticing its existence. Yep, it makes sense to have that one applied too, but this is pretty much the same problem: it will conflict with the ->exec_op() rework. Can you please send it to the ML so I can track it in patchwork and respin it when ->exec_op() is out. I'll also need someone owning a HW using NAND_ECC_HW_OOB_FIRST to test the changes. Do you have such HW? > > 1. https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/xburst/patches-3.18/003-NAND-Add-support-for-subpage-reads-for-NAND_ECC_HW_O.patch;h=974eb7a5db9a09347fa6137461f030575f9a4328;hb=c03d4317a6bc891cb4a5e89cbdd77f37c23aff86 > > Thanks, > PrasannaKumar
Hi Boris, On 30 October 2017 at 18:33, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > On Mon, 30 Oct 2017 18:17:50 +0530 > PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com> wrote: > >> Hi Boris, >> >> On 30 October 2017 at 14:04, Boris Brezillon >> <boris.brezillon@free-electrons.com> wrote: >> > Hi PrasannaKumar, >> > >> > On Sat, 28 Oct 2017 13:13:51 +0530 >> > PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com> wrote: >> > >> >> From: Lars-Peter Clausen <lars@metafoo.de> >> >> >> >> Avoid sending unnecessary READ commands to the chip. >> >> >> >> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> >> >> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com> >> >> --- >> >> This patch is taken from git://projects.qi-hardware.com/qi-kernel.git >> >> branch jz-3.16. From [1] and [2] it can be seen that the patch author >> >> thinks this can be sent upstream but it never happened so far. This >> >> patch is used in OpenWRT as seen from [3]. >> > >> > Sounds reasonable, but it's likely to conflict with something I'd like >> > to queue for 4.16 [1]. I'll rebase this patch on nand/next once the >> > ->exec_op() series is merged. Don't hesitate to ping me if I forget. >> > >> > Regards, >> > >> > Boris >> > >> > [1]http://patchwork.ozlabs.org/project/linux-mtd/list/?series=8923 >> >> Okay. I will remind you if required. >> >> There is another patch used in OpenWRT at [1]. Do you mind to have a >> look at it and take that if it is useful? I don't see a point in me >> sending it as a RFC as I am not contributing anything to it other than >> noticing its existence. > > Yep, it makes sense to have that one applied too, but this is pretty > much the same problem: it will conflict with the ->exec_op() rework. True. I did realise that. > Can you please send it to the ML so I can track it in patchwork and Sure. > respin it when ->exec_op() is out. I'll also need someone owning a HW > using NAND_ECC_HW_OOB_FIRST to test the changes. Do you have such HW? I am looking at OpenWRT and trying to post patches that I feel useful. I do not have access to JZ4740. Grepping over driver/mtd/nand I can find that only JZ4740 and DaVinci supports this. When I last searched I could not find people with access to Ingenic JZ4740 SoC (which is targeted by arch/mips/xburst in OpenWRT). I can check with people who have newer SoC from Ingenic if that has the required hardware. Please let me know how to test that patch if I could find someone who can test that patch. Thanks, PrasannaKumar
Hi Boris, On 30 October 2017 at 19:04, PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com> wrote: > Hi Boris, > > On 30 October 2017 at 18:33, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: >> >> Yep, it makes sense to have that one applied too, but this is pretty >> much the same problem: it will conflict with the ->exec_op() rework. > > True. I did realise that. > >> Can you please send it to the ML so I can track it in patchwork and > > Sure. I could not get that patch compiled yet. I will revisit it next week and send the patch. Regards, PrasannaKumar
Hi Boris, On 30 October 2017 at 14:04, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > Hi PrasannaKumar, > > On Sat, 28 Oct 2017 13:13:51 +0530 > PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com> wrote: > >> From: Lars-Peter Clausen <lars@metafoo.de> >> >> Avoid sending unnecessary READ commands to the chip. >> >> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> >> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com> >> --- >> This patch is taken from git://projects.qi-hardware.com/qi-kernel.git >> branch jz-3.16. From [1] and [2] it can be seen that the patch author >> thinks this can be sent upstream but it never happened so far. This >> patch is used in OpenWRT as seen from [3]. > > Sounds reasonable, but it's likely to conflict with something I'd like > to queue for 4.16 [1]. I'll rebase this patch on nand/next once the > ->exec_op() series is merged. Don't hesitate to ping me if I forget. > > Regards, > > Boris > > [1]http://patchwork.ozlabs.org/project/linux-mtd/list/?series=8923 > >> >> I have only compile tested the patch. >> >> 1. https://www.mail-archive.com/discussion@lists.en.qi-hardware.com/msg04635.html >> 2. https://www.mail-archive.com/discussion@lists.en.qi-hardware.com/msg04639.html >> 3. https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/xburst/patches-3.18/002-NAND-Optimize-NAND_ECC_HW_OOB_FIRST-read.patch;h=046da51912de3cd4444779df5de13d2c1999719a;hb=c03d4317a6bc891cb4a5e89cbdd77f37c23aff86 >> >> drivers/mtd/nand/nand_base.c | 16 ++++++++++++---- >> 1 file changed, 12 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c >> index 12edaae..4bf3bdb 100644 >> --- a/drivers/mtd/nand/nand_base.c >> +++ b/drivers/mtd/nand/nand_base.c >> @@ -1680,9 +1680,15 @@ static int nand_read_page_hwecc_oob_first(struct mtd_info *mtd, >> unsigned int max_bitflips = 0; >> >> /* Read the OOB area first */ >> - chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page); >> - chip->read_buf(mtd, chip->oob_poi, mtd->oobsize); >> - chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page); >> + if (mtd->writesize > 512) { >> + chip->cmdfunc(mtd, NAND_CMD_READ0, mtd->writesize, page); >> + chip->read_buf(mtd, chip->oob_poi, mtd->oobsize); >> + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, 0, -1); >> + } else { >> + chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page); >> + chip->read_buf(mtd, chip->oob_poi, mtd->oobsize); >> + chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page); >> + } >> >> ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code, chip->oob_poi, 0, >> chip->ecc.total); >> @@ -1902,8 +1908,10 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from, >> __func__, buf); >> >> read_retry: >> - if (nand_standard_page_accessors(&chip->ecc)) >> + if (nand_standard_page_accessors(&chip->ecc) && >> + chip->ecc.mode != NAND_ECC_HW_OOB_FIRST) { >> chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page); >> + } >> >> /* >> * Now read the page into the buffer. Absent an error, > Can you please revisit this? Thanks, PrasannaKumar
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 12edaae..4bf3bdb 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -1680,9 +1680,15 @@ static int nand_read_page_hwecc_oob_first(struct mtd_info *mtd, unsigned int max_bitflips = 0; /* Read the OOB area first */ - chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page); - chip->read_buf(mtd, chip->oob_poi, mtd->oobsize); - chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page); + if (mtd->writesize > 512) { + chip->cmdfunc(mtd, NAND_CMD_READ0, mtd->writesize, page); + chip->read_buf(mtd, chip->oob_poi, mtd->oobsize); + chip->cmdfunc(mtd, NAND_CMD_RNDOUT, 0, -1); + } else { + chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page); + chip->read_buf(mtd, chip->oob_poi, mtd->oobsize); + chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page); + } ret = mtd_ooblayout_get_eccbytes(mtd, ecc_code, chip->oob_poi, 0, chip->ecc.total); @@ -1902,8 +1908,10 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from, __func__, buf); read_retry: - if (nand_standard_page_accessors(&chip->ecc)) + if (nand_standard_page_accessors(&chip->ecc) && + chip->ecc.mode != NAND_ECC_HW_OOB_FIRST) { chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page); + } /* * Now read the page into the buffer. Absent an error,