Re: [PATCH v5 07/23] mtd: nand: denali: do not propagate NAND_STATUS_FAIL to waitfunc()

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

Le Thu, 8 Jun 2017 15:11:03 +0900,
Masahiro Yamada <yamada.masahiro@socionext.com> a écrit :

> Hi Boris,
> 
> 
> 2017-06-07 22:33 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > On Wed,  7 Jun 2017 20:52:16 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >  
> >> Currently, the error handling of denali_write_page(_raw) is a bit
> >> complicated.  If the program command fails, NAND_STATUS_FAIL is set
> >> to the driver internal denali->status, then read out later by
> >> denali_waitfunc().
> >>
> >> We can avoid it by exploiting the nand_write_page() implementation.
> >> If chip->ecc.write_page(_raw) returns negative code (i.e. -EIO), it
> >> errors out immediately.  This gives the same result as returning
> >> NAND_STATUS_FAIL from chip->waitfunc.  In either way, -EIO is
> >> returned to the upper MTD layer.  
> >
> > Actually, this is how it's supposed to work now (when they set
> > the NAND_ECC_CUSTOM_PAGE_ACCESS flag, drivers are expected to wait for
> > the program operation to finish and return -EIO if it failed), so you're
> > all good ;-).
> >  
> >>
> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> >> ---
> >>
> >> Changes in v5: None
> >> Changes in v4: None
> >> Changes in v3: None
> >> Changes in v2:
> >>   - Newly added
> >>
> >>  drivers/mtd/nand/denali.c | 12 ++++--------
> >>  drivers/mtd/nand/denali.h |  1 -
> >>  2 files changed, 4 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> >> index 1897fe238290..22acfc34b546 100644
> >> --- a/drivers/mtd/nand/denali.c
> >> +++ b/drivers/mtd/nand/denali.c
> >> @@ -1005,6 +1005,7 @@ static int write_page(struct mtd_info *mtd, struct nand_chip *chip,
> >>       size_t size = mtd->writesize + mtd->oobsize;
> >>       uint32_t irq_status;
> >>       uint32_t irq_mask = INTR__DMA_CMD_COMP | INTR__PROGRAM_FAIL;  
> >
> > As mentioned in my previous patch, I think you should wait for
> > INTR__PROGRAM_COMP | INTR__PROGRAM_FAIL here.  
> 
> No.
> It is intentional to use INTR__DMA_CMD_COMP
> instead of INTR__PROGRAM_COMP here.
> 
> 
> This is very strange of this IP,
> INTR__PROGRAM_COMP is never set when DMA mode is being used.
> (INTR__DMA_CMD_COMP is set instead.)

Indeed, this is really strange. Are you sure the page is actually
programmed when you receive the INTR__DMA_CMD_COMP interrupt?
Because INTR__DMA_CMD_COMP is likely to happen before the PAGEPROG
command has finished, which is not good (the core might start a new
operation while the NAND is still busy).

Anyway, if INTR__DMA_CMD_COMP is what should be set, it clearly
deserves a comment.

> 
> 
> As far as I tested this IP,
> INTR__PROGRAM_COMP is set only when data are written by PIO mode.

It doesn't make much sense (not saying you're wrong, just that the IP
is weird). PROG completed should be independent of the data transfer
step. Sure it happens after transferring data to the NAND, but then you
still have to execute the PAGEPROG command and wait until the NAND
becomes ready again. That's when I'd expect PROGRAM_COMP (or
PROGRAM_FAIL) to be triggered.

> 
> 
> I introduced PIO transfer in
> http://patchwork.ozlabs.org/patch/772398/
> 
> I used INTR__PROGRAM_COMP in denali_pio_write().
> 

Yep, I see that.

> 
> 
> >> +     int ret = 0;
> >>
> >>       denali->page = page;
> >>
> >> @@ -1038,13 +1039,13 @@ static int write_page(struct mtd_info *mtd, struct nand_chip *chip,
> >>       if (irq_status == 0) {
> >>               dev_err(denali->dev, "timeout on write_page (type = %d)\n",
> >>                       raw_xfer);
> >> -             denali->status = NAND_STATUS_FAIL;
> >> +             ret = -EIO;
> >>       }  
> >
> >         if (irq_status & INTR__PROGRAM_FAIL) {
> >                 dev_err(denali->dev, "page program failed (type = %d)\n",
> >                         raw_xfer);
> >                 ret = -EIO;
> >         }  
> 
> This will be fixed anyway by
> http://patchwork.ozlabs.org/patch/772414/

Note that PROG_FAILED is quite different from a timeout (usually
happens when a block becomes bad), so it probably deserve a specific
error message.

> 
> 
> I do not want to include unrelated change.
> 
> 

Okay.