Re: DENALI: can't detect NAND chip

Masahiro Yamada
2018-06-12T09:26:05Z

2018-06-12 18:21 GMT+09:00 Richard Weinberger <richard@sigma-star.at>:
> Am Dienstag, 5. Juni 2018, 09:36:27 CEST schrieb Boris Brezillon:
>> On Tue, 5 Jun 2018 10:43:49 +0900
>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>>
>> > 2018-06-05 7:01 GMT+09:00 Richard Weinberger <richard@sigma-star.at>:
>> > > Am Montag, 4. Juni 2018, 22:57:32 CEST schrieb Richard Weinberger:
>> > >> Am Montag, 4. Juni 2018, 22:51:17 CEST schrieb Boris Brezillon:
>> > >> > > According to the datasheet, the NAND clock rate should be 50Mhz, which is what
>> > >> > > is returned in both cases.
>> > >> > > So this does not really look invalid to me. :)
>> > >> >
>> > >> > Did you try to put a scope on the RE or WE pin? The x2 factor looks too
>> > >> > perfect to be just a coincidence.
>> > >>
>> > >> Not yet, but you are right the x2 factor is really interesting.
>> > >
>> > > The NFC uses two clocks, nand_x_clk and nand_clk.
>> > > nand_clk is nand_x_clk / 4.
>> > > In the device tree nand_clk is referenced, and therefore used to calculate
>> > > the timings. So we might need to use the 200Mhz clock instead of the 50Mhz
>> > > for the calculation.
>> > >
>> >
>> >
>> > Yes.
>> >
>> >
>> > Strictly speaking, the Denali IP takes three clocks.
>> >
>> > [1] clk : core clock
>> > [2] clk_x: bus interface clock
>> > [3] ecc_clk: for ECC engine
>> >
>> >
>> >
>> > Also in my SoCs (Socionext UniPhier),
>> >
>> > clk = 50MHz,
>> > clk_x = ecc_clk = 200MHz
>> >
>> >
>> >
>> > In this case, the ->setup_data_interface hook
>> > is interested in the frequency of [2] clk_x.
>> >
>> > However, clk_x is not a core clock.
>> > I admit this is confusing.
>> >
>> >
>> > If we really want to make the DT-binding precise,
>> > we can change the clock requirement like follows:
>> >
>> > clock-names = "clk", "clk_x", "ecc_clk";
>>
>> How about:
>>
>> clock-names = "nand", "nand_x", "ecc";
>>
>> The clk prefix/suffix is really redundant here.
>>
>> > clocks = <...>, <...>, <...>;
>>
>> If the IP really takes 3 different clks in input, then it should be
>> represented like that.
>>
>> >
>> >
>> > This might be an annoying churn, though...
>> >
>> >
>>
>> I guess you're talking about backward compat with existing dtb. This
>> should be a problem, and we can even fix the Richard's problem for
>> those dtbs if we hardcode the nand_x_clk rate to 200Mhz when nand_x_clk
>> is missing (see below).
>
> [...]
>
> Masahiro, which fix do you prefer?
>
> We could also just fix the dts file.
> e.g.
> commit c0097d0b498bdefb427c352d615432e97f1998aa
> Author: Richard Weinberger <richard@nod.at>
> Date: Tue Jun 12 11:16:22 2018 +0200
>
> arm: dts: socfpga: Use right denali clock
>
> To calculate the timing parameters a 200MHz clock is needed, in case of
> socfpga this is nand_x_clk and not nand_clk.
>
> Cc: stable@kernel.org
> Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by setup_data_interface()")
> Signed-off-by: Richard Weinberger <richard@nod.at>
>
> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> index 486d4e7433ed..8fbddc2a5c33 100644
> --- a/arch/arm/boot/dts/socfpga.dtsi
> +++ b/arch/arm/boot/dts/socfpga.dtsi
> @@ -754,7 +754,7 @@
> reg-names = "nand_data", "denali_reg";
> interrupts = <0x0 0x90 0x4>;
> dma-mask = <0xffffffff>;
> - clocks = <&nand_clk>;
> + clocks = <&nand_x_clk>;
> status = "disabled";
> };
>
> Thanks,
> //richard
>

I almost finishing the denali driver update.
I am now writing commit log. Will post it soon.

(By taking care of the backward compatibility in the driver,
you do not need to rush to fix your DT.)

--
Best Regards
Masahiro Yamada