Re: [U-Boot-Users] [PATCH 2/7 v3] NAND_CMD_READOOB is not supported by all chips, read OOB with the page instead
On Mon, 4 Aug 2008, Scott Wood wrote:
On Mon, Aug 04, 2008 at 02:45:33PM +0200, Guennadi Liakhovetski wrote:
I _think_ this should work with all NAND chips. Otherwise we might have to
introduce a configuration variable.
Which small-page NAND chips can't handle READOOB? On large page devices,
nand_command changes it to READ0.
It's a large-page device. And, as far as I understand the datasheet, to
read data at arbitrary offset in a page, you first have to issue a READ
PAGE (READ0) for _the_ _whole_ page, then you can use RANDOM DATA READ to
read arbitrary data within this page. Whereas, the driver attempts to use
READ0 to read the bad-block marker directly, which doesn't work with this
chip. At least this is my understanding.
That said, doing it all at once could result in smaller, faster, and
simpler code.
@@ -150,8 +135,6 @@ static int nand_read_page(struct mtd_info *mtd, int
block, int page, uchar *dst)
u_char *ecc_code;
u_char *oob_data;
int i;
- int eccsize = CFG_NAND_ECCSIZE;
- int eccbytes = CFG_NAND_ECCBYTES;
Any particular reason for this change? It's more readable as is, IMHO.
Acually, it was to improve readability:-) First, this way you can easier
grep. Secondly, when I see an assignment to a _variable_, I expect, that
this variable's value can indeed _vary_. So, it makes extra work looking
through the code and verifying what other values this variable takes.
Thus, at the very least I would add const to the definition. And, I do
think using constants directly makes it clearer...
@@ -195,6 +180,7 @@ static int nand_load(struct mtd_info *mtd, int offs,
int uboot_size, uchar *dst)
int block;
int blockcopy_count;
int page;
+ unsigned read = 0;
unsigned int, please.
Ok...
+ int badblock = 0;
+ for (page = 0; page CFG_NAND_PAGE_COUNT; page++) {
+ nand_read_page(mtd, block, page, dst);
+ if ((!page
+#ifdef CFG_NAND_BBT_2NDPAGE
+|| page == 1
+#endif
Please use page == 0 rather than !page when checking for an actual value
of zero as opposed to a zero that means not valid or similar.
Ok...
+ ) dst[CFG_NAND_PAGE_SIZE] != 0xff) {
+ badblock = 1;
+ break;
}
+ /* Overwrite skipped pages */
+ if (read = offs)
+ dst += CFG_NAND_PAGE_SIZE;
+ read += CFG_NAND_PAGE_SIZE;
+ }
I don't follow the logic here -- you're discarding a number of good
blocks equal to the offset? That might make sense if we were starting at
block zero, and defining the offset as not including any bad blocks
before the image -- but the first block we read is at the start of the
image, not the start of flash.
Right, that's a bug. Hope, it's fixed now.
@@ -241,12 +239,18 @@ void nand_boot(void)
nand_chip.dev_ready = NULL; /* preset to NULL */
board_nand_init(nand_chip);
+ if (nand_chip.select_chip)
+ nand_chip.select_chip(nand_info, 0);
+
/*
* Load U-Boot image from NAND into RAM
*/
ret = nand_load(nand_info, CFG_NAND_U_BOOT_OFFS, CFG_NAND_U_BOOT_SIZE,
(uchar *)CFG_NAND_U_BOOT_DST);
+ if (nand_chip.select_chip)
+ nand_chip.select_chip(nand_info, -1);
+
This seems like an unrelated change, that wasn't described in the
changelog.
Ok, will describe in the changelog.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: [EMAIL PROTECTED]
-
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100url=/
___
U-Boot-Users mailing list
U-Boot-Users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/u-boot-users
Re: [U-Boot-Users] [PATCH 2/7 v3] NAND_CMD_READOOB is not supported by all chips, read OOB with the page instead
On Tue, Aug 05, 2008 at 03:08:04PM +0200, Guennadi Liakhovetski wrote: It's a large-page device. And, as far as I understand the datasheet, to read data at arbitrary offset in a page, you first have to issue a READ PAGE (READ0) for _the_ _whole_ page, then you can use RANDOM DATA READ to read arbitrary data within this page. Whereas, the driver attempts to use READ0 to read the bad-block marker directly, which doesn't work with this chip. At least this is my understanding. Are you saying that your NAND chip can't read the OOB by issuing READ0 with the appropriate column address? Which chip is this, and where can I find a manual? @@ -150,8 +135,6 @@ static int nand_read_page(struct mtd_info *mtd, int block, int page, uchar *dst) u_char *ecc_code; u_char *oob_data; int i; - int eccsize = CFG_NAND_ECCSIZE; - int eccbytes = CFG_NAND_ECCBYTES; Any particular reason for this change? It's more readable as is, IMHO. Acually, it was to improve readability:-) First, this way you can easier grep. Grep will find the initialization. Secondly, when I see an assignment to a _variable_, I expect, that this variable's value can indeed _vary_. So, it makes extra work looking through the code and verifying what other values this variable takes. Thus, at the very least I would add const to the definition. And, I do think using constants directly makes it clearer... It replaces a short lower-case name with a longer all-caps name that forces line breaks. I'm fine with adding const. -Scott - This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100url=/ ___ U-Boot-Users mailing list U-Boot-Users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/u-boot-users
Re: [U-Boot-Users] [PATCH 2/7 v3] NAND_CMD_READOOB is not supported by all chips, read OOB with the page instead
On Tue, 5 Aug 2008, Scott Wood wrote: On Tue, Aug 05, 2008 at 03:08:04PM +0200, Guennadi Liakhovetski wrote: It's a large-page device. And, as far as I understand the datasheet, to read data at arbitrary offset in a page, you first have to issue a READ PAGE (READ0) for _the_ _whole_ page, then you can use RANDOM DATA READ to read arbitrary data within this page. Whereas, the driver attempts to use READ0 to read the bad-block marker directly, which doesn't work with this chip. At least this is my understanding. Are you saying that your NAND chip can't read the OOB by issuing READ0 with the appropriate column address? Which chip is this, and where can I find a manual? At least, this is how I understood it, I might be wrong though: http://download.micron.com/pdf/datasheets/flash/nand/2_4_8gb_nand_m49a.pdf pages 21, 22. @@ -150,8 +135,6 @@ static int nand_read_page(struct mtd_info *mtd, int block, int page, uchar *dst) u_char *ecc_code; u_char *oob_data; int i; - int eccsize = CFG_NAND_ECCSIZE; - int eccbytes = CFG_NAND_ECCBYTES; Any particular reason for this change? It's more readable as is, IMHO. Acually, it was to improve readability:-) First, this way you can easier grep. Grep will find the initialization. Secondly, when I see an assignment to a _variable_, I expect, that this variable's value can indeed _vary_. So, it makes extra work looking through the code and verifying what other values this variable takes. Thus, at the very least I would add const to the definition. And, I do think using constants directly makes it clearer... It replaces a short lower-case name with a longer all-caps name that forces line breaks. I'm fine with adding const. Ok... Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: [EMAIL PROTECTED] - This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100url=/ ___ U-Boot-Users mailing list U-Boot-Users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/u-boot-users
Re: [U-Boot-Users] [PATCH 2/7 v3] NAND_CMD_READOOB is not supported by all chips, read OOB with the page instead
Guennadi Liakhovetski wrote: On Tue, 5 Aug 2008, Scott Wood wrote: Are you saying that your NAND chip can't read the OOB by issuing READ0 with the appropriate column address? Which chip is this, and where can I find a manual? At least, this is how I understood it, I might be wrong though: http://download.micron.com/pdf/datasheets/flash/nand/2_4_8gb_nand_m49a.pdf pages 21, 22. The READ0 command says, Starting from the initial column address and going to the end of the page, read the data by repeatedly pulsing RE# at the maximum tRC rate (see Figure 14). That looks normal... have you tried it? IIUC, the complete page of data refers to the transfer from the NAND storage to the chip's page buffer, not to the transfer from the buffer to the CPU. -Scott - This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100url=/ ___ U-Boot-Users mailing list U-Boot-Users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/u-boot-users
[U-Boot-Users] [PATCH 2/7 v3] NAND_CMD_READOOB is not supported by all chips, read OOB with the page instead
I _think_ this should work with all NAND chips. Otherwise we might have to
introduce a configuration variable.
Signed-off-by: Guennadi Liakhovetski [EMAIL PROTECTED]
---
nand_spl/nand_boot.c | 64 ++---
1 files changed, 34 insertions(+), 30 deletions(-)
diff --git a/nand_spl/nand_boot.c b/nand_spl/nand_boot.c
index 563a80b..0de4c4b 100644
--- a/nand_spl/nand_boot.c
+++ b/nand_spl/nand_boot.c
@@ -128,21 +128,6 @@ static int nand_command(struct mtd_info *mtd, int block,
int page, int offs, u8
}
#endif
-static int nand_is_bad_block(struct mtd_info *mtd, int block)
-{
- struct nand_chip *this = mtd-priv;
-
- nand_command(mtd, block, 0, CFG_NAND_BAD_BLOCK_POS, NAND_CMD_READOOB);
-
- /*
-* Read one byte
-*/
- if (this-read_byte(mtd) != 0xff)
- return 1;
-
- return 0;
-}
-
static int nand_read_page(struct mtd_info *mtd, int block, int page, uchar
*dst)
{
struct nand_chip *this = mtd-priv;
@@ -150,8 +135,6 @@ static int nand_read_page(struct mtd_info *mtd, int block,
int page, uchar *dst)
u_char *ecc_code;
u_char *oob_data;
int i;
- int eccsize = CFG_NAND_ECCSIZE;
- int eccbytes = CFG_NAND_ECCBYTES;
int eccsteps = CFG_NAND_ECCSTEPS;
uint8_t *p = dst;
int stat;
@@ -163,11 +146,12 @@ static int nand_read_page(struct mtd_info *mtd, int
block, int page, uchar *dst)
*/
ecc_calc = (u_char *)(CFG_SDRAM_BASE + 0x1);
ecc_code = ecc_calc + 0x100;
- oob_data = ecc_calc + 0x200;
+ oob_data = p + CFG_NAND_PAGE_SIZE; /* Append OOB to the page data */
- for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
+ for (i = 0; eccsteps;
+eccsteps--, i += CFG_NAND_ECCBYTES, p += CFG_NAND_ECCSIZE) {
this-enable_hwecc(mtd, NAND_ECC_READ);
- this-read_buf(mtd, p, eccsize);
+ this-read_buf(mtd, p, CFG_NAND_ECCSIZE);
this-calculate_ecc(mtd, p, ecc_calc[i]);
}
this-read_buf(mtd, oob_data, CFG_NAND_OOBSIZE);
@@ -179,7 +163,8 @@ static int nand_read_page(struct mtd_info *mtd, int block,
int page, uchar *dst)
eccsteps = CFG_NAND_ECCSTEPS;
p = dst;
- for (i = 0 ; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
+ for (i = 0; eccsteps;
+eccsteps--, i += CFG_NAND_ECCBYTES, p += CFG_NAND_ECCSIZE) {
/* No chance to do something with the possible error message
* from correct_data(). We just hope that all possible errors
* are corrected by this routine.
@@ -195,6 +180,7 @@ static int nand_load(struct mtd_info *mtd, int offs, int
uboot_size, uchar *dst)
int block;
int blockcopy_count;
int page;
+ unsigned read = 0;
/*
* offs has to be aligned to a block address!
@@ -202,18 +188,30 @@ static int nand_load(struct mtd_info *mtd, int offs, int
uboot_size, uchar *dst)
block = offs / CFG_NAND_BLOCK_SIZE;
blockcopy_count = 0;
- while (blockcopy_count (uboot_size / CFG_NAND_BLOCK_SIZE)) {
- if (!nand_is_bad_block(mtd, block)) {
- /*
-* Skip bad blocks
-*/
- for (page = 0; page CFG_NAND_PAGE_COUNT; page++) {
- nand_read_page(mtd, block, page, dst);
- dst += CFG_NAND_PAGE_SIZE;
+ while (blockcopy_count ((uboot_size + CFG_NAND_BLOCK_SIZE - 1) /
+ CFG_NAND_BLOCK_SIZE)) {
+ /*
+* Skip bad blocks
+*/
+ int badblock = 0;
+ for (page = 0; page CFG_NAND_PAGE_COUNT; page++) {
+ nand_read_page(mtd, block, page, dst);
+ if ((!page
+#ifdef CFG_NAND_BBT_2NDPAGE
+|| page == 1
+#endif
+ ) dst[CFG_NAND_PAGE_SIZE] != 0xff) {
+ badblock = 1;
+ break;
}
+ /* Overwrite skipped pages */
+ if (read = offs)
+ dst += CFG_NAND_PAGE_SIZE;
+ read += CFG_NAND_PAGE_SIZE;
+ }
+ if (!badblock)
blockcopy_count++;
- }
block++;
}
@@ -241,12 +239,18 @@ void nand_boot(void)
nand_chip.dev_ready = NULL; /* preset to NULL */
board_nand_init(nand_chip);
+ if (nand_chip.select_chip)
+ nand_chip.select_chip(nand_info, 0);
+
/*
* Load U-Boot image from NAND into RAM
*/
ret = nand_load(nand_info, CFG_NAND_U_BOOT_OFFS, CFG_NAND_U_BOOT_SIZE,
(uchar
Re: [U-Boot-Users] [PATCH 2/7 v3] NAND_CMD_READOOB is not supported by all chips, read OOB with the page instead
On Mon, Aug 04, 2008 at 02:45:33PM +0200, Guennadi Liakhovetski wrote:
I _think_ this should work with all NAND chips. Otherwise we might have to
introduce a configuration variable.
Which small-page NAND chips can't handle READOOB? On large page devices,
nand_command changes it to READ0.
That said, doing it all at once could result in smaller, faster, and
simpler code.
@@ -150,8 +135,6 @@ static int nand_read_page(struct mtd_info *mtd, int
block, int page, uchar *dst)
u_char *ecc_code;
u_char *oob_data;
int i;
- int eccsize = CFG_NAND_ECCSIZE;
- int eccbytes = CFG_NAND_ECCBYTES;
Any particular reason for this change? It's more readable as is, IMHO.
@@ -195,6 +180,7 @@ static int nand_load(struct mtd_info *mtd, int offs, int
uboot_size, uchar *dst)
int block;
int blockcopy_count;
int page;
+ unsigned read = 0;
unsigned int, please.
+ int badblock = 0;
+ for (page = 0; page CFG_NAND_PAGE_COUNT; page++) {
+ nand_read_page(mtd, block, page, dst);
+ if ((!page
+#ifdef CFG_NAND_BBT_2NDPAGE
+ || page == 1
+#endif
Please use page == 0 rather than !page when checking for an actual value
of zero as opposed to a zero that means not valid or similar.
+ ) dst[CFG_NAND_PAGE_SIZE] != 0xff) {
+ badblock = 1;
+ break;
}
+ /* Overwrite skipped pages */
+ if (read = offs)
+ dst += CFG_NAND_PAGE_SIZE;
+ read += CFG_NAND_PAGE_SIZE;
+ }
I don't follow the logic here -- you're discarding a number of good
blocks equal to the offset? That might make sense if we were starting at
block zero, and defining the offset as not including any bad blocks
before the image -- but the first block we read is at the start of the
image, not the start of flash.
@@ -241,12 +239,18 @@ void nand_boot(void)
nand_chip.dev_ready = NULL; /* preset to NULL */
board_nand_init(nand_chip);
+ if (nand_chip.select_chip)
+ nand_chip.select_chip(nand_info, 0);
+
/*
* Load U-Boot image from NAND into RAM
*/
ret = nand_load(nand_info, CFG_NAND_U_BOOT_OFFS, CFG_NAND_U_BOOT_SIZE,
(uchar *)CFG_NAND_U_BOOT_DST);
+ if (nand_chip.select_chip)
+ nand_chip.select_chip(nand_info, -1);
+
This seems like an unrelated change, that wasn't described in the
changelog.
-Scott
-
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100url=/
___
U-Boot-Users mailing list
U-Boot-Users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/u-boot-users
