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

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Marek Vasut <marex@denx.de>
Cc: Dinh Nguyen <dinguyen@kernel.org>,
	Masahiro Yamada <masahiroy@kernel.org>,
	Boris Brezillon <boris.brezillon@collabora.com>,
	linux-mtd <linux-mtd@lists.infradead.org>,
	Tim Sander <tim@krieglstein.org>
Subject: Re: [PATCH] Revert "mtd: rawnand: denali: get ->setup_data_interface() working again"
Date: Wed, 11 Mar 2020 14:08:07 +0100	[thread overview]
Message-ID: <20200311140807.6f56baf3@xps13> (raw)
In-Reply-To: <d4b4abf4-1af9-d57c-5b93-2d56a5dc456b@denx.de>

Hi Marek,

Marek Vasut <marex@denx.de> wrote on Wed, 11 Mar 2020 13:52:30 +0100:

> On 3/9/20 11:27 AM, Masahiro Yamada wrote:
> > Hi.  
> 
> Hi,
> 
> [...]
> 
> >>>> See attached patch, with which (without this revert) you get this:
> >>>> denali->reg + TWHR2_AND_WE_2_RE = 0x00001414 -> 0x0000143f
> >>>> denali->reg + TCWAW_AND_ADDR_2_DATA = 0x0000143f -> 0x00001432
> >>>> denali->reg + RE_2_WE = 0x00000014 -> 0x00000019
> >>>> denali->reg + ACC_CLKS = 0x00000004 -> 0x00000005
> >>>> denali->reg + RDWR_EN_LO_CNT = 0x00000002 -> 0x00000009
> >>>> denali->reg + RDWR_EN_HI_CNT = 0x00000002 -> 0x00000004
> >>>> denali->reg + CS_SETUP_CNT = 0x00000001 -> 0x00000008
> >>>> denali->reg + RE_2_RE = 0x00000014 -> 0x00000019  
> >>>
> >>> OK, the left-hand side is probably the timing
> >>> set up by U-Boot.  
> >>
> >> Yep, the timings that work. So now, how do you get to those working
> >> timings using the Linux driver ?  
> > 
> > 
> > How about
> > 0001-denali-more-complicated-calculation-for-timings.patch
> > 
> > + following ?
> > 
> > diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
> > index b0482108a127..ea38aa42873e 100644
> > --- a/drivers/mtd/nand/raw/denali.c
> > +++ b/drivers/mtd/nand/raw/denali.c
> > @@ -860,9 +860,9 @@ static int denali_setup_data_interface(struct
> > nand_chip *chip, int chipnr,
> > 
> >         /*
> >          * Determine the minimum of acc_clks to meet the data setup timing.
> > -        * (one additional clock cycle just in case)
> > +        * (two additional clock cycles just in case)
> >          */
> > -       acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x) + 1;
> > +       acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x) + 2;
> > 
> >         /* Determine the minimum of rdwr_en_lo_cnt from RE#/WE# pulse width */
> >         rdwr_en_lo = DIV_ROUND_UP(max(timings->tRP_min, timings->tWP_min), t_x);  
> 
> Like the attached one ?
> 
> That seems to work, but -- the calculated timings differ from the ones
> which are calculated by U-Boot and which were tested to work well.
> That's not good, I would expect both timings to be identical:

There is no such "timings tested to work well". Timings represent
minimum and maximum values for certain operations on the NAND bus, you
can have two different values that will both work in the same
condition. And it is expected that Linux is more clever than U-Boot and
may optimize better the timings depending on the selected mode ([0-5])
(hence the different calls to ->setup_data_interface().

Run a stress test, if it passes, you should be good :)

Thanks,
Miquèl

>  Denali: clk_rate=31250000, clk_x_rate=125000000
>  Denali: tREA=40000
>  Denali: tRHW=200000
>  Denali: tRHZ=200000
>  Denali: tCCS=500000000
>  Denali: tWHR=120000
>  Denali: tADL=400000
>  Denali: tREH=30000
>  Denali: tWH=30000
>  Denali: tRP=50000
>  Denali: tWP=50000
>  Denali: tRC=100000
>  Denali: tWC=100000
>  Denali: tCS=70000
>  Denali: tCEA=100000
>  Denali: acc_clks=8
>  Denali: re_2_we=25
>  Denali: re_2_re=25
>  Denali: we_2_re=63
>  Denali: addr_2_data=50
>  Denali: rdwr_en_hi=4
>  Denali: rdwr_en_lo_hi=13
>  Denali: rdwr_en_lo=9
>  Denali: cs_setup=5
> 
>  denali->reg + TWHR2_AND_WE_2_RE = 0x00001414 -> 0x0000143f
>  denali->reg + TCWAW_AND_ADDR_2_DATA = 0x0000143f -> 0x00001432
>  denali->reg + RE_2_WE = 0x00000014 -> 0x00000019
>  denali->reg + ACC_CLKS = 0x00000004 -> 0x00000008
>  denali->reg + RDWR_EN_LO_CNT = 0x00000002 -> 0x00000009
>  denali->reg + RDWR_EN_HI_CNT = 0x00000002 -> 0x00000004
>  denali->reg + CS_SETUP_CNT = 0x00000001 -> 0x00000005
>  denali->reg + RE_2_RE = 0x00000014 -> 0x00000019
> 
>  denali->reg + TWHR2_AND_WE_2_RE = 0x0000143f -> 0x0000143f
>  denali->reg + TCWAW_AND_ADDR_2_DATA = 0x00001432 -> 0x00001432
>  denali->reg + RE_2_WE = 0x00000019 -> 0x00000019
>  denali->reg + ACC_CLKS = 0x00000008 -> 0x00000008
>  denali->reg + RDWR_EN_LO_CNT = 0x00000009 -> 0x00000009
>  denali->reg + RDWR_EN_HI_CNT = 0x00000004 -> 0x00000004
>  denali->reg + CS_SETUP_CNT = 0x00000005 -> 0x00000005
>  denali->reg + RE_2_RE = 0x00000019 -> 0x00000019
> 
>  denali->reg + TWHR2_AND_WE_2_RE = 0x0000143f -> 0x0000143f
>  denali->reg + TCWAW_AND_ADDR_2_DATA = 0x00001432 -> 0x00001432
>  denali->reg + RE_2_WE = 0x00000019 -> 0x00000019
>  denali->reg + ACC_CLKS = 0x00000008 -> 0x00000008
>  denali->reg + RDWR_EN_LO_CNT = 0x00000009 -> 0x00000009
>  denali->reg + RDWR_EN_HI_CNT = 0x00000004 -> 0x00000004
>  denali->reg + CS_SETUP_CNT = 0x00000005 -> 0x00000005
>  denali->reg + RE_2_RE = 0x00000019 -> 0x00000019
> ...
> 

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