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

[Masahiro Yamada <masahiroy@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5483 bytes --]

Hi,



On Thu, Feb 13, 2020 at 2:45 AM Marek Vasut <marex@denx.de> wrote:
>
> On 2/12/20 5:56 PM, Masahiro Yamada wrote:
> > Hi.
>
> Hi,
>
> [...]
>
> >>>>>>>>>>> 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."
>
> That sounds like some root clock. You can read the entire documentation
> for the SoCFPGA CV/AV here:
> http://www.altera.com/literature/hb/cyclone-v/cv_5v4.pdf
> See section 3 , look for 'nand' there. Note that NAND can be supplied
> from at least two different PLLs -- main and peripheral.
>
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2018-July/592702.html
> >
> >
> >
> > Maybe, you are using a brand-new,
> > different type of SOCFPGA?
>
> Cyclone V and Arria V , so no.
>
> >>> 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@mail.gmail.com/
> >>>
> >>> 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.
>
> Why would misconfiguring the IP be worth trying ?



There is no change around the ->setup_data_interface() hook
after v4.19
The only difference I could think of is the clock frequency.

But, it is OK if you do not want to test it.

And you are confident.

So, let's suspect the ->setup_data_interface() hook.


If possible, can you provide the dump of
the attached debug code?




>
> >>> 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?
>
> I think Boris / Miquel mentioned that above already.
>
> > 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.
>
> They are non-zero, 31.25 MHz and 125 MHz respectively.
>
> > 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?
>
> I don't know ? The calculated timings are apparently not working.
>
> > 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);
>
> 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().




-- 
Best Regards
Masahiro Yamada

[-- Attachment #2: 0001-denali-dump-timing-parameters.patch --]
[-- Type: text/x-patch, Size: 4446 bytes --]

From 8734c5fd3f5917b765bd64b1d78d59bbbc22039e Mon Sep 17 00:00:00 2001
From: Masahiro Yamada <masahiroy@kernel.org>
Date: Mon, 17 Feb 2020 16:48:06 +0900
Subject: [PATCH] denali: dump timing parameters

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---
 drivers/mtd/nand/raw/denali.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
index fafd0a0aa8e2..c332ca3cba94 100644
--- a/drivers/mtd/nand/raw/denali.c
+++ b/drivers/mtd/nand/raw/denali.c
@@ -794,6 +794,8 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 	if (chipnr == NAND_DATA_IFACE_CHECK_ONLY)
 		return 0;
 
+	printk("Denali: clk_rate=%ld, clk_x_rate=%ld\n", denali->clk_rate, denali->clk_x_rate);
+
 	sel = &to_denali_chip(chip)->sels[chipnr];
 
 	/* tREA -> ACC_CLKS */
@@ -805,10 +807,16 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 	tmp |= FIELD_PREP(ACC_CLKS__VALUE, acc_clks);
 	sel->acc_clks = tmp;
 
+	printk("Denali: tREA=%d\n", timings->tREA_max);
+	printk("Denali: acc_clks=%d\n", acc_clks);
+
 	/* tRWH -> RE_2_WE */
 	re_2_we = DIV_ROUND_UP(timings->tRHW_min, t_x);
 	re_2_we = min_t(int, re_2_we, RE_2_WE__VALUE);
 
+	printk("Denali: tRHW=%d\n", timings->tRHW_min);
+	printk("Denali: re_2_we=%d\n", re_2_we);
+
 	tmp = ioread32(denali->reg + RE_2_WE);
 	tmp &= ~RE_2_WE__VALUE;
 	tmp |= FIELD_PREP(RE_2_WE__VALUE, re_2_we);
@@ -818,6 +826,9 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 	re_2_re = DIV_ROUND_UP(timings->tRHZ_max, t_x);
 	re_2_re = min_t(int, re_2_re, RE_2_RE__VALUE);
 
+	printk("Denali: tRHZ=%d\n", timings->tRHZ_max);
+	printk("Denali: re_2_re=%d\n", re_2_re);
+
 	tmp = ioread32(denali->reg + RE_2_RE);
 	tmp &= ~RE_2_RE__VALUE;
 	tmp |= FIELD_PREP(RE_2_RE__VALUE, re_2_re);
@@ -832,6 +843,10 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 	we_2_re = DIV_ROUND_UP(max(timings->tCCS_min, timings->tWHR_min), t_x);
 	we_2_re = min_t(int, we_2_re, TWHR2_AND_WE_2_RE__WE_2_RE);
 
+	printk("Denali: tCCS=%d\n", timings->tCCS_min);
+	printk("Denali: tWHR=%d\n", timings->tWHR_min);
+	printk("Denali: we_2_re=%d\n", we_2_re);
+
 	tmp = ioread32(denali->reg + TWHR2_AND_WE_2_RE);
 	tmp &= ~TWHR2_AND_WE_2_RE__WE_2_RE;
 	tmp |= FIELD_PREP(TWHR2_AND_WE_2_RE__WE_2_RE, we_2_re);
@@ -847,6 +862,9 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 	addr_2_data = DIV_ROUND_UP(timings->tADL_min, t_x);
 	addr_2_data = min_t(int, addr_2_data, addr_2_data_mask);
 
+	printk("Denali: tADL=%d\n", timings->tADL_min);
+	printk("Denali: addr_2_data=%d\n", addr_2_data);
+
 	tmp = ioread32(denali->reg + TCWAW_AND_ADDR_2_DATA);
 	tmp &= ~TCWAW_AND_ADDR_2_DATA__ADDR_2_DATA;
 	tmp |= FIELD_PREP(TCWAW_AND_ADDR_2_DATA__ADDR_2_DATA, addr_2_data);
@@ -857,6 +875,10 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 				  t_x);
 	rdwr_en_hi = min_t(int, rdwr_en_hi, RDWR_EN_HI_CNT__VALUE);
 
+	printk("Denali: tREH=%d\n", timings->tREH_min);
+	printk("Denali: tWH=%d\n", timings->tWH_min);
+	printk("Denali: rdwr_en_hi=%d\n", rdwr_en_hi);
+
 	tmp = ioread32(denali->reg + RDWR_EN_HI_CNT);
 	tmp &= ~RDWR_EN_HI_CNT__VALUE;
 	tmp |= FIELD_PREP(RDWR_EN_HI_CNT__VALUE, rdwr_en_hi);
@@ -870,6 +892,13 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 	rdwr_en_lo = max(rdwr_en_lo, rdwr_en_lo_hi - rdwr_en_hi);
 	rdwr_en_lo = min_t(int, rdwr_en_lo, RDWR_EN_LO_CNT__VALUE);
 
+	printk("Denali: tRP=%d\n", timings->tRP_min);
+	printk("Denali: tWP=%d\n", timings->tWP_min);
+	printk("Denali: tRC=%d\n", timings->tRC_min);
+	printk("Denali: tWC=%d\n", timings->tWC_min);
+	printk("Denali: rdwr_en_lo_hi=%d\n", rdwr_en_lo_hi);
+	printk("Denali: rdwr_en_lo=%d\n", rdwr_en_lo);
+
 	tmp = ioread32(denali->reg + RDWR_EN_LO_CNT);
 	tmp &= ~RDWR_EN_LO_CNT__VALUE;
 	tmp |= FIELD_PREP(RDWR_EN_LO_CNT__VALUE, rdwr_en_lo);
@@ -881,6 +910,10 @@ static int denali_setup_data_interface(struct nand_chip *chip, int chipnr,
 			0);
 	cs_setup = min_t(int, cs_setup, CS_SETUP_CNT__VALUE);
 
+	printk("Denali: tCS=%d\n", timings->tCS_min);
+	printk("Denali: tCEA=%d\n", timings->tCEA_max);
+	printk("Denali: cs_setup=%d\n", cs_setup);
+
 	tmp = ioread32(denali->reg + CS_SETUP_CNT);
 	tmp &= ~CS_SETUP_CNT__VALUE;
 	tmp |= FIELD_PREP(CS_SETUP_CNT__VALUE, cs_setup);
-- 
2.17.1


[-- Attachment #3: Type: text/plain, Size: 144 bytes --]

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