Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751558AbdFHGLN (ORCPT ); Thu, 8 Jun 2017 02:11:13 -0400 Received: from conssluserg-02.nifty.com ([210.131.2.81]:32226 "EHLO conssluserg-02.nifty.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750919AbdFHGLM (ORCPT ); Thu, 8 Jun 2017 02:11:12 -0400 DKIM-Filter: OpenDKIM Filter v2.10.3 conssluserg-02.nifty.com v586B4Lp014259 X-Nifty-SrcIP: [209.85.213.170] MIME-Version: 1.0 In-Reply-To: <20170607153318.7e341dff@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> From: Masahiro Yamada Date: Thu, 8 Jun 2017 15:11:03 +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: Cyrille Pitchen , Richard Weinberger , Marek Vasut , David Woodhouse , Chuanxiao Dong , Linux Kernel Mailing List , Dinh Nguyen , linux-mtd@lists.infradead.org, Masami Hiramatsu , Artem Bityutskiy , Jassi Brar , Brian Norris , Enrico Jorns 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-Length: 3025 Lines: 98 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.) As far as I tested this IP, INTR__PROGRAM_COMP is set only when data are written by PIO mode. I introduced PIO transfer in http://patchwork.ozlabs.org/patch/772398/ I used INTR__PROGRAM_COMP in denali_pio_write(). >> + 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/ I do not want to include unrelated change. -- Best Regards Masahiro Yamada