[2/7] memory: atmel-ebi: Simplify SMC config code

Commit Message

Boris BREZILLON July 24, 2017, 7:21 p.m. UTC

Hi Alexander,

Le Mon, 24 Jul 2017 11:12:18 +0200,
Alexander Dahl <[e-mail]> a écrit :

> Hello Boris,
> 
> while testing v4.13-rc2 on an at91sam9g20 baed platform I'm coming back 
> to this topic. Meanwhile the whole new SMC and NAND below EBI stuff is 
> in mainline, this TDF bug however is still in there. See below (all 
> quoted code parts from v4.13-rc2):
> 
> Am Donnerstag, 2. März 2017, 13:30:13 schrieb Boris Brezillon:
> > Alexander Dahl <[e-mail]> wrote:  
> > > #define ATMEL_SMC_MODE_TDF(x)           (((x) - 1) << 16)  
> 
> […]
> 
> > > The hardware manual (AT91SAM9G20) says values from 0 to 15 (4bit,
> > > 0x0 to 0xF) are possible and I guess the goal is to set it to a
> > > value corresponding to the value in ns from the dts or to 15 if
> > > it's greater (or -EINVAL in the new version).
> > > 
> > > However how can one set it to zero? Put in zero to the div you get
> > > zero for ncycles or val and that goes as x into (((x) - 1) << 16)
> > > which results in 0xF ending up as TDF_CYCLES in the mode register,
> > > right?  
> > 
> > Indeed.
> >   
> > > I can of course set a slightly greater value, which ends up in a
> > > calculated register value of zero, but that seems more a hack to me
> > > and is not obvious if I just look at the DTS.  
> > 
> > No, we should fix the bug.
> >   
> > > If I'm right this might be topic of another bugfix patch, or should
> > > it be done right in a v2 of this one?  
> > 
> > It should be done right in a v2. Something like:
> > 
> > 		if (ncycles < ATMEL_SMC_MODE_TDF_MIN)
> > 			ncycles = ATMEL_SMC_MODE_TDF_MIN;
> > 
> > with
> > 
> > #define ATMEL_SMC_MODE_TDF_MIN 1  
> 
> I checked the SAMA5D3x datasheet today and it has the same mode register 
> layout regarding the TDF parts. So allowed are register values from 0 to 
> 15 ending up in 0 to 15 clock cycles of Data Float Time.
> 
> The code in include/linux/mfd/syscon/atmel-smc.h is this:
> 
> #define ATMEL_SMC_MODE_TDF_MASK			GENMASK(19, 16)
> #define ATMEL_SMC_MODE_TDF(x)			(((x) - 1) << 16)
> #define ATMEL_SMC_MODE_TDF_MAX			16
> #define ATMEL_SMC_MODE_TDF_MIN			1
> #define ATMEL_SMC_MODE_TDFMODE_OPTIMIZED	BIT(20)
> 
> This ATMEL_SMC_MODE_TDF() is used in drivers/memory/atmel-ebi.c to setup 
> the external memory interface with timings from dts. A line there inside 
> an ebi node may look like this (see for example 
> arch/arm/boot/dts/sama5d3xcm.dtsi):
> 
> atmel,smc-tdf-ns = <0>;
> 
> The value is expected in nanoseconds and I would expect a direct mapping 
> from 0ns to a register value of 0. This is not the case in code 
> (drivers/memory/atmel-ebi.c):
> 
> 		if (ncycles > ATMEL_SMC_MODE_TDF_MAX ||
> 		    ncycles < ATMEL_SMC_MODE_TDF_MIN) {
> 			ret = -EINVAL;
> 			goto out;
> 		}
> 
> ATMEL_SMC_MODE_TDF_MIN is 1, so a possible 0 value from dts is not 
> allowed in here and atmel_ebi_xslate_smc_timings() fails. In fact to get 
> a register value of 0 for 0 TDF clock cycles I would have to set e.g. 
> 8ns in dts. So this didn't make it in some v2 and is still broken.

Yep, sorry about that.

> 
> I could fix this and provide a patch, but I'm not sure about the second 
> place where ATMEL_SMC_MODE_TDF() is used which is the NAND driver in 
> drivers/mtd/nand/atmel/nand-controller.c. I'm not familiar enough with 
> NAND to judge if this "min = 1, max = 16, decrease by 1 for applying to 
> register" approach is needed there. I would say no, because it also is a 
> counterintuitive offset, but I would prefer a explanation why the code 
> is like this, before touching and breaking anything. ;-)

There is a good reason for this "- 1": the doc says the exact number of
tDF cycles is TDF_CYCLES + 1. When you are expressing timings in ns it does
matter, because you don't want to wait more than necessary. Say the master
clk period is X ns and you want a tDF of X ns. If you divide tDF_ns by the
clk period you get one, and you only want to wait 1 cycle, not two.

The NAND driver seems to do the right thing already [1].

Below is my suggestion below to fix the problem. Did you have something else
in mind? In any case, can you send a patch to fix it (either using my
suggestion or something else if you prefer).

Regards,

Boris


[1]http://elixir.free-electrons.com/linux/v4.13-rc1/source/drivers/mtd/nand/atmel/nand-controller.c#L1309

--->8---

Comments

Alexander Dahl July 25, 2017, 11:43 a.m. UTC | #1

Hello Boris,

Am Montag, 24. Juli 2017, 21:21:09 schrieb Boris Brezillon:
> There is a good reason for this "- 1": the doc says the exact number
> of tDF cycles is TDF_CYCLES + 1. When you are expressing timings in
> ns it does matter, because you don't want to wait more than
> necessary. Say the master clk period is X ns and you want a tDF of X
> ns. If you divide tDF_ns by the clk period you get one, and you only
> want to wait 1 cycle, not two.

This makes sense. I tried several values through the same algorithm with 
a small test program and it gives the expected results. Thanks for the 
explanation. :-)

> The NAND driver seems to do the right thing already [1].
> 
> Below is my suggestion below to fix the problem. Did you have
> something else in mind? In any case, can you send a patch to fix it
> (either using my suggestion or something else if you prefer).

I prepared a small patch series together with two other small fixes for 
the atmel ebi driver and will send it to the list in a minute.

Greets
Alex

Patch

diff --git a/drivers/memory/atmel-ebi.c b/drivers/memory/atmel-ebi.c
index 99e644cda4d1..d94604590d02 100644
--- a/drivers/memory/atmel-ebi.c
+++ b/drivers/memory/atmel-ebi.c
@@ -120,12 +120,14 @@  static int atmel_ebi_xslate_smc_timings(struct atmel_ebi_dev *ebid,
        if (!ret) {
                required = true;
                ncycles = DIV_ROUND_UP(val, clk_period_ns);
-               if (ncycles > ATMEL_SMC_MODE_TDF_MAX ||
-                   ncycles < ATMEL_SMC_MODE_TDF_MIN) {
+               if (ncycles > ATMEL_SMC_MODE_TDF_MAX) {
                        ret = -EINVAL;
                        goto out;
                }
 
+               if (ncycles < ATMEL_SMC_MODE_TDF_MIN)
+                       ncycles = ATMEL_SMC_MODE_TDF_MIN;
+
                smcconf->mode |= ATMEL_SMC_MODE_TDF(ncycles);
        }