| Message ID | 633b8440-2961-e84a-edde-5a5afb9f0f52@sigmadesigns.com |
|---|---|
| State | Rejected |
| Delegated to: | Boris Brezillon |
| Headers | show |
On Wed, 17 May 2017 12:41:01 +0200 Marc Gonzalez <marc_gonzalez@sigmadesigns.com> wrote: > On 16/05/2017 18:27, Boris Brezillon wrote: > > > Drivers setting NAND_ECC_CUSTOM_PAGE_ACCESS are supposed to handle the > > full read/write page sequence, and waiting for a page to actually be > > programmed is part of this write-page sequence. > > This is also what is done in ->write_oob_xxx() hooks, so let's do that in > > ->write_page_xxx() as well to make it consistent. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > --- > > drivers/mtd/nand/atmel/nand-controller.c | 6 +++++- > > drivers/mtd/nand/nand_base.c | 10 ++++++---- > > drivers/mtd/nand/nand_micron.c | 10 ++++++++-- > > drivers/mtd/nand/tango_nand.c | 13 ++++++++++++- > > 4 files changed, 31 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > > index 8dafd2a54e11..08ff98c47e1f 100644 > > --- a/drivers/mtd/nand/nand_base.c > > +++ b/drivers/mtd/nand/nand_base.c > > @@ -2574,11 +2574,13 @@ static int nand_write_page(struct mtd_info *mtd, struct nand_chip *chip, > > if (status < 0) > > return status; > > > > - if (nand_standard_page_accessors(&chip->ecc)) > > + if (nand_standard_page_accessors(&chip->ecc)) { > > chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1); > > - status = chip->waitfunc(mtd, chip); > > - if (status & NAND_STATUS_FAIL) > > - return -EIO; > > + > > + status = chip->waitfunc(mtd, chip); > > + if (status & NAND_STATUS_FAIL) > > + return -EIO; > > + } > > AFAIU, the wait operation used to be unconditional; > but it is now skipped for CUSTOM accessors. OK. Yep. > > > > diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c > > index a2150b15d4c1..8498fa36e533 100644 > > --- a/drivers/mtd/nand/tango_nand.c > > +++ b/drivers/mtd/nand/tango_nand.c > > @@ -295,7 +295,7 @@ static int tango_write_page(struct mtd_info *mtd, struct nand_chip *chip, > > const u8 *buf, int oob_required, int page) > > { > > struct tango_nfc *nfc = to_tango_nfc(chip->controller); > > - int err, len = mtd->writesize; > > + int err, status, len = mtd->writesize; > > > > /* Calling tango_write_oob() would send PAGEPROG twice */ > > if (oob_required) > > @@ -306,6 +306,10 @@ static int tango_write_page(struct mtd_info *mtd, struct nand_chip *chip, > > if (err) > > return err; > > > > + status = chip->waitfunc(mtd, chip); > > + if (status & NAND_STATUS_FAIL) > > + return -EIO; > > + > > return 0; > > } > > When I introduced the custom_accessors flag, I missed the removal > of this wait operation. The tango NFC is supposed to take care of > everything, from start to finish. > > I applied the following patch: > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -2675,6 +2675,9 @@ static int nand_write_page(struct mtd_info *mtd, struct nand_chip *chip, > if (status < 0) > return status; > > + if (!nand_standard_page_accessors(&chip->ecc)) > + return 0; > + > > And ran mtd_speedtest. No measurable change. > mtd_stresstest did not detect any issues. Okay. I think I'll keep the patch as is to avoid introducing extra functional changes. You can then send a patch removing the extra chip->waitfunc() from tango_write_page() and explain why this is unneeded in your commit message. Would that work for you? > > > @@ -423,9 +427,16 @@ static int tango_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip, > > static int tango_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip, > > const u8 *buf, int oob_required, int page) > > { > > + int status; > > + > > chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0, page); > > raw_write(chip, buf, chip->oob_poi); > > chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1); > > + > > + status = chip->waitfunc(mtd, chip); > > + if (status & NAND_STATUS_FAIL) > > + return -EIO; > > + > > return 0; > > } > > This one would still be required, since we're going behind the NFC's back. > Should we test NAND_STATUS_FAIL in tango_write_oob() too? Yes, you should. > It might not matter, since you plan to change it to nand_prog_page_end_op() > ultimately. Well, I don't know when this will land, so we'd better fix it now and mark it for stable backport. Regards, Boris
On 17/05/2017 12:51, Boris Brezillon wrote: > On Wed, 17 May 2017 12:41:01 +0200 > Marc Gonzalez <marc_gonzalez@sigmadesigns.com> wrote: > >> On 16/05/2017 18:27, Boris Brezillon wrote: >> >>> Drivers setting NAND_ECC_CUSTOM_PAGE_ACCESS are supposed to handle the >>> full read/write page sequence, and waiting for a page to actually be >>> programmed is part of this write-page sequence. >>> This is also what is done in ->write_oob_xxx() hooks, so let's do that in >>> ->write_page_xxx() as well to make it consistent. >>> >>> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> >>> --- >>> drivers/mtd/nand/atmel/nand-controller.c | 6 +++++- >>> drivers/mtd/nand/nand_base.c | 10 ++++++---- >>> drivers/mtd/nand/nand_micron.c | 10 ++++++++-- >>> drivers/mtd/nand/tango_nand.c | 13 ++++++++++++- >>> 4 files changed, 31 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c >>> index 8dafd2a54e11..08ff98c47e1f 100644 >>> --- a/drivers/mtd/nand/nand_base.c >>> +++ b/drivers/mtd/nand/nand_base.c >>> @@ -2574,11 +2574,13 @@ static int nand_write_page(struct mtd_info *mtd, struct nand_chip *chip, >>> if (status < 0) >>> return status; >>> >>> - if (nand_standard_page_accessors(&chip->ecc)) >>> + if (nand_standard_page_accessors(&chip->ecc)) { >>> chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1); >>> - status = chip->waitfunc(mtd, chip); >>> - if (status & NAND_STATUS_FAIL) >>> - return -EIO; >>> + >>> + status = chip->waitfunc(mtd, chip); >>> + if (status & NAND_STATUS_FAIL) >>> + return -EIO; >>> + } >> >> AFAIU, the wait operation used to be unconditional; >> but it is now skipped for CUSTOM accessors. OK. > > Yep. > >> >> >>> diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c >>> index a2150b15d4c1..8498fa36e533 100644 >>> --- a/drivers/mtd/nand/tango_nand.c >>> +++ b/drivers/mtd/nand/tango_nand.c >>> @@ -295,7 +295,7 @@ static int tango_write_page(struct mtd_info *mtd, struct nand_chip *chip, >>> const u8 *buf, int oob_required, int page) >>> { >>> struct tango_nfc *nfc = to_tango_nfc(chip->controller); >>> - int err, len = mtd->writesize; >>> + int err, status, len = mtd->writesize; >>> >>> /* Calling tango_write_oob() would send PAGEPROG twice */ >>> if (oob_required) >>> @@ -306,6 +306,10 @@ static int tango_write_page(struct mtd_info *mtd, struct nand_chip *chip, >>> if (err) >>> return err; >>> >>> + status = chip->waitfunc(mtd, chip); >>> + if (status & NAND_STATUS_FAIL) >>> + return -EIO; >>> + >>> return 0; >>> } >> >> When I introduced the custom_accessors flag, I missed the removal >> of this wait operation. The tango NFC is supposed to take care of >> everything, from start to finish. >> >> I applied the following patch: >> >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c >> --- a/drivers/mtd/nand/nand_base.c >> +++ b/drivers/mtd/nand/nand_base.c >> @@ -2675,6 +2675,9 @@ static int nand_write_page(struct mtd_info *mtd, struct nand_chip *chip, >> if (status < 0) >> return status; >> >> + if (!nand_standard_page_accessors(&chip->ecc)) >> + return 0; >> + >> >> And ran mtd_speedtest. No measurable change. >> mtd_stresstest did not detect any issues. > > Okay. I think I'll keep the patch as is to avoid introducing extra > functional changes. > You can then send a patch removing the extra chip->waitfunc() from > tango_write_page() and explain why this is unneeded in your commit > message. Would that work for you? Could you ping me when the patch hits linux-next? Then I'll send a fixup. Regards.
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -2675,6 +2675,9 @@ static int nand_write_page(struct mtd_info *mtd, struct nand_chip *chip, if (status < 0) return status; + if (!nand_standard_page_accessors(&chip->ecc)) + return 0; + And ran mtd_speedtest. No measurable change. mtd_stresstest did not detect any issues.