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

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

[Boris Brezillon]

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;
 
 	ncycles = DIV_ROUND_UP(conf->timings.sdr.tAR_min, mckperiodps);
-- 
2.11.0

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

[Quentin Schulz]

Hi Boris,

On 23/08/2017 20:45, Boris Brezillon (by way of Boris Brezillon
<boris.brezillon@free-electrons.com>) 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>
Tested-by: Quentin Schulz <quentin.schulz@free-electrons.com>

Thanks,
Quentin

> ---
>  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;
>  
>  	ncycles = DIV_ROUND_UP(conf->timings.sdr.tAR_min, mckperiodps);
> 

-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Brian Norris]

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
> 

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

[Boris Brezillon]

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.

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

[Brian Norris]

Hi,

On Fri, Aug 25, 2017 at 08:23:06AM +0200, Boris Brezillon wrote:
> On Thu, 24 Aug 2017 21:09:13 -0700
> Brian Norris <computersforpeace@gmail.com> wrote:
> > 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.

I haven't read ONFI specs in a while, but that sounds sorta reasonable.
I don't know why ONFI updates would retroactively change timings
though...

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

Perhaps I'm not thinking through well enough, but I don't know how the
caller would make a reasonable decision about this. It sounds more like
something needed fixed in the ONFI handling, like you mention above. If
the device actually allows 200ns, we shouldn't be passing in a 400ns
specification.

Anyway, I don't think this is an immediate concern, so not worth hacking
up this patch.

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

Pushed to linux-mtd.git.

Brian