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 14:19:17 +0100:

> On 3/11/20 2:08 PM, Miquel Raynal wrote:
> > Hi Marek,  
> 
> Hi Miquel,
> 
> > Marek Vasut <marex@denx.de> wrote on Wed, 11 Mar 2020 13:52:30 +0100:
> >   
> >> On 3/9/20 11:27 AM, Masahiro Yamada wrote:  
> >>> Hi.    
> >>
> >> Hi,
> >>
> >> [...]
> >>  
> >>>>>> See attached patch, with which (without this revert) you get this:
> >>>>>> denali->reg + TWHR2_AND_WE_2_RE = 0x00001414 -> 0x0000143f
> >>>>>> denali->reg + TCWAW_AND_ADDR_2_DATA = 0x0000143f -> 0x00001432
> >>>>>> denali->reg + RE_2_WE = 0x00000014 -> 0x00000019
> >>>>>> denali->reg + ACC_CLKS = 0x00000004 -> 0x00000005
> >>>>>> denali->reg + RDWR_EN_LO_CNT = 0x00000002 -> 0x00000009
> >>>>>> denali->reg + RDWR_EN_HI_CNT = 0x00000002 -> 0x00000004
> >>>>>> denali->reg + CS_SETUP_CNT = 0x00000001 -> 0x00000008
> >>>>>> denali->reg + RE_2_RE = 0x00000014 -> 0x00000019    
> >>>>>
> >>>>> OK, the left-hand side is probably the timing
> >>>>> set up by U-Boot.    
> >>>>
> >>>> Yep, the timings that work. So now, how do you get to those working
> >>>> timings using the Linux driver ?    
> >>>
> >>>
> >>> How about
> >>> 0001-denali-more-complicated-calculation-for-timings.patch
> >>>
> >>> + following ?
> >>>
> >>> 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 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.

> 
> > 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.

> 
> > 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 :)

Thanks,
Miquèl

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