Re: [PATCH] mtd: nand: atmel: Relax tADL_min constraint

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Thu, 24 Aug 2017 21:09:13 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> Hi,
> 
> On Wed, Aug 23, 2017 at 08:45:01PM +0200, Boris Brezillon wrote:
> > Version 4 of the ONFI spec mandates that tADL be at least 400 nanoseconds,
> > but, depending on the master clock rate, 400 ns may not fit in the tADL
> > field of the SMC reg. We need to relax the check and accept the -ERANGE
> > return code.
> > 
> > Note that previous versions of the ONFI spec had a lower tADL_min (100 or
> > 200 ns). It's not clear why this timing constraint got increased but it
> > seems most NANDs are fine with values lower than 400ns, so we should be
> > safe.
> > 
> > Fixes: f9ce2eddf176 ("mtd: nand: atmel: Add ->setup_data_interface() hooks")
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  drivers/mtd/nand/atmel/nand-controller.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/nand/atmel/nand-controller.c b/drivers/mtd/nand/atmel/nand-controller.c
> > index 2c8baa0c2c4e..ceec21bd30c4 100644
> > --- a/drivers/mtd/nand/atmel/nand-controller.c
> > +++ b/drivers/mtd/nand/atmel/nand-controller.c
> > @@ -1364,7 +1364,18 @@ static int atmel_smc_nand_prepare_smcconf(struct atmel_nand *nand,
> >  	ret = atmel_smc_cs_conf_set_timing(smcconf,
> >  					   ATMEL_HSMC_TIMINGS_TADL_SHIFT,
> >  					   ncycles);
> > -	if (ret)
> > +	/*
> > +	 * Version 4 of the ONFI spec mandates that tADL be at least 400
> > +	 * nanoseconds, but, depending on the master clock rate, 400 ns may not
> > +	 * fit in the tADL field of the SMC reg. We need to relax the check and
> > +	 * accept the -ERANGE return code.
> > +	 *
> > +	 * Note that previous versions of the ONFI spec had a lower tADL_min
> > +	 * (100 or 200 ns). It's not clear why this timing constraint got
> > +	 * increased but it seems most NANDs are fine with values lower than
> > +	 * 400ns, so we should be safe.
> > +	 */
> > +	if (ret && ret != -ERANGE)
> >  		return ret;  
> 
> So I take it you're fine with falling back to this case, where you just
> get the "max" (and "max" is not quite 400ns)?

Right, max in this specific case is 71, and AFAIK the maximum master
clock frequency we have on atmel boards is 200MHz (cycle = 5ns), so
we'll actually get 5 * 71 = 355ns. Given that all atmel platforms I
know have at most ONFI 2 compliant NANDs connected on it, and
ONFI 2 says tADL_min should be 200ns, we should be good.

BTW, I think it would be good to handle timing differences between ONFI
versions. Right now I took the most constraining timing among all ONFI
versions and put it in the nand_timings table, but it might be better
to adjust some timings based on chip->onfi_version to avoid problems
like the one I'm fixing here.

> 
>         /*
>          * Let's just put the maximum we can if the requested setting does
>          * not fit in the register field.
>          * We still return -ERANGE in case the caller cares.
>          */
> 
> Could be nice if there was some kind of sanity check still (e.g., don't
> allow 1ns when we requested 1000ns), but I'm not sure what that would
> be.

I can add a min_cycles argument to the atmel_smc_cs_conf_set_timing()
function to let the caller decide what is appropriate.

> 
> Unless I hear screaming, I'll queue this up and send it out within a
> day.

Thanks a lot.