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

From: Boris Brezillon <boris.brezillon@collabora.com>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Marek Vasut <marex@denx.de>, Dinh Nguyen <dinguyen@kernel.org>,
	Masahiro Yamada <masahiroy@kernel.org>,
	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 11:05:14 +0100	[thread overview]
Message-ID: <20200205110514.27406d5a@collabora.com> (raw)
In-Reply-To: <20200205105045.6877aca6@xps13>

On Wed, 5 Feb 2020 10:50:45 +0100
Miquel Raynal <miquel.raynal@bootlin.com> 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.
> 
> But I think using ->software_data_interface is the right solution. So

You probably mean ->setup_data_interface() :-).

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

+1. If ->setup_data_interface() is buggy, it should be fixed.

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