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

2012-04-17 Thread Simon Glass 
From: Jim Lin 

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: Jim Lin 
Signed-off-by: Simon Glass 
---
Changes in v2:
- Added comment about the behaviour of the 'resp' register
- Call set_bus_width_page_size() at init to report errors earlier
- Change set_bus_width_page_size() to return an error when needed
- Change timing structure member to u32 to match device tree
- Check for supported bus width in board_nand_init()
- Fix tegra nand header file to remove BIT defines
- Implement a dummy nand_select_chip() instead of nand_hwcontro()
- Make nand_command() display an error on an unknown command
- Minor code tidy-ups in driver for style
- Move cache logic into a separate dma_prepare() function
- Remove CMD_TRANS_SIZE_BYTESx enum
- Remove space after casts
- Remove use of 'register' variables
- Rename struct nand_info to struct nand_drv to avoid nand_info_t confusion
- Support 4096 byte page devices, drop 1024 and 2048
- Tidy up nand_waitfor_cmd_completion() logic
- Update NAND binding to add "nvidia," prefix
- Use s32 for device tree integer values

Changes in v3:
- Update fdt binding to make everything Nvidia-specific

 arch/arm/include/asm/arch-tegra2/tegra2.h |1 +
 drivers/mtd/nand/Makefile |1 +
 drivers/mtd/nand/tegra2_nand.c| 1095 +
 drivers/mtd/nand/tegra2_nand.h|  257 +++
 include/fdtdec.h  |1 +
 lib/fdtdec.c  |1 +
 6 files changed, 1356 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 1d1b628..7efccf8 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -61,6 +61,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 000..55dd16f
--- /dev/null
+++ b/drivers/mtd/nand/tegra2_nand.c
@@ -0,0 +1,1095 @@
+/*
+ * Copyright (c) 2011 The Chromium OS Authors.
+ * (C) Copyright 2011 NVIDIA Corporation 
+ * (C) Copyright 2006 Detlev Zundel, d...@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
+ */
+#define DEBUG
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "tegra2_nand.h"
+
+DECLARE_GLOBAL_DATA_PTR;
+
+#define NAND_CMD_TIMEOUT_MS10
+
+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_

Re: [U-Boot] [PATCH v3 6/7] tegra: nand: Add Tegra NAND driver

2012-05-21 Thread Scott Wood 
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 000..7e74be7
>> --- /dev/null
>> +++ b/drivers/mtd/nand/tegra2_nand.h
>> @@ -0,0 +1,257 @@
>> +/*
>> + * (C) Copyright 2011 NVIDIA Corporation 
>> + *
>> + * 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_SHIFT20
>> +#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_

-Scott

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3 6/7] tegra: nand: Add Tegra NAND driver

2012-05-22 Thread Simon Glass 
Hi Scott,

On Mon, May 21, 2012 at 8:47 AM, Scott Wood  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 000..7e74be7
> >> --- /dev/null
> >> +++ b/drivers/mtd/nand/tegra2_nand.h
> >> @@ -0,0 +1,257 @@
> >> +/*
> >> + * (C) Copyright 2011 NVIDIA Corporation 
> >> + *
> >> + * 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_SHIFT20
> >> +#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?


>
> -Scott
>
>
Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3 6/7] tegra: nand: Add Tegra NAND driver

2012-05-22 Thread Scott Wood 
On 05/22/2012 03:04 PM, Simon Glass wrote:
> Hi Scott,
> 
> On Mon, May 21, 2012 at 8:47 AM, Scott Wood  > 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 000..7e74be7
> >> --- /dev/null
> >> +++ b/drivers/mtd/nand/tegra2_nand.h
> >> @@ -0,0 +1,257 @@
> >> +/*
> >> + * (C) Copyright 2011 NVIDIA Corporation  >
> >> + *
> >> + * 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_SHIFT20
> >> +#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?

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

-Scott

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3 6/7] tegra: nand: Add Tegra NAND driver

2012-05-22 Thread Simon Glass 
Hi Scott,

On Tue, May 22, 2012 at 1:06 PM, Scott Wood  wrote:

> On 05/22/2012 03:04 PM, Simon Glass wrote:
> > Hi Scott,
> >
> > On Mon, May 21, 2012 at 8:47 AM, Scott Wood  > > 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 000..7e74be7
> > >> --- /dev/null
> > >> +++ b/drivers/mtd/nand/tegra2_nand.h
> > >> @@ -0,0 +1,257 @@
> > >> +/*
> > >> + * (C) Copyright 2011 NVIDIA Corporation  > >
> > >> + *
> > >> + * 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_SHIFT20
> > >> +#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?
>
> If it can only be included in the NAND driver, why does it need to be in
> a header at all?
>

It doesn't - it's just a convenient way to separate out the definitions
from the rest of the code. Quite a lot of drivers in U-Boot have private .h
files.


>
> -Scott
>
> Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3 6/7] tegra: nand: Add Tegra NAND driver

2012-05-22 Thread Scott Wood 
On 05/22/2012 03:06 PM, Scott Wood wrote:
> On 05/22/2012 03:04 PM, Simon Glass wrote:
>> Hi Scott,
>>
>> On Mon, May 21, 2012 at 8:47 AM, Scott Wood > > 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 000..7e74be7
>> >> --- /dev/null
>> >> +++ b/drivers/mtd/nand/tegra2_nand.h
>> >> @@ -0,0 +1,257 @@
>> >> +/*
>> >> + * (C) Copyright 2011 NVIDIA Corporation > >
>> >> + *
>> >> + * 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_SHIFT20
>> >> +#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?
> 
> If it can only be included in the NAND driver, why does it need to be in
> a header at all?

That said, I see it's in drivers/mtd/nand/ and not include/, so if you
really want to structure it that way, it's OK with me.

-Scott

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3 6/7] tegra: nand: Add Tegra NAND driver

2012-04-25 Thread Scott Wood 
On 04/17/2012 01:50 PM, Simon Glass wrote:
> +#define DEBUG

Did you mean to leave this in?

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

This isn't the default read function, it's the tegra read function.

> +static uint8_t read_byte(struct mtd_info *mtd)
> +{
> + struct nand_chip *chip = mtd->priv;
> + u32 dword_read;
> + struct nand_drv *info;
> +
> + info = (struct nand_drv *)chip->priv;
> + if (info->pio_byte_index > 3)
> + return 0;
> +
> + /* We can read this data multiple times and get the same word */
> + dword_read = readl(&info->reg->resp);
> + dword_read = dword_read >> (8 * info->pio_byte_index);
> + info->pio_byte_index++;
> + return (uint8_t)dword_read;
> +}

So you only read up to 4 bytes via this method?  If this is really all
that's ever needed, please add a comment to that effect.

> +/**
> + * [DEFAULT] Write buffer to chip
> + *
> + * @param mtdMTD device structure
> + * @param bufdata buffer
> + * @param lennumber 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_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...

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

> + writel(l, &info->reg->resp);
> + writel(CMD_GO | CMD_PIO | CMD_TX |
> + ((4 - 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");

You need to wait for completion every 4 bytes?  Where is the DMA?

> + case NAND_CMD_RNDOUT:
> + printf("%s: Unsupported RNDOUT command\n", __func__);
> + return;
[snip]
> + default:
> + printf("%s: Unsupported command %d\n", __func__, command);
> + return;
> + }

Doesn't the print in the default case already handle RNDOUT?

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

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

What is the difference between an "A" fail and a "B" fail?  Do you
really want to do the blank_check twice?

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

Should use virt_to_phys(), even if it currently makes no difference on
this platform.

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

Make this static.

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

> diff --git a/drivers/mtd/nand/tegra2_nand.h b/drivers/mtd/nand/tegra2_nand.h
> new file mode 100644
> index 000..7e74be7
> --- /dev/null
> +++ b/drivers/mtd/nand/tegra2_nand.h
> @@ -0,0 +1,257 @@
> +/*
> + * (C

Re: [U-Boot] [PATCH v3 6/7] tegra: nand: Add Tegra NAND driver

2012-07-05 Thread Scott Wood 
On 07/04/2012 02:46 AM, Jim Lin wrote:
>> -Original Message-
>> From: Scott Wood [mailto:scottw...@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:
>>> +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.

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

>> Should also comment that there's an endian dependency here.
> What do you mean? Could you explain more or have an example?

You load a value using host endianness, and store it using a little
endian accessor.  This would be fine if it represented a real 32-bit
integer, but it's really a sequence of bytes that should not be swapped.

It's not a big deal if you don't see the driver ever being used with a
big endian host, but a comment would be nice.

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

I'd like to see the old way go away eventually.

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

Please try to get rid of this.  This is a public mailing list.

-Scott

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3 6/7] tegra: nand: Add Tegra NAND driver

2012-07-06 Thread Stephen Warren 
On 07/05/2012 07:28 PM, Scott Wood wrote:
> On 07/04/2012 02:46 AM, Jim Lin wrote:
>>> -Original Message-
>>> From: Scott Wood [mailto:scottw...@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:
 +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.
> 
> OK, didn't realize modern ARM could deal with unaligned accesses.

I'd like to see some more research here; I know that recent printk
changes in the ARM kernel caused unaligned accesses which in turn caused
significant problems (including on Tegra). This might only have been for
64-bit accesses, so perhaps 32-bit accesses don't have an issue, but if
you could confirm this Jim, that'd be great.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot