[U-Boot] [PATCH 0/6] tegra: Add NAND flash support

[U-Boot] [PATCH 0/6] tegra: Add NAND flash support

[Simon Glass]

This series adds NAND flash support to Tegra and enables it on Seaboard.

Included here is a proposed device tree binding which I'm sure will
stretch some eyes. The binding includes information about the NAND
controller as well as the connected NAND device. The Seaboard has a
Hynix HY27UF4G2B.

The driver supports ECC-based access and uses DMA and NAND acceleration
features of the Tegra SOC to provide access at reasonable speed.


Jim Lin (1):
  tegra: nand: Add Tegra NAND driver

Simon Glass (5):
  fdt: Add debugging to fdtdec_get_int/addr()
  tegra: Add NAND support to funcmux
  tegra: fdt: Add NAND controller binding and definitions
  tegra: fdt: Add NAND definitions to fdt
  tegra: Enable NAND on Seaboard

 arch/arm/cpu/armv7/tegra2/funcmux.c           |    7 +
 arch/arm/dts/tegra20.dtsi                     |    7 +-
 arch/arm/include/asm/arch-tegra2/funcmux.h    |    3 +
 arch/arm/include/asm/arch-tegra2/tegra2.h     |    1 +
 board/nvidia/dts/tegra2-seaboard.dts          |   15 +
 doc/device-tree-bindings/nand/nvidia-nand.txt |   68 ++
 drivers/mtd/nand/Makefile                     |    1 +
 drivers/mtd/nand/tegra2_nand.c                | 1074 +++++++++++++++++++++++++
 drivers/mtd/nand/tegra2_nand.h                |  303 +++++++
 include/configs/seaboard.h                    |    9 +
 include/fdtdec.h                              |    1 +
 lib/fdtdec.c                                  |   23 +-
 12 files changed, 1505 insertions(+), 7 deletions(-)
 create mode 100644 doc/device-tree-bindings/nand/nvidia-nand.txt
 create mode 100644 drivers/mtd/nand/tegra2_nand.c
 create mode 100644 drivers/mtd/nand/tegra2_nand.h

-- 
1.7.7.3

[U-Boot] [PATCH 2/6] tegra: Add NAND support to funcmux

[Simon Glass]

Add selection of NAND flash pins to the funcmux.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 arch/arm/cpu/armv7/tegra2/funcmux.c        |    7 +++++++
 arch/arm/include/asm/arch-tegra2/funcmux.h |    3 +++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/arch/arm/cpu/armv7/tegra2/funcmux.c b/arch/arm/cpu/armv7/tegra2/funcmux.c
index 73950d0..f84ec74 100644
--- a/arch/arm/cpu/armv7/tegra2/funcmux.c
+++ b/arch/arm/cpu/armv7/tegra2/funcmux.c
@@ -156,6 +156,13 @@ int funcmux_select(enum periph_id id, int config)
 		}
 		break;
 
+	case PERIPH_ID_NDFLASH:
+		if (config == FUNCMUX_NDFLASH_ATC) {
+			pinmux_set_func(PINGRP_ATC, PMUX_FUNC_NAND);
+			pinmux_tristate_disable(PINGRP_ATC);
+		}
+		break;
+
 	default:
 		debug("%s: invalid periph_id %d", __func__, id);
 		return -1;
diff --git a/arch/arm/include/asm/arch-tegra2/funcmux.h b/arch/arm/include/asm/arch-tegra2/funcmux.h
index ae73c72..9419522 100644
--- a/arch/arm/include/asm/arch-tegra2/funcmux.h
+++ b/arch/arm/include/asm/arch-tegra2/funcmux.h
@@ -47,6 +47,9 @@ enum {
 	FUNCMUX_SDMMC4_ATC_ATD_8BIT = 0,
 	FUNCMUX_SDMMC4_ATB_GMA_4_BIT,
 	FUNCMUX_SDMMC4_ATB_GMA_GME_8_BIT,
+
+	/* NAND flags */
+	FUNCMUX_NDFLASH_ATC = 0,
 };
 
 /**
-- 
1.7.7.3

[U-Boot] [PATCH 2/6] tegra: Add NAND support to funcmux

[Stephen Warren]

On 01/13/2012 04:10 PM, Simon Glass wrote:
> Add selection of NAND flash pins to the funcmux.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Acked-by: Stephen Warren <swarren@nvidia.com>

-- 
nvpublic

[PATCH 3/6] tegra: fdt: Add NAND controller binding and definitions

Add a NAND controller along with a bindings file for review.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 arch/arm/dts/tegra20.dtsi                     |    7 ++-
 doc/device-tree-bindings/nand/nvidia-nand.txt |   68 +++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 1 deletions(-)
 create mode 100644 doc/device-tree-bindings/nand/nvidia-nand.txt

diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi
index c009f16..33d6972 100644
--- a/arch/arm/dts/tegra20.dtsi
+++ b/arch/arm/dts/tegra20.dtsi
@@ -210,5 +210,10 @@
 		reg = <0x7000f400 0x200>;
 	};
 
+	nand: nand-controller@0x70008000 {
+		#address-cells = <0>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra20-nand";
+		reg = <0x70008000 0x100>;
+	};
 };
-
diff --git a/doc/device-tree-bindings/nand/nvidia-nand.txt b/doc/device-tree-bindings/nand/nvidia-nand.txt
new file mode 100644
index 0000000..3674cf3
--- /dev/null
+++ b/doc/device-tree-bindings/nand/nvidia-nand.txt
@@ -0,0 +1,68 @@
+NAND Flash
+----------
+
+(there isn't yet a generic binding in Linux, so this describes what is in
+U-Boot)
+
+The device node for a NAND flash device is as described in the document
+"Open Firmware Recommended Practice : Universal Serial Bus" with the
+following modifications and additions :
+
+Required properties :
+ - compatible : Should be "manufacture,device", "nand-flash"
+ - page-data-bytes : Number of bytes in the data area
+ - page-spare-bytes : * Number of bytes in spare area
+       spare area = skipped-spare-bytes + data-ecc-bytes + tag-bytes
+			+ tag-ecc-bytes
+ - skipped-spare-bytes : Number of bytes to skip at start of spare area
+	(these are typically used for bad block maintenance)
+ - data-ecc-bytes : Number of ECC bytes for data area
+ - tag-bytes :Number of tag bytes in spare area
+ - tag-ecc-bytes : Number ECC bytes to be generated for tag bytes
+
+(replace -bytes with -size or -length?)
+
+This node should sit inside its controller.
+
+
+Nvidia NAND Controller
+----------------------
+
+The device node for a NAND flash controller is as described in the document
+"Open Firmware Recommended Practice : Universal Serial Bus" with the
+following modifications and additions :
+
+Optional properties:
+
+wp-gpio : GPIO of write-protect line, three cells in the format:
+		phandle, parameter, flags
+width : bus width of the NAND device in bits
+
+For now here is something specific to the Nvidia controller, with naming
+based on Nvidia's original (non-fdt) NAND driver:
+
+ - nvidia,nand-timing : Timing parameters for the NAND. Each is in ns.
+	Order is: MAX_TRP_TREA, TWB, Max(tCS, tCH, tALS, tALH),
+	TWHR, Max(tCS, tCH, tALS, tALH), TWH, TWP, TRH, TADL
+
+	MAX_TRP_TREA is:
+		non-EDO mode: Max(tRP, tREA) + 6ns
+		EDO mode: tRP timing
+
+Example:
+
+nand-controller@0x70008000 {
+	compatible = "nvidia,tegra20-nand";
+	wp-gpios = <&gpio 59 0>;		/* PH3 */
+	width = <8>;
+	nvidia,timing = <26 100 20 80 20 10 12 10 70>;
+	nand@0 {
+		compatible = "hynix,hy27uf4g2b", "nand-flash";
+		page-data-bytes = <2048>;
+		tag-ecc-bytes = <4>;
+		tag-bytes = <20>;
+		data-ecc-bytes = <36>;
+		skipped-spare-bytes = <4>;
+		page-spare-bytes = <64>;
+	};
+};
-- 
1.7.7.3

[U-Boot] [PATCH 3/6] tegra: fdt: Add NAND controller binding and definitions

[Simon Glass]

Add a NAND controller along with a bindings file for review.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 arch/arm/dts/tegra20.dtsi                     |    7 ++-
 doc/device-tree-bindings/nand/nvidia-nand.txt |   68 +++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 1 deletions(-)
 create mode 100644 doc/device-tree-bindings/nand/nvidia-nand.txt

diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi
index c009f16..33d6972 100644
--- a/arch/arm/dts/tegra20.dtsi
+++ b/arch/arm/dts/tegra20.dtsi
@@ -210,5 +210,10 @@
 		reg = <0x7000f400 0x200>;
 	};
 
+	nand: nand-controller at 0x70008000 {
+		#address-cells = <0>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra20-nand";
+		reg = <0x70008000 0x100>;
+	};
 };
-
diff --git a/doc/device-tree-bindings/nand/nvidia-nand.txt b/doc/device-tree-bindings/nand/nvidia-nand.txt
new file mode 100644
index 0000000..3674cf3
--- /dev/null
+++ b/doc/device-tree-bindings/nand/nvidia-nand.txt
@@ -0,0 +1,68 @@
+NAND Flash
+----------
+
+(there isn't yet a generic binding in Linux, so this describes what is in
+U-Boot)
+
+The device node for a NAND flash device is as described in the document
+"Open Firmware Recommended Practice : Universal Serial Bus" with the
+following modifications and additions :
+
+Required properties :
+ - compatible : Should be "manufacture,device", "nand-flash"
+ - page-data-bytes : Number of bytes in the data area
+ - page-spare-bytes : * Number of bytes in spare area
+       spare area = skipped-spare-bytes + data-ecc-bytes + tag-bytes
+			+ tag-ecc-bytes
+ - skipped-spare-bytes : Number of bytes to skip at start of spare area
+	(these are typically used for bad block maintenance)
+ - data-ecc-bytes : Number of ECC bytes for data area
+ - tag-bytes :Number of tag bytes in spare area
+ - tag-ecc-bytes : Number ECC bytes to be generated for tag bytes
+
+(replace -bytes with -size or -length?)
+
+This node should sit inside its controller.
+
+
+Nvidia NAND Controller
+----------------------
+
+The device node for a NAND flash controller is as described in the document
+"Open Firmware Recommended Practice : Universal Serial Bus" with the
+following modifications and additions :
+
+Optional properties:
+
+wp-gpio : GPIO of write-protect line, three cells in the format:
+		phandle, parameter, flags
+width : bus width of the NAND device in bits
+
+For now here is something specific to the Nvidia controller, with naming
+based on Nvidia's original (non-fdt) NAND driver:
+
+ - nvidia,nand-timing : Timing parameters for the NAND. Each is in ns.
+	Order is: MAX_TRP_TREA, TWB, Max(tCS, tCH, tALS, tALH),
+	TWHR, Max(tCS, tCH, tALS, tALH), TWH, TWP, TRH, TADL
+
+	MAX_TRP_TREA is:
+		non-EDO mode: Max(tRP, tREA) + 6ns
+		EDO mode: tRP timing
+
+Example:
+
+nand-controller at 0x70008000 {
+	compatible = "nvidia,tegra20-nand";
+	wp-gpios = <&gpio 59 0>;		/* PH3 */
+	width = <8>;
+	nvidia,timing = <26 100 20 80 20 10 12 10 70>;
+	nand at 0 {
+		compatible = "hynix,hy27uf4g2b", "nand-flash";
+		page-data-bytes = <2048>;
+		tag-ecc-bytes = <4>;
+		tag-bytes = <20>;
+		data-ecc-bytes = <36>;
+		skipped-spare-bytes = <4>;
+		page-spare-bytes = <64>;
+	};
+};
-- 
1.7.7.3

Re: [PATCH 3/6] tegra: fdt: Add NAND controller binding and definitions

[Stephen Warren]

On 01/13/2012 04:10 PM, Simon Glass wrote:
> Add a NAND controller along with a bindings file for review.

A few questions to start with:

> diff --git a/doc/device-tree-bindings/nand/nvidia-nand.txt b/doc/device-tree-bindings/nand/nvidia-nand.txt

> +NAND Flash
> +----------
> +
> +(there isn't yet a generic binding in Linux, so this describes what is in
> +U-Boot)
> +
> +The device node for a NAND flash device is as described in the document
> +"Open Firmware Recommended Practice : Universal Serial Bus" with the
> +following modifications and additions :
> +
> +Required properties :
> + - compatible : Should be "manufacture,device", "nand-flash"
> + - page-data-bytes : Number of bytes in the data area
> + - page-spare-bytes : * Number of bytes in spare area
> +       spare area = skipped-spare-bytes + data-ecc-bytes + tag-bytes
> +			+ tag-ecc-bytes
> + - skipped-spare-bytes : Number of bytes to skip at start of spare area
> +	(these are typically used for bad block maintenance)
> + - data-ecc-bytes : Number of ECC bytes for data area
> + - tag-bytes :Number of tag bytes in spare area
> + - tag-ecc-bytes : Number ECC bytes to be generated for tag bytes

Are any of those values really needed?

I looked through all the NAND references I could find in the Linux
kernel in arch/*/boot/dts/* and none of them seem to have this kind of
information.

Looking at the drivers, they execute some form of identification command
on the NAND device which gives back a device ID, which is then looked up
in a table of known devices to give the information above.

I checked the Tegra NAND driver that's in the kernel chromeos-3.0
branch, and it does the same thing, albeit it open-codes some of the
identification routines rather than just calling into the common code.

Judging by arch/*/boot/dts/*, it is standard practice to have a node for
the NAND device itself though; it's used to house (optional) partition
definitions. In the kernel,
Documentation/devicetree/bindings/mtd/mtd-physmap.txt discusses the
format of these partition nodes, and e.g.
arch/powerpc/boot/dts/canyonlands.dts (amongst many) uses this on NAND.

> +Nvidia NAND Controller
> +----------------------
> +
> +The device node for a NAND flash controller is as described in the document
> +"Open Firmware Recommended Practice : Universal Serial Bus" with the
> +following modifications and additions :
> +
> +Optional properties:
> +
> +wp-gpio : GPIO of write-protect line, three cells in the format:
> +		phandle, parameter, flags
> +width : bus width of the NAND device in bits
> +
> +For now here is something specific to the Nvidia controller, with naming
> +based on Nvidia's original (non-fdt) NAND driver:
> +
> + - nvidia,nand-timing : Timing parameters for the NAND. Each is in ns.
> +	Order is: MAX_TRP_TREA, TWB, Max(tCS, tCH, tALS, tALH),
> +	TWHR, Max(tCS, tCH, tALS, tALH), TWH, TWP, TRH, TADL
> +
> +	MAX_TRP_TREA is:
> +		non-EDO mode: Max(tRP, tREA) + 6ns
> +		EDO mode: tRP timing

At first glance, it seems reasonable to have this in the NAND node; it's
certainly impossible to probe the timing parameters. Since NAND is so
standardized though, I wonder if there is a standard set of timing
parameters so that we could have a standard device tree binding for this?

> +Example:
> +
> +nand-controller@0x70008000 {
> +	compatible = "nvidia,tegra20-nand";
> +	wp-gpios = <&gpio 59 0>;		/* PH3 */
> +	width = <8>;
> +	nvidia,timing = <26 100 20 80 20 10 12 10 70>;
> +	nand@0 {
> +		compatible = "hynix,hy27uf4g2b", "nand-flash";
> +		page-data-bytes = <2048>;
> +		tag-ecc-bytes = <4>;
> +		tag-bytes = <20>;
> +		data-ecc-bytes = <36>;
> +		skipped-spare-bytes = <4>;
> +		page-spare-bytes = <64>;
> +	};
> +};

-- 
nvpublic

[U-Boot] [PATCH 3/6] tegra: fdt: Add NAND controller binding and definitions

[Stephen Warren]

On 01/13/2012 04:10 PM, Simon Glass wrote:
> Add a NAND controller along with a bindings file for review.

A few questions to start with:

> diff --git a/doc/device-tree-bindings/nand/nvidia-nand.txt b/doc/device-tree-bindings/nand/nvidia-nand.txt

> +NAND Flash
> +----------
> +
> +(there isn't yet a generic binding in Linux, so this describes what is in
> +U-Boot)
> +
> +The device node for a NAND flash device is as described in the document
> +"Open Firmware Recommended Practice : Universal Serial Bus" with the
> +following modifications and additions :
> +
> +Required properties :
> + - compatible : Should be "manufacture,device", "nand-flash"
> + - page-data-bytes : Number of bytes in the data area
> + - page-spare-bytes : * Number of bytes in spare area
> +       spare area = skipped-spare-bytes + data-ecc-bytes + tag-bytes
> +			+ tag-ecc-bytes
> + - skipped-spare-bytes : Number of bytes to skip at start of spare area
> +	(these are typically used for bad block maintenance)
> + - data-ecc-bytes : Number of ECC bytes for data area
> + - tag-bytes :Number of tag bytes in spare area
> + - tag-ecc-bytes : Number ECC bytes to be generated for tag bytes

Are any of those values really needed?

I looked through all the NAND references I could find in the Linux
kernel in arch/*/boot/dts/* and none of them seem to have this kind of
information.

Looking at the drivers, they execute some form of identification command
on the NAND device which gives back a device ID, which is then looked up
in a table of known devices to give the information above.

I checked the Tegra NAND driver that's in the kernel chromeos-3.0
branch, and it does the same thing, albeit it open-codes some of the
identification routines rather than just calling into the common code.

Judging by arch/*/boot/dts/*, it is standard practice to have a node for
the NAND device itself though; it's used to house (optional) partition
definitions. In the kernel,
Documentation/devicetree/bindings/mtd/mtd-physmap.txt discusses the
format of these partition nodes, and e.g.
arch/powerpc/boot/dts/canyonlands.dts (amongst many) uses this on NAND.

> +Nvidia NAND Controller
> +----------------------
> +
> +The device node for a NAND flash controller is as described in the document
> +"Open Firmware Recommended Practice : Universal Serial Bus" with the
> +following modifications and additions :
> +
> +Optional properties:
> +
> +wp-gpio : GPIO of write-protect line, three cells in the format:
> +		phandle, parameter, flags
> +width : bus width of the NAND device in bits
> +
> +For now here is something specific to the Nvidia controller, with naming
> +based on Nvidia's original (non-fdt) NAND driver:
> +
> + - nvidia,nand-timing : Timing parameters for the NAND. Each is in ns.
> +	Order is: MAX_TRP_TREA, TWB, Max(tCS, tCH, tALS, tALH),
> +	TWHR, Max(tCS, tCH, tALS, tALH), TWH, TWP, TRH, TADL
> +
> +	MAX_TRP_TREA is:
> +		non-EDO mode: Max(tRP, tREA) + 6ns
> +		EDO mode: tRP timing

At first glance, it seems reasonable to have this in the NAND node; it's
certainly impossible to probe the timing parameters. Since NAND is so
standardized though, I wonder if there is a standard set of timing
parameters so that we could have a standard device tree binding for this?

> +Example:
> +
> +nand-controller at 0x70008000 {
> +	compatible = "nvidia,tegra20-nand";
> +	wp-gpios = <&gpio 59 0>;		/* PH3 */
> +	width = <8>;
> +	nvidia,timing = <26 100 20 80 20 10 12 10 70>;
> +	nand at 0 {
> +		compatible = "hynix,hy27uf4g2b", "nand-flash";
> +		page-data-bytes = <2048>;
> +		tag-ecc-bytes = <4>;
> +		tag-bytes = <20>;
> +		data-ecc-bytes = <36>;
> +		skipped-spare-bytes = <4>;
> +		page-spare-bytes = <64>;
> +	};
> +};

-- 
nvpublic

Re: [PATCH 3/6] tegra: fdt: Add NAND controller binding and definitions

[Simon Glass]

Hi Stephen,

On Thu, Jan 19, 2012 at 5:03 PM, Stephen Warren <swarren@nvidia.com> wrote:
> On 01/13/2012 04:10 PM, Simon Glass wrote:
>> Add a NAND controller along with a bindings file for review.
>
> A few questions to start with:
>
>> diff --git a/doc/device-tree-bindings/nand/nvidia-nand.txt b/doc/device-tree-bindings/nand/nvidia-nand.txt
>
>> +NAND Flash
>> +----------
>> +
>> +(there isn't yet a generic binding in Linux, so this describes what is in
>> +U-Boot)
>> +
>> +The device node for a NAND flash device is as described in the document
>> +"Open Firmware Recommended Practice : Universal Serial Bus" with the
>> +following modifications and additions :
>> +
>> +Required properties :
>> + - compatible : Should be "manufacture,device", "nand-flash"
>> + - page-data-bytes : Number of bytes in the data area
>> + - page-spare-bytes : * Number of bytes in spare area
>> +       spare area = skipped-spare-bytes + data-ecc-bytes + tag-bytes
>> +                     + tag-ecc-bytes
>> + - skipped-spare-bytes : Number of bytes to skip at start of spare area
>> +     (these are typically used for bad block maintenance)
>> + - data-ecc-bytes : Number of ECC bytes for data area
>> + - tag-bytes :Number of tag bytes in spare area
>> + - tag-ecc-bytes : Number ECC bytes to be generated for tag bytes
>
> Are any of those values really needed?
>
> I looked through all the NAND references I could find in the Linux
> kernel in arch/*/boot/dts/* and none of them seem to have this kind of
> information.
>
> Looking at the drivers, they execute some form of identification command
> on the NAND device which gives back a device ID, which is then looked up
> in a table of known devices to give the information above.
>
> I checked the Tegra NAND driver that's in the kernel chromeos-3.0
> branch, and it does the same thing, albeit it open-codes some of the
> identification routines rather than just calling into the common code.

Well that's pretty grim I think. That code should try to use the ONFi
way if it is available and supported, or provide a mechanism to
configure it (via device tree) if not / preferred.

U-Boot does have the same ID lookup feature for the mtd layer, but it
only has page size, block size and bus width.

I do wonder why this driver cannot take more of the information it
needs from the upper levels. Admittedly not all of the info is there,
but it could perhaps be inferred.

>
> Judging by arch/*/boot/dts/*, it is standard practice to have a node for
> the NAND device itself though; it's used to house (optional) partition
> definitions. In the kernel,
> Documentation/devicetree/bindings/mtd/mtd-physmap.txt discusses the
> format of these partition nodes, and e.g.
> arch/powerpc/boot/dts/canyonlands.dts (amongst many) uses this on NAND.

Yes.

>
>> +Nvidia NAND Controller
>> +----------------------
>> +
>> +The device node for a NAND flash controller is as described in the document
>> +"Open Firmware Recommended Practice : Universal Serial Bus" with the
>> +following modifications and additions :
>> +
>> +Optional properties:
>> +
>> +wp-gpio : GPIO of write-protect line, three cells in the format:
>> +             phandle, parameter, flags
>> +width : bus width of the NAND device in bits
>> +
>> +For now here is something specific to the Nvidia controller, with naming
>> +based on Nvidia's original (non-fdt) NAND driver:
>> +
>> + - nvidia,nand-timing : Timing parameters for the NAND. Each is in ns.
>> +     Order is: MAX_TRP_TREA, TWB, Max(tCS, tCH, tALS, tALH),
>> +     TWHR, Max(tCS, tCH, tALS, tALH), TWH, TWP, TRH, TADL
>> +
>> +     MAX_TRP_TREA is:
>> +             non-EDO mode: Max(tRP, tREA) + 6ns
>> +             EDO mode: tRP timing
>
> At first glance, it seems reasonable to have this in the NAND node; it's
> certainly impossible to probe the timing parameters. Since NAND is so
> standardized though, I wonder if there is a standard set of timing
> parameters so that we could have a standard device tree binding for this?

The closest datasheet I could find was this:

http://www.hynix.com/datasheet/pdf/flash/HY27(U_S)F(08_16)1G2M%20Series(Rev1.1).pdf

It has a table on p21 with 32 timing parameters.

But what we want here is the timings for the Tegra NAND controller.
Every controller will have a different take on these parameters - some
will be ignored, some added together, etc. So I think it is better to
think in terms of what the controller needs rather than getting
sidetracked on some average of all the NAND datasheets.

>
>> +Example:
>> +
>> +nand-controller@0x70008000 {
>> +     compatible = "nvidia,tegra20-nand";
>> +     wp-gpios = <&gpio 59 0>;                /* PH3 */
>> +     width = <8>;
>> +     nvidia,timing = <26 100 20 80 20 10 12 10 70>;
>> +     nand@0 {
>> +             compatible = "hynix,hy27uf4g2b", "nand-flash";
>> +             page-data-bytes = <2048>;
>> +             tag-ecc-bytes = <4>;
>> +             tag-bytes = <20>;
>> +             data-ecc-bytes = <36>;
>> +             skipped-spare-bytes = <4>;
>> +             page-spare-bytes = <64>;
>> +     };
>> +};
>
> --
> nvpublic

Regards,
Simon

[U-Boot] [PATCH 3/6] tegra: fdt: Add NAND controller binding and definitions

[Simon Glass]

Hi Stephen,

On Thu, Jan 19, 2012 at 5:03 PM, Stephen Warren <swarren@nvidia.com> wrote:
> On 01/13/2012 04:10 PM, Simon Glass wrote:
>> Add a NAND controller along with a bindings file for review.
>
> A few questions to start with:
>
>> diff --git a/doc/device-tree-bindings/nand/nvidia-nand.txt b/doc/device-tree-bindings/nand/nvidia-nand.txt
>
>> +NAND Flash
>> +----------
>> +
>> +(there isn't yet a generic binding in Linux, so this describes what is in
>> +U-Boot)
>> +
>> +The device node for a NAND flash device is as described in the document
>> +"Open Firmware Recommended Practice : Universal Serial Bus" with the
>> +following modifications and additions :
>> +
>> +Required properties :
>> + - compatible : Should be "manufacture,device", "nand-flash"
>> + - page-data-bytes : Number of bytes in the data area
>> + - page-spare-bytes : * Number of bytes in spare area
>> + ? ? ? spare area = skipped-spare-bytes + data-ecc-bytes + tag-bytes
>> + ? ? ? ? ? ? ? ? ? ? + tag-ecc-bytes
>> + - skipped-spare-bytes : Number of bytes to skip at start of spare area
>> + ? ? (these are typically used for bad block maintenance)
>> + - data-ecc-bytes : Number of ECC bytes for data area
>> + - tag-bytes :Number of tag bytes in spare area
>> + - tag-ecc-bytes : Number ECC bytes to be generated for tag bytes
>
> Are any of those values really needed?
>
> I looked through all the NAND references I could find in the Linux
> kernel in arch/*/boot/dts/* and none of them seem to have this kind of
> information.
>
> Looking at the drivers, they execute some form of identification command
> on the NAND device which gives back a device ID, which is then looked up
> in a table of known devices to give the information above.
>
> I checked the Tegra NAND driver that's in the kernel chromeos-3.0
> branch, and it does the same thing, albeit it open-codes some of the
> identification routines rather than just calling into the common code.

Well that's pretty grim I think. That code should try to use the ONFi
way if it is available and supported, or provide a mechanism to
configure it (via device tree) if not / preferred.

U-Boot does have the same ID lookup feature for the mtd layer, but it
only has page size, block size and bus width.

I do wonder why this driver cannot take more of the information it
needs from the upper levels. Admittedly not all of the info is there,
but it could perhaps be inferred.

>
> Judging by arch/*/boot/dts/*, it is standard practice to have a node for
> the NAND device itself though; it's used to house (optional) partition
> definitions. In the kernel,
> Documentation/devicetree/bindings/mtd/mtd-physmap.txt discusses the
> format of these partition nodes, and e.g.
> arch/powerpc/boot/dts/canyonlands.dts (amongst many) uses this on NAND.

Yes.

>
>> +Nvidia NAND Controller
>> +----------------------
>> +
>> +The device node for a NAND flash controller is as described in the document
>> +"Open Firmware Recommended Practice : Universal Serial Bus" with the
>> +following modifications and additions :
>> +
>> +Optional properties:
>> +
>> +wp-gpio : GPIO of write-protect line, three cells in the format:
>> + ? ? ? ? ? ? phandle, parameter, flags
>> +width : bus width of the NAND device in bits
>> +
>> +For now here is something specific to the Nvidia controller, with naming
>> +based on Nvidia's original (non-fdt) NAND driver:
>> +
>> + - nvidia,nand-timing : Timing parameters for the NAND. Each is in ns.
>> + ? ? Order is: MAX_TRP_TREA, TWB, Max(tCS, tCH, tALS, tALH),
>> + ? ? TWHR, Max(tCS, tCH, tALS, tALH), TWH, TWP, TRH, TADL
>> +
>> + ? ? MAX_TRP_TREA is:
>> + ? ? ? ? ? ? non-EDO mode: Max(tRP, tREA) + 6ns
>> + ? ? ? ? ? ? EDO mode: tRP timing
>
> At first glance, it seems reasonable to have this in the NAND node; it's
> certainly impossible to probe the timing parameters. Since NAND is so
> standardized though, I wonder if there is a standard set of timing
> parameters so that we could have a standard device tree binding for this?

The closest datasheet I could find was this:

http://www.hynix.com/datasheet/pdf/flash/HY27(U_S)F(08_16)1G2M%20Series(Rev1.1).pdf

It has a table on p21 with 32 timing parameters.

But what we want here is the timings for the Tegra NAND controller.
Every controller will have a different take on these parameters - some
will be ignored, some added together, etc. So I think it is better to
think in terms of what the controller needs rather than getting
sidetracked on some average of all the NAND datasheets.

>
>> +Example:
>> +
>> +nand-controller at 0x70008000 {
>> + ? ? compatible = "nvidia,tegra20-nand";
>> + ? ? wp-gpios = <&gpio 59 0>; ? ? ? ? ? ? ? ?/* PH3 */
>> + ? ? width = <8>;
>> + ? ? nvidia,timing = <26 100 20 80 20 10 12 10 70>;
>> + ? ? nand at 0 {
>> + ? ? ? ? ? ? compatible = "hynix,hy27uf4g2b", "nand-flash";
>> + ? ? ? ? ? ? page-data-bytes = <2048>;
>> + ? ? ? ? ? ? tag-ecc-bytes = <4>;
>> + ? ? ? ? ? ? tag-bytes = <20>;
>> + ? ? ? ? ? ? data-ecc-bytes = <36>;
>> + ? ? ? ? ? ? skipped-spare-bytes = <4>;
>> + ? ? ? ? ? ? page-spare-bytes = <64>;
>> + ? ? };
>> +};
>
> --
> nvpublic

Regards,
Simon

[PATCH 1/6] fdt: Add debugging to fdtdec_get_int/addr()

[Simon Glass]

The new debugging shows the value of integers and addresses read
from the device tree.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 lib/fdtdec.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index f08bfca..5d97e2a 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -77,11 +77,16 @@ fdt_addr_t fdtdec_get_addr(const void *blob, int node,
 	const fdt_addr_t *cell;
 	int len;
 
-	debug("get_addr: %s\n", prop_name);
+	debug("%s: %s\n", __func__, prop_name);
 	cell = fdt_getprop(blob, node, prop_name, &len);
 	if (cell && (len == sizeof(fdt_addr_t) ||
-			len == sizeof(fdt_addr_t) * 2))
-		return fdt_addr_to_cpu(*cell);
+			len == sizeof(fdt_addr_t) * 2)) {
+		fdt_addr_t addr = fdt_addr_to_cpu(*cell);
+
+		debug("%p\n", (void *)addr);
+		return addr;
+	}
+	debug("(not found)\n");
 	return FDT_ADDR_T_NONE;
 }
 
@@ -91,10 +96,15 @@ s32 fdtdec_get_int(const void *blob, int node, const char *prop_name,
 	const s32 *cell;
 	int len;
 
-	debug("get_size: %s\n", prop_name);
+	debug("%s: %s: ", __func__, prop_name);
 	cell = fdt_getprop(blob, node, prop_name, &len);
-	if (cell && len >= sizeof(s32))
-		return fdt32_to_cpu(cell[0]);
+	if (cell && len >= sizeof(s32)) {
+		s32 val = fdt32_to_cpu(cell[0]);
+
+		debug("%#x (%d)\n", val, val);
+		return val;
+	}
+	debug("(not found)\n");
 	return default_val;
 }
 
-- 
1.7.7.3

[U-Boot] [PATCH 1/6] fdt: Add debugging to fdtdec_get_int/addr()

[Simon Glass]

The new debugging shows the value of integers and addresses read
from the device tree.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 lib/fdtdec.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index f08bfca..5d97e2a 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -77,11 +77,16 @@ fdt_addr_t fdtdec_get_addr(const void *blob, int node,
 	const fdt_addr_t *cell;
 	int len;
 
-	debug("get_addr: %s\n", prop_name);
+	debug("%s: %s\n", __func__, prop_name);
 	cell = fdt_getprop(blob, node, prop_name, &len);
 	if (cell && (len == sizeof(fdt_addr_t) ||
-			len == sizeof(fdt_addr_t) * 2))
-		return fdt_addr_to_cpu(*cell);
+			len == sizeof(fdt_addr_t) * 2)) {
+		fdt_addr_t addr = fdt_addr_to_cpu(*cell);
+
+		debug("%p\n", (void *)addr);
+		return addr;
+	}
+	debug("(not found)\n");
 	return FDT_ADDR_T_NONE;
 }
 
@@ -91,10 +96,15 @@ s32 fdtdec_get_int(const void *blob, int node, const char *prop_name,
 	const s32 *cell;
 	int len;
 
-	debug("get_size: %s\n", prop_name);
+	debug("%s: %s: ", __func__, prop_name);
 	cell = fdt_getprop(blob, node, prop_name, &len);
-	if (cell && len >= sizeof(s32))
-		return fdt32_to_cpu(cell[0]);
+	if (cell && len >= sizeof(s32)) {
+		s32 val = fdt32_to_cpu(cell[0]);
+
+		debug("%#x (%d)\n", val, val);
+		return val;
+	}
+	debug("(not found)\n");
 	return default_val;
 }
 
-- 
1.7.7.3

[PATCH 4/6] tegra: fdt: Add NAND definitions to fdt

[Simon Glass]

Add a flash node to handle the NAND, including memory timings and
page / block size information.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 board/nvidia/dts/tegra2-seaboard.dts |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/board/nvidia/dts/tegra2-seaboard.dts b/board/nvidia/dts/tegra2-seaboard.dts
index 33260a4..cc9abe2 100644
--- a/board/nvidia/dts/tegra2-seaboard.dts
+++ b/board/nvidia/dts/tegra2-seaboard.dts
@@ -125,4 +125,19 @@
 				0x00000000 0x00000000 0x00000000 0x00000000 >;
 		};
 	};
+
+	nand-controller@0x70008000 {
+		wp-gpios = <&gpio 59 0>;		/* PH3 */
+		width = <8>;
+		nvidia,timing = <26 100 20 80 20 10 12 10 70>;
+		nand@0 {
+			compatible = "hynix,hy27uf4g2b", "nand-flash";
+			page-data-bytes = <2048>;
+			tag-ecc-bytes = <4>;
+			tag-bytes = <20>;
+			data-ecc-bytes = <36>;
+			skipped-spare-bytes = <4>;
+			page-spare-bytes = <64>;
+		};
+	};
 };
-- 
1.7.7.3

[U-Boot] [PATCH 4/6] tegra: fdt: Add NAND definitions to fdt

[Simon Glass]

Add a flash node to handle the NAND, including memory timings and
page / block size information.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 board/nvidia/dts/tegra2-seaboard.dts |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/board/nvidia/dts/tegra2-seaboard.dts b/board/nvidia/dts/tegra2-seaboard.dts
index 33260a4..cc9abe2 100644
--- a/board/nvidia/dts/tegra2-seaboard.dts
+++ b/board/nvidia/dts/tegra2-seaboard.dts
@@ -125,4 +125,19 @@
 				0x00000000 0x00000000 0x00000000 0x00000000 >;
 		};
 	};
+
+	nand-controller at 0x70008000 {
+		wp-gpios = <&gpio 59 0>;		/* PH3 */
+		width = <8>;
+		nvidia,timing = <26 100 20 80 20 10 12 10 70>;
+		nand at 0 {
+			compatible = "hynix,hy27uf4g2b", "nand-flash";
+			page-data-bytes = <2048>;
+			tag-ecc-bytes = <4>;
+			tag-bytes = <20>;
+			data-ecc-bytes = <36>;
+			skipped-spare-bytes = <4>;
+			page-spare-bytes = <64>;
+		};
+	};
 };
-- 
1.7.7.3

[U-Boot] [PATCH 5/6] tegra: nand: Add Tegra NAND driver

[Simon Glass]

From: Jim Lin <jilin@nvidia.com>

A device tree is used to configure the NAND, including memory
timings and block/pages sizes.

If this node is not present or is disabled, then NAND will not
be initialized.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 arch/arm/include/asm/arch-tegra2/tegra2.h |    1 +
 drivers/mtd/nand/Makefile                 |    1 +
 drivers/mtd/nand/tegra2_nand.c            | 1074 +++++++++++++++++++++++++++++
 drivers/mtd/nand/tegra2_nand.h            |  303 ++++++++
 include/fdtdec.h                          |    1 +
 lib/fdtdec.c                              |    1 +
 6 files changed, 1381 insertions(+), 0 deletions(-)
 create mode 100644 drivers/mtd/nand/tegra2_nand.c
 create mode 100644 drivers/mtd/nand/tegra2_nand.h

diff --git a/arch/arm/include/asm/arch-tegra2/tegra2.h b/arch/arm/include/asm/arch-tegra2/tegra2.h
index d4ada10..a080b63 100644
--- a/arch/arm/include/asm/arch-tegra2/tegra2.h
+++ b/arch/arm/include/asm/arch-tegra2/tegra2.h
@@ -39,6 +39,7 @@
 #define NV_PA_APB_UARTC_BASE	(NV_PA_APB_MISC_BASE + 0x6200)
 #define NV_PA_APB_UARTD_BASE	(NV_PA_APB_MISC_BASE + 0x6300)
 #define NV_PA_APB_UARTE_BASE	(NV_PA_APB_MISC_BASE + 0x6400)
+#define TEGRA2_NAND_BASE	(NV_PA_APB_MISC_BASE + 0x8000)
 #define TEGRA2_SPI_BASE		(NV_PA_APB_MISC_BASE + 0xC380)
 #define TEGRA2_PMC_BASE		(NV_PA_APB_MISC_BASE + 0xE400)
 #define TEGRA2_FUSE_BASE	(NV_PA_APB_MISC_BASE + 0xF800)
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index 36ee454..5bb67e8 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -60,6 +60,7 @@ COBJS-$(CONFIG_NAND_NOMADIK) += nomadik.o
 COBJS-$(CONFIG_NAND_S3C2410) += s3c2410_nand.o
 COBJS-$(CONFIG_NAND_S3C64XX) += s3c64xx.o
 COBJS-$(CONFIG_NAND_SPEAR) += spr_nand.o
+COBJS-$(CONFIG_TEGRA2_NAND) += tegra2_nand.o
 COBJS-$(CONFIG_NAND_OMAP_GPMC) += omap_gpmc.o
 COBJS-$(CONFIG_NAND_PLAT) += nand_plat.o
 endif
diff --git a/drivers/mtd/nand/tegra2_nand.c b/drivers/mtd/nand/tegra2_nand.c
new file mode 100644
index 0000000..e4ae6fc
--- /dev/null
+++ b/drivers/mtd/nand/tegra2_nand.c
@@ -0,0 +1,1074 @@
+/*
+ * Copyright (c) 2011 The Chromium OS Authors.
+ * (C) Copyright 2011 NVIDIA Corporation <www.nvidia.com>
+ * (C) Copyright 2006 Detlev Zundel, dzu at denx.de
+ * (C) Copyright 2006 DENX Software Engineering
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include <common.h>
+#include <asm/io.h>
+#include <nand.h>
+#include <asm/arch/clk_rst.h>
+#include <asm/arch/clock.h>
+#include <asm/arch/funcmux.h>
+#include <asm/arch/gpio.h>
+#include <asm/errno.h>
+#include <asm-generic/gpio.h>
+#include <fdtdec.h>
+#include "tegra2_nand.h"
+
+DECLARE_GLOBAL_DATA_PTR;
+
+#define NAND_CMD_TIMEOUT_MS		10
+
+static struct nand_ecclayout eccoob = {
+	.eccpos = {
+		4,  5,  6,  7,  8,  9,  10, 11, 12,
+		13, 14, 15, 16, 17, 18, 19, 20, 21,
+		22, 23, 24, 25, 26, 27, 28, 29, 30,
+		31, 32, 33, 34, 35, 36, 37, 38, 39,
+		60, 61, 62, 63,
+	},
+};
+
+enum {
+	ECC_OK,
+	ECC_TAG_ERROR = 1 << 0,
+	ECC_DATA_ERROR = 1 << 1
+};
+
+/* Timing parameters */
+enum {
+	FDT_NAND_MAX_TRP_TREA,
+	FDT_NAND_TWB,
+	FDT_NAND_MAX_TCR_TAR_TRR,
+	FDT_NAND_TWHR,
+	FDT_NAND_MAX_TCS_TCH_TALS_TALH,
+	FDT_NAND_TWH,
+	FDT_NAND_TWP,
+	FDT_NAND_TRH,
+	FDT_NAND_TADL,
+
+	FDT_NAND_TIMING_COUNT
+};
+
+/* Information about an attached NAND chip */
+struct fdt_nand {
+	struct nand_ctlr *reg;
+	int enabled;		/* 1 to enable, 0 to disable */
+	struct fdt_gpio_state wp_gpio;	/* write-protect GPIO */
+	int width;		/* bit width, normally 8 */
+	int tag_ecc_bytes;	/* ECC bytes to be generated for tag bytes */
+	int tag_bytes;		/* Tag bytes in spare area */
+	int data_ecc_bytes;	/* ECC bytes for data area */
+	int skipped_spare_bytes;
+	/*
+	 * How many bytes in spare area:
+	 * spare area = skipped bytes + ECC bytes of data area
+	 * + tag bytes + ECC bytes of tag bytes
+	 */
+	int page_spare_bytes;
+	int page_data_bytes;	/* Bytes in data area */
+	unsigned timing[FDT_NAND_TIMING_COUNT];
+};
+
+struct nand_info {
+	struct nand_ctlr *reg;
+
+	/*
+	* When running in PIO mode to get READ ID bytes from register
+	* RESP_0, we need this variable as an index to know which byte in
+	* register RESP_0 should be read.
+	* Because common code in nand_base.c invokes read_byte function two
+	* times for NAND_CMD_READID.
+	* And our controller returns 4 bytes@once in register RESP_0.
+	*/
+	int pio_byte_index;
+	struct fdt_nand config;
+};
+
+struct nand_info nand_ctrl;
+
+/**
+ * Wait for command completion
+ *
+ * @param reg	nand_ctlr structure
+ * @return
+ *	1 - Command completed
+ *	0 - Timeout
+ */
+static int nand_waitfor_cmd_completion(struct nand_ctlr *reg)
+{
+	int i;
+	u32 reg_val;
+
+	for (i = 0; i < NAND_CMD_TIMEOUT_MS * 1000; i++) {
+		if ((readl(&reg->command) & CMD_GO) ||
+			!(readl(&reg->status) &
+				STATUS_RBSY0) ||
+			!(readl(&reg->isr) &
+				ISR_IS_CMD_DONE)) {
+			udelay(1);
+			continue;
+		}
+		reg_val = readl(&reg->dma_mst_ctrl);
+		/*
+		 * If DMA_MST_CTRL_EN_A_ENABLE or
+		 * DMA_MST_CTRL_EN_B_ENABLE is set,
+		 * that means DMA engine is running, then we
+		 * have to wait until
+		 * DMA_MST_CTRL_IS_DMA_DONE
+		 * is cleared for DMA transfer completion.
+		 */
+		if (reg_val & (DMA_MST_CTRL_EN_A_ENABLE |
+			DMA_MST_CTRL_EN_B_ENABLE)) {
+			if (reg_val & DMA_MST_CTRL_IS_DMA_DONE)
+				return 1;
+		} else
+			return 1;
+		udelay(1);
+	}
+	return 0;
+}
+
+/**
+ * [DEFAULT] Read one byte from the chip
+ *
+ * @param mtd	MTD device structure
+ * @return	data byte
+ *
+ * Default read function for 8bit bus-width
+ */
+static uint8_t read_byte(struct mtd_info *mtd)
+{
+	struct nand_chip *chip = mtd->priv;
+	int dword_read;
+	struct nand_info *info;
+
+	info = (struct nand_info *) chip->priv;
+
+	dword_read = readl(&info->reg->resp);
+	dword_read = dword_read >> (8 * info->pio_byte_index);
+	info->pio_byte_index++;
+	return (uint8_t) dword_read;
+}
+
+/**
+ * [DEFAULT] Write buffer to chip
+ *
+ * @param mtd	MTD device structure
+ * @param buf	data buffer
+ * @param len	number of bytes to write
+ *
+ * Default write function for 8bit bus-width
+ */
+static void write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
+{
+	int i, j, l;
+	struct nand_chip *chip = mtd->priv;
+	struct nand_info *info;
+
+	info = (struct nand_info *) chip->priv;
+
+	for (i = 0; i < len / 4; i++) {
+		l = ((int *)buf)[i];
+		writel(l, &info->reg->resp);
+		writel(CMD_GO | CMD_PIO | CMD_TX |
+			(CMD_TRANS_SIZE_BYTES4 <<
+				CMD_TRANS_SIZE_SHIFT)
+			| CMD_A_VALID | CMD_CE0,
+			&info->reg->command);
+
+		if (!nand_waitfor_cmd_completion(info->reg))
+			printf("Command timeout during write_buf\n");
+	}
+	if (len & 3) {
+		l = 0;
+		for (j = 0; j < (len & 3); j++)
+			l |= (((int) buf[i * 4 + j]) << (8 * j));
+
+		writel(l, &info->reg->resp);
+		writel(CMD_GO | CMD_PIO | CMD_TX |
+			(((len & 3) - 1) << CMD_TRANS_SIZE_SHIFT) |
+			CMD_A_VALID | CMD_CE0,
+			&info->reg->command);
+		if (!nand_waitfor_cmd_completion(info->reg))
+			printf("Command timeout during write_buf\n");
+	}
+}
+
+/**
+ * [DEFAULT] Read chip data into buffer
+ *
+ * @param mtd	MTD device structure
+ * @param buf	buffer to store date
+ * @param len	number of bytes to read
+ *
+ * Default read function for 8bit bus-width
+ */
+static void read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
+{
+	int i, j, l;
+	struct nand_chip *chip = mtd->priv;
+	int *buf_dword;
+	struct nand_info *info;
+
+	info = (struct nand_info *) chip->priv;
+
+	buf_dword = (int *) buf;
+	for (i = 0; i < len / 4; i++) {
+		writel(CMD_GO | CMD_PIO | CMD_RX |
+			(CMD_TRANS_SIZE_BYTES4 <<
+				CMD_TRANS_SIZE_SHIFT)
+			| CMD_A_VALID | CMD_CE0,
+			&info->reg->command);
+		if (!nand_waitfor_cmd_completion(info->reg))
+			printf("Command timeout during read_buf\n");
+		l = readl(&info->reg->resp);
+		buf_dword[i] = l;
+	}
+	if (len & 3) {
+		writel(CMD_GO | CMD_PIO | CMD_RX |
+			(((len & 3) - 1) << CMD_TRANS_SIZE_SHIFT) |
+			CMD_A_VALID | CMD_CE0,
+			&info->reg->command);
+		if (!nand_waitfor_cmd_completion(info->reg))
+			printf("Command timeout during read_buf\n");
+		l = readl(&info->reg->resp);
+		for (j = 0; j < (len & 3); j++)
+			buf[i * 4 + j] = (char) (l >> (8 * j));
+	}
+}
+
+/**
+ * Check NAND status to see if it is ready or not
+ *
+ * @param mtd	MTD device structure
+ * @return
+ *	1 - ready
+ *	0 - not ready
+ */
+static int nand_dev_ready(struct mtd_info *mtd)
+{
+	register struct nand_chip *chip = mtd->priv;
+	int reg_val;
+	struct nand_info *info;
+
+	info = (struct nand_info *) chip->priv;
+
+	reg_val = readl(&info->reg->status);
+	if (reg_val & STATUS_RBSY0)
+		return 1;
+	else
+		return 0;
+}
+
+/* Hardware specific access to control-lines */
+static void nand_hwcontrol(struct mtd_info *mtd, int cmd,
+	unsigned int ctrl)
+{
+}
+
+/**
+ * Clear all interrupt status bits
+ *
+ * @param reg	nand_ctlr structure
+ */
+static void nand_clear_interrupt_status(struct nand_ctlr *reg)
+{
+	u32 reg_val;
+
+	/* Clear interrupt status */
+	reg_val = readl(&reg->isr);
+	writel(reg_val, &reg->isr);
+}
+
+/**
+ * [DEFAULT] Send command to NAND device
+ *
+ * @param mtd		MTD device structure
+ * @param command	the command to be sent
+ * @param column	the column address for this command, -1 if none
+ * @param page_addr	the page address for this command, -1 if none
+ */
+static void nand_command(struct mtd_info *mtd, unsigned int command,
+	int column, int page_addr)
+{
+	register struct nand_chip *chip = mtd->priv;
+	struct nand_info *info;
+
+	info = (struct nand_info *) chip->priv;
+
+	/*
+	 * Write out the command to the device.
+	 */
+	if (mtd->writesize < 2048) {
+		/*
+		 * Only command NAND_CMD_RESET or NAND_CMD_READID will come
+		 * here before mtd->writesize is initialized, we don't have
+		 * any action here because page size of NAND HY27UF084G2B
+		 * is 2048 bytes and mtd->writesize will be 2048 after
+		 * initialized.
+		 */
+	} else {
+		/* Emulate NAND_CMD_READOOB */
+		if (command == NAND_CMD_READOOB) {
+			column += mtd->writesize;
+			command = NAND_CMD_READ0;
+		}
+
+		/* Adjust columns for 16 bit bus-width */
+		if (column != -1 && (chip->options & NAND_BUSWIDTH_16))
+			column >>= 1;
+	}
+
+	nand_clear_interrupt_status(info->reg);
+
+	/* Stop DMA engine, clear DMA completion status */
+	writel(DMA_MST_CTRL_EN_A_DISABLE
+		| DMA_MST_CTRL_EN_B_DISABLE
+		| DMA_MST_CTRL_IS_DMA_DONE,
+		&info->reg->dma_mst_ctrl);
+
+	/*
+	 * Program and erase have their own busy handlers
+	 * status and sequential in needs no delay
+	 */
+	switch (command) {
+	case NAND_CMD_READID:
+		writel(NAND_CMD_READID, &info->reg->cmd_reg1);
+		writel(CMD_GO | CMD_CLE | CMD_ALE | CMD_PIO
+			| CMD_RX |
+			(CMD_TRANS_SIZE_BYTES4 << CMD_TRANS_SIZE_SHIFT)
+			| CMD_CE0,
+			&info->reg->command);
+		info->pio_byte_index = 0;
+		break;
+	case NAND_CMD_READ0:
+		writel(NAND_CMD_READ0, &info->reg->cmd_reg1);
+		writel(NAND_CMD_READSTART, &info->reg->cmd_reg2);
+		writel((page_addr << 16) | (column & 0xFFFF),
+			&info->reg->addr_reg1);
+		writel(page_addr >> 16, &info->reg->addr_reg2);
+		return;
+	case NAND_CMD_SEQIN:
+		writel(NAND_CMD_SEQIN, &info->reg->cmd_reg1);
+		writel(NAND_CMD_PAGEPROG, &info->reg->cmd_reg2);
+		writel((page_addr << 16) | (column & 0xFFFF),
+			&info->reg->addr_reg1);
+		writel(page_addr >> 16,
+			&info->reg->addr_reg2);
+		return;
+	case NAND_CMD_PAGEPROG:
+		return;
+	case NAND_CMD_ERASE1:
+		writel(NAND_CMD_ERASE1, &info->reg->cmd_reg1);
+		writel(NAND_CMD_ERASE2, &info->reg->cmd_reg2);
+		writel(page_addr, &info->reg->addr_reg1);
+		writel(CMD_GO | CMD_CLE | CMD_ALE |
+			CMD_SEC_CMD | CMD_CE0 | CMD_ALE_BYTES3,
+			&info->reg->command);
+		break;
+	case NAND_CMD_RNDOUT:
+		return;
+	case NAND_CMD_ERASE2:
+		return;
+	case NAND_CMD_STATUS:
+		writel(NAND_CMD_STATUS, &info->reg->cmd_reg1);
+		writel(CMD_GO | CMD_CLE | CMD_PIO | CMD_RX
+			| (CMD_TRANS_SIZE_BYTES1 <<
+				CMD_TRANS_SIZE_SHIFT)
+			| CMD_CE0,
+			&info->reg->command);
+		info->pio_byte_index = 0;
+		break;
+	case NAND_CMD_RESET:
+		writel(NAND_CMD_RESET, &info->reg->cmd_reg1);
+		writel(CMD_GO | CMD_CLE | CMD_CE0,
+			&info->reg->command);
+		break;
+	default:
+		return;
+	}
+	if (!nand_waitfor_cmd_completion(info->reg))
+		printf("Command 0x%02X timeout\n", command);
+}
+
+/**
+ * Check whether the pointed buffer are all 0xff (blank).
+ *
+ * @param buf	data buffer for blank check
+ * @param len	length of the buffer in byte
+ * @return
+ *	1 - blank
+ *	0 - non-blank
+ */
+static int blank_check(u8 *buf, int len)
+{
+	int i;
+
+	for (i = 0; i < len; i++)
+		if (buf[i] != 0xFF)
+			return 0;
+	return 1;
+}
+
+/**
+ * After a DMA transfer for read, we call this function to see whether there
+ * is any uncorrectable error on the pointed data buffer or oob buffer.
+ *
+ * @param reg		nand_ctlr structure
+ * @param databuf	data buffer
+ * @param a_len		data buffer length
+ * @param oobbuf	oob buffer
+ * @param b_len		oob buffer length
+ * @return
+ *	ECC_OK - no ECC error or correctable ECC error
+ *	ECC_TAG_ERROR - uncorrectable tag ECC error
+ *	ECC_DATA_ERROR - uncorrectable data ECC error
+ *	ECC_DATA_ERROR + ECC_TAG_ERROR - uncorrectable data+tag ECC error
+ */
+static int check_ecc_error(struct nand_ctlr *reg, u8 *databuf,
+	int a_len, u8 *oobbuf, int b_len)
+{
+	int return_val = ECC_OK;
+	u32 reg_val;
+
+	if (!(readl(&reg->isr) & ISR_IS_ECC_ERR))
+		return ECC_OK;
+
+	reg_val = readl(&reg->dec_status);
+	if ((reg_val & DEC_STATUS_A_ECC_FAIL) && databuf) {
+		reg_val = readl(&reg->bch_dec_status_buf);
+		/*
+		 * If uncorrectable error occurs on data area, then see whether
+		 * they are all FF. If all are FF, it's a blank page.
+		 * Not error.
+		 */
+		if ((reg_val & BCH_DEC_STATUS_FAIL_SEC_FLAG_MASK) &&
+			!blank_check(databuf, a_len))
+			return_val |= ECC_DATA_ERROR;
+	}
+
+	if ((reg_val & DEC_STATUS_B_ECC_FAIL) && oobbuf) {
+		reg_val = readl(&reg->bch_dec_status_buf);
+		/*
+		 * If uncorrectable error occurs on tag area, then see whether
+		 * they are all FF. If all are FF, it's a blank page.
+		 * Not error.
+		 */
+		if ((reg_val & BCH_DEC_STATUS_FAIL_TAG_MASK) &&
+			!blank_check(oobbuf, b_len))
+			return_val |= ECC_TAG_ERROR;
+	}
+
+	return return_val;
+}
+
+/**
+ * Set GO bit to send command to device
+ *
+ * @param reg	nand_ctlr structure
+ */
+static void start_command(struct nand_ctlr *reg)
+{
+	u32 reg_val;
+
+	reg_val = readl(&reg->command);
+	reg_val |= CMD_GO;
+	writel(reg_val, &reg->command);
+}
+
+/**
+ * Clear command GO bit, DMA GO bit, and DMA completion status
+ *
+ * @param reg	nand_ctlr structure
+ */
+static void stop_command(struct nand_ctlr *reg)
+{
+	/* Stop command */
+	writel(0, &reg->command);
+
+	/* Stop DMA engine and clear DMA completion status */
+	writel(DMA_MST_CTRL_GO_DISABLE
+		| DMA_MST_CTRL_IS_DMA_DONE,
+		&reg->dma_mst_ctrl);
+}
+
+/**
+ * Set up NAND bus width and page size
+ *
+ * @param info		nand_info structure
+ * @param *reg_val	address of reg_val
+ * @return none - value is set in reg_val
+ */
+static void set_bus_width_page_size(struct fdt_nand *config,
+	u32 *reg_val)
+{
+	if (config->width == 8)
+		*reg_val = CFG_BUS_WIDTH_8BIT;
+	else
+		*reg_val = CFG_BUS_WIDTH_16BIT;
+
+	if (config->page_data_bytes == 256)
+		*reg_val |= CFG_PAGE_SIZE_256;
+	else if (config->page_data_bytes == 512)
+		*reg_val |= CFG_PAGE_SIZE_512;
+	else if (config->page_data_bytes == 1024)
+		*reg_val |= CFG_PAGE_SIZE_1024;
+	else if (config->page_data_bytes == 2048)
+		*reg_val |= CFG_PAGE_SIZE_2048;
+}
+
+/**
+ * Page read/write function
+ *
+ * @param mtd		mtd info structure
+ * @param chip		nand chip info structure
+ * @param buf		data buffer
+ * @param page		page number
+ * @param with_ecc	1 to enable ECC, 0 to disable ECC
+ * @param is_writing	0 for read, 1 for write
+ * @return	0 when successfully completed
+ *		-EIO when command timeout
+ */
+static int nand_rw_page(struct mtd_info *mtd, struct nand_chip *chip,
+	uint8_t *buf, int page, int with_ecc, int is_writing)
+{
+	u32 reg_val;
+	int tag_size;
+	struct nand_oobfree *free = chip->ecc.layout->oobfree;
+	/* 128 is larger than the value that our HW can support. */
+	u32 tag_buf[128];
+	char *tag_ptr;
+	struct nand_info *info;
+	struct fdt_nand *config;
+
+	if (((int) buf) & 0x03) {
+		printf("buf 0x%X has to be 4-byte aligned\n", (u32) buf);
+		return -EINVAL;
+	}
+
+	info = (struct nand_info *) chip->priv;
+	config = &info->config;
+
+	/* Need to be 4-byte aligned */
+	tag_ptr = (char *) &tag_buf;
+
+	stop_command(info->reg);
+
+	writel((1 << chip->page_shift) - 1, &info->reg->dma_cfg_a);
+	writel((u32) buf, &info->reg->data_block_ptr);
+
+	if (with_ecc) {
+		writel((u32) tag_ptr, &info->reg->tag_ptr);
+		if (is_writing)
+			memcpy(tag_ptr, chip->oob_poi + free->offset,
+				config->tag_bytes +
+				config->tag_ecc_bytes);
+	} else
+		writel((u32) chip->oob_poi, &info->reg->tag_ptr);
+
+	set_bus_width_page_size(&info->config, &reg_val);
+
+	/* Set ECC selection, configure ECC settings */
+	if (with_ecc) {
+		tag_size = config->tag_bytes + config->tag_ecc_bytes;
+		reg_val |= (CFG_SKIP_SPARE_SEL_4
+			| CFG_SKIP_SPARE_ENABLE
+			| CFG_HW_ECC_CORRECTION_ENABLE
+			| CFG_ECC_EN_TAG_DISABLE
+			| CFG_HW_ECC_SEL_RS
+			| CFG_HW_ECC_ENABLE
+			| CFG_TVAL4
+			| (tag_size - 1));
+
+		if (!is_writing) {
+			tag_size += config->skipped_spare_bytes;
+			invalidate_dcache_range((unsigned long) tag_ptr,
+				((unsigned long) tag_ptr) + tag_size);
+		} else
+			flush_dcache_range((unsigned long) tag_ptr,
+				((unsigned long) tag_ptr) + tag_size);
+	} else {
+		tag_size = mtd->oobsize;
+		reg_val |= (CFG_SKIP_SPARE_DISABLE
+			| CFG_HW_ECC_CORRECTION_DISABLE
+			| CFG_ECC_EN_TAG_DISABLE
+			| CFG_HW_ECC_DISABLE
+			| (tag_size - 1));
+		if (!is_writing) {
+			invalidate_dcache_range((unsigned long) chip->oob_poi,
+				((unsigned long) chip->oob_poi) + tag_size);
+		} else {
+			flush_dcache_range((unsigned long) chip->oob_poi,
+				((unsigned long) chip->oob_poi) + tag_size);
+		}
+	}
+	writel(reg_val, &info->reg->config);
+
+	if (!is_writing) {
+		invalidate_dcache_range((unsigned long) buf,
+			((unsigned long) buf) +
+			(1 << chip->page_shift));
+	} else {
+		flush_dcache_range((unsigned long) buf,
+			((unsigned long) buf) +
+			(1 << chip->page_shift));
+	}
+
+	writel(BCH_CONFIG_BCH_ECC_DISABLE, &info->reg->bch_config);
+
+	writel(tag_size - 1, &info->reg->dma_cfg_b);
+
+	nand_clear_interrupt_status(info->reg);
+
+	reg_val = CMD_CLE | CMD_ALE
+		| CMD_SEC_CMD
+		| (CMD_ALE_BYTES5 << CMD_ALE_BYTE_SIZE_SHIFT)
+		| CMD_A_VALID
+		| CMD_B_VALID
+		| (CMD_TRANS_SIZE_BYTES_PAGE_SIZE_SEL <<
+		CMD_TRANS_SIZE_SHIFT)
+		| CMD_CE0;
+	if (!is_writing)
+		reg_val |= (CMD_AFT_DAT_DISABLE | CMD_RX);
+	else
+		reg_val |= (CMD_AFT_DAT_ENABLE | CMD_TX);
+	writel(reg_val, &info->reg->command);
+
+	/* Setup DMA engine */
+	reg_val = DMA_MST_CTRL_GO_ENABLE
+		| DMA_MST_CTRL_BURST_8WORDS
+		| DMA_MST_CTRL_EN_A_ENABLE
+		| DMA_MST_CTRL_EN_B_ENABLE;
+
+	if (!is_writing)
+		reg_val |= DMA_MST_CTRL_DIR_READ;
+	else
+		reg_val |= DMA_MST_CTRL_DIR_WRITE;
+
+	writel(reg_val, &info->reg->dma_mst_ctrl);
+
+	start_command(info->reg);
+
+	if (!nand_waitfor_cmd_completion(info->reg)) {
+		if (!is_writing)
+			printf("Read Page 0x%X timeout ", page);
+		else
+			printf("Write Page 0x%X timeout ", page);
+		if (with_ecc)
+			printf("with ECC");
+		else
+			printf("without ECC");
+		printf("\n");
+		return -EIO;
+	}
+
+	if (with_ecc && !is_writing) {
+		memcpy(chip->oob_poi, tag_ptr,
+			config->skipped_spare_bytes);
+		memcpy(chip->oob_poi + free->offset,
+			tag_ptr + config->skipped_spare_bytes,
+			config->tag_bytes);
+		reg_val = (u32) check_ecc_error(info->reg, (u8 *) buf,
+			1 << chip->page_shift,
+			(u8 *) (tag_ptr + config->skipped_spare_bytes),
+			config->tag_bytes);
+		if (reg_val & ECC_TAG_ERROR)
+			printf("Read Page 0x%X tag ECC error\n", page);
+		if (reg_val & ECC_DATA_ERROR)
+			printf("Read Page 0x%X data ECC error\n",
+				page);
+		if (reg_val & (ECC_DATA_ERROR | ECC_TAG_ERROR))
+			return -EIO;
+	}
+	return 0;
+}
+
+/**
+ * Hardware ecc based page read function
+ *
+ * @param mtd	mtd info structure
+ * @param chip	nand chip info structure
+ * @param buf	buffer to store read data
+ * @param page	page number to read
+ * @return	0 when successfully completed
+ *		-EIO when command timeout
+ */
+static int nand_read_page_hwecc(struct mtd_info *mtd,
+	struct nand_chip *chip, uint8_t *buf, int page)
+{
+	return nand_rw_page(mtd, chip, buf, page, 1, 0);
+}
+
+/**
+ * Hardware ecc based page write function
+ *
+ * @param mtd	mtd info structure
+ * @param chip	nand chip info structure
+ * @param buf	data buffer
+ */
+static void nand_write_page_hwecc(struct mtd_info *mtd,
+	struct nand_chip *chip, const uint8_t *buf)
+{
+	int page;
+	struct nand_info *info;
+
+	info = (struct nand_info *) chip->priv;
+
+	page = (readl(&info->reg->addr_reg1) >> 16) |
+		(readl(&info->reg->addr_reg2) << 16);
+
+	nand_rw_page(mtd, chip, (uint8_t *) buf, page, 1, 1);
+}
+
+
+/**
+ * Read raw page data without ecc
+ *
+ * @param mtd	mtd info structure
+ * @param chip	nand chip info structure
+ * @param buf	buffer to store read data
+ * @param page	page number to read
+ * @return	0 when successfully completed
+ *		-EINVAL when chip->oob_poi is not double-word aligned
+ *		-EIO when command timeout
+ */
+static int nand_read_page_raw(struct mtd_info *mtd,
+	struct nand_chip *chip, uint8_t *buf, int page)
+{
+	return nand_rw_page(mtd, chip, buf, page, 0, 0);
+}
+
+/**
+ * Raw page write function
+ *
+ * @param mtd	mtd info structure
+ * @param chip	nand chip info structure
+ * @param buf	data buffer
+ */
+static void nand_write_page_raw(struct mtd_info *mtd,
+		struct nand_chip *chip,	const uint8_t *buf)
+{
+	int page;
+	struct nand_info *info;
+
+	info = (struct nand_info *) chip->priv;
+	page = (readl(&info->reg->addr_reg1) >> 16) |
+		(readl(&info->reg->addr_reg2) << 16);
+
+	nand_rw_page(mtd, chip, (uint8_t *) buf, page, 0, 1);
+}
+
+/**
+ * OOB data read/write function
+ *
+ * @param mtd		mtd info structure
+ * @param chip		nand chip info structure
+ * @param page		page number to read
+ * @param with_ecc	1 to enable ECC, 0 to disable ECC
+ * @param is_writing	0 for read, 1 for write
+ * @return	0 when successfully completed
+ *		-EINVAL when chip->oob_poi is not double-word aligned
+ *		-EIO when command timeout
+ */
+static int nand_rw_oob(struct mtd_info *mtd, struct nand_chip *chip,
+	int page, int with_ecc, int is_writing)
+{
+	u32 reg_val;
+	int tag_size;
+	struct nand_oobfree *free = chip->ecc.layout->oobfree;
+	struct nand_info *info;
+
+	if (((int) chip->oob_poi) & 0x03)
+		return -EINVAL;
+
+	info = (struct nand_info *) chip->priv;
+	stop_command(info->reg);
+
+	writel((u32) chip->oob_poi, &info->reg->tag_ptr);
+
+	set_bus_width_page_size(&info->config, &reg_val);
+
+	/* Set ECC selection */
+	tag_size = mtd->oobsize;
+	if (with_ecc)
+		reg_val |= CFG_ECC_EN_TAG_ENABLE;
+	else
+		reg_val |= (CFG_ECC_EN_TAG_DISABLE);
+
+	reg_val |= ((tag_size - 1) |
+		CFG_SKIP_SPARE_DISABLE |
+		CFG_HW_ECC_CORRECTION_DISABLE |
+		CFG_HW_ECC_DISABLE);
+	writel(reg_val, &info->reg->config);
+
+	if (!is_writing)
+		invalidate_dcache_range((unsigned long) chip->oob_poi,
+			((unsigned long) chip->oob_poi) + tag_size);
+	else
+		flush_dcache_range((unsigned long) chip->oob_poi,
+			((unsigned long) chip->oob_poi) + tag_size);
+
+	writel(BCH_CONFIG_BCH_ECC_DISABLE, &info->reg->bch_config);
+
+	if (is_writing && with_ecc)
+		tag_size -= info->config.tag_ecc_bytes;
+
+	writel(tag_size - 1, &info->reg->dma_cfg_b);
+
+	nand_clear_interrupt_status(info->reg);
+
+	reg_val = CMD_CLE | CMD_ALE
+		| CMD_SEC_CMD
+		| (CMD_ALE_BYTES5 << CMD_ALE_BYTE_SIZE_SHIFT)
+		| CMD_B_VALID
+		| CMD_CE0;
+	if (!is_writing)
+		reg_val |= (CMD_AFT_DAT_DISABLE | CMD_RX);
+	else
+		reg_val |= (CMD_AFT_DAT_ENABLE | CMD_TX);
+	writel(reg_val, &info->reg->command);
+
+	/* Setup DMA engine */
+	reg_val = DMA_MST_CTRL_GO_ENABLE
+		| DMA_MST_CTRL_BURST_8WORDS
+		| DMA_MST_CTRL_EN_B_ENABLE;
+	if (!is_writing)
+		reg_val |= DMA_MST_CTRL_DIR_READ;
+	else
+		reg_val |= DMA_MST_CTRL_DIR_WRITE;
+
+	writel(reg_val, &info->reg->dma_mst_ctrl);
+
+	start_command(info->reg);
+
+	if (!nand_waitfor_cmd_completion(info->reg)) {
+		if (!is_writing)
+			printf("Read OOB of Page 0x%X timeout\n", page);
+		else
+			printf("Write OOB of Page 0x%X timeout\n", page);
+		return -EIO;
+	}
+
+	if (with_ecc && !is_writing) {
+		reg_val = (u32) check_ecc_error(info->reg, 0, 0,
+			(u8 *) (chip->oob_poi + free->offset),
+			info->config.tag_bytes);
+		if (reg_val & ECC_TAG_ERROR)
+			printf("Read OOB of Page 0x%X tag ECC error\n", page);
+	}
+	return 0;
+}
+
+/**
+ * OOB data read function
+ *
+ * @param mtd		mtd info structure
+ * @param chip		nand chip info structure
+ * @param page		page number to read
+ * @param sndcmd	flag whether to issue read command or not
+ * @return	1 - issue read command next time
+ *		0 - not to issue
+ */
+static int nand_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
+	int page, int sndcmd)
+{
+	if (sndcmd) {
+		chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
+		sndcmd = 0;
+	}
+	nand_rw_oob(mtd, chip, page, 0, 0);
+	return sndcmd;
+}
+
+/**
+ * OOB data write function
+ *
+ * @param mtd	mtd info structure
+ * @param chip	nand chip info structure
+ * @param page	page number to write
+ * @return	0 when successfully completed
+ *		-EINVAL when chip->oob_poi is not double-word aligned
+ *		-EIO when command timeout
+ */
+static int nand_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
+	int page)
+{
+	chip->cmdfunc(mtd, NAND_CMD_SEQIN, mtd->writesize, page);
+
+	return nand_rw_oob(mtd, chip, page, 0, 1);
+}
+
+/**
+ * Set up NAND memory timings according to the provided parameters
+ *
+ * @param timing	Timing parameters
+ * @param reg		NAND controller register address
+ */
+static void setup_timing(unsigned timing[FDT_NAND_TIMING_COUNT],
+			 struct nand_ctlr *reg)
+{
+	u32 reg_val, clk_rate, clk_period, time_val;
+
+	clk_rate = (u32) clock_get_periph_rate(PERIPH_ID_NDFLASH,
+		CLOCK_ID_PERIPH) / 1000000;
+	clk_period = 1000 / clk_rate;
+	reg_val = ((timing[FDT_NAND_MAX_TRP_TREA] / clk_period) <<
+		TIMING_TRP_RESP_CNT_SHIFT) & TIMING_TRP_RESP_CNT_MASK;
+	reg_val |= ((timing[FDT_NAND_TWB] / clk_period) <<
+		TIMING_TWB_CNT_SHIFT) & TIMING_TWB_CNT_MASK;
+	time_val = timing[FDT_NAND_MAX_TCR_TAR_TRR] / clk_period;
+	if (time_val > 2)
+		reg_val |= ((time_val - 2) << TIMING_TCR_TAR_TRR_CNT_SHIFT) &
+			TIMING_TCR_TAR_TRR_CNT_MASK;
+	reg_val |= ((timing[FDT_NAND_TWHR] / clk_period) <<
+		TIMING_TWHR_CNT_SHIFT) & TIMING_TWHR_CNT_MASK;
+	time_val = timing[FDT_NAND_MAX_TCS_TCH_TALS_TALH] / clk_period;
+	if (time_val > 1)
+		reg_val |= ((time_val - 1) << TIMING_TCS_CNT_SHIFT) &
+			TIMING_TCS_CNT_MASK;
+	reg_val |= ((timing[FDT_NAND_TWH] / clk_period) <<
+		TIMING_TWH_CNT_SHIFT) & TIMING_TWH_CNT_MASK;
+	reg_val |= ((timing[FDT_NAND_TWP] / clk_period) <<
+		TIMING_TWP_CNT_SHIFT) & TIMING_TWP_CNT_MASK;
+	reg_val |= ((timing[FDT_NAND_TRH] / clk_period) <<
+		TIMING_TRH_CNT_SHIFT) & TIMING_TRH_CNT_MASK;
+	reg_val |= ((timing[FDT_NAND_MAX_TRP_TREA] / clk_period) <<
+		TIMING_TRP_CNT_SHIFT) & TIMING_TRP_CNT_MASK;
+	writel(reg_val, &reg->timing);
+
+	reg_val = 0;
+	time_val = timing[FDT_NAND_TADL] / clk_period;
+	if (time_val > 2)
+		reg_val = (time_val - 2) & TIMING2_TADL_CNT_MASK;
+	writel(reg_val, &reg->timing2);
+}
+
+/**
+ * Decode NAND parameters from the device tree
+ *
+ * @param blob	Device tree blob
+ * @param node	Node containing "nand-flash" compatble node
+ * @return 0 if ok, -ve on error (FDT_ERR_...)
+ */
+int fdt_decode_nand(const void *blob, int node, struct fdt_nand *config)
+{
+	int err;
+
+	config->reg = (struct nand_ctlr *)fdtdec_get_addr(blob, node, "reg");
+	config->enabled = fdtdec_get_is_enabled(blob, node);
+	config->width = fdtdec_get_int(blob, node, "width", 8);
+	err = fdtdec_decode_gpio(blob, node, "wp-gpios", &config->wp_gpio);
+	if (err)
+		return err;
+	err = fdtdec_get_int_array(blob, node, "nvidia,timing",
+			config->timing, FDT_NAND_TIMING_COUNT);
+	if (err < 0)
+		return err;
+
+	/* Now look up the controller and decode that */
+	node = fdt_next_node(blob, node, NULL);
+	if (node < 0)
+		return node;
+
+	config->page_data_bytes = fdtdec_get_int(blob, node,
+						"page-data-bytes", -1);
+	config->tag_ecc_bytes = fdtdec_get_int(blob, node,
+						"tag-ecc-bytes", -1);
+	config->tag_bytes = fdtdec_get_int(blob, node, "tag-bytes", -1);
+	config->data_ecc_bytes = fdtdec_get_int(blob, node,
+						"data-ecc-bytes", -1);
+	config->skipped_spare_bytes = fdtdec_get_int(blob, node,
+						"skipped-spare-bytes", -1);
+	config->page_spare_bytes = fdtdec_get_int(blob, node,
+						"page-spare-bytes", -1);
+	if (config->page_data_bytes == -1 || config->tag_ecc_bytes == -1 ||
+		config->tag_bytes == -1 || config->data_ecc_bytes == -1 ||
+		config->skipped_spare_bytes == -1 ||
+		config->page_spare_bytes == -1)
+		return -FDT_ERR_NOTFOUND;
+
+	return 0;
+}
+
+/**
+ * Board-specific NAND initialization
+ *
+ * @param nand	nand chip info structure
+ * @return 0, after initialized, -1 on error
+ */
+int board_nand_init(struct nand_chip *nand)
+{
+	struct nand_info *info = &nand_ctrl;
+	struct fdt_nand *config = &info->config;
+	int node;
+
+	node = fdtdec_next_compatible(gd->fdt_blob, 0,
+				      COMPAT_NVIDIA_TEGRA20_NAND);
+	if (node < 0)
+		return -1;
+	if (fdt_decode_nand(gd->fdt_blob, node, config)) {
+		printf("Could not decode nand-flash in device tree\n");
+		return -1;
+	}
+	if (!config->enabled)
+		return -1;
+	info->reg = config->reg;
+	eccoob.eccbytes = config->data_ecc_bytes + config->tag_ecc_bytes;
+	eccoob.oobavail = config->tag_bytes;
+	eccoob.oobfree[0].offset = config->skipped_spare_bytes +
+				config->data_ecc_bytes;
+	eccoob.oobfree[0].length = config->tag_bytes;
+
+	nand->ecc.mode = NAND_ECC_HW;
+	nand->ecc.layout = &eccoob;
+	nand->ecc.size = config->page_data_bytes;
+	nand->ecc.bytes = config->page_spare_bytes;
+
+	nand->options = LP_OPTIONS;
+	nand->cmdfunc = nand_command;
+	nand->read_byte = read_byte;
+	nand->read_buf = read_buf;
+	nand->write_buf = write_buf;
+	nand->ecc.read_page = nand_read_page_hwecc;
+	nand->ecc.write_page = nand_write_page_hwecc;
+	nand->ecc.read_page_raw = nand_read_page_raw;
+	nand->ecc.write_page_raw = nand_write_page_raw;
+	nand->ecc.read_oob = nand_read_oob;
+	nand->ecc.write_oob = nand_write_oob;
+	nand->cmd_ctrl = nand_hwcontrol;
+	nand->dev_ready  = nand_dev_ready;
+	nand->priv = &nand_ctrl;
+
+	/* Adjust controller clock rate */
+	clock_start_periph_pll(PERIPH_ID_NDFLASH, CLOCK_ID_PERIPH, 52000000);
+
+	/* Adjust timing for NAND device */
+	setup_timing(config->timing, info->reg);
+
+	funcmux_select(PERIPH_ID_NDFLASH, FUNCMUX_DEFAULT);
+	fdtdec_setup_gpio(&config->wp_gpio);
+	gpio_direction_output(config->wp_gpio.gpio, 1);
+
+	return 0;
+}
diff --git a/drivers/mtd/nand/tegra2_nand.h b/drivers/mtd/nand/tegra2_nand.h
new file mode 100644
index 0000000..58c8a95
--- /dev/null
+++ b/drivers/mtd/nand/tegra2_nand.h
@@ -0,0 +1,303 @@
+/*
+ * (C) Copyright 2011 NVIDIA Corporation <www.nvidia.com>
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+/* Oh dear I suspect no one will like these Bit values? */
+enum {
+	Bit0 = 1 << 0,
+	Bit1 = 1 << 1,
+	Bit2 = 1 << 2,
+	Bit3 = 1 << 3,
+	Bit4 = 1 << 4,
+	Bit5 = 1 << 5,
+	Bit6 = 1 << 6,
+	Bit7 = 1 << 7,
+	Bit8 = 1 << 8,
+	Bit9 = 1 << 9,
+	Bit10 = 1 << 10,
+	Bit11 = 1 << 11,
+	Bit12 = 1 << 12,
+	Bit13 = 1 << 13,
+	Bit14 = 1 << 14,
+	Bit15 = 1 << 15,
+	Bit16 = 1 << 16,
+	Bit17 = 1 << 17,
+	Bit18 = 1 << 18,
+	Bit19 = 1 << 19,
+	Bit20 = 1 << 20,
+	Bit21 = 1 << 21,
+	Bit22 = 1 << 22,
+	Bit23 = 1 << 23,
+	Bit24 = 1 << 24,
+	Bit25 = 1 << 25,
+	Bit26 = 1 << 26,
+	Bit27 = 1 << 27,
+	Bit28 = 1 << 28,
+	Bit29 = 1 << 29,
+	Bit30 = 1 << 30,
+	Bit31 = 1 << 31
+};
+
+/* register offset */
+#define COMMAND_0		0x00
+#define CMD_GO		Bit31
+#define CMD_CLE		Bit30
+#define CMD_ALE		Bit29
+#define CMD_PIO		Bit28
+#define CMD_TX		Bit27
+#define CMD_RX		Bit26
+#define CMD_SEC_CMD	Bit25
+#define CMD_AFT_DAT_MASK	Bit24
+#define CMD_AFT_DAT_DISABLE	0
+#define CMD_AFT_DAT_ENABLE	Bit24
+#define CMD_TRANS_SIZE_SHIFT	20
+enum {
+	CMD_TRANS_SIZE_BYTES1 = 0,
+	CMD_TRANS_SIZE_BYTES2,
+	CMD_TRANS_SIZE_BYTES3,
+	CMD_TRANS_SIZE_BYTES4,
+	CMD_TRANS_SIZE_BYTES5,
+	CMD_TRANS_SIZE_BYTES6,
+	CMD_TRANS_SIZE_BYTES7,
+	CMD_TRANS_SIZE_BYTES8,
+	CMD_TRANS_SIZE_BYTES_PAGE_SIZE_SEL
+};
+
+#define CMD_TRANS_SIZE_BYTES_PAGE_SIZE_SEL	8
+#define CMD_A_VALID	Bit19
+#define CMD_B_VALID	Bit18
+#define CMD_RD_STATUS_CHK	Bit17
+#define CMD_R_BSY_CHK	Bit16
+#define CMD_CE7		Bit15
+#define CMD_CE6		Bit14
+#define CMD_CE5		Bit13
+#define CMD_CE4		Bit12
+#define CMD_CE3		Bit11
+#define CMD_CE2		Bit10
+#define CMD_CE1		Bit9
+#define CMD_CE0		Bit8
+#define CMD_CLE_BYTE_SIZE_SHIFT	4
+enum {
+	CMD_CLE_BYTES1 = 0,
+	CMD_CLE_BYTES2,
+	CMD_CLE_BYTES3,
+	CMD_CLE_BYTES4,
+};
+#define CMD_ALE_BYTE_SIZE_SHIFT	0
+enum {
+	CMD_ALE_BYTES1 = 0,
+	CMD_ALE_BYTES2,
+	CMD_ALE_BYTES3,
+	CMD_ALE_BYTES4,
+	CMD_ALE_BYTES5,
+	CMD_ALE_BYTES6,
+	CMD_ALE_BYTES7,
+	CMD_ALE_BYTES8
+};
+
+#define STATUS_0		0x04
+#define STATUS_RBSY0	Bit8
+
+#define ISR_0			0x08
+#define ISR_IS_CMD_DONE	Bit5
+#define ISR_IS_ECC_ERR	Bit4
+
+#define IER_0			0x0C
+
+#define CFG_0		0x10
+#define CFG_HW_ECC_MASK		Bit31
+#define CFG_HW_ECC_DISABLE		0
+#define CFG_HW_ECC_ENABLE		Bit31
+#define CFG_HW_ECC_SEL_MASK		Bit30
+#define CFG_HW_ECC_SEL_HAMMING	0
+#define CFG_HW_ECC_SEL_RS		Bit30
+#define CFG_HW_ECC_CORRECTION_MASK	Bit29
+#define CFG_HW_ECC_CORRECTION_DISABLE	0
+#define CFG_HW_ECC_CORRECTION_ENABLE	Bit29
+#define CFG_PIPELINE_EN_MASK		Bit28
+#define CFG_PIPELINE_EN_DISABLE	0
+#define CFG_PIPELINE_EN_ENABLE	Bit28
+#define CFG_ECC_EN_TAG_MASK		Bit27
+#define CFG_ECC_EN_TAG_DISABLE	0
+#define CFG_ECC_EN_TAG_ENABLE	Bit27
+#define CFG_TVALUE_MASK		(Bit25 | Bit24)
+enum {
+	CFG_TVAL4 = 0 << 24,
+	CFG_TVAL6 = 1 << 24,
+	CFG_TVAL8 = 2 << 24
+};
+#define CFG_SKIP_SPARE_MASK		Bit23
+#define CFG_SKIP_SPARE_DISABLE	0
+#define CFG_SKIP_SPARE_ENABLE	Bit23
+#define CFG_COM_BSY_MASK		Bit22
+#define CFG_COM_BSY_DISABLE		0
+#define CFG_COM_BSY_ENABLE		Bit22
+#define CFG_BUS_WIDTH_MASK		Bit21
+#define CFG_BUS_WIDTH_8BIT		0
+#define CFG_BUS_WIDTH_16BIT		Bit21
+#define CFG_LPDDR1_MODE_MASK		Bit20
+#define CFG_LPDDR1_MODE_DISABLE	0
+#define CFG_LPDDR1_MODE_ENABLE	Bit20
+#define CFG_EDO_MODE_MASK		Bit19
+#define CFG_EDO_MODE_DISABLE		0
+#define CFG_EDO_MODE_ENABLE		Bit19
+#define CFG_PAGE_SIZE_SEL_MASK	(Bit18 | Bit17 | Bit16)
+enum {
+	CFG_PAGE_SIZE_256	= 0 << 16,
+	CFG_PAGE_SIZE_512	= 1 << 16,
+	CFG_PAGE_SIZE_1024	= 2 << 16,
+	CFG_PAGE_SIZE_2048	= 3 << 16,
+	CFG_PAGE_SIZE_4096	= 4 << 16
+};
+#define CFG_SKIP_SPARE_SEL_MASK		(Bit15 | Bit14)
+enum {
+	CFG_SKIP_SPARE_SEL_4		= 0 << 14,
+	CFG_SKIP_SPARE_SEL_8		= 1 << 14,
+	CFG_SKIP_SPARE_SEL_12	= 2 << 14,
+	CFG_SKIP_SPARE_SEL_16	= 3 << 14
+};
+#define CFG_TAG_BYTE_SIZE_MASK	0x1FF
+
+#define TIMING_0		0x14
+#define TIMING_TRP_RESP_CNT_SHIFT	28
+#define TIMING_TRP_RESP_CNT_MASK	(Bit31 | Bit30 | Bit29 | Bit28)
+#define TIMING_TWB_CNT_SHIFT		24
+#define TIMING_TWB_CNT_MASK		(Bit27 | Bit26 | Bit25 | Bit24)
+#define TIMING_TCR_TAR_TRR_CNT_SHIFT	20
+#define TIMING_TCR_TAR_TRR_CNT_MASK	(Bit23 | Bit22 | Bit21 | Bit20)
+#define TIMING_TWHR_CNT_SHIFT		16
+#define TIMING_TWHR_CNT_MASK		(Bit19 | Bit18 | Bit17 | Bit16)
+#define TIMING_TCS_CNT_SHIFT		14
+#define TIMING_TCS_CNT_MASK		(Bit15 | Bit14)
+#define TIMING_TWH_CNT_SHIFT		12
+#define TIMING_TWH_CNT_MASK		(Bit13 | Bit12)
+#define TIMING_TWP_CNT_SHIFT		8
+#define TIMING_TWP_CNT_MASK		(Bit11 | Bit10 | Bit9 | Bit8)
+#define TIMING_TRH_CNT_SHIFT		4
+#define TIMING_TRH_CNT_MASK		(Bit5 | Bit4)
+#define TIMING_TRP_CNT_SHIFT		0
+#define TIMING_TRP_CNT_MASK		(Bit3 | Bit2 | Bit1 | Bit0)
+
+#define RESP_0			0x18
+
+#define TIMING2_0		0x1C
+#define TIMING2_TADL_CNT_SHIFT		0
+#define TIMING2_TADL_CNT_MASK		(Bit3 | Bit2 | Bit1 | Bit0)
+
+#define CMD_REG1_0		0x20
+#define CMD_REG2_0		0x24
+#define ADDR_REG1_0		0x28
+#define ADDR_REG2_0		0x2C
+
+#define DMA_MST_CTRL_0		0x30
+#define DMA_MST_CTRL_GO_MASK		Bit31
+#define DMA_MST_CTRL_GO_DISABLE		0
+#define DMA_MST_CTRL_GO_ENABLE		Bit31
+#define DMA_MST_CTRL_DIR_MASK		Bit30
+#define DMA_MST_CTRL_DIR_READ		0
+#define DMA_MST_CTRL_DIR_WRITE		Bit30
+#define DMA_MST_CTRL_PERF_EN_MASK	Bit29
+#define DMA_MST_CTRL_PERF_EN_DISABLE	0
+#define DMA_MST_CTRL_PERF_EN_ENABLE	Bit29
+#define DMA_MST_CTRL_REUSE_BUFFER_MASK	Bit27
+#define DMA_MST_CTRL_REUSE_BUFFER_DISABLE	0
+#define DMA_MST_CTRL_REUSE_BUFFER_ENABLE	Bit27
+#define DMA_MST_CTRL_BURST_SIZE_MASK	(Bit26 | Bit25 | Bit24)
+enum {
+	DMA_MST_CTRL_BURST_1WORDS	= 2 << 24,
+	DMA_MST_CTRL_BURST_4WORDS	= 3 << 24,
+	DMA_MST_CTRL_BURST_8WORDS	= 4 << 24,
+	DMA_MST_CTRL_BURST_16WORDS	= 5 << 24
+};
+#define DMA_MST_CTRL_IS_DMA_DONE	Bit20
+#define DMA_MST_CTRL_EN_A_MASK		Bit2
+#define DMA_MST_CTRL_EN_A_DISABLE	0
+#define DMA_MST_CTRL_EN_A_ENABLE	Bit2
+#define DMA_MST_CTRL_EN_B_MASK		Bit1
+#define DMA_MST_CTRL_EN_B_DISABLE	0
+#define DMA_MST_CTRL_EN_B_ENABLE	Bit1
+
+#define DMA_CFG_A_0		0x34
+#define DMA_CFG_B_0		0x38
+#define FIFO_CTRL_0		0x3C
+#define DATA_BLOCK_PTR_0	0x40
+#define TAG_PTR_0		0x44
+#define ECC_PTR_0		0x48
+
+#define DEC_STATUS_0		0x4C
+#define DEC_STATUS_A_ECC_FAIL		Bit1
+#define DEC_STATUS_B_ECC_FAIL		Bit0
+
+#define BCH_CONFIG_0		0xCC
+#define BCH_CONFIG_BCH_TVALUE_MASK	(Bit5 | Bit4)
+enum {
+	BCH_CONFIG_BCH_TVAL4	= 0 << 4,
+	BCH_CONFIG_BCH_TVAL8	= 1 << 4,
+	BCH_CONFIG_BCH_TVAL14	= 2 << 4,
+	BCH_CONFIG_BCH_TVAL16	= 3 << 4
+};
+#define BCH_CONFIG_BCH_ECC_MASK		Bit0
+#define BCH_CONFIG_BCH_ECC_DISABLE	0
+#define BCH_CONFIG_BCH_ECC_ENABLE	Bit0
+
+#define BCH_DEC_RESULT_0	0xD0
+#define BCH_DEC_RESULT_CORRFAIL_ERR_MASK	Bit8
+#define BCH_DEC_RESULT_PAGE_COUNT_MASK		0xFF
+
+#define BCH_DEC_STATUS_BUF_0	0xD4
+#define BCH_DEC_STATUS_FAIL_SEC_FLAG_MASK	0xFF000000
+#define BCH_DEC_STATUS_CORR_SEC_FLAG_MASK	0x00FF0000
+#define BCH_DEC_STATUS_FAIL_TAG_MASK		Bit14
+#define BCH_DEC_STATUS_CORR_TAG_MASK		Bit13
+#define BCH_DEC_STATUS_MAX_CORR_CNT_MASK (Bit12 | Bit11 | Bit10 | Bit9 | Bit8)
+#define BCH_DEC_STATUS_PAGE_NUMBER_MASK		0xFF
+
+#define LP_OPTIONS (NAND_NO_READRDY | NAND_NO_AUTOINCR)
+
+struct nand_ctlr {
+	u32	command;	/* offset 00h */
+	u32	status;		/* offset 04h */
+	u32	isr;		/* offset 08h */
+	u32	ier;		/* offset 0Ch */
+	u32	config;		/* offset 10h */
+	u32	timing;		/* offset 14h */
+	u32	resp;		/* offset 18h */
+	u32	timing2;	/* offset 1Ch */
+	u32	cmd_reg1;	/* offset 20h */
+	u32	cmd_reg2;	/* offset 24h */
+	u32	addr_reg1;	/* offset 28h */
+	u32	addr_reg2;	/* offset 2Ch */
+	u32	dma_mst_ctrl;	/* offset 30h */
+	u32	dma_cfg_a;	/* offset 34h */
+	u32	dma_cfg_b;	/* offset 38h */
+	u32	fifo_ctrl;	/* offset 3Ch */
+	u32	data_block_ptr;	/* offset 40h */
+	u32	tag_ptr;	/* offset 44h */
+	u32	resv1;		/* offset 48h */
+	u32	dec_status;	/* offset 4Ch */
+	u32	hwstatus_cmd;	/* offset 50h */
+	u32	hwstatus_mask;	/* offset 54h */
+	u32	resv2[29];
+	u32	bch_config;	/* offset CCh */
+	u32	bch_dec_result;	/* offset D0h */
+	u32	bch_dec_status_buf;
+				/* offset D4h */
+};
diff --git a/include/fdtdec.h b/include/fdtdec.h
index a14f2d7..64e2f88 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -62,6 +62,7 @@ enum fdt_compat_id {
 	COMPAT_NVIDIA_TEGRA20_DVC,	/* Tegra2 dvc (really just i2c) */
 	COMPAT_NVIDIA_TEGRA20_EMC,	/* Tegra2 memory controller */
 	COMPAT_NVIDIA_TEGRA20_EMC_TABLE, /* Tegra2 memory timing table */
+	COMPAT_NVIDIA_TEGRA20_NAND,	/* Tegra2 NAND controller */
 
 	COMPAT_COUNT,
 };
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index 5d97e2a..db377c7 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -42,6 +42,7 @@ static const char * const compat_names[COMPAT_COUNT] = {
 	COMPAT(NVIDIA_TEGRA20_DVC, "nvidia,tegra20-i2c-dvc"),
 	COMPAT(NVIDIA_TEGRA20_EMC, "nvidia,tegra20-emc"),
 	COMPAT(NVIDIA_TEGRA20_EMC_TABLE, "nvidia,tegra20-emc-table"),
+	COMPAT(NVIDIA_TEGRA20_NAND, "nvidia,tegra20-nand"),
 };
 
 const char *fdtdec_get_compatible(enum fdt_compat_id id)
-- 
1.7.7.3

[U-Boot] [PATCH 5/6] tegra: nand: Add Tegra NAND driver

[Mike Frysinger]

On Friday 13 January 2012 18:10:55 Simon Glass wrote:
> From: Jim Lin <jilin@nvidia.com>
> 
> A device tree is used to configure the NAND, including memory
> timings and block/pages sizes.
> 
> If this node is not present or is disabled, then NAND will not
> be initialized.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

the Author needs to sign off too ...
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120114/14670706/attachment.pgp>

[U-Boot] [PATCH 5/6] tegra: nand: Add Tegra NAND driver

[Simon Glass]

Hi Mike,

On Sat, Jan 14, 2012 at 8:03 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Friday 13 January 2012 18:10:55 Simon Glass wrote:
>> From: Jim Lin <jilin@nvidia.com>
>>
>> A device tree is used to configure the NAND, including memory
>> timings and block/pages sizes.
>>
>> If this node is not present or is disabled, then NAND will not
>> be initialized.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>
> the Author needs to sign off too ...

I have a lot like that. It doesn't feel right for me to just add their
sign-off after all my changes. I was hoping that they would reply to
the list with it. Does that work? The original commit does not have a
sign-off.

Regards,
Simon

> -mike

[U-Boot] [PATCH 5/6] tegra: nand: Add Tegra NAND driver

[Mike Frysinger]

On Saturday 14 January 2012 23:08:45 Simon Glass wrote:
> On Sat, Jan 14, 2012 at 8:03 PM, Mike Frysinger wrote:
> > On Friday 13 January 2012 18:10:55 Simon Glass wrote:
> >> From: Jim Lin <jilin@nvidia.com>
> >> 
> >> A device tree is used to configure the NAND, including memory
> >> timings and block/pages sizes.
> >> 
> >> If this node is not present or is disabled, then NAND will not
> >> be initialized.
> >> 
> >> Signed-off-by: Simon Glass <sjg@chromium.org>
> > 
> > the Author needs to sign off too ...
> 
> I have a lot like that. It doesn't feel right for me to just add their
> sign-off after all my changes. I was hoping that they would reply to
> the list with it. Does that work? The original commit does not have a
> sign-off.

sure, if the author replies with their s-o-b, or says they're OK with you 
adding it, that's fine.  but you're right that you can't just add it yourself 
without them saying so first ...
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120114/8c114ff7/attachment.pgp>

[U-Boot] [PATCH 5/6] tegra: nand: Add Tegra NAND driver

[Jim Lin]

Simon,
Could you also add

  Signed-off-by: Jim Lin <jilin@nvidia.com>

for me?

Thanks,
Jim


-----Original Message-----
From: Mike Frysinger [mailto:vapier at gentoo.org] 
Sent: Sunday, January 15, 2012 12:12 PM
To: Simon Glass
Cc: u-boot at lists.denx.de; Jim Lin; Tom Warren; Scott Wood
Subject: Re: [U-Boot] [PATCH 5/6] tegra: nand: Add Tegra NAND driver

* PGP Signed by an unknown key

On Saturday 14 January 2012 23:08:45 Simon Glass wrote:
> On Sat, Jan 14, 2012 at 8:03 PM, Mike Frysinger wrote:
> > On Friday 13 January 2012 18:10:55 Simon Glass wrote:
> >> From: Jim Lin <jilin@nvidia.com>
> >> 
> >> A device tree is used to configure the NAND, including memory
> >> timings and block/pages sizes.
> >> 
> >> If this node is not present or is disabled, then NAND will not
> >> be initialized.
> >> 
> >> Signed-off-by: Simon Glass <sjg@chromium.org>
> > 
> > the Author needs to sign off too ...
> 
> I have a lot like that. It doesn't feel right for me to just add their
> sign-off after all my changes. I was hoping that they would reply to
> the list with it. Does that work? The original commit does not have a
> sign-off.

sure, if the author replies with their s-o-b, or says they're OK with you 
adding it, that's fine.  but you're right that you can't just add it yourself 
without them saying so first ...
-mike

* Unknown Key
* 0xE837F581
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

[U-Boot] [PATCH 5/6] tegra: nand: Add Tegra NAND driver

[Stephen Warren]

On 01/13/2012 04:10 PM, Simon Glass wrote:
> From: Jim Lin <jilin@nvidia.com>
> 
> A device tree is used to configure the NAND, including memory
> timings and block/pages sizes.
> 
> If this node is not present or is disabled, then NAND will not
> be initialized.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

> diff --git a/drivers/mtd/nand/tegra2_nand.c b/drivers/mtd/nand/tegra2_nand.c

> +/**
> + * Wait for command completion
> + *
> + * @param reg  nand_ctlr structure
> + * @return
> + *     1 - Command completed
> + *     0 - Timeout
> + */
> +static int nand_waitfor_cmd_completion(struct nand_ctlr *reg)
> +{
> +       int i;
> +       u32 reg_val;
> +
> +       for (i = 0; i < NAND_CMD_TIMEOUT_MS * 1000; i++) {
> +               if ((readl(&reg->command) & CMD_GO) ||
> +                       !(readl(&reg->status) &
> +                               STATUS_RBSY0) ||
> +                       !(readl(&reg->isr) &
> +                               ISR_IS_CMD_DONE)) {
> +                       udelay(1);
> +                       continue;
> +               }
> +               reg_val = readl(&reg->dma_mst_ctrl);
> +               /*
> +                * If DMA_MST_CTRL_EN_A_ENABLE or
> +                * DMA_MST_CTRL_EN_B_ENABLE is set,
> +                * that means DMA engine is running, then we
> +                * have to wait until
> +                * DMA_MST_CTRL_IS_DMA_DONE
> +                * is cleared for DMA transfer completion.
> +                */
> +               if (reg_val & (DMA_MST_CTRL_EN_A_ENABLE |
> +                       DMA_MST_CTRL_EN_B_ENABLE)) {
> +                       if (reg_val & DMA_MST_CTRL_IS_DMA_DONE)
> +                               return 1;
> +               } else
> +                       return 1;
> +               udelay(1);


To be more consistent with the first if/continue block, wouldn't it be
better to recast that last if test and udelay as:

               if (reg_val & (DMA_MST_CTRL_EN_A_ENABLE |
                       DMA_MST_CTRL_EN_B_ENABLE)) {
                       if (!(reg_val & DMA_MST_CTRL_IS_DMA_DONE)) {
                               udelay(1);
                               continue;
                       }
               }

               break;


> +/**
> + * [DEFAULT] Send command to NAND device
> + *
> + * @param mtd          MTD device structure
> + * @param command      the command to be sent
> + * @param column       the column address for this command, -1 if none
> + * @param page_addr    the page address for this command, -1 if none
> + */
> +static void nand_command(struct mtd_info *mtd, unsigned int command,
> +       int column, int page_addr)
> +{
> +       register struct nand_chip *chip = mtd->priv;
> +       struct nand_info *info;
> +
> +       info = (struct nand_info *) chip->priv;
> +
> +       /*
> +        * Write out the command to the device.
> +        */
> +       if (mtd->writesize < 2048) {
> +               /*
> +                * Only command NAND_CMD_RESET or NAND_CMD_READID will come
> +                * here before mtd->writesize is initialized, we don't have
> +                * any action here because page size of NAND HY27UF084G2B
> +                * is 2048 bytes and mtd->writesize will be 2048 after
> +                * initialized.
> +                */

What if the NAND flash doesn't have a page size of 2048 bytes? The
driver shouldn't make such assumptions about the flash chip that happens
to be connected. Should the if above be:

    if (mtd->writesize == 0)

to be generic?

Should this if branch validate that the command being executed is a
legitimate command for the not-yet-fully-initialized case?

> +/**
> + * Set up NAND bus width and page size
> + *
> + * @param info         nand_info structure
> + * @param *reg_val     address of reg_val
> + * @return none - value is set in reg_val
> + */
> +static void set_bus_width_page_size(struct fdt_nand *config,
> +       u32 *reg_val)
> +{
> +       if (config->width == 8)
> +               *reg_val = CFG_BUS_WIDTH_8BIT;
> +       else

Shouldn't that be else if (config->width == 16)

> +               *reg_val = CFG_BUS_WIDTH_16BIT;


... and there be an else clause that returns an error?

> +
> +       if (config->page_data_bytes == 256)
> +               *reg_val |= CFG_PAGE_SIZE_256;
> +       else if (config->page_data_bytes == 512)
> +               *reg_val |= CFG_PAGE_SIZE_512;
> +       else if (config->page_data_bytes == 1024)
> +               *reg_val |= CFG_PAGE_SIZE_1024;
> +       else if (config->page_data_bytes == 2048)
> +               *reg_val |= CFG_PAGE_SIZE_2048;

And similarly, and else clause that returns an error here?

-- 
nvpublic

[U-Boot] [PATCH 5/6] tegra: nand: Add Tegra NAND driver

[Simon Glass]

Hi Stephen,

On Fri, Jan 20, 2012 at 9:31 AM, Stephen Warren <swarren@nvidia.com> wrote:
> On 01/13/2012 04:10 PM, Simon Glass wrote:
>> From: Jim Lin <jilin@nvidia.com>
>>
>> A device tree is used to configure the NAND, including memory
>> timings and block/pages sizes.
>>
>> If this node is not present or is disabled, then NAND will not
>> be initialized.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>
>> diff --git a/drivers/mtd/nand/tegra2_nand.c b/drivers/mtd/nand/tegra2_nand.c
>
>> +/**
>> + * Wait for command completion
>> + *
>> + * @param reg ?nand_ctlr structure
>> + * @return
>> + * ? ? 1 - Command completed
>> + * ? ? 0 - Timeout
>> + */
>> +static int nand_waitfor_cmd_completion(struct nand_ctlr *reg)
>> +{
>> + ? ? ? int i;
>> + ? ? ? u32 reg_val;
>> +
>> + ? ? ? for (i = 0; i < NAND_CMD_TIMEOUT_MS * 1000; i++) {
>> + ? ? ? ? ? ? ? if ((readl(&reg->command) & CMD_GO) ||
>> + ? ? ? ? ? ? ? ? ? ? ? !(readl(&reg->status) &
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? STATUS_RBSY0) ||
>> + ? ? ? ? ? ? ? ? ? ? ? !(readl(&reg->isr) &
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ISR_IS_CMD_DONE)) {
>> + ? ? ? ? ? ? ? ? ? ? ? udelay(1);
>> + ? ? ? ? ? ? ? ? ? ? ? continue;
>> + ? ? ? ? ? ? ? }
>> + ? ? ? ? ? ? ? reg_val = readl(&reg->dma_mst_ctrl);
>> + ? ? ? ? ? ? ? /*
>> + ? ? ? ? ? ? ? ?* If DMA_MST_CTRL_EN_A_ENABLE or
>> + ? ? ? ? ? ? ? ?* DMA_MST_CTRL_EN_B_ENABLE is set,
>> + ? ? ? ? ? ? ? ?* that means DMA engine is running, then we
>> + ? ? ? ? ? ? ? ?* have to wait until
>> + ? ? ? ? ? ? ? ?* DMA_MST_CTRL_IS_DMA_DONE
>> + ? ? ? ? ? ? ? ?* is cleared for DMA transfer completion.
>> + ? ? ? ? ? ? ? ?*/
>> + ? ? ? ? ? ? ? if (reg_val & (DMA_MST_CTRL_EN_A_ENABLE |
>> + ? ? ? ? ? ? ? ? ? ? ? DMA_MST_CTRL_EN_B_ENABLE)) {
>> + ? ? ? ? ? ? ? ? ? ? ? if (reg_val & DMA_MST_CTRL_IS_DMA_DONE)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? return 1;
>> + ? ? ? ? ? ? ? } else
>> + ? ? ? ? ? ? ? ? ? ? ? return 1;
>> + ? ? ? ? ? ? ? udelay(1);
>
>
> To be more consistent with the first if/continue block, wouldn't it be
> better to recast that last if test and udelay as:
>
> ? ? ? ? ? ? ? if (reg_val & (DMA_MST_CTRL_EN_A_ENABLE |
> ? ? ? ? ? ? ? ? ? ? ? DMA_MST_CTRL_EN_B_ENABLE)) {
> ? ? ? ? ? ? ? ? ? ? ? if (!(reg_val & DMA_MST_CTRL_IS_DMA_DONE)) {
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? udelay(1);
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? continue;
> ? ? ? ? ? ? ? ? ? ? ? }
> ? ? ? ? ? ? ? }
>
> ? ? ? ? ? ? ? break;

Yes that is better - but I've split it up a bit as it is too ugly.
Also I do want to actually make use of the udelay that is there,
rather than breaking out of the loop by default.

>
>
>> +/**
>> + * [DEFAULT] Send command to NAND device
>> + *
>> + * @param mtd ? ? ? ? ?MTD device structure
>> + * @param command ? ? ?the command to be sent
>> + * @param column ? ? ? the column address for this command, -1 if none
>> + * @param page_addr ? ?the page address for this command, -1 if none
>> + */
>> +static void nand_command(struct mtd_info *mtd, unsigned int command,
>> + ? ? ? int column, int page_addr)
>> +{
>> + ? ? ? register struct nand_chip *chip = mtd->priv;
>> + ? ? ? struct nand_info *info;
>> +
>> + ? ? ? info = (struct nand_info *) chip->priv;
>> +
>> + ? ? ? /*
>> + ? ? ? ?* Write out the command to the device.
>> + ? ? ? ?*/
>> + ? ? ? if (mtd->writesize < 2048) {
>> + ? ? ? ? ? ? ? /*
>> + ? ? ? ? ? ? ? ?* Only command NAND_CMD_RESET or NAND_CMD_READID will come
>> + ? ? ? ? ? ? ? ?* here before mtd->writesize is initialized, we don't have
>> + ? ? ? ? ? ? ? ?* any action here because page size of NAND HY27UF084G2B
>> + ? ? ? ? ? ? ? ?* is 2048 bytes and mtd->writesize will be 2048 after
>> + ? ? ? ? ? ? ? ?* initialized.
>> + ? ? ? ? ? ? ? ?*/
>
> What if the NAND flash doesn't have a page size of 2048 bytes? The
> driver shouldn't make such assumptions about the flash chip that happens
> to be connected. Should the if above be:
>
> ? ?if (mtd->writesize == 0)
>
> to be generic?

Well I have just removed this, as Scott suggested.

>
> Should this if branch validate that the command being executed is a
> legitimate command for the not-yet-fully-initialized case?

Removed

>
>> +/**
>> + * Set up NAND bus width and page size
>> + *
>> + * @param info ? ? ? ? nand_info structure
>> + * @param *reg_val ? ? address of reg_val
>> + * @return none - value is set in reg_val
>> + */
>> +static void set_bus_width_page_size(struct fdt_nand *config,
>> + ? ? ? u32 *reg_val)
>> +{
>> + ? ? ? if (config->width == 8)
>> + ? ? ? ? ? ? ? *reg_val = CFG_BUS_WIDTH_8BIT;
>> + ? ? ? else
>
> Shouldn't that be else if (config->width == 16)
>
>> + ? ? ? ? ? ? ? *reg_val = CFG_BUS_WIDTH_16BIT;
>
>
> ... and there be an else clause that returns an error?

Done, have also called it from init as Scott suggests.

>
>> +
>> + ? ? ? if (config->page_data_bytes == 256)
>> + ? ? ? ? ? ? ? *reg_val |= CFG_PAGE_SIZE_256;
>> + ? ? ? else if (config->page_data_bytes == 512)
>> + ? ? ? ? ? ? ? *reg_val |= CFG_PAGE_SIZE_512;
>> + ? ? ? else if (config->page_data_bytes == 1024)
>> + ? ? ? ? ? ? ? *reg_val |= CFG_PAGE_SIZE_1024;
>> + ? ? ? else if (config->page_data_bytes == 2048)
>> + ? ? ? ? ? ? ? *reg_val |= CFG_PAGE_SIZE_2048;
>
> And similarly, and else clause that returns an error here?

Done

>
> --
> nvpublic

Regards,
Simon

[U-Boot] [PATCH 5/6] tegra: nand: Add Tegra NAND driver

[Scott Wood]

On 01/13/2012 05:10 PM, Simon Glass wrote:
> +/* Information about an attached NAND chip */
> +struct fdt_nand {
> +	struct nand_ctlr *reg;
> +	int enabled;		/* 1 to enable, 0 to disable */
> +	struct fdt_gpio_state wp_gpio;	/* write-protect GPIO */
> +	int width;		/* bit width, normally 8 */
> +	int tag_ecc_bytes;	/* ECC bytes to be generated for tag bytes */
> +	int tag_bytes;		/* Tag bytes in spare area */
> +	int data_ecc_bytes;	/* ECC bytes for data area */
> +	int skipped_spare_bytes;
> +	/*
> +	 * How many bytes in spare area:
> +	 * spare area = skipped bytes + ECC bytes of data area
> +	 * + tag bytes + ECC bytes of tag bytes
> +	 */
> +	int page_spare_bytes;
> +	int page_data_bytes;	/* Bytes in data area */
> +	unsigned timing[FDT_NAND_TIMING_COUNT];

s/unsigned/u32/g

Likewise, any of these other fields that correspond to device tree
fields should be u32 or s32.

...and even when you do mean "unsigned int", please spell it out.

> +struct nand_info {

Please do not define "struct nand_info" when there is already a
different "nand_info_t".

> +struct nand_info nand_ctrl;

static (or better, dynamic).

> +/**
> + * Wait for command completion
> + *
> + * @param reg	nand_ctlr structure
> + * @return
> + *	1 - Command completed
> + *	0 - Timeout
> + */
> +static int nand_waitfor_cmd_completion(struct nand_ctlr *reg)
> +{
> +	int i;
> +	u32 reg_val;
> +
> +	for (i = 0; i < NAND_CMD_TIMEOUT_MS * 1000; i++) {
> +		if ((readl(&reg->command) & CMD_GO) ||
> +			!(readl(&reg->status) &
> +				STATUS_RBSY0) ||
> +			!(readl(&reg->isr) &
> +				ISR_IS_CMD_DONE)) {
> +			udelay(1);
> +			continue;
> +		}
> +		reg_val = readl(&reg->dma_mst_ctrl);
> +		/*
> +		 * If DMA_MST_CTRL_EN_A_ENABLE or
> +		 * DMA_MST_CTRL_EN_B_ENABLE is set,
> +		 * that means DMA engine is running, then we
> +		 * have to wait until
> +		 * DMA_MST_CTRL_IS_DMA_DONE
> +		 * is cleared for DMA transfer completion.
> +		 */
> +		if (reg_val & (DMA_MST_CTRL_EN_A_ENABLE |
> +			DMA_MST_CTRL_EN_B_ENABLE)) {
> +			if (reg_val & DMA_MST_CTRL_IS_DMA_DONE)
> +				return 1;

Please don't line up continuation lines with the if-body.

E.g. either line up DMA_MST_CTRL_EN_B_ENABLE with
DMA_MST_CTRL_EN_A_ENABLE (my preference), or just add another tab (if
you want to be a tabs-only purist).

> +		} else
> +			return 1;

If braces are used on one side of the if/else, use on both sides.

> +		udelay(1);
> +	}
> +	return 0;
> +}
> +
> +/**
> + * [DEFAULT] Read one byte from the chip
> + *
> + * @param mtd	MTD device structure
> + * @return	data byte
> + *
> + * Default read function for 8bit bus-width
> + */
> +static uint8_t read_byte(struct mtd_info *mtd)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +	int dword_read;

s/int/u32/

> +	struct nand_info *info;
> +
> +	info = (struct nand_info *) chip->priv;
> +
> +	dword_read = readl(&info->reg->resp);
> +	dword_read = dword_read >> (8 * info->pio_byte_index);
> +	info->pio_byte_index++;
> +	return (uint8_t) dword_read;

No space after casts.

What happens when pio_byte_index > 3?  Please don't assume that upper
bits will be ignored, even if that happens to be true on this chip.

How does info->reg->resp work?  You don't seem to be choosing the dword
to read based on pio_byte_index, which suggests that the act of reading
resp changes what will be read the next time.  But, you read it on each
byte, which would advance resp four times too fast if it's simply
returning a new dword each time.

If the hardware is really auto-advancing resp only on every fourth
access, that needs a comment.

> +/* Hardware specific access to control-lines */
> +static void nand_hwcontrol(struct mtd_info *mtd, int cmd,
> +	unsigned int ctrl)
> +{
> +}

Don't implement this at all if it doesn't make sense for this hardware.

> +
> +/**
> + * Clear all interrupt status bits
> + *
> + * @param reg	nand_ctlr structure
> + */
> +static void nand_clear_interrupt_status(struct nand_ctlr *reg)
> +{
> +	u32 reg_val;
> +
> +	/* Clear interrupt status */
> +	reg_val = readl(&reg->isr);
> +	writel(reg_val, &reg->isr);
> +}
> +
> +/**
> + * [DEFAULT] Send command to NAND device
> + *
> + * @param mtd		MTD device structure
> + * @param command	the command to be sent
> + * @param column	the column address for this command, -1 if none
> + * @param page_addr	the page address for this command, -1 if none
> + */
> +static void nand_command(struct mtd_info *mtd, unsigned int command,
> +	int column, int page_addr)
> +{
> +	register struct nand_chip *chip = mtd->priv;

Are you really seeing any difference by using the register keyword?

> +	struct nand_info *info;
> +
> +	info = (struct nand_info *) chip->priv;
> +
> +	/*
> +	 * Write out the command to the device.
> +	 */
> +	if (mtd->writesize < 2048) {
> +		/*
> +		 * Only command NAND_CMD_RESET or NAND_CMD_READID will come
> +		 * here before mtd->writesize is initialized, we don't have
> +		 * any action here because page size of NAND HY27UF084G2B
> +		 * is 2048 bytes and mtd->writesize will be 2048 after
> +		 * initialized.
> +		 */

Why are you assuming a particular NAND chip here?

If you want to not support certain page sizes in this driver, fine, but
document that somewhere more prominent, and I don't see why this test is
needed.

> +	} else {
> +		/* Emulate NAND_CMD_READOOB */
> +		if (command == NAND_CMD_READOOB) {
> +			column += mtd->writesize;
> +			command = NAND_CMD_READ0;
> +		}
> +
> +		/* Adjust columns for 16 bit bus-width */
> +		if (column != -1 && (chip->options & NAND_BUSWIDTH_16))
> +			column >>= 1;
> +	}

Why does the treatment of column for NAND_CMD_READID depend on whether
you've yet filled in mtd->writesize?

> +	nand_clear_interrupt_status(info->reg);
> +
> +	/* Stop DMA engine, clear DMA completion status */
> +	writel(DMA_MST_CTRL_EN_A_DISABLE
> +		| DMA_MST_CTRL_EN_B_DISABLE
> +		| DMA_MST_CTRL_IS_DMA_DONE,
> +		&info->reg->dma_mst_ctrl);
> +
> +	/*
> +	 * Program and erase have their own busy handlers
> +	 * status and sequential in needs no delay
> +	 */
> +	switch (command) {
> +	case NAND_CMD_READID:
> +		writel(NAND_CMD_READID, &info->reg->cmd_reg1);
> +		writel(CMD_GO | CMD_CLE | CMD_ALE | CMD_PIO
> +			| CMD_RX |
> +			(CMD_TRANS_SIZE_BYTES4 << CMD_TRANS_SIZE_SHIFT)
> +			| CMD_CE0,
> +			&info->reg->command);
> +		info->pio_byte_index = 0;
> +		break;

Looks like you don't use column at all for READID -- no ONFI support, I
guess.

> +	case NAND_CMD_READ0:
> +		writel(NAND_CMD_READ0, &info->reg->cmd_reg1);
> +		writel(NAND_CMD_READSTART, &info->reg->cmd_reg2);
> +		writel((page_addr << 16) | (column & 0xFFFF),
> +			&info->reg->addr_reg1);
> +		writel(page_addr >> 16, &info->reg->addr_reg2);
> +		return;
> +	case NAND_CMD_SEQIN:
> +		writel(NAND_CMD_SEQIN, &info->reg->cmd_reg1);
> +		writel(NAND_CMD_PAGEPROG, &info->reg->cmd_reg2);
> +		writel((page_addr << 16) | (column & 0xFFFF),
> +			&info->reg->addr_reg1);
> +		writel(page_addr >> 16,
> +			&info->reg->addr_reg2);
> +		return;
> +	case NAND_CMD_PAGEPROG:
> +		return;
> +	case NAND_CMD_ERASE1:
> +		writel(NAND_CMD_ERASE1, &info->reg->cmd_reg1);
> +		writel(NAND_CMD_ERASE2, &info->reg->cmd_reg2);
> +		writel(page_addr, &info->reg->addr_reg1);
> +		writel(CMD_GO | CMD_CLE | CMD_ALE |
> +			CMD_SEC_CMD | CMD_CE0 | CMD_ALE_BYTES3,
> +			&info->reg->command);
> +		break;
> +	case NAND_CMD_RNDOUT:
> +		return;

Don't silently ignore RNDOUT -- if you don't support it and it happens,
complain loudly.

> +	case NAND_CMD_ERASE2:
> +		return;
> +	case NAND_CMD_STATUS:
> +		writel(NAND_CMD_STATUS, &info->reg->cmd_reg1);
> +		writel(CMD_GO | CMD_CLE | CMD_PIO | CMD_RX
> +			| (CMD_TRANS_SIZE_BYTES1 <<
> +				CMD_TRANS_SIZE_SHIFT)
> +			| CMD_CE0,
> +			&info->reg->command);
> +		info->pio_byte_index = 0;
> +		break;
> +	case NAND_CMD_RESET:
> +		writel(NAND_CMD_RESET, &info->reg->cmd_reg1);
> +		writel(CMD_GO | CMD_CLE | CMD_CE0,
> +			&info->reg->command);
> +		break;
> +	default:
> +		return;

Likewise, complain if you get an unrecognized command.

> +/**
> + * Set up NAND bus width and page size
> + *
> + * @param info		nand_info structure
> + * @param *reg_val	address of reg_val
> + * @return none - value is set in reg_val
> + */
> +static void set_bus_width_page_size(struct fdt_nand *config,
> +	u32 *reg_val)
> +{
> +	if (config->width == 8)
> +		*reg_val = CFG_BUS_WIDTH_8BIT;
> +	else
> +		*reg_val = CFG_BUS_WIDTH_16BIT;
> +
> +	if (config->page_data_bytes == 256)
> +		*reg_val |= CFG_PAGE_SIZE_256;
> +	else if (config->page_data_bytes == 512)
> +		*reg_val |= CFG_PAGE_SIZE_512;
> +	else if (config->page_data_bytes == 1024)
> +		*reg_val |= CFG_PAGE_SIZE_1024;
> +	else if (config->page_data_bytes == 2048)
> +		*reg_val |= CFG_PAGE_SIZE_2048;

Really, page sizes of 256 and 1024 bytes?

[U-Boot] [PATCH 5/6] tegra: nand: Add Tegra NAND driver

[Simon Glass]

Hi Scott,

On Fri, Jan 20, 2012 at 2:55 PM, Scott Wood <scottwood@freescale.com> wrote:
> On 01/13/2012 05:10 PM, Simon Glass wrote:
>> +/* Information about an attached NAND chip */
>> +struct fdt_nand {
>> + ? ? struct nand_ctlr *reg;
>> + ? ? int enabled; ? ? ? ? ? ?/* 1 to enable, 0 to disable */
>> + ? ? struct fdt_gpio_state wp_gpio; ?/* write-protect GPIO */
>> + ? ? int width; ? ? ? ? ? ? ?/* bit width, normally 8 */
>> + ? ? int tag_ecc_bytes; ? ? ?/* ECC bytes to be generated for tag bytes */
>> + ? ? int tag_bytes; ? ? ? ? ?/* Tag bytes in spare area */
>> + ? ? int data_ecc_bytes; ? ? /* ECC bytes for data area */
>> + ? ? int skipped_spare_bytes;
>> + ? ? /*
>> + ? ? ?* How many bytes in spare area:
>> + ? ? ?* spare area = skipped bytes + ECC bytes of data area
>> + ? ? ?* + tag bytes + ECC bytes of tag bytes
>> + ? ? ?*/
>> + ? ? int page_spare_bytes;
>> + ? ? int page_data_bytes; ? ?/* Bytes in data area */
>> + ? ? unsigned timing[FDT_NAND_TIMING_COUNT];
>
> s/unsigned/u32/g

Done

>
> Likewise, any of these other fields that correspond to device tree
> fields should be u32 or s32.

Done

>
> ...and even when you do mean "unsigned int", please spell it out.
>
>> +struct nand_info {
>
> Please do not define "struct nand_info" when there is already a
> different "nand_info_t".

Done

>
>> +struct nand_info nand_ctrl;
>
> static (or better, dynamic).

Done, what is dynamic?

>
>> +/**
>> + * Wait for command completion
>> + *
>> + * @param reg ? ? ? ?nand_ctlr structure
>> + * @return
>> + * ? 1 - Command completed
>> + * ? 0 - Timeout
>> + */
>> +static int nand_waitfor_cmd_completion(struct nand_ctlr *reg)
>> +{
>> + ? ? int i;
>> + ? ? u32 reg_val;
>> +
>> + ? ? for (i = 0; i < NAND_CMD_TIMEOUT_MS * 1000; i++) {
>> + ? ? ? ? ? ? if ((readl(&reg->command) & CMD_GO) ||
>> + ? ? ? ? ? ? ? ? ? ? !(readl(&reg->status) &
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? STATUS_RBSY0) ||
>> + ? ? ? ? ? ? ? ? ? ? !(readl(&reg->isr) &
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ISR_IS_CMD_DONE)) {
>> + ? ? ? ? ? ? ? ? ? ? udelay(1);
>> + ? ? ? ? ? ? ? ? ? ? continue;
>> + ? ? ? ? ? ? }
>> + ? ? ? ? ? ? reg_val = readl(&reg->dma_mst_ctrl);
>> + ? ? ? ? ? ? /*
>> + ? ? ? ? ? ? ?* If DMA_MST_CTRL_EN_A_ENABLE or
>> + ? ? ? ? ? ? ?* DMA_MST_CTRL_EN_B_ENABLE is set,
>> + ? ? ? ? ? ? ?* that means DMA engine is running, then we
>> + ? ? ? ? ? ? ?* have to wait until
>> + ? ? ? ? ? ? ?* DMA_MST_CTRL_IS_DMA_DONE
>> + ? ? ? ? ? ? ?* is cleared for DMA transfer completion.
>> + ? ? ? ? ? ? ?*/
>> + ? ? ? ? ? ? if (reg_val & (DMA_MST_CTRL_EN_A_ENABLE |
>> + ? ? ? ? ? ? ? ? ? ? DMA_MST_CTRL_EN_B_ENABLE)) {
>> + ? ? ? ? ? ? ? ? ? ? if (reg_val & DMA_MST_CTRL_IS_DMA_DONE)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? return 1;
>
> Please don't line up continuation lines with the if-body.
>
> E.g. either line up DMA_MST_CTRL_EN_B_ENABLE with
> DMA_MST_CTRL_EN_A_ENABLE (my preference), or just add another tab (if
> you want to be a tabs-only purist).

Yes, ick, have fixed that.

>
>> + ? ? ? ? ? ? } else
>> + ? ? ? ? ? ? ? ? ? ? return 1;
>
> If braces are used on one side of the if/else, use on both sides.

Done

>
>> + ? ? ? ? ? ? udelay(1);
>> + ? ? }
>> + ? ? return 0;
>> +}
>> +
>> +/**
>> + * [DEFAULT] Read one byte from the chip
>> + *
>> + * @param mtd ? ? ? ?MTD device structure
>> + * @return ? data byte
>> + *
>> + * Default read function for 8bit bus-width
>> + */
>> +static uint8_t read_byte(struct mtd_info *mtd)
>> +{
>> + ? ? struct nand_chip *chip = mtd->priv;
>> + ? ? int dword_read;
>
> s/int/u32/

done

>
>> + ? ? struct nand_info *info;
>> +
>> + ? ? info = (struct nand_info *) chip->priv;
>> +
>> + ? ? dword_read = readl(&info->reg->resp);
>> + ? ? dword_read = dword_read >> (8 * info->pio_byte_index);
>> + ? ? info->pio_byte_index++;
>> + ? ? return (uint8_t) dword_read;
>
> No space after casts.

I think I have fixed that globally.

>
> What happens when pio_byte_index > 3? ?Please don't assume that upper
> bits will be ignored, even if that happens to be true on this chip.

I added an assert() - there is no way to return an error here. I'm not
sure what you mean by upper bits - there is a uint8_t cast.

>
> How does info->reg->resp work? ?You don't seem to be choosing the dword
> to read based on pio_byte_index, which suggests that the act of reading
> resp changes what will be read the next time. ?But, you read it on each
> byte, which would advance resp four times too fast if it's simply
> returning a new dword each time.
>
> If the hardware is really auto-advancing resp only on every fourth
> access, that needs a comment.

I was just looking at that. Have added a comment.

>
>> +/* Hardware specific access to control-lines */
>> +static void nand_hwcontrol(struct mtd_info *mtd, int cmd,
>> + ? ? unsigned int ctrl)
>> +{
>> +}
>
> Don't implement this at all if it doesn't make sense for this hardware.

It isn't implemented (the function is empty), but if it is not defined
then the NAND layer will die when it calls a null function. What
should I do here?

>
>> +
>> +/**
>> + * Clear all interrupt status bits
>> + *
>> + * @param reg ? ? ? ?nand_ctlr structure
>> + */
>> +static void nand_clear_interrupt_status(struct nand_ctlr *reg)
>> +{
>> + ? ? u32 reg_val;
>> +
>> + ? ? /* Clear interrupt status */
>> + ? ? reg_val = readl(&reg->isr);
>> + ? ? writel(reg_val, &reg->isr);
>> +}
>> +
>> +/**
>> + * [DEFAULT] Send command to NAND device
>> + *
>> + * @param mtd ? ? ? ? ? ? ? ?MTD device structure
>> + * @param command ? ?the command to be sent
>> + * @param column ? ? the column address for this command, -1 if none
>> + * @param page_addr ?the page address for this command, -1 if none
>> + */
>> +static void nand_command(struct mtd_info *mtd, unsigned int command,
>> + ? ? int column, int page_addr)
>> +{
>> + ? ? register struct nand_chip *chip = mtd->priv;
>
> Are you really seeing any difference by using the register keyword?

I doubt it, will remove globally.

>
>> + ? ? struct nand_info *info;
>> +
>> + ? ? info = (struct nand_info *) chip->priv;
>> +
>> + ? ? /*
>> + ? ? ?* Write out the command to the device.
>> + ? ? ?*/
>> + ? ? if (mtd->writesize < 2048) {
>> + ? ? ? ? ? ? /*
>> + ? ? ? ? ? ? ?* Only command NAND_CMD_RESET or NAND_CMD_READID will come
>> + ? ? ? ? ? ? ?* here before mtd->writesize is initialized, we don't have
>> + ? ? ? ? ? ? ?* any action here because page size of NAND HY27UF084G2B
>> + ? ? ? ? ? ? ?* is 2048 bytes and mtd->writesize will be 2048 after
>> + ? ? ? ? ? ? ?* initialized.
>> + ? ? ? ? ? ? ?*/
>
> Why are you assuming a particular NAND chip here?
>
> If you want to not support certain page sizes in this driver, fine, but
> document that somewhere more prominent, and I don't see why this test is
> needed.

Yes I agree, will punt it.

>
>> + ? ? } else {
>> + ? ? ? ? ? ? /* Emulate NAND_CMD_READOOB */
>> + ? ? ? ? ? ? if (command == NAND_CMD_READOOB) {
>> + ? ? ? ? ? ? ? ? ? ? column += mtd->writesize;
>> + ? ? ? ? ? ? ? ? ? ? command = NAND_CMD_READ0;
>> + ? ? ? ? ? ? }
>> +
>> + ? ? ? ? ? ? /* Adjust columns for 16 bit bus-width */
>> + ? ? ? ? ? ? if (column != -1 && (chip->options & NAND_BUSWIDTH_16))
>> + ? ? ? ? ? ? ? ? ? ? column >>= 1;
>> + ? ? }
>
> Why does the treatment of column for NAND_CMD_READID depend on whether
> you've yet filled in mtd->writesize?

Not sure, punted.

>
>> + ? ? nand_clear_interrupt_status(info->reg);
>> +
>> + ? ? /* Stop DMA engine, clear DMA completion status */
>> + ? ? writel(DMA_MST_CTRL_EN_A_DISABLE
>> + ? ? ? ? ? ? | DMA_MST_CTRL_EN_B_DISABLE
>> + ? ? ? ? ? ? | DMA_MST_CTRL_IS_DMA_DONE,
>> + ? ? ? ? ? ? &info->reg->dma_mst_ctrl);
>> +
>> + ? ? /*
>> + ? ? ?* Program and erase have their own busy handlers
>> + ? ? ?* status and sequential in needs no delay
>> + ? ? ?*/
>> + ? ? switch (command) {
>> + ? ? case NAND_CMD_READID:
>> + ? ? ? ? ? ? writel(NAND_CMD_READID, &info->reg->cmd_reg1);
>> + ? ? ? ? ? ? writel(CMD_GO | CMD_CLE | CMD_ALE | CMD_PIO
>> + ? ? ? ? ? ? ? ? ? ? | CMD_RX |
>> + ? ? ? ? ? ? ? ? ? ? (CMD_TRANS_SIZE_BYTES4 << CMD_TRANS_SIZE_SHIFT)
>> + ? ? ? ? ? ? ? ? ? ? | CMD_CE0,
>> + ? ? ? ? ? ? ? ? ? ? &info->reg->command);
>> + ? ? ? ? ? ? info->pio_byte_index = 0;
>> + ? ? ? ? ? ? break;
>
> Looks like you don't use column at all for READID -- no ONFI support, I
> guess.

Yes.

>
>> + ? ? case NAND_CMD_READ0:
>> + ? ? ? ? ? ? writel(NAND_CMD_READ0, &info->reg->cmd_reg1);
>> + ? ? ? ? ? ? writel(NAND_CMD_READSTART, &info->reg->cmd_reg2);
>> + ? ? ? ? ? ? writel((page_addr << 16) | (column & 0xFFFF),
>> + ? ? ? ? ? ? ? ? ? ? &info->reg->addr_reg1);
>> + ? ? ? ? ? ? writel(page_addr >> 16, &info->reg->addr_reg2);
>> + ? ? ? ? ? ? return;
>> + ? ? case NAND_CMD_SEQIN:
>> + ? ? ? ? ? ? writel(NAND_CMD_SEQIN, &info->reg->cmd_reg1);
>> + ? ? ? ? ? ? writel(NAND_CMD_PAGEPROG, &info->reg->cmd_reg2);
>> + ? ? ? ? ? ? writel((page_addr << 16) | (column & 0xFFFF),
>> + ? ? ? ? ? ? ? ? ? ? &info->reg->addr_reg1);
>> + ? ? ? ? ? ? writel(page_addr >> 16,
>> + ? ? ? ? ? ? ? ? ? ? &info->reg->addr_reg2);
>> + ? ? ? ? ? ? return;
>> + ? ? case NAND_CMD_PAGEPROG:
>> + ? ? ? ? ? ? return;
>> + ? ? case NAND_CMD_ERASE1:
>> + ? ? ? ? ? ? writel(NAND_CMD_ERASE1, &info->reg->cmd_reg1);
>> + ? ? ? ? ? ? writel(NAND_CMD_ERASE2, &info->reg->cmd_reg2);
>> + ? ? ? ? ? ? writel(page_addr, &info->reg->addr_reg1);
>> + ? ? ? ? ? ? writel(CMD_GO | CMD_CLE | CMD_ALE |
>> + ? ? ? ? ? ? ? ? ? ? CMD_SEC_CMD | CMD_CE0 | CMD_ALE_BYTES3,
>> + ? ? ? ? ? ? ? ? ? ? &info->reg->command);
>> + ? ? ? ? ? ? break;
>> + ? ? case NAND_CMD_RNDOUT:
>> + ? ? ? ? ? ? return;
>
> Don't silently ignore RNDOUT -- if you don't support it and it happens,
> complain loudly.

OK, I added a printf(), that should be loud enough. Would prefer a
debug() though.

>
>> + ? ? case NAND_CMD_ERASE2:
>> + ? ? ? ? ? ? return;
>> + ? ? case NAND_CMD_STATUS:
>> + ? ? ? ? ? ? writel(NAND_CMD_STATUS, &info->reg->cmd_reg1);
>> + ? ? ? ? ? ? writel(CMD_GO | CMD_CLE | CMD_PIO | CMD_RX
>> + ? ? ? ? ? ? ? ? ? ? | (CMD_TRANS_SIZE_BYTES1 <<
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? CMD_TRANS_SIZE_SHIFT)
>> + ? ? ? ? ? ? ? ? ? ? | CMD_CE0,
>> + ? ? ? ? ? ? ? ? ? ? &info->reg->command);
>> + ? ? ? ? ? ? info->pio_byte_index = 0;
>> + ? ? ? ? ? ? break;
>> + ? ? case NAND_CMD_RESET:
>> + ? ? ? ? ? ? writel(NAND_CMD_RESET, &info->reg->cmd_reg1);
>> + ? ? ? ? ? ? writel(CMD_GO | CMD_CLE | CMD_CE0,
>> + ? ? ? ? ? ? ? ? ? ? &info->reg->command);
>> + ? ? ? ? ? ? break;
>> + ? ? default:
>> + ? ? ? ? ? ? return;
>
> Likewise, complain if you get an unrecognized command.

Done - I wonder why we can't just return an error?

>
>> +/**
>> + * Set up NAND bus width and page size
>> + *
>> + * @param info ? ? ? ? ? ? ? nand_info structure
>> + * @param *reg_val ? address of reg_val
>> + * @return none - value is set in reg_val
>> + */
>> +static void set_bus_width_page_size(struct fdt_nand *config,
>> + ? ? u32 *reg_val)
>> +{
>> + ? ? if (config->width == 8)
>> + ? ? ? ? ? ? *reg_val = CFG_BUS_WIDTH_8BIT;
>> + ? ? else
>> + ? ? ? ? ? ? *reg_val = CFG_BUS_WIDTH_16BIT;
>> +
>> + ? ? if (config->page_data_bytes == 256)
>> + ? ? ? ? ? ? *reg_val |= CFG_PAGE_SIZE_256;
>> + ? ? else if (config->page_data_bytes == 512)
>> + ? ? ? ? ? ? *reg_val |= CFG_PAGE_SIZE_512;
>> + ? ? else if (config->page_data_bytes == 1024)
>> + ? ? ? ? ? ? *reg_val |= CFG_PAGE_SIZE_1024;
>> + ? ? else if (config->page_data_bytes == 2048)
>> + ? ? ? ? ? ? *reg_val |= CFG_PAGE_SIZE_2048;
>
> Really, page sizes of 256 and 1024 bytes?
>
> From elsewhere in the driver you only support 2048 byte pages, so why
> not just check for that and error (not silently continue) if it's
> anything else?

I don't think it is that bad - I believe the driver should all the
options, although I have not tested it or gone into it carefully. Have
added error checking.

>
>> +}
>> +
>> +/**
>> + * Page read/write function
>> + *
>> + * @param mtd ? ? ? ? ? ? ? ?mtd info structure
>> + * @param chip ? ? ? ? ? ? ? nand chip info structure
>> + * @param buf ? ? ? ? ? ? ? ?data buffer
>> + * @param page ? ? ? ? ? ? ? page number
>> + * @param with_ecc ? 1 to enable ECC, 0 to disable ECC
>> + * @param is_writing 0 for read, 1 for write
>> + * @return ? 0 when successfully completed
>> + * ? ? ? ? ? -EIO when command timeout
>> + */
>> +static int nand_rw_page(struct mtd_info *mtd, struct nand_chip *chip,
>> + ? ? uint8_t *buf, int page, int with_ecc, int is_writing)
>> +{
>> + ? ? u32 reg_val;
>> + ? ? int tag_size;
>> + ? ? struct nand_oobfree *free = chip->ecc.layout->oobfree;
>> + ? ? /* 128 is larger than the value that our HW can support. */
>> + ? ? u32 tag_buf[128];
>> + ? ? char *tag_ptr;
>> + ? ? struct nand_info *info;
>> + ? ? struct fdt_nand *config;
>> +
>> + ? ? if (((int) buf) & 0x03) {
>
> s/int/uintptr_t/ (or unsigned long)

done

>
>> + ? ? ? ? ? ? printf("buf 0x%X has to be 4-byte aligned\n", (u32) buf);
>
> %p

done

>
>> + ? ? set_bus_width_page_size(&info->config, &reg_val);
>
> You need to set up the bus width and page size on every page transfer?
> Why not figure out the value to write in the register at init time (thus
> making it a good place to reject unsupported configurations)?

Done, added a check to the init.

>
>> + ? ? /* Set ECC selection, configure ECC settings */
>> + ? ? if (with_ecc) {
>> + ? ? ? ? ? ? tag_size = config->tag_bytes + config->tag_ecc_bytes;
>> + ? ? ? ? ? ? reg_val |= (CFG_SKIP_SPARE_SEL_4
>> + ? ? ? ? ? ? ? ? ? ? | CFG_SKIP_SPARE_ENABLE
>> + ? ? ? ? ? ? ? ? ? ? | CFG_HW_ECC_CORRECTION_ENABLE
>> + ? ? ? ? ? ? ? ? ? ? | CFG_ECC_EN_TAG_DISABLE
>> + ? ? ? ? ? ? ? ? ? ? | CFG_HW_ECC_SEL_RS
>> + ? ? ? ? ? ? ? ? ? ? | CFG_HW_ECC_ENABLE
>> + ? ? ? ? ? ? ? ? ? ? | CFG_TVAL4
>> + ? ? ? ? ? ? ? ? ? ? | (tag_size - 1));
>> +
>> + ? ? ? ? ? ? if (!is_writing) {
>> + ? ? ? ? ? ? ? ? ? ? tag_size += config->skipped_spare_bytes;
>> + ? ? ? ? ? ? ? ? ? ? invalidate_dcache_range((unsigned long) tag_ptr,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ((unsigned long) tag_ptr) + tag_size);
>> + ? ? ? ? ? ? } else
>> + ? ? ? ? ? ? ? ? ? ? flush_dcache_range((unsigned long) tag_ptr,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ((unsigned long) tag_ptr) + tag_size);
>> + ? ? } else {
>> + ? ? ? ? ? ? tag_size = mtd->oobsize;
>> + ? ? ? ? ? ? reg_val |= (CFG_SKIP_SPARE_DISABLE
>> + ? ? ? ? ? ? ? ? ? ? | CFG_HW_ECC_CORRECTION_DISABLE
>> + ? ? ? ? ? ? ? ? ? ? | CFG_ECC_EN_TAG_DISABLE
>> + ? ? ? ? ? ? ? ? ? ? | CFG_HW_ECC_DISABLE
>> + ? ? ? ? ? ? ? ? ? ? | (tag_size - 1));
>> + ? ? ? ? ? ? if (!is_writing) {
>> + ? ? ? ? ? ? ? ? ? ? invalidate_dcache_range((unsigned long) chip->oob_poi,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ((unsigned long) chip->oob_poi) + tag_size);
>> + ? ? ? ? ? ? } else {
>> + ? ? ? ? ? ? ? ? ? ? flush_dcache_range((unsigned long) chip->oob_poi,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ((unsigned long) chip->oob_poi) + tag_size);
>> + ? ? ? ? ? ? }
>> + ? ? }
>> + ? ? writel(reg_val, &info->reg->config);
>> +
>> + ? ? if (!is_writing) {
>> + ? ? ? ? ? ? invalidate_dcache_range((unsigned long) buf,
>> + ? ? ? ? ? ? ? ? ? ? ((unsigned long) buf) +
>> + ? ? ? ? ? ? ? ? ? ? (1 << chip->page_shift));
>> + ? ? } else {
>> + ? ? ? ? ? ? flush_dcache_range((unsigned long) buf,
>> + ? ? ? ? ? ? ? ? ? ? ((unsigned long) buf) +
>> + ? ? ? ? ? ? ? ? ? ? (1 << chip->page_shift));
>> + ? ? }
>
> Please factor out all this cache stuff into a dma prepare function that
> takes start, length, and is_writing. ?Is it even really needed to
> invalidate rather than flush in the read case? ?It doesn't look like
> these buffers are even going to be cacheline-aligned...

Done

I am not keen on adding cache alignment into every driver - IMO this
should happen at the level above as with MMC, USB, etc. A fairly
trivial fix to nand_base caches most of the problems. I will include a
patch in my next series. Anyway for now, Tegra has cache off because
of all these warnings.

>
>> +int fdt_decode_nand(const void *blob, int node, struct fdt_nand *config)
>> +{
>> + ? ? int err;
>> +
>> + ? ? config->reg = (struct nand_ctlr *)fdtdec_get_addr(blob, node, "reg");
>> + ? ? config->enabled = fdtdec_get_is_enabled(blob, node);
>> + ? ? config->width = fdtdec_get_int(blob, node, "width", 8);
>> + ? ? err = fdtdec_decode_gpio(blob, node, "wp-gpios", &config->wp_gpio);
>> + ? ? if (err)
>> + ? ? ? ? ? ? return err;
>> + ? ? err = fdtdec_get_int_array(blob, node, "nvidia,timing",
>> + ? ? ? ? ? ? ? ? ? ? config->timing, FDT_NAND_TIMING_COUNT);
>> + ? ? if (err < 0)
>> + ? ? ? ? ? ? return err;
>> +
>> + ? ? /* Now look up the controller and decode that */
>> + ? ? node = fdt_next_node(blob, node, NULL);
>> + ? ? if (node < 0)
>> + ? ? ? ? ? ? return node;
>> +
>> + ? ? config->page_data_bytes = fdtdec_get_int(blob, node,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "page-data-bytes", -1);
>> + ? ? config->tag_ecc_bytes = fdtdec_get_int(blob, node,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "tag-ecc-bytes", -1);
>> + ? ? config->tag_bytes = fdtdec_get_int(blob, node, "tag-bytes", -1);
>> + ? ? config->data_ecc_bytes = fdtdec_get_int(blob, node,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "data-ecc-bytes", -1);
>> + ? ? config->skipped_spare_bytes = fdtdec_get_int(blob, node,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "skipped-spare-bytes", -1);
>> + ? ? config->page_spare_bytes = fdtdec_get_int(blob, node,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "page-spare-bytes", -1);
>
> Has this binding been accepted into Linux's documentation or another
> canonical source?

No

>
> Usually vendor prefixes are asked for on such properties (similar to
> "nvidia,timing").

Done

>
>> +/* Oh dear I suspect no one will like these Bit values? */
>
> Indeed.
>
>> +enum {
>> + ? ? Bit0 = 1 << 0,
>> + ? ? Bit1 = 1 << 1,
>> + ? ? Bit2 = 1 << 2,
>
> Please just open code this in the functional definitions.

Done

>
>> +enum {
>> + ? ? CMD_TRANS_SIZE_BYTES1 = 0,
>> + ? ? CMD_TRANS_SIZE_BYTES2,
>> + ? ? CMD_TRANS_SIZE_BYTES3,
>> + ? ? CMD_TRANS_SIZE_BYTES4,
>> + ? ? CMD_TRANS_SIZE_BYTES5,
>> + ? ? CMD_TRANS_SIZE_BYTES6,
>> + ? ? CMD_TRANS_SIZE_BYTES7,
>> + ? ? CMD_TRANS_SIZE_BYTES8,
>> + ? ? CMD_TRANS_SIZE_BYTES_PAGE_SIZE_SEL
>> +};
>
> Why not just use the number of bytes directly, minus one?

Done, except for the last one which makes sense as a define I think.

>
> -Scott
>

Regards,
Simon

[U-Boot] [PATCH 5/6] tegra: nand: Add Tegra NAND driver

[Scott Wood]

On 04/13/2012 12:42 PM, Simon Glass wrote:
> Hi Scott,
> 
> On Fri, Jan 20, 2012 at 2:55 PM, Scott Wood <scottwood@freescale.com> wrote:
>> On 01/13/2012 05:10 PM, Simon Glass wrote:
>>> +struct nand_info nand_ctrl;
>>
>> static (or better, dynamic).
> 
> Done, what is dynamic?

Allocate the structure dynamically in your init function, and write the
code in such a way as to allow multiple instances.  Not mandatory if you
really think you'll never have multiple instances, but would be nice and
shouldn't complicate things much.

>>> +     struct nand_info *info;
>>> +
>>> +     info = (struct nand_info *) chip->priv;
>>> +
>>> +     dword_read = readl(&info->reg->resp);
>>> +     dword_read = dword_read >> (8 * info->pio_byte_index);
>>> +     info->pio_byte_index++;
>>> +     return (uint8_t) dword_read;
>>
>> No space after casts.
> 
> I think I have fixed that globally.
> 
>>
>> What happens when pio_byte_index > 3?  Please don't assume that upper
>> bits will be ignored, even if that happens to be true on this chip.
> 
> I added an assert() - there is no way to return an error here. I'm not
> sure what you mean by upper bits - there is a uint8_t cast.

By upper bits I mean in the shift count.  Don't assume that
dword_read >> 32 is the same as dword_read >> 0.  It's undefined in C.

>> How does info->reg->resp work?  You don't seem to be choosing the dword
>> to read based on pio_byte_index, which suggests that the act of reading
>> resp changes what will be read the next time.  But, you read it on each
>> byte, which would advance resp four times too fast if it's simply
>> returning a new dword each time.
>>
>> If the hardware is really auto-advancing resp only on every fourth
>> access, that needs a comment.
> 
> I was just looking at that. Have added a comment.
> 
>>
>>> +/* Hardware specific access to control-lines */
>>> +static void nand_hwcontrol(struct mtd_info *mtd, int cmd,
>>> +     unsigned int ctrl)
>>> +{
>>> +}
>>
>> Don't implement this at all if it doesn't make sense for this hardware.
> 
> It isn't implemented (the function is empty), but if it is not defined
> then the NAND layer will die when it calls a null function. What
> should I do here?

It should only be called if you don't replace nand_command[_lp] and
nand_select_chip.  If it's because of nand_select_chip, I'd rather see a
dummy implementation of that.

>>> +     case NAND_CMD_READ0:
>>> +             writel(NAND_CMD_READ0, &info->reg->cmd_reg1);
>>> +             writel(NAND_CMD_READSTART, &info->reg->cmd_reg2);
>>> +             writel((page_addr << 16) | (column & 0xFFFF),
>>> +                     &info->reg->addr_reg1);
>>> +             writel(page_addr >> 16, &info->reg->addr_reg2);
>>> +             return;
>>> +     case NAND_CMD_SEQIN:
>>> +             writel(NAND_CMD_SEQIN, &info->reg->cmd_reg1);
>>> +             writel(NAND_CMD_PAGEPROG, &info->reg->cmd_reg2);
>>> +             writel((page_addr << 16) | (column & 0xFFFF),
>>> +                     &info->reg->addr_reg1);
>>> +             writel(page_addr >> 16,
>>> +                     &info->reg->addr_reg2);
>>> +             return;
>>> +     case NAND_CMD_PAGEPROG:
>>> +             return;
>>> +     case NAND_CMD_ERASE1:
>>> +             writel(NAND_CMD_ERASE1, &info->reg->cmd_reg1);
>>> +             writel(NAND_CMD_ERASE2, &info->reg->cmd_reg2);
>>> +             writel(page_addr, &info->reg->addr_reg1);
>>> +             writel(CMD_GO | CMD_CLE | CMD_ALE |
>>> +                     CMD_SEC_CMD | CMD_CE0 | CMD_ALE_BYTES3,
>>> +                     &info->reg->command);
>>> +             break;
>>> +     case NAND_CMD_RNDOUT:
>>> +             return;
>>
>> Don't silently ignore RNDOUT -- if you don't support it and it happens,
>> complain loudly.
> 
> OK, I added a printf(), that should be loud enough. Would prefer a
> debug() though.

I don't think it should be debug() -- this is indicating a problem (you
could be losing data), not just potentially helpful information.

>>> +     case NAND_CMD_ERASE2:
>>> +             return;
>>> +     case NAND_CMD_STATUS:
>>> +             writel(NAND_CMD_STATUS, &info->reg->cmd_reg1);
>>> +             writel(CMD_GO | CMD_CLE | CMD_PIO | CMD_RX
>>> +                     | (CMD_TRANS_SIZE_BYTES1 <<
>>> +                             CMD_TRANS_SIZE_SHIFT)
>>> +                     | CMD_CE0,
>>> +                     &info->reg->command);
>>> +             info->pio_byte_index = 0;
>>> +             break;
>>> +     case NAND_CMD_RESET:
>>> +             writel(NAND_CMD_RESET, &info->reg->cmd_reg1);
>>> +             writel(CMD_GO | CMD_CLE | CMD_CE0,
>>> +                     &info->reg->command);
>>> +             break;
>>> +     default:
>>> +             return;
>>
>> Likewise, complain if you get an unrecognized command.
> 
> Done - I wonder why we can't just return an error?

Because the interface wasn't designed to allow that, unfortunately.  If
you want to change that, start in Linux.

>>> +/**
>>> + * Set up NAND bus width and page size
>>> + *
>>> + * @param info               nand_info structure
>>> + * @param *reg_val   address of reg_val
>>> + * @return none - value is set in reg_val
>>> + */
>>> +static void set_bus_width_page_size(struct fdt_nand *config,
>>> +     u32 *reg_val)
>>> +{
>>> +     if (config->width == 8)
>>> +             *reg_val = CFG_BUS_WIDTH_8BIT;
>>> +     else
>>> +             *reg_val = CFG_BUS_WIDTH_16BIT;
>>> +
>>> +     if (config->page_data_bytes == 256)
>>> +             *reg_val |= CFG_PAGE_SIZE_256;
>>> +     else if (config->page_data_bytes == 512)
>>> +             *reg_val |= CFG_PAGE_SIZE_512;
>>> +     else if (config->page_data_bytes == 1024)
>>> +             *reg_val |= CFG_PAGE_SIZE_1024;
>>> +     else if (config->page_data_bytes == 2048)
>>> +             *reg_val |= CFG_PAGE_SIZE_2048;
>>
>> Really, page sizes of 256 and 1024 bytes?
>>
>> From elsewhere in the driver you only support 2048 byte pages, so why
>> not just check for that and error (not silently continue) if it's
>> anything else?
> 
> I don't think it is that bad - I believe the driver should all the
> options, although I have not tested it or gone into it carefully. Have
> added error checking.

I've never heard of a NAND chip with 1024-byte pages, and the only
reference to 256-byte pages I've seen is in the "museum IDs" section of
the ID table.

Can this controller support 4096-byte pages (or 8192)?

>>> +     if (!is_writing) {
>>> +             invalidate_dcache_range((unsigned long) buf,
>>> +                     ((unsigned long) buf) +
>>> +                     (1 << chip->page_shift));
>>> +     } else {
>>> +             flush_dcache_range((unsigned long) buf,
>>> +                     ((unsigned long) buf) +
>>> +                     (1 << chip->page_shift));
>>> +     }
>>
>> Please factor out all this cache stuff into a dma prepare function that
>> takes start, length, and is_writing.  Is it even really needed to
>> invalidate rather than flush in the read case?  It doesn't look like
>> these buffers are even going to be cacheline-aligned...
> 
> Done
> 
> I am not keen on adding cache alignment into every driver - IMO this
> should happen at the level above as with MMC, USB, etc. A fairly
> trivial fix to nand_base caches most of the problems. I will include a
> patch in my next series. Anyway for now, Tegra has cache off because
> of all these warnings.

Why would nand_base be in a position to know that you have special
alignment needs?  Most NAND controllers don't do DMA, and many systems
don't require cacheline alignment for DMA.  Linux hasn't needed this,
why does U-Boot?

>>> +int fdt_decode_nand(const void *blob, int node, struct fdt_nand *config)
>>> +{
>>> +     int err;
>>> +
>>> +     config->reg = (struct nand_ctlr *)fdtdec_get_addr(blob, node, "reg");
>>> +     config->enabled = fdtdec_get_is_enabled(blob, node);
>>> +     config->width = fdtdec_get_int(blob, node, "width", 8);
>>> +     err = fdtdec_decode_gpio(blob, node, "wp-gpios", &config->wp_gpio);
>>> +     if (err)
>>> +             return err;
>>> +     err = fdtdec_get_int_array(blob, node, "nvidia,timing",
>>> +                     config->timing, FDT_NAND_TIMING_COUNT);
>>> +     if (err < 0)
>>> +             return err;
>>> +
>>> +     /* Now look up the controller and decode that */
>>> +     node = fdt_next_node(blob, node, NULL);
>>> +     if (node < 0)
>>> +             return node;
>>> +
>>> +     config->page_data_bytes = fdtdec_get_int(blob, node,
>>> +                                             "page-data-bytes", -1);
>>> +     config->tag_ecc_bytes = fdtdec_get_int(blob, node,
>>> +                                             "tag-ecc-bytes", -1);
>>> +     config->tag_bytes = fdtdec_get_int(blob, node, "tag-bytes", -1);
>>> +     config->data_ecc_bytes = fdtdec_get_int(blob, node,
>>> +                                             "data-ecc-bytes", -1);
>>> +     config->skipped_spare_bytes = fdtdec_get_int(blob, node,
>>> +                                             "skipped-spare-bytes", -1);
>>> +     config->page_spare_bytes = fdtdec_get_int(blob, node,
>>> +                                             "page-spare-bytes", -1);
>>
>> Has this binding been accepted into Linux's documentation or another
>> canonical source?
> 
> No

It would be good to get that settled first.  I assume you'll want to use
this binding with Linux eventually.

This code also needs better namespacing -- this isn't applicable for all
NAND controllers/bindings.

-Scott

[U-Boot] [PATCH 5/6] tegra: nand: Add Tegra NAND driver

[Simon Glass]

Hi Scott,

On Fri, Apr 13, 2012 at 11:09 AM, Scott Wood <scottwood@freescale.com> wrote:
> On 04/13/2012 12:42 PM, Simon Glass wrote:
>> Hi Scott,
>>
>> On Fri, Jan 20, 2012 at 2:55 PM, Scott Wood <scottwood@freescale.com> wrote:
>>> On 01/13/2012 05:10 PM, Simon Glass wrote:
>>>> +struct nand_info nand_ctrl;
>>>
>>> static (or better, dynamic).
>>
>> Done, what is dynamic?
>
> Allocate the structure dynamically in your init function, and write the
> code in such a way as to allow multiple instances. ?Not mandatory if you
> really think you'll never have multiple instances, but would be nice and
> shouldn't complicate things much.

There can be only one in Tegra, at least with current devices.

>
>>>> + ? ? struct nand_info *info;
>>>> +
>>>> + ? ? info = (struct nand_info *) chip->priv;
>>>> +
>>>> + ? ? dword_read = readl(&info->reg->resp);
>>>> + ? ? dword_read = dword_read >> (8 * info->pio_byte_index);
>>>> + ? ? info->pio_byte_index++;
>>>> + ? ? return (uint8_t) dword_read;
>>>
>>> No space after casts.
>>
>> I think I have fixed that globally.
>>
>>>
>>> What happens when pio_byte_index > 3? ?Please don't assume that upper
>>> bits will be ignored, even if that happens to be true on this chip.
>>
>> I added an assert() - there is no way to return an error here. I'm not
>> sure what you mean by upper bits - there is a uint8_t cast.
>
> By upper bits I mean in the shift count. ?Don't assume that
> dword_read >> 32 is the same as dword_read >> 0. ?It's undefined in C.

OK I see, I have handled that.

>
>>> How does info->reg->resp work? ?You don't seem to be choosing the dword
>>> to read based on pio_byte_index, which suggests that the act of reading
>>> resp changes what will be read the next time. ?But, you read it on each
>>> byte, which would advance resp four times too fast if it's simply
>>> returning a new dword each time.
>>>
>>> If the hardware is really auto-advancing resp only on every fourth
>>> access, that needs a comment.
>>
>> I was just looking at that. Have added a comment.
>>
>>>
>>>> +/* Hardware specific access to control-lines */
>>>> +static void nand_hwcontrol(struct mtd_info *mtd, int cmd,
>>>> + ? ? unsigned int ctrl)
>>>> +{
>>>> +}
>>>
>>> Don't implement this at all if it doesn't make sense for this hardware.
>>
>> It isn't implemented (the function is empty), but if it is not defined
>> then the NAND layer will die when it calls a null function. What
>> should I do here?
>
> It should only be called if you don't replace nand_command[_lp] and
> nand_select_chip. ?If it's because of nand_select_chip, I'd rather see a
> dummy implementation of that.

OK I see, will change it.

>
>>>> + ? ? case NAND_CMD_READ0:
>>>> + ? ? ? ? ? ? writel(NAND_CMD_READ0, &info->reg->cmd_reg1);
>>>> + ? ? ? ? ? ? writel(NAND_CMD_READSTART, &info->reg->cmd_reg2);
>>>> + ? ? ? ? ? ? writel((page_addr << 16) | (column & 0xFFFF),
>>>> + ? ? ? ? ? ? ? ? ? ? &info->reg->addr_reg1);
>>>> + ? ? ? ? ? ? writel(page_addr >> 16, &info->reg->addr_reg2);
>>>> + ? ? ? ? ? ? return;
>>>> + ? ? case NAND_CMD_SEQIN:
>>>> + ? ? ? ? ? ? writel(NAND_CMD_SEQIN, &info->reg->cmd_reg1);
>>>> + ? ? ? ? ? ? writel(NAND_CMD_PAGEPROG, &info->reg->cmd_reg2);
>>>> + ? ? ? ? ? ? writel((page_addr << 16) | (column & 0xFFFF),
>>>> + ? ? ? ? ? ? ? ? ? ? &info->reg->addr_reg1);
>>>> + ? ? ? ? ? ? writel(page_addr >> 16,
>>>> + ? ? ? ? ? ? ? ? ? ? &info->reg->addr_reg2);
>>>> + ? ? ? ? ? ? return;
>>>> + ? ? case NAND_CMD_PAGEPROG:
>>>> + ? ? ? ? ? ? return;
>>>> + ? ? case NAND_CMD_ERASE1:
>>>> + ? ? ? ? ? ? writel(NAND_CMD_ERASE1, &info->reg->cmd_reg1);
>>>> + ? ? ? ? ? ? writel(NAND_CMD_ERASE2, &info->reg->cmd_reg2);
>>>> + ? ? ? ? ? ? writel(page_addr, &info->reg->addr_reg1);
>>>> + ? ? ? ? ? ? writel(CMD_GO | CMD_CLE | CMD_ALE |
>>>> + ? ? ? ? ? ? ? ? ? ? CMD_SEC_CMD | CMD_CE0 | CMD_ALE_BYTES3,
>>>> + ? ? ? ? ? ? ? ? ? ? &info->reg->command);
>>>> + ? ? ? ? ? ? break;
>>>> + ? ? case NAND_CMD_RNDOUT:
>>>> + ? ? ? ? ? ? return;
>>>
>>> Don't silently ignore RNDOUT -- if you don't support it and it happens,
>>> complain loudly.
>>
>> OK, I added a printf(), that should be loud enough. Would prefer a
>> debug() though.
>
> I don't think it should be debug() -- this is indicating a problem (you
> could be losing data), not just potentially helpful information.

Yes, the API should be changed to return a value, then a debug might be useful.

>
>>>> + ? ? case NAND_CMD_ERASE2:
>>>> + ? ? ? ? ? ? return;
>>>> + ? ? case NAND_CMD_STATUS:
>>>> + ? ? ? ? ? ? writel(NAND_CMD_STATUS, &info->reg->cmd_reg1);
>>>> + ? ? ? ? ? ? writel(CMD_GO | CMD_CLE | CMD_PIO | CMD_RX
>>>> + ? ? ? ? ? ? ? ? ? ? | (CMD_TRANS_SIZE_BYTES1 <<
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? CMD_TRANS_SIZE_SHIFT)
>>>> + ? ? ? ? ? ? ? ? ? ? | CMD_CE0,
>>>> + ? ? ? ? ? ? ? ? ? ? &info->reg->command);
>>>> + ? ? ? ? ? ? info->pio_byte_index = 0;
>>>> + ? ? ? ? ? ? break;
>>>> + ? ? case NAND_CMD_RESET:
>>>> + ? ? ? ? ? ? writel(NAND_CMD_RESET, &info->reg->cmd_reg1);
>>>> + ? ? ? ? ? ? writel(CMD_GO | CMD_CLE | CMD_CE0,
>>>> + ? ? ? ? ? ? ? ? ? ? &info->reg->command);
>>>> + ? ? ? ? ? ? break;
>>>> + ? ? default:
>>>> + ? ? ? ? ? ? return;
>>>
>>> Likewise, complain if you get an unrecognized command.
>>
>> Done - I wonder why we can't just return an error?
>
> Because the interface wasn't designed to allow that, unfortunately. ?If
> you want to change that, start in Linux.

http://www.youtube.com/watch?v=K8E_zMLCRNg

>
>>>> +/**
>>>> + * Set up NAND bus width and page size
>>>> + *
>>>> + * @param info ? ? ? ? ? ? ? nand_info structure
>>>> + * @param *reg_val ? address of reg_val
>>>> + * @return none - value is set in reg_val
>>>> + */
>>>> +static void set_bus_width_page_size(struct fdt_nand *config,
>>>> + ? ? u32 *reg_val)
>>>> +{
>>>> + ? ? if (config->width == 8)
>>>> + ? ? ? ? ? ? *reg_val = CFG_BUS_WIDTH_8BIT;
>>>> + ? ? else
>>>> + ? ? ? ? ? ? *reg_val = CFG_BUS_WIDTH_16BIT;
>>>> +
>>>> + ? ? if (config->page_data_bytes == 256)
>>>> + ? ? ? ? ? ? *reg_val |= CFG_PAGE_SIZE_256;
>>>> + ? ? else if (config->page_data_bytes == 512)
>>>> + ? ? ? ? ? ? *reg_val |= CFG_PAGE_SIZE_512;
>>>> + ? ? else if (config->page_data_bytes == 1024)
>>>> + ? ? ? ? ? ? *reg_val |= CFG_PAGE_SIZE_1024;
>>>> + ? ? else if (config->page_data_bytes == 2048)
>>>> + ? ? ? ? ? ? *reg_val |= CFG_PAGE_SIZE_2048;
>>>
>>> Really, page sizes of 256 and 1024 bytes?
>>>
>>> From elsewhere in the driver you only support 2048 byte pages, so why
>>> not just check for that and error (not silently continue) if it's
>>> anything else?
>>
>> I don't think it is that bad - I believe the driver should all the
>> options, although I have not tested it or gone into it carefully. Have
>> added error checking.
>
> I've never heard of a NAND chip with 1024-byte pages, and the only
> reference to 256-byte pages I've seen is in the "museum IDs" section of
> the ID table.

I will remove them then.

>
> Can this controller support 4096-byte pages (or 8192)?

Have added 4096, but I don't think it supports 8192 (shown as
'reserved' in datasheet).

>
>>>> + ? ? if (!is_writing) {
>>>> + ? ? ? ? ? ? invalidate_dcache_range((unsigned long) buf,
>>>> + ? ? ? ? ? ? ? ? ? ? ((unsigned long) buf) +
>>>> + ? ? ? ? ? ? ? ? ? ? (1 << chip->page_shift));
>>>> + ? ? } else {
>>>> + ? ? ? ? ? ? flush_dcache_range((unsigned long) buf,
>>>> + ? ? ? ? ? ? ? ? ? ? ((unsigned long) buf) +
>>>> + ? ? ? ? ? ? ? ? ? ? (1 << chip->page_shift));
>>>> + ? ? }
>>>
>>> Please factor out all this cache stuff into a dma prepare function that
>>> takes start, length, and is_writing. ?Is it even really needed to
>>> invalidate rather than flush in the read case? ?It doesn't look like
>>> these buffers are even going to be cacheline-aligned...
>>
>> Done
>>
>> I am not keen on adding cache alignment into every driver - IMO this
>> should happen at the level above as with MMC, USB, etc. A fairly
>> trivial fix to nand_base caches most of the problems. I will include a
>> patch in my next series. Anyway for now, Tegra has cache off because
>> of all these warnings.
>
> Why would nand_base be in a position to know that you have special
> alignment needs? ?Most NAND controllers don't do DMA, and many systems
> don't require cacheline alignment for DMA. ?Linux hasn't needed this,
> why does U-Boot?

There is now an ARCH_DMA_MINALIGN define in U-Boot, and a
ALLOC_CACHE_ALIGN_BUFFER macro just for this purpose. There has been
some effort in making things like ext2, mmc and USB work properly with
DMA alignment. I don't see any need to make it hard to the driver - if
we can avoid bounce buffers we should.

>
>>>> +int fdt_decode_nand(const void *blob, int node, struct fdt_nand *config)
>>>> +{
>>>> + ? ? int err;
>>>> +
>>>> + ? ? config->reg = (struct nand_ctlr *)fdtdec_get_addr(blob, node, "reg");
>>>> + ? ? config->enabled = fdtdec_get_is_enabled(blob, node);
>>>> + ? ? config->width = fdtdec_get_int(blob, node, "width", 8);
>>>> + ? ? err = fdtdec_decode_gpio(blob, node, "wp-gpios", &config->wp_gpio);
>>>> + ? ? if (err)
>>>> + ? ? ? ? ? ? return err;
>>>> + ? ? err = fdtdec_get_int_array(blob, node, "nvidia,timing",
>>>> + ? ? ? ? ? ? ? ? ? ? config->timing, FDT_NAND_TIMING_COUNT);
>>>> + ? ? if (err < 0)
>>>> + ? ? ? ? ? ? return err;
>>>> +
>>>> + ? ? /* Now look up the controller and decode that */
>>>> + ? ? node = fdt_next_node(blob, node, NULL);
>>>> + ? ? if (node < 0)
>>>> + ? ? ? ? ? ? return node;
>>>> +
>>>> + ? ? config->page_data_bytes = fdtdec_get_int(blob, node,
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "page-data-bytes", -1);
>>>> + ? ? config->tag_ecc_bytes = fdtdec_get_int(blob, node,
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "tag-ecc-bytes", -1);
>>>> + ? ? config->tag_bytes = fdtdec_get_int(blob, node, "tag-bytes", -1);
>>>> + ? ? config->data_ecc_bytes = fdtdec_get_int(blob, node,
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "data-ecc-bytes", -1);
>>>> + ? ? config->skipped_spare_bytes = fdtdec_get_int(blob, node,
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "skipped-spare-bytes", -1);
>>>> + ? ? config->page_spare_bytes = fdtdec_get_int(blob, node,
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "page-spare-bytes", -1);
>>>
>>> Has this binding been accepted into Linux's documentation or another
>>> canonical source?
>>
>> No
>
> It would be good to get that settled first. ?I assume you'll want to use
> this binding with Linux eventually.

I am not sure - from what I hear Linux does ID lookup instead - but
really this binding is only useful without ONFI support I suspect.

>
> This code also needs better namespacing -- this isn't applicable for all
> NAND controllers/bindings.

Do you mean the "nvidia," prefix?

I will send another version of the series with comments received so
far addressed.

>
> -Scott
>

Regards,
Simon

[U-Boot] [PATCH 5/6] tegra: nand: Add Tegra NAND driver

[Scott Wood]

On 04/13/2012 01:25 PM, Simon Glass wrote:
> Hi Scott,
> 
> On Fri, Apr 13, 2012 at 11:09 AM, Scott Wood <scottwood@freescale.com> wrote:
>> On 04/13/2012 12:42 PM, Simon Glass wrote:
>>> I am not keen on adding cache alignment into every driver - IMO this
>>> should happen at the level above as with MMC, USB, etc. A fairly
>>> trivial fix to nand_base caches most of the problems. I will include a
>>> patch in my next series. Anyway for now, Tegra has cache off because
>>> of all these warnings.
>>
>> Why would nand_base be in a position to know that you have special
>> alignment needs?  Most NAND controllers don't do DMA, and many systems
>> don't require cacheline alignment for DMA.  Linux hasn't needed this,
>> why does U-Boot?
> 
> There is now an ARCH_DMA_MINALIGN define in U-Boot, and a
> ALLOC_CACHE_ALIGN_BUFFER macro just for this purpose. There has been
> some effort in making things like ext2, mmc and USB work properly with
> DMA alignment. I don't see any need to make it hard to the driver - if
> we can avoid bounce buffers we should.

I understand the desire to avoid bounce buffers, but I also don't want
to introduce unnecessary differences between Linux and U-Boot in
nand_base.c.  How does your Linux driver handle this?

>>>>> +int fdt_decode_nand(const void *blob, int node, struct fdt_nand *config)
>>>>> +{
>>>>> +     int err;
>>>>> +
>>>>> +     config->reg = (struct nand_ctlr *)fdtdec_get_addr(blob, node, "reg");
>>>>> +     config->enabled = fdtdec_get_is_enabled(blob, node);
>>>>> +     config->width = fdtdec_get_int(blob, node, "width", 8);
>>>>> +     err = fdtdec_decode_gpio(blob, node, "wp-gpios", &config->wp_gpio);
>>>>> +     if (err)
>>>>> +             return err;
>>>>> +     err = fdtdec_get_int_array(blob, node, "nvidia,timing",
>>>>> +                     config->timing, FDT_NAND_TIMING_COUNT);
>>>>> +     if (err < 0)
>>>>> +             return err;
>>>>> +
>>>>> +     /* Now look up the controller and decode that */
>>>>> +     node = fdt_next_node(blob, node, NULL);
>>>>> +     if (node < 0)
>>>>> +             return node;
>>>>> +
>>>>> +     config->page_data_bytes = fdtdec_get_int(blob, node,
>>>>> +                                             "page-data-bytes", -1);
>>>>> +     config->tag_ecc_bytes = fdtdec_get_int(blob, node,
>>>>> +                                             "tag-ecc-bytes", -1);
>>>>> +     config->tag_bytes = fdtdec_get_int(blob, node, "tag-bytes", -1);
>>>>> +     config->data_ecc_bytes = fdtdec_get_int(blob, node,
>>>>> +                                             "data-ecc-bytes", -1);
>>>>> +     config->skipped_spare_bytes = fdtdec_get_int(blob, node,
>>>>> +                                             "skipped-spare-bytes", -1);
>>>>> +     config->page_spare_bytes = fdtdec_get_int(blob, node,
>>>>> +                                             "page-spare-bytes", -1);
>>>>
>>>> Has this binding been accepted into Linux's documentation or another
>>>> canonical source?
>>>
>>> No
>>
>> It would be good to get that settled first.  I assume you'll want to use
>> this binding with Linux eventually.
> 
> I am not sure - from what I hear Linux does ID lookup instead - but
> really this binding is only useful without ONFI support I suspect.

I'm not sure what difference you're seeing between U-Boot and Linux.
Both U-Boot and Linux uses the ID table in the absence of ONFI.

>> This code also needs better namespacing -- this isn't applicable for all
>> NAND controllers/bindings.
> 
> Do you mean the "nvidia," prefix?

No, I mean the fact that this is a global function called
"fdt_decode_nand", taking a "struct fdt_nand" argument, that is specific
to one controller type.

-Scott

[U-Boot] [PATCH 5/6] tegra: nand: Add Tegra NAND driver

[Simon Glass]

Hi Scott,

On Fri, Apr 13, 2012 at 11:34 AM, Scott Wood <scottwood@freescale.com> wrote:
> On 04/13/2012 01:25 PM, Simon Glass wrote:
>> Hi Scott,
>>
>> On Fri, Apr 13, 2012 at 11:09 AM, Scott Wood <scottwood@freescale.com> wrote:
>>> On 04/13/2012 12:42 PM, Simon Glass wrote:
>>>> I am not keen on adding cache alignment into every driver - IMO this
>>>> should happen at the level above as with MMC, USB, etc. A fairly
>>>> trivial fix to nand_base caches most of the problems. I will include a
>>>> patch in my next series. Anyway for now, Tegra has cache off because
>>>> of all these warnings.
>>>
>>> Why would nand_base be in a position to know that you have special
>>> alignment needs? ?Most NAND controllers don't do DMA, and many systems
>>> don't require cacheline alignment for DMA. ?Linux hasn't needed this,
>>> why does U-Boot?
>>
>> There is now an ARCH_DMA_MINALIGN define in U-Boot, and a
>> ALLOC_CACHE_ALIGN_BUFFER macro just for this purpose. There has been
>> some effort in making things like ext2, mmc and USB work properly with
>> DMA alignment. I don't see any need to make it hard to the driver - if
>> we can avoid bounce buffers we should.
>
> I understand the desire to avoid bounce buffers, but I also don't want
> to introduce unnecessary differences between Linux and U-Boot in
> nand_base.c. ?How does your Linux driver handle this?

I have included a patch with the changes in the next version, it's
pretty minimal.

I don't see a linux driver in mainline Linux. We did have one in our
tree but it seems to be gone also. That one had lots of TODOs, and
used bounce buffers from what I can tell.

>
>>>>>> +int fdt_decode_nand(const void *blob, int node, struct fdt_nand *config)
>>>>>> +{
>>>>>> + ? ? int err;
>>>>>> +
>>>>>> + ? ? config->reg = (struct nand_ctlr *)fdtdec_get_addr(blob, node, "reg");
>>>>>> + ? ? config->enabled = fdtdec_get_is_enabled(blob, node);
>>>>>> + ? ? config->width = fdtdec_get_int(blob, node, "width", 8);
>>>>>> + ? ? err = fdtdec_decode_gpio(blob, node, "wp-gpios", &config->wp_gpio);
>>>>>> + ? ? if (err)
>>>>>> + ? ? ? ? ? ? return err;
>>>>>> + ? ? err = fdtdec_get_int_array(blob, node, "nvidia,timing",
>>>>>> + ? ? ? ? ? ? ? ? ? ? config->timing, FDT_NAND_TIMING_COUNT);
>>>>>> + ? ? if (err < 0)
>>>>>> + ? ? ? ? ? ? return err;
>>>>>> +
>>>>>> + ? ? /* Now look up the controller and decode that */
>>>>>> + ? ? node = fdt_next_node(blob, node, NULL);
>>>>>> + ? ? if (node < 0)
>>>>>> + ? ? ? ? ? ? return node;
>>>>>> +
>>>>>> + ? ? config->page_data_bytes = fdtdec_get_int(blob, node,
>>>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "page-data-bytes", -1);
>>>>>> + ? ? config->tag_ecc_bytes = fdtdec_get_int(blob, node,
>>>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "tag-ecc-bytes", -1);
>>>>>> + ? ? config->tag_bytes = fdtdec_get_int(blob, node, "tag-bytes", -1);
>>>>>> + ? ? config->data_ecc_bytes = fdtdec_get_int(blob, node,
>>>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "data-ecc-bytes", -1);
>>>>>> + ? ? config->skipped_spare_bytes = fdtdec_get_int(blob, node,
>>>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "skipped-spare-bytes", -1);
>>>>>> + ? ? config->page_spare_bytes = fdtdec_get_int(blob, node,
>>>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "page-spare-bytes", -1);
>>>>>
>>>>> Has this binding been accepted into Linux's documentation or another
>>>>> canonical source?
>>>>
>>>> No
>>>
>>> It would be good to get that settled first. ?I assume you'll want to use
>>> this binding with Linux eventually.
>>
>> I am not sure - from what I hear Linux does ID lookup instead - but
>> really this binding is only useful without ONFI support I suspect.
>
> I'm not sure what difference you're seeing between U-Boot and Linux.
> Both U-Boot and Linux uses the ID table in the absence of ONFI.

I don't think this driver supports ONFI. I haven't tried it though.

>
>>> This code also needs better namespacing -- this isn't applicable for all
>>> NAND controllers/bindings.
>>
>> Do you mean the "nvidia," prefix?
>
> No, I mean the fact that this is a global function called
> "fdt_decode_nand", taking a "struct fdt_nand" argument, that is specific
> to one controller type.

OK, so rename struct fdt_nand to struct fdt_tegra_nand or similar? I
will do this in the next version.

>
> -Scott
>

Regards,
Simon

[U-Boot] [PATCH 5/6] tegra: nand: Add Tegra NAND driver

[Scott Wood]

On 04/13/2012 01:45 PM, Simon Glass wrote:
> Hi Scott,
> 
> On Fri, Apr 13, 2012 at 11:34 AM, Scott Wood <scottwood@freescale.com> wrote:
>> On 04/13/2012 01:25 PM, Simon Glass wrote:
>>>>>> Has this binding been accepted into Linux's documentation or another
>>>>>> canonical source?
>>>>>
>>>>> No
>>>>
>>>> It would be good to get that settled first.  I assume you'll want to use
>>>> this binding with Linux eventually.
>>>
>>> I am not sure - from what I hear Linux does ID lookup instead - but
>>> really this binding is only useful without ONFI support I suspect.
>>
>> I'm not sure what difference you're seeing between U-Boot and Linux.
>> Both U-Boot and Linux uses the ID table in the absence of ONFI.
> 
> I don't think this driver supports ONFI. I haven't tried it though.

But even in the absence of ONFI, what is the difference?  Both Linux and
U-Boot use the ID table.  If you need this stuff in U-boot you'll need
it in Linux.

-Scott

[U-Boot] [PATCH 5/6] tegra: nand: Add Tegra NAND driver

[Simon Glass]

Hi Scott,

On Fri, Apr 13, 2012 at 11:47 AM, Scott Wood <scottwood@freescale.com> wrote:
> On 04/13/2012 01:45 PM, Simon Glass wrote:
>> Hi Scott,
>>
>> On Fri, Apr 13, 2012 at 11:34 AM, Scott Wood <scottwood@freescale.com> wrote:
>>> On 04/13/2012 01:25 PM, Simon Glass wrote:
>>>>>>> Has this binding been accepted into Linux's documentation or another
>>>>>>> canonical source?
>>>>>>
>>>>>> No
>>>>>
>>>>> It would be good to get that settled first. ?I assume you'll want to use
>>>>> this binding with Linux eventually.
>>>>
>>>> I am not sure - from what I hear Linux does ID lookup instead - but
>>>> really this binding is only useful without ONFI support I suspect.
>>>
>>> I'm not sure what difference you're seeing between U-Boot and Linux.
>>> Both U-Boot and Linux uses the ID table in the absence of ONFI.
>>
>> I don't think this driver supports ONFI. I haven't tried it though.
>
> But even in the absence of ONFI, what is the difference? ?Both Linux and
> U-Boot use the ID table. ?If you need this stuff in U-boot you'll need
> it in Linux.

Quite possibly, but without a Linux driver it is hard to compare.

>
> -Scott
>

Regards,
Simon

[U-Boot] [PATCH 6/6] tegra: Enable NAND on Seaboard

[Simon Glass]

This enables NAND support for the Seaboard.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 include/configs/seaboard.h |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/include/configs/seaboard.h b/include/configs/seaboard.h
index 7b6f92a..b31ac0c 100644
--- a/include/configs/seaboard.h
+++ b/include/configs/seaboard.h
@@ -105,4 +105,13 @@
 #define CONFIG_USB_STORAGE
 #define CONFIG_CMD_USB
 
+/* NAND support */
+#define CONFIG_CMD_NAND
+#define CONFIG_TEGRA2_NAND
+
+/* Max number of NAND devices */
+#define CONFIG_SYS_MAX_NAND_DEVICE	1
+
+/* Somewhat oddly, the NAND base address must be a config option */
+#define CONFIG_SYS_NAND_BASE	TEGRA2_NAND_BASE
 #endif /* __CONFIG_H */
-- 
1.7.7.3