| Message ID | 20240905055333.2363358-3-linchengming884@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | mtd: spi-nand: Add support for read retry | expand |
Hi Cheng Ming, linchengming884@gmail.com wrote on Thu, 5 Sep 2024 13:53:33 +0800: > From: Cheng Ming Lin <chengminglin@mxic.com.tw> > > When the host ECC fails to correct the data error of NAND device, > there's a special read for data recovery method which host setups > for the next read retry mode and may recover the lost data by host > ECC again. > > Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw> > --- > drivers/mtd/nand/spi/core.c | 33 +++++++++++++++++++++++++++++++-- > 1 file changed, 31 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c > index e0b6715e5dfe..2f21ea926132 100644 > --- a/drivers/mtd/nand/spi/core.c > +++ b/drivers/mtd/nand/spi/core.c > @@ -640,6 +640,7 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from, > struct nand_io_iter iter; > bool disable_ecc = false; > bool ecc_failed = false; > + u8 retry_mode = 0; > int ret = 0; > > if (ops->mode == MTD_OPS_RAW || !spinand->eccinfo.ooblayout) > @@ -657,20 +658,45 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from, This will no longer apply with continuous support added in. However please make this only in the non continuous path. I don't think it can fly in the other. > if (ret) > break; > > +read_retry: > ret = spinand_read_page(spinand, &iter.req); > if (ret < 0 && ret != -EBADMSG) > break; > > - if (ret == -EBADMSG) > + if (ret == -EBADMSG && spinand->info->fixups) { > + if (spinand->read_retries && ((retry_mode + 1) < spinand->read_retries)) { ++retry_mode? > + retry_mode++; So this can be dropped. > + ret = spinand->info->fixups->setup_read_retry(spinand, retry_mode); > + if (ret < 0) > + break; No, you need to set ecc_failed here. > + > + /* Reset ecc_stats; retry */ > + mtd->ecc_stats = old_stats; > + goto read_retry; > + } else { > + /* No more retry modes; real failure */ > + ecc_failed = true; > + } > + } else if (ret == -EBADMSG) { > ecc_failed = true; > - else > + } else { > max_bitflips = max_t(unsigned int, max_bitflips, ret); > + } > > ret = 0; > ops->retlen += iter.req.datalen; > ops->oobretlen += iter.req.ooblen; > + > + /* Reset to retry mode 0*/ > + if (retry_mode) { retry_mode = 0; > + ret = spinand->info->fixups->setup_read_retry(spinand, 0); retry_mode); > + if (ret < 0) > + break; this if clause is useless. > + retry_mode = 0; And then this can be dropped from here. > + } > } > > + Spurious line > if (ops->stats) { > ops->stats->uncorrectable_errors += > mtd->ecc_stats.failed - old_stats.failed; > @@ -1095,6 +1121,9 @@ int spinand_match_and_init(struct spinand_device *spinand, > spinand->flags = table[i].flags; > spinand->id.len = 1 + table[i].devid.len; > spinand->select_target = table[i].select_target; > + spinand->info = info; > + if (spinand->info->fixups && spinand->info->fixups->init_read_retry) > + spinand->read_retries = spinand->info->fixups->init_read_retry(spinand); Now I get you init. Ok, fine. > > op = spinand_select_op_variant(spinand, > info->op_variants.read_cache); Thanks, Miquèl
Hi Miquel, Miquel Raynal <miquel.raynal@bootlin.com> 於 2024年10月1日 週二 下午6:17寫道： > > Hi Cheng Ming, > > linchengming884@gmail.com wrote on Thu, 5 Sep 2024 13:53:33 +0800: > > > From: Cheng Ming Lin <chengminglin@mxic.com.tw> > > > > When the host ECC fails to correct the data error of NAND device, > > there's a special read for data recovery method which host setups > > for the next read retry mode and may recover the lost data by host > > ECC again. > > > > Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw> > > --- > > drivers/mtd/nand/spi/core.c | 33 +++++++++++++++++++++++++++++++-- > > 1 file changed, 31 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c > > index e0b6715e5dfe..2f21ea926132 100644 > > --- a/drivers/mtd/nand/spi/core.c > > +++ b/drivers/mtd/nand/spi/core.c > > @@ -640,6 +640,7 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from, > > struct nand_io_iter iter; > > bool disable_ecc = false; > > bool ecc_failed = false; > > + u8 retry_mode = 0; > > int ret = 0; > > > > if (ops->mode == MTD_OPS_RAW || !spinand->eccinfo.ooblayout) > > @@ -657,20 +658,45 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from, > > This will no longer apply with continuous support added in. However > please make this only in the non continuous path. I don't think it can > fly in the other. Thanks for your helpful suggestions. > > > if (ret) > > break; > > > > +read_retry: > > ret = spinand_read_page(spinand, &iter.req); > > if (ret < 0 && ret != -EBADMSG) > > break; > > > > - if (ret == -EBADMSG) > > + if (ret == -EBADMSG && spinand->info->fixups) { > > + if (spinand->read_retries && ((retry_mode + 1) < spinand->read_retries)) { > > ++retry_mode? > > + retry_mode++; > > So this can be dropped. > > > + ret = spinand->info->fixups->setup_read_retry(spinand, retry_mode); > > + if (ret < 0) > > + break; > > No, you need to set ecc_failed here. > > > + > > + /* Reset ecc_stats; retry */ > > + mtd->ecc_stats = old_stats; > > + goto read_retry; > > + } else { > > + /* No more retry modes; real failure */ > > + ecc_failed = true; > > + } > > + } else if (ret == -EBADMSG) { > > ecc_failed = true; > > - else > > + } else { > > max_bitflips = max_t(unsigned int, max_bitflips, ret); > > + } > > > > ret = 0; > > ops->retlen += iter.req.datalen; > > ops->oobretlen += iter.req.ooblen; > > + > > + /* Reset to retry mode 0*/ > > + if (retry_mode) { > > retry_mode = 0; > > > + ret = spinand->info->fixups->setup_read_retry(spinand, 0); > > retry_mode); > > > + if (ret < 0) > > + break; > > this if clause is useless. > > > + retry_mode = 0; > > And then this can be dropped from here. > > > + } > > } > > > > + > > Spurious line > > > if (ops->stats) { > > ops->stats->uncorrectable_errors += > > mtd->ecc_stats.failed - old_stats.failed; > > @@ -1095,6 +1121,9 @@ int spinand_match_and_init(struct spinand_device *spinand, > > spinand->flags = table[i].flags; > > spinand->id.len = 1 + table[i].devid.len; > > spinand->select_target = table[i].select_target; > > + spinand->info = info; > > + if (spinand->info->fixups && spinand->info->fixups->init_read_retry) > > + spinand->read_retries = spinand->info->fixups->init_read_retry(spinand); > > Now I get you init. Ok, fine. > > > > > op = spinand_select_op_variant(spinand, > > info->op_variants.read_cache); > Thank you for all your suggestions. I will modify the patch according to your advice. > > Thanks, > Miquèl Thanks, Cheng Ming Lin
diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c index e0b6715e5dfe..2f21ea926132 100644 --- a/drivers/mtd/nand/spi/core.c +++ b/drivers/mtd/nand/spi/core.c @@ -640,6 +640,7 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from, struct nand_io_iter iter; bool disable_ecc = false; bool ecc_failed = false; + u8 retry_mode = 0; int ret = 0; if (ops->mode == MTD_OPS_RAW || !spinand->eccinfo.ooblayout) @@ -657,20 +658,45 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from, if (ret) break; +read_retry: ret = spinand_read_page(spinand, &iter.req); if (ret < 0 && ret != -EBADMSG) break; - if (ret == -EBADMSG) + if (ret == -EBADMSG && spinand->info->fixups) { + if (spinand->read_retries && ((retry_mode + 1) < spinand->read_retries)) { + retry_mode++; + ret = spinand->info->fixups->setup_read_retry(spinand, retry_mode); + if (ret < 0) + break; + + /* Reset ecc_stats; retry */ + mtd->ecc_stats = old_stats; + goto read_retry; + } else { + /* No more retry modes; real failure */ + ecc_failed = true; + } + } else if (ret == -EBADMSG) { ecc_failed = true; - else + } else { max_bitflips = max_t(unsigned int, max_bitflips, ret); + } ret = 0; ops->retlen += iter.req.datalen; ops->oobretlen += iter.req.ooblen; + + /* Reset to retry mode 0*/ + if (retry_mode) { + ret = spinand->info->fixups->setup_read_retry(spinand, 0); + if (ret < 0) + break; + retry_mode = 0; + } } + if (ops->stats) { ops->stats->uncorrectable_errors += mtd->ecc_stats.failed - old_stats.failed; @@ -1095,6 +1121,9 @@ int spinand_match_and_init(struct spinand_device *spinand, spinand->flags = table[i].flags; spinand->id.len = 1 + table[i].devid.len; spinand->select_target = table[i].select_target; + spinand->info = info; + if (spinand->info->fixups && spinand->info->fixups->init_read_retry) + spinand->read_retries = spinand->info->fixups->init_read_retry(spinand); op = spinand_select_op_variant(spinand, info->op_variants.read_cache);