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

[Masahiro Yamada <masahiroy@xxxxxxxxxx>]

Hi.


On Wed, Feb 12, 2020 at 10:41 PM Marek Vasut <marex@xxxxxxx> wrote:
>
> On 2/12/20 10:00 AM, Masahiro Yamada wrote:
> > Hi Marek,
>
> Hi,
>
> > 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.
>
> You can configure whatever rate you want in the QSys HPS component.

OK.

The SOCFPGA maintainer, Dinh Nguyen, said this:
"From the clock controller, it provides a single 200MHz clock to the NAND
IP. Inside the NAND IP, there is a /4 for the clk. The 200MHz clock is
used for the clk_x and ecc_clk."


http://lists.infradead.org/pipermail/linux-arm-kernel/2018-July/592702.html



Maybe, you are using a brand-new,
different type of SOCFPGA?



> > What would happen if you hard-code:
> > denali->clk_rate = 50000000;
> > denali->clk_x_rate = 200000000;
>
> It will not work, because the IP would be using incorrect clock.

I wanted to see the past tense here instead of
future tense + subjunctive mood.

I wanted you to try it.



>
> > 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/.
>
> No, this is a feature of the platform, you can configure any clock you
> want pretty much.


OK, but if you agree the 4.19.10 is working,

denali->clk_rate = 50000000; denali->clk_x_rate = 200000000;

is worth trying.


>
> > 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.
>
> The 4.19 was working fine for Tim (and me as well), because it didn't
> have this bug which this patch removes.


d311e0c27b8fcc27f707f8ca did not exist in 4.19

But, 7a08dbaedd36 did not exist either in 4.19


$ git describe  7a08dbae
v4.20-rc2-34-g7a08dbaedd36
$ git describe  d311e0c
v5.0-rc2-3-gd311e0c27b8f


So, your patch is getting it back to
v4.20-rc2-34-g7a08dbaedd36
where the condition for ->setup_data_interface() call
is accidentally inverted for the Denali driver.



BTW, did you notice your code was doing the opposite
to your commit description?


Your commit description goes like this:

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


You clearly state denali->clk_rate and denali->clk_x_rate
are non-zero values.

If this patch were applied, we would end up with NAND_KEEP_TIMINGS set.
Then ->setup_data_interface() hook would be skipped.
So, the timings from previous stage would be retained.

Is this what you want to do?


One more thing, if your patch were applied,
we would get back to the situation
where the back-trace happens due to zero-division:


When denali->clk_x_rate is zero,
NAND_KEEP_TIMINGS would not be set with your patch.
So, ->setup_data_interface() would be called.

This would cause zero divion at line 781.
        t_x = DIV_ROUND_DOWN_ULL(1000000000000ULL, denali->clk_x_rate);



-- 
Best Regards
Masahiro Yamada

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