Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751205AbdGOKwH convert rfc822-to-8bit (ORCPT ); Sat, 15 Jul 2017 06:52:07 -0400 Received: from mail.free-electrons.com ([62.4.15.54]:44437 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751161AbdGOKwF (ORCPT ); Sat, 15 Jul 2017 06:52:05 -0400 Date: Sat, 15 Jul 2017 12:52:00 +0200 From: Miquel RAYNAL To: Boris Brezillon Cc: Han Xu , "richard@nod.at" , "cyrille.pitchen@wedev4u.fr" , "linux-kernel@vger.kernel.org" , "marek.vasut@gmail.com" , "linux-mtd@lists.infradead.org" , "computersforpeace@gmail.com" , "dwmw2@infradead.org" Subject: Re: [PATCH] mtd: gpmi-nand: do not fail setting ONFI timing mode if available Message-ID: <20170715125200.0b74d494@xps13> In-Reply-To: <20170714193143.034c4c24@bbrezillon> References: <20170713192030.22177-1-miquel.raynal@free-electrons.com> <20170713221520.268bc554@bbrezillon> <178298b1-b482-e636-2daa-1a6595b8c139@nxp.com> <20170714193143.034c4c24@bbrezillon> Organization: Free Electrons 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: 4341 Lines: 122 Hi Han and Boris, On Fri, 14 Jul 2017 19:31:43 +0200 Boris Brezillon wrote: > 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. I do agree with both of you. If sent this patch without asking myself more questions because: not checking if the timings have been properly set by a call to ->onfi_get_features() is what the nand core does. http://elixir.free-electrons.com/linux/v4.12/source/drivers/mtd/nand/nand_base.c#L1110 Of course it would be better to use the ->setup_data_interface() but this is much bigger effort. Regards, Miquèl > > 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. */ > > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ -- Miquel Raynal, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com