From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Marek Vasut <marex@denx.de>
Cc: Dinh Nguyen <dinguyen@kernel.org>,
Masahiro Yamada <masahiroy@kernel.org>,
Boris Brezillon <boris.brezillon@collabora.com>,
linux-mtd <linux-mtd@lists.infradead.org>,
Tim Sander <tim@krieglstein.org>
Subject: Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
Date: Wed, 11 Mar 2020 15:39:23 +0100 [thread overview]
Message-ID: <20200311153923.443f3e64@xps13> (raw)
In-Reply-To: <aaec50bb-05da-8d4e-3e15-17fbfeb52f68@denx.de>
Hi Marek,
Marek Vasut <marex@denx.de> wrote on Wed, 11 Mar 2020 15:07:27 +0100:
> On 3/11/20 2:33 PM, Miquel Raynal wrote:
> > Hi Marek,
>
> Hi,
>
> [...]
>
> >>>>> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
> >>>>> index b0482108a127..ea38aa42873e 100644
> >>>>> --- a/drivers/mtd/nand/raw/denali.c
> >>>>> +++ b/drivers/mtd/nand/raw/denali.c
> >>>>> @@ -860,9 +860,9 @@ static int denali_setup_data_interface(struct
> >>>>> nand_chip *chip, int chipnr,
> >>>>>
> >>>>> /*
> >>>>> * Determine the minimum of acc_clks to meet the data setup timing.
> >>>>> - * (one additional clock cycle just in case)
> >>>>> + * (two additional clock cycles just in case)
> >>>>> */
> >>>>> - acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x) + 1;
> >>>>> + acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x) + 2;
> >>>>>
> >>>>> /* Determine the minimum of rdwr_en_lo_cnt from RE#/WE# pulse width */
> >>>>> rdwr_en_lo = DIV_ROUND_UP(max(timings->tRP_min, timings->tWP_min), t_x);
> >>>>
> >>>> Like the attached one ?
> >>>>
> >>>> That seems to work, but -- the calculated timings differ from the ones
> >>>> which are calculated by U-Boot and which were tested to work well.
> >>>> That's not good, I would expect both timings to be identical:
> >>>
> >>> There is no such "timings tested to work well".
> >>
> >> Hmmm, the board went through full temperature range testing in a chamber
> >> with those timings and passed, and there are boards with those exact
> >> timings deployed for years now with older kernel version, which work
> >> too. So I would expect they are good and "timings tested to work well".
> >>
> >>> Timings represent
> >>> minimum and maximum values for certain operations on the NAND bus, you
> >>> can have two different values that will both work in the same
> >>> condition. And it is expected that Linux is more clever than U-Boot
> >>
> >> Errr, why ?
> >
> > Because sometimes people write simpler driver in U-Boot, or even
> > hardcoded values.
>
> I see, this is not the case with denali nand driver though.
>
> > I checked the denali driver and indeed u-boot should not be much clever
> > than Linux. Are the differences significant? The code is so close, you
> > can probably check why you have differences. Also verify that the same
> > ONFI mode is used.
>
> It might've made sense to check those driver differences before making
> such an statement ;-)
> That said, I don't think either U-Boot or Linux uses the ONFI
> information for this NAND, but I might be wrong.
I don't know what is the exact device but most of the time, even for
non ONFI-compliant chips, the core starts talking at the lowest ONFI
speed (mode 0) and then negotiate with the NAND chip the actual timings
to use. This works if get/set_features is supported, otherwise you
might have a default mode somewhere. Is it the same in both cases? Does
the core tries to apply the same timings? Is the calculation the same?
These are pointers but I am sure you can figure all that out.
> >>> and
> >>> may optimize better the timings depending on the selected mode ([0-5])
> >>> (hence the different calls to ->setup_data_interface().
> >>
> >> I would expect those two should produce identical timing parameters,
> >> period, otherwise one or the other is wrong. Thus far, it was Linux that
> >> produced non-working results.
> >
> > Again, we define minimum and maximum delays. If the right thing is to
> > not wait more than 5us and you wait up to 6, it does not mean you
> > wrote "bad timings". 4us would be a bad timing though. It depends on
> > what you are looking at.
>
> I am look at for example
>
> denali->reg + TCWAW_AND_ADDR_2_DATA = 0x0000143f -> 0x00001432
>
> Register was 0x143f before, now is 0x1432 , which is less.
> I guess that would be the "bad timing" then ?
Well, is it a minimum or a maximum ? How do you know U-Boot value is
straight on the edge? If you want to know if timings are valid, open
the part datasheet, do the math with a paper and compare. This is the
scientific way to declare timings valid or invalid.
> >>> Run a stress test, if it passes, you should be good :)
> >>
> >> Thank you for the hint, I think the stress test thus far could be
> >> considered sufficient. I guess we can agree on that ?
> >
> > Oh yeah absolutely :)
Just to be sure, we are talking about the new timings derived with
Masahiro's patch in Linux here, right?
Because "perfect timings" => "work in the oven" but let be clear on
the fact that "work in the oven" does not imply "perfect timings".
Thanks,
Miquèl
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
next prev parent reply other threads:[~2020-03-11 14:46 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-05 7:08 [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again" Marek Vasut
2020-02-05 9:12 ` Miquel Raynal
2020-02-05 9:41 ` Marek Vasut
2020-02-05 9:50 ` Miquel Raynal
2020-02-05 10:05 ` Boris Brezillon
2020-02-05 10:08 ` Marek Vasut
2020-02-11 10:04 ` Marek Vasut
2020-02-11 16:07 ` Miquel Raynal
2020-02-11 20:35 ` Marek Vasut
2020-02-12 9:00 ` Masahiro Yamada
2020-02-12 9:37 ` Marek Vasut
2020-02-12 16:56 ` Masahiro Yamada
2020-02-12 17:13 ` Masahiro Yamada
2020-02-12 17:44 ` Marek Vasut
2020-02-17 8:33 ` Masahiro Yamada
2020-02-18 5:55 ` Masahiro Yamada
2020-02-19 18:42 ` Marek Vasut
2020-02-25 0:41 ` Masahiro Yamada
2020-03-03 17:11 ` Marek Vasut
2020-03-09 10:27 ` Masahiro Yamada
2020-03-11 12:52 ` Marek Vasut
2020-03-11 13:08 ` Miquel Raynal
2020-03-11 13:19 ` Marek Vasut
2020-03-11 13:33 ` Miquel Raynal
2020-03-11 14:07 ` Marek Vasut
2020-03-11 14:39 ` Miquel Raynal [this message]
2020-03-14 14:48 ` Marek Vasut
2020-03-17 9:27 ` Masahiro Yamada
2020-03-16 4:36 ` Masahiro Yamada
2020-02-19 18:27 ` Marek Vasut
2020-02-25 0:38 ` Masahiro Yamada
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=20200311153923.443f3e64@xps13 \
--to=miquel.raynal@bootlin.com \
--cc=boris.brezillon@collabora.com \
--cc=dinguyen@kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=marex@denx.de \
--cc=masahiroy@kernel.org \
--cc=tim@krieglstein.org \
/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).