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

[Masahiro Yamada <masahiroy@xxxxxxxxxx>]

Hi.

On Thu, Feb 20, 2020 at 3:44 AM Marek Vasut <marex@xxxxxxx> wrote:
>
> On 2/17/20 9:33 AM, Masahiro Yamada wrote:
> > Hi,
>
> Hi,
>
> [...]
>
> > If possible, can you provide the dump of
> > the attached debug code?
>
> Without this revert:


Thanks for the dump.

denali_setup_data_interface() is called multiple times.

>
> Denali: clk_rate=31250000, clk_x_rate=125000000
> Denali: tREA=40000
> Denali: acc_clks=5
> Denali: tRHW=200000
> Denali: re_2_we=25
> Denali: tRHZ=200000
> Denali: re_2_re=25
> Denali: tCCS=500000000
> Denali: tWHR=120000
> Denali: we_2_re=63
> Denali: tADL=400000
> Denali: addr_2_data=50
> Denali: tREH=30000
> Denali: tWH=30000
> Denali: rdwr_en_hi=4
> Denali: tRP=50000
> Denali: tWP=50000
> Denali: tRC=100000
> Denali: tWC=100000
> Denali: rdwr_en_lo_hi=13
> Denali: rdwr_en_lo=9
> Denali: tCS=70000
> Denali: tCEA=100000
> Denali: cs_setup=8


This is the first call,
which I am not interested in.



> Denali: clk_rate=31250000, clk_x_rate=125000000
> Denali: tREA=16000
> Denali: acc_clks=2
> Denali: tRHW=100000
> Denali: re_2_we=13
> Denali: tRHZ=100000
> Denali: re_2_re=13
> Denali: tCCS=100000
> Denali: tWHR=80000
> Denali: we_2_re=13
> Denali: tADL=400000
> Denali: addr_2_data=50
> Denali: tREH=7000
> Denali: tWH=7000
> Denali: rdwr_en_hi=1
> Denali: tRP=10000
> Denali: tWP=10000
> Denali: tRC=20000
> Denali: tWC=20000
> Denali: rdwr_en_lo_hi=4
> Denali: rdwr_en_lo=3
> Denali: tCS=15000
> Denali: tCEA=25000
> Denali: cs_setup=2


This is the second call,
which sets up the final register values.

The parameter, acc_clks=2, is
the part I suspect the most.

(and that is why I attached two patches
to tweak acc_clks).



>
> With this revert, setup_data_interface() is not called, so there is no log.
>
> [...]
>
> >>> 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);
> >>
> >> If the clock are non-zero, how would this be a division by zero ?
> >
> >
> > You have a false assumption "If the clock are non-zero...".
> >
> > clk_get_rate() may return zero if the clock driver
> > is not ready to provide the frequency information.
> >
> >
> >
> > The current code:
> >   If clk_rate or clk_x_rate is zero,
> >    do not call denali_setup_data_interface().
> >   If neither clk_rate nor clk_x is zero,
> >    call denali_setup_data_interface().
> >
> >
> > With your patch:
> >   If clk_rate or clk_x_rate is zero,
> >    call denali_setup_data_interface(),
> >    which causes zero division.
> >   If neither clk_rate nor clk_x is zero,
> >    do not call denali_setup_data_interface().
>
> OK, so it's just a miscommunication. In my case, neither of the clock
> are zero. On SoCFPGA, I think clk_rate = clk_x_rate / 4, but I'm not
> sure if that's always the case.


Drivers must be written in a generic manner
to take care of any possibility.

clk_get_rate() returns 0 if the clock driver
does not support ->recalc_rate() operation
or CONFIG_HAVE_CLK=n.



-- 
Best Regards
Masahiro Yamada

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