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

[Brian Norris <computersforpeace@gmail.com>]

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)?

        /*
         * 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.

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

Brian

>  	ncycles = DIV_ROUND_UP(conf->timings.sdr.tAR_min, mckperiodps);
> -- 
> 2.11.0
>