From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751591AbdFHKEW convert rfc822-to-8bit (ORCPT ); Thu, 8 Jun 2017 06:04:22 -0400 Received: from mail.free-electrons.com ([62.4.15.54]:49157 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751392AbdFHKEV (ORCPT ); Thu, 8 Jun 2017 06:04:21 -0400 Date: Thu, 8 Jun 2017 12:04:09 +0200 From: Boris Brezillon To: Masahiro Yamada 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 Subject: Re: [PATCH v5 07/23] mtd: nand: denali: do not propagate NAND_STATUS_FAIL to waitfunc() Message-ID: <20170608120409.0fb16b9d@bbrezillon> In-Reply-To: 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> X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 8 Jun 2017 18:43:47 +0900 Masahiro Yamada wrote: > 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. Actually, I agree with you: it's clearer to use the INTR__DMA_CMD_COMP / INTR__PROGRAM_COMP events here :P.