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

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

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; }

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, };
/**

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

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>; + }; +};

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>;
- };
+};

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...
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

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@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>; + }; + }; };

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@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 at 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(®->command) & CMD_GO) || + !(readl(®->status) & + STATUS_RBSY0) || + !(readl(®->isr) & + ISR_IS_CMD_DONE)) { + udelay(1); + continue; + } + reg_val = readl(®->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(®->isr); + writel(reg_val, ®->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(®->isr) & ISR_IS_ECC_ERR)) + return ECC_OK; + + reg_val = readl(®->dec_status); + if ((reg_val & DEC_STATUS_A_ECC_FAIL) && databuf) { + reg_val = readl(®->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(®->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(®->command); + reg_val |= CMD_GO; + writel(reg_val, ®->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, ®->command); + + /* Stop DMA engine and clear DMA completion status */ + writel(DMA_MST_CTRL_GO_DISABLE + | DMA_MST_CTRL_IS_DMA_DONE, + ®->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, ®_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, ®_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, ®->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, ®->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)

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

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

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

Simon, Could you also add
Signed-off-by: Jim Lin jilin@nvidia.com
for me?
Thanks, Jim
-----Original Message----- From: Mike Frysinger [mailto:vapier@gentoo.org] Sent: Sunday, January 15, 2012 12:12 PM To: Simon Glass Cc: u-boot@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. -----------------------------------------------------------------------------------

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(®->command) & CMD_GO) ||
!(readl(®->status) &
STATUS_RBSY0) ||
!(readl(®->isr) &
ISR_IS_CMD_DONE)) {
udelay(1);
continue;
}
reg_val = readl(®->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?

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(®->command) & CMD_GO) ||
- !(readl(®->status) &
- STATUS_RBSY0) ||
- !(readl(®->isr) &
- ISR_IS_CMD_DONE)) {
- udelay(1);
- continue;
- }
- reg_val = readl(®->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

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(®->command) & CMD_GO) ||
!(readl(®->status) &
STATUS_RBSY0) ||
!(readl(®->isr) &
ISR_IS_CMD_DONE)) {
udelay(1);
continue;
}
reg_val = readl(®->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(®->isr);
- writel(reg_val, ®->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?
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?
+}
+/**
- 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)
printf("buf 0x%X has to be 4-byte aligned\n", (u32) buf);
%p
- set_bus_width_page_size(&info->config, ®_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)?
- /* 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...
+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?
Usually vendor prefixes are asked for on such properties (similar to "nvidia,timing").
+/* 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.
+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?
-Scott

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(®->command) & CMD_GO) ||
- !(readl(®->status) &
- STATUS_RBSY0) ||
- !(readl(®->isr) &
- ISR_IS_CMD_DONE)) {
- udelay(1);
- continue;
- }
- reg_val = readl(®->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(®->isr);
- writel(reg_val, ®->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, ®_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

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

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

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

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

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

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

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 */
participants (5)
-
Jim Lin
-
Mike Frysinger
-
Scott Wood
-
Simon Glass
-
Stephen Warren