Re: [U-Boot] [PATCH 07/12 v2] ARM: OMAP3: Add memory and syslib common files, add NAND support

2008-10-07 Thread Nishanth Menon
Dirk Behme said the following on 10/07/2008 04:42 AM:

 Scott Wood wrote:
 On Fri, Oct 03, 2008 at 12:40:25PM +0200, [EMAIL PROTECTED]
 wrote:

 +#include common.h
 +#include asm/io.h
 +#include asm/arch/mem.h
 +#include linux/mtd/nand_ecc.h
 +
 +#if defined(CONFIG_CMD_NAND)
 +
 +#include nand.h


 Move the #ifdef to the Makefile.

 Did this. Additionally, I moved OMAP3 NAND driver to
 drivers/mtd/nand/omap3.c.
I would recommend having a nand_omap_gpmc.c if the driver can be made
generic enough.

 +/*
 + * nand_read_buf16 - [DEFAULT] read chip data into buffer
 + * @mtd:MTD device structure
 + * @buf:buffer to store date
 + * @len:number of bytes to read
 + *
 + * Default read function for 16bit buswith
 + */
 +static void omap_nand_read_buf(struct mtd_info *mtd, u_char *buf,
 int len)
 +{
 +int i;
 +struct nand_chip *this = mtd-priv;
 +u16 *p = (u16 *) buf;
 +len = 1;
 +
 +for (i = 0; i  len; i++)
 +p[i] = readw(this-IO_ADDR_R);
 +}


 How does this differ from the default implementation?

 It doesn't differ ;)

 So I removed this and tried to use default nand_read_buf16() instead:

 nand-read_buf = nand_read_buf16;

 in board_nand_init(). But this doesn't compile as nand_read_buf16() is
 static in nand_base.c. How do I use it in OMAP3 NAND driver? Marked it
 as FIXME in patch.
Probably does not need an explicit initialization, mtd nand_scan should
populate that. in fact IMHO, the read/write buf ops might be duplicating
mtd's implementation..  :(



 +/*
 + *  omap_calculate_ecc - Generate non-inverted ECC bytes.
 + *
 + *  Using noninverted ECC can be considered ugly since writing a blank
 + *  page ie. padding will clear the ECC bytes. This is no problem as
 + *  long nobody is trying to write data on the seemingly unused page.
 + *  Reading an erased page will produce an ECC mismatch between
 + *  generated and read ECC bytes that has to be dealt with separately.


 Is this a hardware limitation?  If so, say so in the comment.  If not,
 why do it this way?

 Don't know.

 All: Any help?
The issue is simple: assume we read a page of 0xFF's(fresh erased), IF
using H/w ECC engine within GPMC, the result of read will be 0x0 while
the ECC offsets of the spare area will be 0xFF which will result in an
ECC mismatch.
 +.eccbytes = 12,
 +.eccpos = {
 +   2, 3, 4, 5,
 +   6, 7, 8, 9,
 +   10, 11, 12, 13},
 +.oobfree = { {20, 50} }/* don't care */


 Bytes 64-69 of a 64-byte OOB are free?
 What don't we care about?
 +static struct nand_ecclayout hw_nand_oob_64 = {

 Don't know (or understand?).

 All: Any help?
I do not get it either.. ECCPOS is in offset bytes, and oobfree should
be {.offset=20,.length=44} /*I always hated struct initialization done
as above..*/, but then,

 ===
 --- u-boot-arm.orig/drivers/mtd/nand/nand_base.c
 +++ u-boot-arm/drivers/mtd/nand/nand_base.c
 @@ -2730,6 +2730,151 @@ int nand_scan_tail(struct mtd_info *mtd)
 return 0;
 }

 +#if defined(CONFIG_OMAP)  (defined(CONFIG_OMAP3_BEAGLE) \
 +|| defined(CONFIG_OMAP3_EVM) || defined(CONFIG_OVERO))
 +void nand_switch_ecc(struct mtd_info *mtd)


 NACK.  First, explain why you need to be able to dynamically switch ECC
 modes.

 Two topics here, changes in cmd_nand.c and nand_base.c.

 cmd_nand.c:

 We need to be able to switch ECC at runtime cause some images have to
 be written to NAND with HW ECC and some with SW ECC. This depends on
 what user (reader) of these parts expect.

 OMAP3 has a boot ROM which is able to read a second level loader
 (called x-loader) from NAND and start/execute it. This 2nd level
 loader has to be written by U-Boot using HW ECC as ROM code does HW
 ECC to read the image. All other parts, e.g. Linux kernel, use SW ECC
 as default. For this we have to use SW ECC to write images, then.

 Therefore we add an additional user command in cmd_nand.c to switch
 ECC depending on what user wants to write.
why not use h/w ecc which rom code understands in kernel and u-boot. H/w
ecc will be faster in comparison to doing s/w ecc and there is good
support in MTD to do it, then there would be no reason for s/w ecc IMHO..

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


Re: [U-Boot] [PATCH 07/12 v2] ARM: OMAP3: Add memory and syslib common files, add NAND support

2008-10-07 Thread Dirk Behme


Unfortunately, except Nishanth's comments, I didn't get any further 
help (e.g. from TI) for this yet. So I started to look at this myself. 
Please forgive everything I missed as I'm no NAND expert. Maybe you 
like to explain some additional details regarding what I missed ;)


First version of updated NAND patch in attachment. This is RFC only. 
I'd like to fix anything still open. When this is done and when 
everybody is fine with this, I will send an updated v3 of whole OMAP3 
patch set including the then final NAND patch (and the fixes for all 
other comments).


Scott Wood wrote:

On Fri, Oct 03, 2008 at 12:40:25PM +0200, [EMAIL PROTECTED] wrote:


+#include common.h
+#include asm/io.h
+#include asm/arch/mem.h
+#include linux/mtd/nand_ecc.h
+
+#if defined(CONFIG_CMD_NAND)
+
+#include nand.h



Move the #ifdef to the Makefile.


Did this. Additionally, I moved OMAP3 NAND driver to 
drivers/mtd/nand/omap3.c.



+/*
+ * nand_read_buf16 - [DEFAULT] read chip data into buffer
+ * @mtd:   MTD device structure
+ * @buf:   buffer to store date
+ * @len:   number of bytes to read
+ *
+ * Default read function for 16bit buswith
+ */
+static void omap_nand_read_buf(struct mtd_info *mtd, u_char *buf, int len)
+{
+   int i;
+   struct nand_chip *this = mtd-priv;
+   u16 *p = (u16 *) buf;
+   len = 1;
+
+   for (i = 0; i  len; i++)
+   p[i] = readw(this-IO_ADDR_R);
+}



How does this differ from the default implementation?


It doesn't differ ;)

So I removed this and tried to use default nand_read_buf16() instead:

nand-read_buf = nand_read_buf16;

in board_nand_init(). But this doesn't compile as nand_read_buf16() is 
static in nand_base.c. How do I use it in OMAP3 NAND driver? Marked it 
as FIXME in patch.



+static void omap_nand_write_buf(struct mtd_info *mtd, const uint8_t *buf,
+   int len)
+{
+   int i;
+   int j = 0;
+   struct nand_chip *chip = mtd-priv;
+
+   for (i = 0; i  len; i++) {
+   writeb(buf[i], chip-IO_ADDR_W);
+   for (j = 0; j  10; j++) ;
+   }
+
+}
+
+/*
+ * omap_nand_read_buf - read data from NAND controller into buffer
+ * @mtd:MTD device structure
+ * @buf:buffer to store date
+ * @len:number of bytes to read
+ *
+ */
+static void omap_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
+{
+   int i;
+   int j = 0;
+   struct nand_chip *chip = mtd-priv;
+
+   for (i = 0; i  len; i++) {
+   buf[i] = readb(chip-IO_ADDR_R);
+   while (GPMC_BUF_EMPTY == (readl(GPMC_STATUS)  GPMC_BUF_FULL));
+   }
+}



I'm a bit confused... with 16-bit NAND, you check GMPC_STATUS[BUF_FULL]
when writing, but with 8-bit NAND, you check it when reading?  And 8-bit
writes have an arbitrary, cpu-speed-dependent delay, and 16-bit reads
have no delay at all?


Removed 8-bit stuff completely as it isn't used.


+static void omap_hwecc_init(struct nand_chip *chip)
+{
+   unsigned long val = 0x0;
+
+   /* Init ECC Control Register */
+   /* Clear all ECC  | Enable Reg1 */
+   val = ((0x0001  8) | 0x0001);
+   __raw_writel(val, GPMC_BASE + GPMC_ECC_CONTROL);
+   __raw_writel(0x3fcff000, GPMC_BASE + GPMC_ECC_SIZE_CONFIG);
+}



Why raw?


Removed all _raw_ . At ARM, it seems that __raw_writex()/readx() are 
the same as  writex()/readx().



+/*
+ * omap_correct_data - Compares the ecc read from nand spare area with
+ * ECC registers values
+ * and corrects one bit error if it has occured
+ * @mtd:MTD device structure
+ * @dat:page data
+ * @read_ecc:   ecc read from nand flash
+ * @calc_ecc:   ecc read from ECC registers
+ */
+static int omap_correct_data(struct mtd_info *mtd, u_char *dat,
+u_char *read_ecc, u_char *calc_ecc)
+{
+   return 0;
+}



This obviously is not correcting anything.  If the errors are corrected
automatically by the controller, say so in the comments.


I replaced this with the version from Nishanth's U-Boot v2:

http://git.denx.de/?p=u-boot/u-boot-v2.git;a=blob;f=drivers/nand/nand_omap_gpmc.c;h=afd676a74194c63a1e3cf581d210235a9d4c78ba;hb=HEAD


+/*
+ *  omap_calculate_ecc - Generate non-inverted ECC bytes.
+ *
+ *  Using noninverted ECC can be considered ugly since writing a blank
+ *  page ie. padding will clear the ECC bytes. This is no problem as
+ *  long nobody is trying to write data on the seemingly unused page.
+ *  Reading an erased page will produce an ECC mismatch between
+ *  generated and read ECC bytes that has to be dealt with separately.



Is this a hardware limitation?  If so, say so in the comment.  If not,
why do it this way?


Don't know.

All: Any help?


+ *  @mtd:  MTD structure
+ *  @dat:  unused
+ *  @ecc_code: ecc_code buffer
+ */
+static int omap_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
+   

Re: [U-Boot] [PATCH 07/12 v2] ARM: OMAP3: Add memory and syslib common files, add NAND support

2008-10-07 Thread Scott Wood
On Tue, Oct 07, 2008 at 11:42:38AM +0200, Dirk Behme wrote:
 Is it OK if config gets written before control, or if this whole thing
 gets done out of order with respect to other raw writes?

 Hmm. I replaced this with the version from Nishanth's U-Boot v2 (see  
 link above). If this isn't ok, can you explain a little more?

I was referring to the raw accessors, which you removed, so it's OK.

 I.e. with patch in attachment we don't touch nand_base.c any more, but  
 still have an #ifdeffed user command in cmd_nand.c. If you don't like  
 this, please give a hint how to better do this.

If at all possible, fix Linux to accept hardware ECC.

Otherwise, either add something to the MTD API that the nand command can
call, or have your own board-specific command defined in the board file. 
Don't put platform-specific ifdefs in generic files.

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


Re: [U-Boot] [PATCH 07/12 v2] ARM: OMAP3: Add memory and syslib common files, add NAND support

2008-10-07 Thread Scott Wood
On Tue, Oct 07, 2008 at 06:25:11AM -0500, Nishanth Menon wrote:
 Dirk Behme said the following on 10/07/2008 04:42 AM:
  It doesn't differ ;)
 
  So I removed this and tried to use default nand_read_buf16() instead:
 
  nand-read_buf = nand_read_buf16;
 
  in board_nand_init(). But this doesn't compile as nand_read_buf16() is
  static in nand_base.c. How do I use it in OMAP3 NAND driver? Marked it
  as FIXME in patch.
 Probably does not need an explicit initialization, mtd nand_scan should
 populate that.

Correct, NULL methods will be filled in with defaults if applicable.

  +/*
  + *  omap_calculate_ecc - Generate non-inverted ECC bytes.
  + *
  + *  Using noninverted ECC can be considered ugly since writing a blank
  + *  page ie. padding will clear the ECC bytes. This is no problem as
  + *  long nobody is trying to write data on the seemingly unused page.
  + *  Reading an erased page will produce an ECC mismatch between
  + *  generated and read ECC bytes that has to be dealt with separately.
 
 
  Is this a hardware limitation?  If so, say so in the comment.  If not,
  why do it this way?
 
  Don't know.
 
  All: Any help?
 The issue is simple: assume we read a page of 0xFF's(fresh erased), IF
 using H/w ECC engine within GPMC, the result of read will be 0x0 while
 the ECC offsets of the spare area will be 0xFF which will result in an
 ECC mismatch.

Right, I'd just like to see an explicit statement that this is the only way
to do HW ECC that the hardware supports (following verification of that
fact, of course), along with a pointer to where in the code the ECC error
when reading an empty page is dealt with.

  +.eccbytes = 12,
  +.eccpos = {
  +   2, 3, 4, 5,
  +   6, 7, 8, 9,
  +   10, 11, 12, 13},
  +.oobfree = { {20, 50} }/* don't care */
 
 
  Bytes 64-69 of a 64-byte OOB are free?
  What don't we care about?
  +static struct nand_ecclayout hw_nand_oob_64 = {
 
  Don't know (or understand?).
 
  All: Any help?
 I do not get it either.. ECCPOS is in offset bytes, and oobfree should
 be {.offset=20,.length=44} /*I always hated struct initialization done
 as above..*/, but then,

Why not offset 14, length 50?

  We need to be able to switch ECC at runtime cause some images have to
  be written to NAND with HW ECC and some with SW ECC. This depends on
  what user (reader) of these parts expect.
 
  OMAP3 has a boot ROM which is able to read a second level loader
  (called x-loader) from NAND and start/execute it. This 2nd level
  loader has to be written by U-Boot using HW ECC as ROM code does HW
  ECC to read the image. All other parts, e.g. Linux kernel, use SW ECC
  as default. For this we have to use SW ECC to write images, then.
 
  Therefore we add an additional user command in cmd_nand.c to switch
  ECC depending on what user wants to write.
 why not use h/w ecc which rom code understands in kernel and u-boot. H/w
 ecc will be faster in comparison to doing s/w ecc and there is good
 support in MTD to do it, then there would be no reason for s/w ecc IMHO..

Agreed.

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


Re: [U-Boot] [PATCH 07/12 v2] ARM: OMAP3: Add memory and syslib common files, add NAND support

2008-10-07 Thread Scott Wood
Menon, Nishanth wrote:
 I do not get it either.. ECCPOS is in offset bytes, and oobfree should
 be {.offset=20,.length=44} /*I always hated struct initialization done
 as above..*/, but then,
 Why not offset 14, length 50?
 How about this part being used by ubi/jffs2 or some fs.. I cant think 
 off-hand if there an implication, but yep, I suspect there is no reason why 
 not..

The higher levels allocate their OOB usage from the regions that the 
low-level driver claims are free.

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


Re: [U-Boot] [PATCH 07/12 v2] ARM: OMAP3: Add memory and syslib common files, add NAND support

2008-10-07 Thread Dirk Behme
Scott Wood wrote:
 On Tue, Oct 07, 2008 at 11:42:38AM +0200, Dirk Behme wrote:
 
Is it OK if config gets written before control, or if this whole thing
gets done out of order with respect to other raw writes?

Hmm. I replaced this with the version from Nishanth's U-Boot v2 (see  
link above). If this isn't ok, can you explain a little more?
 
 
 I was referring to the raw accessors, which you removed, so it's OK.

:)

I.e. with patch in attachment we don't touch nand_base.c any more, but  
still have an #ifdeffed user command in cmd_nand.c. If you don't like  
this, please give a hint how to better do this.
 
 
 If at all possible, fix Linux to accept hardware ECC.

Yes, as mentioned by Nishanth, too, this will be possible. But I don't 
think in the short term, and not with breaking compatibility with 
kernels already out there. So for the moment I like to have both 
options, HW  SW ECC.

 Otherwise, either add something to the MTD API that the nand command can
 call, or have your own board-specific command defined in the board file. 
 Don't put platform-specific ifdefs in generic files.

Do you have any example how to extend MTD API and/or can link to 
example where board-specific command is already used?

Thanks

Dirk

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


Re: [U-Boot] [PATCH 07/12 v2] ARM: OMAP3: Add memory and syslib common files, add NAND support

2008-10-07 Thread Dirk Behme
Scott Wood wrote:
 On Tue, Oct 07, 2008 at 06:25:11AM -0500, Nishanth Menon wrote:
 
Dirk Behme said the following on 10/07/2008 04:42 AM:

It doesn't differ ;)

So I removed this and tried to use default nand_read_buf16() instead:

nand-read_buf = nand_read_buf16;

in board_nand_init(). But this doesn't compile as nand_read_buf16() is
static in nand_base.c. How do I use it in OMAP3 NAND driver? Marked it
as FIXME in patch.

Probably does not need an explicit initialization, mtd nand_scan should
populate that.
 
 
 Correct, NULL methods will be filled in with defaults if applicable.

ok, will do so, this is easy :)

+/*
+ *  omap_calculate_ecc - Generate non-inverted ECC bytes.
+ *
+ *  Using noninverted ECC can be considered ugly since writing a blank
+ *  page ie. padding will clear the ECC bytes. This is no problem as
+ *  long nobody is trying to write data on the seemingly unused page.
+ *  Reading an erased page will produce an ECC mismatch between
+ *  generated and read ECC bytes that has to be dealt with separately.


Is this a hardware limitation?  If so, say so in the comment.  If not,
why do it this way?

Don't know.

All: Any help?

The issue is simple: assume we read a page of 0xFF's(fresh erased), IF
using H/w ECC engine within GPMC, the result of read will be 0x0 while
the ECC offsets of the spare area will be 0xFF which will result in an
ECC mismatch.
 
 Right, I'd just like to see an explicit statement that this is the only way
 to do HW ECC that the hardware supports (following verification of that
 fact, of course), along with a pointer to where in the code the ECC error
 when reading an empty page is dealt with.

Will add Nishanth's explanation to comment and check code for this.

+.eccbytes = 12,
+.eccpos = {
+   2, 3, 4, 5,
+   6, 7, 8, 9,
+   10, 11, 12, 13},
+.oobfree = { {20, 50} }/* don't care */


Bytes 64-69 of a 64-byte OOB are free?
What don't we care about?

+static struct nand_ecclayout hw_nand_oob_64 = {

Don't know (or understand?).

All: Any help?

I do not get it either.. ECCPOS is in offset bytes, and oobfree should
be {.offset=20,.length=44} /*I always hated struct initialization done
as above..*/, but then,
 
 
 Why not offset 14, length 50?

Seems I need a closer look what we are talking about here ;)

We need to be able to switch ECC at runtime cause some images have to
be written to NAND with HW ECC and some with SW ECC. This depends on
what user (reader) of these parts expect.

OMAP3 has a boot ROM which is able to read a second level loader
(called x-loader) from NAND and start/execute it. This 2nd level
loader has to be written by U-Boot using HW ECC as ROM code does HW
ECC to read the image. All other parts, e.g. Linux kernel, use SW ECC
as default. For this we have to use SW ECC to write images, then.

Therefore we add an additional user command in cmd_nand.c to switch
ECC depending on what user wants to write.

why not use h/w ecc which rom code understands in kernel and u-boot. H/w
ecc will be faster in comparison to doing s/w ecc and there is good
support in MTD to do it, then there would be no reason for s/w ecc IMHO..
 
 
 Agreed.

As already mentioned in previous post, I think for the the moment we 
have to go with both ways.

To summarize the open points I will look at:

a) Remove unnecessary nand-read_buf init
b) Add comment and check code for HW ecc issue with erased page
c) Fix offset 14, length 50 issue
d) Extend MTD API with a call to switch HW/SW ecc, remove 
platform-specific ifdefs in generic files

Nishanth M.: Would be nice to get some help for these changes. E.g. 
can you say if this HW ecc issue with erased page is handled correctly 
in exisiting code?

Thanks for the help

Dirk

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


Re: [U-Boot] [PATCH 07/12 v2] ARM: OMAP3: Add memory and syslib common files, add NAND support

2008-10-07 Thread Menon, Nishanth
 -Original Message-
 From: Scott Wood [mailto:[EMAIL PROTECTED]
 Sent: Tuesday, October 07, 2008 12:30 PM
 To: Nishanth Menon
 Cc: Dirk Behme; u-boot@lists.denx.de; Kamat, Nishant; Menon, Nishanth
 Subject: Re: [U-Boot] [PATCH 07/12 v2] ARM: OMAP3: Add memory and syslib
 common files, add NAND support
   +.eccbytes = 12,
   +.eccpos = {
   +   2, 3, 4, 5,
   +   6, 7, 8, 9,
   +   10, 11, 12, 13},
   +.oobfree = { {20, 50} }/* don't care */
  
  
   Bytes 64-69 of a 64-byte OOB are free?
   What don't we care about?
   +static struct nand_ecclayout hw_nand_oob_64 = {
  
   Don't know (or understand?).
  
   All: Any help?
  I do not get it either.. ECCPOS is in offset bytes, and oobfree should
  be {.offset=20,.length=44} /*I always hated struct initialization done
  as above..*/, but then,
 
 Why not offset 14, length 50?
How about this part being used by ubi/jffs2 or some fs.. I cant think off-hand 
if there an implication, but yep, I suspect there is no reason why not..

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


Re: [U-Boot] [PATCH 07/12 v2] ARM: OMAP3: Add memory and syslib common files, add NAND support

2008-10-07 Thread Scott Wood
Dirk Behme wrote:
 Otherwise, either add something to the MTD API that the nand command can
 call, or have your own board-specific command defined in the board file. 
 Don't put platform-specific ifdefs in generic files.
 
 Do you have any example how to extend MTD API

You'd need to add callbacks in the device structures; since this is a 
workaround for legacy support, and not something that is desireable in 
most cases, I'd go with the latter option of a board-specific command.

 and/or can link to 
 example where board-specific command is already used?

grep -rI U_BOOT_CMD board/

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


[U-Boot] [PATCH 07/12 v2] ARM: OMAP3: Add memory and syslib common files, add NAND support

2008-10-03 Thread dirk . behme
Subject: [PATCH 07/12 v2] ARM: OMAP3: Add memory and syslib common files, add 
NAND support

From: Dirk Behme [EMAIL PROTECTED]

Add memory and syslib common files, add NAND support

Signed-off-by: Dirk Behme [EMAIL PROTECTED]

---
Changes in version v2:

- Move common ARM Cortex A8 code to cpu/arm_cortexa8/ and OMAP3 SoC specific 
common code to cpu/arm_cortexa8/omap3 as proposed by Wolfgang.

 common/cmd_nand.c   |   29 ++
 cpu/arm_cortexa8/omap3/Makefile |2 
 cpu/arm_cortexa8/omap3/mem.c|  301 ++
 cpu/arm_cortexa8/omap3/nand.c   |  399 
 cpu/arm_cortexa8/omap3/syslib.c |   72 +++
 drivers/mtd/nand/nand_base.c|  145 ++
 examples/Makefile   |6 
 7 files changed, 951 insertions(+), 3 deletions(-)

Index: u-boot-arm/cpu/arm_cortexa8/omap3/mem.c
===
--- /dev/null
+++ u-boot-arm/cpu/arm_cortexa8/omap3/mem.c
@@ -0,0 +1,301 @@
+/*
+ * (C) Copyright 2008
+ * Texas Instruments, www.ti.com
+ *
+ * Author :
+ * Manikandan Pillai [EMAIL PROTECTED]
+ *
+ * Initial Code from:
+ * Richard Woodruff [EMAIL PROTECTED]
+ * Syed Mohammed Khasim [EMAIL PROTECTED]
+ *
+ * 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 asm/arch/bits.h
+#include asm/arch/mem.h
+#include asm/arch/sys_proto.h
+#include command.h
+
+/* Only One NAND allowed on board at a time.
+ * The GPMC CS Base for the same
+ */
+unsigned int boot_flash_base;
+unsigned int boot_flash_off;
+unsigned int boot_flash_sec;
+unsigned int boot_flash_type;
+volatile unsigned int boot_flash_env_addr;
+
+/* help common/env_flash.c */
+#ifdef ENV_IS_VARIABLE
+
+uchar(*boot_env_get_char_spec) (int index);
+int (*boot_env_init) (void);
+int (*boot_saveenv) (void);
+void (*boot_env_relocate_spec) (void);
+
+/* 16 bit NAND */
+uchar env_get_char_spec(int index);
+int env_init(void);
+int saveenv(void);
+void env_relocate_spec(void);
+extern char *env_name_spec;
+
+#if defined(CONFIG_CMD_NAND)
+u8 is_nand;
+#endif
+
+#if defined(CONFIG_CMD_ONENAND)
+u8 is_onenand;
+#endif
+
+#endif /* ENV_IS_VARIABLE */
+
+#if defined(CONFIG_CMD_NAND)
+static u32 gpmc_m_nand[GPMC_MAX_REG] = {
+   M_NAND_GPMC_CONFIG1,
+   M_NAND_GPMC_CONFIG2,
+   M_NAND_GPMC_CONFIG3,
+   M_NAND_GPMC_CONFIG4,
+   M_NAND_GPMC_CONFIG5,
+   M_NAND_GPMC_CONFIG6, 0
+};
+unsigned int nand_cs_base;
+#endif
+
+#if defined(CONFIG_CMD_ONENAND)
+static u32 gpmc_onenand[GPMC_MAX_REG] = {
+   ONENAND_GPMC_CONFIG1,
+   ONENAND_GPMC_CONFIG2,
+   ONENAND_GPMC_CONFIG3,
+   ONENAND_GPMC_CONFIG4,
+   ONENAND_GPMC_CONFIG5,
+   ONENAND_GPMC_CONFIG6, 0
+};
+unsigned int onenand_cs_base;
+
+#endif
+
+/**
+ * make_cs1_contiguous() - for es2 and above remap cs1 behind cs0 to allow
+ *  command line mem=xyz use all memory with out discontinuous support
+ *  compiled in.  Could do it at the ATAG, but there really is two banks...
+ * Called as part of 2nd phase DDR init.
+ **/
+void make_cs1_contiguous(void)
+{
+   u32 size, a_add_low, a_add_high;
+
+   size = get_sdr_cs_size(SDRC_CS0_OSET);
+   size /= SZ_32M; /* find size to offset CS1 */
+   a_add_high = (size  3)  8;   /* set up low field */
+   a_add_low = (size  0x3C)  2; /* set up high field */
+   __raw_writel((a_add_high | a_add_low), SDRC_CS_CFG);
+
+}
+
+/
+ *  mem_ok() - test used to see if timings are correct
+ * for a part. Helps in guessing which part
+ * we are currently using.
+ ***/
+u32 mem_ok(void)
+{
+   u32 val1, val2, addr;
+   u32 pattern = 0x12345678;
+
+   addr = OMAP34XX_SDRC_CS0;
+
+   __raw_writel(0x0, addr + 0x400);  /* clear pos A */
+   __raw_writel(pattern, addr);  /* pattern to pos B */
+   __raw_writel(0x0, addr + 4);  /* remove pattern off the bus */
+   val1 = __raw_readl(addr + 0x400); /* get pos A value */
+   val2 = __raw_readl(addr);  

Re: [U-Boot] [PATCH 07/12 v2] ARM: OMAP3: Add memory and syslib common files, add NAND support

2008-10-03 Thread Scott Wood
On Fri, Oct 03, 2008 at 12:40:25PM +0200, [EMAIL PROTECTED] wrote:
 +#include common.h
 +#include asm/io.h
 +#include asm/arch/mem.h
 +#include linux/mtd/nand_ecc.h
 +
 +#if defined(CONFIG_CMD_NAND)
 +
 +#include nand.h

Move the #ifdef to the Makefile.

 +/*
 + * nand_read_buf16 - [DEFAULT] read chip data into buffer
 + * @mtd: MTD device structure
 + * @buf: buffer to store date
 + * @len: number of bytes to read
 + *
 + * Default read function for 16bit buswith
 + */
 +static void omap_nand_read_buf(struct mtd_info *mtd, u_char *buf, int len)
 +{
 + int i;
 + struct nand_chip *this = mtd-priv;
 + u16 *p = (u16 *) buf;
 + len = 1;
 +
 + for (i = 0; i  len; i++)
 + p[i] = readw(this-IO_ADDR_R);
 +}

How does this differ from the default implementation?

 +static void omap_nand_write_buf(struct mtd_info *mtd, const uint8_t *buf,
 + int len)
 +{
 + int i;
 + int j = 0;
 + struct nand_chip *chip = mtd-priv;
 +
 + for (i = 0; i  len; i++) {
 + writeb(buf[i], chip-IO_ADDR_W);
 + for (j = 0; j  10; j++) ;
 + }
 +
 +}
 +
 +/*
 + * omap_nand_read_buf - read data from NAND controller into buffer
 + * @mtd:MTD device structure
 + * @buf:buffer to store date
 + * @len:number of bytes to read
 + *
 + */
 +static void omap_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
 +{
 + int i;
 + int j = 0;
 + struct nand_chip *chip = mtd-priv;
 +
 + for (i = 0; i  len; i++) {
 + buf[i] = readb(chip-IO_ADDR_R);
 + while (GPMC_BUF_EMPTY == (readl(GPMC_STATUS)  GPMC_BUF_FULL));
 + }
 +}

I'm a bit confused... with 16-bit NAND, you check GMPC_STATUS[BUF_FULL]
when writing, but with 8-bit NAND, you check it when reading?  And 8-bit
writes have an arbitrary, cpu-speed-dependent delay, and 16-bit reads
have no delay at all?

 +static void omap_hwecc_init(struct nand_chip *chip)
 +{
 + unsigned long val = 0x0;
 +
 + /* Init ECC Control Register */
 + /* Clear all ECC  | Enable Reg1 */
 + val = ((0x0001  8) | 0x0001);
 + __raw_writel(val, GPMC_BASE + GPMC_ECC_CONTROL);
 + __raw_writel(0x3fcff000, GPMC_BASE + GPMC_ECC_SIZE_CONFIG);
 +}

Why raw?

 +/*
 + * omap_correct_data - Compares the ecc read from nand spare area with
 + * ECC registers values
 + *   and corrects one bit error if it has occured
 + * @mtd:  MTD device structure
 + * @dat:  page data
 + * @read_ecc: ecc read from nand flash
 + * @calc_ecc: ecc read from ECC registers
 + */
 +static int omap_correct_data(struct mtd_info *mtd, u_char *dat,
 +  u_char *read_ecc, u_char *calc_ecc)
 +{
 + return 0;
 +}

This obviously is not correcting anything.  If the errors are corrected
automatically by the controller, say so in the comments.

 +/*
 + *  omap_calculate_ecc - Generate non-inverted ECC bytes.
 + *
 + *  Using noninverted ECC can be considered ugly since writing a blank
 + *  page ie. padding will clear the ECC bytes. This is no problem as
 + *  long nobody is trying to write data on the seemingly unused page.
 + *  Reading an erased page will produce an ECC mismatch between
 + *  generated and read ECC bytes that has to be dealt with separately.

Is this a hardware limitation?  If so, say so in the comment.  If not,
why do it this way?

 + *  @mtd:MTD structure
 + *  @dat:unused
 + *  @ecc_code:   ecc_code buffer
 + */
 +static int omap_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
 +   u_char *ecc_code)
 +{
 + unsigned long val = 0x0;
 + unsigned long reg;
 +
 + /* Start Reading from HW ECC1_Result = 0x200 */
 + reg = (unsigned long) (GPMC_BASE + GPMC_ECC1_RESULT);
 + val = __raw_readl(reg);
 +
 + *ecc_code++ = ECC_P1_128_E(val);
 + *ecc_code++ = ECC_P1_128_O(val);
 + *ecc_code++ = ECC_P512_2048_E(val) | ECC_P512_2048_O(val)  4;

These macros are used nowhere else; why obscure what it's doing by moving
it to the top of the file?

 +static void omap_enable_hwecc(struct mtd_info *mtd, int mode)
 +{
 + struct nand_chip *chip = mtd-priv;
 + unsigned int val = __raw_readl(GPMC_BASE + GPMC_ECC_CONFIG);
 + unsigned int dev_width = (chip-options  NAND_BUSWIDTH_16)  1;
 +
 + switch (mode) {
 + case NAND_ECC_READ:
 + __raw_writel(0x101, GPMC_BASE + GPMC_ECC_CONTROL);
 + /* ECC col width | CS | ECC Enable */
 + val = (dev_width  7) | (cs  1) | (0x1);
 + break;
 + case NAND_ECC_READSYN:
 + __raw_writel(0x100, GPMC_BASE + GPMC_ECC_CONTROL);
 + /* ECC col width | CS | ECC Enable */
 + val = (dev_width  7) | (cs  1) | (0x1);
 + break;
 + case NAND_ECC_WRITE:
 + __raw_writel(0x101, GPMC_BASE + GPMC_ECC_CONTROL);
 + /* ECC