Messages in this thread | | | Date | Thu, 8 Jun 2017 12:04:09 +0200 | From | Boris Brezillon <> | Subject | Re: [PATCH v5 07/23] mtd: nand: denali: do not propagate NAND_STATUS_FAIL to waitfunc() |
| |
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.
| |