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

[Marek Vasut <marex@xxxxxxx>]

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.

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

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

Great :)

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