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

[Masahiro Yamada <masahiroy@xxxxxxxxxx>]

Hi Marek,


On Wed, Feb 12, 2020 at 5:35 AM Marek Vasut <marex@xxxxxxx> wrote:
>
> 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.


Interesting.
I have never seen such clock rates before.


For all known upstream platforms
(Altera SOCFPGA, Socionext UniPhier, Intel MRST),
the NAND controller core clock is 50 MHz,
the NAND bus clock is 200MHz.



What would happen if you hard-code:
denali->clk_rate = 50000000;
denali->clk_x_rate = 200000000;

like I had already suggested to Tim Sander:
https://lore.kernel.org/lkml/CAK7LNAQOCoJC0RzOhTEofHdR+zU5sQTxV-t4nERBExW1ddW5hw@xxxxxxxxxxxxxx/

Unfortunately, he did not want to do it, but
I am still interested in this experiment because
I suspect this might be a bug of drivers/clk/socfpga/.


4.19.10 kernel (, which Tim Sander agreed the timing was working fine)
was hard-coding them in order to deal with the immature SOCFPGA clock driver.

See this code:
https://elixir.bootlin.com/linux/v4.19.10/source/drivers/mtd/nand/raw/denali_dt.c#L162


Masahiro Yamada




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



-- 
Best Regards
Masahiro Yamada

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