Re: [PATCH] mtd: nand: Wait for PAGEPROG to finish in drivers setting NAND_ECC_CUSTOM_PAGE_ACCESS

[Boris Brezillon <boris.brezillon@free-electrons.com>]

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