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

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Marek Vasut <marex@denx.de>
Cc: Dinh Nguyen <dinguyen@kernel.org>,
	Masahiro Yamada <masahiroy@kernel.org>,
	Boris Brezillon <boris.brezillon@collabora.com>,
	linux-mtd@lists.infradead.org, Tim Sander <tim@krieglstein.org>
Subject: Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
Date: Tue, 11 Feb 2020 17:07:07 +0100	[thread overview]
Message-ID: <20200211170707.2183625e@xps13> (raw)
In-Reply-To: <b6f094b7-01a9-34a2-bf10-019a05bc7cb3@denx.de>

Hi Marek, Masahiro,

Marek Vasut <marex@denx.de> 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@denx.de> 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@denx.de> 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@collabora.com>
> >>>>> Cc: Dinh Nguyen <dinguyen@kernel.org>
> >>>>> Cc: Masahiro Yamada <masahiroy@kernel.org>
> >>>>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> >>>>> Cc: Tim Sander <tim@krieglstein.org>
> >>>>> To: linux-mtd <linux-mtd@lists.infradead.org>
> >>>>> ---
> >>>>>  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.

Thanks,
Miquèl

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