[PATCH v3 0/9] mtd: nand: add sunxi NAND Flash Controller support

[PATCH v3 0/9] mtd: nand: add sunxi NAND Flash Controller support

[Boris BREZILLON]

This series adds support for the sunxi NAND Flash Controller (NFC).

I'll split this series for the next round:
1) NAND timings
2) sunxi NAND controller driver

But I'd like to have some feedback on the NAND timings stuff before doing this.

Any review on the sunxi driver is welcome (for the moment I only got feedback
on the NAND timings code).

I also have patches adding support for HW randomizer and per ECC partition
config that you can find here:
https://groups.google.com/forum/#!searchin/linux-sunxi/boris/linux-sunxi/s3lBb01I0Js/Bem2X1wKPa4J
But I'd like to get the sunxi NAND driver mainlined before adding more
intrusive parts.

Best Regards,

Boris

Changes since v2:
 - merge HW ECC implementation in base implementation patch
 - fix timing config when interfacing with an ONFI compatible chip

Changes since v1:
 - add HW ECC support
 - rework NAND timings retrieval (use ONFI timing mode instead of raw timings)
 - add nand-ecc-level property to specify NAND ECC requirements from DT


Boris BREZILLON (9):
  mtd: nand: define struct nand_timings
  mtd: nand: add ONFI timing mode to nand_timings converter
  of: mtd: add NAND timing mode retrieval support
  of: mtd: add documentation for the ONFI NAND timing mode property
  mtd: nand: add sunxi NAND flash controller support
  mtd: nand: add sunxi NFC dt bindings doc
  ARM: dt/sunxi: add NFC node to Allwinner A20 SoC
  ARM: dt/sunxi: add A20 NAND controller pin definitions
  ARM: sunxi/dt: enable NAND on cubietruck board

 Documentation/devicetree/bindings/mtd/nand.txt     |    8 +
 .../devicetree/bindings/mtd/sunxi-nand.txt         |   48 +
 arch/arm/boot/dts/sun7i-a20-cubietruck.dts         |   17 +
 arch/arm/boot/dts/sun7i-a20.dtsi                   |   91 ++
 drivers/mtd/nand/Kconfig                           |    6 +
 drivers/mtd/nand/Makefile                          |    3 +-
 drivers/mtd/nand/nand_timings.c                    |  248 ++++
 drivers/mtd/nand/sunxi_nand.c                      | 1276 ++++++++++++++++++++
 drivers/of/of_mtd.c                                |   19 +
 include/linux/mtd/nand.h                           |   53 +
 include/linux/of_mtd.h                             |    8 +
 11 files changed, 1776 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/mtd/sunxi-nand.txt
 create mode 100644 drivers/mtd/nand/nand_timings.c
 create mode 100644 drivers/mtd/nand/sunxi_nand.c

-- 
1.7.9.5

[PATCH v3 1/9] mtd: nand: define struct nand_timings

[Boris BREZILLON]

Define a struct containing the standard NAND timings as described in NAND
datasheets.

Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
---
 include/linux/mtd/nand.h |   49 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 389b3c5..f3ff3a3 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -846,4 +846,53 @@ static inline bool nand_is_slc(struct nand_chip *chip)
 {
 	return chip->bits_per_cell == 1;
 }
+
+/**
+ * struct nand_sdr_timings - SDR NAND chip timings
+ *
+ * This struct defines the timing requirements of a SDR NAND chip.
+ * These informations can be found in every NAND datasheets and the timings
+ * meaning are described in the ONFI specifications:
+ * www.onfi.org/~/media/ONFI/specs/onfi_3_1_spec.pdf? (chapter 4.15 Timing
+ * Parameters)
+ *
+ */
+
+struct nand_sdr_timings {
+	u32 tALH_min;
+	u32 tADL_min;
+	u32 tALS_min;
+	u32 tAR_min;
+	u32 tCEA_max;
+	u32 tCEH_min;
+	u32 tCH_min;
+	u32 tCHZ_max;
+	u32 tCLH_min;
+	u32 tCLR_min;
+	u32 tCLS_min;
+	u32 tCOH_min;
+	u32 tCS_min;
+	u32 tDH_min;
+	u32 tDS_min;
+	u32 tFEAT_max;
+	u32 tIR_min;
+	u32 tITC_max;
+	u32 tRC_min;
+	u32 tREA_max;
+	u32 tREH_min;
+	u32 tRHOH_min;
+	u32 tRHW_min;
+	u32 tRHZ_max;
+	u32 tRLOH_min;
+	u32 tRP_min;
+	u32 tRR_min;
+	u64 tRST_max;
+	u32 tWB_max;
+	u32 tWC_min;
+	u32 tWH_min;
+	u32 tWHR_min;
+	u32 tWP_min;
+	u32 tWW_min;
+};
+
 #endif /* __LINUX_MTD_NAND_H */
-- 
1.7.9.5

[PATCH v3 1/9] mtd: nand: define struct nand_timings

[Brian Norris]

Hi Boris,

On Wed, Mar 12, 2014 at 07:07:36PM +0100, Boris BREZILLON wrote:
> Define a struct containing the standard NAND timings as described in NAND
> datasheets.
> 
> Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
> ---
>  include/linux/mtd/nand.h |   49 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 389b3c5..f3ff3a3 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -846,4 +846,53 @@ static inline bool nand_is_slc(struct nand_chip *chip)
>  {
>  	return chip->bits_per_cell == 1;
>  }
> +
> +/**
> + * struct nand_sdr_timings - SDR NAND chip timings
> + *
> + * This struct defines the timing requirements of a SDR NAND chip.
> + * These informations can be found in every NAND datasheets and the timings
> + * meaning are described in the ONFI specifications:
> + * www.onfi.org/~/media/ONFI/specs/onfi_3_1_spec.pdf? (chapter 4.15 Timing

Can you remove the unicode U+200E character?

> + * Parameters)

Please document the units for these fields here. It looks like you're
using picoseconds.

> + *
> + */
> +

Extra blank line.

> +struct nand_sdr_timings {
> +	u32 tALH_min;
> +	u32 tADL_min;
> +	u32 tALS_min;
> +	u32 tAR_min;
> +	u32 tCEA_max;
> +	u32 tCEH_min;
> +	u32 tCH_min;
> +	u32 tCHZ_max;
> +	u32 tCLH_min;
> +	u32 tCLR_min;
> +	u32 tCLS_min;
> +	u32 tCOH_min;
> +	u32 tCS_min;
> +	u32 tDH_min;
> +	u32 tDS_min;
> +	u32 tFEAT_max;
> +	u32 tIR_min;
> +	u32 tITC_max;
> +	u32 tRC_min;
> +	u32 tREA_max;
> +	u32 tREH_min;
> +	u32 tRHOH_min;
> +	u32 tRHW_min;
> +	u32 tRHZ_max;
> +	u32 tRLOH_min;
> +	u32 tRP_min;
> +	u32 tRR_min;
> +	u64 tRST_max;
> +	u32 tWB_max;
> +	u32 tWC_min;
> +	u32 tWH_min;
> +	u32 tWHR_min;
> +	u32 tWP_min;
> +	u32 tWW_min;
> +};
> +
>  #endif /* __LINUX_MTD_NAND_H */

Brian

[PATCH v3 1/9] mtd: nand: define struct nand_timings

[Boris BREZILLON]

Hi Brian,

On 30/04/2014 19:51, Brian Norris wrote:
> Hi Boris,
>
> On Wed, Mar 12, 2014 at 07:07:36PM +0100, Boris BREZILLON wrote:
>> +
>> +/**
>> + * struct nand_sdr_timings - SDR NAND chip timings
>> + *
>> + * This struct defines the timing requirements of a SDR NAND chip.
>> + * These informations can be found in every NAND datasheets and the timings
>> + * meaning are described in the ONFI specifications:
>> + * www.onfi.org/~/media/ONFI/specs/onfi_3_1_spec.pdf? (chapter 4.15 Timing
> Can you remove the unicode U+200E character?

Sure

>
>> + * Parameters)
> Please document the units for these fields here. It looks like you're
> using picoseconds.

I'll add field units (which are indeed picoseconds) to this comment.

Best Regards,

Boris

[PATCH v3 1/9] mtd: nand: define struct nand_timings

[Lee Jones]

> > Define a struct containing the standard NAND timings as described in NAND
> > datasheets.
> > 
> > Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
> > ---
> >  include/linux/mtd/nand.h |   49 ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> > 
> > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> > index 389b3c5..f3ff3a3 100644
> > --- a/include/linux/mtd/nand.h
> > +++ b/include/linux/mtd/nand.h

[...]

> > + * Parameters)
> 
> Please document the units for these fields here. It looks like you're
> using picoseconds.

Can't we leave this open to interpretation?  For instance, it's more
convenient for our driver to handle these as nano second values.

> > + *
> > + */
> > +
> 
> Extra blank line.
> 
> > +struct nand_sdr_timings {
> > +	u32 tALH_min;
> > +	u32 tADL_min;
> > +	u32 tALS_min;
> > +	u32 tAR_min;
> > +	u32 tCEA_max;
> > +	u32 tCEH_min;
> > +	u32 tCH_min;
> > +	u32 tCHZ_max;
> > +	u32 tCLH_min;
> > +	u32 tCLR_min;
> > +	u32 tCLS_min;
> > +	u32 tCOH_min;
> > +	u32 tCS_min;

	u32 tCSD_min;

> > +	u32 tDH_min;
> > +	u32 tDS_min;
> > +	u32 tFEAT_max;
> > +	u32 tIR_min;
> > +	u32 tITC_max;

	u32 tR_max;

> > +	u32 tRC_min;
> > +	u32 tREA_max;
> > +	u32 tREH_min;
> > +	u32 tRHOH_min;
> > +	u32 tRHW_min;
> > +	u32 tRHZ_max;
> > +	u32 tRLOH_min;
> > +	u32 tRP_min;
> > +	u32 tRR_min;
> > +	u64 tRST_max;
> > +	u32 tWB_max;
> > +	u32 tWC_min;
> > +	u32 tWH_min;
> > +	u32 tWHR_min;
> > +	u32 tWP_min;
> > +	u32 tWW_min;
> > +};
> > +
> >  #endif /* __LINUX_MTD_NAND_H */

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v3 1/9] mtd: nand: define struct nand_timings

[Boris BREZILLON]

On 08/05/2014 16:29, Lee Jones wrote:
>>> Define a struct containing the standard NAND timings as described in NAND
>>> datasheets.
>>>
>>> Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
>>> ---
>>>  include/linux/mtd/nand.h |   49 ++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 49 insertions(+)
>>>
>>> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
>>> index 389b3c5..f3ff3a3 100644
>>> --- a/include/linux/mtd/nand.h
>>> +++ b/include/linux/mtd/nand.h
> [...]
>
>>> + * Parameters)
>> Please document the units for these fields here. It looks like you're
>> using picoseconds.
> Can't we leave this open to interpretation?  For instance, it's more
> convenient for our driver to handle these as nano second values.
>
>>> + *
>>> + */
>>> +
>> Extra blank line.
>>
>>> +struct nand_sdr_timings {
>>> +	u32 tALH_min;
>>> +	u32 tADL_min;
>>> +	u32 tALS_min;
>>> +	u32 tAR_min;
>>> +	u32 tCEA_max;
>>> +	u32 tCEH_min;
>>> +	u32 tCH_min;
>>> +	u32 tCHZ_max;
>>> +	u32 tCLH_min;
>>> +	u32 tCLR_min;
>>> +	u32 tCLS_min;
>>> +	u32 tCOH_min;
>>> +	u32 tCS_min;
> 	u32 tCSD_min;

This is related to ddr2 timings, and this structure only define sdr
related timings.

>>> +	u32 tDH_min;
>>> +	u32 tDS_min;
>>> +	u32 tFEAT_max;
>>> +	u32 tIR_min;
>>> +	u32 tITC_max;
> 	u32 tR_max;

Actually this one cannot be retrieved from the ONFI timing mode, you'll
have to read bytes 137 and 138 in the parameter page.
Hence I got rid of it in the first place. Then I considered adding all
those missing timings in another structure (see [1]).

I noticed you posted something similar on the MTD mailing list (and
Lucas was waiting for the feature too).
Could we work together to propose something that fulfills all our needs ?

Here's what I need:
- get detailled timings to be able to configure the sunxi NAND
controller appropriately
- a way to get these timings on non-ONFi NANDs (the proposed solution is
exposing the closest  supported ONFI timing mode in the NAND chip DT
node, but I'm open to any new proposal).


Best Regards,

Boris

[1] http://lists.infradead.org/pipermail/linux-mtd/2014-March/052525.html


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

[PATCH v3 1/9] mtd: nand: define struct nand_timings

[Brian Norris]

On Thu, May 08, 2014 at 03:29:30PM +0100, Lee Jones wrote:
> > 
> > Please document the units for these fields here. It looks like you're
> > using picoseconds.
> 
> Can't we leave this open to interpretation?  For instance, it's more
> convenient for our driver to handle these as nano second values.

No, their values will be determined by the nand_base core, and we must
have something consistent for drivers to rely on. However, I don't
really have a hard preference on nanoseconds versus picoseconds. If we
see that many of the values reach low-digit nanosecons, or fractional
nanoseconds, it probably makes sense to have the higher resolution.

> > > +struct nand_sdr_timings {
...
> > > +	u32 tCS_min;
> 
> 	u32 tCSD_min;

Is this a suggested addition, Lee? I agree with Boris that this looks
like a DDR mode, which should not be covered here.

> > > +	u32 tDH_min;
> > > +	u32 tDS_min;
> > > +	u32 tFEAT_max;
> > > +	u32 tIR_min;
> > > +	u32 tITC_max;
> 
> 	u32 tR_max;

Same here, is this a suggested new field? If you need it, then we can
follow up like Boris suggested, with a different method, since tR is not
part of the electrical parameters of the timing mode.

...
> > > +};
> > > +
> > >  #endif /* __LINUX_MTD_NAND_H */

Brian

[PATCH v3 2/9] mtd: nand: add ONFI timing mode to nand_timings converter

[Boris BREZILLON]

Add a converter to retrieve NAND timings from an ONFI NAND timing mode.
This only support SDR NAND timings for now.

Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
---
 drivers/mtd/nand/Makefile       |    2 +-
 drivers/mtd/nand/nand_timings.c |  248 +++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/nand.h        |    4 +
 3 files changed, 253 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mtd/nand/nand_timings.c

diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index c970ce7..0b8a822 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -2,7 +2,7 @@
 # linux/drivers/nand/Makefile
 #
 
-obj-$(CONFIG_MTD_NAND)			+= nand.o
+obj-$(CONFIG_MTD_NAND)			+= nand.o nand_timings.o
 obj-$(CONFIG_MTD_NAND_ECC)		+= nand_ecc.o
 obj-$(CONFIG_MTD_NAND_BCH)		+= nand_bch.o
 obj-$(CONFIG_MTD_NAND_IDS)		+= nand_ids.o nand_hynix.o
diff --git a/drivers/mtd/nand/nand_timings.c b/drivers/mtd/nand/nand_timings.c
new file mode 100644
index 0000000..f66fe95
--- /dev/null
+++ b/drivers/mtd/nand/nand_timings.c
@@ -0,0 +1,248 @@
+/*
+ *  Copyright (C) 2014 Boris BREZILLON <b.brezillon.dev@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+#include <linux/mtd/nand.h>
+
+static const struct nand_sdr_timings onfi_sdr_timings[] = {
+	/* Mode 0 */
+	{
+		.tADL_min = 200000,
+		.tALH_min = 20000,
+		.tALS_min = 50000,
+		.tAR_min = 25000,
+		.tCEA_max = 100000,
+		.tCEH_min = 20000,
+		.tCH_min = 20000,
+		.tCHZ_max = 100000,
+		.tCLH_min = 20000,
+		.tCLR_min = 20000,
+		.tCLS_min = 50000,
+		.tCOH_min = 0,
+		.tCS_min = 70000,
+		.tDH_min = 20000,
+		.tDS_min = 40000,
+		.tFEAT_max = 1000000,
+		.tIR_min = 10000,
+		.tITC_max = 1000000,
+		.tRC_min = 100000,
+		.tREA_max = 40000,
+		.tREH_min = 30000,
+		.tRHOH_min = 0,
+		.tRHW_min = 200000,
+		.tRHZ_max = 200000,
+		.tRLOH_min = 0,
+		.tRP_min = 50000,
+		.tRST_max = 250000000000,
+		.tWB_max = 200000,
+		.tRR_min = 40000,
+		.tWC_min = 100000,
+		.tWH_min = 30000,
+		.tWHR_min = 120000,
+		.tWP_min = 50000,
+		.tWW_min = 100000,
+	},
+	/* Mode 1 */
+	{
+		.tADL_min = 100000,
+		.tALH_min = 10000,
+		.tALS_min = 25000,
+		.tAR_min = 10000,
+		.tCEA_max = 45000,
+		.tCEH_min = 20000,
+		.tCH_min = 10000,
+		.tCHZ_max = 50000,
+		.tCLH_min = 10000,
+		.tCLR_min = 10000,
+		.tCLS_min = 25000,
+		.tCOH_min = 15000,
+		.tCS_min = 35000,
+		.tDH_min = 10000,
+		.tDS_min = 20000,
+		.tFEAT_max = 1000000,
+		.tIR_min = 0,
+		.tITC_max = 1000000,
+		.tRC_min = 50000,
+		.tREA_max = 30000,
+		.tREH_min = 15000,
+		.tRHOH_min = 15000,
+		.tRHW_min = 100000,
+		.tRHZ_max = 100000,
+		.tRLOH_min = 0,
+		.tRP_min = 25000,
+		.tRR_min = 20000,
+		.tRST_max = 500000000,
+		.tWB_max = 100000,
+		.tWC_min = 45000,
+		.tWH_min = 15000,
+		.tWHR_min = 80000,
+		.tWP_min = 25000,
+		.tWW_min = 100000,
+	},
+	/* Mode 2 */
+	{
+		.tADL_min = 100000,
+		.tALH_min = 10000,
+		.tALS_min = 15000,
+		.tAR_min = 10000,
+		.tCEA_max = 30000,
+		.tCEH_min = 20000,
+		.tCH_min = 10000,
+		.tCHZ_max = 50000,
+		.tCLH_min = 10000,
+		.tCLR_min = 10000,
+		.tCLS_min = 15000,
+		.tCOH_min = 15000,
+		.tCS_min = 25000,
+		.tDH_min = 5000,
+		.tDS_min = 15000,
+		.tFEAT_max = 1000000,
+		.tIR_min = 0,
+		.tITC_max = 1000000,
+		.tRC_min = 35000,
+		.tREA_max = 25000,
+		.tREH_min = 15000,
+		.tRHOH_min = 15000,
+		.tRHW_min = 100000,
+		.tRHZ_max = 100000,
+		.tRLOH_min = 0,
+		.tRR_min = 20000,
+		.tRST_max = 500000000,
+		.tWB_max = 100000,
+		.tRP_min = 17000,
+		.tWC_min = 35000,
+		.tWH_min = 15000,
+		.tWHR_min = 80000,
+		.tWP_min = 17000,
+		.tWW_min = 100000,
+	},
+	/* Mode 3 */
+	{
+		.tADL_min = 100000,
+		.tALH_min = 5000,
+		.tALS_min = 10000,
+		.tAR_min = 10000,
+		.tCEA_max = 25000,
+		.tCEH_min = 20000,
+		.tCH_min = 5000,
+		.tCHZ_max = 50000,
+		.tCLH_min = 5000,
+		.tCLR_min = 10000,
+		.tCLS_min = 10000,
+		.tCOH_min = 15000,
+		.tCS_min = 25000,
+		.tDH_min = 5000,
+		.tDS_min = 10000,
+		.tFEAT_max = 1000000,
+		.tIR_min = 0,
+		.tITC_max = 1000000,
+		.tRC_min = 30000,
+		.tREA_max = 20000,
+		.tREH_min = 10000,
+		.tRHOH_min = 15000,
+		.tRHW_min = 100000,
+		.tRHZ_max = 100000,
+		.tRLOH_min = 0,
+		.tRP_min = 15000,
+		.tRR_min = 20000,
+		.tRST_max = 500000000,
+		.tWB_max = 100000,
+		.tWC_min = 30000,
+		.tWH_min = 10000,
+		.tWHR_min = 80000,
+		.tWP_min = 15000,
+		.tWW_min = 100000,
+	},
+	/* Mode 4 */
+	{
+		.tADL_min = 70000,
+		.tALH_min = 5000,
+		.tALS_min = 10000,
+		.tAR_min = 10000,
+		.tCEA_max = 25000,
+		.tCEH_min = 20000,
+		.tCH_min = 5000,
+		.tCHZ_max = 30000,
+		.tCLH_min = 5000,
+		.tCLR_min = 10000,
+		.tCLS_min = 10000,
+		.tCOH_min = 15000,
+		.tCS_min = 20000,
+		.tDH_min = 5000,
+		.tDS_min = 10000,
+		.tFEAT_max = 1000000,
+		.tIR_min = 0,
+		.tITC_max = 1000000,
+		.tRC_min = 25000,
+		.tREA_max = 20000,
+		.tREH_min = 10000,
+		.tRHOH_min = 15000,
+		.tRHW_min = 100000,
+		.tRHZ_max = 100000,
+		.tRLOH_min = 5000,
+		.tRP_min = 12000,
+		.tRR_min = 20000,
+		.tRST_max = 500000000,
+		.tWB_max = 100000,
+		.tWC_min = 25000,
+		.tWH_min = 10000,
+		.tWHR_min = 80000,
+		.tWP_min = 12000,
+		.tWW_min = 100000,
+	},
+	/* Mode 5 */
+	{
+		.tADL_min = 70000,
+		.tALH_min = 5000,
+		.tALS_min = 10000,
+		.tAR_min = 10000,
+		.tCEA_max = 25000,
+		.tCEH_min = 20000,
+		.tCH_min = 5000,
+		.tCHZ_max = 30000,
+		.tCLH_min = 5000,
+		.tCLR_min = 10000,
+		.tCLS_min = 10000,
+		.tCOH_min = 15000,
+		.tCS_min = 15000,
+		.tDH_min = 5000,
+		.tDS_min = 7000,
+		.tFEAT_max = 1000000,
+		.tIR_min = 0,
+		.tITC_max = 1000000,
+		.tRC_min = 20000,
+		.tREA_max = 16000,
+		.tREH_min = 7000,
+		.tRHOH_min = 15000,
+		.tRHW_min = 100000,
+		.tRHZ_max = 100000,
+		.tRLOH_min = 5000,
+		.tRP_min = 10000,
+		.tRR_min = 20000,
+		.tRST_max = 500000000,
+		.tWB_max = 100000,
+		.tWC_min = 20000,
+		.tWH_min = 7000,
+		.tWHR_min = 80000,
+		.tWP_min = 10000,
+		.tWW_min = 100000,
+	},
+};
+
+/**
+ * onfi_async_timing_mode_to_sdr_timings - [NAND Interface] Retrieve NAND
+ * timings according to the given ONFI timing mode
+ * @mode: ONFI timing mode
+ */
+const struct nand_sdr_timings *onfi_async_timing_mode_to_sdr_timings(int mode)
+{
+	if (mode < 0 || mode > ARRAY_SIZE(onfi_sdr_timings))
+		return ERR_PTR(-EINVAL);
+
+	return &onfi_sdr_timings[mode];
+}
+EXPORT_SYMBOL(onfi_async_timing_mode_to_sdr_timings);
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index f3ff3a3..b8e3c2b 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -847,6 +847,7 @@ static inline bool nand_is_slc(struct nand_chip *chip)
 	return chip->bits_per_cell == 1;
 }
 
+
 /**
  * struct nand_sdr_timings - SDR NAND chip timings
  *
@@ -895,4 +896,7 @@ struct nand_sdr_timings {
 	u32 tWW_min;
 };
 
+/* convert an ONFI timing mode to its timing characteristics. */
+const struct nand_sdr_timings *onfi_async_timing_mode_to_sdr_timings(int mode);
+
 #endif /* __LINUX_MTD_NAND_H */
-- 
1.7.9.5

[PATCH v3 2/9] mtd: nand: add ONFI timing mode to nand_timings converter

[Brian Norris]

Hi,

On Wed, Mar 12, 2014 at 07:07:37PM +0100, Boris BREZILLON wrote:
> Add a converter to retrieve NAND timings from an ONFI NAND timing mode.
> This only support SDR NAND timings for now.
> 
> Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
> ---
>  drivers/mtd/nand/Makefile       |    2 +-
>  drivers/mtd/nand/nand_timings.c |  248 +++++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/nand.h        |    4 +
>  3 files changed, 253 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/mtd/nand/nand_timings.c
> 
> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index c970ce7..0b8a822 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -2,7 +2,7 @@
>  # linux/drivers/nand/Makefile
>  #
>  
> -obj-$(CONFIG_MTD_NAND)			+= nand.o
> +obj-$(CONFIG_MTD_NAND)			+= nand.o nand_timings.o
>  obj-$(CONFIG_MTD_NAND_ECC)		+= nand_ecc.o
>  obj-$(CONFIG_MTD_NAND_BCH)		+= nand_bch.o
>  obj-$(CONFIG_MTD_NAND_IDS)		+= nand_ids.o nand_hynix.o

This patch doesn't apply to l2-mtd.git. Are you basing this on your
Hynix patches? I thought some (all?) of them were determined
unnecessary. Anyway, please rebase on l2-mtd.git, since there are a
couple of other bits that don't apply properly.

> diff --git a/drivers/mtd/nand/nand_timings.c b/drivers/mtd/nand/nand_timings.c
> new file mode 100644
> index 0000000..f66fe95
> --- /dev/null
> +++ b/drivers/mtd/nand/nand_timings.c
> @@ -0,0 +1,248 @@
> +/*
> + *  Copyright (C) 2014 Boris BREZILLON <b.brezillon.dev@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +#include <linux/mtd/nand.h>
> +
> +static const struct nand_sdr_timings onfi_sdr_timings[] = {
> +	/* Mode 0 */
> +	{
> +		.tADL_min = 200000,
> +		.tALH_min = 20000,
> +		.tALS_min = 50000,
> +		.tAR_min = 25000,
> +		.tCEA_max = 100000,
> +		.tCEH_min = 20000,
> +		.tCH_min = 20000,
> +		.tCHZ_max = 100000,
> +		.tCLH_min = 20000,
> +		.tCLR_min = 20000,
> +		.tCLS_min = 50000,
> +		.tCOH_min = 0,
> +		.tCS_min = 70000,
> +		.tDH_min = 20000,
> +		.tDS_min = 40000,
> +		.tFEAT_max = 1000000,
> +		.tIR_min = 10000,
> +		.tITC_max = 1000000,
> +		.tRC_min = 100000,
> +		.tREA_max = 40000,
> +		.tREH_min = 30000,
> +		.tRHOH_min = 0,
> +		.tRHW_min = 200000,
> +		.tRHZ_max = 200000,
> +		.tRLOH_min = 0,
> +		.tRP_min = 50000,
> +		.tRST_max = 250000000000,

I was initially wary of potential overflow here, but apparently the C
standard (section 6.4.4.1) ensures that literal constants like this will
be promoted to a sufficiently-large type.

> +		.tWB_max = 200000,
> +		.tRR_min = 40000,
> +		.tWC_min = 100000,
> +		.tWH_min = 30000,
> +		.tWHR_min = 120000,
> +		.tWP_min = 50000,
> +		.tWW_min = 100000,
> +	},
> +	/* Mode 1 */
> +	{
> +		.tADL_min = 100000,
> +		.tALH_min = 10000,
> +		.tALS_min = 25000,
> +		.tAR_min = 10000,
> +		.tCEA_max = 45000,
> +		.tCEH_min = 20000,
> +		.tCH_min = 10000,
> +		.tCHZ_max = 50000,
> +		.tCLH_min = 10000,
> +		.tCLR_min = 10000,
> +		.tCLS_min = 25000,
> +		.tCOH_min = 15000,
> +		.tCS_min = 35000,
> +		.tDH_min = 10000,
> +		.tDS_min = 20000,
> +		.tFEAT_max = 1000000,
> +		.tIR_min = 0,
> +		.tITC_max = 1000000,
> +		.tRC_min = 50000,
> +		.tREA_max = 30000,
> +		.tREH_min = 15000,
> +		.tRHOH_min = 15000,
> +		.tRHW_min = 100000,
> +		.tRHZ_max = 100000,
> +		.tRLOH_min = 0,
> +		.tRP_min = 25000,
> +		.tRR_min = 20000,
> +		.tRST_max = 500000000,
> +		.tWB_max = 100000,
> +		.tWC_min = 45000,
> +		.tWH_min = 15000,
> +		.tWHR_min = 80000,
> +		.tWP_min = 25000,
> +		.tWW_min = 100000,
> +	},
> +	/* Mode 2 */
> +	{
> +		.tADL_min = 100000,
> +		.tALH_min = 10000,
> +		.tALS_min = 15000,
> +		.tAR_min = 10000,
> +		.tCEA_max = 30000,
> +		.tCEH_min = 20000,
> +		.tCH_min = 10000,
> +		.tCHZ_max = 50000,
> +		.tCLH_min = 10000,
> +		.tCLR_min = 10000,
> +		.tCLS_min = 15000,
> +		.tCOH_min = 15000,
> +		.tCS_min = 25000,
> +		.tDH_min = 5000,
> +		.tDS_min = 15000,
> +		.tFEAT_max = 1000000,
> +		.tIR_min = 0,
> +		.tITC_max = 1000000,
> +		.tRC_min = 35000,
> +		.tREA_max = 25000,
> +		.tREH_min = 15000,
> +		.tRHOH_min = 15000,
> +		.tRHW_min = 100000,
> +		.tRHZ_max = 100000,
> +		.tRLOH_min = 0,
> +		.tRR_min = 20000,
> +		.tRST_max = 500000000,
> +		.tWB_max = 100000,
> +		.tRP_min = 17000,
> +		.tWC_min = 35000,
> +		.tWH_min = 15000,
> +		.tWHR_min = 80000,
> +		.tWP_min = 17000,
> +		.tWW_min = 100000,
> +	},
> +	/* Mode 3 */
> +	{
> +		.tADL_min = 100000,
> +		.tALH_min = 5000,
> +		.tALS_min = 10000,
> +		.tAR_min = 10000,
> +		.tCEA_max = 25000,
> +		.tCEH_min = 20000,
> +		.tCH_min = 5000,
> +		.tCHZ_max = 50000,
> +		.tCLH_min = 5000,
> +		.tCLR_min = 10000,
> +		.tCLS_min = 10000,
> +		.tCOH_min = 15000,
> +		.tCS_min = 25000,
> +		.tDH_min = 5000,
> +		.tDS_min = 10000,
> +		.tFEAT_max = 1000000,
> +		.tIR_min = 0,
> +		.tITC_max = 1000000,
> +		.tRC_min = 30000,
> +		.tREA_max = 20000,
> +		.tREH_min = 10000,
> +		.tRHOH_min = 15000,
> +		.tRHW_min = 100000,
> +		.tRHZ_max = 100000,
> +		.tRLOH_min = 0,
> +		.tRP_min = 15000,
> +		.tRR_min = 20000,
> +		.tRST_max = 500000000,
> +		.tWB_max = 100000,
> +		.tWC_min = 30000,
> +		.tWH_min = 10000,
> +		.tWHR_min = 80000,
> +		.tWP_min = 15000,
> +		.tWW_min = 100000,
> +	},
> +	/* Mode 4 */
> +	{
> +		.tADL_min = 70000,
> +		.tALH_min = 5000,
> +		.tALS_min = 10000,
> +		.tAR_min = 10000,
> +		.tCEA_max = 25000,
> +		.tCEH_min = 20000,
> +		.tCH_min = 5000,
> +		.tCHZ_max = 30000,
> +		.tCLH_min = 5000,
> +		.tCLR_min = 10000,
> +		.tCLS_min = 10000,
> +		.tCOH_min = 15000,
> +		.tCS_min = 20000,
> +		.tDH_min = 5000,
> +		.tDS_min = 10000,
> +		.tFEAT_max = 1000000,
> +		.tIR_min = 0,
> +		.tITC_max = 1000000,
> +		.tRC_min = 25000,
> +		.tREA_max = 20000,
> +		.tREH_min = 10000,
> +		.tRHOH_min = 15000,
> +		.tRHW_min = 100000,
> +		.tRHZ_max = 100000,
> +		.tRLOH_min = 5000,
> +		.tRP_min = 12000,
> +		.tRR_min = 20000,
> +		.tRST_max = 500000000,
> +		.tWB_max = 100000,
> +		.tWC_min = 25000,
> +		.tWH_min = 10000,
> +		.tWHR_min = 80000,
> +		.tWP_min = 12000,
> +		.tWW_min = 100000,
> +	},
> +	/* Mode 5 */
> +	{
> +		.tADL_min = 70000,
> +		.tALH_min = 5000,
> +		.tALS_min = 10000,
> +		.tAR_min = 10000,
> +		.tCEA_max = 25000,
> +		.tCEH_min = 20000,
> +		.tCH_min = 5000,
> +		.tCHZ_max = 30000,
> +		.tCLH_min = 5000,
> +		.tCLR_min = 10000,
> +		.tCLS_min = 10000,
> +		.tCOH_min = 15000,
> +		.tCS_min = 15000,
> +		.tDH_min = 5000,
> +		.tDS_min = 7000,
> +		.tFEAT_max = 1000000,
> +		.tIR_min = 0,
> +		.tITC_max = 1000000,
> +		.tRC_min = 20000,
> +		.tREA_max = 16000,
> +		.tREH_min = 7000,
> +		.tRHOH_min = 15000,
> +		.tRHW_min = 100000,
> +		.tRHZ_max = 100000,
> +		.tRLOH_min = 5000,
> +		.tRP_min = 10000,
> +		.tRR_min = 20000,
> +		.tRST_max = 500000000,
> +		.tWB_max = 100000,
> +		.tWC_min = 20000,
> +		.tWH_min = 7000,
> +		.tWHR_min = 80000,
> +		.tWP_min = 10000,
> +		.tWW_min = 100000,
> +	},
> +};
> +
> +/**
> + * onfi_async_timing_mode_to_sdr_timings - [NAND Interface] Retrieve NAND
> + * timings according to the given ONFI timing mode
> + * @mode: ONFI timing mode
> + */
> +const struct nand_sdr_timings *onfi_async_timing_mode_to_sdr_timings(int mode)
> +{
> +	if (mode < 0 || mode > ARRAY_SIZE(onfi_sdr_timings))

Should be ">=".

	if (mode < 0 || mode >= ARRAY_SIZE(onfi_sdr_timings))

> +		return ERR_PTR(-EINVAL);
> +
> +	return &onfi_sdr_timings[mode];
> +}
> +EXPORT_SYMBOL(onfi_async_timing_mode_to_sdr_timings);
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index f3ff3a3..b8e3c2b 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -847,6 +847,7 @@ static inline bool nand_is_slc(struct nand_chip *chip)
>  	return chip->bits_per_cell == 1;
>  }
>  
> +

Unnecessary whitespace change.

>  /**
>   * struct nand_sdr_timings - SDR NAND chip timings
>   *
> @@ -895,4 +896,7 @@ struct nand_sdr_timings {
>  	u32 tWW_min;
>  };
>  
> +/* convert an ONFI timing mode to its timing characteristics. */
> +const struct nand_sdr_timings *onfi_async_timing_mode_to_sdr_timings(int mode);
> +
>  #endif /* __LINUX_MTD_NAND_H */

Brian

[PATCH v3 2/9] mtd: nand: add ONFI timing mode to nand_timings converter

[Brian Norris]

Hi Boris,

Since I think you were planning on revisiting this soon, I have one more
comment:

On Wed, Mar 12, 2014 at 07:07:37PM +0100, Boris BREZILLON wrote:
> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index c970ce7..0b8a822 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -2,7 +2,7 @@
>  # linux/drivers/nand/Makefile
>  #
>  
> -obj-$(CONFIG_MTD_NAND)			+= nand.o
> +obj-$(CONFIG_MTD_NAND)			+= nand.o nand_timings.o

This is not the right place, as it will generate a new module
'nand_timings' (nand_timings.ko, if CONFIG_MTD_NAND=m). You probably
want to just add nand_timings.o to the 'nand-objs' list at the bottom of
the Makefile, like this:

	nand-objs := nand_base.o nand_bbt.o nand_timings.o

>  obj-$(CONFIG_MTD_NAND_ECC)		+= nand_ecc.o
>  obj-$(CONFIG_MTD_NAND_BCH)		+= nand_bch.o
>  obj-$(CONFIG_MTD_NAND_IDS)		+= nand_ids.o nand_hynix.o

Thanks,
Brian

[PATCH v3 3/9] of: mtd: add NAND timing mode retrieval support

[Boris BREZILLON]

Add a function to retrieve NAND timing mode (ONFI timing mode) from a given
DT node.

Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
---
 drivers/of/of_mtd.c    |   19 +++++++++++++++++++
 include/linux/of_mtd.h |    8 ++++++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/of/of_mtd.c b/drivers/of/of_mtd.c
index b7361ed..8bdaa0b 100644
--- a/drivers/of/of_mtd.c
+++ b/drivers/of/of_mtd.c
@@ -117,3 +117,22 @@ bool of_get_nand_on_flash_bbt(struct device_node *np)
 	return of_property_read_bool(np, "nand-on-flash-bbt");
 }
 EXPORT_SYMBOL_GPL(of_get_nand_on_flash_bbt);
+
+/**
+ * of_get_nand_timings - Get nand timings for the given device_node
+ * @np:	Pointer to the given device_node
+ *
+ * return 0 on success errno other wise
+ */
+int of_get_nand_onfi_timing_mode(struct device_node *np)
+{
+	int err;
+	u32 mode;
+
+	err = of_property_read_u32(np, "onfi,nand-timing-mode", &mode);
+	if (err)
+		return err;
+
+	return mode;
+}
+EXPORT_SYMBOL_GPL(of_get_nand_onfi_timing_mode);
diff --git a/include/linux/of_mtd.h b/include/linux/of_mtd.h
index e266caa..c8310ae 100644
--- a/include/linux/of_mtd.h
+++ b/include/linux/of_mtd.h
@@ -9,6 +9,8 @@
 #ifndef __LINUX_OF_MTD_H
 #define __LINUX_OF_MTD_H
 
+#include <linux/mtd/nand.h>
+
 #ifdef CONFIG_OF_MTD
 
 #include <linux/of.h>
@@ -17,6 +19,7 @@ int of_get_nand_ecc_step_size(struct device_node *np);
 int of_get_nand_ecc_strength(struct device_node *np);
 int of_get_nand_bus_width(struct device_node *np);
 bool of_get_nand_on_flash_bbt(struct device_node *np);
+int of_get_nand_onfi_timing_mode(struct device_node *np);
 
 #else /* CONFIG_OF_MTD */
 
@@ -45,6 +48,11 @@ static inline bool of_get_nand_on_flash_bbt(struct device_node *np)
 	return false;
 }
 
+static inline int of_get_nand_onfi_timing_mode(struct device_node *np)
+{
+	return -ENOSYS;
+}
+
 #endif /* CONFIG_OF_MTD */
 
 #endif /* __LINUX_OF_MTD_H */
-- 
1.7.9.5

[PATCH v3 3/9] of: mtd: add NAND timing mode retrieval support

[Brian Norris]

On Wed, Mar 12, 2014 at 07:07:38PM +0100, Boris BREZILLON wrote:
> Add a function to retrieve NAND timing mode (ONFI timing mode) from a given
> DT node.
> 
> Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
> ---
>  drivers/of/of_mtd.c    |   19 +++++++++++++++++++
>  include/linux/of_mtd.h |    8 ++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/drivers/of/of_mtd.c b/drivers/of/of_mtd.c
> index b7361ed..8bdaa0b 100644
> --- a/drivers/of/of_mtd.c
> +++ b/drivers/of/of_mtd.c
> @@ -117,3 +117,22 @@ bool of_get_nand_on_flash_bbt(struct device_node *np)
>  	return of_property_read_bool(np, "nand-on-flash-bbt");
>  }
>  EXPORT_SYMBOL_GPL(of_get_nand_on_flash_bbt);
> +
> +/**
> + * of_get_nand_timings - Get nand timings for the given device_node
> + * @np:	Pointer to the given device_node
> + *
> + * return 0 on success errno other wise
> + */
> +int of_get_nand_onfi_timing_mode(struct device_node *np)
> +{
> +	int err;
> +	u32 mode;
> +
> +	err = of_property_read_u32(np, "onfi,nand-timing-mode", &mode);
> +	if (err)
> +		return err;
> +
> +	return mode;

To fit the style of the rest of this file (and to save some lines) this
might as well be:

	return ret ? ret : mode;

> +}
> +EXPORT_SYMBOL_GPL(of_get_nand_onfi_timing_mode);
> diff --git a/include/linux/of_mtd.h b/include/linux/of_mtd.h
> index e266caa..c8310ae 100644
> --- a/include/linux/of_mtd.h
> +++ b/include/linux/of_mtd.h
> @@ -9,6 +9,8 @@
>  #ifndef __LINUX_OF_MTD_H
>  #define __LINUX_OF_MTD_H
>  
> +#include <linux/mtd/nand.h>
> +

What's this header used for here? I think you can drop it.

>  #ifdef CONFIG_OF_MTD
>  
>  #include <linux/of.h>
> @@ -17,6 +19,7 @@ int of_get_nand_ecc_step_size(struct device_node *np);
>  int of_get_nand_ecc_strength(struct device_node *np);
>  int of_get_nand_bus_width(struct device_node *np);
>  bool of_get_nand_on_flash_bbt(struct device_node *np);
> +int of_get_nand_onfi_timing_mode(struct device_node *np);
>  
>  #else /* CONFIG_OF_MTD */
>  
> @@ -45,6 +48,11 @@ static inline bool of_get_nand_on_flash_bbt(struct device_node *np)
>  	return false;
>  }
>  
> +static inline int of_get_nand_onfi_timing_mode(struct device_node *np)
> +{
> +	return -ENOSYS;
> +}
> +
>  #endif /* CONFIG_OF_MTD */
>  
>  #endif /* __LINUX_OF_MTD_H */

Brian

[PATCH v3 4/9] of: mtd: add documentation for the ONFI NAND timing mode property

[Boris BREZILLON]

Add documentation for the ONFI NAND timing mode property.

Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
---
 Documentation/devicetree/bindings/mtd/nand.txt |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/nand.txt b/Documentation/devicetree/bindings/mtd/nand.txt
index b53f92e..2046027 100644
--- a/Documentation/devicetree/bindings/mtd/nand.txt
+++ b/Documentation/devicetree/bindings/mtd/nand.txt
@@ -19,3 +19,11 @@ errors per {size} bytes".
 The interpretation of these parameters is implementation-defined, so not all
 implementations must support all possible combinations. However, implementations
 are encouraged to further specify the value(s) they support.
+
+- onfi,nand-timing-mode: an integer encoding the supported ONFI timing modes of
+  the NAND chip. Each supported mode is represented as a bit position (i.e. :
+  mode 0 and 1 => (1 << 0) | (1 << 1) = 0x3).
+  This is only used when the chip does not support the ONFI standard.
+  The last bit set represent the closest mode fulfilling the NAND chip timings.
+  For a full description of the different timing modes see this document:
+  www.onfi.org/~/media/ONFI/specs/onfi_3_1_spec.pdf
-- 
1.7.9.5

[PATCH v3 4/9] of: mtd: add documentation for the ONFI NAND timing mode property

[Warner Losh]

On Mar 12, 2014, at 12:07 PM, Boris BREZILLON <b.brezillon.dev@gmail.com> wrote:

> Add documentation for the ONFI NAND timing mode property.

I don?t see a Toggle/JEDEC mode timing property. Will that be defined for Toshiba, Samsung
and San Disk flash? Or will this be limited to Micron, Intel and Hynix (the only ones
supporting ONFI)?

Warner


> Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
> ---
> Documentation/devicetree/bindings/mtd/nand.txt |    8 ++++++++
> 1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/nand.txt b/Documentation/devicetree/bindings/mtd/nand.txt
> index b53f92e..2046027 100644
> --- a/Documentation/devicetree/bindings/mtd/nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/nand.txt
> @@ -19,3 +19,11 @@ errors per {size} bytes".
> The interpretation of these parameters is implementation-defined, so not all
> implementations must support all possible combinations. However, implementations
> are encouraged to further specify the value(s) they support.
> +
> +- onfi,nand-timing-mode: an integer encoding the supported ONFI timing modes of
> +  the NAND chip. Each supported mode is represented as a bit position (i.e. :
> +  mode 0 and 1 => (1 << 0) | (1 << 1) = 0x3).
> +  This is only used when the chip does not support the ONFI standard.
> +  The last bit set represent the closest mode fulfilling the NAND chip timings.
> +  For a full description of the different timing modes see this document:
> +  www.onfi.org/~/media/ONFI/specs/onfi_3_1_spec.pdf
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 4/9] of: mtd: add documentation for the ONFI NAND timing mode property

[Boris BREZILLON]

Le 12/03/2014 19:27, Warner Losh a ?crit :
> On Mar 12, 2014, at 12:07 PM, Boris BREZILLON <b.brezillon.dev@gmail.com> wrote:
>
>> Add documentation for the ONFI NAND timing mode property.
> I don?t see a Toggle/JEDEC mode timing property. Will that be defined for Toshiba, Samsung
> and San Disk flash? Or will this be limited to Micron, Intel and Hynix (the only ones
> supporting ONFI)?

There is currently no Toggle/JEDEC timing mode support, and I don't know 
what these
timing modes describe.
But I guess they can be converted to standard timings described in 
nand_sdr_timings
struct (or in your case in the future nand_ddr_timings struct).

Could you check that the timings described by these modes use the same 
naming convention
as those defined in the ONFI specification 
(www.onfi.org/~/media/ONFI/specs/onfi_3_1_spec.pdf
  chapter 4.15.4) ?

If this is the case, we can define a new DT property and new converters.

Best Regards,

Boris

> Warner
>
>
>> Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
>> ---
>> Documentation/devicetree/bindings/mtd/nand.txt |    8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/nand.txt b/Documentation/devicetree/bindings/mtd/nand.txt
>> index b53f92e..2046027 100644
>> --- a/Documentation/devicetree/bindings/mtd/nand.txt
>> +++ b/Documentation/devicetree/bindings/mtd/nand.txt
>> @@ -19,3 +19,11 @@ errors per {size} bytes".
>> The interpretation of these parameters is implementation-defined, so not all
>> implementations must support all possible combinations. However, implementations
>> are encouraged to further specify the value(s) they support.
>> +
>> +- onfi,nand-timing-mode: an integer encoding the supported ONFI timing modes of
>> +  the NAND chip. Each supported mode is represented as a bit position (i.e. :
>> +  mode 0 and 1 => (1 << 0) | (1 << 1) = 0x3).
>> +  This is only used when the chip does not support the ONFI standard.
>> +  The last bit set represent the closest mode fulfilling the NAND chip timings.
>> +  For a full description of the different timing modes see this document:
>> +  www.onfi.org/~/media/ONFI/specs/onfi_3_1_spec.pdf
>> -- 
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 4/9] of: mtd: add documentation for the ONFI NAND timing mode property

[Brian Norris]

Hi Boris,

On Wed, Mar 12, 2014 at 07:07:39PM +0100, Boris BREZILLON wrote:
> Add documentation for the ONFI NAND timing mode property.
> 
> Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
> ---
>  Documentation/devicetree/bindings/mtd/nand.txt |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/nand.txt b/Documentation/devicetree/bindings/mtd/nand.txt
> index b53f92e..2046027 100644
> --- a/Documentation/devicetree/bindings/mtd/nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/nand.txt
> @@ -19,3 +19,11 @@ errors per {size} bytes".
>  The interpretation of these parameters is implementation-defined, so not all
>  implementations must support all possible combinations. However, implementations
>  are encouraged to further specify the value(s) they support.
> +
> +- onfi,nand-timing-mode: an integer encoding the supported ONFI timing modes of
> +  the NAND chip. Each supported mode is represented as a bit position (i.e. :
> +  mode 0 and 1 => (1 << 0) | (1 << 1) = 0x3).
> +  This is only used when the chip does not support the ONFI standard.
> +  The last bit set represent the closest mode fulfilling the NAND chip timings.
> +  For a full description of the different timing modes see this document:
> +  www.onfi.org/~/media/ONFI/specs/onfi_3_1_spec.pdf

I'm not 100% convinced this property should go in the device tree. With
most other flash properties (device size, page size, and even minimum
ECC requirements), we try to auto-detect these parameters to some
extent. ONFI makes it easy for some class of chips, but for others, we
typically rely on an in-kernel device ID table or ID decoding heuristic
-- we don't require a DT description of every property of the flash. So
what makes this property different?

I realize that we may not include device ID entries for every flash that
you need in the ID table (although we still are able to detect the
important properties accurately, like page and block size). But would it
suffice to default these flash to a lowest common timing mode, like mode
0?

If no other option works well, then I am still open to describing the
supported timing modes in the DT.

BTW, this bitfield property looks kinda strange to me. Do non-ONFI flash
typically support multiple timing modes? And if so, how are we supposed
to *change* modes? AFAIK, ONFI provides the only standard for
configuring the flash's timing mode. So maybe you're really only wanting
a "default timing mode" property that is a single integer, not a
bitfield.

Regards,
Brian

[PATCH v3 4/9] of: mtd: add documentation for the ONFI NAND timing mode property

[Boris BREZILLON]

Hi Brian,

On 20/05/2014 20:25, Brian Norris wrote:
> Hi Boris,
>
> On Wed, Mar 12, 2014 at 07:07:39PM +0100, Boris BREZILLON wrote:
>> Add documentation for the ONFI NAND timing mode property.
>>
>> Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
>> ---
>>  Documentation/devicetree/bindings/mtd/nand.txt |    8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/nand.txt b/Documentation/devicetree/bindings/mtd/nand.txt
>> index b53f92e..2046027 100644
>> --- a/Documentation/devicetree/bindings/mtd/nand.txt
>> +++ b/Documentation/devicetree/bindings/mtd/nand.txt
>> @@ -19,3 +19,11 @@ errors per {size} bytes".
>>  The interpretation of these parameters is implementation-defined, so not all
>>  implementations must support all possible combinations. However, implementations
>>  are encouraged to further specify the value(s) they support.
>> +
>> +- onfi,nand-timing-mode: an integer encoding the supported ONFI timing modes of
>> +  the NAND chip. Each supported mode is represented as a bit position (i.e. :
>> +  mode 0 and 1 => (1 << 0) | (1 << 1) = 0x3).
>> +  This is only used when the chip does not support the ONFI standard.
>> +  The last bit set represent the closest mode fulfilling the NAND chip timings.
>> +  For a full description of the different timing modes see this document:
>> +  www.onfi.org/~/media/ONFI/specs/onfi_3_1_spec.pdf
> I'm not 100% convinced this property should go in the device tree. With
> most other flash properties (device size, page size, and even minimum
> ECC requirements), we try to auto-detect these parameters to some
> extent. ONFI makes it easy for some class of chips, but for others, we
> typically rely on an in-kernel device ID table or ID decoding heuristic
> -- we don't require a DT description of every property of the flash. So
> what makes this property different?

AFAICT nothing, but the same goes for the ECC requirements, and we've
recently added DT bindings to define these requirements.
I'm not telling we should drop these ECC requirements bindings (actually
I'm using them :-)), but what's different with the timings requirements ?

Moreover, we will end up with a lot of new entries in the device ID
table if we decide to put these informations in this table.

>
> I realize that we may not include device ID entries for every flash that
> you need in the ID table (although we still are able to detect the
> important properties accurately, like page and block size). But would it
> suffice to default these flash to a lowest common timing mode, like mode
> 0?

IMHO this is not a good solution: you'll end up with lower perfomances
on most of the supported NAND chips and I'm not sure this is what we want.

>
> If no other option works well, then I am still open to describing the
> supported timing modes in the DT.
>
> BTW, this bitfield property looks kinda strange to me. Do non-ONFI flash
> typically support multiple timing modes? And if so, how are we supposed
> to *change* modes? AFAIK, ONFI provides the only standard for
> configuring the flash's timing mode. So maybe you're really only wanting
> a "default timing mode" property that is a single integer, not a
> bitfield.

Indeed, I based it on the ONFI NAND timings mode model, but AFAIK (tell
me if I'm wrong), it should work because most of the timings are min
requirements.
This means, even if you provide slower signals transitions, the NAND
will work as expected.

But I can modify the bindings to just encode the maximum supported
timing mode.

Thanks for your feedback.

Best Regards,

Boris


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

[PATCH v3 4/9] of: mtd: add documentation for the ONFI NAND timing mode property

[Jason Gunthorpe]

On Tue, May 20, 2014 at 09:30:33PM +0200, Boris BREZILLON wrote:

> AFAICT nothing, but the same goes for the ECC requirements, and we've
> recently added DT bindings to define these requirements.
> I'm not telling we should drop these ECC requirements bindings (actually
> I'm using them :-)), but what's different with the timings requirements ?

ECC requirements are almost always something that has to be matched to
the bootloader (since the bootloader typicaly reads the NAND to boot),
so it is sensible to put that in the DT

The timings are a property of the chip, and if they can be detected
they should be. IMHO, the main purpose of a DT property would be to
lower the speed if, for some reason, the board cannot support the
device's full speed.

> Indeed, I based it on the ONFI NAND timings mode model, but AFAIK
> (tell me if I'm wrong), it should work because most of the timings
> are min requirements.  This means, even if you provide slower
> signals transitions, the NAND will work as expected.

IIRC for ONFI a device must always work in the mode 0 timings, without
requiring a command?

Jason

[PATCH v3 4/9] of: mtd: add documentation for the ONFI NAND timing mode property

[Brian Norris]

On Tue, May 20, 2014 at 01:51:40PM -0600, Jason Gunthorpe wrote:
> On Tue, May 20, 2014 at 09:30:33PM +0200, Boris BREZILLON wrote:
> > AFAICT nothing, but the same goes for the ECC requirements, and we've
> > recently added DT bindings to define these requirements.
> > I'm not telling we should drop these ECC requirements bindings (actually
> > I'm using them :-)), but what's different with the timings requirements ?
> 
> ECC requirements are almost always something that has to be matched to
> the bootloader (since the bootloader typicaly reads the NAND to boot),
> so it is sensible to put that in the DT

+1 You beat me to this :)

> The timings are a property of the chip, and if they can be detected
> they should be. IMHO, the main purpose of a DT property would be to
> lower the speed if, for some reason, the board cannot support the
> device's full speed.

Agreed.

Now, we still have the open question of whether we can autodetect timing
modes easily for non-ONFI chips.

> > Indeed, I based it on the ONFI NAND timings mode model, but AFAIK
> > (tell me if I'm wrong), it should work because most of the timings
> > are min requirements.  This means, even if you provide slower
> > signals transitions, the NAND will work as expected.
> 
> IIRC for ONFI a device must always work in the mode 0 timings, without
> requiring a command?

I believe so.

FYI, despite the name of the binding, we are mostly interested in
non-ONFI NAND here.

Brian

[PATCH v3 4/9] of: mtd: add documentation for the ONFI NAND timing mode property

[Brian Norris]

On Tue, May 20, 2014 at 09:30:33PM +0200, Boris BREZILLON wrote:
> Hi Brian,
> 
> On 20/05/2014 20:25, Brian Norris wrote:
> > Hi Boris,
> >
> > On Wed, Mar 12, 2014 at 07:07:39PM +0100, Boris BREZILLON wrote:
> >> Add documentation for the ONFI NAND timing mode property.
> >>
> >> Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
> >> ---
> >>  Documentation/devicetree/bindings/mtd/nand.txt |    8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/mtd/nand.txt b/Documentation/devicetree/bindings/mtd/nand.txt
> >> index b53f92e..2046027 100644
> >> --- a/Documentation/devicetree/bindings/mtd/nand.txt
> >> +++ b/Documentation/devicetree/bindings/mtd/nand.txt
> >> @@ -19,3 +19,11 @@ errors per {size} bytes".
> >>  The interpretation of these parameters is implementation-defined, so not all
> >>  implementations must support all possible combinations. However, implementations
> >>  are encouraged to further specify the value(s) they support.
> >> +
> >> +- onfi,nand-timing-mode: an integer encoding the supported ONFI timing modes of
> >> +  the NAND chip. Each supported mode is represented as a bit position (i.e. :
> >> +  mode 0 and 1 => (1 << 0) | (1 << 1) = 0x3).
> >> +  This is only used when the chip does not support the ONFI standard.
> >> +  The last bit set represent the closest mode fulfilling the NAND chip timings.
> >> +  For a full description of the different timing modes see this document:
> >> +  www.onfi.org/~/media/ONFI/specs/onfi_3_1_spec.pdf
> > I'm not 100% convinced this property should go in the device tree. With
> > most other flash properties (device size, page size, and even minimum
> > ECC requirements), we try to auto-detect these parameters to some
> > extent. ONFI makes it easy for some class of chips, but for others, we
> > typically rely on an in-kernel device ID table or ID decoding heuristic
> > -- we don't require a DT description of every property of the flash. So
> > what makes this property different?
> 
> AFAICT nothing, but the same goes for the ECC requirements, and we've
> recently added DT bindings to define these requirements.
> I'm not telling we should drop these ECC requirements bindings (actually
> I'm using them :-)), but what's different with the timings requirements ?

ECC selection is not quite as scientific; with ECC, there are external
factors that influence the ECC mode that you should use, since any data
read/written from Linux has to be compatible with any data read/written
with another entity (e.g., bootloader). Note that the ECC bindings do
not represent a property of the flash chip itself (i.e., they don't hold
the "minimum required ECC strength"), but of the entire flash system
(i.e., "what ECC must I use to play nicely with the rest of the world").

With timing modes, this is purely a property of the flash chip, and we
do not have to synchronize it with the bootloader. We don't exactly care
if a bootloader and Linux use slightly different timing modes.

> Moreover, we will end up with a lot of new entries in the device ID
> table if we decide to put these informations in this table.

Yes, that could be a problem.

What sort of non-ONFI flash chips do you have that need this property?
And what timing mode(s) do they use? Is there, for instance, a pattern
such that all Hynix MLC of a certain generation use a particular timing
mode?

> > I realize that we may not include device ID entries for every flash that
> > you need in the ID table (although we still are able to detect the
> > important properties accurately, like page and block size). But would it
> > suffice to default these flash to a lowest common timing mode, like mode
> > 0?
> 
> IMHO this is not a good solution: you'll end up with lower perfomances
> on most of the supported NAND chips and I'm not sure this is what we want.

No, we wouldn't want to always use mode 0. But it's possible we can get
good enough heuristics for most flash, if we can integrate timing modes
into the current extended ID decoding. Not sure.

I'm also concerned here that this kind of binding will be difficult to
use properly. A user/developer/board-designer would have to read the
datasheet and compare all its values to the ONFI spec to find the
closest match, and they would have to do this for each new flash they
use. If we can help them by autodetecting this, that would be great.

> > If no other option works well, then I am still open to describing the
> > supported timing modes in the DT.
> >
> > BTW, this bitfield property looks kinda strange to me. Do non-ONFI flash
> > typically support multiple timing modes? And if so, how are we supposed
> > to *change* modes? AFAIK, ONFI provides the only standard for
> > configuring the flash's timing mode. So maybe you're really only wanting
> > a "default timing mode" property that is a single integer, not a
> > bitfield.
> 
> Indeed, I based it on the ONFI NAND timings mode model, but AFAIK (tell
> me if I'm wrong), it should work because most of the timings are min
> requirements.
> This means, even if you provide slower signals transitions, the NAND
> will work as expected.

So you're saying that even though the chip actually specifies a single
set of timings, you would describe this as a bitmask of several
supported ONFI timing modes, up to the "max performance"?

Is there ever a case where (for instance) a non-ONFI flash supports the
equivalent of timing mode 3, but it does not support mode 2 or 1?

> But I can modify the bindings to just encode the maximum supported
> timing mode.

AIUI, the non-ONFI datasheets really only specify a single timing mode,
so I think we should only specify the "max." And as a bonus, this
actually makes the binding easier to use. A driver does not care about
how many different modes are supported; it only needs to know what the
max is.

Brian

[PATCH v3 4/9] of: mtd: add documentation for the ONFI NAND timing mode property

[Boris BREZILLON]

On 20/05/2014 21:52, Brian Norris wrote:
> On Tue, May 20, 2014 at 09:30:33PM +0200, Boris BREZILLON wrote:
>> Hi Brian,
>>
>> On 20/05/2014 20:25, Brian Norris wrote:
>>> Hi Boris,
>>>
>>> On Wed, Mar 12, 2014 at 07:07:39PM +0100, Boris BREZILLON wrote:
>>>> Add documentation for the ONFI NAND timing mode property.
>>>>
>>>> Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
>>>> ---
>>>>  Documentation/devicetree/bindings/mtd/nand.txt |    8 ++++++++
>>>>  1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mtd/nand.txt b/Documentation/devicetree/bindings/mtd/nand.txt
>>>> index b53f92e..2046027 100644
>>>> --- a/Documentation/devicetree/bindings/mtd/nand.txt
>>>> +++ b/Documentation/devicetree/bindings/mtd/nand.txt
>>>> @@ -19,3 +19,11 @@ errors per {size} bytes".
>>>>  The interpretation of these parameters is implementation-defined, so not all
>>>>  implementations must support all possible combinations. However, implementations
>>>>  are encouraged to further specify the value(s) they support.
>>>> +
>>>> +- onfi,nand-timing-mode: an integer encoding the supported ONFI timing modes of
>>>> +  the NAND chip. Each supported mode is represented as a bit position (i.e. :
>>>> +  mode 0 and 1 => (1 << 0) | (1 << 1) = 0x3).
>>>> +  This is only used when the chip does not support the ONFI standard.
>>>> +  The last bit set represent the closest mode fulfilling the NAND chip timings.
>>>> +  For a full description of the different timing modes see this document:
>>>> +  www.onfi.org/~/media/ONFI/specs/onfi_3_1_spec.pdf
>>> I'm not 100% convinced this property should go in the device tree. With
>>> most other flash properties (device size, page size, and even minimum
>>> ECC requirements), we try to auto-detect these parameters to some
>>> extent. ONFI makes it easy for some class of chips, but for others, we
>>> typically rely on an in-kernel device ID table or ID decoding heuristic
>>> -- we don't require a DT description of every property of the flash. So
>>> what makes this property different?
>> AFAICT nothing, but the same goes for the ECC requirements, and we've
>> recently added DT bindings to define these requirements.
>> I'm not telling we should drop these ECC requirements bindings (actually
>> I'm using them :-)), but what's different with the timings requirements ?
> ECC selection is not quite as scientific; with ECC, there are external
> factors that influence the ECC mode that you should use, since any data
> read/written from Linux has to be compatible with any data read/written
> with another entity (e.g., bootloader). Note that the ECC bindings do
> not represent a property of the flash chip itself (i.e., they don't hold
> the "minimum required ECC strength"), but of the entire flash system
> (i.e., "what ECC must I use to play nicely with the rest of the world").

If the ECC bindings don't encode the "minimum required ECC strength" but
rather the "ECC config on a specific board" then I guess "minimum
required ECC strength" for non-ONFI chips should be defined somewhere
else (stored in the device ID table ?).

Actually, in the sunxi NAND controller driver, I'm using the DT defined
ECC config when the NAND does not support ONFI timings retrieval.

>
> With timing modes, this is purely a property of the flash chip, and we
> do not have to synchronize it with the bootloader. We don't exactly care
> if a bootloader and Linux use slightly different timing modes.

Agreed.


>
>> Moreover, we will end up with a lot of new entries in the device ID
>> table if we decide to put these informations in this table.
> Yes, that could be a problem.
>
> What sort of non-ONFI flash chips do you have that need this property?

I only have the Hynix one defined in my patch series.
Other people tested my driver on different boards but I don't recall
exactly which NAND they had (a samsung one IIRC).

> And what timing mode(s) do they use? Is there, for instance, a pattern
> such that all Hynix MLC of a certain generation use a particular timing
> mode?

I'll take a look.

>
>>> I realize that we may not include device ID entries for every flash that
>>> you need in the ID table (although we still are able to detect the
>>> important properties accurately, like page and block size). But would it
>>> suffice to default these flash to a lowest common timing mode, like mode
>>> 0?
>> IMHO this is not a good solution: you'll end up with lower perfomances
>> on most of the supported NAND chips and I'm not sure this is what we want.
> No, we wouldn't want to always use mode 0. But it's possible we can get
> good enough heuristics for most flash, if we can integrate timing modes
> into the current extended ID decoding. Not sure.
>
> I'm also concerned here that this kind of binding will be difficult to
> use properly. A user/developer/board-designer would have to read the
> datasheet and compare all its values to the ONFI spec to find the
> closest match, and they would have to do this for each new flash they
> use. If we can help them by autodetecting this, that would be great.
 

>
>>> If no other option works well, then I am still open to describing the
>>> supported timing modes in the DT.
>>>
>>> BTW, this bitfield property looks kinda strange to me. Do non-ONFI flash
>>> typically support multiple timing modes? And if so, how are we supposed
>>> to *change* modes? AFAIK, ONFI provides the only standard for
>>> configuring the flash's timing mode. So maybe you're really only wanting
>>> a "default timing mode" property that is a single integer, not a
>>> bitfield.
>> Indeed, I based it on the ONFI NAND timings mode model, but AFAIK (tell
>> me if I'm wrong), it should work because most of the timings are min
>> requirements.
>> This means, even if you provide slower signals transitions, the NAND
>> will work as expected.
> So you're saying that even though the chip actually specifies a single
> set of timings, you would describe this as a bitmask of several
> supported ONFI timing modes, up to the "max performance"?
>
> Is there ever a case where (for instance) a non-ONFI flash supports the
> equivalent of timing mode 3, but it does not support mode 2 or 1?

I don't think so.

>> But I can modify the bindings to just encode the maximum supported
>> timing mode.
> AIUI, the non-ONFI datasheets really only specify a single timing mode,
> so I think we should only specify the "max." And as a bonus, this
> actually makes the binding easier to use. A driver does not care about
> how many different modes are supported; it only needs to know what the
> max is.

Agreed, actually my first binding was defining it this way.

>
> Brian

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

[PATCH v3 4/9] of: mtd: add documentation for the ONFI NAND timing mode property

[Brian Norris]

Hi Boris,

Looking back at this thread, there's at least one or two things I forgot
to answer. Sorry.

On Tue, May 20, 2014 at 11:32:04PM +0200, Boris BREZILLON wrote:
> On 20/05/2014 21:52, Brian Norris wrote:
[...]
> If the ECC bindings don't encode the "minimum required ECC strength" but
> rather the "ECC config on a specific board" then I guess "minimum
> required ECC strength" for non-ONFI chips should be defined somewhere
> else (stored in the device ID table ?).

They are. See nand_flash_dev::ecc, which holds fields for
ecc_strength_ds and step_ds. If we have to, we can add a "timing mode"
field to this struct.

> > So you're saying that even though the chip actually specifies a single
> > set of timings, you would describe this as a bitmask of several
> > supported ONFI timing modes, up to the "max performance"?
> >
> > Is there ever a case where (for instance) a non-ONFI flash supports the
> > equivalent of timing mode 3, but it does not support mode 2 or 1?
> 
> I don't think so.

OK, then I don't think the mask approach is necessary, if we do ever
settle on using a DT binding here. (I hope we can avoid this.)

> >> But I can modify the bindings to just encode the maximum supported
> >> timing mode.
> > AIUI, the non-ONFI datasheets really only specify a single timing mode,
> > so I think we should only specify the "max." And as a bonus, this
> > actually makes the binding easier to use. A driver does not care about
> > how many different modes are supported; it only needs to know what the
> > max is.
> 
> Agreed, actually my first binding was defining it this way.

Was there a good reason for changing it?

Thanks,
Brian

[PATCH v3 5/9] mtd: nand: add sunxi NAND flash controller support

[Boris BREZILLON]

Add support for the sunxi NAND Flash Controller (NFC).

Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
---
 drivers/mtd/nand/Kconfig      |    6 +
 drivers/mtd/nand/Makefile     |    1 +
 drivers/mtd/nand/sunxi_nand.c | 1276 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 1283 insertions(+)
 create mode 100644 drivers/mtd/nand/sunxi_nand.c

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 90ff447..8a28c06 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -510,4 +510,10 @@ config MTD_NAND_XWAY
 	  Enables support for NAND Flash chips on Lantiq XWAY SoCs. NAND is attached
 	  to the External Bus Unit (EBU).
 
+config MTD_NAND_SUNXI
+	tristate "Support for NAND on Allwinner SoCs"
+	depends on ARCH_SUNXI
+	help
+	  Enables support for NAND Flash chips on Allwinner SoCs.
+
 endif # MTD_NAND
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index 0b8a822..34f45d8 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -49,5 +49,6 @@ obj-$(CONFIG_MTD_NAND_JZ4740)		+= jz4740_nand.o
 obj-$(CONFIG_MTD_NAND_GPMI_NAND)	+= gpmi-nand/
 obj-$(CONFIG_MTD_NAND_XWAY)		+= xway_nand.o
 obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH)	+= bcm47xxnflash/
+obj-$(CONFIG_MTD_NAND_SUNXI)		+= sunxi_nand.o
 
 nand-objs := nand_base.o nand_bbt.o
diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c
new file mode 100644
index 0000000..e93cc44
--- /dev/null
+++ b/drivers/mtd/nand/sunxi_nand.c
@@ -0,0 +1,1276 @@
+/*
+ * Copyright (C) 2013 Boris BREZILLON <b.brezillon.dev@gmail.com>
+ *
+ * Derived from:
+ *	https://github.com/yuq/sunxi-nfc-mtd
+ *	Copyright (C) 2013 Qiang Yu <yuq825@gmail.com>
+ *
+ *	https://github.com/hno/Allwinner-Info
+ *	Copyright (C) 2013 Henrik Nordstr?m <Henrik Nordstr?m>
+ *
+ *	Copyright (C) 2013 Dmitriy B. <rzk333@gmail.com>
+ *	Copyright (C) 2013 Sergey Lapin <slapin@ossfans.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/dma-mapping.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/of_mtd.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/nand.h>
+#include <linux/mtd/partitions.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/dmaengine.h>
+#include <linux/gpio.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+
+#define NFC_REG_CTL		0x0000
+#define NFC_REG_ST		0x0004
+#define NFC_REG_INT		0x0008
+#define NFC_REG_TIMING_CTL	0x000C
+#define NFC_REG_TIMING_CFG	0x0010
+#define NFC_REG_ADDR_LOW	0x0014
+#define NFC_REG_ADDR_HIGH	0x0018
+#define NFC_REG_SECTOR_NUM	0x001C
+#define NFC_REG_CNT		0x0020
+#define NFC_REG_CMD		0x0024
+#define NFC_REG_RCMD_SET	0x0028
+#define NFC_REG_WCMD_SET	0x002C
+#define NFC_REG_IO_DATA		0x0030
+#define NFC_REG_ECC_CTL		0x0034
+#define NFC_REG_ECC_ST		0x0038
+#define NFC_REG_DEBUG		0x003C
+#define NFC_REG_ECC_CNT0	0x0040
+#define NFC_REG_ECC_CNT1	0x0044
+#define NFC_REG_ECC_CNT2	0x0048
+#define NFC_REG_ECC_CNT3	0x004c
+#define NFC_REG_USER_DATA_BASE	0x0050
+#define NFC_REG_SPARE_AREA	0x00A0
+#define NFC_RAM0_BASE		0x0400
+#define NFC_RAM1_BASE		0x0800
+
+/*define bit use in NFC_CTL*/
+#define NFC_EN				(1 << 0)
+#define NFC_RESET			(1 << 1)
+#define NFC_BUS_WIDYH			(1 << 2)
+#define NFC_RB_SEL			(1 << 3)
+#define NFC_CE_SEL			(7 << 24)
+#define NFC_CE_CTL			(1 << 6)
+#define NFC_CE_CTL1			(1 << 7)
+#define NFC_PAGE_SIZE			(0xf << 8)
+#define NFC_SAM				(1 << 12)
+#define NFC_RAM_METHOD			(1 << 14)
+#define NFC_DEBUG_CTL			(1 << 31)
+
+/*define bit use in NFC_ST*/
+#define NFC_RB_B2R			(1 << 0)
+#define NFC_CMD_INT_FLAG		(1 << 1)
+#define NFC_DMA_INT_FLAG		(1 << 2)
+#define NFC_CMD_FIFO_STATUS		(1 << 3)
+#define NFC_STA				(1 << 4)
+#define NFC_NATCH_INT_FLAG		(1 << 5)
+#define NFC_RB_STATE0			(1 << 8)
+#define NFC_RB_STATE1			(1 << 9)
+#define NFC_RB_STATE2			(1 << 10)
+#define NFC_RB_STATE3			(1 << 11)
+
+/*define bit use in NFC_INT*/
+#define NFC_B2R_INT_ENABLE		(1 << 0)
+#define NFC_CMD_INT_ENABLE		(1 << 1)
+#define NFC_DMA_INT_ENABLE		(1 << 2)
+#define NFC_INT_MASK			(NFC_B2R_INT_ENABLE | \
+					 NFC_CMD_INT_ENABLE | \
+					 NFC_DMA_INT_ENABLE)
+
+
+/*define bit use in NFC_CMD*/
+#define NFC_CMD_LOW_BYTE		(0xff << 0)
+#define NFC_CMD_HIGH_BYTE		(0xff << 8)
+#define NFC_ADR_NUM			(0x7 << 16)
+#define NFC_SEND_ADR			(1 << 19)
+#define NFC_ACCESS_DIR			(1 << 20)
+#define NFC_DATA_TRANS			(1 << 21)
+#define NFC_SEND_CMD1			(1 << 22)
+#define NFC_WAIT_FLAG			(1 << 23)
+#define NFC_SEND_CMD2			(1 << 24)
+#define NFC_SEQ				(1 << 25)
+#define NFC_DATA_SWAP_METHOD		(1 << 26)
+#define NFC_ROW_AUTO_INC		(1 << 27)
+#define NFC_SEND_CMD3			(1 << 28)
+#define NFC_SEND_CMD4			(1 << 29)
+#define NFC_CMD_TYPE			(3 << 30)
+
+/* define bit use in NFC_RCMD_SET*/
+#define NFC_READ_CMD			(0xff << 0)
+#define NFC_RANDOM_READ_CMD0		(0xff << 8)
+#define NFC_RANDOM_READ_CMD1		(0xff << 16)
+
+/*define bit use in NFC_WCMD_SET*/
+#define NFC_PROGRAM_CMD			(0xff << 0)
+#define NFC_RANDOM_WRITE_CMD		(0xff << 8)
+#define NFC_READ_CMD0			(0xff << 16)
+#define NFC_READ_CMD1			(0xff << 24)
+
+/*define bit use in NFC_ECC_CTL*/
+#define NFC_ECC_EN			(1 << 0)
+#define NFC_ECC_PIPELINE		(1 << 3)
+#define NFC_ECC_EXCEPTION		(1 << 4)
+#define NFC_ECC_BLOCK_SIZE		(1 << 5)
+#define NFC_RANDOM_EN			(1 << 9)
+#define NFC_RANDOM_DIRECTION		(1 << 10)
+#define NFC_ECC_MODE_SHIFT		12
+#define NFC_ECC_MODE			(0xf << NFC_ECC_MODE_SHIFT)
+#define NFC_RANDOM_SEED			(0x7fff << 16)
+
+
+
+enum sunxi_nand_rb_type {
+	RB_NONE,
+	RB_NATIVE,
+	RB_GPIO,
+};
+
+struct sunxi_nand_rb {
+	enum sunxi_nand_rb_type type;
+	union {
+		int gpio;
+		int nativeid;
+	} info;
+};
+
+struct sunxi_nand_chip_sel {
+	u8 cs;
+	struct sunxi_nand_rb rb;
+};
+
+#define DEFAULT_NAME_FORMAT	"nand@%d"
+#define MAX_NAME_SIZE		(sizeof("nand@") + 2)
+
+struct sunxi_nand_hw_ecc {
+	int mode;
+	struct nand_ecclayout layout;
+};
+
+struct sunxi_nand_chip {
+	struct list_head node;
+	struct nand_chip nand;
+	struct mtd_info mtd;
+	char default_name[MAX_NAME_SIZE];
+	unsigned long clk_rate;
+	int selected;
+	int nsels;
+	struct sunxi_nand_chip_sel sels[0];
+};
+
+static inline struct sunxi_nand_chip *to_sunxi_nand(struct nand_chip *nand)
+{
+	return container_of(nand, struct sunxi_nand_chip, nand);
+}
+
+struct sunxi_nfc {
+	struct nand_hw_control controller;
+	void __iomem *regs;
+	int irq;
+	struct clk *ahb_clk;
+	struct clk *sclk;
+	unsigned long assigned_cs;
+	unsigned long clk_rate;
+	struct list_head chips;
+	struct completion complete;
+};
+
+static inline struct sunxi_nfc *to_sunxi_nfc(struct nand_hw_control *ctrl)
+{
+	return container_of(ctrl, struct sunxi_nfc, controller);
+}
+
+static irqreturn_t sunxi_nfc_interrupt(int irq, void *dev_id)
+{
+	struct sunxi_nfc *nfc = dev_id;
+	u32 st = readl(nfc->regs + NFC_REG_ST);
+	u32 ien = readl(nfc->regs + NFC_REG_INT);
+
+	if (!(ien & st))
+		return IRQ_NONE;
+
+	if ((ien & st) == ien)
+		complete(&nfc->complete);
+
+	writel(st & NFC_INT_MASK, nfc->regs + NFC_REG_ST);
+	writel(~st & ien & NFC_INT_MASK, nfc->regs + NFC_REG_INT);
+
+	return IRQ_HANDLED;
+}
+
+static int sunxi_nfc_wait_int(struct sunxi_nfc *nfc, u32 flags,
+			      unsigned int timeout_ms)
+{
+	init_completion(&nfc->complete);
+
+	writel(flags, nfc->regs + NFC_REG_INT);
+	if (!timeout_ms)
+		wait_for_completion(&nfc->complete);
+	else if (!wait_for_completion_timeout(&nfc->complete,
+					      msecs_to_jiffies(timeout_ms)))
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+static int sunxi_nfc_dev_ready(struct mtd_info *mtd)
+{
+	struct nand_chip *nand = mtd->priv;
+	struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
+	struct sunxi_nfc *nfc = to_sunxi_nfc(sunxi_nand->nand.controller);
+	struct sunxi_nand_rb *rb;
+	unsigned long timeo = (sunxi_nand->nand.state == FL_ERASING ? 400 : 20);
+	int ret;
+
+	if (sunxi_nand->selected < 0)
+		return 0;
+
+	rb = &sunxi_nand->sels[sunxi_nand->selected].rb;
+
+	switch (rb->type) {
+	case RB_NATIVE:
+		ret = !!(readl(nfc->regs + NFC_REG_ST) &
+			 (NFC_RB_STATE0 << rb->info.nativeid));
+		if (ret)
+			break;
+
+		sunxi_nfc_wait_int(nfc, NFC_RB_B2R, timeo);
+		ret = !!(readl(nfc->regs + NFC_REG_ST) &
+			 (NFC_RB_STATE0 << rb->info.nativeid));
+		break;
+	case RB_GPIO:
+		ret = gpio_get_value(rb->info.gpio);
+		break;
+	case RB_NONE:
+	default:
+		ret = 0;
+		pr_err("cannot check R/B NAND status!");
+		break;
+	}
+
+	return ret;
+}
+
+static void sunxi_nfc_select_chip(struct mtd_info *mtd, int chip)
+{
+	struct nand_chip *nand = mtd->priv;
+	struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
+	struct sunxi_nfc *nfc = to_sunxi_nfc(sunxi_nand->nand.controller);
+	struct sunxi_nand_chip_sel *sel;
+	u32 ctl;
+
+	if (chip > 0 && chip >= sunxi_nand->nsels)
+		return;
+
+	if (chip == sunxi_nand->selected)
+		return;
+
+	ctl = readl(nfc->regs + NFC_REG_CTL) &
+	      ~(NFC_CE_SEL | NFC_RB_SEL | NFC_EN);
+
+	if (chip >= 0) {
+		sel = &sunxi_nand->sels[chip];
+
+		ctl |= (sel->cs << 24) | NFC_EN |
+		       (((nand->page_shift - 10) & 0xf) << 8);
+		if (sel->rb.type == RB_NONE) {
+			nand->dev_ready = NULL;
+		} else {
+			nand->dev_ready = sunxi_nfc_dev_ready;
+			if (sel->rb.type == RB_NATIVE)
+				ctl |= (sel->rb.info.nativeid << 3);
+		}
+
+		writel(mtd->writesize, nfc->regs + NFC_REG_SPARE_AREA);
+
+		if (nfc->clk_rate != sunxi_nand->clk_rate) {
+			clk_set_rate(nfc->sclk, sunxi_nand->clk_rate);
+			nfc->clk_rate = sunxi_nand->clk_rate;
+		}
+	}
+
+	writel(ctl, nfc->regs + NFC_REG_CTL);
+
+	sunxi_nand->selected = chip;
+}
+
+static void sunxi_nfc_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
+{
+	struct nand_chip *nand = mtd->priv;
+	struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
+	struct sunxi_nfc *nfc = to_sunxi_nfc(sunxi_nand->nand.controller);
+	int cnt;
+	int offs = 0;
+	u32 tmp;
+
+	while (len > offs) {
+		cnt = len - offs;
+		if (cnt > 1024)
+			cnt = 1024;
+
+		while ((readl(nfc->regs + NFC_REG_ST) & NFC_CMD_FIFO_STATUS))
+			;
+		writel(cnt, nfc->regs + NFC_REG_CNT);
+		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD;
+		writel(tmp, nfc->regs + NFC_REG_CMD);
+		sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
+		if (buf)
+			memcpy_fromio(buf + offs, nfc->regs + NFC_RAM0_BASE,
+				      cnt);
+		offs += cnt;
+	}
+}
+
+static void sunxi_nfc_write_buf(struct mtd_info *mtd, const uint8_t *buf,
+				int len)
+{
+	struct nand_chip *nand = mtd->priv;
+	struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
+	struct sunxi_nfc *nfc = to_sunxi_nfc(sunxi_nand->nand.controller);
+	int cnt;
+	int offs = 0;
+	u32 tmp;
+
+	while (len > offs) {
+		cnt = len - offs;
+		if (cnt > 1024)
+			cnt = 1024;
+
+		while ((readl(nfc->regs + NFC_REG_ST) & NFC_CMD_FIFO_STATUS))
+			;
+		writel(cnt, nfc->regs + NFC_REG_CNT);
+		memcpy_toio(nfc->regs + NFC_RAM0_BASE, buf + offs, cnt);
+		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD |
+		      NFC_ACCESS_DIR;
+		writel(tmp, nfc->regs + NFC_REG_CMD);
+		sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
+		offs += cnt;
+	}
+}
+
+static uint8_t sunxi_nfc_read_byte(struct mtd_info *mtd)
+{
+	uint8_t ret;
+
+	sunxi_nfc_read_buf(mtd, &ret, 1);
+
+	return ret;
+}
+
+static void sunxi_nfc_cmd_ctrl(struct mtd_info *mtd, int dat,
+			       unsigned int ctrl)
+{
+	struct nand_chip *nand = mtd->priv;
+	struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
+	struct sunxi_nfc *nfc = to_sunxi_nfc(sunxi_nand->nand.controller);
+	u32 tmp;
+
+	while ((readl(nfc->regs + NFC_REG_ST) & NFC_CMD_FIFO_STATUS))
+		;
+
+	if (ctrl & NAND_CTRL_CHANGE) {
+		tmp = readl(nfc->regs + NFC_REG_CTL);
+		if (ctrl & NAND_NCE)
+			tmp |= NFC_CE_CTL;
+		else
+			tmp &= ~NFC_CE_CTL;
+		writel(tmp, nfc->regs + NFC_REG_CTL);
+	}
+
+	if (dat == NAND_CMD_NONE)
+		return;
+
+	if (ctrl & NAND_CLE) {
+		writel(NFC_SEND_CMD1 | dat, nfc->regs + NFC_REG_CMD);
+	} else {
+		writel(dat, nfc->regs + NFC_REG_ADDR_LOW);
+		writel(NFC_SEND_ADR, nfc->regs + NFC_REG_CMD);
+	}
+
+	sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
+}
+
+static int sunxi_nfc_hw_ecc_read_page(struct mtd_info *mtd,
+				      struct nand_chip *chip, uint8_t *buf,
+				      int oob_required, int page)
+{
+	struct sunxi_nfc *nfc = to_sunxi_nfc(chip->controller);
+	struct nand_ecc_ctrl *ecc = &chip->ecc;
+	struct nand_ecclayout *layout = ecc->layout;
+	struct sunxi_nand_hw_ecc *data = ecc->priv;
+	int steps = mtd->writesize / ecc->size;
+	unsigned int max_bitflips = 0;
+	int offset;
+	u32 tmp;
+	int i;
+	int cnt;
+
+	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
+	tmp &= ~(NFC_ECC_MODE | NFC_ECC_PIPELINE | NFC_ECC_BLOCK_SIZE |
+		 NFC_ECC_BLOCK_SIZE);
+	tmp |= NFC_ECC_EN | (data->mode << NFC_ECC_MODE_SHIFT);
+
+	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
+
+	for (i = 0; i < steps; i++) {
+		if (i)
+			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, i * ecc->size, -1);
+
+		offset = mtd->writesize + layout->eccpos[i * ecc->bytes] - 4;
+
+		chip->read_buf(mtd, NULL, ecc->size);
+
+		chip->cmdfunc(mtd, NAND_CMD_RNDOUT, offset, -1);
+		while ((readl(nfc->regs + NFC_REG_ST) & NFC_CMD_FIFO_STATUS))
+			;
+
+		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | (1 << 30);
+		writel(tmp, nfc->regs + NFC_REG_CMD);
+		sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
+		memcpy_fromio(buf + (i * ecc->size),
+			      nfc->regs + NFC_RAM0_BASE, ecc->size);
+
+		if (readl(nfc->regs + NFC_REG_ECC_ST) & 0x1) {
+			mtd->ecc_stats.failed++;
+		} else {
+			tmp = readl(nfc->regs + NFC_REG_ECC_CNT0) & 0xff;
+			mtd->ecc_stats.corrected += tmp;
+			max_bitflips = max_t(unsigned int, max_bitflips, tmp);
+		}
+
+		if (oob_required) {
+			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, offset, -1);
+			while ((readl(nfc->regs + NFC_REG_ST) &
+			       NFC_CMD_FIFO_STATUS))
+				;
+			offset -= mtd->writesize;
+			chip->read_buf(mtd, chip->oob_poi + offset,
+				      ecc->bytes + 4);
+		}
+	}
+
+	if (oob_required) {
+		cnt = ecc->layout->oobfree[0].length - 4;
+		if (cnt > 0) {
+			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, mtd->writesize,
+				      -1);
+			chip->read_buf(mtd, chip->oob_poi, cnt);
+		}
+	}
+
+	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
+	tmp &= ~NFC_ECC_EN;
+
+	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
+
+	return max_bitflips;
+}
+
+static int sunxi_nfc_hw_ecc_write_page(struct mtd_info *mtd,
+				       struct nand_chip *chip,
+				       const uint8_t *buf, int oob_required)
+{
+	struct sunxi_nfc *nfc = to_sunxi_nfc(chip->controller);
+	struct nand_ecc_ctrl *ecc = &chip->ecc;
+	struct nand_ecclayout *layout = ecc->layout;
+	struct sunxi_nand_hw_ecc *data = ecc->priv;
+	int offset;
+	u32 tmp;
+	int i;
+	int cnt;
+
+	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
+	tmp &= ~(NFC_ECC_MODE | NFC_ECC_PIPELINE | NFC_ECC_BLOCK_SIZE |
+		 NFC_ECC_BLOCK_SIZE);
+	tmp |= NFC_ECC_EN | (data->mode << NFC_ECC_MODE_SHIFT);
+
+	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
+
+	for (i = 0; i < mtd->writesize / ecc->size; i++) {
+		if (i)
+			chip->cmdfunc(mtd, NAND_CMD_RNDIN, i * ecc->size, -1);
+
+		chip->write_buf(mtd, buf + (i * ecc->size), ecc->size);
+
+		offset = layout->eccpos[i * ecc->bytes] - 4 + mtd->writesize;
+
+		/* Fill OOB data in */
+		if (oob_required) {
+			tmp = 0xffffffff;
+			memcpy_toio(nfc->regs + NFC_REG_USER_DATA_BASE, &tmp,
+				    4);
+		} else {
+			memcpy_toio(nfc->regs + NFC_REG_USER_DATA_BASE,
+				    chip->oob_poi + offset - mtd->writesize,
+				    4);
+		}
+
+		chip->cmdfunc(mtd, NAND_CMD_RNDIN, offset, -1);
+		while ((readl(nfc->regs + NFC_REG_ST) &
+		       NFC_CMD_FIFO_STATUS))
+			;
+
+		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | NFC_ACCESS_DIR |
+		      (1 << 30);
+		writel(tmp, nfc->regs + NFC_REG_CMD);
+		sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
+	}
+
+	if (oob_required) {
+		cnt = ecc->layout->oobfree[0].length - 4;
+		if (cnt > 0) {
+			chip->cmdfunc(mtd, NAND_CMD_RNDIN, mtd->writesize, -1);
+			chip->write_buf(mtd, chip->oob_poi, cnt);
+		}
+	}
+
+	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
+	tmp &= ~(NFC_ECC_EN | NFC_ECC_PIPELINE);
+
+	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
+
+	return 0;
+}
+
+static int sunxi_nfc_hw_syndrome_ecc_read_page(struct mtd_info *mtd,
+					       struct nand_chip *chip,
+					       uint8_t *buf, int oob_required,
+					       int page)
+{
+	struct sunxi_nfc *nfc = to_sunxi_nfc(chip->controller);
+	struct nand_ecc_ctrl *ecc = &chip->ecc;
+	struct sunxi_nand_hw_ecc *data = ecc->priv;
+	int steps = mtd->writesize / ecc->size;
+	unsigned int max_bitflips = 0;
+	uint8_t *oob = chip->oob_poi;
+	int offset = 0;
+	int cnt;
+	u32 tmp;
+	int i;
+
+	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
+	tmp &= ~(NFC_ECC_MODE | NFC_ECC_PIPELINE | NFC_ECC_BLOCK_SIZE |
+		 NFC_ECC_BLOCK_SIZE);
+	tmp |= NFC_ECC_EN | (data->mode << NFC_ECC_MODE_SHIFT);
+
+	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
+
+	for (i = 0; i < steps; i++) {
+		chip->read_buf(mtd, NULL, ecc->size);
+
+		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | (1 << 30);
+		writel(tmp, nfc->regs + NFC_REG_CMD);
+		sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
+		memcpy_fromio(buf, nfc->regs + NFC_RAM0_BASE, ecc->size);
+		buf += ecc->size;
+		offset += ecc->size;
+
+		if (readl(nfc->regs + NFC_REG_ECC_ST) & 0x1) {
+			mtd->ecc_stats.failed++;
+		} else {
+			tmp = readl(nfc->regs + NFC_REG_ECC_CNT0) & 0xff;
+			mtd->ecc_stats.corrected += tmp;
+			max_bitflips = max_t(unsigned int, max_bitflips, tmp);
+		}
+
+		if (oob_required) {
+			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, offset, -1);
+			chip->read_buf(mtd, oob, ecc->bytes + ecc->prepad);
+			oob += ecc->bytes + ecc->prepad;
+		}
+
+		offset += ecc->bytes + ecc->prepad;
+	}
+
+	if (oob_required) {
+		cnt = mtd->oobsize - (oob - chip->oob_poi);
+		if (cnt > 0) {
+			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, offset, -1);
+			chip->read_buf(mtd, oob, cnt);
+		}
+	}
+
+	writel(readl(nfc->regs + NFC_REG_ECC_CTL) & ~NFC_ECC_EN,
+	       nfc->regs + NFC_REG_ECC_CTL);
+
+	return max_bitflips;
+}
+
+static int sunxi_nfc_hw_syndrome_ecc_write_page(struct mtd_info *mtd,
+						struct nand_chip *chip,
+						const uint8_t *buf,
+						int oob_required)
+{
+	struct sunxi_nfc *nfc = to_sunxi_nfc(chip->controller);
+	struct nand_ecc_ctrl *ecc = &chip->ecc;
+	struct sunxi_nand_hw_ecc *data = ecc->priv;
+	int steps = mtd->writesize / ecc->size;
+	uint8_t *oob = chip->oob_poi;
+	int offset = 0;
+	int cnt;
+	u32 tmp;
+	int i;
+
+	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
+	tmp &= ~(NFC_ECC_MODE | NFC_ECC_PIPELINE | NFC_ECC_BLOCK_SIZE |
+		 NFC_ECC_BLOCK_SIZE);
+	tmp |= NFC_ECC_EN | (data->mode << NFC_ECC_MODE_SHIFT);
+
+	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
+
+	for (i = 0; i < steps; i++) {
+		chip->write_buf(mtd, buf + (i * ecc->size), ecc->size);
+		offset += ecc->size;
+
+		/* Fill OOB data in */
+		if (oob_required) {
+			tmp = 0xffffffff;
+			memcpy_toio(nfc->regs + NFC_REG_USER_DATA_BASE, &tmp,
+				    4);
+		} else {
+			memcpy_toio(nfc->regs + NFC_REG_USER_DATA_BASE, oob ,
+				    4);
+		}
+
+		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | NFC_ACCESS_DIR |
+		      (1 << 30);
+		writel(tmp, nfc->regs + NFC_REG_CMD);
+		sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
+
+		offset += ecc->bytes + ecc->prepad;
+		oob += ecc->bytes + ecc->prepad;
+	}
+
+	if (oob_required) {
+		cnt = mtd->oobsize - (oob - chip->oob_poi);
+		if (cnt > 0) {
+			chip->cmdfunc(mtd, NAND_CMD_RNDIN, offset, -1);
+			chip->write_buf(mtd, oob, cnt);
+		}
+	}
+
+	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
+	tmp &= ~(NFC_ECC_EN | NFC_ECC_PIPELINE);
+
+	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
+
+	return 0;
+}
+
+static int sunxi_nand_chip_set_timings(struct sunxi_nand_chip *chip,
+				       const struct nand_sdr_timings *timings)
+{
+	u32 min_clk_period = 0;
+
+	/* T1 <=> tCLS */
+	if (timings->tCLS_min > min_clk_period)
+		min_clk_period = timings->tCLS_min;
+
+	/* T2 <=> tCLH */
+	if (timings->tCLH_min > min_clk_period)
+		min_clk_period = timings->tCLH_min;
+
+	/* T3 <=> tCS */
+	if (timings->tCS_min > min_clk_period)
+		min_clk_period = timings->tCS_min;
+
+	/* T4 <=> tCH */
+	if (timings->tCH_min > min_clk_period)
+		min_clk_period = timings->tCH_min;
+
+	/* T5 <=> tWP */
+	if (timings->tWP_min > min_clk_period)
+		min_clk_period = timings->tWP_min;
+
+	/* T6 <=> tWH */
+	if (timings->tWH_min > min_clk_period)
+		min_clk_period = timings->tWH_min;
+
+	/* T7 <=> tALS */
+	if (timings->tALS_min > min_clk_period)
+		min_clk_period = timings->tALS_min;
+
+	/* T8 <=> tDS */
+	if (timings->tDS_min > min_clk_period)
+		min_clk_period = timings->tDS_min;
+
+	/* T9 <=> tDH */
+	if (timings->tDH_min > min_clk_period)
+		min_clk_period = timings->tDH_min;
+
+	/* T10 <=> tRR */
+	if (timings->tRR_min > (min_clk_period * 3))
+		min_clk_period = (timings->tRR_min + 2) / 3;
+
+	/* T11 <=> tALH */
+	if (timings->tALH_min > min_clk_period)
+		min_clk_period = timings->tALH_min;
+
+	/* T12 <=> tRP */
+	if (timings->tRP_min > min_clk_period)
+		min_clk_period = timings->tRP_min;
+
+	/* T13 <=> tREH */
+	if (timings->tREH_min > min_clk_period)
+		min_clk_period = timings->tREH_min;
+
+	/* T14 <=> tRC */
+	if (timings->tRC_min > (min_clk_period * 2))
+		min_clk_period = (timings->tRC_min + 1) / 2;
+
+	/* T15 <=> tWC */
+	if (timings->tWC_min > (min_clk_period * 2))
+		min_clk_period = (timings->tWC_min + 1) / 2;
+
+
+	/* min_clk_period = (NAND-clk-period * 2) */
+	if (min_clk_period < 1000)
+		min_clk_period = 1000;
+
+	min_clk_period /= 1000;
+	chip->clk_rate = (2 * 1000000000) / min_clk_period;
+
+	/* TODO: configure T16-T19 */
+
+	return 0;
+}
+
+static int sunxi_nand_chip_init_timings(struct sunxi_nand_chip *chip,
+					struct device_node *np)
+{
+	const struct nand_sdr_timings *timings;
+	int ret;
+	int mode;
+
+	mode = onfi_get_async_timing_mode(&chip->nand);
+	if (mode == ONFI_TIMING_MODE_UNKNOWN) {
+		mode = of_get_nand_onfi_timing_mode(np);
+		if (mode < 0)
+			mode = 1;
+
+		mode = fls(mode) - 1;
+		if (mode < 0)
+			mode = 0;
+	} else {
+		uint8_t feature[ONFI_SUBFEATURE_PARAM_LEN] = {};
+		mode = fls(mode) - 1;
+		if (mode < 0)
+			mode = 0;
+
+		feature[0] = mode;
+		ret = chip->nand.onfi_set_features(&chip->mtd, &chip->nand,
+						ONFI_FEATURE_ADDR_TIMING_MODE,
+						feature);
+		if (ret)
+			return ret;
+	}
+
+	timings = onfi_async_timing_mode_to_sdr_timings(mode);
+	if (IS_ERR(timings))
+		return PTR_ERR(timings);
+
+	return sunxi_nand_chip_set_timings(chip, timings);
+}
+
+static int sunxi_nand_hw_common_ecc_ctrl_init(struct mtd_info *mtd,
+					      struct nand_ecc_ctrl *ecc,
+					      struct device_node *np)
+{
+	struct sunxi_nand_hw_ecc *data;
+	struct nand_ecclayout *layout;
+	int nsectors;
+	int ret;
+
+	if (!ecc->strength || !ecc->size)
+		return -EINVAL;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	/* Add ECC info retrieval from DT */
+	if (ecc->strength <= 16) {
+		ecc->strength = 16;
+		data->mode = 0;
+	} else if (ecc->strength <= 24) {
+		ecc->strength = 24;
+		data->mode = 1;
+	} else if (ecc->strength <= 28) {
+		ecc->strength = 28;
+		data->mode = 2;
+	} else if (ecc->strength <= 32) {
+		ecc->strength = 32;
+		data->mode = 3;
+	} else if (ecc->strength <= 40) {
+		ecc->strength = 40;
+		data->mode = 4;
+	} else if (ecc->strength <= 48) {
+		ecc->strength = 48;
+		data->mode = 5;
+	} else if (ecc->strength <= 56) {
+		ecc->strength = 56;
+		data->mode = 6;
+	} else if (ecc->strength <= 60) {
+		ecc->strength = 60;
+		data->mode = 7;
+	} else if (ecc->strength <= 64) {
+		ecc->strength = 64;
+		data->mode = 8;
+	} else {
+		pr_err("unsupported strength\n");
+		return -ENOTSUPP;
+	}
+
+	/* HW ECC always request ECC bytes for 1024 bytes blocks */
+	ecc->bytes = ((ecc->strength * fls(8 * 1024)) + 7) / 8;
+
+	/* HW ECC always work with even numbers of ECC bytes */
+	if (ecc->bytes % 2)
+		ecc->bytes++;
+
+	layout = &data->layout;
+	nsectors = mtd->writesize / ecc->size;
+
+	if (mtd->oobsize < ((ecc->bytes + 4) * nsectors)) {
+		ret = -EINVAL;
+		goto err;
+	}
+
+	layout->eccbytes = (ecc->bytes * nsectors);
+
+	ecc->layout = layout;
+	ecc->priv = data;
+
+	return 0;
+
+err:
+	kfree(data);
+
+	return ret;
+}
+
+static void sunxi_nand_hw_common_ecc_ctrl_cleanup(struct nand_ecc_ctrl *ecc)
+{
+	kfree(ecc->priv);
+}
+
+static int sunxi_nand_hw_ecc_ctrl_init(struct mtd_info *mtd,
+				       struct nand_ecc_ctrl *ecc,
+				       struct device_node *np)
+{
+	struct nand_ecclayout *layout;
+	int nsectors;
+	int i, j;
+	int ret;
+
+	ret = sunxi_nand_hw_common_ecc_ctrl_init(mtd, ecc, np);
+	if (ret)
+		return ret;
+
+	ecc->read_page = sunxi_nfc_hw_ecc_read_page;
+	ecc->write_page = sunxi_nfc_hw_ecc_write_page;
+	layout = ecc->layout;
+	nsectors = mtd->writesize / ecc->size;
+	/*
+	 * The first 2 bytes are used for BB markers.
+	 * We merge the 4 user available bytes from HW ECC with this
+	 * first section, hence why the + 2 operation (- 2 + 4).
+	 */
+	layout->oobfree[0].length = mtd->oobsize + 2 -
+				    ((ecc->bytes + 4) * nsectors);
+	layout->oobfree[0].offset = 2;
+	for (i = 0; i < nsectors; i++) {
+		/*
+		 * The first 4 ECC block bytes are already counted in the first
+		 * oobfree entry.
+		 */
+		if (i) {
+			layout->oobfree[i].offset =
+				layout->oobfree[i - 1].offset +
+				layout->oobfree[i - 1].length +
+				ecc->bytes;
+			layout->oobfree[i].length = 4;
+		}
+
+		for (j = 0; j < ecc->bytes; j++)
+			layout->eccpos[(ecc->bytes * i) + j] =
+					layout->oobfree[i].offset +
+					layout->oobfree[i].length + j;
+	}
+
+	return 0;
+}
+
+static int sunxi_nand_hw_syndrome_ecc_ctrl_init(struct mtd_info *mtd,
+						struct nand_ecc_ctrl *ecc,
+						struct device_node *np)
+{
+	struct nand_ecclayout *layout;
+	int nsectors;
+	int i;
+	int ret;
+
+	ret = sunxi_nand_hw_common_ecc_ctrl_init(mtd, ecc, np);
+	if (ret)
+		return ret;
+
+	ecc->prepad = 4;
+	ecc->read_page = sunxi_nfc_hw_syndrome_ecc_read_page;
+	ecc->write_page = sunxi_nfc_hw_syndrome_ecc_write_page;
+
+	layout = ecc->layout;
+	nsectors = mtd->writesize / ecc->size;
+
+	for (i = 0; i < (ecc->bytes * nsectors); i++)
+		layout->eccpos[i] = i;
+
+	layout->oobfree[0].length = mtd->oobsize - i;
+	layout->oobfree[0].offset = i;
+
+	return 0;
+}
+
+static void sunxi_nand_ecc_cleanup(struct nand_ecc_ctrl *ecc)
+{
+	switch (ecc->mode) {
+	case NAND_ECC_HW:
+	case NAND_ECC_HW_SYNDROME:
+		sunxi_nand_hw_common_ecc_ctrl_cleanup(ecc);
+		break;
+	default:
+		break;
+	}
+}
+
+static int sunxi_nand_ecc_init(struct mtd_info *mtd, struct nand_ecc_ctrl *ecc,
+			       struct device_node *np)
+{
+	struct nand_chip *nand = mtd->priv;
+	int ecc_step_size, ecc_strength;
+	int ret;
+
+	ecc_step_size = of_get_nand_ecc_step_size(np);
+	ecc_strength = of_get_nand_ecc_strength(np);
+	if (ecc_step_size > 0 && ecc_strength > 0) {
+		ecc->size = ecc_step_size;
+		ecc->strength = ecc_strength;
+	} else {
+		ecc->size = nand->ecc_step_ds;
+		ecc->strength = nand->ecc_strength_ds;
+	}
+
+	ecc->mode = of_get_nand_ecc_mode(np);
+	switch (ecc->mode) {
+	case NAND_ECC_SOFT_BCH:
+		if (!ecc->size || !ecc->strength)
+			return -EINVAL;
+		ecc->bytes = ((ecc->strength * fls(8 * ecc->size)) + 7) / 8;
+		break;
+	case NAND_ECC_HW:
+		ret = sunxi_nand_hw_ecc_ctrl_init(mtd, ecc, np);
+		if (ret)
+			return ret;
+		break;
+	case NAND_ECC_HW_SYNDROME:
+		ret = sunxi_nand_hw_syndrome_ecc_ctrl_init(mtd, ecc, np);
+		if (ret)
+			return ret;
+		break;
+	case NAND_ECC_NONE:
+	case NAND_ECC_SOFT:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int sunxi_nand_chip_init(struct device *dev, struct sunxi_nfc *nfc,
+				struct device_node *np)
+{
+	const struct nand_sdr_timings *timings;
+	struct sunxi_nand_chip *chip;
+	struct mtd_part_parser_data ppdata;
+	struct mtd_info *mtd;
+	struct nand_chip *nand;
+	int nsels;
+	int ret;
+	int i;
+	u32 tmp;
+
+	if (!of_get_property(np, "reg", &nsels))
+		return -EINVAL;
+
+	nsels /= sizeof(u32);
+	if (!nsels)
+		return -EINVAL;
+
+	chip = devm_kzalloc(dev,
+			    sizeof(*chip) +
+			    (nsels * sizeof(struct sunxi_nand_chip_sel)),
+			    GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->nsels = nsels;
+	chip->selected = -1;
+
+	for (i = 0; i < nsels; i++) {
+		ret = of_property_read_u32_index(np, "reg", i, &tmp);
+		if (ret)
+			return ret;
+
+		if (tmp > 7)
+			return -EINVAL;
+
+		if (test_and_set_bit(tmp, &nfc->assigned_cs))
+			return -EINVAL;
+
+		chip->sels[i].cs = tmp;
+
+		if (!of_property_read_u32_index(np, "allwinner,rb", i, &tmp) &&
+		    tmp < 2) {
+			chip->sels[i].rb.type = RB_NATIVE;
+			chip->sels[i].rb.info.nativeid = tmp;
+		} else {
+			ret = of_get_named_gpio(np, "rb-gpios", i);
+			if (ret >= 0) {
+				tmp = ret;
+				chip->sels[i].rb.type = RB_GPIO;
+				chip->sels[i].rb.info.gpio = tmp;
+				ret = devm_gpio_request(dev, tmp, "nand-rb");
+				if (ret)
+					return ret;
+
+				ret = gpio_direction_input(tmp);
+				if (ret)
+					return ret;
+			} else {
+				chip->sels[i].rb.type = RB_NONE;
+			}
+		}
+	}
+
+	timings = onfi_async_timing_mode_to_sdr_timings(0);
+	if (IS_ERR(timings))
+		return PTR_ERR(timings);
+
+	ret = sunxi_nand_chip_set_timings(chip, timings);
+
+	nand = &chip->nand;
+	/* Default tR value specified in the ONFI spec (chapter 4.15.1) */
+	nand->chip_delay = 200;
+	nand->controller = &nfc->controller;
+	nand->select_chip = sunxi_nfc_select_chip;
+	nand->cmd_ctrl = sunxi_nfc_cmd_ctrl;
+	nand->read_buf = sunxi_nfc_read_buf;
+	nand->write_buf = sunxi_nfc_write_buf;
+	nand->read_byte = sunxi_nfc_read_byte;
+
+	if (of_get_nand_on_flash_bbt(np))
+		nand->bbt_options |= NAND_BBT_USE_FLASH;
+
+	mtd = &chip->mtd;
+	mtd->dev.parent = dev;
+	mtd->priv = nand;
+	mtd->owner = THIS_MODULE;
+
+	ret = nand_scan_ident(mtd, nsels, NULL);
+	if (ret)
+		return ret;
+
+	ret = sunxi_nand_chip_init_timings(chip, np);
+	if (ret)
+		return ret;
+
+	ret = sunxi_nand_ecc_init(mtd, &nand->ecc, np);
+	if (ret)
+		return ret;
+
+	ret = nand_scan_tail(mtd);
+	if (ret)
+		return ret;
+
+	if (of_property_read_string(np, "nand-name", &mtd->name)) {
+		snprintf(chip->default_name, MAX_NAME_SIZE,
+			 DEFAULT_NAME_FORMAT, chip->sels[i].cs);
+		mtd->name = chip->default_name;
+	}
+
+	ppdata.of_node = np;
+	ret = mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0);
+	if (!ret)
+		return ret;
+
+	list_add_tail(&chip->node, &nfc->chips);
+
+	return 0;
+}
+
+static int sunxi_nand_chips_init(struct device *dev, struct sunxi_nfc *nfc)
+{
+	struct device_node *np = dev->of_node;
+	struct device_node *nand_np;
+	int nchips = of_get_child_count(np);
+	int ret;
+
+	if (nchips > 8)
+		return -EINVAL;
+
+	for_each_child_of_node(np, nand_np) {
+		ret = sunxi_nand_chip_init(dev, nfc, nand_np);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static void sunxi_nand_chips_cleanup(struct sunxi_nfc *nfc)
+{
+	struct sunxi_nand_chip *chip;
+
+	while (!list_empty(&nfc->chips)) {
+		chip = list_first_entry(&nfc->chips, struct sunxi_nand_chip,
+					node);
+		nand_release(&chip->mtd);
+		sunxi_nand_ecc_cleanup(&chip->nand.ecc);
+	}
+}
+
+static int sunxi_nfc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *r;
+	struct sunxi_nfc *nfc;
+	int ret;
+
+	nfc = devm_kzalloc(dev, sizeof(*nfc), GFP_KERNEL);
+	if (!nfc) {
+		dev_err(dev, "failed to allocate NFC struct\n");
+		return -ENOMEM;
+	}
+
+	spin_lock_init(&nfc->controller.lock);
+	init_waitqueue_head(&nfc->controller.wq);
+	INIT_LIST_HEAD(&nfc->chips);
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	nfc->regs = devm_ioremap_resource(dev, r);
+	if (IS_ERR(nfc->regs)) {
+		dev_err(dev, "failed to remap iomem\n");
+		return PTR_ERR(nfc->regs);
+	}
+
+	nfc->irq = platform_get_irq(pdev, 0);
+	if (nfc->irq < 0) {
+		dev_err(dev, "failed to retrieve irq\n");
+		return nfc->irq;
+	}
+
+	nfc->ahb_clk = devm_clk_get(dev, "ahb_clk");
+	if (IS_ERR(nfc->ahb_clk)) {
+		dev_err(dev, "failed to retrieve ahb_clk\n");
+		return PTR_ERR(nfc->ahb_clk);
+	}
+
+	ret = clk_prepare_enable(nfc->ahb_clk);
+	if (ret)
+		return ret;
+
+	nfc->sclk = devm_clk_get(dev, "sclk");
+	if (IS_ERR(nfc->sclk)) {
+		dev_err(dev, "failed to retrieve nand_clk\n");
+		ret = PTR_ERR(nfc->sclk);
+		goto out_ahb_clk_unprepare;
+	}
+
+	ret = clk_prepare_enable(nfc->sclk);
+	if (ret)
+		goto out_ahb_clk_unprepare;
+
+	/* Reset NFC */
+	writel(readl(nfc->regs + NFC_REG_CTL) | NFC_RESET,
+	       nfc->regs + NFC_REG_CTL);
+	while (readl(nfc->regs + NFC_REG_CTL) & NFC_RESET)
+		;
+
+	writel(0, nfc->regs + NFC_REG_INT);
+	ret = devm_request_irq(dev, nfc->irq, sunxi_nfc_interrupt,
+			       0, "sunxi-nand", nfc);
+	if (ret)
+		goto out_sclk_unprepare;
+
+	platform_set_drvdata(pdev, nfc);
+
+	writel(0x100, nfc->regs + NFC_REG_TIMING_CTL);
+	writel(0x7ff, nfc->regs + NFC_REG_TIMING_CFG);
+
+	ret = sunxi_nand_chips_init(dev, nfc);
+	if (ret) {
+		dev_err(dev, "failed to init nand chips\n");
+		goto out_sclk_unprepare;
+	}
+
+	return 0;
+
+out_sclk_unprepare:
+	clk_disable_unprepare(nfc->sclk);
+out_ahb_clk_unprepare:
+	clk_disable_unprepare(nfc->ahb_clk);
+
+	return ret;
+}
+
+static int sunxi_nfc_remove(struct platform_device *pdev)
+{
+	struct sunxi_nfc *nfc = platform_get_drvdata(pdev);
+
+	sunxi_nand_chips_cleanup(nfc);
+
+	return 0;
+}
+
+static const struct of_device_id sunxi_nfc_ids[] = {
+	{ .compatible = "allwinner,sun4i-nand" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, sunxi_nfc_ids);
+
+static struct platform_driver sunxi_nfc_driver = {
+	.driver = {
+		.name = "sunxi_nand",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(sunxi_nfc_ids),
+	},
+	.probe = sunxi_nfc_probe,
+	.remove = sunxi_nfc_remove,
+};
+module_platform_driver(sunxi_nfc_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Boris BREZILLON");
+MODULE_DESCRIPTION("Allwinner NAND Flash Controller driver");
+MODULE_ALIAS("platform:sunxi_nfc");
-- 
1.7.9.5

[PATCH v3 5/9] mtd: nand: add sunxi NAND flash controller support

[Ezequiel Garcia]

Hello Boris,

Sorry for the review delay.

Emilio, if you have hardware to test this, it would be nice to give
Boris some Tested-by?

On 12 Mar 07:07 PM, Boris BREZILLON wrote:
> Add support for the sunxi NAND Flash Controller (NFC).
> 
> Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
> ---
>  drivers/mtd/nand/Kconfig      |    6 +
>  drivers/mtd/nand/Makefile     |    1 +
>  drivers/mtd/nand/sunxi_nand.c | 1276 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1283 insertions(+)
>  create mode 100644 drivers/mtd/nand/sunxi_nand.c
> 
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index 90ff447..8a28c06 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -510,4 +510,10 @@ config MTD_NAND_XWAY
>  	  Enables support for NAND Flash chips on Lantiq XWAY SoCs. NAND is attached
>  	  to the External Bus Unit (EBU).
>  
> +config MTD_NAND_SUNXI
> +	tristate "Support for NAND on Allwinner SoCs"
> +	depends on ARCH_SUNXI
> +	help
> +	  Enables support for NAND Flash chips on Allwinner SoCs.
> +
>  endif # MTD_NAND
> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index 0b8a822..34f45d8 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -49,5 +49,6 @@ obj-$(CONFIG_MTD_NAND_JZ4740)		+= jz4740_nand.o
>  obj-$(CONFIG_MTD_NAND_GPMI_NAND)	+= gpmi-nand/
>  obj-$(CONFIG_MTD_NAND_XWAY)		+= xway_nand.o
>  obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH)	+= bcm47xxnflash/
> +obj-$(CONFIG_MTD_NAND_SUNXI)		+= sunxi_nand.o
>  
>  nand-objs := nand_base.o nand_bbt.o
> diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c
> new file mode 100644
> index 0000000..e93cc44
> --- /dev/null
> +++ b/drivers/mtd/nand/sunxi_nand.c
> @@ -0,0 +1,1276 @@
> +/*
> + * Copyright (C) 2013 Boris BREZILLON <b.brezillon.dev@gmail.com>
> + *
> + * Derived from:
> + *	https://github.com/yuq/sunxi-nfc-mtd
> + *	Copyright (C) 2013 Qiang Yu <yuq825@gmail.com>
> + *
> + *	https://github.com/hno/Allwinner-Info
> + *	Copyright (C) 2013 Henrik Nordstr?m <Henrik Nordstr?m>
> + *
> + *	Copyright (C) 2013 Dmitriy B. <rzk333@gmail.com>
> + *	Copyright (C) 2013 Sergey Lapin <slapin@ossfans.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/dma-mapping.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_mtd.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/nand.h>
> +#include <linux/mtd/partitions.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/dmaengine.h>
> +#include <linux/gpio.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +
> +#define NFC_REG_CTL		0x0000
> +#define NFC_REG_ST		0x0004
> +#define NFC_REG_INT		0x0008
> +#define NFC_REG_TIMING_CTL	0x000C
> +#define NFC_REG_TIMING_CFG	0x0010
> +#define NFC_REG_ADDR_LOW	0x0014
> +#define NFC_REG_ADDR_HIGH	0x0018
> +#define NFC_REG_SECTOR_NUM	0x001C
> +#define NFC_REG_CNT		0x0020
> +#define NFC_REG_CMD		0x0024
> +#define NFC_REG_RCMD_SET	0x0028
> +#define NFC_REG_WCMD_SET	0x002C
> +#define NFC_REG_IO_DATA		0x0030
> +#define NFC_REG_ECC_CTL		0x0034
> +#define NFC_REG_ECC_ST		0x0038
> +#define NFC_REG_DEBUG		0x003C
> +#define NFC_REG_ECC_CNT0	0x0040
> +#define NFC_REG_ECC_CNT1	0x0044
> +#define NFC_REG_ECC_CNT2	0x0048
> +#define NFC_REG_ECC_CNT3	0x004c
> +#define NFC_REG_USER_DATA_BASE	0x0050
> +#define NFC_REG_SPARE_AREA	0x00A0
> +#define NFC_RAM0_BASE		0x0400
> +#define NFC_RAM1_BASE		0x0800
> +
> +/*define bit use in NFC_CTL*/

nit: Use BIT() for these?

> +#define NFC_EN				(1 << 0)
> +#define NFC_RESET			(1 << 1)
> +#define NFC_BUS_WIDYH			(1 << 2)
> +#define NFC_RB_SEL			(1 << 3)
> +#define NFC_CE_SEL			(7 << 24)
> +#define NFC_CE_CTL			(1 << 6)
> +#define NFC_CE_CTL1			(1 << 7)
> +#define NFC_PAGE_SIZE			(0xf << 8)
> +#define NFC_SAM				(1 << 12)
> +#define NFC_RAM_METHOD			(1 << 14)
> +#define NFC_DEBUG_CTL			(1 << 31)
> +
> +/*define bit use in NFC_ST*/
> +#define NFC_RB_B2R			(1 << 0)
> +#define NFC_CMD_INT_FLAG		(1 << 1)
> +#define NFC_DMA_INT_FLAG		(1 << 2)
> +#define NFC_CMD_FIFO_STATUS		(1 << 3)
> +#define NFC_STA				(1 << 4)
> +#define NFC_NATCH_INT_FLAG		(1 << 5)
> +#define NFC_RB_STATE0			(1 << 8)
> +#define NFC_RB_STATE1			(1 << 9)
> +#define NFC_RB_STATE2			(1 << 10)
> +#define NFC_RB_STATE3			(1 << 11)
> +
> +/*define bit use in NFC_INT*/
> +#define NFC_B2R_INT_ENABLE		(1 << 0)
> +#define NFC_CMD_INT_ENABLE		(1 << 1)
> +#define NFC_DMA_INT_ENABLE		(1 << 2)
> +#define NFC_INT_MASK			(NFC_B2R_INT_ENABLE | \
> +					 NFC_CMD_INT_ENABLE | \
> +					 NFC_DMA_INT_ENABLE)
> +
> +
> +/*define bit use in NFC_CMD*/
> +#define NFC_CMD_LOW_BYTE		(0xff << 0)
> +#define NFC_CMD_HIGH_BYTE		(0xff << 8)
> +#define NFC_ADR_NUM			(0x7 << 16)
> +#define NFC_SEND_ADR			(1 << 19)
> +#define NFC_ACCESS_DIR			(1 << 20)
> +#define NFC_DATA_TRANS			(1 << 21)
> +#define NFC_SEND_CMD1			(1 << 22)
> +#define NFC_WAIT_FLAG			(1 << 23)
> +#define NFC_SEND_CMD2			(1 << 24)
> +#define NFC_SEQ				(1 << 25)
> +#define NFC_DATA_SWAP_METHOD		(1 << 26)
> +#define NFC_ROW_AUTO_INC		(1 << 27)
> +#define NFC_SEND_CMD3			(1 << 28)
> +#define NFC_SEND_CMD4			(1 << 29)
> +#define NFC_CMD_TYPE			(3 << 30)
> +
> +/* define bit use in NFC_RCMD_SET*/
> +#define NFC_READ_CMD			(0xff << 0)
> +#define NFC_RANDOM_READ_CMD0		(0xff << 8)
> +#define NFC_RANDOM_READ_CMD1		(0xff << 16)
> +
> +/*define bit use in NFC_WCMD_SET*/
> +#define NFC_PROGRAM_CMD			(0xff << 0)
> +#define NFC_RANDOM_WRITE_CMD		(0xff << 8)
> +#define NFC_READ_CMD0			(0xff << 16)
> +#define NFC_READ_CMD1			(0xff << 24)
> +
> +/*define bit use in NFC_ECC_CTL*/
> +#define NFC_ECC_EN			(1 << 0)
> +#define NFC_ECC_PIPELINE		(1 << 3)
> +#define NFC_ECC_EXCEPTION		(1 << 4)
> +#define NFC_ECC_BLOCK_SIZE		(1 << 5)
> +#define NFC_RANDOM_EN			(1 << 9)
> +#define NFC_RANDOM_DIRECTION		(1 << 10)
> +#define NFC_ECC_MODE_SHIFT		12
> +#define NFC_ECC_MODE			(0xf << NFC_ECC_MODE_SHIFT)
> +#define NFC_RANDOM_SEED			(0x7fff << 16)
> +
> +
> +

Kill and avoid these extra empty lines.

> +enum sunxi_nand_rb_type {
> +	RB_NONE,
> +	RB_NATIVE,
> +	RB_GPIO,
> +};
> +

Can you add some documentation about this read-back stuff?
I know documenting is not a super-fun activity, but it's a bit tough
to review otherwise.

> +struct sunxi_nand_rb {
> +	enum sunxi_nand_rb_type type;
> +	union {
> +		int gpio;
> +		int nativeid;
> +	} info;
> +};
> +
> +struct sunxi_nand_chip_sel {
> +	u8 cs;
> +	struct sunxi_nand_rb rb;
> +};
> +
> +#define DEFAULT_NAME_FORMAT	"nand@%d"
> +#define MAX_NAME_SIZE		(sizeof("nand@") + 2)
> +
> +struct sunxi_nand_hw_ecc {
> +	int mode;
> +	struct nand_ecclayout layout;
> +};
> +
> +struct sunxi_nand_chip {
> +	struct list_head node;
> +	struct nand_chip nand;
> +	struct mtd_info mtd;
> +	char default_name[MAX_NAME_SIZE];
> +	unsigned long clk_rate;
> +	int selected;
> +	int nsels;
> +	struct sunxi_nand_chip_sel sels[0];

Hm... you prepare the whole multiple chip support but you really support a
single chip? I'd say drop entirely and support just a single chip, it'll
make the driver much cleaner. We can always add the support later, after
proper testing.

> +};
> +
> +static inline struct sunxi_nand_chip *to_sunxi_nand(struct nand_chip *nand)
> +{
> +	return container_of(nand, struct sunxi_nand_chip, nand);
> +}
> +
> +struct sunxi_nfc {
> +	struct nand_hw_control controller;
> +	void __iomem *regs;
> +	int irq;

You don't seem to need the irq stored anywhere, as you're using
devm_request_irq.

> +	struct clk *ahb_clk;
> +	struct clk *sclk;
> +	unsigned long assigned_cs;
> +	unsigned long clk_rate;
> +	struct list_head chips;
> +	struct completion complete;
> +};
> +
> +static inline struct sunxi_nfc *to_sunxi_nfc(struct nand_hw_control *ctrl)
> +{
> +	return container_of(ctrl, struct sunxi_nfc, controller);
> +}
> +
> +static irqreturn_t sunxi_nfc_interrupt(int irq, void *dev_id)
> +{
> +	struct sunxi_nfc *nfc = dev_id;
> +	u32 st = readl(nfc->regs + NFC_REG_ST);
> +	u32 ien = readl(nfc->regs + NFC_REG_INT);
> +
> +	if (!(ien & st))
> +		return IRQ_NONE;
> +
> +	if ((ien & st) == ien)
> +		complete(&nfc->complete);
> +
> +	writel(st & NFC_INT_MASK, nfc->regs + NFC_REG_ST);
> +	writel(~st & ien & NFC_INT_MASK, nfc->regs + NFC_REG_INT);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int sunxi_nfc_wait_int(struct sunxi_nfc *nfc, u32 flags,
> +			      unsigned int timeout_ms)
> +{
> +	init_completion(&nfc->complete);
> +
> +	writel(flags, nfc->regs + NFC_REG_INT);
> +	if (!timeout_ms)
> +		wait_for_completion(&nfc->complete);

In the same vein as infinite loops (see below), I'd avoid this no-timeout wait.
Or maybe you can *guarantee* it'll be always completed?

> +	else if (!wait_for_completion_timeout(&nfc->complete,
> +					      msecs_to_jiffies(timeout_ms)))
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}
> +
> +static int sunxi_nfc_dev_ready(struct mtd_info *mtd)
> +{
> +	struct nand_chip *nand = mtd->priv;
> +	struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
> +	struct sunxi_nfc *nfc = to_sunxi_nfc(sunxi_nand->nand.controller);
> +	struct sunxi_nand_rb *rb;
> +	unsigned long timeo = (sunxi_nand->nand.state == FL_ERASING ? 400 : 20);
> +	int ret;
> +
> +	if (sunxi_nand->selected < 0)
> +		return 0;
> +
> +	rb = &sunxi_nand->sels[sunxi_nand->selected].rb;
> +
> +	switch (rb->type) {
> +	case RB_NATIVE:
> +		ret = !!(readl(nfc->regs + NFC_REG_ST) &
> +			 (NFC_RB_STATE0 << rb->info.nativeid));
> +		if (ret)
> +			break;
> +
> +		sunxi_nfc_wait_int(nfc, NFC_RB_B2R, timeo);
> +		ret = !!(readl(nfc->regs + NFC_REG_ST) &
> +			 (NFC_RB_STATE0 << rb->info.nativeid));
> +		break;
> +	case RB_GPIO:
> +		ret = gpio_get_value(rb->info.gpio);
> +		break;
> +	case RB_NONE:
> +	default:
> +		ret = 0;
> +		pr_err("cannot check R/B NAND status!");

I'd suggest avoiding this kind of pr_err. The user won't know
who's saying this. Try using dev_err or pr_fmt.

> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static void sunxi_nfc_select_chip(struct mtd_info *mtd, int chip)
> +{
> +	struct nand_chip *nand = mtd->priv;
> +	struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
> +	struct sunxi_nfc *nfc = to_sunxi_nfc(sunxi_nand->nand.controller);
> +	struct sunxi_nand_chip_sel *sel;
> +	u32 ctl;
> +
> +	if (chip > 0 && chip >= sunxi_nand->nsels)
> +		return;
> +
> +	if (chip == sunxi_nand->selected)
> +		return;
> +
> +	ctl = readl(nfc->regs + NFC_REG_CTL) &
> +	      ~(NFC_CE_SEL | NFC_RB_SEL | NFC_EN);
> +
> +	if (chip >= 0) {
> +		sel = &sunxi_nand->sels[chip];
> +
> +		ctl |= (sel->cs << 24) | NFC_EN |
> +		       (((nand->page_shift - 10) & 0xf) << 8);
> +		if (sel->rb.type == RB_NONE) {
> +			nand->dev_ready = NULL;
> +		} else {
> +			nand->dev_ready = sunxi_nfc_dev_ready;
> +			if (sel->rb.type == RB_NATIVE)
> +				ctl |= (sel->rb.info.nativeid << 3);
> +		}
> +
> +		writel(mtd->writesize, nfc->regs + NFC_REG_SPARE_AREA);
> +
> +		if (nfc->clk_rate != sunxi_nand->clk_rate) {
> +			clk_set_rate(nfc->sclk, sunxi_nand->clk_rate);
> +			nfc->clk_rate = sunxi_nand->clk_rate;
> +		}
> +	}
> +
> +	writel(ctl, nfc->regs + NFC_REG_CTL);
> +
> +	sunxi_nand->selected = chip;
> +}
> +
> +static void sunxi_nfc_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
> +{
> +	struct nand_chip *nand = mtd->priv;
> +	struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
> +	struct sunxi_nfc *nfc = to_sunxi_nfc(sunxi_nand->nand.controller);
> +	int cnt;
> +	int offs = 0;
> +	u32 tmp;
> +
> +	while (len > offs) {
> +		cnt = len - offs;
> +		if (cnt > 1024)
> +			cnt = 1024;
> +
> +		while ((readl(nfc->regs + NFC_REG_ST) & NFC_CMD_FIFO_STATUS))
> +			;

How about avoiding the infinite loop?

> +		writel(cnt, nfc->regs + NFC_REG_CNT);
> +		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD;
> +		writel(tmp, nfc->regs + NFC_REG_CMD);
> +		sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
> +		if (buf)
> +			memcpy_fromio(buf + offs, nfc->regs + NFC_RAM0_BASE,
> +				      cnt);
> +		offs += cnt;
> +	}
> +}
> +
> +static void sunxi_nfc_write_buf(struct mtd_info *mtd, const uint8_t *buf,
> +				int len)
> +{
> +	struct nand_chip *nand = mtd->priv;
> +	struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
> +	struct sunxi_nfc *nfc = to_sunxi_nfc(sunxi_nand->nand.controller);
> +	int cnt;
> +	int offs = 0;
> +	u32 tmp;
> +
> +	while (len > offs) {
> +		cnt = len - offs;
> +		if (cnt > 1024)
> +			cnt = 1024;
> +
> +		while ((readl(nfc->regs + NFC_REG_ST) & NFC_CMD_FIFO_STATUS))
> +			;

Ditto.

> +		writel(cnt, nfc->regs + NFC_REG_CNT);
> +		memcpy_toio(nfc->regs + NFC_RAM0_BASE, buf + offs, cnt);
> +		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD |
> +		      NFC_ACCESS_DIR;
> +		writel(tmp, nfc->regs + NFC_REG_CMD);
> +		sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
> +		offs += cnt;
> +	}
> +}
> +
> +static uint8_t sunxi_nfc_read_byte(struct mtd_info *mtd)
> +{
> +	uint8_t ret;
> +
> +	sunxi_nfc_read_buf(mtd, &ret, 1);
> +
> +	return ret;
> +}
> +
> +static void sunxi_nfc_cmd_ctrl(struct mtd_info *mtd, int dat,
> +			       unsigned int ctrl)
> +{
> +	struct nand_chip *nand = mtd->priv;
> +	struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
> +	struct sunxi_nfc *nfc = to_sunxi_nfc(sunxi_nand->nand.controller);
> +	u32 tmp;
> +
> +	while ((readl(nfc->regs + NFC_REG_ST) & NFC_CMD_FIFO_STATUS))
> +		;
> +

Ditto.

> +	if (ctrl & NAND_CTRL_CHANGE) {
> +		tmp = readl(nfc->regs + NFC_REG_CTL);
> +		if (ctrl & NAND_NCE)
> +			tmp |= NFC_CE_CTL;
> +		else
> +			tmp &= ~NFC_CE_CTL;
> +		writel(tmp, nfc->regs + NFC_REG_CTL);
> +	}
> +
> +	if (dat == NAND_CMD_NONE)
> +		return;
> +
> +	if (ctrl & NAND_CLE) {
> +		writel(NFC_SEND_CMD1 | dat, nfc->regs + NFC_REG_CMD);
> +	} else {
> +		writel(dat, nfc->regs + NFC_REG_ADDR_LOW);
> +		writel(NFC_SEND_ADR, nfc->regs + NFC_REG_CMD);
> +	}
> +
> +	sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
> +}
> +
> +static int sunxi_nfc_hw_ecc_read_page(struct mtd_info *mtd,
> +				      struct nand_chip *chip, uint8_t *buf,
> +				      int oob_required, int page)
> +{
> +	struct sunxi_nfc *nfc = to_sunxi_nfc(chip->controller);
> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
> +	struct nand_ecclayout *layout = ecc->layout;
> +	struct sunxi_nand_hw_ecc *data = ecc->priv;
> +	int steps = mtd->writesize / ecc->size;
> +	unsigned int max_bitflips = 0;
> +	int offset;
> +	u32 tmp;
> +	int i;
> +	int cnt;
> +
> +	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
> +	tmp &= ~(NFC_ECC_MODE | NFC_ECC_PIPELINE | NFC_ECC_BLOCK_SIZE |
> +		 NFC_ECC_BLOCK_SIZE);
> +	tmp |= NFC_ECC_EN | (data->mode << NFC_ECC_MODE_SHIFT);
> +
> +	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
> +
> +	for (i = 0; i < steps; i++) {
> +		if (i)
> +			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, i * ecc->size, -1);
> +
> +		offset = mtd->writesize + layout->eccpos[i * ecc->bytes] - 4;
> +
> +		chip->read_buf(mtd, NULL, ecc->size);
> +
> +		chip->cmdfunc(mtd, NAND_CMD_RNDOUT, offset, -1);
> +		while ((readl(nfc->regs + NFC_REG_ST) & NFC_CMD_FIFO_STATUS))
> +			;
> +

Ditto.

> +		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | (1 << 30);
> +		writel(tmp, nfc->regs + NFC_REG_CMD);
> +		sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
> +		memcpy_fromio(buf + (i * ecc->size),
> +			      nfc->regs + NFC_RAM0_BASE, ecc->size);
> +
> +		if (readl(nfc->regs + NFC_REG_ECC_ST) & 0x1) {
> +			mtd->ecc_stats.failed++;
> +		} else {
> +			tmp = readl(nfc->regs + NFC_REG_ECC_CNT0) & 0xff;
> +			mtd->ecc_stats.corrected += tmp;
> +			max_bitflips = max_t(unsigned int, max_bitflips, tmp);
> +		}
> +
> +		if (oob_required) {
> +			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, offset, -1);
> +			while ((readl(nfc->regs + NFC_REG_ST) &
> +			       NFC_CMD_FIFO_STATUS))
> +				;

Ditto.

> +			offset -= mtd->writesize;
> +			chip->read_buf(mtd, chip->oob_poi + offset,
> +				      ecc->bytes + 4);
> +		}
> +	}
> +
> +	if (oob_required) {
> +		cnt = ecc->layout->oobfree[0].length - 4;
> +		if (cnt > 0) {
> +			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, mtd->writesize,
> +				      -1);
> +			chip->read_buf(mtd, chip->oob_poi, cnt);
> +		}
> +	}
> +
> +	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
> +	tmp &= ~NFC_ECC_EN;
> +
> +	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
> +
> +	return max_bitflips;
> +}
> +
> +static int sunxi_nfc_hw_ecc_write_page(struct mtd_info *mtd,
> +				       struct nand_chip *chip,
> +				       const uint8_t *buf, int oob_required)
> +{
> +	struct sunxi_nfc *nfc = to_sunxi_nfc(chip->controller);
> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
> +	struct nand_ecclayout *layout = ecc->layout;
> +	struct sunxi_nand_hw_ecc *data = ecc->priv;
> +	int offset;
> +	u32 tmp;
> +	int i;
> +	int cnt;
> +
> +	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
> +	tmp &= ~(NFC_ECC_MODE | NFC_ECC_PIPELINE | NFC_ECC_BLOCK_SIZE |
> +		 NFC_ECC_BLOCK_SIZE);
> +	tmp |= NFC_ECC_EN | (data->mode << NFC_ECC_MODE_SHIFT);
> +
> +	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
> +
> +	for (i = 0; i < mtd->writesize / ecc->size; i++) {
> +		if (i)
> +			chip->cmdfunc(mtd, NAND_CMD_RNDIN, i * ecc->size, -1);
> +
> +		chip->write_buf(mtd, buf + (i * ecc->size), ecc->size);
> +
> +		offset = layout->eccpos[i * ecc->bytes] - 4 + mtd->writesize;
> +
> +		/* Fill OOB data in */
> +		if (oob_required) {
> +			tmp = 0xffffffff;
> +			memcpy_toio(nfc->regs + NFC_REG_USER_DATA_BASE, &tmp,
> +				    4);
> +		} else {
> +			memcpy_toio(nfc->regs + NFC_REG_USER_DATA_BASE,
> +				    chip->oob_poi + offset - mtd->writesize,
> +				    4);
> +		}
> +
> +		chip->cmdfunc(mtd, NAND_CMD_RNDIN, offset, -1);
> +		while ((readl(nfc->regs + NFC_REG_ST) &
> +		       NFC_CMD_FIFO_STATUS))
> +			;
> +

Ditto.

> +		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | NFC_ACCESS_DIR |
> +		      (1 << 30);
> +		writel(tmp, nfc->regs + NFC_REG_CMD);
> +		sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
> +	}
> +
> +	if (oob_required) {
> +		cnt = ecc->layout->oobfree[0].length - 4;
> +		if (cnt > 0) {
> +			chip->cmdfunc(mtd, NAND_CMD_RNDIN, mtd->writesize, -1);
> +			chip->write_buf(mtd, chip->oob_poi, cnt);
> +		}
> +	}
> +
> +	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
> +	tmp &= ~(NFC_ECC_EN | NFC_ECC_PIPELINE);
> +
> +	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
> +
> +	return 0;
> +}
> +
> +static int sunxi_nfc_hw_syndrome_ecc_read_page(struct mtd_info *mtd,
> +					       struct nand_chip *chip,
> +					       uint8_t *buf, int oob_required,
> +					       int page)
> +{
> +	struct sunxi_nfc *nfc = to_sunxi_nfc(chip->controller);
> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
> +	struct sunxi_nand_hw_ecc *data = ecc->priv;
> +	int steps = mtd->writesize / ecc->size;
> +	unsigned int max_bitflips = 0;
> +	uint8_t *oob = chip->oob_poi;
> +	int offset = 0;
> +	int cnt;
> +	u32 tmp;
> +	int i;
> +
> +	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
> +	tmp &= ~(NFC_ECC_MODE | NFC_ECC_PIPELINE | NFC_ECC_BLOCK_SIZE |
> +		 NFC_ECC_BLOCK_SIZE);
> +	tmp |= NFC_ECC_EN | (data->mode << NFC_ECC_MODE_SHIFT);
> +
> +	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
> +
> +	for (i = 0; i < steps; i++) {
> +		chip->read_buf(mtd, NULL, ecc->size);
> +
> +		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | (1 << 30);
> +		writel(tmp, nfc->regs + NFC_REG_CMD);
> +		sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
> +		memcpy_fromio(buf, nfc->regs + NFC_RAM0_BASE, ecc->size);
> +		buf += ecc->size;
> +		offset += ecc->size;
> +
> +		if (readl(nfc->regs + NFC_REG_ECC_ST) & 0x1) {
> +			mtd->ecc_stats.failed++;
> +		} else {
> +			tmp = readl(nfc->regs + NFC_REG_ECC_CNT0) & 0xff;
> +			mtd->ecc_stats.corrected += tmp;
> +			max_bitflips = max_t(unsigned int, max_bitflips, tmp);
> +		}
> +
> +		if (oob_required) {
> +			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, offset, -1);
> +			chip->read_buf(mtd, oob, ecc->bytes + ecc->prepad);
> +			oob += ecc->bytes + ecc->prepad;
> +		}
> +
> +		offset += ecc->bytes + ecc->prepad;
> +	}
> +
> +	if (oob_required) {
> +		cnt = mtd->oobsize - (oob - chip->oob_poi);
> +		if (cnt > 0) {
> +			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, offset, -1);
> +			chip->read_buf(mtd, oob, cnt);
> +		}
> +	}
> +
> +	writel(readl(nfc->regs + NFC_REG_ECC_CTL) & ~NFC_ECC_EN,
> +	       nfc->regs + NFC_REG_ECC_CTL);
> +
> +	return max_bitflips;
> +}
> +
> +static int sunxi_nfc_hw_syndrome_ecc_write_page(struct mtd_info *mtd,
> +						struct nand_chip *chip,
> +						const uint8_t *buf,
> +						int oob_required)
> +{
> +	struct sunxi_nfc *nfc = to_sunxi_nfc(chip->controller);
> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
> +	struct sunxi_nand_hw_ecc *data = ecc->priv;
> +	int steps = mtd->writesize / ecc->size;
> +	uint8_t *oob = chip->oob_poi;
> +	int offset = 0;
> +	int cnt;
> +	u32 tmp;
> +	int i;
> +
> +	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
> +	tmp &= ~(NFC_ECC_MODE | NFC_ECC_PIPELINE | NFC_ECC_BLOCK_SIZE |
> +		 NFC_ECC_BLOCK_SIZE);
> +	tmp |= NFC_ECC_EN | (data->mode << NFC_ECC_MODE_SHIFT);
> +
> +	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
> +
> +	for (i = 0; i < steps; i++) {
> +		chip->write_buf(mtd, buf + (i * ecc->size), ecc->size);
> +		offset += ecc->size;
> +
> +		/* Fill OOB data in */
> +		if (oob_required) {
> +			tmp = 0xffffffff;
> +			memcpy_toio(nfc->regs + NFC_REG_USER_DATA_BASE, &tmp,
> +				    4);
> +		} else {
> +			memcpy_toio(nfc->regs + NFC_REG_USER_DATA_BASE, oob ,
> +				    4);
> +		}
> +
> +		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | NFC_ACCESS_DIR |
> +		      (1 << 30);
> +		writel(tmp, nfc->regs + NFC_REG_CMD);
> +		sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
> +
> +		offset += ecc->bytes + ecc->prepad;
> +		oob += ecc->bytes + ecc->prepad;
> +	}
> +
> +	if (oob_required) {
> +		cnt = mtd->oobsize - (oob - chip->oob_poi);
> +		if (cnt > 0) {
> +			chip->cmdfunc(mtd, NAND_CMD_RNDIN, offset, -1);
> +			chip->write_buf(mtd, oob, cnt);
> +		}
> +	}
> +
> +	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
> +	tmp &= ~(NFC_ECC_EN | NFC_ECC_PIPELINE);
> +
> +	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
> +
> +	return 0;
> +}
> +
> +static int sunxi_nand_chip_set_timings(struct sunxi_nand_chip *chip,
> +				       const struct nand_sdr_timings *timings)
> +{
> +	u32 min_clk_period = 0;
> +
> +	/* T1 <=> tCLS */
> +	if (timings->tCLS_min > min_clk_period)
> +		min_clk_period = timings->tCLS_min;
> +
> +	/* T2 <=> tCLH */
> +	if (timings->tCLH_min > min_clk_period)
> +		min_clk_period = timings->tCLH_min;
> +
> +	/* T3 <=> tCS */
> +	if (timings->tCS_min > min_clk_period)
> +		min_clk_period = timings->tCS_min;
> +
> +	/* T4 <=> tCH */
> +	if (timings->tCH_min > min_clk_period)
> +		min_clk_period = timings->tCH_min;
> +
> +	/* T5 <=> tWP */
> +	if (timings->tWP_min > min_clk_period)
> +		min_clk_period = timings->tWP_min;
> +
> +	/* T6 <=> tWH */
> +	if (timings->tWH_min > min_clk_period)
> +		min_clk_period = timings->tWH_min;
> +
> +	/* T7 <=> tALS */
> +	if (timings->tALS_min > min_clk_period)
> +		min_clk_period = timings->tALS_min;
> +
> +	/* T8 <=> tDS */
> +	if (timings->tDS_min > min_clk_period)
> +		min_clk_period = timings->tDS_min;
> +
> +	/* T9 <=> tDH */
> +	if (timings->tDH_min > min_clk_period)
> +		min_clk_period = timings->tDH_min;
> +
> +	/* T10 <=> tRR */
> +	if (timings->tRR_min > (min_clk_period * 3))
> +		min_clk_period = (timings->tRR_min + 2) / 3;
> +
> +	/* T11 <=> tALH */
> +	if (timings->tALH_min > min_clk_period)
> +		min_clk_period = timings->tALH_min;
> +
> +	/* T12 <=> tRP */
> +	if (timings->tRP_min > min_clk_period)
> +		min_clk_period = timings->tRP_min;
> +
> +	/* T13 <=> tREH */
> +	if (timings->tREH_min > min_clk_period)
> +		min_clk_period = timings->tREH_min;
> +
> +	/* T14 <=> tRC */
> +	if (timings->tRC_min > (min_clk_period * 2))
> +		min_clk_period = (timings->tRC_min + 1) / 2;
> +
> +	/* T15 <=> tWC */
> +	if (timings->tWC_min > (min_clk_period * 2))
> +		min_clk_period = (timings->tWC_min + 1) / 2;
> +
> +
> +	/* min_clk_period = (NAND-clk-period * 2) */
> +	if (min_clk_period < 1000)
> +		min_clk_period = 1000;
> +
> +	min_clk_period /= 1000;
> +	chip->clk_rate = (2 * 1000000000) / min_clk_period;
> +
> +	/* TODO: configure T16-T19 */
> +
> +	return 0;
> +}
> +
> +static int sunxi_nand_chip_init_timings(struct sunxi_nand_chip *chip,
> +					struct device_node *np)
> +{
> +	const struct nand_sdr_timings *timings;
> +	int ret;
> +	int mode;
> +
> +	mode = onfi_get_async_timing_mode(&chip->nand);
> +	if (mode == ONFI_TIMING_MODE_UNKNOWN) {
> +		mode = of_get_nand_onfi_timing_mode(np);
> +		if (mode < 0)
> +			mode = 1;
> +
> +		mode = fls(mode) - 1;
> +		if (mode < 0)
> +			mode = 0;
> +	} else {
> +		uint8_t feature[ONFI_SUBFEATURE_PARAM_LEN] = {};
> +		mode = fls(mode) - 1;
> +		if (mode < 0)
> +			mode = 0;
> +
> +		feature[0] = mode;
> +		ret = chip->nand.onfi_set_features(&chip->mtd, &chip->nand,
> +						ONFI_FEATURE_ADDR_TIMING_MODE,
> +						feature);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	timings = onfi_async_timing_mode_to_sdr_timings(mode);
> +	if (IS_ERR(timings))
> +		return PTR_ERR(timings);
> +
> +	return sunxi_nand_chip_set_timings(chip, timings);
> +}
> +
> +static int sunxi_nand_hw_common_ecc_ctrl_init(struct mtd_info *mtd,
> +					      struct nand_ecc_ctrl *ecc,
> +					      struct device_node *np)
> +{
> +	struct sunxi_nand_hw_ecc *data;
> +	struct nand_ecclayout *layout;
> +	int nsectors;
> +	int ret;
> +
> +	if (!ecc->strength || !ecc->size)
> +		return -EINVAL;
> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	/* Add ECC info retrieval from DT */
> +	if (ecc->strength <= 16) {
> +		ecc->strength = 16;
> +		data->mode = 0;
> +	} else if (ecc->strength <= 24) {
> +		ecc->strength = 24;
> +		data->mode = 1;
> +	} else if (ecc->strength <= 28) {
> +		ecc->strength = 28;
> +		data->mode = 2;
> +	} else if (ecc->strength <= 32) {
> +		ecc->strength = 32;
> +		data->mode = 3;
> +	} else if (ecc->strength <= 40) {
> +		ecc->strength = 40;
> +		data->mode = 4;
> +	} else if (ecc->strength <= 48) {
> +		ecc->strength = 48;
> +		data->mode = 5;
> +	} else if (ecc->strength <= 56) {
> +		ecc->strength = 56;
> +		data->mode = 6;
> +	} else if (ecc->strength <= 60) {
> +		ecc->strength = 60;
> +		data->mode = 7;
> +	} else if (ecc->strength <= 64) {
> +		ecc->strength = 64;
> +		data->mode = 8;
> +	} else {
> +		pr_err("unsupported strength\n");



> +		return -ENOTSUPP;

You're leaking the 'data' allocated above in here.

> +	}
> +
> +	/* HW ECC always request ECC bytes for 1024 bytes blocks */
> +	ecc->bytes = ((ecc->strength * fls(8 * 1024)) + 7) / 8;
> +
> +	/* HW ECC always work with even numbers of ECC bytes */
> +	if (ecc->bytes % 2)
> +		ecc->bytes++;
> +
> +	layout = &data->layout;
> +	nsectors = mtd->writesize / ecc->size;
> +
> +	if (mtd->oobsize < ((ecc->bytes + 4) * nsectors)) {
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	layout->eccbytes = (ecc->bytes * nsectors);
> +
> +	ecc->layout = layout;
> +	ecc->priv = data;
> +
> +	return 0;
> +
> +err:
> +	kfree(data);
> +
> +	return ret;
> +}
> +
> +static void sunxi_nand_hw_common_ecc_ctrl_cleanup(struct nand_ecc_ctrl *ecc)
> +{
> +	kfree(ecc->priv);
> +}
> +
> +static int sunxi_nand_hw_ecc_ctrl_init(struct mtd_info *mtd,
> +				       struct nand_ecc_ctrl *ecc,
> +				       struct device_node *np)
> +{
> +	struct nand_ecclayout *layout;
> +	int nsectors;
> +	int i, j;
> +	int ret;
> +
> +	ret = sunxi_nand_hw_common_ecc_ctrl_init(mtd, ecc, np);
> +	if (ret)
> +		return ret;
> +
> +	ecc->read_page = sunxi_nfc_hw_ecc_read_page;
> +	ecc->write_page = sunxi_nfc_hw_ecc_write_page;
> +	layout = ecc->layout;
> +	nsectors = mtd->writesize / ecc->size;
> +	/*
> +	 * The first 2 bytes are used for BB markers.
> +	 * We merge the 4 user available bytes from HW ECC with this
> +	 * first section, hence why the + 2 operation (- 2 + 4).
> +	 */
> +	layout->oobfree[0].length = mtd->oobsize + 2 -
> +				    ((ecc->bytes + 4) * nsectors);
> +	layout->oobfree[0].offset = 2;
> +	for (i = 0; i < nsectors; i++) {
> +		/*
> +		 * The first 4 ECC block bytes are already counted in the first
> +		 * oobfree entry.
> +		 */
> +		if (i) {
> +			layout->oobfree[i].offset =
> +				layout->oobfree[i - 1].offset +
> +				layout->oobfree[i - 1].length +
> +				ecc->bytes;
> +			layout->oobfree[i].length = 4;
> +		}
> +
> +		for (j = 0; j < ecc->bytes; j++)
> +			layout->eccpos[(ecc->bytes * i) + j] =
> +					layout->oobfree[i].offset +
> +					layout->oobfree[i].length + j;
> +	}
> +
> +	return 0;
> +}
> +
> +static int sunxi_nand_hw_syndrome_ecc_ctrl_init(struct mtd_info *mtd,
> +						struct nand_ecc_ctrl *ecc,
> +						struct device_node *np)
> +{
> +	struct nand_ecclayout *layout;
> +	int nsectors;
> +	int i;
> +	int ret;
> +
> +	ret = sunxi_nand_hw_common_ecc_ctrl_init(mtd, ecc, np);
> +	if (ret)
> +		return ret;
> +
> +	ecc->prepad = 4;
> +	ecc->read_page = sunxi_nfc_hw_syndrome_ecc_read_page;
> +	ecc->write_page = sunxi_nfc_hw_syndrome_ecc_write_page;
> +
> +	layout = ecc->layout;
> +	nsectors = mtd->writesize / ecc->size;
> +
> +	for (i = 0; i < (ecc->bytes * nsectors); i++)
> +		layout->eccpos[i] = i;
> +
> +	layout->oobfree[0].length = mtd->oobsize - i;
> +	layout->oobfree[0].offset = i;
> +
> +	return 0;
> +}
> +
> +static void sunxi_nand_ecc_cleanup(struct nand_ecc_ctrl *ecc)
> +{
> +	switch (ecc->mode) {
> +	case NAND_ECC_HW:
> +	case NAND_ECC_HW_SYNDROME:
> +		sunxi_nand_hw_common_ecc_ctrl_cleanup(ecc);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +static int sunxi_nand_ecc_init(struct mtd_info *mtd, struct nand_ecc_ctrl *ecc,
> +			       struct device_node *np)
> +{
> +	struct nand_chip *nand = mtd->priv;
> +	int ecc_step_size, ecc_strength;
> +	int ret;
> +
> +	ecc_step_size = of_get_nand_ecc_step_size(np);
> +	ecc_strength = of_get_nand_ecc_strength(np);
> +	if (ecc_step_size > 0 && ecc_strength > 0) {
> +		ecc->size = ecc_step_size;
> +		ecc->strength = ecc_strength;
> +	} else {
> +		ecc->size = nand->ecc_step_ds;
> +		ecc->strength = nand->ecc_strength_ds;
> +	}
> +

Shouldn't you check the devicetree value is not weaker than the ONFI-obtained?

> +	ecc->mode = of_get_nand_ecc_mode(np);
> +	switch (ecc->mode) {
> +	case NAND_ECC_SOFT_BCH:
> +		if (!ecc->size || !ecc->strength)
> +			return -EINVAL;
> +		ecc->bytes = ((ecc->strength * fls(8 * ecc->size)) + 7) / 8;
> +		break;
> +	case NAND_ECC_HW:
> +		ret = sunxi_nand_hw_ecc_ctrl_init(mtd, ecc, np);
> +		if (ret)
> +			return ret;
> +		break;
> +	case NAND_ECC_HW_SYNDROME:
> +		ret = sunxi_nand_hw_syndrome_ecc_ctrl_init(mtd, ecc, np);
> +		if (ret)
> +			return ret;
> +		break;
> +	case NAND_ECC_NONE:
> +	case NAND_ECC_SOFT:
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int sunxi_nand_chip_init(struct device *dev, struct sunxi_nfc *nfc,
> +				struct device_node *np)
> +{
> +	const struct nand_sdr_timings *timings;
> +	struct sunxi_nand_chip *chip;
> +	struct mtd_part_parser_data ppdata;
> +	struct mtd_info *mtd;
> +	struct nand_chip *nand;
> +	int nsels;
> +	int ret;
> +	int i;
> +	u32 tmp;
> +
> +	if (!of_get_property(np, "reg", &nsels))
> +		return -EINVAL;
> +
> +	nsels /= sizeof(u32);
> +	if (!nsels)
> +		return -EINVAL;
> +
> +	chip = devm_kzalloc(dev,
> +			    sizeof(*chip) +
> +			    (nsels * sizeof(struct sunxi_nand_chip_sel)),
> +			    GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	chip->nsels = nsels;
> +	chip->selected = -1;
> +
> +	for (i = 0; i < nsels; i++) {
> +		ret = of_property_read_u32_index(np, "reg", i, &tmp);
> +		if (ret)
> +			return ret;
> +
> +		if (tmp > 7)
> +			return -EINVAL;
> +
> +		if (test_and_set_bit(tmp, &nfc->assigned_cs))
> +			return -EINVAL;
> +
> +		chip->sels[i].cs = tmp;
> +
> +		if (!of_property_read_u32_index(np, "allwinner,rb", i, &tmp) &&
> +		    tmp < 2) {
> +			chip->sels[i].rb.type = RB_NATIVE;
> +			chip->sels[i].rb.info.nativeid = tmp;
> +		} else {
> +			ret = of_get_named_gpio(np, "rb-gpios", i);
> +			if (ret >= 0) {
> +				tmp = ret;
> +				chip->sels[i].rb.type = RB_GPIO;
> +				chip->sels[i].rb.info.gpio = tmp;
> +				ret = devm_gpio_request(dev, tmp, "nand-rb");
> +				if (ret)
> +					return ret;
> +
> +				ret = gpio_direction_input(tmp);
> +				if (ret)
> +					return ret;
> +			} else {
> +				chip->sels[i].rb.type = RB_NONE;
> +			}
> +		}
> +	}
> +
> +	timings = onfi_async_timing_mode_to_sdr_timings(0);
> +	if (IS_ERR(timings))
> +		return PTR_ERR(timings);
> +
> +	ret = sunxi_nand_chip_set_timings(chip, timings);
> +
> +	nand = &chip->nand;
> +	/* Default tR value specified in the ONFI spec (chapter 4.15.1) */
> +	nand->chip_delay = 200;
> +	nand->controller = &nfc->controller;
> +	nand->select_chip = sunxi_nfc_select_chip;
> +	nand->cmd_ctrl = sunxi_nfc_cmd_ctrl;
> +	nand->read_buf = sunxi_nfc_read_buf;
> +	nand->write_buf = sunxi_nfc_write_buf;
> +	nand->read_byte = sunxi_nfc_read_byte;
> +
> +	if (of_get_nand_on_flash_bbt(np))
> +		nand->bbt_options |= NAND_BBT_USE_FLASH;
> +
> +	mtd = &chip->mtd;
> +	mtd->dev.parent = dev;
> +	mtd->priv = nand;
> +	mtd->owner = THIS_MODULE;
> +
> +	ret = nand_scan_ident(mtd, nsels, NULL);
> +	if (ret)
> +		return ret;
> +
> +	ret = sunxi_nand_chip_init_timings(chip, np);
> +	if (ret)
> +		return ret;
> +
> +	ret = sunxi_nand_ecc_init(mtd, &nand->ecc, np);
> +	if (ret)
> +		return ret;
> +
> +	ret = nand_scan_tail(mtd);
> +	if (ret)
> +		return ret;
> +
> +	if (of_property_read_string(np, "nand-name", &mtd->name)) {
> +		snprintf(chip->default_name, MAX_NAME_SIZE,
> +			 DEFAULT_NAME_FORMAT, chip->sels[i].cs);
> +		mtd->name = chip->default_name;
> +	}
> +
> +	ppdata.of_node = np;
> +	ret = mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0);
> +	if (!ret)

Aren't you leaking the BBT resources allocated in nand_scan_tail? I **think**
you need to call nand_release if the mtd device register fails.

> +		return ret;
> +
> +	list_add_tail(&chip->node, &nfc->chips);
> +
> +	return 0;
> +}
> +
> +static int sunxi_nand_chips_init(struct device *dev, struct sunxi_nfc *nfc)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct device_node *nand_np;
> +	int nchips = of_get_child_count(np);
> +	int ret;
> +
> +	if (nchips > 8)
> +		return -EINVAL;
> +
> +	for_each_child_of_node(np, nand_np) {
> +		ret = sunxi_nand_chip_init(dev, nfc, nand_np);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void sunxi_nand_chips_cleanup(struct sunxi_nfc *nfc)
> +{
> +	struct sunxi_nand_chip *chip;
> +
> +	while (!list_empty(&nfc->chips)) {
> +		chip = list_first_entry(&nfc->chips, struct sunxi_nand_chip,
> +					node);
> +		nand_release(&chip->mtd);
> +		sunxi_nand_ecc_cleanup(&chip->nand.ecc);
> +	}
> +}
> +
> +static int sunxi_nfc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct resource *r;
> +	struct sunxi_nfc *nfc;
> +	int ret;
> +
> +	nfc = devm_kzalloc(dev, sizeof(*nfc), GFP_KERNEL);
> +	if (!nfc) {
> +		dev_err(dev, "failed to allocate NFC struct\n");

OOM message not needed.

> +		return -ENOMEM;
> +	}
> +
> +	spin_lock_init(&nfc->controller.lock);
> +	init_waitqueue_head(&nfc->controller.wq);
> +	INIT_LIST_HEAD(&nfc->chips);
> +
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	nfc->regs = devm_ioremap_resource(dev, r);
> +	if (IS_ERR(nfc->regs)) {
> +		dev_err(dev, "failed to remap iomem\n");

Message not needed, see devm_ioremap_resource() documentation.

> +		return PTR_ERR(nfc->regs);
> +	}
> +
> +	nfc->irq = platform_get_irq(pdev, 0);
> +	if (nfc->irq < 0) {
> +		dev_err(dev, "failed to retrieve irq\n");
> +		return nfc->irq;
> +	}
> +
> +	nfc->ahb_clk = devm_clk_get(dev, "ahb_clk");
> +	if (IS_ERR(nfc->ahb_clk)) {
> +		dev_err(dev, "failed to retrieve ahb_clk\n");
> +		return PTR_ERR(nfc->ahb_clk);
> +	}
> +
> +	ret = clk_prepare_enable(nfc->ahb_clk);
> +	if (ret)
> +		return ret;
> +
> +	nfc->sclk = devm_clk_get(dev, "sclk");
> +	if (IS_ERR(nfc->sclk)) {
> +		dev_err(dev, "failed to retrieve nand_clk\n");
> +		ret = PTR_ERR(nfc->sclk);
> +		goto out_ahb_clk_unprepare;
> +	}
> +
> +	ret = clk_prepare_enable(nfc->sclk);
> +	if (ret)
> +		goto out_ahb_clk_unprepare;
> +
> +	/* Reset NFC */
> +	writel(readl(nfc->regs + NFC_REG_CTL) | NFC_RESET,
> +	       nfc->regs + NFC_REG_CTL);
> +	while (readl(nfc->regs + NFC_REG_CTL) & NFC_RESET)
> +		;

Again: maybe you should avoid potentially infinite loop, no matter what.

> +
> +	writel(0, nfc->regs + NFC_REG_INT);
> +	ret = devm_request_irq(dev, nfc->irq, sunxi_nfc_interrupt,
> +			       0, "sunxi-nand", nfc);
> +	if (ret)
> +		goto out_sclk_unprepare;
> +
> +	platform_set_drvdata(pdev, nfc);
> +
> +	writel(0x100, nfc->regs + NFC_REG_TIMING_CTL);
> +	writel(0x7ff, nfc->regs + NFC_REG_TIMING_CFG);

Hm... can you remove the magic numbers?

> +
> +	ret = sunxi_nand_chips_init(dev, nfc);
> +	if (ret) {
> +		dev_err(dev, "failed to init nand chips\n");
> +		goto out_sclk_unprepare;
> +	}
> +
> +	return 0;
> +
> +out_sclk_unprepare:
> +	clk_disable_unprepare(nfc->sclk);
> +out_ahb_clk_unprepare:
> +	clk_disable_unprepare(nfc->ahb_clk);
> +
> +	return ret;
> +}
> +
> +static int sunxi_nfc_remove(struct platform_device *pdev)
> +{
> +	struct sunxi_nfc *nfc = platform_get_drvdata(pdev);
> +
> +	sunxi_nand_chips_cleanup(nfc);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sunxi_nfc_ids[] = {
> +	{ .compatible = "allwinner,sun4i-nand" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, sunxi_nfc_ids);
> +
> +static struct platform_driver sunxi_nfc_driver = {
> +	.driver = {
> +		.name = "sunxi_nand",
> +		.owner = THIS_MODULE,
> +		.of_match_table = of_match_ptr(sunxi_nfc_ids),

Redundant of_match_ptr. See 5576bc7bef2919dd2b185bffb768bf9c0da76788.

I think that's all I can spot, without knowing the hardware details.
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 5/9] mtd: nand: add sunxi NAND flash controller support

[Boris BREZILLON]

On 09/05/2014 18:03, Ezequiel Garcia wrote:
> Hello Boris,
>
> Sorry for the review delay.
>
> Emilio, if you have hardware to test this, it would be nice to give
> Boris some Tested-by?
>
> On 12 Mar 07:07 PM, Boris BREZILLON wrote:
>> Add support for the sunxi NAND Flash Controller (NFC).
>>
>> Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
>> ---
>>  drivers/mtd/nand/Kconfig      |    6 +
>>  drivers/mtd/nand/Makefile     |    1 +
>>  drivers/mtd/nand/sunxi_nand.c | 1276 +++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 1283 insertions(+)
>>  create mode 100644 drivers/mtd/nand/sunxi_nand.c
>>
>> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
>> index 90ff447..8a28c06 100644
>> --- a/drivers/mtd/nand/Kconfig
>> +++ b/drivers/mtd/nand/Kconfig
>> @@ -510,4 +510,10 @@ config MTD_NAND_XWAY
>>  	  Enables support for NAND Flash chips on Lantiq XWAY SoCs. NAND is attached
>>  	  to the External Bus Unit (EBU).
>>  
>> +config MTD_NAND_SUNXI
>> +	tristate "Support for NAND on Allwinner SoCs"
>> +	depends on ARCH_SUNXI
>> +	help
>> +	  Enables support for NAND Flash chips on Allwinner SoCs.
>> +
>>  endif # MTD_NAND
>> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
>> index 0b8a822..34f45d8 100644
>> --- a/drivers/mtd/nand/Makefile
>> +++ b/drivers/mtd/nand/Makefile
>> @@ -49,5 +49,6 @@ obj-$(CONFIG_MTD_NAND_JZ4740)		+= jz4740_nand.o
>>  obj-$(CONFIG_MTD_NAND_GPMI_NAND)	+= gpmi-nand/
>>  obj-$(CONFIG_MTD_NAND_XWAY)		+= xway_nand.o
>>  obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH)	+= bcm47xxnflash/
>> +obj-$(CONFIG_MTD_NAND_SUNXI)		+= sunxi_nand.o
>>  
>>  nand-objs := nand_base.o nand_bbt.o
>> diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c
>> new file mode 100644
>> index 0000000..e93cc44
>> --- /dev/null
>> +++ b/drivers/mtd/nand/sunxi_nand.c
>> @@ -0,0 +1,1276 @@
>> +/*
>> + * Copyright (C) 2013 Boris BREZILLON <b.brezillon.dev@gmail.com>
>> + *
>> + * Derived from:
>> + *	https://github.com/yuq/sunxi-nfc-mtd
>> + *	Copyright (C) 2013 Qiang Yu <yuq825@gmail.com>
>> + *
>> + *	https://github.com/hno/Allwinner-Info
>> + *	Copyright (C) 2013 Henrik Nordstr?m <Henrik Nordstr?m>
>> + *
>> + *	Copyright (C) 2013 Dmitriy B. <rzk333@gmail.com>
>> + *	Copyright (C) 2013 Sergey Lapin <slapin@ossfans.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/dma-mapping.h>
>> +#include <linux/slab.h>
>> +#include <linux/module.h>
>> +#include <linux/moduleparam.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/of_mtd.h>
>> +#include <linux/mtd/mtd.h>
>> +#include <linux/mtd/nand.h>
>> +#include <linux/mtd/partitions.h>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/dmaengine.h>
>> +#include <linux/gpio.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +
>> +#define NFC_REG_CTL		0x0000
>> +#define NFC_REG_ST		0x0004
>> +#define NFC_REG_INT		0x0008
>> +#define NFC_REG_TIMING_CTL	0x000C
>> +#define NFC_REG_TIMING_CFG	0x0010
>> +#define NFC_REG_ADDR_LOW	0x0014
>> +#define NFC_REG_ADDR_HIGH	0x0018
>> +#define NFC_REG_SECTOR_NUM	0x001C
>> +#define NFC_REG_CNT		0x0020
>> +#define NFC_REG_CMD		0x0024
>> +#define NFC_REG_RCMD_SET	0x0028
>> +#define NFC_REG_WCMD_SET	0x002C
>> +#define NFC_REG_IO_DATA		0x0030
>> +#define NFC_REG_ECC_CTL		0x0034
>> +#define NFC_REG_ECC_ST		0x0038
>> +#define NFC_REG_DEBUG		0x003C
>> +#define NFC_REG_ECC_CNT0	0x0040
>> +#define NFC_REG_ECC_CNT1	0x0044
>> +#define NFC_REG_ECC_CNT2	0x0048
>> +#define NFC_REG_ECC_CNT3	0x004c
>> +#define NFC_REG_USER_DATA_BASE	0x0050
>> +#define NFC_REG_SPARE_AREA	0x00A0
>> +#define NFC_RAM0_BASE		0x0400
>> +#define NFC_RAM1_BASE		0x0800
>> +
>> +/*define bit use in NFC_CTL*/
> nit: Use BIT() for these?

Okay.

>
>> +#define NFC_EN				(1 << 0)
>> +#define NFC_RESET			(1 << 1)
>> +#define NFC_BUS_WIDYH			(1 << 2)
>> +#define NFC_RB_SEL			(1 << 3)
>> +#define NFC_CE_SEL			(7 << 24)
>> +#define NFC_CE_CTL			(1 << 6)
>> +#define NFC_CE_CTL1			(1 << 7)
>> +#define NFC_PAGE_SIZE			(0xf << 8)
>> +#define NFC_SAM				(1 << 12)
>> +#define NFC_RAM_METHOD			(1 << 14)
>> +#define NFC_DEBUG_CTL			(1 << 31)
>> +
>> +/*define bit use in NFC_ST*/
>> +#define NFC_RB_B2R			(1 << 0)
>> +#define NFC_CMD_INT_FLAG		(1 << 1)
>> +#define NFC_DMA_INT_FLAG		(1 << 2)
>> +#define NFC_CMD_FIFO_STATUS		(1 << 3)
>> +#define NFC_STA				(1 << 4)
>> +#define NFC_NATCH_INT_FLAG		(1 << 5)
>> +#define NFC_RB_STATE0			(1 << 8)
>> +#define NFC_RB_STATE1			(1 << 9)
>> +#define NFC_RB_STATE2			(1 << 10)
>> +#define NFC_RB_STATE3			(1 << 11)
>> +
>> +/*define bit use in NFC_INT*/
>> +#define NFC_B2R_INT_ENABLE		(1 << 0)
>> +#define NFC_CMD_INT_ENABLE		(1 << 1)
>> +#define NFC_DMA_INT_ENABLE		(1 << 2)
>> +#define NFC_INT_MASK			(NFC_B2R_INT_ENABLE | \
>> +					 NFC_CMD_INT_ENABLE | \
>> +					 NFC_DMA_INT_ENABLE)
>> +
>> +
>> +/*define bit use in NFC_CMD*/
>> +#define NFC_CMD_LOW_BYTE		(0xff << 0)
>> +#define NFC_CMD_HIGH_BYTE		(0xff << 8)
>> +#define NFC_ADR_NUM			(0x7 << 16)
>> +#define NFC_SEND_ADR			(1 << 19)
>> +#define NFC_ACCESS_DIR			(1 << 20)
>> +#define NFC_DATA_TRANS			(1 << 21)
>> +#define NFC_SEND_CMD1			(1 << 22)
>> +#define NFC_WAIT_FLAG			(1 << 23)
>> +#define NFC_SEND_CMD2			(1 << 24)
>> +#define NFC_SEQ				(1 << 25)
>> +#define NFC_DATA_SWAP_METHOD		(1 << 26)
>> +#define NFC_ROW_AUTO_INC		(1 << 27)
>> +#define NFC_SEND_CMD3			(1 << 28)
>> +#define NFC_SEND_CMD4			(1 << 29)
>> +#define NFC_CMD_TYPE			(3 << 30)
>> +
>> +/* define bit use in NFC_RCMD_SET*/
>> +#define NFC_READ_CMD			(0xff << 0)
>> +#define NFC_RANDOM_READ_CMD0		(0xff << 8)
>> +#define NFC_RANDOM_READ_CMD1		(0xff << 16)
>> +
>> +/*define bit use in NFC_WCMD_SET*/
>> +#define NFC_PROGRAM_CMD			(0xff << 0)
>> +#define NFC_RANDOM_WRITE_CMD		(0xff << 8)
>> +#define NFC_READ_CMD0			(0xff << 16)
>> +#define NFC_READ_CMD1			(0xff << 24)
>> +
>> +/*define bit use in NFC_ECC_CTL*/
>> +#define NFC_ECC_EN			(1 << 0)
>> +#define NFC_ECC_PIPELINE		(1 << 3)
>> +#define NFC_ECC_EXCEPTION		(1 << 4)
>> +#define NFC_ECC_BLOCK_SIZE		(1 << 5)
>> +#define NFC_RANDOM_EN			(1 << 9)
>> +#define NFC_RANDOM_DIRECTION		(1 << 10)
>> +#define NFC_ECC_MODE_SHIFT		12
>> +#define NFC_ECC_MODE			(0xf << NFC_ECC_MODE_SHIFT)
>> +#define NFC_RANDOM_SEED			(0x7fff << 16)
>> +
>> +
>> +
> Kill and avoid these extra empty lines.

Sure.

>
>> +enum sunxi_nand_rb_type {
>> +	RB_NONE,
>> +	RB_NATIVE,
>> +	RB_GPIO,
>> +};
>> +
> Can you add some documentation about this read-back stuff?

This has nothing to do with read-back, RB means ready/busy :-).

> I know documenting is not a super-fun activity, but it's a bit tough
> to review otherwise.

Okay, I'll try to spend more time documenting non obvious stuff ;-)

>
>> +struct sunxi_nand_rb {
>> +	enum sunxi_nand_rb_type type;
>> +	union {
>> +		int gpio;
>> +		int nativeid;
>> +	} info;
>> +};
>> +
>> +struct sunxi_nand_chip_sel {
>> +	u8 cs;
>> +	struct sunxi_nand_rb rb;
>> +};
>> +
>> +#define DEFAULT_NAME_FORMAT	"nand@%d"
>> +#define MAX_NAME_SIZE		(sizeof("nand@") + 2)
>> +
>> +struct sunxi_nand_hw_ecc {
>> +	int mode;
>> +	struct nand_ecclayout layout;
>> +};
>> +
>> +struct sunxi_nand_chip {
>> +	struct list_head node;
>> +	struct nand_chip nand;
>> +	struct mtd_info mtd;
>> +	char default_name[MAX_NAME_SIZE];
>> +	unsigned long clk_rate;
>> +	int selected;
>> +	int nsels;
>> +	struct sunxi_nand_chip_sel sels[0];
> Hm... you prepare the whole multiple chip support but you really support a
> single chip? I'd say drop entirely and support just a single chip, it'll
> make the driver much cleaner. We can always add the support later, after
> proper testing.

Really, I spent so much time supporting multiple chips!
No I'm kidding :-), this was pretty simple, hence I added multiple chips
support right away.

Anyway, even if I drop this multiple chip support I want to keep the DT
bindings as proposed (one node for the controller and the nand chip
defined as child node of this controller node) to avoid breaking the DT
ABI in future versions.

>> +};
>> +
>> +static inline struct sunxi_nand_chip *to_sunxi_nand(struct nand_chip *nand)
>> +{
>> +	return container_of(nand, struct sunxi_nand_chip, nand);
>> +}
>> +
>> +struct sunxi_nfc {
>> +	struct nand_hw_control controller;
>> +	void __iomem *regs;
>> +	int irq;
> You don't seem to need the irq stored anywhere, as you're using
> devm_request_irq.

Okay, I'll drop this field.

>
>> +	struct clk *ahb_clk;
>> +	struct clk *sclk;
>> +	unsigned long assigned_cs;
>> +	unsigned long clk_rate;
>> +	struct list_head chips;
>> +	struct completion complete;
>> +};
>> +
>> +static inline struct sunxi_nfc *to_sunxi_nfc(struct nand_hw_control *ctrl)
>> +{
>> +	return container_of(ctrl, struct sunxi_nfc, controller);
>> +}
>> +
>> +static irqreturn_t sunxi_nfc_interrupt(int irq, void *dev_id)
>> +{
>> +	struct sunxi_nfc *nfc = dev_id;
>> +	u32 st = readl(nfc->regs + NFC_REG_ST);
>> +	u32 ien = readl(nfc->regs + NFC_REG_INT);
>> +
>> +	if (!(ien & st))
>> +		return IRQ_NONE;
>> +
>> +	if ((ien & st) == ien)
>> +		complete(&nfc->complete);
>> +
>> +	writel(st & NFC_INT_MASK, nfc->regs + NFC_REG_ST);
>> +	writel(~st & ien & NFC_INT_MASK, nfc->regs + NFC_REG_INT);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int sunxi_nfc_wait_int(struct sunxi_nfc *nfc, u32 flags,
>> +			      unsigned int timeout_ms)
>> +{
>> +	init_completion(&nfc->complete);
>> +
>> +	writel(flags, nfc->regs + NFC_REG_INT);
>> +	if (!timeout_ms)
>> +		wait_for_completion(&nfc->complete);
> In the same vein as infinite loops (see below), I'd avoid this no-timeout wait.
> Or maybe you can *guarantee* it'll be always completed?

Sure, I'll just return an error is timeout_ms is set to 0.

>
>> +	else if (!wait_for_completion_timeout(&nfc->complete,
>> +					      msecs_to_jiffies(timeout_ms)))
>> +		return -ETIMEDOUT;
>> +
>> +	return 0;
>> +}
>> +
>> +static int sunxi_nfc_dev_ready(struct mtd_info *mtd)
>> +{
>> +	struct nand_chip *nand = mtd->priv;
>> +	struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
>> +	struct sunxi_nfc *nfc = to_sunxi_nfc(sunxi_nand->nand.controller);
>> +	struct sunxi_nand_rb *rb;
>> +	unsigned long timeo = (sunxi_nand->nand.state == FL_ERASING ? 400 : 20);
>> +	int ret;
>> +
>> +	if (sunxi_nand->selected < 0)
>> +		return 0;
>> +
>> +	rb = &sunxi_nand->sels[sunxi_nand->selected].rb;
>> +
>> +	switch (rb->type) {
>> +	case RB_NATIVE:
>> +		ret = !!(readl(nfc->regs + NFC_REG_ST) &
>> +			 (NFC_RB_STATE0 << rb->info.nativeid));
>> +		if (ret)
>> +			break;
>> +
>> +		sunxi_nfc_wait_int(nfc, NFC_RB_B2R, timeo);
>> +		ret = !!(readl(nfc->regs + NFC_REG_ST) &
>> +			 (NFC_RB_STATE0 << rb->info.nativeid));
>> +		break;
>> +	case RB_GPIO:
>> +		ret = gpio_get_value(rb->info.gpio);
>> +		break;
>> +	case RB_NONE:
>> +	default:
>> +		ret = 0;
>> +		pr_err("cannot check R/B NAND status!");
> I'd suggest avoiding this kind of pr_err. The user won't know
> who's saying this. Try using dev_err or pr_fmt.

I cannot use devm here, because this function might be called before
mtd_info registration (called from nand_scan_ident).
I'll define the pr_fmt macro instead.

>
>> +		break;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static void sunxi_nfc_select_chip(struct mtd_info *mtd, int chip)
>> +{
>> +	struct nand_chip *nand = mtd->priv;
>> +	struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
>> +	struct sunxi_nfc *nfc = to_sunxi_nfc(sunxi_nand->nand.controller);
>> +	struct sunxi_nand_chip_sel *sel;
>> +	u32 ctl;
>> +
>> +	if (chip > 0 && chip >= sunxi_nand->nsels)
>> +		return;
>> +
>> +	if (chip == sunxi_nand->selected)
>> +		return;
>> +
>> +	ctl = readl(nfc->regs + NFC_REG_CTL) &
>> +	      ~(NFC_CE_SEL | NFC_RB_SEL | NFC_EN);
>> +
>> +	if (chip >= 0) {
>> +		sel = &sunxi_nand->sels[chip];
>> +
>> +		ctl |= (sel->cs << 24) | NFC_EN |
>> +		       (((nand->page_shift - 10) & 0xf) << 8);
>> +		if (sel->rb.type == RB_NONE) {
>> +			nand->dev_ready = NULL;
>> +		} else {
>> +			nand->dev_ready = sunxi_nfc_dev_ready;
>> +			if (sel->rb.type == RB_NATIVE)
>> +				ctl |= (sel->rb.info.nativeid << 3);
>> +		}
>> +
>> +		writel(mtd->writesize, nfc->regs + NFC_REG_SPARE_AREA);
>> +
>> +		if (nfc->clk_rate != sunxi_nand->clk_rate) {
>> +			clk_set_rate(nfc->sclk, sunxi_nand->clk_rate);
>> +			nfc->clk_rate = sunxi_nand->clk_rate;
>> +		}
>> +	}
>> +
>> +	writel(ctl, nfc->regs + NFC_REG_CTL);
>> +
>> +	sunxi_nand->selected = chip;
>> +}
>> +
>> +static void sunxi_nfc_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
>> +{
>> +	struct nand_chip *nand = mtd->priv;
>> +	struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
>> +	struct sunxi_nfc *nfc = to_sunxi_nfc(sunxi_nand->nand.controller);
>> +	int cnt;
>> +	int offs = 0;
>> +	u32 tmp;
>> +
>> +	while (len > offs) {
>> +		cnt = len - offs;
>> +		if (cnt > 1024)
>> +			cnt = 1024;
>> +
>> +		while ((readl(nfc->regs + NFC_REG_ST) & NFC_CMD_FIFO_STATUS))
>> +			;
> How about avoiding the infinite loop?

Okay, just have to figure out what the appropriate timeout is.

>
>> +		writel(cnt, nfc->regs + NFC_REG_CNT);
>> +		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD;
>> +		writel(tmp, nfc->regs + NFC_REG_CMD);
>> +		sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
>> +		if (buf)
>> +			memcpy_fromio(buf + offs, nfc->regs + NFC_RAM0_BASE,
>> +				      cnt);
>> +		offs += cnt;
>> +	}
>> +}
>> +
>> +static void sunxi_nfc_write_buf(struct mtd_info *mtd, const uint8_t *buf,
>> +				int len)
>> +{
>> +	struct nand_chip *nand = mtd->priv;
>> +	struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
>> +	struct sunxi_nfc *nfc = to_sunxi_nfc(sunxi_nand->nand.controller);
>> +	int cnt;
>> +	int offs = 0;
>> +	u32 tmp;
>> +
>> +	while (len > offs) {
>> +		cnt = len - offs;
>> +		if (cnt > 1024)
>> +			cnt = 1024;
>> +
>> +		while ((readl(nfc->regs + NFC_REG_ST) & NFC_CMD_FIFO_STATUS))
>> +			;
> Ditto.

Same as above, I'll have to find an appropriate timeout value.

>
>> +		writel(cnt, nfc->regs + NFC_REG_CNT);
>> +		memcpy_toio(nfc->regs + NFC_RAM0_BASE, buf + offs, cnt);
>> +		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD |
>> +		      NFC_ACCESS_DIR;
>> +		writel(tmp, nfc->regs + NFC_REG_CMD);
>> +		sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
>> +		offs += cnt;
>> +	}
>> +}
>> +
>> +static uint8_t sunxi_nfc_read_byte(struct mtd_info *mtd)
>> +{
>> +	uint8_t ret;
>> +
>> +	sunxi_nfc_read_buf(mtd, &ret, 1);
>> +
>> +	return ret;
>> +}
>> +
>> +static void sunxi_nfc_cmd_ctrl(struct mtd_info *mtd, int dat,
>> +			       unsigned int ctrl)
>> +{
>> +	struct nand_chip *nand = mtd->priv;
>> +	struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
>> +	struct sunxi_nfc *nfc = to_sunxi_nfc(sunxi_nand->nand.controller);
>> +	u32 tmp;
>> +
>> +	while ((readl(nfc->regs + NFC_REG_ST) & NFC_CMD_FIFO_STATUS))
>> +		;
>> +
> Ditto.
>
>> +	if (ctrl & NAND_CTRL_CHANGE) {
>> +		tmp = readl(nfc->regs + NFC_REG_CTL);
>> +		if (ctrl & NAND_NCE)
>> +			tmp |= NFC_CE_CTL;
>> +		else
>> +			tmp &= ~NFC_CE_CTL;
>> +		writel(tmp, nfc->regs + NFC_REG_CTL);
>> +	}
>> +
>> +	if (dat == NAND_CMD_NONE)
>> +		return;
>> +
>> +	if (ctrl & NAND_CLE) {
>> +		writel(NFC_SEND_CMD1 | dat, nfc->regs + NFC_REG_CMD);
>> +	} else {
>> +		writel(dat, nfc->regs + NFC_REG_ADDR_LOW);
>> +		writel(NFC_SEND_ADR, nfc->regs + NFC_REG_CMD);
>> +	}
>> +
>> +	sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
>> +}
>> +
>> +static int sunxi_nfc_hw_ecc_read_page(struct mtd_info *mtd,
>> +				      struct nand_chip *chip, uint8_t *buf,
>> +				      int oob_required, int page)
>> +{
>> +	struct sunxi_nfc *nfc = to_sunxi_nfc(chip->controller);
>> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
>> +	struct nand_ecclayout *layout = ecc->layout;
>> +	struct sunxi_nand_hw_ecc *data = ecc->priv;
>> +	int steps = mtd->writesize / ecc->size;
>> +	unsigned int max_bitflips = 0;
>> +	int offset;
>> +	u32 tmp;
>> +	int i;
>> +	int cnt;
>> +
>> +	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
>> +	tmp &= ~(NFC_ECC_MODE | NFC_ECC_PIPELINE | NFC_ECC_BLOCK_SIZE |
>> +		 NFC_ECC_BLOCK_SIZE);
>> +	tmp |= NFC_ECC_EN | (data->mode << NFC_ECC_MODE_SHIFT);
>> +
>> +	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
>> +
>> +	for (i = 0; i < steps; i++) {
>> +		if (i)
>> +			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, i * ecc->size, -1);
>> +
>> +		offset = mtd->writesize + layout->eccpos[i * ecc->bytes] - 4;
>> +
>> +		chip->read_buf(mtd, NULL, ecc->size);
>> +
>> +		chip->cmdfunc(mtd, NAND_CMD_RNDOUT, offset, -1);
>> +		while ((readl(nfc->regs + NFC_REG_ST) & NFC_CMD_FIFO_STATUS))
>> +			;
>> +
> Ditto.
>
>> +		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | (1 << 30);
>> +		writel(tmp, nfc->regs + NFC_REG_CMD);
>> +		sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
>> +		memcpy_fromio(buf + (i * ecc->size),
>> +			      nfc->regs + NFC_RAM0_BASE, ecc->size);
>> +
>> +		if (readl(nfc->regs + NFC_REG_ECC_ST) & 0x1) {
>> +			mtd->ecc_stats.failed++;
>> +		} else {
>> +			tmp = readl(nfc->regs + NFC_REG_ECC_CNT0) & 0xff;
>> +			mtd->ecc_stats.corrected += tmp;
>> +			max_bitflips = max_t(unsigned int, max_bitflips, tmp);
>> +		}
>> +
>> +		if (oob_required) {
>> +			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, offset, -1);
>> +			while ((readl(nfc->regs + NFC_REG_ST) &
>> +			       NFC_CMD_FIFO_STATUS))
>> +				;
> Ditto.
>
>> +			offset -= mtd->writesize;
>> +			chip->read_buf(mtd, chip->oob_poi + offset,
>> +				      ecc->bytes + 4);
>> +		}
>> +	}
>> +
>> +	if (oob_required) {
>> +		cnt = ecc->layout->oobfree[0].length - 4;
>> +		if (cnt > 0) {
>> +			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, mtd->writesize,
>> +				      -1);
>> +			chip->read_buf(mtd, chip->oob_poi, cnt);
>> +		}
>> +	}
>> +
>> +	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
>> +	tmp &= ~NFC_ECC_EN;
>> +
>> +	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
>> +
>> +	return max_bitflips;
>> +}
>> +
>> +static int sunxi_nfc_hw_ecc_write_page(struct mtd_info *mtd,
>> +				       struct nand_chip *chip,
>> +				       const uint8_t *buf, int oob_required)
>> +{
>> +	struct sunxi_nfc *nfc = to_sunxi_nfc(chip->controller);
>> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
>> +	struct nand_ecclayout *layout = ecc->layout;
>> +	struct sunxi_nand_hw_ecc *data = ecc->priv;
>> +	int offset;
>> +	u32 tmp;
>> +	int i;
>> +	int cnt;
>> +
>> +	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
>> +	tmp &= ~(NFC_ECC_MODE | NFC_ECC_PIPELINE | NFC_ECC_BLOCK_SIZE |
>> +		 NFC_ECC_BLOCK_SIZE);
>> +	tmp |= NFC_ECC_EN | (data->mode << NFC_ECC_MODE_SHIFT);
>> +
>> +	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
>> +
>> +	for (i = 0; i < mtd->writesize / ecc->size; i++) {
>> +		if (i)
>> +			chip->cmdfunc(mtd, NAND_CMD_RNDIN, i * ecc->size, -1);
>> +
>> +		chip->write_buf(mtd, buf + (i * ecc->size), ecc->size);
>> +
>> +		offset = layout->eccpos[i * ecc->bytes] - 4 + mtd->writesize;
>> +
>> +		/* Fill OOB data in */
>> +		if (oob_required) {
>> +			tmp = 0xffffffff;
>> +			memcpy_toio(nfc->regs + NFC_REG_USER_DATA_BASE, &tmp,
>> +				    4);
>> +		} else {
>> +			memcpy_toio(nfc->regs + NFC_REG_USER_DATA_BASE,
>> +				    chip->oob_poi + offset - mtd->writesize,
>> +				    4);
>> +		}
>> +
>> +		chip->cmdfunc(mtd, NAND_CMD_RNDIN, offset, -1);
>> +		while ((readl(nfc->regs + NFC_REG_ST) &
>> +		       NFC_CMD_FIFO_STATUS))
>> +			;
>> +
> Ditto.
>
>> +		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | NFC_ACCESS_DIR |
>> +		      (1 << 30);
>> +		writel(tmp, nfc->regs + NFC_REG_CMD);
>> +		sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
>> +	}
>> +
>> +	if (oob_required) {
>> +		cnt = ecc->layout->oobfree[0].length - 4;
>> +		if (cnt > 0) {
>> +			chip->cmdfunc(mtd, NAND_CMD_RNDIN, mtd->writesize, -1);
>> +			chip->write_buf(mtd, chip->oob_poi, cnt);
>> +		}
>> +	}
>> +
>> +	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
>> +	tmp &= ~(NFC_ECC_EN | NFC_ECC_PIPELINE);
>> +
>> +	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
>> +
>> +	return 0;
>> +}
>> +
>> +static int sunxi_nfc_hw_syndrome_ecc_read_page(struct mtd_info *mtd,
>> +					       struct nand_chip *chip,
>> +					       uint8_t *buf, int oob_required,
>> +					       int page)
>> +{
>> +	struct sunxi_nfc *nfc = to_sunxi_nfc(chip->controller);
>> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
>> +	struct sunxi_nand_hw_ecc *data = ecc->priv;
>> +	int steps = mtd->writesize / ecc->size;
>> +	unsigned int max_bitflips = 0;
>> +	uint8_t *oob = chip->oob_poi;
>> +	int offset = 0;
>> +	int cnt;
>> +	u32 tmp;
>> +	int i;
>> +
>> +	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
>> +	tmp &= ~(NFC_ECC_MODE | NFC_ECC_PIPELINE | NFC_ECC_BLOCK_SIZE |
>> +		 NFC_ECC_BLOCK_SIZE);
>> +	tmp |= NFC_ECC_EN | (data->mode << NFC_ECC_MODE_SHIFT);
>> +
>> +	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
>> +
>> +	for (i = 0; i < steps; i++) {
>> +		chip->read_buf(mtd, NULL, ecc->size);
>> +
>> +		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | (1 << 30);
>> +		writel(tmp, nfc->regs + NFC_REG_CMD);
>> +		sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
>> +		memcpy_fromio(buf, nfc->regs + NFC_RAM0_BASE, ecc->size);
>> +		buf += ecc->size;
>> +		offset += ecc->size;
>> +
>> +		if (readl(nfc->regs + NFC_REG_ECC_ST) & 0x1) {
>> +			mtd->ecc_stats.failed++;
>> +		} else {
>> +			tmp = readl(nfc->regs + NFC_REG_ECC_CNT0) & 0xff;
>> +			mtd->ecc_stats.corrected += tmp;
>> +			max_bitflips = max_t(unsigned int, max_bitflips, tmp);
>> +		}
>> +
>> +		if (oob_required) {
>> +			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, offset, -1);
>> +			chip->read_buf(mtd, oob, ecc->bytes + ecc->prepad);
>> +			oob += ecc->bytes + ecc->prepad;
>> +		}
>> +
>> +		offset += ecc->bytes + ecc->prepad;
>> +	}
>> +
>> +	if (oob_required) {
>> +		cnt = mtd->oobsize - (oob - chip->oob_poi);
>> +		if (cnt > 0) {
>> +			chip->cmdfunc(mtd, NAND_CMD_RNDOUT, offset, -1);
>> +			chip->read_buf(mtd, oob, cnt);
>> +		}
>> +	}
>> +
>> +	writel(readl(nfc->regs + NFC_REG_ECC_CTL) & ~NFC_ECC_EN,
>> +	       nfc->regs + NFC_REG_ECC_CTL);
>> +
>> +	return max_bitflips;
>> +}
>> +
>> +static int sunxi_nfc_hw_syndrome_ecc_write_page(struct mtd_info *mtd,
>> +						struct nand_chip *chip,
>> +						const uint8_t *buf,
>> +						int oob_required)
>> +{
>> +	struct sunxi_nfc *nfc = to_sunxi_nfc(chip->controller);
>> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
>> +	struct sunxi_nand_hw_ecc *data = ecc->priv;
>> +	int steps = mtd->writesize / ecc->size;
>> +	uint8_t *oob = chip->oob_poi;
>> +	int offset = 0;
>> +	int cnt;
>> +	u32 tmp;
>> +	int i;
>> +
>> +	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
>> +	tmp &= ~(NFC_ECC_MODE | NFC_ECC_PIPELINE | NFC_ECC_BLOCK_SIZE |
>> +		 NFC_ECC_BLOCK_SIZE);
>> +	tmp |= NFC_ECC_EN | (data->mode << NFC_ECC_MODE_SHIFT);
>> +
>> +	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
>> +
>> +	for (i = 0; i < steps; i++) {
>> +		chip->write_buf(mtd, buf + (i * ecc->size), ecc->size);
>> +		offset += ecc->size;
>> +
>> +		/* Fill OOB data in */
>> +		if (oob_required) {
>> +			tmp = 0xffffffff;
>> +			memcpy_toio(nfc->regs + NFC_REG_USER_DATA_BASE, &tmp,
>> +				    4);
>> +		} else {
>> +			memcpy_toio(nfc->regs + NFC_REG_USER_DATA_BASE, oob ,
>> +				    4);
>> +		}
>> +
>> +		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | NFC_ACCESS_DIR |
>> +		      (1 << 30);
>> +		writel(tmp, nfc->regs + NFC_REG_CMD);
>> +		sunxi_nfc_wait_int(nfc, NFC_CMD_INT_FLAG, 0);
>> +
>> +		offset += ecc->bytes + ecc->prepad;
>> +		oob += ecc->bytes + ecc->prepad;
>> +	}
>> +
>> +	if (oob_required) {
>> +		cnt = mtd->oobsize - (oob - chip->oob_poi);
>> +		if (cnt > 0) {
>> +			chip->cmdfunc(mtd, NAND_CMD_RNDIN, offset, -1);
>> +			chip->write_buf(mtd, oob, cnt);
>> +		}
>> +	}
>> +
>> +	tmp = readl(nfc->regs + NFC_REG_ECC_CTL);
>> +	tmp &= ~(NFC_ECC_EN | NFC_ECC_PIPELINE);
>> +
>> +	writel(tmp, nfc->regs + NFC_REG_ECC_CTL);
>> +
>> +	return 0;
>> +}
>> +
>> +static int sunxi_nand_chip_set_timings(struct sunxi_nand_chip *chip,
>> +				       const struct nand_sdr_timings *timings)
>> +{
>> +	u32 min_clk_period = 0;
>> +
>> +	/* T1 <=> tCLS */
>> +	if (timings->tCLS_min > min_clk_period)
>> +		min_clk_period = timings->tCLS_min;
>> +
>> +	/* T2 <=> tCLH */
>> +	if (timings->tCLH_min > min_clk_period)
>> +		min_clk_period = timings->tCLH_min;
>> +
>> +	/* T3 <=> tCS */
>> +	if (timings->tCS_min > min_clk_period)
>> +		min_clk_period = timings->tCS_min;
>> +
>> +	/* T4 <=> tCH */
>> +	if (timings->tCH_min > min_clk_period)
>> +		min_clk_period = timings->tCH_min;
>> +
>> +	/* T5 <=> tWP */
>> +	if (timings->tWP_min > min_clk_period)
>> +		min_clk_period = timings->tWP_min;
>> +
>> +	/* T6 <=> tWH */
>> +	if (timings->tWH_min > min_clk_period)
>> +		min_clk_period = timings->tWH_min;
>> +
>> +	/* T7 <=> tALS */
>> +	if (timings->tALS_min > min_clk_period)
>> +		min_clk_period = timings->tALS_min;
>> +
>> +	/* T8 <=> tDS */
>> +	if (timings->tDS_min > min_clk_period)
>> +		min_clk_period = timings->tDS_min;
>> +
>> +	/* T9 <=> tDH */
>> +	if (timings->tDH_min > min_clk_period)
>> +		min_clk_period = timings->tDH_min;
>> +
>> +	/* T10 <=> tRR */
>> +	if (timings->tRR_min > (min_clk_period * 3))
>> +		min_clk_period = (timings->tRR_min + 2) / 3;
>> +
>> +	/* T11 <=> tALH */
>> +	if (timings->tALH_min > min_clk_period)
>> +		min_clk_period = timings->tALH_min;
>> +
>> +	/* T12 <=> tRP */
>> +	if (timings->tRP_min > min_clk_period)
>> +		min_clk_period = timings->tRP_min;
>> +
>> +	/* T13 <=> tREH */
>> +	if (timings->tREH_min > min_clk_period)
>> +		min_clk_period = timings->tREH_min;
>> +
>> +	/* T14 <=> tRC */
>> +	if (timings->tRC_min > (min_clk_period * 2))
>> +		min_clk_period = (timings->tRC_min + 1) / 2;
>> +
>> +	/* T15 <=> tWC */
>> +	if (timings->tWC_min > (min_clk_period * 2))
>> +		min_clk_period = (timings->tWC_min + 1) / 2;
>> +
>> +
>> +	/* min_clk_period = (NAND-clk-period * 2) */
>> +	if (min_clk_period < 1000)
>> +		min_clk_period = 1000;
>> +
>> +	min_clk_period /= 1000;
>> +	chip->clk_rate = (2 * 1000000000) / min_clk_period;
>> +
>> +	/* TODO: configure T16-T19 */
>> +
>> +	return 0;
>> +}
>> +
>> +static int sunxi_nand_chip_init_timings(struct sunxi_nand_chip *chip,
>> +					struct device_node *np)
>> +{
>> +	const struct nand_sdr_timings *timings;
>> +	int ret;
>> +	int mode;
>> +
>> +	mode = onfi_get_async_timing_mode(&chip->nand);
>> +	if (mode == ONFI_TIMING_MODE_UNKNOWN) {
>> +		mode = of_get_nand_onfi_timing_mode(np);
>> +		if (mode < 0)
>> +			mode = 1;
>> +
>> +		mode = fls(mode) - 1;
>> +		if (mode < 0)
>> +			mode = 0;
>> +	} else {
>> +		uint8_t feature[ONFI_SUBFEATURE_PARAM_LEN] = {};
>> +		mode = fls(mode) - 1;
>> +		if (mode < 0)
>> +			mode = 0;
>> +
>> +		feature[0] = mode;
>> +		ret = chip->nand.onfi_set_features(&chip->mtd, &chip->nand,
>> +						ONFI_FEATURE_ADDR_TIMING_MODE,
>> +						feature);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	timings = onfi_async_timing_mode_to_sdr_timings(mode);
>> +	if (IS_ERR(timings))
>> +		return PTR_ERR(timings);
>> +
>> +	return sunxi_nand_chip_set_timings(chip, timings);
>> +}
>> +
>> +static int sunxi_nand_hw_common_ecc_ctrl_init(struct mtd_info *mtd,
>> +					      struct nand_ecc_ctrl *ecc,
>> +					      struct device_node *np)
>> +{
>> +	struct sunxi_nand_hw_ecc *data;
>> +	struct nand_ecclayout *layout;
>> +	int nsectors;
>> +	int ret;
>> +
>> +	if (!ecc->strength || !ecc->size)
>> +		return -EINVAL;
>> +
>> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	/* Add ECC info retrieval from DT */
>> +	if (ecc->strength <= 16) {
>> +		ecc->strength = 16;
>> +		data->mode = 0;
>> +	} else if (ecc->strength <= 24) {
>> +		ecc->strength = 24;
>> +		data->mode = 1;
>> +	} else if (ecc->strength <= 28) {
>> +		ecc->strength = 28;
>> +		data->mode = 2;
>> +	} else if (ecc->strength <= 32) {
>> +		ecc->strength = 32;
>> +		data->mode = 3;
>> +	} else if (ecc->strength <= 40) {
>> +		ecc->strength = 40;
>> +		data->mode = 4;
>> +	} else if (ecc->strength <= 48) {
>> +		ecc->strength = 48;
>> +		data->mode = 5;
>> +	} else if (ecc->strength <= 56) {
>> +		ecc->strength = 56;
>> +		data->mode = 6;
>> +	} else if (ecc->strength <= 60) {
>> +		ecc->strength = 60;
>> +		data->mode = 7;
>> +	} else if (ecc->strength <= 64) {
>> +		ecc->strength = 64;
>> +		data->mode = 8;
>> +	} else {
>> +		pr_err("unsupported strength\n");
>
>
>> +		return -ENOTSUPP;
> You're leaking the 'data' allocated above in here.

Oops, I'll fix that.

>
>> +	}
>> +
>> +	/* HW ECC always request ECC bytes for 1024 bytes blocks */
>> +	ecc->bytes = ((ecc->strength * fls(8 * 1024)) + 7) / 8;
>> +
>> +	/* HW ECC always work with even numbers of ECC bytes */
>> +	if (ecc->bytes % 2)
>> +		ecc->bytes++;
>> +
>> +	layout = &data->layout;
>> +	nsectors = mtd->writesize / ecc->size;
>> +
>> +	if (mtd->oobsize < ((ecc->bytes + 4) * nsectors)) {
>> +		ret = -EINVAL;
>> +		goto err;
>> +	}
>> +
>> +	layout->eccbytes = (ecc->bytes * nsectors);
>> +
>> +	ecc->layout = layout;
>> +	ecc->priv = data;
>> +
>> +	return 0;
>> +
>> +err:
>> +	kfree(data);
>> +
>> +	return ret;
>> +}
>> +
>> +static void sunxi_nand_hw_common_ecc_ctrl_cleanup(struct nand_ecc_ctrl *ecc)
>> +{
>> +	kfree(ecc->priv);
>> +}
>> +
>> +static int sunxi_nand_hw_ecc_ctrl_init(struct mtd_info *mtd,
>> +				       struct nand_ecc_ctrl *ecc,
>> +				       struct device_node *np)
>> +{
>> +	struct nand_ecclayout *layout;
>> +	int nsectors;
>> +	int i, j;
>> +	int ret;
>> +
>> +	ret = sunxi_nand_hw_common_ecc_ctrl_init(mtd, ecc, np);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ecc->read_page = sunxi_nfc_hw_ecc_read_page;
>> +	ecc->write_page = sunxi_nfc_hw_ecc_write_page;
>> +	layout = ecc->layout;
>> +	nsectors = mtd->writesize / ecc->size;
>> +	/*
>> +	 * The first 2 bytes are used for BB markers.
>> +	 * We merge the 4 user available bytes from HW ECC with this
>> +	 * first section, hence why the + 2 operation (- 2 + 4).
>> +	 */
>> +	layout->oobfree[0].length = mtd->oobsize + 2 -
>> +				    ((ecc->bytes + 4) * nsectors);
>> +	layout->oobfree[0].offset = 2;
>> +	for (i = 0; i < nsectors; i++) {
>> +		/*
>> +		 * The first 4 ECC block bytes are already counted in the first
>> +		 * oobfree entry.
>> +		 */
>> +		if (i) {
>> +			layout->oobfree[i].offset =
>> +				layout->oobfree[i - 1].offset +
>> +				layout->oobfree[i - 1].length +
>> +				ecc->bytes;
>> +			layout->oobfree[i].length = 4;
>> +		}
>> +
>> +		for (j = 0; j < ecc->bytes; j++)
>> +			layout->eccpos[(ecc->bytes * i) + j] =
>> +					layout->oobfree[i].offset +
>> +					layout->oobfree[i].length + j;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int sunxi_nand_hw_syndrome_ecc_ctrl_init(struct mtd_info *mtd,
>> +						struct nand_ecc_ctrl *ecc,
>> +						struct device_node *np)
>> +{
>> +	struct nand_ecclayout *layout;
>> +	int nsectors;
>> +	int i;
>> +	int ret;
>> +
>> +	ret = sunxi_nand_hw_common_ecc_ctrl_init(mtd, ecc, np);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ecc->prepad = 4;
>> +	ecc->read_page = sunxi_nfc_hw_syndrome_ecc_read_page;
>> +	ecc->write_page = sunxi_nfc_hw_syndrome_ecc_write_page;
>> +
>> +	layout = ecc->layout;
>> +	nsectors = mtd->writesize / ecc->size;
>> +
>> +	for (i = 0; i < (ecc->bytes * nsectors); i++)
>> +		layout->eccpos[i] = i;
>> +
>> +	layout->oobfree[0].length = mtd->oobsize - i;
>> +	layout->oobfree[0].offset = i;
>> +
>> +	return 0;
>> +}
>> +
>> +static void sunxi_nand_ecc_cleanup(struct nand_ecc_ctrl *ecc)
>> +{
>> +	switch (ecc->mode) {
>> +	case NAND_ECC_HW:
>> +	case NAND_ECC_HW_SYNDROME:
>> +		sunxi_nand_hw_common_ecc_ctrl_cleanup(ecc);
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +}
>> +
>> +static int sunxi_nand_ecc_init(struct mtd_info *mtd, struct nand_ecc_ctrl *ecc,
>> +			       struct device_node *np)
>> +{
>> +	struct nand_chip *nand = mtd->priv;
>> +	int ecc_step_size, ecc_strength;
>> +	int ret;
>> +
>> +	ecc_step_size = of_get_nand_ecc_step_size(np);
>> +	ecc_strength = of_get_nand_ecc_strength(np);
>> +	if (ecc_step_size > 0 && ecc_strength > 0) {
>> +		ecc->size = ecc_step_size;
>> +		ecc->strength = ecc_strength;
>> +	} else {
>> +		ecc->size = nand->ecc_step_ds;
>> +		ecc->strength = nand->ecc_strength_ds;
>> +	}
>> +
> Shouldn't you check the devicetree value is not weaker than the ONFI-obtained?

I can definitely do that.

>
>> +	ecc->mode = of_get_nand_ecc_mode(np);
>> +	switch (ecc->mode) {
>> +	case NAND_ECC_SOFT_BCH:
>> +		if (!ecc->size || !ecc->strength)
>> +			return -EINVAL;
>> +		ecc->bytes = ((ecc->strength * fls(8 * ecc->size)) + 7) / 8;
>> +		break;
>> +	case NAND_ECC_HW:
>> +		ret = sunxi_nand_hw_ecc_ctrl_init(mtd, ecc, np);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	case NAND_ECC_HW_SYNDROME:
>> +		ret = sunxi_nand_hw_syndrome_ecc_ctrl_init(mtd, ecc, np);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	case NAND_ECC_NONE:
>> +	case NAND_ECC_SOFT:
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int sunxi_nand_chip_init(struct device *dev, struct sunxi_nfc *nfc,
>> +				struct device_node *np)
>> +{
>> +	const struct nand_sdr_timings *timings;
>> +	struct sunxi_nand_chip *chip;
>> +	struct mtd_part_parser_data ppdata;
>> +	struct mtd_info *mtd;
>> +	struct nand_chip *nand;
>> +	int nsels;
>> +	int ret;
>> +	int i;
>> +	u32 tmp;
>> +
>> +	if (!of_get_property(np, "reg", &nsels))
>> +		return -EINVAL;
>> +
>> +	nsels /= sizeof(u32);
>> +	if (!nsels)
>> +		return -EINVAL;
>> +
>> +	chip = devm_kzalloc(dev,
>> +			    sizeof(*chip) +
>> +			    (nsels * sizeof(struct sunxi_nand_chip_sel)),
>> +			    GFP_KERNEL);
>> +	if (!chip)
>> +		return -ENOMEM;
>> +
>> +	chip->nsels = nsels;
>> +	chip->selected = -1;
>> +
>> +	for (i = 0; i < nsels; i++) {
>> +		ret = of_property_read_u32_index(np, "reg", i, &tmp);
>> +		if (ret)
>> +			return ret;
>> +
>> +		if (tmp > 7)
>> +			return -EINVAL;
>> +
>> +		if (test_and_set_bit(tmp, &nfc->assigned_cs))
>> +			return -EINVAL;
>> +
>> +		chip->sels[i].cs = tmp;
>> +
>> +		if (!of_property_read_u32_index(np, "allwinner,rb", i, &tmp) &&
>> +		    tmp < 2) {
>> +			chip->sels[i].rb.type = RB_NATIVE;
>> +			chip->sels[i].rb.info.nativeid = tmp;
>> +		} else {
>> +			ret = of_get_named_gpio(np, "rb-gpios", i);
>> +			if (ret >= 0) {
>> +				tmp = ret;
>> +				chip->sels[i].rb.type = RB_GPIO;
>> +				chip->sels[i].rb.info.gpio = tmp;
>> +				ret = devm_gpio_request(dev, tmp, "nand-rb");
>> +				if (ret)
>> +					return ret;
>> +
>> +				ret = gpio_direction_input(tmp);
>> +				if (ret)
>> +					return ret;
>> +			} else {
>> +				chip->sels[i].rb.type = RB_NONE;
>> +			}
>> +		}
>> +	}
>> +
>> +	timings = onfi_async_timing_mode_to_sdr_timings(0);
>> +	if (IS_ERR(timings))
>> +		return PTR_ERR(timings);
>> +
>> +	ret = sunxi_nand_chip_set_timings(chip, timings);
>> +
>> +	nand = &chip->nand;
>> +	/* Default tR value specified in the ONFI spec (chapter 4.15.1) */
>> +	nand->chip_delay = 200;
>> +	nand->controller = &nfc->controller;
>> +	nand->select_chip = sunxi_nfc_select_chip;
>> +	nand->cmd_ctrl = sunxi_nfc_cmd_ctrl;
>> +	nand->read_buf = sunxi_nfc_read_buf;
>> +	nand->write_buf = sunxi_nfc_write_buf;
>> +	nand->read_byte = sunxi_nfc_read_byte;
>> +
>> +	if (of_get_nand_on_flash_bbt(np))
>> +		nand->bbt_options |= NAND_BBT_USE_FLASH;
>> +
>> +	mtd = &chip->mtd;
>> +	mtd->dev.parent = dev;
>> +	mtd->priv = nand;
>> +	mtd->owner = THIS_MODULE;
>> +
>> +	ret = nand_scan_ident(mtd, nsels, NULL);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = sunxi_nand_chip_init_timings(chip, np);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = sunxi_nand_ecc_init(mtd, &nand->ecc, np);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = nand_scan_tail(mtd);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (of_property_read_string(np, "nand-name", &mtd->name)) {
>> +		snprintf(chip->default_name, MAX_NAME_SIZE,
>> +			 DEFAULT_NAME_FORMAT, chip->sels[i].cs);
>> +		mtd->name = chip->default_name;
>> +	}
>> +
>> +	ppdata.of_node = np;
>> +	ret = mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0);
>> +	if (!ret)
> Aren't you leaking the BBT resources allocated in nand_scan_tail? I **think**
> you need to call nand_release if the mtd device register fails.

Absolutely, I'll call nand_release before returning the error code.

>
>> +		return ret;
>> +
>> +	list_add_tail(&chip->node, &nfc->chips);
>> +
>> +	return 0;
>> +}
>> +
>> +static int sunxi_nand_chips_init(struct device *dev, struct sunxi_nfc *nfc)
>> +{
>> +	struct device_node *np = dev->of_node;
>> +	struct device_node *nand_np;
>> +	int nchips = of_get_child_count(np);
>> +	int ret;
>> +
>> +	if (nchips > 8)
>> +		return -EINVAL;
>> +
>> +	for_each_child_of_node(np, nand_np) {
>> +		ret = sunxi_nand_chip_init(dev, nfc, nand_np);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void sunxi_nand_chips_cleanup(struct sunxi_nfc *nfc)
>> +{
>> +	struct sunxi_nand_chip *chip;
>> +
>> +	while (!list_empty(&nfc->chips)) {
>> +		chip = list_first_entry(&nfc->chips, struct sunxi_nand_chip,
>> +					node);
>> +		nand_release(&chip->mtd);
>> +		sunxi_nand_ecc_cleanup(&chip->nand.ecc);
>> +	}
>> +}
>> +
>> +static int sunxi_nfc_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct resource *r;
>> +	struct sunxi_nfc *nfc;
>> +	int ret;
>> +
>> +	nfc = devm_kzalloc(dev, sizeof(*nfc), GFP_KERNEL);
>> +	if (!nfc) {
>> +		dev_err(dev, "failed to allocate NFC struct\n");
> OOM message not needed.

I'll drop it.

>
>> +		return -ENOMEM;
>> +	}
>> +
>> +	spin_lock_init(&nfc->controller.lock);
>> +	init_waitqueue_head(&nfc->controller.wq);
>> +	INIT_LIST_HEAD(&nfc->chips);
>> +
>> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	nfc->regs = devm_ioremap_resource(dev, r);
>> +	if (IS_ERR(nfc->regs)) {
>> +		dev_err(dev, "failed to remap iomem\n");
> Message not needed, see devm_ioremap_resource() documentation.

ditto.

>
>> +		return PTR_ERR(nfc->regs);
>> +	}
>> +
>> +	nfc->irq = platform_get_irq(pdev, 0);
>> +	if (nfc->irq < 0) {
>> +		dev_err(dev, "failed to retrieve irq\n");
>> +		return nfc->irq;
>> +	}
>> +
>> +	nfc->ahb_clk = devm_clk_get(dev, "ahb_clk");
>> +	if (IS_ERR(nfc->ahb_clk)) {
>> +		dev_err(dev, "failed to retrieve ahb_clk\n");
>> +		return PTR_ERR(nfc->ahb_clk);
>> +	}
>> +
>> +	ret = clk_prepare_enable(nfc->ahb_clk);
>> +	if (ret)
>> +		return ret;
>> +
>> +	nfc->sclk = devm_clk_get(dev, "sclk");
>> +	if (IS_ERR(nfc->sclk)) {
>> +		dev_err(dev, "failed to retrieve nand_clk\n");
>> +		ret = PTR_ERR(nfc->sclk);
>> +		goto out_ahb_clk_unprepare;
>> +	}
>> +
>> +	ret = clk_prepare_enable(nfc->sclk);
>> +	if (ret)
>> +		goto out_ahb_clk_unprepare;
>> +
>> +	/* Reset NFC */
>> +	writel(readl(nfc->regs + NFC_REG_CTL) | NFC_RESET,
>> +	       nfc->regs + NFC_REG_CTL);
>> +	while (readl(nfc->regs + NFC_REG_CTL) & NFC_RESET)
>> +		;
> Again: maybe you should avoid potentially infinite loop, no matter what.
>
>> +
>> +	writel(0, nfc->regs + NFC_REG_INT);
>> +	ret = devm_request_irq(dev, nfc->irq, sunxi_nfc_interrupt,
>> +			       0, "sunxi-nand", nfc);
>> +	if (ret)
>> +		goto out_sclk_unprepare;
>> +
>> +	platform_set_drvdata(pdev, nfc);
>> +
>> +	writel(0x100, nfc->regs + NFC_REG_TIMING_CTL);
>> +	writel(0x7ff, nfc->regs + NFC_REG_TIMING_CFG);
> Hm... can you remove the magic numbers?

I wish I could, but unfortunately I don't enough information from the
datasheet and reverse engineering work to know what these registers are
used for and these value encodes.

I'll add a TODO comment to state that we're missing informations here
and that it should be reworked as soon as we get these informations.

>
>> +
>> +	ret = sunxi_nand_chips_init(dev, nfc);
>> +	if (ret) {
>> +		dev_err(dev, "failed to init nand chips\n");
>> +		goto out_sclk_unprepare;
>> +	}
>> +
>> +	return 0;
>> +
>> +out_sclk_unprepare:
>> +	clk_disable_unprepare(nfc->sclk);
>> +out_ahb_clk_unprepare:
>> +	clk_disable_unprepare(nfc->ahb_clk);
>> +
>> +	return ret;
>> +}
>> +
>> +static int sunxi_nfc_remove(struct platform_device *pdev)
>> +{
>> +	struct sunxi_nfc *nfc = platform_get_drvdata(pdev);
>> +
>> +	sunxi_nand_chips_cleanup(nfc);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id sunxi_nfc_ids[] = {
>> +	{ .compatible = "allwinner,sun4i-nand" },
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, sunxi_nfc_ids);
>> +
>> +static struct platform_driver sunxi_nfc_driver = {
>> +	.driver = {
>> +		.name = "sunxi_nand",
>> +		.owner = THIS_MODULE,
>> +		.of_match_table = of_match_ptr(sunxi_nfc_ids),
> Redundant of_match_ptr. See 5576bc7bef2919dd2b185bffb768bf9c0da76788.

I'll drop it.

>
> I think that's all I can spot, without knowing the hardware details.


Thanks a lot for your review.

Best Regards,

Boris

[PATCH v3 5/9] mtd: nand: add sunxi NAND flash controller support

[Ezequiel Garcia]

On 09 May 06:47 PM, Boris BREZILLON wrote:
> > Can you add some documentation about this read-back stuff?
> 
> This has nothing to do with read-back, RB means ready/busy :-).
> 

Hehe... right... I guess I was thinking in read/busy.

See? That's why you need to document it: so it's fool-proof.
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v3 5/9] mtd: nand: add sunxi NAND flash controller support

[Brian Norris]

On Fri, May 09, 2014 at 06:47:22PM +0200, Boris BREZILLON wrote:
> On 09/05/2014 18:03, Ezequiel Garcia wrote:
> > On 12 Mar 07:07 PM, Boris BREZILLON wrote:
> >> --- /dev/null
> >> +++ b/drivers/mtd/nand/sunxi_nand.c
> >> @@ -0,0 +1,1276 @@
...
> >> +static int sunxi_nand_ecc_init(struct mtd_info *mtd, struct nand_ecc_ctrl *ecc,
> >> +			       struct device_node *np)
> >> +{
> >> +	struct nand_chip *nand = mtd->priv;
> >> +	int ecc_step_size, ecc_strength;
> >> +	int ret;
> >> +
> >> +	ecc_step_size = of_get_nand_ecc_step_size(np);
> >> +	ecc_strength = of_get_nand_ecc_strength(np);
> >> +	if (ecc_step_size > 0 && ecc_strength > 0) {
> >> +		ecc->size = ecc_step_size;
> >> +		ecc->strength = ecc_strength;
> >> +	} else {
> >> +		ecc->size = nand->ecc_step_ds;
> >> +		ecc->strength = nand->ecc_strength_ds;
> >> +	}
> >> +
> > Shouldn't you check the devicetree value is not weaker than the ONFI-obtained?
> 
> I can definitely do that.

You can do that here, but take a look at the discussion Ezequiel and I
had about this:

  http://thread.gmane.org/gmane.linux.drivers.devicetree/67462

We probably don't want to be doing anything drastic like overriding the
device tree configuration. Instead, we might want to stick a warning
into the core nand_base code that does the proper comparison of the
'*_ds' values with the actual values chosen in
chip->ecc->{size,strength}. The comparison is kind of subtle, actually,
so it'd be good to do it exactly once for everyone.

Brian

[PATCH v3 5/9] mtd: nand: add sunxi NAND flash controller support

[Brian Norris]

On Tue, May 20, 2014 at 11:49:42AM -0700, Brian Norris wrote:
> On Fri, May 09, 2014 at 06:47:22PM +0200, Boris BREZILLON wrote:
> > On 09/05/2014 18:03, Ezequiel Garcia wrote:
> > > On 12 Mar 07:07 PM, Boris BREZILLON wrote:
> > >> --- /dev/null
> > >> +++ b/drivers/mtd/nand/sunxi_nand.c
> > >> @@ -0,0 +1,1276 @@
> ...
> > >> +static int sunxi_nand_ecc_init(struct mtd_info *mtd, struct nand_ecc_ctrl *ecc,
> > >> +			       struct device_node *np)
> > >> +{
> > >> +	struct nand_chip *nand = mtd->priv;
> > >> +	int ecc_step_size, ecc_strength;
> > >> +	int ret;
> > >> +
> > >> +	ecc_step_size = of_get_nand_ecc_step_size(np);
> > >> +	ecc_strength = of_get_nand_ecc_strength(np);
> > >> +	if (ecc_step_size > 0 && ecc_strength > 0) {
> > >> +		ecc->size = ecc_step_size;
> > >> +		ecc->strength = ecc_strength;
> > >> +	} else {
> > >> +		ecc->size = nand->ecc_step_ds;
> > >> +		ecc->strength = nand->ecc_strength_ds;
> > >> +	}
> > >> +
> > > Shouldn't you check the devicetree value is not weaker than the ONFI-obtained?
> > 
> > I can definitely do that.
> 
> You can do that here, but take a look at the discussion Ezequiel and I
> had about this:
> 
>   http://thread.gmane.org/gmane.linux.drivers.devicetree/67462
> 
> We probably don't want to be doing anything drastic like overriding the
> device tree configuration. Instead, we might want to stick a warning
> into the core nand_base code that does the proper comparison of the
> '*_ds' values with the actual values chosen in
> chip->ecc->{size,strength}. The comparison is kind of subtle, actually,
> so it'd be good to do it exactly once for everyone.

I forgot, Ezequiel already submitted this. I'll look at it soon:

  http://patchwork.ozlabs.org/patch/348901/

Brian

[PATCH v3 5/9] mtd: nand: add sunxi NAND flash controller support

[Boris BREZILLON]

On 20/05/2014 21:21, Brian Norris wrote:
> On Tue, May 20, 2014 at 11:49:42AM -0700, Brian Norris wrote:
>> On Fri, May 09, 2014 at 06:47:22PM +0200, Boris BREZILLON wrote:
>>> On 09/05/2014 18:03, Ezequiel Garcia wrote:
>>>> On 12 Mar 07:07 PM, Boris BREZILLON wrote:
>>>>> --- /dev/null
>>>>> +++ b/drivers/mtd/nand/sunxi_nand.c
>>>>> @@ -0,0 +1,1276 @@
>> ...
>>>>> +static int sunxi_nand_ecc_init(struct mtd_info *mtd, struct nand_ecc_ctrl *ecc,
>>>>> +			       struct device_node *np)
>>>>> +{
>>>>> +	struct nand_chip *nand = mtd->priv;
>>>>> +	int ecc_step_size, ecc_strength;
>>>>> +	int ret;
>>>>> +
>>>>> +	ecc_step_size = of_get_nand_ecc_step_size(np);
>>>>> +	ecc_strength = of_get_nand_ecc_strength(np);
>>>>> +	if (ecc_step_size > 0 && ecc_strength > 0) {
>>>>> +		ecc->size = ecc_step_size;
>>>>> +		ecc->strength = ecc_strength;
>>>>> +	} else {
>>>>> +		ecc->size = nand->ecc_step_ds;
>>>>> +		ecc->strength = nand->ecc_strength_ds;
>>>>> +	}
>>>>> +
>>>> Shouldn't you check the devicetree value is not weaker than the ONFI-obtained?
>>> I can definitely do that.
>> You can do that here, but take a look at the discussion Ezequiel and I
>> had about this:
>>
>>   http://thread.gmane.org/gmane.linux.drivers.devicetree/67462
>>
>> We probably don't want to be doing anything drastic like overriding the
>> device tree configuration. Instead, we might want to stick a warning
>> into the core nand_base code that does the proper comparison of the
>> '*_ds' values with the actual values chosen in
>> chip->ecc->{size,strength}. The comparison is kind of subtle, actually,
>> so it'd be good to do it exactly once for everyone.

Fair enough, I'll follow your suggestions on this specific point ;)

> I forgot, Ezequiel already submitted this. I'll look at it soon:
>
>   http://patchwork.ozlabs.org/patch/348901/

Thanks for pointing this out.


>
> Brian

[PATCH v3 6/9] mtd: nand: add sunxi NFC dt bindings doc

[Boris BREZILLON]

Add the sunxi NAND Flash Controller dt bindings documentation.

Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
---
 .../devicetree/bindings/mtd/sunxi-nand.txt         |   48 ++++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/sunxi-nand.txt

diff --git a/Documentation/devicetree/bindings/mtd/sunxi-nand.txt b/Documentation/devicetree/bindings/mtd/sunxi-nand.txt
new file mode 100644
index 0000000..4bf5b07
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/sunxi-nand.txt
@@ -0,0 +1,48 @@
+Allwinner NAND Flash Controller (NFC)
+
+Required properties:
+- compatible : "allwinner,sun4i-nand".
+- reg : shall contain registers location and length for data and reg.
+- interrupts : shall define the nand controller interrupt.
+- #address-cells: shall be set to 1. Encode the nand CS.
+- #size-cells : shall be set to 0.
+- clocks : shall reference nand controller clocks.
+- clock-names : nand controller internal clock names. Shall contain :
+    * "ahb_clk" : AHB gating clock
+    * "sclk" : nand controller clock
+
+Optional children nodes:
+Children nodes represent the available nand chips.
+
+Optional properties:
+- onfi,nand-timing-mode : mandatory if the chip does not support the ONFI
+  standard.
+- allwinner,rb : shall contain the native Ready/Busy ids.
+ or
+- rb-gpios : shall contain the gpios used as R/B pins.
+- nand-ecc-mode : one of the supported ECC modes ("hw", "hw_syndrome", "soft",
+  "soft_bch" or "none")
+
+see Documentation/devicetree/mtd/nand.txt for generic bindings.
+
+
+Examples:
+nfc: nand at 01c03000 {
+	compatible = "allwinner,sun4i-nand";
+	reg = <0x01c03000 0x1000>;
+	interrupts = <0 37 1>;
+	clocks = <&ahb_gates 13>, <&nand_clk>;
+	clock-names = "ahb_clk", "sclk";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&nand_pins_a &nand_cs0_pins_a &nand_rb0_pins_a>;
+	status = "okay";
+
+	nand at 0 {
+		reg = <0>;
+		allwinner,rb = <0>;
+		nand-ecc-mode = "soft_bch";
+		onfi,nand-timing-mode = <4>;
+	};
+};
-- 
1.7.9.5

[PATCH v3 7/9] ARM: dt/sunxi: add NFC node to Allwinner A20 SoC

[Boris BREZILLON]

Add NAND Flash controller node definition to the A20 SoC.

Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
---
 arch/arm/boot/dts/sun7i-a20.dtsi |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index 9ff0948..4c14ed8 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -339,6 +339,17 @@
 		#size-cells = <1>;
 		ranges;
 
+		nfc: nand at 01c03000 {
+			compatible = "allwinner,sun4i-nand";
+			reg = <0x01c03000 0x1000>;
+			interrupts = <0 37 4>;
+			clocks = <&ahb_gates 13>, <&nand_clk>;
+			clock-names = "ahb_clk", "sclk";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+		};
+
 		emac: ethernet at 01c0b000 {
 			compatible = "allwinner,sun4i-a10-emac";
 			reg = <0x01c0b000 0x1000>;
-- 
1.7.9.5

[PATCH v3 8/9] ARM: dt/sunxi: add A20 NAND controller pin definitions

[Boris BREZILLON]

Define the NAND controller pin configs.

Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
---
 arch/arm/boot/dts/sun7i-a20.dtsi |   80 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index 4c14ed8..c8095c5 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -443,6 +443,86 @@
 				allwinner,drive = <0>;
 				allwinner,pull = <0>;
 			};
+
+			nand_pins_a: nand_base0 at 0 {
+				allwinner,pins = "PC0", "PC1", "PC2",
+						"PC5", "PC8", "PC9", "PC10",
+						"PC11", "PC12", "PC13", "PC14",
+						"PC15", "PC16";
+				allwinner,function = "nand0";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
+
+			nand_cs0_pins_a: nand_cs at 0 {
+				allwinner,pins = "PC4";
+				allwinner,function = "nand0";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
+
+			nand_cs1_pins_a: nand_cs at 1 {
+				allwinner,pins = "PC3";
+				allwinner,function = "nand0";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
+
+			nand_cs2_pins_a: nand_cs at 2 {
+				allwinner,pins = "PC17";
+				allwinner,function = "nand0";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
+
+			nand_cs3_pins_a: nand_cs at 3 {
+				allwinner,pins = "PC18";
+				allwinner,function = "nand0";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
+
+			nand_cs4_pins_a: nand_cs at 4 {
+				allwinner,pins = "PC19";
+				allwinner,function = "nand0";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
+
+			nand_cs5_pins_a: nand_cs at 5 {
+				allwinner,pins = "PC20";
+				allwinner,function = "nand0";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
+
+			nand_cs6_pins_a: nand_cs at 6 {
+				allwinner,pins = "PC21";
+				allwinner,function = "nand0";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
+
+			nand_cs7_pins_a: nand_cs at 7 {
+				allwinner,pins = "PC22";
+				allwinner,function = "nand0";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
+
+			nand_rb0_pins_a: nand_rb at 0 {
+				allwinner,pins = "PC6";
+				allwinner,function = "nand0";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
+
+			nand_rb1_pins_a: nand_rb at 1 {
+				allwinner,pins = "PC7";
+				allwinner,function = "nand0";
+				allwinner,drive = <0>;
+				allwinner,pull = <0>;
+			};
 		};
 
 		timer at 01c20c00 {
-- 
1.7.9.5

[PATCH v3 9/9] ARM: sunxi/dt: enable NAND on cubietruck board

[Boris BREZILLON]

Enable the NFC and describe the NAND flash connected to this controller.

Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
---
 arch/arm/boot/dts/sun7i-a20-cubietruck.dts |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
index f9dcb61..7b48539 100644
--- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
+++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
@@ -19,6 +19,23 @@
 	compatible = "cubietech,cubietruck", "allwinner,sun7i-a20";
 
 	soc at 01c00000 {
+		nfc: nand at 01c03000 {
+			pinctrl-names = "default";
+			pinctrl-0 = <&nand_pins_a &nand_cs0_pins_a &nand_rb0_pins_a>;
+			status = "okay";
+
+			nand at 0 {
+				#address-cells = <2>;
+				#size-cells = <2>;
+				reg = <0>;
+				allwinner,rb = <0>;
+				nand-ecc-mode = "hw";
+				onfi,nand-timing-mode = <0x1f>;
+				nand-ecc-strength = <40>;
+				nand-ecc-step-size = <1024>;
+			};
+		};
+
 		pinctrl at 01c20800 {
 			led_pins_cubietruck: led_pins at 0 {
 				allwinner,pins = "PH7", "PH11", "PH20", "PH21";
-- 
1.7.9.5