Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"

[Masahiro Yamada <masahiroy@kernel.org>]

Hi.


On Wed, Mar 11, 2020 at 11:39 PM Miquel Raynal
<miquel.raynal@bootlin.com> wrote:
>
> 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.



In order not to waste time,
I'd like to agree with the Linux/U-Boot difference.


The timing requirement for a NAND chip provided by
drivers/mtd/nand/raw/nand_timing.c
are represented in pico-seconds
(same between in Linux and U-Boot in this regard)


The registers in the Denali IP specifies
"how many clock cycles should be inserted
between rising/falling edges of signals".

So, the register values depend on both
[1] the timing mode of the chip
   and
[2] clock frequencies at which the Denali IP is running


To calculate the clock cycles, denali_setup_data_interface()
needs two clocks (controller core clock, and the NAND bus clock).
But, we should not hard-code the clock frequency in the denali driver.
(The denali driver hard-coded the clock frequencies for the backward
compatibility reason, but our ultimate goal is to move the clock
info to the clock driver.)

In other words, we rely on the clock driver being correct.


denali_setup_data_interface() does the equivalent computation
between Linux and U-Boot.

It is obvious by diffing denali_setup_data_interface().

https://github.com/torvalds/linux/blob/v5.6-rc5/drivers/mtd/nand/raw/denali.c#L764
https://github.com/u-boot/u-boot/blob/v2020.04-rc3/drivers/mtd/nand/raw/denali.c#L917



The difference is the clock frequencies (and he knows it).


In this patch, Marek claims  denali->clk_rate = 31.25 MHz
denali->clk_x_rate = 125 MHz on Linux running on his board.


I am not sure whether commit 3b5015c4d834 (i.e. Linux 5.3)
is related to this or not, but the clock frequencies were
changed after Linux 4.19.


Marek also said that the SOCFPGA clock driver is not ready for U-Boot
as he posted this to U-Boot ML:
http://patchwork.ozlabs.org/patch/1220287/

Then, the clock frequencies fall back to the hard-coded default:
denali->clk_rate = 50 MHz  denali->clk_x_rate = 200 MHz
https://github.com/u-boot/u-boot/blob/v2020.04-rc3/drivers/mtd/nand/raw/denali_dt.c#L141


From this point, it is obvious that U-Boot values are not straight on the edge.
(U-Boot divides the timing values represented in pico-seconds
with the too small clock period.)


Thanks.



> > >>> 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
--
Best Regards
Masahiro Yamada

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/