From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 23C6BC04AAF for ; Tue, 21 May 2019 08:47:25 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id E84B9217D4 for ; Tue, 21 May 2019 08:47:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727019AbfEUIrY convert rfc822-to-8bit (ORCPT ); Tue, 21 May 2019 04:47:24 -0400 Received: from relay9-d.mail.gandi.net ([217.70.183.199]:37605 "EHLO relay9-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726448AbfEUIrX (ORCPT ); Tue, 21 May 2019 04:47:23 -0400 X-Originating-IP: 90.88.22.185 Received: from xps13 (aaubervilliers-681-1-80-185.w90-88.abo.wanadoo.fr [90.88.22.185]) (Authenticated sender: miquel.raynal@bootlin.com) by relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 9DB0EFF816; Tue, 21 May 2019 08:47:14 +0000 (UTC) Date: Tue, 21 May 2019 10:47:13 +0200 From: Miquel Raynal To: masonccyang@mxic.com.tw Cc: bbrezillon@kernel.org, computersforpeace@gmail.com, dwmw2@infradead.org, frieder.schrempf@kontron.de, juliensu@mxic.com.tw, linux-kernel@vger.kernel.org, linux-mtd@lists.infradead.org, marek.vasut@gmail.com, richard@nod.at, vigneshr@ti.com, zhengxunli@mxic.com.tw Subject: Re: [PATCH v2] mtd: rawnand: Add Macronix NAND read retry support Message-ID: <20190521104713.4b3a7769@xps13> In-Reply-To: References: <1558076001-29579-1-git-send-email-masonccyang@mxic.com.tw> <20190520143438.46248bfc@xps13> Organization: Bootlin X-Mailer: Claws Mail 3.17.1 (GTK+ 2.24.32; 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 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi masonccyang@mxic.com.tw, masonccyang@mxic.com.tw wrote on Tue, 21 May 2019 10:42:06 +0800: > Hi Miquel, > > > > Add support for Macronix NAND read retry. > > > > > > Macronix NANDs support specific read operation for data recovery, > > > which can be enabled/disabled with a SET/GET_FEATURE. > > > Driver checks byte 167 of Vendor Blocks in ONFI parameter page table > > > to see if this high-reliability function is supported. > > > > > > Signed-off-by: Mason Yang > > > --- > > > drivers/mtd/nand/raw/nand_macronix.c | 57 > ++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 57 insertions(+) > > > > > > diff --git a/drivers/mtd/nand/raw/nand_macronix.c > b/drivers/mtd/nand/raw/ > > nand_macronix.c > > > index e287e71..1a4dc92 100644 > > > --- a/drivers/mtd/nand/raw/nand_macronix.c > > > +++ b/drivers/mtd/nand/raw/nand_macronix.c > > > @@ -17,6 +17,62 @@ > > > > > > #include "internals.h" > > > > > > +#define MACRONIX_READ_RETRY_BIT BIT(0) > > > +#define MACRONIX_READ_RETRY_MODE 6 > > > > Can you name this define MACRONIX_NUM_READ_RETRY_MODES? > > okay, will fix. > > > > > > + > > > +struct nand_onfi_vendor_macronix { > > > + u8 reserved[1]; > > > > Do you need this "[1]" ? > > okay, just u8 reserved; > > > > > > + u8 reliability_func; > > > +} __packed; > > > + > > > +/* > > > + * Macronix NANDs support using SET/GET_FEATURES to enter/exit read > retry mode > > > + */ > > > +static int macronix_nand_setup_read_retry(struct nand_chip *chip, int > mode) > > > +{ > > > + u8 feature[ONFI_SUBFEATURE_PARAM_LEN]; > > > + int ret, feature_addr = ONFI_FEATURE_ADDR_READ_RETRY; > > > + > > > + if (chip->parameters.supports_set_get_features && > > > + test_bit(feature_addr, chip->parameters.set_feature_list) && > > > + test_bit(feature_addr, chip->parameters.get_feature_list)) { > > > + feature[0] = mode; > > > + ret = nand_set_features(chip, feature_addr, feature); > > > + if (ret) > > > + pr_err("Failed to set read retry moded:%d\n", mode); > > > > Do you have to call nand_get_features() on error? > > okay > > > > > > + > > > + ret = nand_get_features(chip, feature_addr, feature); > > > + if (ret || feature[0] != mode) > > > + pr_err("Failed to verify read retry moded:%d(%d)\n", > > > + mode, feature[0]); > > > > if ret == 0 but feature[0] != mode, shouldn't you return an error? > > okay, will fix. > > > > > > + } > > > + > > > + return ret; > > > > This will produce a Warning at compile time (ret may be used > > uninitialized). Have you tested it? > > Tool chain I used is "gcc-arm-linux-gnueabi" and no Warning at compile > time. What's the output of: gcc-arm-linux-gnueabi -v ? > > Patch it to: > -----------------------------------------------------------------------------> > static int macronix_nand_setup_read_retry(struct nand_chip *chip, int > mode) > { > u8 feature[ONFI_SUBFEATURE_PARAM_LEN]; > int ret, feature_addr = ONFI_FEATURE_ADDR_READ_RETRY; > > if (chip->parameters.supports_set_get_features && > test_bit(feature_addr, chip->parameters.set_feature_list) && > test_bit(feature_addr, chip->parameters.get_feature_list)) { > > feature[0] = mode; > ret = nand_set_features(chip, feature_addr, feature); ^ extra space, please be careful with the typos, and run checkpatch.pl --strict before sending patches. > if (ret) { > pr_err("Failed to set read retry moded:%d\n", > mode); > goto err_out; > } > > ret = nand_get_features(chip, feature_addr, feature); > if (ret) { > pr_err("Failed to get read retry moded:%d\n", > mode); > goto err_out; > } else if (feature[0] != mode) { > pr_err("Failed to verify read retry > moded:%d(%d)\n", > mode, feature[0]); > return -EIO; That's not what I meant. You can keep the former condition but if !ret then ret = -EIO for instance. > } > } > > err_out: > return ret; Again, do not jump to a single return call, directly do the return from the point where you want to quit the function. The problem should be that ret may be used uninitialized, the compiler should tell you that. Thanks, Miquèl