[PATCH 1/4] of: Add device tree bindings for Evatronix

[PATCH 1/4] of: Add device tree bindings for Evatronix

[Ricard Wanderlof]

Devicetree bindings for the driver for the Evatronix NANDFLASH-CTRL NAND flash
controller IP.  This controller is used in the Axis ARTPEC-6 SoC.

The driver supports BCH ECC using the controller's hardware, but there is
also an option to use software BCH ECC. However, the ECC layouts are not
compatible so it's not possible to mix them. The main advantage to using
software ECC is that there are more OOB bytes free, as the hardware is
slightly wasteful on OOB space.

BCH ECC from 4 to 32 bits over 256, 512 or 1024 byte ECC blocks is supported.

Only large-page flash chips are supported, using 4 or 5 address cycles.

Signed-off-by: Ricard Wanderlof <ricardw@axis.com>
---
 .../devicetree/bindings/mtd/evatronix-nand.txt     |   44 ++++++++++++++++++++
 .../devicetree/bindings/vendor-prefixes.txt        |    1 +
 2 files changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/evatronix-nand.txt

diff --git a/Documentation/devicetree/bindings/mtd/evatronix-nand.txt b/Documentation/devicetree/bindings/mtd/evatronix-nand.txt
new file mode 100644
index 0000000..7ceb95a
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/evatronix-nand.txt
@@ -0,0 +1,44 @@
+Evatronix NANDFLASH-CTRL NAND flash controller
+
+Required properties:
+- compatible : "evatronix,nandflash-ctrl"
+- reg : specify bus address and register area size.
+- interrupts : controller interrupt number and irq type.
+- nand-ecc-mode : See nand.txt. Supported values "hw", "soft".
+- nand-ecc-algo : See nand.txt. Supported value "bch".
+- nand-ecc-strength : See nand.txt. Supported values: 4, 8, 16, 24, 32.
+- nand-ecc-step-size : See nand.txt. Supported values: 256, 512, 1024.
+
+Optional properties:
+- nand-on-flash-bbt: See nand.txt.
+- #address-cells, #size-cells: See partition.txt.
+- evatronix,use-bank-select : Use controller bank select function to access
+			      multiple chips, rather than chip enable.
+- evatronix,rb-wired-and: Assume ready/busy signal from all flash chips are
+			  connected using a wired-AND topology rather than
+			  individually.
+- evatronix,timings: Seven 32-bit values for initializing the TIME_SEQ_0,
+		     TIME_SEQ_1, TIMINGS_ASYN, TIME_GEN_SEQ_0, TIME_GEN_SEQ_1,
+		     TIME_GEN_SEQ_2 and TIME_GEN_SEQ_3 registers, respectively.
+
+Example:
+
+nand: nand@f801e000 {
+	compatible = "evatronix,nandflash-ctrl";
+	#address-cells = <1>;
+	#size-cells = <1>;
+	reg = <0xf801e000 0x0200>;
+	interrupts = <0 139 IRQ_TYPE_LEVEL_HIGH>;
+	/* ONFi mode 0 timing, assuming 100 MHz clock. */
+	/* Order is TIME_SEQ_0, TIME_SEQ_1, TIMINGS_ASYN,
+	 * TIME_GEN_SEQ_0, _1, _2, _3 */
+	evatronix,timings = <0x0d151533 0x000b0515 0x00000046
+			     0x00150000 0x00000000 0x00000005 0x00000015>;
+	nand-ecc-mode = "hw";
+	nand-ecc-algo = "bch";
+	nand-on-flash-bbt;
+	nand-ecc-strength = <8>;
+	nand-ecc-step-size = <512>;
+	evatronix,use-bank-select;
+	evatronix,rb-wired-and;
+};
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 86740d4..4018162 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -83,6 +83,7 @@ epson	Seiko Epson Corp.
 est	ESTeem Wireless Modems
 ettus	NI Ettus Research
 eukrea  Eukréa Electromatique
+evatronix	Evatronix SA
 everest	Everest Semiconductor Co. Ltd.
 everspin	Everspin Technologies, Inc.
 excito	Excito
-- 
1.7.10.4

-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Re: [PATCH 1/4] of: Add device tree bindings for Evatronix

[Boris Brezillon]

Hi Ricard,

On Thu, 2 Jun 2016 09:47:18 +0200
Ricard Wanderlof <ricard.wanderlof@axis.com> wrote:

> Devicetree bindings for the driver for the Evatronix NANDFLASH-CTRL NAND flash
> controller IP.  This controller is used in the Axis ARTPEC-6 SoC.
> 
> The driver supports BCH ECC using the controller's hardware, but there is
> also an option to use software BCH ECC. However, the ECC layouts are not
> compatible so it's not possible to mix them. The main advantage to using
> software ECC is that there are more OOB bytes free, as the hardware is
> slightly wasteful on OOB space.
> 
> BCH ECC from 4 to 32 bits over 256, 512 or 1024 byte ECC blocks is supported.
> 
> Only large-page flash chips are supported, using 4 or 5 address cycles.
> 
> Signed-off-by: Ricard Wanderlof <ricardw@axis.com>
> ---
>  .../devicetree/bindings/mtd/evatronix-nand.txt     |   44 ++++++++++++++++++++
>  .../devicetree/bindings/vendor-prefixes.txt        |    1 +
>  2 files changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/evatronix-nand.txt
> 
> diff --git a/Documentation/devicetree/bindings/mtd/evatronix-nand.txt b/Documentation/devicetree/bindings/mtd/evatronix-nand.txt
> new file mode 100644
> index 0000000..7ceb95a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/evatronix-nand.txt
> @@ -0,0 +1,44 @@
> +Evatronix NANDFLASH-CTRL NAND flash controller
> +
> +Required properties:
> +- compatible : "evatronix,nandflash-ctrl"
> +- reg : specify bus address and register area size.
> +- interrupts : controller interrupt number and irq type.
> +- nand-ecc-mode : See nand.txt. Supported values "hw", "soft".
> +- nand-ecc-algo : See nand.txt. Supported value "bch".
> +- nand-ecc-strength : See nand.txt. Supported values: 4, 8, 16, 24, 32.
> +- nand-ecc-step-size : See nand.txt. Supported values: 256, 512, 1024.
> +
> +Optional properties:
> +- nand-on-flash-bbt: See nand.txt.
> +- #address-cells, #size-cells: See partition.txt.
> +- evatronix,use-bank-select : Use controller bank select function to access
> +			      multiple chips, rather than chip enable.

You mean, using a dedicated logic to control the CS lines rather than a
GPIO (controlled by the SW using gpio_set_value())?

It that's the case then I suggest doing it the other way around.
When you want to use a plain GPIO you should define something like:

	cs-gpios = <&pioC X>;

If it's not there the controller should use it's internal logic to
control the CS line.

BTW, you seem to support controlling several CS using the same
controller, which means you'll have to specify which CS the NAND chip
is connected to (see [1]).

> +- evatronix,rb-wired-and: Assume ready/busy signal from all flash chips are
> +			  connected using a wired-AND topology rather than
> +			  individually.

Hm, is that really required? If the R/B line is shared among several
NAND chips, it should be transparent, you just have to specific which
chip is connected to which GPIO (or dedicated R/B pin).

> +- evatronix,timings: Seven 32-bit values for initializing the TIME_SEQ_0,
> +		     TIME_SEQ_1, TIMINGS_ASYN, TIME_GEN_SEQ_0, TIME_GEN_SEQ_1,
> +		     TIME_GEN_SEQ_2 and TIME_GEN_SEQ_3 registers, respectively.


Can this be extracted from the timing mode exposed by the NAND chip.
IMO it shouldn't be defined in the DT.

> +
> +Example:
> +
> +nand: nand@f801e000 {
> +	compatible = "evatronix,nandflash-ctrl";
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	reg = <0xf801e000 0x0200>;
> +	interrupts = <0 139 IRQ_TYPE_LEVEL_HIGH>;
> +	/* ONFi mode 0 timing, assuming 100 MHz clock. */
> +	/* Order is TIME_SEQ_0, TIME_SEQ_1, TIMINGS_ASYN,
> +	 * TIME_GEN_SEQ_0, _1, _2, _3 */
> +	evatronix,timings = <0x0d151533 0x000b0515 0x00000046
> +			     0x00150000 0x00000000 0x00000005 0x00000015>;
> +	nand-ecc-mode = "hw";
> +	nand-ecc-algo = "bch";
> +	nand-on-flash-bbt;
> +	nand-ecc-strength = <8>;
> +	nand-ecc-step-size = <512>;
> +	evatronix,use-bank-select;
> +	evatronix,rb-wired-and;
> +};

We recently added more constraints on the 'NAND controller/NAND chip'
representation in the DT [1].
You should rework your binding (and your code) to match these
constraints, even if you controller is only able to interface with a
single NAND chip.

Thanks,

Boris

[1]https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mtd/nand.txt

> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index 86740d4..4018162 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -83,6 +83,7 @@ epson	Seiko Epson Corp.
>  est	ESTeem Wireless Modems
>  ettus	NI Ettus Research
>  eukrea  Eukréa Electromatique
> +evatronix	Evatronix SA
>  everest	Everest Semiconductor Co. Ltd.
>  everspin	Everspin Technologies, Inc.
>  excito	Excito



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

Re: [PATCH 1/4] of: Add device tree bindings for Evatronix

[Ricard Wanderlof]

Hi Boris,

First of all, thanks for reviewing this.

On Fri, 3 Jun 2016, Boris Brezillon wrote:

> > +
> > +Optional properties:
> > +- nand-on-flash-bbt: See nand.txt.
> > +- #address-cells, #size-cells: See partition.txt.
> > +- evatronix,use-bank-select : Use controller bank select function to access
> > +			      multiple chips, rather than chip enable.
> 
> You mean, using a dedicated logic to control the CS lines rather than a
> GPIO (controlled by the SW using gpio_set_value())?

No. In the SoC on which this has been developed, the nand flash controller 
has dedicated I/O pins for all its functions.

The controller has the concept of 'banks' vs. 'devices'. Essentially, a 
'bank' is a group of multiple chips of the same geometry etc, and the 
controller can send commands in parallel to all devices in the same bank. 
In contrast, different 'banks' can have completely different devices, but 
all devices must be idle before the controller can switch 'banks'.

The usecase for the former would be to increase performance and the latter 
case would be if there were for instance one small NAND flash for program 
storage and a large one for data.

The IP that the driver was tested on has two available chip selects, and 
the DT property controls whether they refer to two devices in the same 
bank vs. two separate banks.

> BTW, you seem to support controlling several CS using the same
> controller, which means you'll have to specify which CS the NAND chip
> is connected to (see [1]).

That is true. The driver as it stands currently only supports a single CS, 
however, the IP as configured in the SoC in question can handle two CS 
lines. My plan is to initially provide support for only the single-CS 
case (there are places in the code where I felt it practical to prepare 
for the multiple-CS case though).

As such, I realize that the previously mentioned property 
(evatronix,use-bank-select) won't really have any meaning in the single-CS 
case, so perhaps it should be omitted at this point completely, and added 
in the future when it is really needed?

> > +- evatronix,rb-wired-and: Assume ready/busy signal from all flash chips are
> > +			  connected using a wired-AND topology rather than
> > +			  individually.
> 
> Hm, is that really required? If the R/B line is shared among several
> NAND chips, it should be transparent, you just have to specific which
> chip is connected to which GPIO (or dedicated R/B pin).

For the SoC in question, the wired-and vs individual R/B lines was a 
choice made during the chip design, which is why I exported this choice as 
a DT property, as it reflects a hardware choice done outside the IP but 
inside the SoC.

I didn't really want to have the added flexibility (and complexity) of 
being able to use any R/B line for any connected flash chip. It seems an 
unnecessary complication for the driver without much gain.

But it would certainly be doable, as the R/B stuff is handled completely 
in software. One would still need a new property that the child nand nodes 
can use to select the R/B line (e.g. evatronix,rb-line). (In the 
configuration we have, as with the CS lines, the R/B line has its own 
dedicated pin, so it's not a GPIO). So it would still require a DT 
property to manage it.

Again, this property doesn't really have any meaning in the single-CS 
case, so should I omit it completely?

> > +- evatronix,timings: Seven 32-bit values for initializing the TIME_SEQ_0,
> > +		     TIME_SEQ_1, TIMINGS_ASYN, TIME_GEN_SEQ_0, TIME_GEN_SEQ_1,
> > +		     TIME_GEN_SEQ_2 and TIME_GEN_SEQ_3 registers, respectively.
> 
> 
> Can this be extracted from the timing mode exposed by the NAND chip.
> IMO it shouldn't be defined in the DT.

I agree, Ideally one would like to read timing parameters from the NAND 
chip (ONFi or JEDEC parameter pages) and determine all the controller IP 
timings from that. There are a couple of reasons I haven't gone that 
route.

Mainly, the reason is that the part of the timing settings are influenced 
by delays within the SoC which are not part of the controller itself, thus 
they cannot be part of the driver code and must come from somewhere else.

The other reason is that the resulting calculations would be rather 
complex to express, given the general case of different controller 
operating frequencies, different types of parameter pages (ONFi vs. 
JEDEC), and a number of settable timing modes (ONFi mode 0, 1, 2, ...). It 
would be a nightmare to test and probably even worse to make future 
changes. At some point, the resulting register values will have to be 
analyzed manually anyway to verify them, checking them against the timings 
for the nand flash chips in question, so one might as well put those 
values into the configuration to start with.

One route to go would be to have a number of 'timings' parameters in the 
.dtsi file for the SoC in question, where the register values are defined 
for different timing modes. The board (.dts) file could then either select 
one of the .dtsi-defined timing modes, or it could be selected 
automatically for those cases where it would be possible (i.e. ONFi timing 
modes).

> > +	/* ONFi mode 0 timing, assuming 100 MHz clock. */
> > +	/* Order is TIME_SEQ_0, TIME_SEQ_1, TIMINGS_ASYN,
> > +	 * TIME_GEN_SEQ_0, _1, _2, _3 */
> > +	evatronix,timings = <0x0d151533 0x000b0515 0x00000046
> > +			     0x00150000 0x00000000 0x00000005 0x00000015>;
> > +	nand-ecc-mode = "hw";
> > +	nand-ecc-algo = "bch";
> > +	nand-on-flash-bbt;
> > +	nand-ecc-strength = <8>;
> > +	nand-ecc-step-size = <512>;
> > +	evatronix,use-bank-select;
> > +	evatronix,rb-wired-and;
> > +};
> 
> We recently added more constraints on the 'NAND controller/NAND chip'
> representation in the DT [1].
> You should rework your binding (and your code) to match these
> constraints, even if you controller is only able to interface with a
> single NAND chip.

Just so I'm clear on this: what you're referring to here is that the 
generic mtd properties (nand-*) are now per chip rather than per 
controller? I will have to rework this.

/Ricard
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Re: [PATCH 1/4] of: Add device tree bindings for Evatronix

[Boris Brezillon]

On Tue, 7 Jun 2016 17:01:41 +0200
Ricard Wanderlof <ricard.wanderlof@axis.com> wrote:

> Hi Boris,
> 
> First of all, thanks for reviewing this.
> 
> On Fri, 3 Jun 2016, Boris Brezillon wrote:
> 
> > > +
> > > +Optional properties:
> > > +- nand-on-flash-bbt: See nand.txt.
> > > +- #address-cells, #size-cells: See partition.txt.
> > > +- evatronix,use-bank-select : Use controller bank select function to access
> > > +			      multiple chips, rather than chip enable.  
> > 
> > You mean, using a dedicated logic to control the CS lines rather than a
> > GPIO (controlled by the SW using gpio_set_value())?  
> 
> No. In the SoC on which this has been developed, the nand flash controller 
> has dedicated I/O pins for all its functions.
> 
> The controller has the concept of 'banks' vs. 'devices'. Essentially, a 
> 'bank' is a group of multiple chips of the same geometry etc, and the 
> controller can send commands in parallel to all devices in the same bank. 
> In contrast, different 'banks' can have completely different devices, but 
> all devices must be idle before the controller can switch 'banks'.
> 
> The usecase for the former would be to increase performance and the latter 
> case would be if there were for instance one small NAND flash for program 
> storage and a large one for data.
> 
> The IP that the driver was tested on has two available chip selects, and 
> the DT property controls whether they refer to two devices in the same 
> bank vs. two separate banks.

Still don't see how it can be used in real world (or maybe I don't
understand how it works).

Say the CS are both selected at the same time, this means you'll send
the same set of data to the 2 chips, right. I see two use cases for
this:
1/ You have 2 chips connected to the same data bus, CS, RB and CLE/ALE
   lines and you want to mirror data written on chip0 on chip1
2/ You have 2 chips connected to the same CS, RB, CLE/ALE lines, but
   chip0 is using the lower 8bits of the bus, and chip1 is using the
   remaining 8bits. This way you fake an 16bits bus

Both cases are unsafe, because of NAND unreliability. Say you're
writing on a block in chip0 this block in chip1 is bad. One of your
write will succeed, the other one will fail.

Could you give a real use case where this 'bank-select' method is
really usefull (and safe)?
If you can't, then drop this mode for now, and always operate in
'chip-enable' mode.

> 
> > BTW, you seem to support controlling several CS using the same
> > controller, which means you'll have to specify which CS the NAND chip
> > is connected to (see [1]).  
> 
> That is true. The driver as it stands currently only supports a single CS, 
> however, the IP as configured in the SoC in question can handle two CS 
> lines. My plan is to initially provide support for only the single-CS 
> case (there are places in the code where I felt it practical to prepare 
> for the multiple-CS case though).
> 
> As such, I realize that the previously mentioned property 
> (evatronix,use-bank-select) won't really have any meaning in the single-CS 
> case, so perhaps it should be omitted at this point completely, and added 
> in the future when it is really needed?

Yes, but I'd still like to see a proper NAND controller/NAND chip
separation in you binding.

> 
> > > +- evatronix,rb-wired-and: Assume ready/busy signal from all flash chips are
> > > +			  connected using a wired-AND topology rather than
> > > +			  individually.  
> > 
> > Hm, is that really required? If the R/B line is shared among several
> > NAND chips, it should be transparent, you just have to specific which
> > chip is connected to which GPIO (or dedicated R/B pin).  
> 
> For the SoC in question, the wired-and vs individual R/B lines was a 
> choice made during the chip design, which is why I exported this choice as 
> a DT property, as it reflects a hardware choice done outside the IP but 
> inside the SoC.


Okay, maybe it's worth keeping this property if the AND logic is
integrated in the SoC.

> 
> I didn't really want to have the added flexibility (and complexity) of 
> being able to use any R/B line for any connected flash chip. It seems an 
> unnecessary complication for the driver without much gain.

Not really, at least if you properly separate the chip and controller
objects it's quite easy to deal with, and I'll ask you to do this clean
separation anyway (even if you say you only have a single chip per
controller) :P.

> 
> But it would certainly be doable, as the R/B stuff is handled completely 
> in software. One would still need a new property that the child nand nodes 
> can use to select the R/B line (e.g. evatronix,rb-line). (In the 
> configuration we have, as with the CS lines, the R/B line has its own 
> dedicated pin, so it's not a GPIO). So it would still require a DT 
> property to manage it.

Yep. The sunxi_nand driver is using allwinner,rb = <X>.

> 
> Again, this property doesn't really have any meaning in the single-CS 
> case, so should I omit it completely?

Keep in mind that you're defining a DT binding, and this binding is
submitting to the 'stable ABI' rule, so you shouldn't think in term of
what your driver supports now, but what you're IP is capable of, and
it's definitely capable of handling several R/B pins.

> 
> > > +- evatronix,timings: Seven 32-bit values for initializing the TIME_SEQ_0,
> > > +		     TIME_SEQ_1, TIMINGS_ASYN, TIME_GEN_SEQ_0, TIME_GEN_SEQ_1,
> > > +		     TIME_GEN_SEQ_2 and TIME_GEN_SEQ_3 registers, respectively.  
> > 
> > 
> > Can this be extracted from the timing mode exposed by the NAND chip.
> > IMO it shouldn't be defined in the DT.  
> 
> I agree, Ideally one would like to read timing parameters from the NAND 
> chip (ONFi or JEDEC parameter pages) and determine all the controller IP 
> timings from that. There are a couple of reasons I haven't gone that 
> route.
> 
> Mainly, the reason is that the part of the timing settings are influenced 
> by delays within the SoC which are not part of the controller itself, thus 
> they cannot be part of the driver code and must come from somewhere else.

Then you should have a way to retrieve these informations (proper clk
drivers, specific bus drivers, or any other means).

> 
> The other reason is that the resulting calculations would be rather 
> complex to express, given the general case of different controller 
> operating frequencies, different types of parameter pages (ONFi vs. 
> JEDEC), and a number of settable timing modes (ONFi mode 0, 1, 2, ...). It 
> would be a nightmare to test and probably even worse to make future 
> changes. At some point, the resulting register values will have to be 
> analyzed manually anyway to verify them, checking them against the timings 
> for the nand flash chips in question, so one might as well put those 
> values into the configuration to start with.

And my answer is no :). Just because it's complicated to extract
these information at the driver level level doesn't mean you should put
it in the DT. That's exactly the opposite: the DT is supposed to encode
the hardware representation, not how you want to configure it.

> 
> One route to go would be to have a number of 'timings' parameters in the 
> .dtsi file for the SoC in question, where the register values are defined 
> for different timing modes. The board (.dts) file could then either select 
> one of the .dtsi-defined timing modes, or it could be selected 
> automatically for those cases where it would be possible (i.e. ONFi timing 
> modes).

Nope, they should not be defined in the DT at all.

How about using a default/safe timing mode for now (ONFI timing mode 0
should work for all NANDs).
The only thing you'll have to do is retrieve the source clk rate and
calculate timing register values accordingly. Or you can even assume
you always have a 100MHz source clk and hardcode it in your driver.

> 
> > > +	/* ONFi mode 0 timing, assuming 100 MHz clock. */
> > > +	/* Order is TIME_SEQ_0, TIME_SEQ_1, TIMINGS_ASYN,
> > > +	 * TIME_GEN_SEQ_0, _1, _2, _3 */
> > > +	evatronix,timings = <0x0d151533 0x000b0515 0x00000046
> > > +			     0x00150000 0x00000000 0x00000005 0x00000015>;
> > > +	nand-ecc-mode = "hw";
> > > +	nand-ecc-algo = "bch";
> > > +	nand-on-flash-bbt;
> > > +	nand-ecc-strength = <8>;
> > > +	nand-ecc-step-size = <512>;
> > > +	evatronix,use-bank-select;
> > > +	evatronix,rb-wired-and;
> > > +};  
> > 
> > We recently added more constraints on the 'NAND controller/NAND chip'
> > representation in the DT [1].
> > You should rework your binding (and your code) to match these
> > constraints, even if you controller is only able to interface with a
> > single NAND chip.  
> 
> Just so I'm clear on this: what you're referring to here is that the 
> generic mtd properties (nand-*) are now per chip rather than per 
> controller? I will have to rework this.

Yes, and your NAND chip should be a sub-node of the NAND controller
node (and not mixed together as you've done here).


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

Re: [PATCH 1/4] of: Add device tree bindings for Evatronix

[Ricard Wanderlof]

On Wed, 8 Jun 2016, Boris Brezillon wrote:

> > > > +Optional properties:
> > > > +- nand-on-flash-bbt: See nand.txt.
> > > > +- #address-cells, #size-cells: See partition.txt.
> > > > +- evatronix,use-bank-select : Use controller bank select function to access
> > > > +			      multiple chips, rather than chip enable.  
> > > 
> > > You mean, using a dedicated logic to control the CS lines rather than a
> > > GPIO (controlled by the SW using gpio_set_value())?  
> > 
> > No. In the SoC on which this has been developed, the nand flash controller 
> > has dedicated I/O pins for all its functions.
> > 
> > The controller has the concept of 'banks' vs. 'devices'. Essentially, a 
> > 'bank' is a group of multiple chips of the same geometry etc, and the 
> > controller can send commands in parallel to all devices in the same bank. 
> > In contrast, different 'banks' can have completely different devices, but 
> > all devices must be idle before the controller can switch 'banks'.
> > 
> > The usecase for the former would be to increase performance and the latter 
> > case would be if there were for instance one small NAND flash for program 
> > storage and a large one for data.
> > 
> > The IP that the driver was tested on has two available chip selects, and 
> > the DT property controls whether they refer to two devices in the same 
> > bank vs. two separate banks.
> 
> Still don't see how it can be used in real world (or maybe I don't
> understand how it works).
> 
> Say the CS are both selected at the same time, this means you'll send
> the same set of data to the 2 chips, right. I see two use cases for
> this:
> 1/ You have 2 chips connected to the same data bus, CS, RB and CLE/ALE
>    lines and you want to mirror data written on chip0 on chip1
> 2/ You have 2 chips connected to the same CS, RB, CLE/ALE lines, but
>    chip0 is using the lower 8bits of the bus, and chip1 is using the
>    remaining 8bits. This way you fake an 16bits bus
> 
> Both cases are unsafe, because of NAND unreliability. Say you're
> writing on a block in chip0 this block in chip1 is bad. One of your
> write will succeed, the other one will fail.
> 
> Could you give a real use case where this 'bank-select' method is
> really usefull (and safe)?
> If you can't, then drop this mode for now, and always operate in
> 'chip-enable' mode.

The use cases as I see are as follows:

a) Two identical chips sharing all but the CS lines, in order to implement 
a seemingly-larger address space. (e.g. two 256 Mbit chips implementing a 
4 GB area). In this case, for certain operations, the controller does not 
have to wait for one device to complete before issuing a command to 
another. I'm not sure how the controller keeps track of the two devices 
though.

b) Two different chips with the same connection, which provide disjunct 
functions, e.g. one (small) flash for program storage and one (large) for 
data.

But your questions have convinced me to leave this part of the driver for 
now, and as you say it can be added in when the need arises.

> > > BTW, you seem to support controlling several CS using the same
> > > controller, which means you'll have to specify which CS the NAND chip
> > > is connected to (see [1]).  
> > 
> > That is true. The driver as it stands currently only supports a single CS, 
> > however, the IP as configured in the SoC in question can handle two CS 
> > lines. My plan is to initially provide support for only the single-CS 
> > case (there are places in the code where I felt it practical to prepare 
> > for the multiple-CS case though).
> > 
> > As such, I realize that the previously mentioned property 
> > (evatronix,use-bank-select) won't really have any meaning in the single-CS 
> > case, so perhaps it should be omitted at this point completely, and added 
> > in the future when it is really needed?
> 
> Yes, but I'd still like to see a proper NAND controller/NAND chip 
> separation in you binding.

Yes, I understand that.

> > > > +- evatronix,rb-wired-and: Assume ready/busy signal from all flash chips are
> > > > +			  connected using a wired-AND topology rather than
> > > > +			  individually.  
> > > 
> > > Hm, is that really required? If the R/B line is shared among several
> > > NAND chips, it should be transparent, you just have to specific which
> > > chip is connected to which GPIO (or dedicated R/B pin).  
> > 
> > For the SoC in question, the wired-and vs individual R/B lines was a 
> > choice made during the chip design, which is why I exported this choice as 
> > a DT property, as it reflects a hardware choice done outside the IP but 
> > inside the SoC.
> 
> Okay, maybe it's worth keeping this property if the AND logic is
> integrated in the SoC.

Ok.

> > I didn't really want to have the added flexibility (and complexity) of 
> > being able to use any R/B line for any connected flash chip. It seems an 
> > unnecessary complication for the driver without much gain.
> 
> Not really, at least if you properly separate the chip and controller
> objects it's quite easy to deal with, and I'll ask you to do this clean
> separation anyway (even if you say you only have a single chip per
> controller) :P.

Yes, the separation of chip and controller is needed, but the R/B line 
flexibility requires an additional mapping functionality within the 
driver. Not rocket science of course, but I can't see much point in it 
(other than to cover up a potential routing mistake done by a PCB 
designer).

> > 
> > Again, this property doesn't really have any meaning in the single-CS 
> > case, so should I omit it completely?
> 
> Keep in mind that you're defining a DT binding, and this binding is
> submitting to the 'stable ABI' rule, so you shouldn't think in term of
> what your driver supports now, but what you're IP is capable of, and
> it's definitely capable of handling several R/B pins.

Yes, I understand about the stable API rule, but at the same time that R/B 
mapping could be added without violating that rule at a later date if 
someone really needed it, couldn't it?

I mean, an IP may be capable of a lot of things, and we don't necessarily 
want to implement them all in the driver to start with and have DT 
propertes for them all do we?

> > > > +- evatronix,timings: Seven 32-bit values for initializing the TIME_SEQ_0,
> > > > +		     TIME_SEQ_1, TIMINGS_ASYN, TIME_GEN_SEQ_0, TIME_GEN_SEQ_1,
> > > > +		     TIME_GEN_SEQ_2 and TIME_GEN_SEQ_3 registers, respectively.  
> > > 
> > > 
> > > Can this be extracted from the timing mode exposed by the NAND chip.
> > > IMO it shouldn't be defined in the DT.  
> > 
> > I agree, Ideally one would like to read timing parameters from the NAND 
> > chip (ONFi or JEDEC parameter pages) and determine all the controller IP 
> > timings from that. There are a couple of reasons I haven't gone that 
> > route.
> > 

I think what I'm worried about is that say we add the functionality to 
take the appropriate parameters and create a set of timing values. We can 
verify those values only under a very limited set of circumstances (a 
certain clock frequency, certain pad delays on a given SoC, limited range 
of flash chips etc). At a later date, the IP is used in a different SoC 
and the original calculation turns out to be wrong in that case. Updating 
the timing calcuation function to work with the original SoC and the new 
one would be a bit of a nightmare I. Since at some point a set of specific 
timing values need to be calculated for a given SoC anyway, we might as 
well use those values as they are.

> And my answer is no :). 

:-)

> Just because it's complicated to extract these information at the driver 
> level level doesn't mean you should put it in the DT. That's exactly the 
> opposite: the DT is supposed to encode the hardware representation, not 
> how you want to configure it.

I would say that the complexity and error-proneness of automatic timing 
calculation outweighs the benefit, but I see I'm fighting a loosing battle 
here...

> How about using a default/safe timing mode for now (ONFI timing mode 0
> should work for all NANDs).
> The only thing you'll have to do is retrieve the source clk rate and
> calculate timing register values accordingly. Or you can even assume
> you always have a 100MHz source clk and hardcode it in your driver.

Yes, that is certainly possible of course, and the driver already has a 
hard-coded default setup for this case.

In that case though the driver could have pre-set setups for other ONFi 
modes, and we could have an _optional_ DT property to select them, the 
reason for that being in order to handle non-ONFi flashes whose timing 
cannot be gleaned from the device itself.

I.e. something like

evatronix,onfi-timing-mode = <2>;

Would that be acceptable?

/Ricard
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Re: [PATCH 1/4] of: Add device tree bindings for Evatronix

[Boris Brezillon]

On Fri, 10 Jun 2016 17:35:24 +0200
Ricard Wanderlof <ricard.wanderlof@axis.com> wrote:

> On Wed, 8 Jun 2016, Boris Brezillon wrote:
> 
> > > > > +Optional properties:
> > > > > +- nand-on-flash-bbt: See nand.txt.
> > > > > +- #address-cells, #size-cells: See partition.txt.
> > > > > +- evatronix,use-bank-select : Use controller bank select function to access
> > > > > +			      multiple chips, rather than chip enable.    
> > > > 
> > > > You mean, using a dedicated logic to control the CS lines rather than a
> > > > GPIO (controlled by the SW using gpio_set_value())?    
> > > 
> > > No. In the SoC on which this has been developed, the nand flash controller 
> > > has dedicated I/O pins for all its functions.
> > > 
> > > The controller has the concept of 'banks' vs. 'devices'. Essentially, a 
> > > 'bank' is a group of multiple chips of the same geometry etc, and the 
> > > controller can send commands in parallel to all devices in the same bank. 
> > > In contrast, different 'banks' can have completely different devices, but 
> > > all devices must be idle before the controller can switch 'banks'.
> > > 
> > > The usecase for the former would be to increase performance and the latter 
> > > case would be if there were for instance one small NAND flash for program 
> > > storage and a large one for data.
> > > 
> > > The IP that the driver was tested on has two available chip selects, and 
> > > the DT property controls whether they refer to two devices in the same 
> > > bank vs. two separate banks.  
> > 
> > Still don't see how it can be used in real world (or maybe I don't
> > understand how it works).
> > 
> > Say the CS are both selected at the same time, this means you'll send
> > the same set of data to the 2 chips, right. I see two use cases for
> > this:
> > 1/ You have 2 chips connected to the same data bus, CS, RB and CLE/ALE
> >    lines and you want to mirror data written on chip0 on chip1
> > 2/ You have 2 chips connected to the same CS, RB, CLE/ALE lines, but
> >    chip0 is using the lower 8bits of the bus, and chip1 is using the
> >    remaining 8bits. This way you fake an 16bits bus
> > 
> > Both cases are unsafe, because of NAND unreliability. Say you're
> > writing on a block in chip0 this block in chip1 is bad. One of your
> > write will succeed, the other one will fail.
> > 
> > Could you give a real use case where this 'bank-select' method is
> > really usefull (and safe)?
> > If you can't, then drop this mode for now, and always operate in
> > 'chip-enable' mode.  
> 
> The use cases as I see are as follows:
> 
> a) Two identical chips sharing all but the CS lines, in order to implement 
> a seemingly-larger address space. (e.g. two 256 Mbit chips implementing a 
> 4 GB area). In this case, for certain operations, the controller does not 
> have to wait for one device to complete before issuing a command to 
> another. I'm not sure how the controller keeps track of the two devices 
> though.

I think it's the chip-enable use case, isn't it?

> 
> b) Two different chips with the same connection, which provide disjunct 
> functions, e.g. one (small) flash for program storage and one (large) for 
> data.

Then they can't share the CS line (or be actived at the same time), at
least it doesn't make any sense to me.

> 
> But your questions have convinced me to leave this part of the driver for 
> now, and as you say it can be added in when the need arises.
> 
> > > > BTW, you seem to support controlling several CS using the same
> > > > controller, which means you'll have to specify which CS the NAND chip
> > > > is connected to (see [1]).    
> > > 
> > > That is true. The driver as it stands currently only supports a single CS, 
> > > however, the IP as configured in the SoC in question can handle two CS 
> > > lines. My plan is to initially provide support for only the single-CS 
> > > case (there are places in the code where I felt it practical to prepare 
> > > for the multiple-CS case though).
> > > 
> > > As such, I realize that the previously mentioned property 
> > > (evatronix,use-bank-select) won't really have any meaning in the single-CS 
> > > case, so perhaps it should be omitted at this point completely, and added 
> > > in the future when it is really needed?  
> > 
> > Yes, but I'd still like to see a proper NAND controller/NAND chip 
> > separation in you binding.  
> 
> Yes, I understand that.
> 
> > > > > +- evatronix,rb-wired-and: Assume ready/busy signal from all flash chips are
> > > > > +			  connected using a wired-AND topology rather than
> > > > > +			  individually.    
> > > > 
> > > > Hm, is that really required? If the R/B line is shared among several
> > > > NAND chips, it should be transparent, you just have to specific which
> > > > chip is connected to which GPIO (or dedicated R/B pin).    
> > > 
> > > For the SoC in question, the wired-and vs individual R/B lines was a 
> > > choice made during the chip design, which is why I exported this choice as 
> > > a DT property, as it reflects a hardware choice done outside the IP but 
> > > inside the SoC.  
> > 
> > Okay, maybe it's worth keeping this property if the AND logic is
> > integrated in the SoC.  
> 
> Ok.
> 
> > > I didn't really want to have the added flexibility (and complexity) of 
> > > being able to use any R/B line for any connected flash chip. It seems an 
> > > unnecessary complication for the driver without much gain.  
> > 
> > Not really, at least if you properly separate the chip and controller
> > objects it's quite easy to deal with, and I'll ask you to do this clean
> > separation anyway (even if you say you only have a single chip per
> > controller) :P.  
> 
> Yes, the separation of chip and controller is needed, but the R/B line 
> flexibility requires an additional mapping functionality within the 
> driver. Not rocket science of course, but I can't see much point in it 
> (other than to cover up a potential routing mistake done by a PCB 
> designer).

You seem to argue on all the minor things I'm asking. Honestly, it
should be hard or error-prone to do it. And let's say you don't support
it in your driver, you should still think about that when designing
your bindings.

> 
> > > 
> > > Again, this property doesn't really have any meaning in the single-CS 
> > > case, so should I omit it completely?  
> > 
> > Keep in mind that you're defining a DT binding, and this binding is
> > submitting to the 'stable ABI' rule, so you shouldn't think in term of
> > what your driver supports now, but what you're IP is capable of, and
> > it's definitely capable of handling several R/B pins.  
> 
> Yes, I understand about the stable API rule, but at the same time that R/B 
> mapping could be added without violating that rule at a later date if 
> someone really needed it, couldn't it?
> 
> I mean, an IP may be capable of a lot of things, and we don't necessarily 
> want to implement them all in the driver to start with and have DT 
> propertes for them all do we?

Hm, you should at least take the capabilities you know about into
account when defining a new binding, and you already know that some
SoCs might decide to expose 2 or more R/B pins, so it should already be
designed this way.

> 
> > > > > +- evatronix,timings: Seven 32-bit values for initializing the TIME_SEQ_0,
> > > > > +		     TIME_SEQ_1, TIMINGS_ASYN, TIME_GEN_SEQ_0, TIME_GEN_SEQ_1,
> > > > > +		     TIME_GEN_SEQ_2 and TIME_GEN_SEQ_3 registers, respectively.    
> > > > 
> > > > 
> > > > Can this be extracted from the timing mode exposed by the NAND chip.
> > > > IMO it shouldn't be defined in the DT.    
> > > 
> > > I agree, Ideally one would like to read timing parameters from the NAND 
> > > chip (ONFi or JEDEC parameter pages) and determine all the controller IP 
> > > timings from that. There are a couple of reasons I haven't gone that 
> > > route.
> > >   
> 
> I think what I'm worried about is that say we add the functionality to 
> take the appropriate parameters and create a set of timing values. We can 
> verify those values only under a very limited set of circumstances (a 
> certain clock frequency, certain pad delays on a given SoC, limited range 
> of flash chips etc). At a later date, the IP is used in a different SoC 
> and the original calculation turns out to be wrong in that case. Updating 
> the timing calcuation function to work with the original SoC and the new 
> one would be a bit of a nightmare I. Since at some point a set of specific 
> timing values need to be calculated for a given SoC anyway, we might as 
> well use those values as they are.
> 
> > And my answer is no :).   
> 
> :-)
> 
> > Just because it's complicated to extract these information at the driver 
> > level level doesn't mean you should put it in the DT. That's exactly the 
> > opposite: the DT is supposed to encode the hardware representation, not 
> > how you want to configure it.  
> 
> I would say that the complexity and error-proneness of automatic timing 
> calculation outweighs the benefit, but I see I'm fighting a loosing battle 
> here...
> 
> > How about using a default/safe timing mode for now (ONFI timing mode 0
> > should work for all NANDs).
> > The only thing you'll have to do is retrieve the source clk rate and
> > calculate timing register values accordingly. Or you can even assume
> > you always have a 100MHz source clk and hardcode it in your driver.  
> 
> Yes, that is certainly possible of course, and the driver already has a 
> hard-coded default setup for this case.
> 
> In that case though the driver could have pre-set setups for other ONFi 
> modes, and we could have an _optional_ DT property to select them, the 
> reason for that being in order to handle non-ONFi flashes whose timing 
> cannot be gleaned from the device itself.
> 
> I.e. something like
> 
> evatronix,onfi-timing-mode = <2>;
> 
> Would that be acceptable?

Actually this has been added to the nand_flash_dev structure, and it's
called onfi_timing_mode_default [1].
If you need a different timing, define a full ID entry for your NAND.

Note that I'm also reworking the NAND detection code to let vendor
specific code initialize a few things [2], which should allow us to get
rid of the full-id entries. This timing mode tweaking could/should be
done at this level.

I'm opposed to the idea of putting information that can be
automatically deduced in the DT. For this specific case, the main
reason is that a board vendor can decide to use different NAND chips
on the same design, and they might not all share the same
capabilities (and you don't want to have a dts for each NAND).

[1]http://lxr.free-electrons.com/source/include/linux/mtd/nand.h#L871
[2]https://lkml.org/lkml/2016/5/27/264


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

Re: [PATCH 1/4] of: Add device tree bindings for Evatronix

[Ricard Wanderlof]

On Fri, 10 Jun 2016, Boris Brezillon wrote:

> > The use cases as I see are as follows:
> > 
> > a) Two identical chips sharing all but the CS lines, in order to implement 
> > a seemingly-larger address space. (e.g. two 256 Mbit chips implementing a 
> > 4 GB area). In this case, for certain operations, the controller does not 
> > have to wait for one device to complete before issuing a command to 
> > another. I'm not sure how the controller keeps track of the two devices 
> > though.
> 
> I think it's the chip-enable use case, isn't it?
> 
> > 
> > b) Two different chips with the same connection, which provide disjunct 
> > functions, e.g. one (small) flash for program storage and one (large) for 
> > data.
> 
> Then they can't share the CS line (or be actived at the same time), at
> least it doesn't make any sense to me.

Sorry if I haven't been clear enough, but in neither case are the CS lines 
shared between the devices. There is still a separate CS for each flash 
chip. The question is how the controller handles the CS lines.

Basically, in the general case, the controller can handle a matrix of nand 
flash chips. There can be a number of banks, each of which can have a 
number of individual CS lines. For the (in this case academic) case of 3 
banks and 4 chip selects per bank, there would be a total of 3 x 4 = 12 CS 
lines.

For the IP configuration the driver was written for, there are only 2 CS 
lines, and we can configure if they are to be viewed by the controller as 
2 CS lines within the same single bank, or 2 separate banks with one CS 
each. This is what the DT property is intended to express. It basically 
translates directly into a register write in the IP.

But, as you are driving at below, the bindings should really cover the 
more general case, for which a simple use-bank-select property doesn't 
really cut it. Since the driver only supports a single CS at the time 
being, I'm really proposing to drop it completely, alternatively we could 
have two: 'banks' and 'devices-per-bank', which reflect what the general 
IP case would be able to handle. For the version of the IP in use these 
would have the permitted values of 1 and 2, with some combinations being 
illegal. Unfortunately, the IP configuration cannot be read out (neither 
the version of it), so it's not possible for the driver to verify the 
DT settings against the actual IP configuration. I don't really know how 
to solve that.

> > > > I didn't really want to have the added flexibility (and complexity) of 
> > > > being able to use any R/B line for any connected flash chip. It seems an 
> > > > unnecessary complication for the driver without much gain.  
> > > 
> > > Not really, at least if you properly separate the chip and controller
> > > objects it's quite easy to deal with, and I'll ask you to do this clean
> > > separation anyway (even if you say you only have a single chip per
> > > controller) :P.  
> > 
> > Yes, the separation of chip and controller is needed, but the R/B line 
> > flexibility requires an additional mapping functionality within the 
> > driver. Not rocket science of course, but I can't see much point in it 
> > (other than to cover up a potential routing mistake done by a PCB 
> > designer).
> 
> You seem to argue on all the minor things I'm asking. Honestly, it
> should be hard or error-prone to do it. And let's say you don't support
> it in your driver, you should still think about that when designing
> your bindings.

Sorry, it's just that over the years I've seen too much code that 
introduces various flexibilities just because someone thought it might be 
'nice to have at some point' or 'because the hardware supports it', but 
which in reality is never used and still must be maintained. Sure, one 
small mapping function is certainly not going to break the bank, far from 
it, but over time the matrix of things that can be configured can grow to 
awkward proportions, and often something thought of as a minor issue can 
turn out to be complex to support in the end. And given a stable API rule, 
it's too late to simplify things once they have been implemented.

Of course, the code should not be written in a way to limit future 
expansion either.

> > I mean, an IP may be capable of a lot of things, and we don't necessarily 
> > want to implement them all in the driver to start with and have DT 
> > propertes for them all do we?
> 
> Hm, you should at least take the capabilities you know about into
> account when defining a new binding, and you already know that some
> SoCs might decide to expose 2 or more R/B pins, so it should already be
> designed this way.

Ok. I'll give it some more thought then.

> I'm opposed to the idea of putting information that can be
> automatically deduced in the DT. For this specific case, the main
> reason is that a board vendor can decide to use different NAND chips
> on the same design, and they might not all share the same
> capabilities (and you don't want to have a dts for each NAND).

Yes, that makes sense of course, but what if someone would want to 
override the automatic settings, for whatever reason, using an optional DT 
property? I can think of several reasons either way, that's why I'm 
asking.

/Ricard
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Re: [PATCH 1/4] of: Add device tree bindings for Evatronix

[Boris Brezillon]

On Fri, 10 Jun 2016 18:46:24 +0200
Ricard Wanderlof <ricard.wanderlof@axis.com> wrote:

> On Fri, 10 Jun 2016, Boris Brezillon wrote:
> 
> > > The use cases as I see are as follows:
> > > 
> > > a) Two identical chips sharing all but the CS lines, in order to implement 
> > > a seemingly-larger address space. (e.g. two 256 Mbit chips implementing a 
> > > 4 GB area). In this case, for certain operations, the controller does not 
> > > have to wait for one device to complete before issuing a command to 
> > > another. I'm not sure how the controller keeps track of the two devices 
> > > though.  
> > 
> > I think it's the chip-enable use case, isn't it?
> >   
> > > 
> > > b) Two different chips with the same connection, which provide disjunct 
> > > functions, e.g. one (small) flash for program storage and one (large) for 
> > > data.  
> > 
> > Then they can't share the CS line (or be actived at the same time), at
> > least it doesn't make any sense to me.  
> 
> Sorry if I haven't been clear enough, but in neither case are the CS lines 
> shared between the devices. There is still a separate CS for each flash 
> chip. The question is how the controller handles the CS lines.
> 
> Basically, in the general case, the controller can handle a matrix of nand 
> flash chips. There can be a number of banks, each of which can have a 
> number of individual CS lines. For the (in this case academic) case of 3 
> banks and 4 chip selects per bank, there would be a total of 3 x 4 = 12 CS 
> lines.
> 
> For the IP configuration the driver was written for, there are only 2 CS 
> lines, and we can configure if they are to be viewed by the controller as 
> 2 CS lines within the same single bank, or 2 separate banks with one CS 
> each. This is what the DT property is intended to express. It basically 
> translates directly into a register write in the IP.

Okay, got it. I guess we should just expose the chip select in a linear
way (3 banks of 4 CS means the controller should support up to 12
chips), unless you really have a way to change the CS pins routing
internally.

> 
> But, as you are driving at below, the bindings should really cover the 
> more general case, for which a simple use-bank-select property doesn't 
> really cut it. Since the driver only supports a single CS at the time 
> being, I'm really proposing to drop it completely, alternatively we could 
> have two: 'banks' and 'devices-per-bank', which reflect what the general 
> IP case would be able to handle. For the version of the IP in use these 
> would have the permitted values of 1 and 2, with some combinations being 
> illegal. Unfortunately, the IP configuration cannot be read out (neither 
> the version of it), so it's not possible for the driver to verify the 
> DT settings against the actual IP configuration. I don't really know how 
> to solve that.

Yes, let's drop the property for now. Just trying to understand how the
IP works ;).

> 
> > > > > I didn't really want to have the added flexibility (and complexity) of 
> > > > > being able to use any R/B line for any connected flash chip. It seems an 
> > > > > unnecessary complication for the driver without much gain.    
> > > > 
> > > > Not really, at least if you properly separate the chip and controller
> > > > objects it's quite easy to deal with, and I'll ask you to do this clean
> > > > separation anyway (even if you say you only have a single chip per
> > > > controller) :P.    
> > > 
> > > Yes, the separation of chip and controller is needed, but the R/B line 
> > > flexibility requires an additional mapping functionality within the 
> > > driver. Not rocket science of course, but I can't see much point in it 
> > > (other than to cover up a potential routing mistake done by a PCB 
> > > designer).  
> > 
> > You seem to argue on all the minor things I'm asking. Honestly, it
> > should be hard or error-prone to do it. And let's say you don't support
> > it in your driver, you should still think about that when designing
> > your bindings.  
> 
> Sorry, it's just that over the years I've seen too much code that 
> introduces various flexibilities just because someone thought it might be 
> 'nice to have at some point' or 'because the hardware supports it', but 
> which in reality is never used and still must be maintained. Sure, one 
> small mapping function is certainly not going to break the bank, far from 
> it, but over time the matrix of things that can be configured can grow to 
> awkward proportions, and often something thought of as a minor issue can 
> turn out to be complex to support in the end. And given a stable API rule, 
> it's too late to simplify things once they have been implemented.
> 
> Of course, the code should not be written in a way to limit future 
> expansion either.
> 
> > > I mean, an IP may be capable of a lot of things, and we don't necessarily 
> > > want to implement them all in the driver to start with and have DT 
> > > propertes for them all do we?  
> > 
> > Hm, you should at least take the capabilities you know about into
> > account when defining a new binding, and you already know that some
> > SoCs might decide to expose 2 or more R/B pins, so it should already be
> > designed this way.  
> 
> Ok. I'll give it some more thought then.
> 
> > I'm opposed to the idea of putting information that can be
> > automatically deduced in the DT. For this specific case, the main
> > reason is that a board vendor can decide to use different NAND chips
> > on the same design, and they might not all share the same
> > capabilities (and you don't want to have a dts for each NAND).  
> 
> Yes, that makes sense of course, but what if someone would want to 
> override the automatic settings, for whatever reason, using an optional DT 
> property? I can think of several reasons either way, that's why I'm 
> asking.

You mean reducing the timings because the board design prevents using
the highest supported mode for example? That would actually be a valid
use case, and I guess we could make a generic property for that
(without the vendor prefix).


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

Re: [PATCH 1/4] of: Add device tree bindings for Evatronix

[Ricard Wanderlof]

On Fri, 10 Jun 2016, Boris Brezillon wrote:

> > Basically, in the general case, the controller can handle a matrix of 
> > nand flash chips. There can be a number of banks, each of which can 
> > have a number of individual CS lines. For the (in this case academic) 
> > case of 3 banks and 4 chip selects per bank, there would be a total of 
> > 3 x 4 = 12 CS lines.
> > 
> > For the IP configuration the driver was written for, there are only 2 
> > CS lines, and we can configure if they are to be viewed by the 
> > controller as 2 CS lines within the same single bank, or 2 separate 
> > banks with one CS each. This is what the DT property is intended to 
> > express. It basically translates directly into a register write in the 
> > IP.
> 
> Okay, got it. I guess we should just expose the chip select in a linear
> way (3 banks of 4 CS means the controller should support up to 12
> chips), unless you really have a way to change the CS pins routing
> internally.

When I go through the driver I will also revisit this and give it some 
more thought if there's a set of bindings which would make sense both for 
the case we have now and for a general configuration of the IP.

> > Yes, that makes sense of course, but what if someone would want to 
> > override the automatic settings, for whatever reason, using an 
> > optional DT property? I can think of several reasons either way, 
> > that's why I'm asking.
>
> You mean reducing the timings because the board design prevents using
> the highest supported mode for example? That would actually be a valid
> use case, and I guess we could make a generic property for that
> (without the vendor prefix).

Yes, either that, or if the automatic selection fails for some reason, say 
in a given case we know that the chips we are using support mode 2 timing, 
but one of them gets misidentified as mode 0. Sure, that is a bug and 
should be fixed of course, but I can imagine commercial situations where 
it may not be feasable to update the kernel, but where a new DT would be 
ok.

/Ricard
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Re: [PATCH 1/4] of: Add device tree bindings for Evatronix

[Ricard Wanderlof]

Hi Boris,

Juggling the rework of the Evatronix NAND flash driver with other projects 
I'm plodding on and will hopefully have something that can be reviewed in 
a few weeks' time.

In the meantime there is an issue which I want to bring up, regarding the 
timing configuration. Back in June we had this discussion, regarding my 
suggestion to put the raw values for the timing control registers in the 
IP in the DT:

On Fri, 10 Jun 2016, Boris Brezillon wrote:

> On Fri, 10 Jun 2016 17:35:24 +0200
> Ricard Wanderlof <ricard.wanderlof@axis.com> wrote:
>
> > > > > > +		     TIME_SEQ_1, TIMINGS_ASYN, TIME_GEN_SEQ_0, TIME_GEN_SEQ_1,
> > > > > > +		     TIME_GEN_SEQ_2 and TIME_GEN_SEQ_3 registers, respectively.    
> > > > > 
> > > > > 
> > > > > Can this be extracted from the timing mode exposed by the NAND chip.
> > > > > IMO it shouldn't be defined in the DT. 
> >
> > [ ... ]
> > 
> > > Just because it's complicated to extract these information at the driver 
> > > level level doesn't mean you should put it in the DT. That's exactly the 
> > > opposite: the DT is supposed to encode the hardware representation, not 
> > > how you want to configure it.  
> > 
> > [ ... ]
> > 
> > > How about using a default/safe timing mode for now (ONFI timing mode 0
> > > should work for all NANDs).
> > > The only thing you'll have to do is retrieve the source clk rate and
> > > calculate timing register values accordingly. Or you can even assume
> > > you always have a 100MHz source clk and hardcode it in your driver.  
> > 
> > Yes, that is certainly possible of course, and the driver already has a 
> > hard-coded default setup for this case.
> > 
> > In that case though the driver could have pre-set setups for other ONFi 
> > modes, and we could have an _optional_ DT property to select them, the 
> > reason for that being in order to handle non-ONFi flashes whose timing 
> > cannot be gleaned from the device itself.
> > 
> > I.e. something like
> > 
> > evatronix,onfi-timing-mode = <2>;
> > 
> Actually this has been added to the nand_flash_dev structure, and it's
> called onfi_timing_mode_default.
> If you need a different timing, define a full ID entry for your NAND.

In our products, we use 1, 2 and 4 Gbit SLC NAND flashes, some of which 
are ONFi complient and some not, depending on the manufacturer. For the 
non-ONFi flashes, the ones which we have looked at and which we specify in 
our products actually support ONFi mode 2. However, in practice, as part 
of the nand_scan_ident() procedure, the chip->onfi_timing_mode_default 
value is set to 0, because the EXTENDED_ID_NAND() macro used for these 
flashes doesn't set the timing mode to anything else. There are some full 
ID listings which apparently set a different timing mode for certain 
flashes (such as Toshiba TC58NVG0S3E).

What I would like to have in the device tree, either as a general mtd or 
proprietary property, is something like the above, possibly called 
something like force-onfi-timing-mode, which forcibly sets the timing mode 
of the hardware. When this property is not set, the timing mode used will 
default to onfi_timing_mode_default for non-ONFi flashes, or 
onfi_get_async_timing_mode() for ONFi flashes.

The reationale behind this is that it is impractical to list all specific 
flash chips which actually support a given timing mode. Say another chip 
comes out on the market either from an existing or a new manufacturer, 
which correctly identifies itself using the ID bytes, but not using the 
full ID. In order to support the speed of this device, we would need to 
upgrade the kernel in the product. If we on the other hand have a hardware 
specification that says that we only use flashes which support a certain 
specified timing mode (because of memory bandwidth requirements), we can 
simply put force-onfi-timing-mode = <2> in the DT, and it will work for 
all flashes that meet the specifications, whether they match the full ID 
or not.

For the case of a .dts file specifying a more general board which can be 
populated with any flash chip, the property can be left out, and the 
timing mode will then be determined using the ID and ONFi parameter page, 
as applicable.

Alternatively having this property as a proprietary one (i.e. 
"evatronix,force-onfi-timing-mode") is an alternative, but I still wanted 
to bring up the discussion for the general case.

/Ricard
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Re: [PATCH 1/4] of: Add device tree bindings for Evatronix

[Boris Brezillon]

Hi Ricard,

On Wed, 26 Oct 2016 13:27:00 +0200
Ricard Wanderlof <ricard.wanderlof@axis.com> wrote:

> Hi Boris,
> 
> Juggling the rework of the Evatronix NAND flash driver with other projects 
> I'm plodding on and will hopefully have something that can be reviewed in 
> a few weeks' time.
> 
> In the meantime there is an issue which I want to bring up, regarding the 
> timing configuration. Back in June we had this discussion, regarding my 
> suggestion to put the raw values for the timing control registers in the 
> IP in the DT:
> 
> On Fri, 10 Jun 2016, Boris Brezillon wrote:
> 
> > On Fri, 10 Jun 2016 17:35:24 +0200
> > Ricard Wanderlof <ricard.wanderlof@axis.com> wrote:
> >  
> > > > > > > +		     TIME_SEQ_1, TIMINGS_ASYN, TIME_GEN_SEQ_0, TIME_GEN_SEQ_1,
> > > > > > > +		     TIME_GEN_SEQ_2 and TIME_GEN_SEQ_3 registers, respectively.      
> > > > > > 
> > > > > > 
> > > > > > Can this be extracted from the timing mode exposed by the NAND chip.
> > > > > > IMO it shouldn't be defined in the DT.   
> > >
> > > [ ... ]
> > >   
> > > > Just because it's complicated to extract these information at the driver 
> > > > level level doesn't mean you should put it in the DT. That's exactly the 
> > > > opposite: the DT is supposed to encode the hardware representation, not 
> > > > how you want to configure it.    
> > > 
> > > [ ... ]
> > >   
> > > > How about using a default/safe timing mode for now (ONFI timing mode 0
> > > > should work for all NANDs).
> > > > The only thing you'll have to do is retrieve the source clk rate and
> > > > calculate timing register values accordingly. Or you can even assume
> > > > you always have a 100MHz source clk and hardcode it in your driver.    
> > > 
> > > Yes, that is certainly possible of course, and the driver already has a 
> > > hard-coded default setup for this case.
> > > 
> > > In that case though the driver could have pre-set setups for other ONFi 
> > > modes, and we could have an _optional_ DT property to select them, the 
> > > reason for that being in order to handle non-ONFi flashes whose timing 
> > > cannot be gleaned from the device itself.
> > > 
> > > I.e. something like
> > > 
> > > evatronix,onfi-timing-mode = <2>;
> > >   
> > Actually this has been added to the nand_flash_dev structure, and it's
> > called onfi_timing_mode_default.
> > If you need a different timing, define a full ID entry for your NAND.  
> 
> In our products, we use 1, 2 and 4 Gbit SLC NAND flashes, some of which 
> are ONFi complient and some not, depending on the manufacturer. For the 
> non-ONFi flashes, the ones which we have looked at and which we specify in 
> our products actually support ONFi mode 2. However, in practice, as part 
> of the nand_scan_ident() procedure, the chip->onfi_timing_mode_default 
> value is set to 0, because the EXTENDED_ID_NAND() macro used for these 
> flashes doesn't set the timing mode to anything else. There are some full 
> ID listings which apparently set a different timing mode for certain 
> flashes (such as Toshiba TC58NVG0S3E).
> 
> What I would like to have in the device tree, either as a general mtd or 
> proprietary property, is something like the above, possibly called 
> something like force-onfi-timing-mode, which forcibly sets the timing mode 
> of the hardware. When this property is not set, the timing mode used will 
> default to onfi_timing_mode_default for non-ONFi flashes, or 
> onfi_get_async_timing_mode() for ONFi flashes.

I'm not against the addition of this onfi-timing-mode property, but not
for the same reason. Your NAND might be able to support a specific
timing mode, but for some reason, the board design prevents using this
mode (interferences on the DATA or CONTROL lines for example).

To me, this property would just serve as a limitation: if the chip
supports mode 5 but the board says that it supports up to mode 2, then
the implementation should stick to mode 2. ITOH, if the 5 is defined in
the DT, but the chip only supports up to mode 2, then mode 2 should be
used.

> 
> The reationale behind this is that it is impractical to list all specific 
> flash chips which actually support a given timing mode. Say another chip 
> comes out on the market either from an existing or a new manufacturer, 
> which correctly identifies itself using the ID bytes, but not using the 
> full ID. In order to support the speed of this device, we would need to 
> upgrade the kernel in the product. If we on the other hand have a hardware 
> specification that says that we only use flashes which support a certain 
> specified timing mode (because of memory bandwidth requirements), we can 
> simply put force-onfi-timing-mode = <2> in the DT, and it will work for 
> all flashes that meet the specifications, whether they match the full ID 
> or not.

Well, I don't think this is a good argument. If you want to support
advanced features, you'll still have to define some chip/vendor
specific implementation, and these will likely be assigned depending on
the full ID. So, in the end, if you want to have good support for a
NAND chip that is not ONFi or JEDEC compliant, you'll have to update
your kernel.

Also note that I'm trying to get rid of the full-id table, and replace
it by vendor detection/initialization [1], so that you can tweak
different things on a per-chip basis (and this includes the timing
mode).

> 
> For the case of a .dts file specifying a more general board which can be 
> populated with any flash chip, the property can be left out, and the 
> timing mode will then be determined using the ID and ONFi parameter page, 
> as applicable.

I'd like to leave as much as possible in the NAND
detection/initialization code, and this includes selecting the best
timing mode (based on JEDEC, ONFi, or vendor specific ID parsing).

Also remember that DT is supposed to be represent the HW blocks, not
their configuration.

> 
> Alternatively having this property as a proprietary one (i.e. 
> "evatronix,force-onfi-timing-mode") is an alternative, but I still wanted 
> to bring up the discussion for the general case.

No, please do not define a private property. I think I'd prefer to
have a generic property, but I'm still not convinced this is necessary.

Regards,

Boris

Re: [PATCH 1/4] of: Add device tree bindings for Evatronix

[Boris Brezillon]

On Wed, 26 Oct 2016 13:52:41 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> Hi Ricard,
> 
> On Wed, 26 Oct 2016 13:27:00 +0200
> Ricard Wanderlof <ricard.wanderlof@axis.com> wrote:
> 
> > Hi Boris,
> > 
> > Juggling the rework of the Evatronix NAND flash driver with other projects 
> > I'm plodding on and will hopefully have something that can be reviewed in 
> > a few weeks' time.
> > 
> > In the meantime there is an issue which I want to bring up, regarding the 
> > timing configuration. Back in June we had this discussion, regarding my 
> > suggestion to put the raw values for the timing control registers in the 
> > IP in the DT:
> > 
> > On Fri, 10 Jun 2016, Boris Brezillon wrote:
> >   
> > > On Fri, 10 Jun 2016 17:35:24 +0200
> > > Ricard Wanderlof <ricard.wanderlof@axis.com> wrote:
> > >    
> > > > > > > > +		     TIME_SEQ_1, TIMINGS_ASYN, TIME_GEN_SEQ_0, TIME_GEN_SEQ_1,
> > > > > > > > +		     TIME_GEN_SEQ_2 and TIME_GEN_SEQ_3 registers, respectively.        
> > > > > > > 
> > > > > > > 
> > > > > > > Can this be extracted from the timing mode exposed by the NAND chip.
> > > > > > > IMO it shouldn't be defined in the DT.     
> > > >
> > > > [ ... ]
> > > >     
> > > > > Just because it's complicated to extract these information at the driver 
> > > > > level level doesn't mean you should put it in the DT. That's exactly the 
> > > > > opposite: the DT is supposed to encode the hardware representation, not 
> > > > > how you want to configure it.      
> > > > 
> > > > [ ... ]
> > > >     
> > > > > How about using a default/safe timing mode for now (ONFI timing mode 0
> > > > > should work for all NANDs).
> > > > > The only thing you'll have to do is retrieve the source clk rate and
> > > > > calculate timing register values accordingly. Or you can even assume
> > > > > you always have a 100MHz source clk and hardcode it in your driver.      
> > > > 
> > > > Yes, that is certainly possible of course, and the driver already has a 
> > > > hard-coded default setup for this case.
> > > > 
> > > > In that case though the driver could have pre-set setups for other ONFi 
> > > > modes, and we could have an _optional_ DT property to select them, the 
> > > > reason for that being in order to handle non-ONFi flashes whose timing 
> > > > cannot be gleaned from the device itself.
> > > > 
> > > > I.e. something like
> > > > 
> > > > evatronix,onfi-timing-mode = <2>;
> > > >     
> > > Actually this has been added to the nand_flash_dev structure, and it's
> > > called onfi_timing_mode_default.
> > > If you need a different timing, define a full ID entry for your NAND.    
> > 
> > In our products, we use 1, 2 and 4 Gbit SLC NAND flashes, some of which 
> > are ONFi complient and some not, depending on the manufacturer. For the 
> > non-ONFi flashes, the ones which we have looked at and which we specify in 
> > our products actually support ONFi mode 2. However, in practice, as part 
> > of the nand_scan_ident() procedure, the chip->onfi_timing_mode_default 
> > value is set to 0, because the EXTENDED_ID_NAND() macro used for these 
> > flashes doesn't set the timing mode to anything else. There are some full 
> > ID listings which apparently set a different timing mode for certain 
> > flashes (such as Toshiba TC58NVG0S3E).
> > 
> > What I would like to have in the device tree, either as a general mtd or 
> > proprietary property, is something like the above, possibly called 
> > something like force-onfi-timing-mode, which forcibly sets the timing mode 
> > of the hardware. When this property is not set, the timing mode used will 
> > default to onfi_timing_mode_default for non-ONFi flashes, or 
> > onfi_get_async_timing_mode() for ONFi flashes.  
> 
> I'm not against the addition of this onfi-timing-mode property, but not
> for the same reason. Your NAND might be able to support a specific
> timing mode, but for some reason, the board design prevents using this
> mode (interferences on the DATA or CONTROL lines for example).
> 
> To me, this property would just serve as a limitation: if the chip
> supports mode 5 but the board says that it supports up to mode 2, then
> the implementation should stick to mode 2. ITOH, if the 5 is defined in
> the DT, but the chip only supports up to mode 2, then mode 2 should be
> used.
> 
> > 
> > The reationale behind this is that it is impractical to list all specific 
> > flash chips which actually support a given timing mode. Say another chip 
> > comes out on the market either from an existing or a new manufacturer, 
> > which correctly identifies itself using the ID bytes, but not using the 
> > full ID. In order to support the speed of this device, we would need to 
> > upgrade the kernel in the product. If we on the other hand have a hardware 
> > specification that says that we only use flashes which support a certain 
> > specified timing mode (because of memory bandwidth requirements), we can 
> > simply put force-onfi-timing-mode = <2> in the DT, and it will work for 
> > all flashes that meet the specifications, whether they match the full ID 
> > or not.  
> 
> Well, I don't think this is a good argument. If you want to support
> advanced features, you'll still have to define some chip/vendor
> specific implementation, and these will likely be assigned depending on
> the full ID. So, in the end, if you want to have good support for a
> NAND chip that is not ONFi or JEDEC compliant, you'll have to update
> your kernel.
> 
> Also note that I'm trying to get rid of the full-id table, and replace
> it by vendor detection/initialization [1], so that you can tweak
> different things on a per-chip basis (and this includes the timing
> mode).

Oops, forgot to put the reference:

[1]http://lkml.iu.edu/hypermail/linux/kernel/1606.2/02892.html

> 
> > 
> > For the case of a .dts file specifying a more general board which can be 
> > populated with any flash chip, the property can be left out, and the 
> > timing mode will then be determined using the ID and ONFi parameter page, 
> > as applicable.  
> 
> I'd like to leave as much as possible in the NAND
> detection/initialization code, and this includes selecting the best
> timing mode (based on JEDEC, ONFi, or vendor specific ID parsing).
> 
> Also remember that DT is supposed to be represent the HW blocks, not
> their configuration.
> 
> > 
> > Alternatively having this property as a proprietary one (i.e. 
> > "evatronix,force-onfi-timing-mode") is an alternative, but I still wanted 
> > to bring up the discussion for the general case.  
> 
> No, please do not define a private property. I think I'd prefer to
> have a generic property, but I'm still not convinced this is necessary.
> 
> Regards,
> 
> Boris
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/4] of: Add device tree bindings for Evatronix

[Ricard Wanderlof]

Hi Boris,

On Wed, 26 Oct 2016, Boris Brezillon wrote:

> On Wed, 26 Oct 2016 13:27:00 +0200
> Ricard Wanderlof <ricard.wanderlof@axis.com> wrote:
>
> > > > 
> > > > evatronix,onfi-timing-mode = <2>;
> > > >   
> > [ ... ]
> > In our products, we use 1, 2 and 4 Gbit SLC NAND flashes, some of which 
> > are ONFi complient and some not, depending on the manufacturer. For the 
> > non-ONFi flashes, the ones which we have looked at and which we specify in 
> > our products actually support ONFi mode 2. However, in practice, as part 
> > of the nand_scan_ident() procedure, the chip->onfi_timing_mode_default 
> > value is set to 0, because the EXTENDED_ID_NAND() macro used for these 
> > flashes doesn't set the timing mode to anything else. There are some full 
> > ID listings which apparently set a different timing mode for certain 
> > flashes (such as Toshiba TC58NVG0S3E).
> > 
> > What I would like to have in the device tree, either as a general mtd or 
> > proprietary property, is something like the above, possibly called 
> > something like force-onfi-timing-mode, which forcibly sets the timing mode 
> > of the hardware. When this property is not set, the timing mode used will 
> > default to onfi_timing_mode_default for non-ONFi flashes, or 
> > onfi_get_async_timing_mode() for ONFi flashes.
> 
> I'm not against the addition of this onfi-timing-mode property, but not
> for the same reason. Your NAND might be able to support a specific
> timing mode, but for some reason, the board design prevents using this
> mode (interferences on the DATA or CONTROL lines for example).
> 
> To me, this property would just serve as a limitation: if the chip
> supports mode 5 but the board says that it supports up to mode 2, then
> the implementation should stick to mode 2. ITOH, if the 5 is defined in
> the DT, but the chip only supports up to mode 2, then mode 2 should be
> used.

Yes, I remember you had that suggestion back in June as well. That would 
be more of a 'restrict-onfi-timing-mode' that sets the maximum speed to be 
used. I can see the use of that too, but that's not what I'm talking about 
here of course. Although the reason is the same: it sets up something that 
cannot be determined by probing.

> > The reationale behind this is that it is impractical to list all specific 
> > flash chips which actually support a given timing mode. Say another chip 
> > comes out on the market either from an existing or a new manufacturer, 
> > which correctly identifies itself using the ID bytes, but not using the 
> > full ID. In order to support the speed of this device, we would need to 
> > upgrade the kernel in the product. If we on the other hand have a hardware 
> > specification that says that we only use flashes which support a certain 
> > specified timing mode (because of memory bandwidth requirements), we can 
> > simply put force-onfi-timing-mode = <2> in the DT, and it will work for 
> > all flashes that meet the specifications, whether they match the full ID 
> > or not.
> 
> Well, I don't think this is a good argument. If you want to support
> advanced features, you'll still have to define some chip/vendor
> specific implementation, and these will likely be assigned depending on
> the full ID. So, in the end, if you want to have good support for a
> NAND chip that is not ONFi or JEDEC compliant, you'll have to update
> your kernel.

It's true that some flashes have advanced features, such as built-in ECC, 
which would be nice to support, however, our experience has been that such 
features are very vendor-specific, and we try to avoid avoid using them as 
it would tie us to a specific flash chip vendor.

However, mildly accelerated chip timing (i.e. ONFi mode 2) I wouldn't call 
an advanced feature, and it seems to be supported by all of the chips in 
the 1..4 Gb range that we've seen, although I wouldn't be so bold and go 
so far as to say that I know all available flash chips (at least in this 
size range) support that mode. That's why I'm reluctant to suggest 
changing the entries for these flashes in the table in nand_ids.c, which 
would be another alternative. And if we did make that change, it might 
cause problems in systems which currently use onfi_timing_mode_default if 
the timing suddenly becomes faster after a kernel update. Although, at 
least looking at the 4.8 tree I'm using for development, it's only the 
sunxi NAND driver which uses it, so I guess it would be alright to change 
as it is not in widespread use yet? [**]

I was looking at the timing specs for a range of 2 and 4 Gb flashes from a 
number of manufacturers a while back, and the timing specificiations were 
surprisingly similar, even across the non-ONFi manufacturers.

> Also note that I'm trying to get rid of the full-id table, and replace
> it by vendor detection/initialization [1], so that you can tweak
> different things on a per-chip basis (and this includes the timing
> mode).

I'm assuming [1] refers to https://lkml.org/lkml/2016/5/27/264 which you 
have referenced before. Has this patch set been applied yet? I did a 
cursory search for it in the kernel commit log but couldn't find it, so I 
haven't had a closer look at it since I can't really make use of any 
features contained therein yet if that is so.

> > For the case of a .dts file specifying a more general board which can be 
> > populated with any flash chip, the property can be left out, and the 
> > timing mode will then be determined using the ID and ONFi parameter page, 
> > as applicable.
> 
> I'd like to leave as much as possible in the NAND
> detection/initialization code, and this includes selecting the best
> timing mode (based on JEDEC, ONFi, or vendor specific ID parsing).
> 
> Also remember that DT is supposed to be represent the HW blocks, not
> their configuration.

Well, in the case of the timing mode, it does represent the external chip 
that is conected, in terms of information that apparently cannot be 
probed, in a similar manner that there are properties to specify how a 
given pin on an audio codec is connected, for instance. One minor tweak 
would be to have the property specify not the specific timing mode to use, 
but range of timing modes supported by the chip (i.e. a vector of bits 
like ONFI_TIMING_MODE_0 etc. However, in practice in a given setup, one 
would only want to use the fastest mode anyway, so there'd be little point 
in specifying any other (except mode 0 as it is the default).

[**] A related question is what values this parameter is supposed to have 
in the ID table. Is it the numeric timing mode (i.e. 2 for mode 2), or is 
it a bit vector which is an or of ONFI_TIMING_MODE_0, etc ? The sunxi 
driver seems to presume the latter, whereas the entries in nand_ids.c 
would seem to imply the former, with values such as 2 (for the 
TC58NVG0S3E) and 4 (for the H27UCG8T2ATR-BC) - surely these chips support 
more than a single timing mode?

/Ricard
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Re: [PATCH 1/4] of: Add device tree bindings for Evatronix

[Boris Brezillon]

On Wed, 26 Oct 2016 14:31:53 +0200
Ricard Wanderlof <ricard.wanderlof@axis.com> wrote:

> Hi Boris,
> 
> On Wed, 26 Oct 2016, Boris Brezillon wrote:
> 
> > On Wed, 26 Oct 2016 13:27:00 +0200
> > Ricard Wanderlof <ricard.wanderlof@axis.com> wrote:
> >  
> > > > > 
> > > > > evatronix,onfi-timing-mode = <2>;
> > > > >     
> > > [ ... ]
> > > In our products, we use 1, 2 and 4 Gbit SLC NAND flashes, some of which 
> > > are ONFi complient and some not, depending on the manufacturer. For the 
> > > non-ONFi flashes, the ones which we have looked at and which we specify in 
> > > our products actually support ONFi mode 2. However, in practice, as part 
> > > of the nand_scan_ident() procedure, the chip->onfi_timing_mode_default 
> > > value is set to 0, because the EXTENDED_ID_NAND() macro used for these 
> > > flashes doesn't set the timing mode to anything else. There are some full 
> > > ID listings which apparently set a different timing mode for certain 
> > > flashes (such as Toshiba TC58NVG0S3E).
> > > 
> > > What I would like to have in the device tree, either as a general mtd or 
> > > proprietary property, is something like the above, possibly called 
> > > something like force-onfi-timing-mode, which forcibly sets the timing mode 
> > > of the hardware. When this property is not set, the timing mode used will 
> > > default to onfi_timing_mode_default for non-ONFi flashes, or 
> > > onfi_get_async_timing_mode() for ONFi flashes.  
> > 
> > I'm not against the addition of this onfi-timing-mode property, but not
> > for the same reason. Your NAND might be able to support a specific
> > timing mode, but for some reason, the board design prevents using this
> > mode (interferences on the DATA or CONTROL lines for example).
> > 
> > To me, this property would just serve as a limitation: if the chip
> > supports mode 5 but the board says that it supports up to mode 2, then
> > the implementation should stick to mode 2. ITOH, if the 5 is defined in
> > the DT, but the chip only supports up to mode 2, then mode 2 should be
> > used.  
> 
> Yes, I remember you had that suggestion back in June as well. That would 
> be more of a 'restrict-onfi-timing-mode' that sets the maximum speed to be 
> used. I can see the use of that too, but that's not what I'm talking about 
> here of course. Although the reason is the same: it sets up something that 
> cannot be determined by probing.
> 
> > > The reationale behind this is that it is impractical to list all specific 
> > > flash chips which actually support a given timing mode. Say another chip 
> > > comes out on the market either from an existing or a new manufacturer, 
> > > which correctly identifies itself using the ID bytes, but not using the 
> > > full ID. In order to support the speed of this device, we would need to 
> > > upgrade the kernel in the product. If we on the other hand have a hardware 
> > > specification that says that we only use flashes which support a certain 
> > > specified timing mode (because of memory bandwidth requirements), we can 
> > > simply put force-onfi-timing-mode = <2> in the DT, and it will work for 
> > > all flashes that meet the specifications, whether they match the full ID 
> > > or not.  
> > 
> > Well, I don't think this is a good argument. If you want to support
> > advanced features, you'll still have to define some chip/vendor
> > specific implementation, and these will likely be assigned depending on
> > the full ID. So, in the end, if you want to have good support for a
> > NAND chip that is not ONFi or JEDEC compliant, you'll have to update
> > your kernel.  
> 
> It's true that some flashes have advanced features, such as built-in ECC, 
> which would be nice to support, however, our experience has been that such 
> features are very vendor-specific, and we try to avoid avoid using them as 
> it would tie us to a specific flash chip vendor.
> 
> However, mildly accelerated chip timing (i.e. ONFi mode 2) I wouldn't call 
> an advanced feature, and it seems to be supported by all of the chips in 
> the 1..4 Gb range that we've seen, although I wouldn't be so bold and go 
> so far as to say that I know all available flash chips (at least in this 
> size range) support that mode. That's why I'm reluctant to suggest 
> changing the entries for these flashes in the table in nand_ids.c, which 
> would be another alternative. And if we did make that change, it might 
> cause problems in systems which currently use onfi_timing_mode_default if 
> the timing suddenly becomes faster after a kernel update. Although, at 
> least looking at the 4.8 tree I'm using for development, it's only the 
> sunxi NAND driver which uses it, so I guess it would be alright to change 
> as it is not in widespread use yet? [**]

I would definitely prefer this approach.

> 
> I was looking at the timing specs for a range of 2 and 4 Gb flashes from a 
> number of manufacturers a while back, and the timing specificiations were 
> surprisingly similar, even across the non-ONFi manufacturers.

That's not so surprising, NAND vendors try to compete with each others,
and they are pretty much all using the same technology, which explains
why for a given generation of chip (which is usually related to a chip
size), the timings are close.

> 
> > Also note that I'm trying to get rid of the full-id table, and replace
> > it by vendor detection/initialization [1], so that you can tweak
> > different things on a per-chip basis (and this includes the timing
> > mode).  
> 
> I'm assuming [1] refers to https://lkml.org/lkml/2016/5/27/264 which you 
> have referenced before.

Yes.

> Has this patch set been applied yet?

Nope, not yet.

> I did a 
> cursory search for it in the kernel commit log but couldn't find it, so I 
> haven't had a closer look at it since I can't really make use of any 
> features contained therein yet if that is so.

I was waiting for more reviews/tests, but it did not happen. I might
consider applying the last version even if nobody reviewed it, but I
fear it will break some platforms, hence my hesitations to apply it
without any external tests/reviews.

> 
> > > For the case of a .dts file specifying a more general board which can be 
> > > populated with any flash chip, the property can be left out, and the 
> > > timing mode will then be determined using the ID and ONFi parameter page, 
> > > as applicable.  
> > 
> > I'd like to leave as much as possible in the NAND
> > detection/initialization code, and this includes selecting the best
> > timing mode (based on JEDEC, ONFi, or vendor specific ID parsing).
> > 
> > Also remember that DT is supposed to be represent the HW blocks, not
> > their configuration.  
> 
> Well, in the case of the timing mode, it does represent the external chip 
> that is conected, in terms of information that apparently cannot be 
> probed, in a similar manner that there are properties to specify how a 
> given pin on an audio codec is connected, for instance. One minor tweak 
> would be to have the property specify not the specific timing mode to use, 
> but range of timing modes supported by the chip (i.e. a vector of bits 
> like ONFI_TIMING_MODE_0 etc. However, in practice in a given setup, one 
> would only want to use the fastest mode anyway, so there'd be little point 
> in specifying any other (except mode 0 as it is the default).
> 
> [**] A related question is what values this parameter is supposed to have 
> in the ID table. Is it the numeric timing mode (i.e. 2 for mode 2), or is 
> it a bit vector which is an or of ONFI_TIMING_MODE_0, etc ?

It's supposed to be the highest supported mode, not a bitmap
containing all the supported modes.

> The sunxi 
> driver seems to presume the latter, whereas the entries in nand_ids.c 
> would seem to imply the former, with values such as 2 (for the 
> TC58NVG0S3E) and 4 (for the H27UCG8T2ATR-BC) - surely these chips support 
> more than a single timing mode?

Hm, no it's not. As can be seen here [1], if the NAND is not ONFi, then
we're using the onfi_timing_mode_default value, which is encoding the
highest supported mode. The other case is when the chip is ONFi
compliant, and in this case, the timing_mode field in exposing
supported modes as a bitmap.

One last thing, we recently introduced a generic timing selection/setup
infrastructure [2] to avoid duplicating the code pointed here [1] in all
drivers. It's been merged in 4.9, and that'd would be better to use
this infrastructure in your driver.

[1]http://lxr.free-electrons.com/source/drivers/mtd/nand/sunxi_nand.c#L1723
[2]https://www.spinics.net/lists/arm-kernel/msg532007.html

Re: [PATCH 1/4] of: Add device tree bindings for Evatronix

[Ricard Wanderlof]

Hi Boris,

On Wed, 26 Oct 2016, Boris Brezillon wrote:

> > However, mildly accelerated chip timing (i.e. ONFi mode 2) I wouldn't call 
> > an advanced feature, and it seems to be supported by all of the chips in 
> > the 1..4 Gb range that we've seen, although I wouldn't be so bold and go 
> > so far as to say that I know all available flash chips (at least in this 
> > size range) support that mode. That's why I'm reluctant to suggest 
> > changing the entries for these flashes in the table in nand_ids.c, which 
> > would be another alternative. And if we did make that change, it might 
> > cause problems in systems which currently use onfi_timing_mode_default if 
> > the timing suddenly becomes faster after a kernel update. Although, at 
> > least looking at the 4.8 tree I'm using for development, it's only the 
> > sunxi NAND driver which uses it, so I guess it would be alright to change 
> > as it is not in widespread use yet?
> 
> I would definitely prefer this approach.

I don't mind making a patch for nand_ids.c which sets a more aggressive 
timing mode for at least flashes in the 1..4 Gb range. As I mentioned 
though I'm not entirely confident saying that that is in fact the case for 
_all_ flashes of that size, but perhaps it's good enough for now making 
that assumption given the flashes I've actually looked at? (I could refer 
to a number of chips in the commit message for reference).

> > The sunxi driver seems to presume the latter, whereas the entries in 
> > nand_ids.c would seem to imply the former, with values such as 2 (for 
> > the TC58NVG0S3E) and 4 (for the H27UCG8T2ATR-BC) - surely these chips 
> > support more than a single timing mode?
> >
> Hm, no it's not. As can be seen here [1], if the NAND is not ONFi, then
> we're using the onfi_timing_mode_default value, which is encoding the
> highest supported mode. The other case is when the chip is ONFi
> compliant, and in this case, the timing_mode field in exposing
> supported modes as a bitmap.
> [1]http://lxr.free-electrons.com/source/drivers/mtd/nand/sunxi_nand.c#L1723

Yes, you're right of course, I was thrown by the fact that the mode 
variable in the sunxi_nand_chip_init_timings() function is used to 
both represent the bit vector and the actual timing mode.

> One last thing, we recently introduced a generic timing selection/setup
> infrastructure [2] to avoid duplicating the code pointed here [1] in all
> drivers. It's been merged in 4.9, and that'd would be better to use
> this infrastructure in your driver.
>
> [2]https://www.spinics.net/lists/arm-kernel/msg532007.html

One thing I would have liked to have is to have the actual timing mode 
number in the nand_data_interface struct, for controllers like in my case 
where it's problematic to take the numeric timing values and map them to 
register values, and the timing register values need to be pre-computed. 
There might be other reasons where the driver might want to know the mode 
number. Sure, once a mode has been agreed on it is set in 
chip->onfi_timing_mode_default, but that won't help 
nand_init_data_interface() when it calls 
chip->setup_data_interface(..., true) to determine if a given mode is in 
fact supported by the controller.


A minor related issue which I been pondering over is that the fact that 
the supported timing modes of an ONFi flash are expressed as a bit vector 
rather than just a single 'highest possible mode' value, which indicates 
that the idea is that a flash may support a certain mode but not 
necessarily all the ones below, e.g. supporting modes 3, 2 and 0. A 
cursory look at the timing mode definition gives the impression that lower 
speeds are subcases of the higher speed ones, but it's hard to say if 
there are relationships between certain parameters that mean this is not 
necessarily so,

However, when we take the highest supported mode from the ID table, we 
convert it into a continguous bit vector using GENMASK() in 
nand_base.c:nand_init_data_interface(), thus assuming that the flash will 
in fact support all slower modes as well.

Perhaps there are no flashes which support a non-contiguous range of 
timing modes (I don't have enough experience with ONFi flashes to say 
one way or the other), it just seemed to me an odd discrepency and 
limitation for non-ONFi flashes.

/Ricard
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Re: [PATCH 1/4] of: Add device tree bindings for Evatronix

[Boris Brezillon]

On Fri, 28 Oct 2016 16:35:20 +0200
Ricard Wanderlof <ricard.wanderlof@axis.com> wrote:

> Hi Boris,
> 
> On Wed, 26 Oct 2016, Boris Brezillon wrote:
> 
> > > However, mildly accelerated chip timing (i.e. ONFi mode 2) I wouldn't call 
> > > an advanced feature, and it seems to be supported by all of the chips in 
> > > the 1..4 Gb range that we've seen, although I wouldn't be so bold and go 
> > > so far as to say that I know all available flash chips (at least in this 
> > > size range) support that mode. That's why I'm reluctant to suggest 
> > > changing the entries for these flashes in the table in nand_ids.c, which 
> > > would be another alternative. And if we did make that change, it might 
> > > cause problems in systems which currently use onfi_timing_mode_default if 
> > > the timing suddenly becomes faster after a kernel update. Although, at 
> > > least looking at the 4.8 tree I'm using for development, it's only the 
> > > sunxi NAND driver which uses it, so I guess it would be alright to change 
> > > as it is not in widespread use yet?  
> > 
> > I would definitely prefer this approach.  
> 
> I don't mind making a patch for nand_ids.c which sets a more aggressive 
> timing mode for at least flashes in the 1..4 Gb range. As I mentioned 
> though I'm not entirely confident saying that that is in fact the case for 
> _all_ flashes of that size, but perhaps it's good enough for now making 
> that assumption given the flashes I've actually looked at? (I could refer 
> to a number of chips in the commit message for reference).
> 
> > > The sunxi driver seems to presume the latter, whereas the entries in 
> > > nand_ids.c would seem to imply the former, with values such as 2 (for 
> > > the TC58NVG0S3E) and 4 (for the H27UCG8T2ATR-BC) - surely these chips 
> > > support more than a single timing mode?
> > >  
> > Hm, no it's not. As can be seen here [1], if the NAND is not ONFi, then
> > we're using the onfi_timing_mode_default value, which is encoding the
> > highest supported mode. The other case is when the chip is ONFi
> > compliant, and in this case, the timing_mode field in exposing
> > supported modes as a bitmap.
> > [1]http://lxr.free-electrons.com/source/drivers/mtd/nand/sunxi_nand.c#L1723  
> 
> Yes, you're right of course, I was thrown by the fact that the mode 
> variable in the sunxi_nand_chip_init_timings() function is used to 
> both represent the bit vector and the actual timing mode.
> 
> > One last thing, we recently introduced a generic timing selection/setup
> > infrastructure [2] to avoid duplicating the code pointed here [1] in all
> > drivers. It's been merged in 4.9, and that'd would be better to use
> > this infrastructure in your driver.
> >
> > [2]https://www.spinics.net/lists/arm-kernel/msg532007.html  
> 
> One thing I would have liked to have is to have the actual timing mode 
> number in the nand_data_interface struct, for controllers like in my case 
> where it's problematic to take the numeric timing values and map them to 
> register values, and the timing register values need to be pre-computed. 

What do you mean by pre-computed? If you're able to pre-compute the
timings, why can't you do that at runtime?

> There might be other reasons where the driver might want to know the mode 
> number.

Do you have real examples?

> Sure, once a mode has been agreed on it is set in 
> chip->onfi_timing_mode_default, but that won't help 
> nand_init_data_interface() when it calls 
> chip->setup_data_interface(..., true) to determine if a given mode is in 
> fact supported by the controller.

We could pass the ONFI timing mode here, but some timings are not
defined using this mode (like tCCS), and I'm not sure we have the same
mapping between ONFI and JEDEC modes. The other thing is that I wanted
to leave the door open for vendor specific timing definitions which do
not exactly match one of the timing mode defined in the ONFI spec.

That's why I didn't bother exposing the ONFI timing mode to the NAND
controller ->setup_data_interface() implementation.

> 
> 
> A minor related issue which I been pondering over is that the fact that 
> the supported timing modes of an ONFi flash are expressed as a bit vector 
> rather than just a single 'highest possible mode' value, which indicates 
> that the idea is that a flash may support a certain mode but not 
> necessarily all the ones below, e.g. supporting modes 3, 2 and 0. A 
> cursory look at the timing mode definition gives the impression that lower 
> speeds are subcases of the higher speed ones, but it's hard to say if 
> there are relationships between certain parameters that mean this is not 
> necessarily so,
> 
> However, when we take the highest supported mode from the ID table, we 
> convert it into a continguous bit vector using GENMASK() in 
> nand_base.c:nand_init_data_interface(), thus assuming that the flash will 
> in fact support all slower modes as well.
> 
> Perhaps there are no flashes which support a non-contiguous range of 
> timing modes (I don't have enough experience with ONFi flashes to say 
> one way or the other), it just seemed to me an odd discrepency and 
> limitation for non-ONFi flashes.

That was a deliberate choice. Honestly, I've never seen a NAND that is
capable of supporting higher modes without supporting the lower ones.
If that ever happens, we'll switch back to a bitmap, but that's rather
unlikely.

Re: [PATCH 1/4] of: Add device tree bindings for Evatronix

[Ricard Wanderlof]

On Fri, 28 Oct 2016, Boris Brezillon wrote:

> > One thing I would have liked to have is to have the actual timing mode 
> > number in the nand_data_interface struct, for controllers like in my case 
> > where it's problematic to take the numeric timing values and map them to 
> > register values, and the timing register values need to be pre-computed. 
> 
> What do you mean by pre-computed? If you're able to pre-compute the
> timings, why can't you do that at runtime?

The short answer in this case is because the timing values were delivered 
by our ASIC integration vendor, partly after running simulations to verify 
the exact timing using the actual silicon layout. That is why I originally 
wanted to put those timing register values directly in the DT, becuase 
they are in practice part of the specifications for the actual ASIC. In 
essence, we looked at the flash chips we wanted to use with the ASIC, and 
realized they all conformed to ONFi mode 2. Our vendor then provided us 
with mode 2 timing values that they guaranteed would work with the chip. 
(In addition, mode 0 is used during initial boot so minimize the 
likeleyhood that something would go wrong, given that the boot loader is 
in a ROM.) Especially given that there was no mtd timing setup 
infrastructure at the time, we had no interest in how the values were 
derived, or to support more modes.

Another issue is how to model the internal delays within the ASIC between 
the NAND controller IP and the pads, which are also part of the equation.

I don't know if our case is unique, but at least it is an example of a 
situation in which runtime calculation of the timing register values is 
problematic.

> > There might be other reasons where the driver might want to know the mode 
> > number.
> 
> Do you have real examples?

No, it just seemed like a reasonable concept, given that 
nand_init_data_interface() actually does this mapping anyway, so it might 
as well inform chip->setup_data_interface() about it, since the 
information is right there anyway.

> We could pass the ONFI timing mode here, but some timings are not
> defined using this mode (like tCCS), and I'm not sure we have the same
> mapping between ONFI and JEDEC modes. The other thing is that I wanted
> to leave the door open for vendor specific timing definitions which do
> not exactly match one of the timing mode defined in the ONFI spec.
> 
> That's why I didn't bother exposing the ONFI timing mode to the NAND
> controller ->setup_data_interface() implementation.

Ok, so the reason is really future expansion, where ONFi modes 0..5 may 
not be the only modes to be configured. Fair enough.

> > A minor related issue which I been pondering over is that the fact that 
> > the supported timing modes of an ONFi flash are expressed as a bit vector 
> > rather than just a single 'highest possible mode' value, which indicates 
> > that the idea is that a flash may support a certain mode but not 
> > necessarily all the ones below, e.g. supporting modes 3, 2 and 0.
> 
> That was a deliberate choice. Honestly, I've never seen a NAND that is
> capable of supporting higher modes without supporting the lower ones.
> If that ever happens, we'll switch back to a bitmap, but that's rather
> unlikely.

I agree, I would find that hard to believe too. I just don't have the 
experience to back that up. And the fact that ONFi chips actually do 
report this as a bitmap indicates that someone at some time considered 
this a distinct possibility. Or it could just be part of making the 
specification as general as possible without actually having any practical 
case for it.

/Ricard
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Re: [PATCH 1/4] of: Add device tree bindings for Evatronix

[Boris Brezillon]

On Fri, 28 Oct 2016 18:01:44 +0200
Ricard Wanderlof <ricard.wanderlof@axis.com> wrote:

> On Fri, 28 Oct 2016, Boris Brezillon wrote:
> 
> > > One thing I would have liked to have is to have the actual timing mode 
> > > number in the nand_data_interface struct, for controllers like in my case 
> > > where it's problematic to take the numeric timing values and map them to 
> > > register values, and the timing register values need to be pre-computed.   
> > 
> > What do you mean by pre-computed? If you're able to pre-compute the
> > timings, why can't you do that at runtime?  
> 
> The short answer in this case is because the timing values were delivered 
> by our ASIC integration vendor, partly after running simulations to verify 
> the exact timing using the actual silicon layout. That is why I originally 
> wanted to put those timing register values directly in the DT, becuase 
> they are in practice part of the specifications for the actual ASIC. In 
> essence, we looked at the flash chips we wanted to use with the ASIC, and 
> realized they all conformed to ONFi mode 2. Our vendor then provided us 
> with mode 2 timing values that they guaranteed would work with the chip. 
> (In addition, mode 0 is used during initial boot so minimize the 
> likeleyhood that something would go wrong, given that the boot loader is 
> in a ROM.) Especially given that there was no mtd timing setup 
> infrastructure at the time, we had no interest in how the values were 
> derived, or to support more modes.
> 
> Another issue is how to model the internal delays within the ASIC between 
> the NAND controller IP and the pads, which are also part of the equation.
> 
> I don't know if our case is unique, but at least it is an example of a 
> situation in which runtime calculation of the timing register values is 
> problematic.

Okay. I guess we could add the associated ONFI timing mode to the
nand_data_interface struct (it would represent the closest one if the
NAND is not ONFI and some vendor specific implementation decided to
tweak the timing values).

> 
> > > There might be other reasons where the driver might want to know the mode 
> > > number.  
> > 
> > Do you have real examples?  
> 
> No, it just seemed like a reasonable concept, given that 
> nand_init_data_interface() actually does this mapping anyway, so it might 
> as well inform chip->setup_data_interface() about it, since the 
> information is right there anyway.
> 
> > We could pass the ONFI timing mode here, but some timings are not
> > defined using this mode (like tCCS), and I'm not sure we have the same
> > mapping between ONFI and JEDEC modes. The other thing is that I wanted
> > to leave the door open for vendor specific timing definitions which do
> > not exactly match one of the timing mode defined in the ONFI spec.
> > 
> > That's why I didn't bother exposing the ONFI timing mode to the NAND
> > controller ->setup_data_interface() implementation.  
> 
> Ok, so the reason is really future expansion, where ONFi modes 0..5 may 
> not be the only modes to be configured. Fair enough.

Yep.