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

[Marek Vasut <marex@xxxxxxx>]

On 2/11/20 5:07 PM, Miquel Raynal wrote:
> Hi Marek, Masahiro,
> 
> Marek Vasut <marex@xxxxxxx> wrote on Tue, 11 Feb 2020 11:04:10 +0100:
> 
>> On 2/5/20 11:08 AM, Marek Vasut wrote:
>>> On 2/5/20 10:50 AM, Miquel Raynal wrote:  
>>>> Hi Marek,
>>>>
>>>> Marek Vasut <marex@xxxxxxx> wrote on Wed, 5 Feb 2020 10:41:05 +0100:
>>>>  
>>>>> On 2/5/20 10:12 AM, Miquel Raynal wrote:  
>>>>>> Hi Marek,
>>>>>>
>>>>>> Marek Vasut <marex@xxxxxxx> wrote on Wed,  5 Feb 2020 08:08:34 +0100:
>>>>>>     
>>>>>>> This reverts commit d311e0c27b8fcc27f707f8cac48cd8bdc4155224, which
>>>>>>> completely breaks NAND access on Altera SoCFPGA (detected on ArriaV
>>>>>>> SoC).
>>>>>>>
>>>>>>> On SoCFPGA, denali->clk_rate = 31.25 MHz and denali->clk_x_rate = 125 MHz,
>>>>>>> hence the driver sets NAND_KEEP_TIMINGS flag. This did not happen before
>>>>>>> and is actually incorrect, as on SoCFPGA we do not want to retain timings
>>>>>>> from previous stage (the timings might be incorrect or outright invalid).
>>>>>>>
>>>>>>> Cc: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
>>>>>>> Cc: Dinh Nguyen <dinguyen@xxxxxxxxxx>
>>>>>>> Cc: Masahiro Yamada <masahiroy@xxxxxxxxxx>
>>>>>>> Cc: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
>>>>>>> Cc: Tim Sander <tim@xxxxxxxxxxxxxxx>
>>>>>>> To: linux-mtd <linux-mtd@xxxxxxxxxxxxxxxxxxx>
>>>>>>> ---
>>>>>>>  drivers/mtd/nand/raw/denali.c | 2 +-
>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
>>>>>>> index b6c463d02167..5fe3c62a756e 100644
>>>>>>> --- a/drivers/mtd/nand/raw/denali.c
>>>>>>> +++ b/drivers/mtd/nand/raw/denali.c
>>>>>>> @@ -1209,7 +1209,7 @@ int denali_chip_init(struct denali_controller *denali,
>>>>>>>  	}
>>>>>>>  
>>>>>>>  	/* clk rate info is needed for setup_data_interface */
>>>>>>> -	if (!denali->clk_rate || !denali->clk_x_rate)    
>>>>>>
>>>>>> I don't get it, if both clk_rate and clk_x_rate are set, the if
>>>>>> condition will not be entered, right?    
>>>>>
>>>>> Err, then it's the other way around and I need to keep the timings on
>>>>> socfpga ?  
>>>>
>>>> Ok.
>>>>
>>>> Do you have a different compatible? Or a register to check? How do you
>>>> discriminate the different platforms by software? The quick and dirty
>>>> solution is to add a special case for your platform and specifically
>>>> use the NAND_KEEP_TIMINGS horror.  
>>>
>>> Sure, there's a socfpga compatible and at least two for uniphier.
>>>   
>>>> But I think using ->software_data_interface is the right solution. So
>>>> I would highly recommend fixing the implementation of this hook
>>>> for your platform and in this case the commit reverted is not the
>>>> culprit, the one introducing setup_data_interface is (for the Fixes:
>>>> tag).  
>>>
>>> I'll leave the details to Yamada-san.  
>>
>> Just got a confirmation that this fixes NAND behavior on SoCFPGA, so
>> this patch should go in in some form.
> 
> I'm sure it fixes it, but it is definitely not going in the right
> direction!
> 
> The right thing to do is fixing ->setup_data_interface().
> 
> The bad thing to do if someone tells me that he will fix
> ->setup_data_interface() in a second time is to keep the
> NAND_KEEP_TIMINGS flag only for a single compatible.

OK, I'll leave that to Yamada-san. I don't really want to interfere with
his work on the Denali NAND driver.

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