* [PATCH] mtd: gpmi-nand: do not fail setting ONFI timing mode if available @ 2017-07-13 19:20 Miquel Raynal 2017-07-13 20:15 ` Boris Brezillon 0 siblings, 1 reply; 6+ messages in thread From: Miquel Raynal @ 2017-07-13 19:20 UTC (permalink / raw) To: han.xu, boris.brezillon, richard, linux-mtd Cc: dwmw2, computersforpeace, marek.vasut, cyrille.pitchen, linux-kernel, Miquel Raynal 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. Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com> --- 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. */ -- 2.11.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: gpmi-nand: do not fail setting ONFI timing mode if available 2017-07-13 19:20 [PATCH] mtd: gpmi-nand: do not fail setting ONFI timing mode if available Miquel Raynal @ 2017-07-13 20:15 ` Boris Brezillon 2017-07-14 14:53 ` Han Xu 0 siblings, 1 reply; 6+ messages in thread From: Boris Brezillon @ 2017-07-13 20:15 UTC (permalink / raw) To: Miquel Raynal Cc: han.xu, richard, linux-mtd, dwmw2, computersforpeace, marek.vasut, cyrille.pitchen, linux-kernel Hi Miquel, Le Thu, 13 Jul 2017 21:20:30 +0200, Miquel Raynal <miquel.raynal@free-electrons.com> 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 > > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com> > --- > 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. */ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: gpmi-nand: do not fail setting ONFI timing mode if available 2017-07-13 20:15 ` Boris Brezillon @ 2017-07-14 14:53 ` Han Xu 2017-07-14 17:31 ` Boris Brezillon 0 siblings, 1 reply; 6+ messages in thread From: Han Xu @ 2017-07-14 14:53 UTC (permalink / raw) To: Boris Brezillon, Miquel Raynal Cc: 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 On 07/13/2017 03:15 PM, Boris Brezillon wrote: > Hi Miquel, > > Le Thu, 13 Jul 2017 21:20:30 +0200, > Miquel Raynal <miquel.raynal@free-electrons.com> 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. >> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com> >> --- >> 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. */ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: gpmi-nand: do not fail setting ONFI timing mode if available 2017-07-14 14:53 ` Han Xu @ 2017-07-14 17:31 ` Boris Brezillon 2017-07-15 10:52 ` Miquel RAYNAL 0 siblings, 1 reply; 6+ messages in thread From: Boris Brezillon @ 2017-07-14 17:31 UTC (permalink / raw) 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 Hi Han, Le Fri, 14 Jul 2017 14:53:39 +0000, Han Xu <han.xu@nxp.com> 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 <miquel.raynal@free-electrons.com> 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 <miquel.raynal@free-electrons.com> > >> --- > >> 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. */ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: gpmi-nand: do not fail setting ONFI timing mode if available 2017-07-14 17:31 ` Boris Brezillon @ 2017-07-15 10:52 ` Miquel RAYNAL 2017-07-15 12:47 ` Boris Brezillon 0 siblings, 1 reply; 6+ messages in thread From: Miquel RAYNAL @ 2017-07-15 10:52 UTC (permalink / raw) 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 Hi Han and Boris, On Fri, 14 Jul 2017 19:31:43 +0200 Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > Hi Han, > > Le Fri, 14 Jul 2017 14:53:39 +0000, > Han Xu <han.xu@nxp.com> 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 <miquel.raynal@free-electrons.com> 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 <miquel.raynal@free-electrons.com> > > >> --- > > >> 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mtd: gpmi-nand: do not fail setting ONFI timing mode if available 2017-07-15 10:52 ` Miquel RAYNAL @ 2017-07-15 12:47 ` Boris Brezillon 0 siblings, 0 replies; 6+ messages in thread From: Boris Brezillon @ 2017-07-15 12:47 UTC (permalink / raw) To: Miquel RAYNAL 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 Le Sat, 15 Jul 2017 12:52:00 +0200, Miquel RAYNAL <miquel.raynal@free-electrons.com> a écrit : > Hi Han and Boris, > > On Fri, 14 Jul 2017 19:31:43 +0200 > Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > > > Hi Han, > > > > Le Fri, 14 Jul 2017 14:53:39 +0000, > > Han Xu <han.xu@nxp.com> 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 <miquel.raynal@free-electrons.com> 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 Probably something we should fix in nand_setup_data_interface(). Checking if a parameter has been properly set by reading it back sounds like a good practice. Note that, based on the tests Sascha and I did back when he implemented the ->setup_data_interface() infrastructure, I doubt setting timing mode on an SDR NAND has any effect. This is the very reason I initially suggested you to drop the extra check in the GPMI driver: if the NAND properly implements SET/GET_FEATURES(timing), then SET_FEATURES(timing, X) should work just fine, and if it does not but still advertise that it support modes 0 to X, that means SET_FEATURES(timing, X) is useless and we shouldn't care if GET_FEATURES(timing) returns a wrong value. > > Of course it would be better to use the ->setup_data_interface() > but this is much bigger effort. Yes, I was doing this suggestion to know if Han (or someone else) had planned to support this feature. And yes, the initial effort is not comparable to the 7-lines patch you submitted, but having NAND timings selection logic in a single place is easier to maintain. Note that I'm not requiring this rework, just gently suggesting to think about it ;-). ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-07-15 12:47 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-07-13 19:20 [PATCH] mtd: gpmi-nand: do not fail setting ONFI timing mode if available Miquel Raynal 2017-07-13 20:15 ` Boris Brezillon 2017-07-14 14:53 ` Han Xu 2017-07-14 17:31 ` Boris Brezillon 2017-07-15 10:52 ` Miquel RAYNAL 2017-07-15 12:47 ` Boris Brezillon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox