From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.bootlin.com ([62.4.15.54]) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1fSfdA-00028k-0A for linux-mtd@lists.infradead.org; Tue, 12 Jun 2018 09:30:26 +0000 Date: Tue, 12 Jun 2018 11:29:58 +0200 From: Boris Brezillon To: Richard Weinberger Cc: Masahiro Yamada , Philipp Rosenberger , linux-mtd , Benedikt Spranger Subject: Re: DENALI: can't detect NAND chip Message-ID: <20180612112959.254ae994@bbrezillon> In-Reply-To: <2538157.EbBc2oB3DW@blindfold> References: <5461eb1c-597a-e100-c325-b0fed0eb0d10@linutronix.de> <20180605093627.03ee6a95@bbrezillon> <2538157.EbBc2oB3DW@blindfold> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Tue, 12 Jun 2018 11:21:03 +0200 Richard Weinberger wrote: > Am Dienstag, 5. Juni 2018, 09:36:27 CEST schrieb Boris Brezillon: > > On Tue, 5 Jun 2018 10:43:49 +0900 > > Masahiro Yamada wrote: > > > > > 2018-06-05 7:01 GMT+09:00 Richard Weinberger : > > > > 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? I prefer the solution where we properly describe all the clks that are needed by the NAND controller in the DT + the workaround to still support existing DTs. > > We could also just fix the dts file. > e.g. > commit c0097d0b498bdefb427c352d615432e97f1998aa > Author: Richard Weinberger > 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 > > 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 >