tegra: Add NAND flash support

doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt new file +2 -0

new file mode 100644
index 0000000..2484556
@@ -0,0 +1,68 @@
+NAND Flash
+----------

Quoted 13 lines · Simon Glass

doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt new file +7 -0

new file mode 100644
index 0000000..2484556
@@ -0,0 +1,68 @@
+NAND Flash
+----------
+
+(there isn't yet a generic binding in Linux, so this describes what is in
+U-Boot. There should not be Linux-specific or U-Boot specific binding, just
+a binding that describes this hardware. But agreeing a binding in Linux in
+the absence of a driver may be beyond my powers.)

doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt line 12

Quoted 5 lines

doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt new file +5 -0

+
+The device node for a NAND flash device is as follows:
+
+Required properties :
+ - compatible : Should be "manufacturer,device", "nand-flash"

Again, "nand-flash" is not an appropriate compatible.  There is no generic nand-flash binding.

doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt line 16

Quoted 5 lines

doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt new file +4 -0

+ - nvidia,page-data-bytes : Number of bytes in the data area
+ - nvidia,page-spare-bytes : Number of bytes in spare area
+       spare area = skipped-spare-bytes + data-ecc-bytes + tag-bytes
+                     + tag-ecc-bytes

Do you really need this stuff to be in the device tree?  You should be able to determine this information from the ID table.

doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt line 18

Quoted 3 lines

doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt new file +2 -0

+ - nvidia,skipped-spare-bytes : Number of bytes to skip at start of spare area
+     (these are typically used for bad block maintenance)

So this binding can't deal with the bad block marker being somewhere other than the beginning of the spare area (e.g. 8-bit small page NAND)?

doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt line 19

Quoted 2 lines

doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt new file +1 -0

+ - nvidia,data-ecc-bytes : Number of ECC bytes for data area

Number of ECC bytes per page?  Number of ECC bytes per ECC block? Number of data bytes per ECC block?

doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt line 20

Quoted 1 line

doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt new file +1 -0

+ - nvidia,tag-bytes :Number of tag bytes in spare area

What are tag bytes?

doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt line 34

Quoted 10 lines

doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt new file +9 -0

+Nvidia NAND Controller
+----------------------
+
+The device node for a NAND flash controller is as follows:
+
+Optional properties:
+
+nvidia,wp-gpios : GPIO of write-protect line, three cells in the format:
+             phandle, parameter, flags

Doesn't the number of cells depend on the GPIO controller binding?

doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt line 39

Quoted 6 lines

doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt new file +5 -0

+nvidia,nand-width : bus width of the NAND device in bits
+
+ - 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

Might want to point out that there's one cell per timing parameter.

-Scott

+Jim, who wrote the driver originally

Hi Scott,

Quoted 5 lines · Scott Wood

Quoted 4 lines

+ - nvidia,page-data-bytes : Number of bytes in the data area
+ - nvidia,page-spare-bytes : Number of bytes in spare area

doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt new file +2 -0

+       spare area = skipped-spare-bytes + data-ecc-bytes + tag-bytes
+                     + tag-ecc-bytes

Quoted 2 lines

Do you really need this stuff to be in the device tree? You should be able to determine this information from the ID table.

I suspect so - the driver originally had a lot of CONFIGs for this. Maybe someone who wants to take it further could do this as part of supporting ONFI?

I will see if Jim Lin can take another look.

Quoted 11 lines

doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt line 34

Quoted 9 lines

doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt new file +9 -0

+Nvidia NAND Controller
+----------------------
+
+The device node for a NAND flash controller is as follows:
+
+Optional properties:
+
+nvidia,wp-gpios : GPIO of write-protect line, three cells in the format:
+             phandle, parameter, flags

Doesn't the number of cells depend on the GPIO controller binding?

Yes, but this is the binding Tegra uses.

Quoted 17 lines · Simon Glass

+Jim, who wrote the driver originally

Hi Scott,

Quoted 7 lines · Scott Wood

Quoted 4 lines

+ - nvidia,page-data-bytes : Number of bytes in the data area
+ - nvidia,page-spare-bytes : Number of bytes in spare area
+       spare area = skipped-spare-bytes + data-ecc-bytes + tag-bytes
+                     + tag-ecc-bytes

Do you really need this stuff to be in the device tree?  You should be able to determine this information from the ID table.

I suspect so - the driver originally had a lot of CONFIGs for this. Maybe someone who wants to take it further could do this as part of supporting ONFI?

Quoted 1 line

I will see if Jim Lin can take another look.

You don't need ONFI to get the page/spare size out of the ID table.

The generic NAND code should already be doing this for you (fills in mtd->writesize and mtd->oobsize).  If you need it during setup, we now have CONFIG_SYS_NAND_SELF_INIT that allows splitting up nand_scan_ident() from nand_scan_tail().

Quoted 13 lines

Quoted 12 lines

doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt line 34

Quoted 10 lines

doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt new file +9 -0

+Nvidia NAND Controller
+----------------------
+
+The device node for a NAND flash controller is as follows:
+
+Optional properties:
+
+nvidia,wp-gpios : GPIO of write-protect line, three cells in the format:
+             phandle, parameter, flags

Doesn't the number of cells depend on the GPIO controller binding?

Quoted 1 line

Yes, but this is the binding Tegra uses.

Still, it doesn't belong in the NAND binding.  Maybe a future chip wants to use this NAND binding but a different GPIO binding.  If nothing else, people tend to copy-and-paste such descriptions.  We've still got people adding bindings for Freescale devices saying interrupts are encoded as a pair of cells, even though the interrupt controller now uses four cells per interrupt.

-Scott

Hi Scott,

Quoted 4 lines · Scott Wood

Quoted 3 lines · Simon Glass

Quoted 1 line · Scott Wood

Doesn't the number of cells depend on the GPIO controller binding?

Quoted 8 lines

Quoted 1 line

Yes, but this is the binding Tegra uses.

Still, it doesn't belong in the NAND binding. Maybe a future chip wants to use this NAND binding but a different GPIO binding. If nothing else, people tend to copy-and-paste such descriptions. We've still got people adding bindings for Freescale devices saying interrupts are encoded as a pair of cells, even though the interrupt controller now uses four cells per interrupt.

OK I see - are you are saying that we should just say something like:

"nvidia,wp-gpios : GPIO of write-protect line, as defined by gpio bindings"

+#define DEBUG

+/**
+ * [DEFAULT] Read one byte from the chip
+ *
+ * @param mtd MTD device structure
+ * @return data byte
+ *
+ * Default read function for 8bit bus-width
+ */

drivers/mtd/nand/tegra2_nand.c new file +6 -0

+static uint8_t read_byte(struct mtd_info *mtd)
+{
+	struct nand_chip *chip = mtd->priv;
+	u32 dword_read;
+	struct nand_drv *info;
+

drivers/mtd/nand/tegra2_nand.c new file +9 -0

+/**
+ * [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
+ */

drivers/mtd/nand/tegra2_nand.c new file +3 -0

+	case NAND_CMD_RNDOUT:
+		printf("%s: Unsupported RNDOUT command\n", __func__);
+		return;

drivers/mtd/nand/tegra2_nand.c new file +12 -0

+	if ((reg_val & DEC_STATUS_A_ECC_FAIL) && databuf) {
+		reg_val = readl(&reg->bch_dec_status_buf);
+		/*
+		 * If uncorrectable error occurs on data area, then see whether
+		 * they are all FF. If all are FF, it's a blank page.
+		 * Not error.
+		 */
+		if ((reg_val & BCH_DEC_STATUS_FAIL_SEC_FLAG_MASK) &&
+			!blank_check(databuf, a_len))
+			return_val |= ECC_DATA_ERROR;
+	}
+

drivers/mtd/nand/tegra2_nand.c new file +6 -0

+	/* 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);

+int fdt_decode_nand(const void *blob, int node, struct fdt_nand *config)

drivers/mtd/nand/tegra2_nand.h new file +26 -0

new file mode 100644
index 0000000..7e74be7
@@ -0,0 +1,257 @@
+/*
+ * (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
+ */
+
+/* register offset */
+#define COMMAND_0		0x00
+#define CMD_GO			(1 << 31)
+#define CMD_CLE			(1 << 30)

Quoted 32 lines

drivers/mtd/nand/tegra2_nand.h new file +26 -0

new file mode 100644
index 0000000..7e74be7
@@ -0,0 +1,257 @@
+/*
+ * (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
+ */
+
+/* register offset */
+#define COMMAND_0		0x00
+#define CMD_GO			(1 << 31)
+#define CMD_CLE			(1 << 30)

drivers/mtd/nand/tegra2_nand.h line 36

Quoted 10 lines

drivers/mtd/nand/tegra2_nand.h new file +10 -0

+#define CMD_ALE			(1 << 29)
+#define CMD_PIO			(1 << 28)
+#define CMD_TX			(1 << 27)
+#define CMD_RX			(1 << 26)
+#define CMD_SEC_CMD		(1 << 25)
+#define CMD_AFT_DAT_MASK	(1 << 24)
+#define CMD_AFT_DAT_DISABLE	0
+#define CMD_AFT_DAT_ENABLE	(1 << 24)
+#define CMD_TRANS_SIZE_SHIFT	20
+#define CMD_TRANS_SIZE_PAGE	8

Please use proper namespacing on symbols defined in headers.

[Jim] Scott, I don't know what you meant. Could you give me an example?
Thanks.
The names like AFT_DAT_MASK, TRANS_SIZE_SHIFT, and TRANS_SIZE_PAGE come from field names of register in data sheet.

Quoted 1 line · Jim Lin

Review

Quoted 1 line

(quoted diff) +0 -0

b/drivers/mtd/nand/tegra2_nand.h

Quoted 45 lines

Quoted 31 lines

new file mode 100644
index 0000000..7e74be7

drivers/mtd/nand/tegra2_nand.h new file +26 -0

@@ -0,0 +1,257 @@
+/*
+ * (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
+ */
+
+/* register offset */
+#define COMMAND_0           0x00
+#define CMD_GO                      (1 << 31)
+#define CMD_CLE                     (1 << 30)

drivers/mtd/nand/tegra2_nand.h line 36

Quoted 10 lines

drivers/mtd/nand/tegra2_nand.h new file +10 -0

+#define CMD_ALE                     (1 << 29)
+#define CMD_PIO                     (1 << 28)
+#define CMD_TX                      (1 << 27)
+#define CMD_RX                      (1 << 26)
+#define CMD_SEC_CMD         (1 << 25)
+#define CMD_AFT_DAT_MASK    (1 << 24)
+#define CMD_AFT_DAT_DISABLE 0
+#define CMD_AFT_DAT_ENABLE  (1 << 24)
+#define CMD_TRANS_SIZE_SHIFT        20
+#define CMD_TRANS_SIZE_PAGE 8

Please use proper namespacing on symbols defined in headers.

Quoted 3 lines

[Jim] Scott, I don't know what you meant. Could you give me an example?
Thanks.
The names like AFT_DAT_MASK, TRANS_SIZE_SHIFT, and TRANS_SIZE_PAGE come

from field names of register in data sheet.

Names in your data sheet are presumably unique within that chip, but not within U-Boot as a whole. Names like CMD_CLE, CMD_TX, etc. are particularly likely to conflict (at least, if at least one other driver were similarly loose with its naming).

Prefix these with TEGRA_ or TEGRA_NAND_

-Scott

Hi Scott,

On Mon, May 21, 2012 at 8:47 AM, Scott Wood <[email protected] <mailto:[email protected]>> wrote:

On 05/21/2012 05:07 AM, Jim Lin wrote:
>> diff --git a/drivers/mtd/nand/tegra2_nand.h b/drivers/mtd/nand/tegra2_nand.h
>> new file mode 100644
>> index 0000000..7e74be7
>> --- /dev/null
>> +++ b/drivers/mtd/nand/tegra2_nand.h
>> @@ -0,0 +1,257 @@
>> +/*
>> + * (C) Copyright 2011 NVIDIA Corporation <www.nvidia.com
<http://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
>> + */
>> +
>> +/* register offset */
>> +#define COMMAND_0 0x00
>> +#define CMD_GO (1 << 31)
>> +#define CMD_CLE (1 << 30)
>> +#define CMD_ALE (1 << 29)
>> +#define CMD_PIO (1 << 28)
>> +#define CMD_TX (1 << 27)
>> +#define CMD_RX (1 << 26)
>> +#define CMD_SEC_CMD (1 << 25)
>> +#define CMD_AFT_DAT_MASK (1 << 24)
>> +#define CMD_AFT_DAT_DISABLE 0
>> +#define CMD_AFT_DAT_ENABLE (1 << 24)
>> +#define CMD_TRANS_SIZE_SHIFT 20
>> +#define CMD_TRANS_SIZE_PAGE 8
>
> Please use proper namespacing on symbols defined in headers.

>
> [Jim] Scott, I don't know what you meant. Could you give me an example?
> Thanks.
> The names like AFT_DAT_MASK, TRANS_SIZE_SHIFT, and TRANS_SIZE_PAGE come from field names of register in data sheet.

Names in your data sheet are presumably unique within that chip, but not within U-Boot as a whole. Names like CMD_CLE, CMD_TX, etc. are particularly likely to conflict (at least, if at least one other driver were similarly loose with its naming).

Prefix these with TEGRA_ or TEGRA_NAND_

I'm not thrilled with this idea. It adds a lot of needless boilerplate to the code IMO.

This header can only be included by the tegra NAND driver after all. Please can you reconsider this?

Quoted 18 lines · Simon Glass

Hi Scott,

On Mon, May 21, 2012 at 8:47 AM, Scott Wood <[email protected] <mailto:[email protected]>> wrote:

    On 05/21/2012 05:07 AM, Jim Lin wrote:
    >> diff --git a/drivers/mtd/nand/tegra2_nand.h
    b/drivers/mtd/nand/tegra2_nand.h
    >> new file mode 100644
    >> index 0000000..7e74be7
    >> --- /dev/null
    >> +++ b/drivers/mtd/nand/tegra2_nand.h
    >> @@ -0,0 +1,257 @@
    >> +/*
    >> + * (C) Copyright 2011 NVIDIA Corporation <www.nvidia.com
    <http://www.nvidia.com>>
    >> + *
    >> + * See file CREDITS for list of people who contributed to this

Quoted 8 lines

    >> + * 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,

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

+ * 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

Quoted 23 lines

Quoted 23 lines

Quoted 9 lines

+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */

drivers/mtd/nand/tegra2_nand.h new file +5 -0

+
+/* register offset */
+#define COMMAND_0           0x00
+#define CMD_GO                      (1 << 31)
+#define CMD_CLE                     (1 << 30)

drivers/mtd/nand/tegra2_nand.h line 36

Quoted 10 lines

drivers/mtd/nand/tegra2_nand.h new file +10 -0

+#define CMD_ALE                     (1 << 29)
+#define CMD_PIO                     (1 << 28)
+#define CMD_TX                      (1 << 27)
+#define CMD_RX                      (1 << 26)
+#define CMD_SEC_CMD         (1 << 25)
+#define CMD_AFT_DAT_MASK    (1 << 24)
+#define CMD_AFT_DAT_DISABLE 0
+#define CMD_AFT_DAT_ENABLE  (1 << 24)
+#define CMD_TRANS_SIZE_SHIFT        20
+#define CMD_TRANS_SIZE_PAGE 8

Please use proper namespacing on symbols defined in headers.

Quoted 7 lines

Quoted 1 line

[Jim] Scott, I don't know what you meant. Could you give me an

example?
> Thanks.
> The names like AFT_DAT_MASK, TRANS_SIZE_SHIFT, and TRANS_SIZE_PAGE come from field names of register in data sheet.

Names in your data sheet are presumably unique within that chip, but

not

Quoted 2 lines

within U-Boot as a whole. Names like CMD_CLE, CMD_TX, etc. are particularly likely to conflict (at least, if at least one other

driver

Quoted 5 lines

were similarly loose with its naming).

Prefix these with TEGRA_ or TEGRA_NAND_

Quoted 5 lines

I'm not thrilled with this idea. It adds a lot of needless boilerplate to the code IMO.

This header can only be included by the tegra NAND driver after all. Please can you reconsider this?

If it can only be included in the NAND driver, why does it need to be in a header at all?

-Scott

Regards,

Quoted 68 lines · Simon Glass

Hi Scott,

On Mon, May 21, 2012 at 8:47 AM, Scott Wood <[email protected] <mailto:[email protected]>> wrote:

On 05/21/2012 05:07 AM, Jim Lin wrote:
>> diff --git a/drivers/mtd/nand/tegra2_nand.h b/drivers/mtd/nand/tegra2_nand.h
>> new file mode 100644
>> index 0000000..7e74be7
>> --- /dev/null
>> +++ b/drivers/mtd/nand/tegra2_nand.h
>> @@ -0,0 +1,257 @@
>> +/*
>> + * (C) Copyright 2011 NVIDIA Corporation <www.nvidia.com
<http://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
>> + */
>> +
>> +/* register offset */
>> +#define COMMAND_0 0x00
>> +#define CMD_GO (1 << 31)
>> +#define CMD_CLE (1 << 30)
>> +#define CMD_ALE (1 << 29)
>> +#define CMD_PIO (1 << 28)
>> +#define CMD_TX (1 << 27)
>> +#define CMD_RX (1 << 26)
>> +#define CMD_SEC_CMD (1 << 25)
>> +#define CMD_AFT_DAT_MASK (1 << 24)
>> +#define CMD_AFT_DAT_DISABLE 0
>> +#define CMD_AFT_DAT_ENABLE (1 << 24)
>> +#define CMD_TRANS_SIZE_SHIFT 20
>> +#define CMD_TRANS_SIZE_PAGE 8
>
> Please use proper namespacing on symbols defined in headers.

>
> [Jim] Scott, I don't know what you meant. Could you give me an example?
> Thanks.
> The names like AFT_DAT_MASK, TRANS_SIZE_SHIFT, and TRANS_SIZE_PAGE come from field names of register in data sheet.

Names in your data sheet are presumably unique within that chip, but not within U-Boot as a whole. Names like CMD_CLE, CMD_TX, etc. are particularly likely to conflict (at least, if at least one other driver were similarly loose with its naming).

Prefix these with TEGRA_ or TEGRA_NAND_

Quoted 5 lines

I'm not thrilled with this idea. It adds a lot of needless boilerplate to the code IMO.

This header can only be included by the tegra NAND driver after all. Please can you reconsider this?

If it can only be included in the NAND driver, why does it need to be in a header at all?

Quoted 8 lines

-----Original Message-----
From: Scott Wood [mailto:scottwood at freescale.com]
Sent: Thursday, April 26, 2012 6:17 AM
To: Simon Glass
Cc: U-Boot Mailing List; Tom Warren; Stephen Warren; Jim Lin; Stephen Warren
Subject: Re: [PATCH v3 6/7] tegra: nand: Add Tegra NAND driver

On 04/17/2012 01:50 PM, Simon Glass wrote:

Quoted 13 lines

drivers/mtd/nand/tegra2_nand.c line 220

Quoted 10 lines

drivers/mtd/nand/tegra2_nand.c new file +10 -0

+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_drv *info;
+
+	info = (struct nand_drv *)chip->priv;
+
+	for (i = 0; i < len / 4; i++) {
+		l = ((int *)buf)[i];

If you're assuming the buffer is 32-bit aligned, comment it. Ideally these assumptions should be stated in the interface itself...

This doesn't mean that buf needs to be 32-bit aligned. It only says each write is 32-bit.

Quoted 1 line

Should also comment that there's an endian dependency here.

What do you mean? Could you explain more or have an example?

Quoted 9 lines

drivers/mtd/nand/tegra2_nand.c line 1036

Quoted 7 lines

drivers/mtd/nand/tegra2_nand.c new file +7 -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)

Please consider using CONFIG_SYS_NAND_SELF_INIT.

So far I don't see the demand.

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

Quoted 21 lines · Jim Lin

Quoted 21 lines

-----Original Message-----
From: Scott Wood [mailto:scottwood at freescale.com]
Sent: Thursday, April 26, 2012 6:17 AM
To: Simon Glass
Cc: U-Boot Mailing List; Tom Warren; Stephen Warren; Jim Lin; Stephen Warren
Subject: Re: [PATCH v3 6/7] tegra: nand: Add Tegra NAND driver

drivers/mtd/nand/tegra2_nand.c line 220

Quoted 10 lines · Simon Glass

drivers/mtd/nand/tegra2_nand.c new file +10 -0

+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_drv *info;
+
+	info = (struct nand_drv *)chip->priv;
+
+	for (i = 0; i < len / 4; i++) {
+		l = ((int *)buf)[i];

If you're assuming the buffer is 32-bit aligned, comment it. Ideally these assumptions should be stated in the interface itself...

Quoted 2 lines

This doesn't mean that buf needs to be 32-bit aligned. It only says each write is 32-bit.

OK, didn't realize modern ARM could deal with unaligned accesses.

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

Included here is a proposed device tree binding with most of the properties private to "nvidia,". 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.

Quoted 9 lines · Simon Glass

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

Included here is a proposed device tree binding with most of the properties private to "nvidia,". 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.

I've been working on getting a similar driver up and running in the Linux kernel. While I can successfully read data from the flash (my hardware has the same Hynix chip) I've run into a bit of a problem with the partition tables.

That however doesn't produce the expected results. Looking at the UART output produced by the bootstrap fastboot.bin it looks like it's actually allocating more blocks than necessary.

Both of these issues lead to the problem that if I encode the same partition information in the DT, the kernel's representation will not match what the nvflash tool did.

Yes, I'd recommend not putting information in DT that can be easily extracted from the partition table on the device itself.

Out of curiosity though, why do you even need the Tegra PT support; I'd assume in a situation like this, you'd just replace the whole flash with BCT, PT, EBT at once rather than attempting to update just parts of it (since the BCT must be updated when writing a new EBT content anyway), so the existing partition layout wouldn't matter, and the whole device could just be treated raw.

Quoted 2 lines · Stephen Warren

Yes, I'd recommend not putting information in DT that can be easily extracted from the partition table on the device itself.

The problem is that neither the format of the BCT nor that of the PT is documented anywhere. It seems like the BCT contains a reference to where in the flash the PT starts but I wasn't able to find out where.

Actually I'd need more than those three partitions. I need at least five, optimally six. In addition to BCT, PT and EBT I need one for the uImage and the root filesystem.

As I said before, the biggest problem with updating the whole content is that there's no documentation about either the BCT or the PT. There's cbootimage on gitorious that has some information about the BCT, but it's incomplete.

Quoted 3 lines · Thierry Reding

The problem is that neither the format of the BCT nor that of the PT is documented anywhere. It seems like the BCT contains a reference to where in the flash the PT starts but I wasn't able to find out where.

I have filed an internal bug to request this information be added to the TRM. We will see what happens.

Quoted 3 lines

Actually I'd need more than those three partitions. I need at least five, optimally six. In addition to BCT, PT and EBT I need one for the uImage and the root filesystem.

Oh I see; you're mingling the boot flash and filesystem into a single memory device. It simpler (but I imagine more costly) not too:-)

Quoted 3 lines

As I said before, the biggest problem with updating the whole content is that there's no documentation about either the BCT or the PT. There's cbootimage on gitorious that has some information about the BCT, but it's incomplete.

Out of curiosity, what's missing from cbootimage?

Quoted 4 lines · Stephen Warren

Quoted 3 lines · Thierry Reding

The problem is that neither the format of the BCT nor that of the PT is documented anywhere. It seems like the BCT contains a reference to where in the flash the PT starts but I wasn't able to find out where.

Quoted 5 lines

Quoted 3 lines

As I said before, the biggest problem with updating the whole content is that there's no documentation about either the BCT or the PT. There's cbootimage on gitorious that has some information about the BCT, but it's incomplete.

Out of curiosity, what's missing from cbootimage?

It's missing support for PT. That may not be necessary in a setup where we initialize the NAND from Linux user space, though, so maybe it would be enough.

Quoted 5 lines · Thierry Reding

Quoted 4 lines · Stephen Warren

Quoted 3 lines · Thierry Reding

The problem is that neither the format of the BCT nor that of the PT is documented anywhere. It seems like the BCT contains a reference to where in the flash the PT starts but I wasn't able to find out where.

...

Quoted 9 lines

Quoted 5 lines

Quoted 3 lines

As I said before, the biggest problem with updating the whole content is that there's no documentation about either the BCT or the PT. There's cbootimage on gitorious that has some information about the BCT, but it's incomplete.

Out of curiosity, what's missing from cbootimage?

It's missing support for PT. That may not be necessary in a setup where we initialize the NAND from Linux user space, though, so maybe it would be enough.

I don't believe the Tegra boot process actually /requires/ the PT even exist. IIRC, the boot ROM searches for the BCT, and the BCT contains a pointer to the bootloader (e.g. U-Boot), so it's only at a later stage in the game that anything would care about the PT. As such, worrying about the PT (or even including it) may be pointless.

I believe that TrimSlice's firmware recovery SD card images are created solely using cbootimage, and hence most likely have no PT, although obviously no additional partitions/file-systems on the media.

Perhaps you could define some hard-coded "MTD" partitions (e.g. the first 1MB and the rest), where the first 1MB contains BCT + U-Boot and the rest contains a regular MBR or GPT partition table. I /think/ there may even be a kernel command-line option to define the MTD partition layout?

Or, you could probably even get away with using a GPT for the entire NAND by placing just the secondary GPT at the end of the NAND, putting the BCT+U-Boot right at the start, and defining a GPT partition to protect/cover BCT+U-Boot. I vaguely recall trying this on some Tegra device, but I may be wrong.