Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751469AbdFHJnx (ORCPT ); Thu, 8 Jun 2017 05:43:53 -0400 Received: from conssluserg-02.nifty.com ([210.131.2.81]:51281 "EHLO conssluserg-02.nifty.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750788AbdFHJnw (ORCPT ); Thu, 8 Jun 2017 05:43:52 -0400 DKIM-Filter: OpenDKIM Filter v2.10.3 conssluserg-02.nifty.com v589hnRB029188 X-Nifty-SrcIP: [209.85.213.176] MIME-Version: 1.0 In-Reply-To: <20170608090503.11969545@bbrezillon> References: <1496836352-8016-1-git-send-email-yamada.masahiro@socionext.com> <1496836352-8016-8-git-send-email-yamada.masahiro@socionext.com> <20170607153318.7e341dff@bbrezillon> <20170608090503.11969545@bbrezillon> From: Masahiro Yamada Date: Thu, 8 Jun 2017 18:43:47 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v5 07/23] mtd: nand: denali: do not propagate NAND_STATUS_FAIL to waitfunc() To: Boris Brezillon Cc: Dinh Nguyen , Enrico Jorns , Richard Weinberger , Cyrille Pitchen , Artem Bityutskiy , Linux Kernel Mailing List , Marek Vasut , linux-mtd@lists.infradead.org, Masami Hiramatsu , Chuanxiao Dong , Jassi Brar , Brian Norris , David Woodhouse Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id v589hv6n010644 Content-Length: 6223 Lines: 203 Hi Boris, 2017-06-08 16:05 GMT+09:00 Boris Brezillon : > Le Thu, 8 Jun 2017 15:11:03 +0900, > Masahiro Yamada a écrit : > >> Hi Boris, >> >> >> 2017-06-07 22:33 GMT+09:00 Boris Brezillon : >> > On Wed, 7 Jun 2017 20:52:16 +0900 >> > Masahiro Yamada 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 >> >> --- >> >> >> >> 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. > 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). As explained above, INTR__PAGE_XFER_INC happens before the PAGEPROG. Then, INTR__DMA_CMD_COMP happens when the PAGEPROG has finished. > Anyway, if INTR__DMA_CMD_COMP is what should be set, it clearly > deserves a comment. Will add 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. You can do like that (execute 0x10 command separately) by using the raw command mode. (MODE_11) When using high level interface of this IP, the controller will take care of 0x80 command, address cycle, data cycle, then 0x10 command. Anyway, we agree this IP is strange. >> >> >> 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. > OK. Will consider it. -- Best Regards Masahiro Yamada