| Message ID | 20170823184501.7665-1-boris.brezillon@free-electrons.com |
|---|---|
| State | Accepted |
| Delegated to: | Brian Norris |
| Headers | show |
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); >
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 >
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.
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
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);
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(-)