From: Masahiro Yamada <yamada.masahiro@socionext.com>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Boris Brezillon <bbrezillon@kernel.org>,
Richard Weinberger <richard@nod.at>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Marek Vasut <marek.vasut@gmail.com>,
linux-mtd <linux-mtd@lists.infradead.org>,
Brian Norris <computersforpeace@gmail.com>,
David Woodhouse <dwmw2@infradead.org>
Subject: Re: [PATCH] mtd: rawnand: call onfi_fill_data_interface() once again after nand_detect
Date: Fri, 8 Feb 2019 17:35:32 +0900 [thread overview]
Message-ID: <CAK7LNAQ7P8iEkhyRBbHLdt6yN-dDgpLZOs256qwk2BpADiGcGw@mail.gmail.com> (raw)
In-Reply-To: <20190207140136.2ae62982@xps13>
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/
next prev parent reply other threads:[~2019-02-08 8:36 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2019-02-08 21:45 ` Miquel Raynal
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAK7LNAQ7P8iEkhyRBbHLdt6yN-dDgpLZOs256qwk2BpADiGcGw@mail.gmail.com \
--to=yamada.masahiro@socionext.com \
--cc=bbrezillon@kernel.org \
--cc=computersforpeace@gmail.com \
--cc=dwmw2@infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=marek.vasut@gmail.com \
--cc=miquel.raynal@bootlin.com \
--cc=richard@nod.at \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).