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

[Masahiro Yamada <masahiroy@xxxxxxxxxx>]

Hi,


On Sat, Mar 14, 2020 at 11:49 PM Marek Vasut <marex@xxxxxxx> wrote:
>
> On 3/11/20 3:39 PM, Miquel Raynal wrote:
> > Hi Marek,
>
> Hello Miquel,
>
> [...]
>
> >>> 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.
>
> The calculation is obviously not the same anymore, due to the recent
> changes in the Linux driver, which seems to have broken it (in Linux).
>
> >>>>> 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.
>
> If the value were straight at the edge, I would expect this would
> trigger errors over the years, when those values were used, or maybe it
> would trigger an error in the thermal chamber tests ? If neither of that
> happens, then the values are probably not at the edge enough to matter.


This is a trade-off between the performance and the safety.

If you want to be very safe, you can use
the power-on-reset defaults.
The default register values are chosen to insert long enough
wait time to work with any devices.
Of course, it is slow, though.

Generally speaking, the datasheet spec contains the deviation,
so I think aligning with nand_timings.c will guarantee the
device will work over years.

So, in my understanding, ->setup_data_interface() hook
can simply encode struct nand_sdr_timings into the
controller registers.

One question I am still wondering is why this problem was
triggered only after Linux 4.19

As you said, the clock manager on SOCFPGA can change the
clock frequencies that drive the NAND controller.

In your setting,
denali->clk_rate = 31.25 MHz and denali->clk_x_rate = 125 MHz

But, maybe
denali->clk_rate = 50 MHz and denali->clk_x_rate = 200 MHz
on somebody else's. This exactly matches to the frequencies
the denali drivers originally used.
So, the register values are on the edge in this case.
But, nobody reported the problem before v4.19.





> That said, timing calculations do not factor in only the datasheet
> values, but also signal propagation delays and other details of the data
> path on the PCB and in the SOC, so it's not as simple as you claim it
> is.


I think you are right.
Most of parameters can be derived from the ONFI datasheet,
but the acc_clks register must contain the propagation delays.

This is why I was asking you to test the iffy patches
that increment the acc_clks by 1 or 2.



> Moreover, while the NAND datasheet is available in public, the
> Denali IP datasheet is not, so "do the math with a paper and compare" is
> inapplicable here either way, sorry.
>
> >>>>> 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?
>
> The timings which went through extensive testing are the original ones.
>
> The ones coming out of Masahiro's patch at least do not trigger those
> massive UBI errors, however they were tested only very lightly. And I
> feel like adding +1/-1 somewhere into the patch is rather iffy, so I
> would hope the Denali datasheet has something about this and why this is
> needed.


This is iffy, but I have no idea to parameterize
the SoC/board-dependent delay.

In the posted patch, I used 'data_setup_on_host'
to avoid the magic number.




>
> > Because "perfect timings" => "work in the oven" but let be clear on
> > the fact that "work in the oven" does not imply "perfect timings".
>
> Let's be clear that I still prefer "practically working and possibly
> imperfect" over "theoretically perfect and practically not working".
>
> Also, correction, thermal chamber is not an owen, it does testing over
> the entire temperature range of the device.
>
> --
> Best regards,
> Marek Vasut



-- 
Best Regards
Masahiro Yamada

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