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

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

On Thu, 8 Jun 2017 18:43:47 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> Hi Boris,
> 
> 
> 2017-06-08 16:05 GMT+09:00 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?  
> 
> Yes.
> After my test, I concluded INTR__DMA_CMD_COMP is asserted
> when page program is completed.
> 
> 
> 
> Rationale:
> 
> Denali User's Guide describes the IRQ bits as follows:
> 
> 
> Bit 2 (dma_cmd_comp)    A data DMA command has completed on this bank
>   ...
> Bit 7 (program_comp)    Device finished the last issued program command
>   ...
> Bit 12 (INT_act)        R/B pin of device transitioned from low to high
>   ...
> Bit 15 (page_xfer_inc)  For every page of data transfer to or from the device,
>                         this bit will be set.
> 
> 
> 
> In my test,  ->write_page() hook triggers IRQ bits as follows:
> 
>  - Write access with DMA
>       bit 15 is asserted first,
>       then some timer later  bit 12 and bit 2 are asserted at the same time
> 
>  - Write access with PIO
>       bit 15 is asserted first,
>       then some time later   bit 12 and bit 7 are asserted at the same time
> 
> 
> 
> NAND devices toggle R/B# pin when page program is completed.
> So, bit 2 (dma_cmd_comp) means the completion of page program.
> 
> 
> I assume your next question here.
> "So, why don't you wait for INTR__INT_ACT
> instead of INTR__DMA_CMD_COMP / INTR__PROGRAM_COMP?
> It should work regardless of transfer mode."
> This has a point.
> We can always check R/B# transition for read, write, erase, or whatever.
> This is just a matter of taste, but I am just keeping code that uses
> dedicated IRQ bits for each mode.

Actually, I agree with you: it's clearer to use the INTR__DMA_CMD_COMP /
INTR__PROGRAM_COMP events here :P.