* Re: [RESEND PATCH v3 5/5] mtd: nand: remove custom 'erased check' implementation [not found] ` <1441296222-14989-6-git-send-email-boris.brezillon@free-electrons.com> @ 2015-09-21 23:28 ` Brian Norris 2015-09-22 8:08 ` Boris Brezillon 0 siblings, 1 reply; 2+ messages in thread From: Brian Norris @ 2015-09-21 23:28 UTC (permalink / raw) To: Boris Brezillon Cc: David Woodhouse, linux-mtd, Andrea Scian, Richard Weinberger, linux-mips, Ralf Baechle + others On Thu, Sep 03, 2015 at 06:03:42PM +0200, Boris Brezillon wrote: > Some drivers are manually checking for 'erased pages' while correcting > ECC bytes. > This logic is now done by the core infrastructure, and can thus be removed > from those drivers. > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > --- > drivers/mtd/nand/davinci_nand.c | 8 -------- > drivers/mtd/nand/diskonchip.c | 32 ++------------------------------ > drivers/mtd/nand/jz4740_nand.c | 18 ------------------ > 3 files changed, 2 insertions(+), 56 deletions(-) > > diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c > index 816ef53..0d6adbe 100644 > --- a/drivers/mtd/nand/davinci_nand.c > +++ b/drivers/mtd/nand/davinci_nand.c > @@ -316,14 +316,6 @@ static int nand_davinci_correct_4bit(struct mtd_info *mtd, > unsigned num_errors, corrected; > unsigned long timeo; > > - /* All bytes 0xff? It's an erased page; ignore its ECC. */ > - for (i = 0; i < 10; i++) { > - if (ecc_code[i] != 0xff) > - goto compare; > - } > - return 0; > - > -compare: > /* Unpack ten bytes into eight 10 bit values. We know we're > * little-endian, and use type punning for less shifting/masking. > */ > diff --git a/drivers/mtd/nand/diskonchip.c b/drivers/mtd/nand/diskonchip.c > index 7da266a..eb65769 100644 > --- a/drivers/mtd/nand/diskonchip.c > +++ b/drivers/mtd/nand/diskonchip.c > @@ -936,37 +936,9 @@ static int doc200x_correct_data(struct mtd_info *mtd, u_char *dat, > calc_ecc[i] = ReadDOC_(docptr, DoC_Mplus_ECCSyndrome0 + i); > else > calc_ecc[i] = ReadDOC_(docptr, DoC_ECCSyndrome0 + i); > - if (calc_ecc[i] != empty_read_syndrome[i]) > - emptymatch = 0; > } > - /* If emptymatch=1, the read syndrome is consistent with an > - all-0xff data and stored ecc block. Check the stored ecc. */ > - if (emptymatch) { > - for (i = 0; i < 6; i++) { > - if (read_ecc[i] == 0xff) > - continue; > - emptymatch = 0; > - break; > - } > - } > - /* If emptymatch still =1, check the data block. */ > - if (emptymatch) { > - /* Note: this somewhat expensive test should not be triggered > - often. It could be optimized away by examining the data in > - the readbuf routine, and remembering the result. */ > - for (i = 0; i < 512; i++) { > - if (dat[i] == 0xff) > - continue; > - emptymatch = 0; > - break; > - } > - } > - /* If emptymatch still =1, this is almost certainly a freshly- > - erased block, in which case the ECC will not come out right. > - We'll suppress the error and tell the caller everything's > - OK. Because it is. */ > - if (!emptymatch) > - ret = doc_ecc_decode(rs_decoder, dat, calc_ecc); > + > + ret = doc_ecc_decode(rs_decoder, dat, calc_ecc); > if (ret > 0) > printk(KERN_ERR "doc200x_correct_data corrected %d errors\n", ret); > } I see new warnings: drivers/mtd/nand/diskonchip.c: At top level: drivers/mtd/nand/diskonchip.c: In function ‘doc200x_correct_data’: drivers/mtd/nand/diskonchip.c:915:6: warning: unused variable ‘emptymatch’ [-Wunused-variable] int emptymatch = 1; ^ drivers/mtd/nand/diskonchip.c:79:15: warning: ‘empty_read_syndrome’ defined but not used [-Wunused-variable] static u_char empty_read_syndrome[6] = { 0x26, 0xff, 0x6d, 0x47, 0x73, 0x7a }; ^ I'd also like test results for these drivers before doing this. And I'm not sure how to find good testers for these drivers, even it users still exist. > diff --git a/drivers/mtd/nand/jz4740_nand.c b/drivers/mtd/nand/jz4740_nand.c > index ba44af3..4d73276 100644 > --- a/drivers/mtd/nand/jz4740_nand.c > +++ b/drivers/mtd/nand/jz4740_nand.c > @@ -224,24 +224,6 @@ static int jz_nand_correct_ecc_rs(struct mtd_info *mtd, uint8_t *dat, > uint32_t t; > unsigned int timeout = 1000; > > - t = read_ecc[0]; > - > - if (t == 0xff) { > - for (i = 1; i < 9; ++i) > - t &= read_ecc[i]; > - > - t &= dat[0]; > - t &= dat[nand->chip.ecc.size / 2]; > - t &= dat[nand->chip.ecc.size - 1]; > - > - if (t == 0xff) { > - for (i = 1; i < nand->chip.ecc.size - 1; ++i) > - t &= dat[i]; > - if (t == 0xff) > - return 0; > - } > - } > - > for (i = 0; i < 9; ++i) > writeb(read_ecc[i], nand->base + JZ_REG_NAND_PAR0 + i); > > -- > 1.9.1 > ^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [RESEND PATCH v3 5/5] mtd: nand: remove custom 'erased check' implementation 2015-09-21 23:28 ` [RESEND PATCH v3 5/5] mtd: nand: remove custom 'erased check' implementation Brian Norris @ 2015-09-22 8:08 ` Boris Brezillon 0 siblings, 0 replies; 2+ messages in thread From: Boris Brezillon @ 2015-09-22 8:08 UTC (permalink / raw) To: Brian Norris Cc: David Woodhouse, linux-mtd, Andrea Scian, Richard Weinberger, linux-mips, Ralf Baechle Hi Brian, On Mon, 21 Sep 2015 16:28:32 -0700 Brian Norris <computersforpeace@gmail.com> wrote: > + others > > On Thu, Sep 03, 2015 at 06:03:42PM +0200, Boris Brezillon wrote: > > Some drivers are manually checking for 'erased pages' while correcting > > ECC bytes. > > This logic is now done by the core infrastructure, and can thus be removed > > from those drivers. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > --- > > drivers/mtd/nand/davinci_nand.c | 8 -------- > > drivers/mtd/nand/diskonchip.c | 32 ++------------------------------ > > drivers/mtd/nand/jz4740_nand.c | 18 ------------------ > > 3 files changed, 2 insertions(+), 56 deletions(-) > > > > diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c > > index 816ef53..0d6adbe 100644 > > --- a/drivers/mtd/nand/davinci_nand.c > > +++ b/drivers/mtd/nand/davinci_nand.c > > @@ -316,14 +316,6 @@ static int nand_davinci_correct_4bit(struct mtd_info *mtd, > > unsigned num_errors, corrected; > > unsigned long timeo; > > > > - /* All bytes 0xff? It's an erased page; ignore its ECC. */ > > - for (i = 0; i < 10; i++) { > > - if (ecc_code[i] != 0xff) > > - goto compare; > > - } > > - return 0; > > - > > -compare: > > /* Unpack ten bytes into eight 10 bit values. We know we're > > * little-endian, and use type punning for less shifting/masking. > > */ > > diff --git a/drivers/mtd/nand/diskonchip.c b/drivers/mtd/nand/diskonchip.c > > index 7da266a..eb65769 100644 > > --- a/drivers/mtd/nand/diskonchip.c > > +++ b/drivers/mtd/nand/diskonchip.c > > @@ -936,37 +936,9 @@ static int doc200x_correct_data(struct mtd_info *mtd, u_char *dat, > > calc_ecc[i] = ReadDOC_(docptr, DoC_Mplus_ECCSyndrome0 + i); > > else > > calc_ecc[i] = ReadDOC_(docptr, DoC_ECCSyndrome0 + i); > > - if (calc_ecc[i] != empty_read_syndrome[i]) > > - emptymatch = 0; > > } > > - /* If emptymatch=1, the read syndrome is consistent with an > > - all-0xff data and stored ecc block. Check the stored ecc. */ > > - if (emptymatch) { > > - for (i = 0; i < 6; i++) { > > - if (read_ecc[i] == 0xff) > > - continue; > > - emptymatch = 0; > > - break; > > - } > > - } > > - /* If emptymatch still =1, check the data block. */ > > - if (emptymatch) { > > - /* Note: this somewhat expensive test should not be triggered > > - often. It could be optimized away by examining the data in > > - the readbuf routine, and remembering the result. */ > > - for (i = 0; i < 512; i++) { > > - if (dat[i] == 0xff) > > - continue; > > - emptymatch = 0; > > - break; > > - } > > - } > > - /* If emptymatch still =1, this is almost certainly a freshly- > > - erased block, in which case the ECC will not come out right. > > - We'll suppress the error and tell the caller everything's > > - OK. Because it is. */ > > - if (!emptymatch) > > - ret = doc_ecc_decode(rs_decoder, dat, calc_ecc); > > + > > + ret = doc_ecc_decode(rs_decoder, dat, calc_ecc); > > if (ret > 0) > > printk(KERN_ERR "doc200x_correct_data corrected %d errors\n", ret); > > } > > I see new warnings: > > drivers/mtd/nand/diskonchip.c: At top level: > drivers/mtd/nand/diskonchip.c: In function ‘doc200x_correct_data’: > drivers/mtd/nand/diskonchip.c:915:6: warning: unused variable ‘emptymatch’ [-Wunused-variable] > int emptymatch = 1; > ^ > drivers/mtd/nand/diskonchip.c:79:15: warning: ‘empty_read_syndrome’ defined but not used [-Wunused-variable] > static u_char empty_read_syndrome[6] = { 0x26, 0xff, 0x6d, 0x47, 0x73, 0x7a }; > ^ Oops, I'll fix those warnings. > > I'd also like test results for these drivers before doing this. And I'm > not sure how to find good testers for these drivers, even it users still > exist. Yep, the goal of this patch was to let people owning boards embedding those chips test the changes. If we don't get anyone to test it, maybe we should leave those drivers unchanged at the expense of doing twice the test in case of bitflips in erased pages or uncorrectable bitflips (which is really unlikely). Best Regards, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2015-09-22 8:08 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1441296222-14989-1-git-send-email-boris.brezillon@free-electrons.com>
[not found] ` <1441296222-14989-6-git-send-email-boris.brezillon@free-electrons.com>
2015-09-21 23:28 ` [RESEND PATCH v3 5/5] mtd: nand: remove custom 'erased check' implementation Brian Norris
2015-09-22 8:08 ` Boris Brezillon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox