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

[Marek Vasut <marex@xxxxxxx>]

On 3/11/20 2:08 PM, Miquel Raynal wrote:
> Hi Marek,

Hi Miquel,

> Marek Vasut <marex@xxxxxxx> 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 ?

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

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

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