* [PATCH] mtd: rawnand: call onfi_fill_data_interface() once again after nand_detect @ 2019-02-07 9:57 Masahiro Yamada 2019-02-07 10:16 ` Miquel Raynal 0 siblings, 1 reply; 7+ messages in thread From: Masahiro Yamada @ 2019-02-07 9:57 UTC (permalink / raw) To: linux-mtd, Boris Brezillon, Miquel Raynal Cc: Marek Vasut, Richard Weinberger, Boris Brezillon, linux-kernel, Masahiro Yamada, Brian Norris, David Woodhouse nand_scan_ident() calls onfi_fill_data_interface() at its entry to set up the initial timing parameters. The timing parameters are needed not only for ->setup_data_interface(), but also for giving the correct delay to NAND_OP_WAIT_RDY, for example. If the driver sets the NAND_KEEP_TIMINGS flag, or does not support ->setup_data_interface() hook, those parameters will never updated. Before nand_detect(), we never know whether the chip is ONFi or not. So, onfi_fill_data_interface() has to assume the worst case, i.e. non-ONFi. After nand_detect(), if the chip turns out to be ONFi-compliant, we can optimize tPROG_max, tBERS_max, etc. Call onfi_fill_data_interface() once again. Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- drivers/mtd/nand/raw/nand_base.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c index 9b3d7ff..35e543c 100644 --- a/drivers/mtd/nand/raw/nand_base.c +++ b/drivers/mtd/nand/raw/nand_base.c @@ -5040,6 +5040,9 @@ static int nand_scan_ident(struct nand_chip *chip, unsigned int maxchips, nand_deselect_target(chip); + /* If the chip turns out ONFi, we can optimize timing parameters. */ + onfi_fill_data_interface(chip, NAND_SDR_IFACE, 0); + /* Check for a chip array */ for (i = 1; i < maxchips; i++) { u8 id[2]; -- 2.7.4 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] mtd: rawnand: call onfi_fill_data_interface() once again after nand_detect 2019-02-07 9:57 [PATCH] mtd: rawnand: call onfi_fill_data_interface() once again after nand_detect Masahiro Yamada @ 2019-02-07 10:16 ` Miquel Raynal 2019-02-07 10:46 ` Masahiro Yamada 0 siblings, 1 reply; 7+ messages in thread From: Miquel Raynal @ 2019-02-07 10:16 UTC (permalink / raw) To: Masahiro Yamada Cc: Marek Vasut, Richard Weinberger, Boris Brezillon, linux-kernel, Boris Brezillon, linux-mtd, Brian Norris, David Woodhouse Hi Masahiro, Masahiro Yamada <yamada.masahiro@socionext.com> wrote on Thu, 7 Feb 2019 18:57:56 +0900: > nand_scan_ident() calls onfi_fill_data_interface() at its entry > to set up the initial timing parameters. > > The timing parameters are needed not only for ->setup_data_interface(), > but also for giving the correct delay to NAND_OP_WAIT_RDY, for example. > > If the driver sets the NAND_KEEP_TIMINGS flag, or does not support > ->setup_data_interface() hook, those parameters will never updated. ^ be > > Before nand_detect(), we never know whether the chip is ONFi or not. > So, onfi_fill_data_interface() has to assume the worst case, i.e. > non-ONFi. s/ONFi/ONFI/? > > After nand_detect(), if the chip turns out to be ONFi-compliant, > we can optimize tPROG_max, tBERS_max, etc. > > Call onfi_fill_data_interface() once again. Sorry but I don't get why this is needed as there is the same call at the top of this function. Can you be more specific on where/when the missing call produces a failure? > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > --- > > drivers/mtd/nand/raw/nand_base.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c > index 9b3d7ff..35e543c 100644 > --- a/drivers/mtd/nand/raw/nand_base.c > +++ b/drivers/mtd/nand/raw/nand_base.c > @@ -5040,6 +5040,9 @@ static int nand_scan_ident(struct nand_chip *chip, unsigned int maxchips, > > nand_deselect_target(chip); > > + /* If the chip turns out ONFi, we can optimize timing parameters. */ > + onfi_fill_data_interface(chip, NAND_SDR_IFACE, 0); > + > /* Check for a chip array */ > for (i = 1; i < maxchips; i++) { > u8 id[2]; Thanks, Miquèl ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mtd: rawnand: call onfi_fill_data_interface() once again after nand_detect 2019-02-07 10:16 ` Miquel Raynal @ 2019-02-07 10:46 ` Masahiro Yamada 2019-02-07 10:51 ` Masahiro Yamada 2019-02-07 13:01 ` Miquel Raynal 0 siblings, 2 replies; 7+ messages in thread From: Masahiro Yamada @ 2019-02-07 10:46 UTC (permalink / raw) To: Miquel Raynal Cc: Marek Vasut, Richard Weinberger, Boris Brezillon, Linux Kernel Mailing List, Boris Brezillon, linux-mtd, Brian Norris, David Woodhouse On Thu, Feb 7, 2019 at 7:16 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Masahiro, > > Masahiro Yamada <yamada.masahiro@socionext.com> wrote on Thu, 7 Feb > 2019 18:57:56 +0900: > > > nand_scan_ident() calls onfi_fill_data_interface() at its entry > > to set up the initial timing parameters. > > > > The timing parameters are needed not only for ->setup_data_interface(), > > but also for giving the correct delay to NAND_OP_WAIT_RDY, for example. > > > > If the driver sets the NAND_KEEP_TIMINGS flag, or does not support > > ->setup_data_interface() hook, those parameters will never updated. > > ^ be Will fix (if v2 is welcome) > > > > Before nand_detect(), we never know whether the chip is ONFi or not. > > So, onfi_fill_data_interface() has to assume the worst case, i.e. > > non-ONFi. > > s/ONFi/ONFI/? Will fix. Looks like I was misunderstanding maybe because the letter 'I' in the logo (http://www.onfi.org/) looks like a lowercase... > > > > > After nand_detect(), if the chip turns out to be ONFi-compliant, > > we can optimize tPROG_max, tBERS_max, etc. > > > > Call onfi_fill_data_interface() once again. > > Sorry but I don't get why this is needed as there is the same call at > the top of this function. Can you be more specific on where/when the > missing call produces a failure? onfi_fill_data_interface() sets different values for tPROG_max, tBER_max, tR_max, tCCS_min depending on whether the chip is ONFI or not. For the first call, onfi_fill_data_interface() chooses the else-part since we never know the chip specification at this point. If we call onfi_fill_data_interface() once again after nand_detect(), it may choose the if-part. If a driver supports ->setup_data_interface(), nand_init_data_interface() will set the optimal timing parameters anyway. But, if a driver does not support ->setup_data_interface(), it will not happen since nand_has_setup_data_iface() returns false. > > > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > --- > > > > drivers/mtd/nand/raw/nand_base.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c > > index 9b3d7ff..35e543c 100644 > > --- a/drivers/mtd/nand/raw/nand_base.c > > +++ b/drivers/mtd/nand/raw/nand_base.c > > @@ -5040,6 +5040,9 @@ static int nand_scan_ident(struct nand_chip *chip, unsigned int maxchips, > > > > nand_deselect_target(chip); > > > > + /* If the chip turns out ONFi, we can optimize timing parameters. */ > > + onfi_fill_data_interface(chip, NAND_SDR_IFACE, 0); > > + > > /* Check for a chip array */ > > for (i = 1; i < maxchips; i++) { > > u8 id[2]; > > > Thanks, > Miquèl -- Best Regards Masahiro Yamada ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mtd: rawnand: call onfi_fill_data_interface() once again after nand_detect 2019-02-07 10:46 ` Masahiro Yamada @ 2019-02-07 10:51 ` Masahiro Yamada 2019-02-07 13:01 ` Miquel Raynal 1 sibling, 0 replies; 7+ messages in thread From: Masahiro Yamada @ 2019-02-07 10:51 UTC (permalink / raw) To: Miquel Raynal Cc: Marek Vasut, Richard Weinberger, Boris Brezillon, Linux Kernel Mailing List, Boris Brezillon, linux-mtd, Brian Norris, David Woodhouse On Thu, Feb 7, 2019 at 7:46 PM Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > On Thu, Feb 7, 2019 at 7:16 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Masahiro, > > > > Masahiro Yamada <yamada.masahiro@socionext.com> wrote on Thu, 7 Feb > > 2019 18:57:56 +0900: > > > > > nand_scan_ident() calls onfi_fill_data_interface() at its entry > > > to set up the initial timing parameters. > > > > > > The timing parameters are needed not only for ->setup_data_interface(), > > > but also for giving the correct delay to NAND_OP_WAIT_RDY, for example. > > > > > > If the driver sets the NAND_KEEP_TIMINGS flag, or does not support > > > ->setup_data_interface() hook, those parameters will never updated. > > > > ^ be > > Will fix (if v2 is welcome) > > > > > > > > Before nand_detect(), we never know whether the chip is ONFi or not. > > > So, onfi_fill_data_interface() has to assume the worst case, i.e. > > > non-ONFi. > > > > s/ONFi/ONFI/? > > Will fix. > > Looks like I was misunderstanding > maybe because the letter 'I' in the logo > (http://www.onfi.org/) > looks like a lowercase... http://www.onfi.org/ says "Discover the advantages of an ONFi world" Perhaps, is ONFi also correct?? Anyway, I will align with the majority. There are only three instances in the kernel tree. $ git grep ONFi drivers/mtd/nand/raw/nand_legacy.c: * a byte. The ONFi spec (Revision 3.1; 2012-09-19, Section 2.16) reads: drivers/mtd/nand/raw/nand_legacy.c: /* EZ-NAND can take upto 250ms as per ONFi v4.0 */ drivers/mtd/nand/raw/nand_legacy.c: /* EZ-NAND can take upto 250ms as per ONFi v4.0 */ -- Best Regards Masahiro Yamada ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mtd: rawnand: call onfi_fill_data_interface() once again after nand_detect 2019-02-07 10:46 ` Masahiro Yamada 2019-02-07 10:51 ` Masahiro Yamada @ 2019-02-07 13:01 ` Miquel Raynal 2019-02-08 8:35 ` Masahiro Yamada 1 sibling, 1 reply; 7+ messages in thread From: Miquel Raynal @ 2019-02-07 13:01 UTC (permalink / raw) To: Masahiro Yamada Cc: Marek Vasut, Richard Weinberger, Boris Brezillon, Linux Kernel Mailing List, Boris Brezillon, linux-mtd, Brian Norris, David Woodhouse Hi Masahiro, Masahiro Yamada <yamada.masahiro@socionext.com> wrote on Thu, 7 Feb 2019 19:46:54 +0900: > On Thu, Feb 7, 2019 at 7:16 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Masahiro, > > > > Masahiro Yamada <yamada.masahiro@socionext.com> wrote on Thu, 7 Feb > > 2019 18:57:56 +0900: > > > > > nand_scan_ident() calls onfi_fill_data_interface() at its entry > > > to set up the initial timing parameters. > > > > > > The timing parameters are needed not only for ->setup_data_interface(), > > > but also for giving the correct delay to NAND_OP_WAIT_RDY, for example. > > > > > > If the driver sets the NAND_KEEP_TIMINGS flag, or does not support > > > ->setup_data_interface() hook, those parameters will never updated. > > > > ^ be > > Will fix (if v2 is welcome) > > > > > > > > Before nand_detect(), we never know whether the chip is ONFi or not. > > > So, onfi_fill_data_interface() has to assume the worst case, i.e. > > > non-ONFi. > > > > s/ONFi/ONFI/? > > Will fix. > > Looks like I was misunderstanding > maybe because the letter 'I' in the logo > (http://www.onfi.org/) > looks like a lowercase... > > Oh right. I don't know what's best. Pick your favorite :) > > > > > > > > > > After nand_detect(), if the chip turns out to be ONFi-compliant, > > > we can optimize tPROG_max, tBERS_max, etc. > > > > > > Call onfi_fill_data_interface() once again. > > > > Sorry but I don't get why this is needed as there is the same call at > > the top of this function. Can you be more specific on where/when the > > missing call produces a failure? > > > onfi_fill_data_interface() sets different values > for tPROG_max, tBER_max, tR_max, tCCS_min > depending on whether the chip is ONFI or not. > > For the first call, onfi_fill_data_interface() > chooses the else-part since we never know > the chip specification at this point. > > If we call onfi_fill_data_interface() once again > after nand_detect(), it may choose the if-part. > > > If a driver supports ->setup_data_interface(), > nand_init_data_interface() will set the optimal > timing parameters anyway. > > But, if a driver does not support ->setup_data_interface(), > it will not happen since nand_has_setup_data_iface() returns false. And I think this is the expected behavior. Calling again onfi_fill_data_interface() would probably enhance a bit the timings. The effect is that later exchanges with the NAND chip would be just a bit faster. But if you care about performance, then why not implementing ->setup_data_interface()? Even a dummy implementation would do the trick: only accept timing mode 0 without any changes on the controller side. Unless you give me a use case where this is not possible, I don't think it is worth changing this path. Thanks, Miquèl ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mtd: rawnand: call onfi_fill_data_interface() once again after nand_detect 2019-02-07 13:01 ` Miquel Raynal @ 2019-02-08 8:35 ` Masahiro Yamada 2019-02-08 21:45 ` Miquel Raynal 0 siblings, 1 reply; 7+ messages in thread From: Masahiro Yamada @ 2019-02-08 8:35 UTC (permalink / raw) To: Miquel Raynal Cc: Boris Brezillon, Richard Weinberger, Linux Kernel Mailing List, Marek Vasut, linux-mtd, Brian Norris, David Woodhouse HI Miquel, On Thu, Feb 7, 2019 at 10:02 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Masahiro, > > Masahiro Yamada <yamada.masahiro@socionext.com> wrote on Thu, 7 Feb > 2019 19:46:54 +0900: > > > On Thu, Feb 7, 2019 at 7:16 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > Hi Masahiro, > > > > > > Masahiro Yamada <yamada.masahiro@socionext.com> wrote on Thu, 7 Feb > > > 2019 18:57:56 +0900: > > > > > > > nand_scan_ident() calls onfi_fill_data_interface() at its entry > > > > to set up the initial timing parameters. > > > > > > > > The timing parameters are needed not only for ->setup_data_interface(), > > > > but also for giving the correct delay to NAND_OP_WAIT_RDY, for example. > > > > > > > > If the driver sets the NAND_KEEP_TIMINGS flag, or does not support > > > > ->setup_data_interface() hook, those parameters will never updated. > > > > > > ^ be > > > > Will fix (if v2 is welcome) > > > > > > > > > > > > Before nand_detect(), we never know whether the chip is ONFi or not. > > > > So, onfi_fill_data_interface() has to assume the worst case, i.e. > > > > non-ONFi. > > > > > > s/ONFi/ONFI/? > > > > Will fix. > > > > Looks like I was misunderstanding > > maybe because the letter 'I' in the logo > > (http://www.onfi.org/) > > looks like a lowercase... > > > > > > Oh right. I don't know what's best. Pick your favorite :) > > > > > > > > > > > > > > > > After nand_detect(), if the chip turns out to be ONFi-compliant, > > > > we can optimize tPROG_max, tBERS_max, etc. > > > > > > > > Call onfi_fill_data_interface() once again. > > > > > > Sorry but I don't get why this is needed as there is the same call at > > > the top of this function. Can you be more specific on where/when the > > > missing call produces a failure? > > > > > > onfi_fill_data_interface() sets different values > > for tPROG_max, tBER_max, tR_max, tCCS_min > > depending on whether the chip is ONFI or not. > > > > For the first call, onfi_fill_data_interface() > > chooses the else-part since we never know > > the chip specification at this point. > > > > If we call onfi_fill_data_interface() once again > > after nand_detect(), it may choose the if-part. > > > > > > If a driver supports ->setup_data_interface(), > > nand_init_data_interface() will set the optimal > > timing parameters anyway. > > > > But, if a driver does not support ->setup_data_interface(), > > it will not happen since nand_has_setup_data_iface() returns false. > > And I think this is the expected behavior. Calling again > onfi_fill_data_interface() would probably enhance a bit the timings. > The effect is that later exchanges with the NAND chip would be just a > bit faster. But if you care about performance, then why not implementing > ->setup_data_interface()? Even a dummy implementation would do the > trick: only accept timing mode 0 without any changes on the controller > side. My driver (denali) does implement ->setup_data_interface(). When I was testing this thoroughly on my board, I noticed the timing parameters were slightly changed after nand_detect() detected ONFI chip. > Unless you give me a use case where this is not possible, I don't think > it is worth changing this path. Only the use case I can come up with is when NAND_KEEP_TIMINGS was set. But, it is just a matter of timeout values. So, please throw away this patch. > > Thanks, > Miquèl > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ -- Best Regards Masahiro Yamada ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mtd: rawnand: call onfi_fill_data_interface() once again after nand_detect 2019-02-08 8:35 ` Masahiro Yamada @ 2019-02-08 21:45 ` Miquel Raynal 0 siblings, 0 replies; 7+ messages in thread From: Miquel Raynal @ 2019-02-08 21:45 UTC (permalink / raw) To: Masahiro Yamada Cc: Boris Brezillon, Richard Weinberger, Linux Kernel Mailing List, Marek Vasut, linux-mtd, Brian Norris, David Woodhouse Hi Masahiro, Masahiro Yamada <yamada.masahiro@socionext.com> wrote on Fri, 8 Feb 2019 17:35:32 +0900: > HI Miquel, > > On Thu, Feb 7, 2019 at 10:02 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Masahiro, > > > > Masahiro Yamada <yamada.masahiro@socionext.com> wrote on Thu, 7 Feb > > 2019 19:46:54 +0900: > > > > > On Thu, Feb 7, 2019 at 7:16 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > Hi Masahiro, > > > > > > > > Masahiro Yamada <yamada.masahiro@socionext.com> wrote on Thu, 7 Feb > > > > 2019 18:57:56 +0900: > > > > > > > > > nand_scan_ident() calls onfi_fill_data_interface() at its entry > > > > > to set up the initial timing parameters. > > > > > > > > > > The timing parameters are needed not only for ->setup_data_interface(), > > > > > but also for giving the correct delay to NAND_OP_WAIT_RDY, for example. > > > > > > > > > > If the driver sets the NAND_KEEP_TIMINGS flag, or does not support > > > > > ->setup_data_interface() hook, those parameters will never updated. > > > > > > > > ^ be > > > > > > Will fix (if v2 is welcome) > > > > > > > > > > > > > > > > Before nand_detect(), we never know whether the chip is ONFi or not. > > > > > So, onfi_fill_data_interface() has to assume the worst case, i.e. > > > > > non-ONFi. > > > > > > > > s/ONFi/ONFI/? > > > > > > Will fix. > > > > > > Looks like I was misunderstanding > > > maybe because the letter 'I' in the logo > > > (http://www.onfi.org/) > > > looks like a lowercase... > > > > > > > > > > Oh right. I don't know what's best. Pick your favorite :) > > > > > > > > > > > > > > > > > > > > > > After nand_detect(), if the chip turns out to be ONFi-compliant, > > > > > we can optimize tPROG_max, tBERS_max, etc. > > > > > > > > > > Call onfi_fill_data_interface() once again. > > > > > > > > Sorry but I don't get why this is needed as there is the same call at > > > > the top of this function. Can you be more specific on where/when the > > > > missing call produces a failure? > > > > > > > > > onfi_fill_data_interface() sets different values > > > for tPROG_max, tBER_max, tR_max, tCCS_min > > > depending on whether the chip is ONFI or not. > > > > > > For the first call, onfi_fill_data_interface() > > > chooses the else-part since we never know > > > the chip specification at this point. > > > > > > If we call onfi_fill_data_interface() once again > > > after nand_detect(), it may choose the if-part. > > > > > > > > > If a driver supports ->setup_data_interface(), > > > nand_init_data_interface() will set the optimal > > > timing parameters anyway. > > > > > > But, if a driver does not support ->setup_data_interface(), > > > it will not happen since nand_has_setup_data_iface() returns false. > > > > And I think this is the expected behavior. Calling again > > onfi_fill_data_interface() would probably enhance a bit the timings. > > The effect is that later exchanges with the NAND chip would be just a > > bit faster. But if you care about performance, then why not implementing > > ->setup_data_interface()? Even a dummy implementation would do the > > trick: only accept timing mode 0 without any changes on the controller > > side. > > > My driver (denali) does implement ->setup_data_interface(). Fortunately, yes! :) > > When I was testing this thoroughly on my board, > I noticed the timing parameters were slightly changed > after nand_detect() detected ONFI chip. I see. > > > Unless you give me a use case where this is not possible, I don't think > > it is worth changing this path. > > Only the use case I can come up with is when NAND_KEEP_TIMINGS was set. > But, it is just a matter of timeout values. > > So, please throw away this patch. Ok! Thanks anyway for the proposal! Miquèl ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-02-08 21:45 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-07 9:57 [PATCH] mtd: rawnand: call onfi_fill_data_interface() once again after nand_detect Masahiro Yamada 2019-02-07 10:16 ` Miquel Raynal 2019-02-07 10:46 ` Masahiro Yamada 2019-02-07 10:51 ` Masahiro Yamada 2019-02-07 13:01 ` Miquel Raynal 2019-02-08 8:35 ` Masahiro Yamada 2019-02-08 21:45 ` Miquel Raynal
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).