Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754703AbdGNRbu convert rfc822-to-8bit (ORCPT ); Fri, 14 Jul 2017 13:31:50 -0400 Received: from mail.free-electrons.com ([62.4.15.54]:33199 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753844AbdGNRbs (ORCPT ); Fri, 14 Jul 2017 13:31:48 -0400 Date: Fri, 14 Jul 2017 19:31:43 +0200 From: Boris Brezillon To: Han Xu Cc: Miquel Raynal , "richard@nod.at" , "linux-mtd@lists.infradead.org" , "dwmw2@infradead.org" , "computersforpeace@gmail.com" , "marek.vasut@gmail.com" , "cyrille.pitchen@wedev4u.fr" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH] mtd: gpmi-nand: do not fail setting ONFI timing mode if available Message-ID: <20170714193143.034c4c24@bbrezillon> In-Reply-To: <178298b1-b482-e636-2daa-1a6595b8c139@nxp.com> References: <20170713192030.22177-1-miquel.raynal@free-electrons.com> <20170713221520.268bc554@bbrezillon> <178298b1-b482-e636-2daa-1a6595b8c139@nxp.com> X-Mailer: Claws Mail 3.14.1 (GTK+ 2.24.31; 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 Content-Length: 3342 Lines: 84 Hi Han, Le Fri, 14 Jul 2017 14:53:39 +0000, Han Xu a écrit : > On 07/13/2017 03:15 PM, Boris Brezillon wrote: > > Hi Miquel, > > > > Le Thu, 13 Jul 2017 21:20:30 +0200, > > Miquel Raynal a écrit : > > > >> GPMI NFC driver fails to apply timing mode if the ->onfi_get_features() > >> does not return the mode that was previously applied. > >> > >> We can assume that a nand chip supports a timing as long as it is > >> read from the ONFI parameter page. Reading back a different mode than > >> the one previously applied does not mean the mode is unsupported but > >> that the nand chip does not implement the ONFI feature because it > >> probably does not need to. > >> > >> The output of ->onfi_get_feature() is irrelevant so delete it. > > Having the NAND part that is not supporting the get/set(timing_mode) > > feature explicitly mentioned in the commit message would help reviewers > > understand why this patch is needed. > > > > Also mention that, even though the SET/GET_FEATURES(timing_mode) is > > marked as required in the ONFI spec, this Macronix chip does not > > support it which could be considered as a bug. > > > > Regards, > > > > Boris > > Yes, this is a Macronix chip bug and I have reproduced on my side, > ignoring the GET_FEATURE checking is a workaround and the chip will > still works in EDO mode 5, but I don't accept to remove the reasonable > checking code for a chip bug. I understand why you're reluctant to remove this check just to make one particular chip work correctly, but, on the other hand, if we were only supporting non-broken NAND chip in mainline, plenty of boards wouldn't be supported. Flash vendors tend to take liberties with standards, that's a fact, and once the chip is out there's nothing we can do about it, except add a workaround to support it. So let's try to find a solution that makes everyone happy: now that we have nand_manufacturer_ops, we can easily let manufacturer code flag specific chip features as broken and let the core or drivers test for it before using the feature. This way, the gpmi-nand driver could check this flag before trying to call ->onfi_set/get_features(TIMING). Would that work for you? BTW, that'd be great to have this driver converted to the ->setup_data_interface() approach at some point. Regards, Boris > > >> Signed-off-by: Miquel Raynal > >> --- > >> drivers/mtd/nand/gpmi-nand/gpmi-lib.c | 7 ------- > >> 1 file changed, 7 deletions(-) > >> > >> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c > >> index 141bd70a49c2..4d137145439c 100644 > >> --- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c > >> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c > >> @@ -939,13 +939,6 @@ static int enable_edo_mode(struct gpmi_nand_data *this, int mode) > >> if (ret) > >> goto err_out; > >> > >> - /* [2] send GET FEATURE command to double-check the timing mode */ > >> - memset(feature, 0, ONFI_SUBFEATURE_PARAM_LEN); > >> - ret = nand->onfi_get_features(mtd, nand, > >> - ONFI_FEATURE_ADDR_TIMING_MODE, feature); > >> - if (ret || feature[0] != mode) > >> - goto err_out; > >> - > >> nand->select_chip(mtd, -1); > >> > >> /* [3] set the main IO clock, 100MHz for mode 5, 80MHz for mode 4. */