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: Wed, 5 Feb 2020 10:50:45 +0100	[thread overview]
Message-ID: <20200205105045.6877aca6@xps13> (raw)
In-Reply-To: <45a10680-5fe6-7cab-a7ef-f7f7a952e822@denx.de>

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.

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

> 
> >> +	if (denali->clk_rate && denali->clk_x_rate)
> >>  		chip->options |= NAND_KEEP_TIMINGS;  
> 
> [...]

Thanks,
Miquèl

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