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

[Miquel Raynal <miquel.raynal@bootlin.com>]

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/