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

Boris Brezillon
2017-06-08T10:04:43Z

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.