Re: [U-Boot] [PATCH 1/2] Fix OneNAND ipl to read CONFIG_SYS_MONITOR_LEN
Hi Scott,
- if (page < 2 && (onenand_readw(THIS_ONENAND(ONENAND_SPARERAM)) != 0xffff))
Line length.
- Corrected.
int pagesize = ONENAND_PAGE_SIZE;
- int nblocks = CONFIG_SYS_MONITOR_LEN / ONENAND_BLOCK_SIZE;
Shouldn't nblocks be rounded up?
pagesize <<= 1;
nblocks = (nblocks + 1) >> 1;- }
Hmm, might be clearer to just do this:
if (onenand_readw(THIS_ONENAND(ONENAND_REG_TECHNOLOGY))) { pagesize = 4096; pageshift = 12; } else { pagesize = 2048; pageshift = 11; }
nblocks = (CONFIG_SYS_MONITOR_LEN + pagesize - 1) >> pageshift;
- Right , But the above line dosent give the nblocks , used erasesize and erase_shift.
ONENAND_PAGE_SIZE as a constant should go away since it's not always true.
- Removed it.
- for (; block < nblocks; block++) {
for (; page < ONENAND_PAGES_PER_BLOCK; page++) {if (onenand_read_page(block, page, buf + offset, pagesize)) {/* This block is bad. Skip it and read next block */nblocks++;break;}offset += pagesize;
If you find a bad block marker in the second page of a block, shouldn't you rewind offset to the beginning of the block?
- Correct , have reverted back.
BTW, if we're going to have "onenand_readw" and "onenand_writew" wrappers, can we make them useful by combining them with THIS_ONENAND?
- Corrected , Now onenand_readw/onenand_writew conatins THIS_ONENAND.
Please do find the updated patch below.
With Regards Moorthy
Currently OneNAND initial program loader (ipl) reads only block 0 ie 128KB. However, u-boot image for apollon board is 195KB making the board unbootable with OneNAND.
Fix ipl to read CONFIG_SYS_MONITOR_LEN. CONFIG_SYS_MONITOR_LEN macro holds the U-Boot image size.
Signed-off-by: Gangheyamoorthy moorthy.apg@samsung.com Signed-off-by: Rohit Hagargundgi h.rohit@samsung.com ---
diff --git a/onenand_ipl/onenand_boot.c b/onenand_ipl/onenand_boot.c index 86428cc..63995ce 100644 --- a/onenand_ipl/onenand_boot.c +++ b/onenand_ipl/onenand_boot.c @@ -36,7 +36,7 @@ void start_oneboot(void)
buf = (uchar *) CONFIG_SYS_LOAD_ADDR;
- onenand_read_block0(buf); + onenand_read_block(buf);
((init_fnc_t *)CONFIG_SYS_LOAD_ADDR)();
diff --git a/onenand_ipl/onenand_ipl.h b/onenand_ipl/onenand_ipl.h index c5a722d..412572a 100644 --- a/onenand_ipl/onenand_ipl.h +++ b/onenand_ipl/onenand_ipl.h @@ -31,5 +31,5 @@ #define READ_INTERRUPT() \ onenand_readw(THIS_ONENAND(ONENAND_REG_INTERRUPT))
-extern int onenand_read_block0(unsigned char *buf); +extern int onenand_read_block(unsigned char *buf); #endif diff --git a/onenand_ipl/onenand_read.c b/onenand_ipl/onenand_read.c index 6d04943..7415c7f 100644 --- a/onenand_ipl/onenand_read.c +++ b/onenand_ipl/onenand_read.c @@ -49,20 +49,20 @@ static inline int onenand_read_page(ulong block, ulong page, #endif
onenand_writew(onenand_block_address(block), - THIS_ONENAND(ONENAND_REG_START_ADDRESS1)); + ONENAND_REG_START_ADDRESS1);
onenand_writew(onenand_bufferram_address(block), - THIS_ONENAND(ONENAND_REG_START_ADDRESS2)); + ONENAND_REG_START_ADDRESS2);
onenand_writew(onenand_sector_address(page), - THIS_ONENAND(ONENAND_REG_START_ADDRESS8)); + ONENAND_REG_START_ADDRESS8);
onenand_writew(onenand_buffer_address(), - THIS_ONENAND(ONENAND_REG_START_BUFFER)); + ONENAND_REG_START_BUFFER);
- onenand_writew(ONENAND_INT_CLEAR, THIS_ONENAND(ONENAND_REG_INTERRUPT)); + onenand_writew(ONENAND_INT_CLEAR, ONENAND_REG_INTERRUPT);
- onenand_writew(ONENAND_CMD_READ, THIS_ONENAND(ONENAND_REG_COMMAND)); + onenand_writew(ONENAND_CMD_READ, ONENAND_REG_COMMAND);
#ifndef __HAVE_ARCH_MEMCPY32 p = (unsigned long *) buf; @@ -72,6 +72,10 @@ static inline int onenand_read_page(ulong block, ulong page, while (!(READ_INTERRUPT() & ONENAND_INT_READ)) continue;
+ /* Check for invalid block mark */ + if (page < 2 && (onenand_readw(ONENAND_SPARERAM) != 0xffff)) + return 1; + #ifdef __HAVE_ARCH_MEMCPY32 /* 32 bytes boundary memory copy */ memcpy32(buf, base, pagesize); @@ -89,25 +93,39 @@ static inline int onenand_read_page(ulong block, ulong page, #define ONENAND_PAGES_PER_BLOCK 64
/** - * onenand_read_block - Read a block data to buf + * onenand_read_block - Read First 'n' consecutive Good blocks holding + * data to buf * @return 0 on success */ -int onenand_read_block0(unsigned char *buf) +int onenand_read_block(unsigned char *buf) { - int page, offset = 0; - int pagesize = ONENAND_PAGE_SIZE; - - /* MLC OneNAND has 4KiB page size */ - if (onenand_readw(THIS_ONENAND(ONENAND_REG_TECHNOLOGY))) - pagesize <<= 1; + int block = 0, page = ONENAND_START_PAGE, offset = 0; + int pagesize = 0, erase_shift =0; + int erasesize = 0, nblocks = 0; + + if(onenand_readw(ONENAND_REG_TECHNOLOGY)) { + pagesize = 4096; /* MLC OneNAND has 4KiB pagesize */ + erase_shift = 18; + } else { + pagesize = 2048; + erase_shift = 17; + } + erasesize = ONENAND_PAGES_PER_BLOCK * pagesize; + nblocks = (CONFIG_SYS_MONITOR_LEN + erasesize -1) >> erase_shift;
/* NOTE: you must read page from page 1 of block 0 */ /* read the block page by page*/ - for (page = ONENAND_START_PAGE; - page < ONENAND_PAGES_PER_BLOCK; page++) { - - onenand_read_page(0, page, buf + offset, pagesize); - offset += pagesize; + for (; block < nblocks; block++) { + for (; page < ONENAND_PAGES_PER_BLOCK; page++) { + if (onenand_read_page(block, page, buf + offset, pagesize)) { + /* This block is bad. Skip it and read next block */ + offset -= page * pagesize; + nblocks++; + break; + } + offset += pagesize; + } + page = 0; }
return 0;
On Mon, Mar 23, 2009 at 02:32:20PM +0530, apgmoorthy wrote:
nblocks = (CONFIG_SYS_MONITOR_LEN + pagesize - 1) >> pageshift;
- Right , But the above line dosent give the nblocks , used erasesize and erase_shift.
D'oh, right.
Please do find the updated patch below.
It's easier if updated patches are sent separately from the reply (both to avoid getting lost, and to avoid having to manually strip discussion from the changelog).
The patch looks good other than some style nits:
Signed-off-by: Gangheyamoorthy moorthy.apg@samsung.com Signed-off-by: Rohit Hagargundgi h.rohit@samsung.com
The signed-off-by lines should go in chronological order of handling; thus, yours should be at the bottom (as the most recent one to touch or forward the patch).
- /* Check for invalid block mark */
- if (page < 2 && (onenand_readw(ONENAND_SPARERAM) != 0xffff))
return 1;
Unnecessary parens.
#ifdef __HAVE_ARCH_MEMCPY32 /* 32 bytes boundary memory copy */ memcpy32(buf, base, pagesize); @@ -89,25 +93,39 @@ static inline int onenand_read_page(ulong block, ulong page, #define ONENAND_PAGES_PER_BLOCK 64
/**
- onenand_read_block - Read a block data to buf
- onenand_read_block - Read First 'n' consecutive Good blocks holding
data to buf
"Read SYS_MONITOR_LEN from beginning of OneNAND, skipping bad blocks"
- @return 0 on success
*/ -int onenand_read_block0(unsigned char *buf) +int onenand_read_block(unsigned char *buf) {
- int page, offset = 0;
- int pagesize = ONENAND_PAGE_SIZE;
- /* MLC OneNAND has 4KiB page size */
- if (onenand_readw(THIS_ONENAND(ONENAND_REG_TECHNOLOGY)))
pagesize <<= 1;
- int block = 0, page = ONENAND_START_PAGE, offset = 0;
- int pagesize = 0, erase_shift =0;
- int erasesize = 0, nblocks = 0;
s/=0/= 0/
- if(onenand_readw(ONENAND_REG_TECHNOLOGY)) {
Space after "if".
pagesize = 4096; /* MLC OneNAND has 4KiB pagesize */erase_shift = 18;- } else {
pagesize = 2048;erase_shift = 17;- }
- erasesize = ONENAND_PAGES_PER_BLOCK * pagesize;
- nblocks = (CONFIG_SYS_MONITOR_LEN + erasesize -1) >> erase_shift;
Blank line after the closing brace at the end of a block (except with a hanging else, or similar).
s/-1/- 1/
- for (; block < nblocks; block++) {
for (; page < ONENAND_PAGES_PER_BLOCK; page++) {
Why not do block = 0 and page = 0 here, rather than at the beginning of the function and (in the case of page) at the end of the loop?
if (onenand_read_page(block, page, buf + offset, pagesize)) {/* This block is bad. Skip it and read next block */
Line length.
-Scott
Dear Scott Wood,
In message 20090323212514.GA30976@ld0162-tx32.am.freescale.net you wrote:
- /* Check for invalid block mark */
- if (page < 2 && (onenand_readw(ONENAND_SPARERAM) != 0xffff))
return 1;Unnecessary parens.
Where? I find them pretty useful. Please keep!
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
Dear Scott Wood,
In message 20090323212514.GA30976@ld0162-tx32.am.freescale.net you wrote:
- /* Check for invalid block mark */
- if (page < 2 && (onenand_readw(ONENAND_SPARERAM) != 0xffff))
return 1;Unnecessary parens.
Where? I find them pretty useful.
Around the second comparison. Why "if (a < b && (c != d))" and not "if (a < b && c != d)", or if the parens are preferred, "if ((a < b) && (c != d))"? Is it because "c" is a function call?
Please keep!
OK -- I guess this is another of the unwritten points on which U-boot's style deviates from that which is typical in Linux.
-Scott
Dear Scott,
In message 49C80B99.5010808@freescale.com you wrote:
- if (page < 2 && (onenand_readw(ONENAND_SPARERAM) != 0xffff))
return 1;Unnecessary parens.
Where? I find them pretty useful.
Around the second comparison. Why "if (a < b && (c != d))" and not "if (a < b && c != d)", or if the parens are preferred, "if ((a < b) && (c != d))"? Is it because "c" is a function call?
Actually I'd probably write this as
if ((page < 2) && (onenand_readw(ONENAND_SPARERAM) != 0xffff))
for consistency, but being lazy I guess I might use the same code as the OP.
OK -- I guess this is another of the unwritten points on which U-boot's style deviates from that which is typical in Linux.
Actually this is not some formal style (at least no rule that I know of), but personal taste :-)
When I parse something like
if (page < 2 && onenand_readw(ONENAND_SPARERAM) != 0xffff)
I can eaisly see the "page < 2" expression because it is relatively short. But I have to look twice for the "onenand_readw(ONENAND_SPARE- RAM) != 0xffff" part because it is long and includes nested parens. So I feel tempted to make this easier to read by surrounding it with parens - like the OP did.
Best regards,
Wolfgang Denk
Hi Wolfgang, ----- Original Message ----- From: "Wolfgang Denk" wd@denx.de To: "Scott Wood" scottwood@freescale.com Cc: u-boot@lists.denx.de; "apgmoorthy" moorthy.apg@samsung.com Sent: Tuesday, March 24, 2009 4:16 AM Subject: Re: [U-Boot] [PATCH 1/2] Fix OneNAND ipl to read CONFIG_SYS_MONITOR_LEN
Dear Scott,
In message 49C80B99.5010808@freescale.com you wrote:
- if (page < 2 && (onenand_readw(ONENAND_SPARERAM) !=
0xffff))
- return 1;
Unnecessary parens.
Where? I find them pretty useful.
Around the second comparison. Why "if (a < b && (c != d))" and not "if (a < b && c != d)", or if the parens are preferred, "if ((a < b) && (c != d))"? Is it because "c" is a function call?
Actually I'd probably write this as
if ((page < 2) && (onenand_readw(ONENAND_SPARERAM) != 0xffff))
for consistency, but being lazy I guess I might use the same code as the OP.
OK -- I guess this is another of the unwritten points on which U-boot's style deviates from that which is typical in Linux.
Actually this is not some formal style (at least no rule that I know of), but personal taste :-)
When I parse something like
if (page < 2 && onenand_readw(ONENAND_SPARERAM) != 0xffff)
I can eaisly see the "page < 2" expression because it is relatively short. But I have to look twice for the "onenand_readw(ONENAND_SPARE- RAM) != 0xffff" part because it is long and includes nested parens. So I feel tempted to make this easier to read by surrounding it with parens - like the OP did.
I agree with for better readabilty we will chnage and resend it today only. We will try to improve readability of code.
Thanks for suggestion. Amit
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de I'm frequently appalled by the low regard you Earthmen have for life. -- Spock, "The Galileo Seven", stardate 2822.3 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Hi Scott,
On Tuesday, March 24, 2009 2:55 AM , Scott Wood wrote
Please do find the updated patch below.
It's easier if updated patches are sent separately from the reply (both to avoid getting lost, and to avoid having to manually strip discussion from the changelog).
Signed-off-by: Gangheyamoorthy moorthy.apg@samsung.com Signed-off-by: Rohit Hagargundgi h.rohit@samsung.com
The signed-off-by lines should go in chronological order of handling; thus, yours should be at the bottom (as the most recent one to touch or forward the patch).
- Thanks, Will take care.
- /* Check for invalid block mark */
- if (page < 2 && (onenand_readw(ONENAND_SPARERAM) != 0xffff))
return 1;
Unnecessary parens.
- I think , parens make it more readable.
- onenand_read_block - Read a block data to buf
- onenand_read_block - Read First 'n' consecutive Good blocks holding
data to buf
"Read SYS_MONITOR_LEN from beginning of OneNAND, skipping bad blocks"
- Changed it in Function Header.
- int block = 0, page = ONENAND_START_PAGE, offset = 0;
- int pagesize = 0, erase_shift =0;
- int erasesize = 0, nblocks = 0;
s/=0/= 0/
- if(onenand_readw(ONENAND_REG_TECHNOLOGY)) {
Space after "if".
- Corrected them.
- } else {
pagesize = 2048;erase_shift = 17;- }
- erasesize = ONENAND_PAGES_PER_BLOCK * pagesize;
- nblocks = (CONFIG_SYS_MONITOR_LEN + erasesize -1) >> erase_shift;
Blank line after the closing brace at the end of a block (except with a hanging else, or similar).
s/-1/- 1/
- Changed them.
- for (; block < nblocks; block++) {
for (; page < ONENAND_PAGES_PER_BLOCK; page++) {
Why not do block = 0 and page = 0 here, rather than at the beginning of the function and (in the case of page) at the end of the loop?
- Initialized block in the for. With 'page' we need to start with page 1 in Block 0.
if (onenand_read_page(block, page, buf + offset, pagesize)) {/* This block is bad. Skip it and read next block */
Line length.
- Resolved.
With Regards Moorthy
Currently OneNAND initial program loader (ipl) reads only block 0 ie 128KB. However, u-boot image for apollon board is 195KB making the board unbootable with OneNAND.
Fix ipl to read CONFIG_SYS_MONITOR_LEN. CONFIG_SYS_MONITOR_LEN macro holds the U-Boot image size.
Signed-off-by: Rohit Hagargundgi h.rohit@samsung.com Signed-off-by: Gangheyamoorthy moorthy.apg@samsung.com --- onenand_boot.c | 2 - onenand_ipl.h | 8 ++----- onenand_read.c | 58 +++++++++++++++++++++++++++++++++++++++------------------ 3 files changed, 44 insertions(+), 24 deletions(-) diff --git a/onenand_ipl/onenand_boot.c b/onenand_ipl/onenand_boot.c index 86428cc..63995ce 100644 --- a/onenand_ipl/onenand_boot.c +++ b/onenand_ipl/onenand_boot.c @@ -36,7 +36,7 @@ void start_oneboot(void)
buf = (uchar *) CONFIG_SYS_LOAD_ADDR;
- onenand_read_block0(buf); + onenand_read_block(buf);
((init_fnc_t *)CONFIG_SYS_LOAD_ADDR)();
diff --git a/onenand_ipl/onenand_ipl.h b/onenand_ipl/onenand_ipl.h index 57e54f5..412572a 100644 --- a/onenand_ipl/onenand_ipl.h +++ b/onenand_ipl/onenand_ipl.h @@ -23,15 +23,13 @@
#include <linux/mtd/onenand_regs.h>
-#define onenand_readw(a) readw(a) -#define onenand_writew(v, a) writew(v, a) +#define onenand_readw(a) readw(THIS_ONENAND(a)) +#define onenand_writew(v, a) writew(v, THIS_ONENAND(a))
#define THIS_ONENAND(a) (CONFIG_SYS_ONENAND_BASE + (a))
#define READ_INTERRUPT() \ onenand_readw(THIS_ONENAND(ONENAND_REG_INTERRUPT))
-#define ONENAND_PAGE_SIZE 2048 - -extern int onenand_read_block0(unsigned char *buf); +extern int onenand_read_block(unsigned char *buf); #endif diff --git a/onenand_ipl/onenand_read.c b/onenand_ipl/onenand_read.c index 6d04943..7886358 100644 --- a/onenand_ipl/onenand_read.c +++ b/onenand_ipl/onenand_read.c @@ -49,20 +49,20 @@ static inline int onenand_read_page(ulong block, ulong page, #endif
onenand_writew(onenand_block_address(block), - THIS_ONENAND(ONENAND_REG_START_ADDRESS1)); + ONENAND_REG_START_ADDRESS1);
onenand_writew(onenand_bufferram_address(block), - THIS_ONENAND(ONENAND_REG_START_ADDRESS2)); + ONENAND_REG_START_ADDRESS2);
onenand_writew(onenand_sector_address(page), - THIS_ONENAND(ONENAND_REG_START_ADDRESS8)); + ONENAND_REG_START_ADDRESS8);
onenand_writew(onenand_buffer_address(), - THIS_ONENAND(ONENAND_REG_START_BUFFER)); + ONENAND_REG_START_BUFFER);
- onenand_writew(ONENAND_INT_CLEAR, THIS_ONENAND(ONENAND_REG_INTERRUPT)); + onenand_writew(ONENAND_INT_CLEAR, ONENAND_REG_INTERRUPT);
- onenand_writew(ONENAND_CMD_READ, THIS_ONENAND(ONENAND_REG_COMMAND)); + onenand_writew(ONENAND_CMD_READ, ONENAND_REG_COMMAND);
#ifndef __HAVE_ARCH_MEMCPY32 p = (unsigned long *) buf; @@ -72,6 +72,10 @@ static inline int onenand_read_page(ulong block, ulong page, while (!(READ_INTERRUPT() & ONENAND_INT_READ)) continue;
+ /* Check for invalid block mark */ + if (page < 2 && (onenand_readw(ONENAND_SPARERAM) != 0xffff)) + return 1; + #ifdef __HAVE_ARCH_MEMCPY32 /* 32 bytes boundary memory copy */ memcpy32(buf, base, pagesize); @@ -89,25 +93,43 @@ static inline int onenand_read_page(ulong block, ulong page, #define ONENAND_PAGES_PER_BLOCK 64
/** - * onenand_read_block - Read a block data to buf + * onenand_read_block - Read CONFIG_SYS_MONITOR_LEN from begining + * of OneNAND, skipping bad blocks * @return 0 on success */ -int onenand_read_block0(unsigned char *buf) +int onenand_read_block(unsigned char *buf) { - int page, offset = 0; - int pagesize = ONENAND_PAGE_SIZE; + int block; + int page = ONENAND_START_PAGE, offset = 0; + int pagesize = 0, erase_shift = 0; + int erasesize = 0, nblocks = 0; + + if (onenand_readw(ONENAND_REG_TECHNOLOGY)) { + pagesize = 4096; /* MLC OneNAND has 4KiB pagesize */ + erase_shift = 18; + } else { + pagesize = 2048; + erase_shift = 17; + }
- /* MLC OneNAND has 4KiB page size */ - if (onenand_readw(THIS_ONENAND(ONENAND_REG_TECHNOLOGY))) - pagesize <<= 1; + erasesize = ONENAND_PAGES_PER_BLOCK * pagesize; + nblocks = (CONFIG_SYS_MONITOR_LEN + erasesize - 1) >> erase_shift;
/* NOTE: you must read page from page 1 of block 0 */ /* read the block page by page*/ - for (page = ONENAND_START_PAGE; - page < ONENAND_PAGES_PER_BLOCK; page++) { - - onenand_read_page(0, page, buf + offset, pagesize); - offset += pagesize; + for (block = 0; block < nblocks; block++) { + for (; page < ONENAND_PAGES_PER_BLOCK; page++) { + if (onenand_read_page(block, page, buf + offset, + pagesize)) { + /* This block is bad. Skip it + * and read next block */ + offset -= page * pagesize; + nblocks++; + break; + } + offset += pagesize; + } + page = 0; }
return 0;
apgmoorthy wrote:
Currently OneNAND initial program loader (ipl) reads only block 0 ie 128KB. However, u-boot image for apollon board is 195KB making the board unbootable with OneNAND.
Fix ipl to read CONFIG_SYS_MONITOR_LEN. CONFIG_SYS_MONITOR_LEN macro holds the U-Boot image size.
Signed-off-by: Rohit Hagargundgi h.rohit@samsung.com Signed-off-by: Gangheyamoorthy moorthy.apg@samsung.com
Applied to u-boot-nand-flash, with the below whitespace errors fixed:
Applying RE: [U-Boot] [PATCH 1/2] Fix OneNAND ipl to read CONFIG_SYS_MONITOR_LEN .dotest/patch:128: trailing whitespace. /* This block is bad. Skip it .dotest/patch:130: space before tab in indent. offset -= page * pagesize; warning: 2 lines add whitespace errors.
Note that there are a couple of board files (apollon and nmdk8815) that use the OneNAND loader that do not define CONFIG_SYS_MONITOR_LEN. I've added the maintainers to the Cc: list.
-Scott
Hi Rubini,
AS suggested by scott , Please share your views regarding CONFIG_SYS_MONITOR_LEN because image size getting bigger then 1 block we have to use this macro for uboot image size.
----- Original Message ----- From: "Scott Wood" scottwood@freescale.com To: "apgmoorthy" moorthy.apg@samsung.com Cc: u-boot@lists.denx.de; rubini@unipv.it Sent: Tuesday, March 31, 2009 4:03 AM Subject: Re: [U-Boot] [PATCH 1/2] Fix OneNAND ipl to read CONFIG_SYS_MONITOR_LEN
apgmoorthy wrote:
Currently OneNAND initial program loader (ipl) reads only block 0 ie 128KB. However, u-boot image for apollon board is 195KB making the board unbootable with OneNAND.
Fix ipl to read CONFIG_SYS_MONITOR_LEN. CONFIG_SYS_MONITOR_LEN macro holds the U-Boot image size.
Signed-off-by: Rohit Hagargundgi h.rohit@samsung.com Signed-off-by: Gangheyamoorthy moorthy.apg@samsung.com
Applied to u-boot-nand-flash, with the below whitespace errors fixed:
Applying RE: [U-Boot] [PATCH 1/2] Fix OneNAND ipl to read CONFIG_SYS_MONITOR_LEN .dotest/patch:128: trailing whitespace. /* This block is bad. Skip it .dotest/patch:130: space before tab in indent. offset -= page * pagesize; warning: 2 lines add whitespace errors.
Note that there are a couple of board files (apollon and nmdk8815) that use the OneNAND loader that do not define CONFIG_SYS_MONITOR_LEN. I've added the maintainers to the Cc: list.
-Scott
Thanks Amit
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Hello.
Note that there are a couple of board files (apollon and nmdk8815) that use the OneNAND loader that do not define CONFIG_SYS_MONITOR_LEN. I've added the maintainers to the Cc: list.
Sorry for the delay.
In the nomadik board the OneNAND driver is not yet present, a few init lines are still missing (I have an error to fix before submitting those lines). I'll take care of CONFIG_SYS_MONITOR_LEN together with that.
Note however that on the Nomadik we don't use the IPL, since u-boot is loaded by other code (a proprietary IPL that onlocks the CPU before handing control to ours) so u-boot isn't even in the first sector of OneNAND.
/alessandro
Alessandro Rubini wrote:
Hello.
Note that there are a couple of board files (apollon and nmdk8815) that use the OneNAND loader that do not define CONFIG_SYS_MONITOR_LEN. I've added the maintainers to the Cc: list.
Sorry for the delay.
In the nomadik board the OneNAND driver is not yet present, a few init lines are still missing (I have an error to fix before submitting those lines). I'll take care of CONFIG_SYS_MONITOR_LEN together with that.
Note however that on the Nomadik we don't use the IPL, since u-boot is loaded by other code (a proprietary IPL that onlocks the CPU before handing control to ours) so u-boot isn't even in the first sector of OneNAND.
OK, sorry about that. I ran into the issue when building apollon, and nmdk8815 looked like it was in a similar situation from grepping config files.
-Scott
Hi Scott,
----- Original Message ----- From: "Scott Wood" scottwood@freescale.com To: "Alessandro Rubini" rubini-list@gnudd.com Cc: u-boot@lists.denx.de; moorthy.apg@samsung.com Sent: Tuesday, March 31, 2009 9:43 PM Subject: Re: [U-Boot] [PATCH 1/2] Fix OneNAND ipl to read CONFIG_SYS_MONITOR_LEN
Alessandro Rubini wrote:
Hello.
Note that there are a couple of board files (apollon and nmdk8815) that use the OneNAND loader that do not define CONFIG_SYS_MONITOR_LEN. I've added the maintainers to the Cc: list.
Sorry for the delay.
In the nomadik board the OneNAND driver is not yet present, a few init lines are still missing (I have an error to fix before submitting those lines). I'll take care of CONFIG_SYS_MONITOR_LEN together with that.
Note however that on the Nomadik we don't use the IPL, since u-boot is loaded by other code (a proprietary IPL that onlocks the CPU before handing control to ours) so u-boot isn't even in the first sector of OneNAND.
OK, sorry about that. I ran into the issue when building apollon, and nmdk8815 looked like it was in a similar situation from grepping config files.
-Scott
From Alessandro mail it is claear CONFIG_SYS_MONITOR_LEN
only used by apollon that can be changed by Apollon users independently. we will check othe rissue of white space and release patch.
Amit
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Amit Kumar Sharma wrote:
From Alessandro mail it is claear CONFIG_SYS_MONITOR_LEN only used by apollon that can be changed by Apollon users independently. we will check othe rissue of white space and release patch.
No need; I've already fixed the whitespace and applied the patch (as noted in the previous e-mail).
-Scott
Hi Scott,
On Tuesday, March 31, 2009 4:04 AM Scott Wood Wrote :
Note that there are a couple of board files (apollon and nmdk8815) that use the OneNAND loader that do not define CONFIG_SYS_MONITOR_LEN. I've added the maintainers to the Cc: list.
CONFIG_SYS_MONITOR_LEN is not defined in include/configs/apollon.h as of now.
This is done by the Post : [U-Boot] [PATCH 2/2] Fix OneNAND ipl to read CONFIG_SYS_MONITOR_LEN
What do you feel about getting this one inside u-boot-nand-flash ?
With Regards Moorthy
apgmoorthy wrote:
Hi Scott,
On Tuesday, March 31, 2009 4:04 AM Scott Wood Wrote :
Note that there are a couple of board files (apollon and nmdk8815) that use the OneNAND loader that do not define CONFIG_SYS_MONITOR_LEN. I've added the maintainers to the Cc: list.
CONFIG_SYS_MONITOR_LEN is not defined in include/configs/apollon.h as of now.
This is done by the Post : [U-Boot] [PATCH 2/2] Fix OneNAND ipl to read CONFIG_SYS_MONITOR_LEN
What do you feel about getting this one inside u-boot-nand-flash ?
Such a change should go through the board maintainer, who knows better what the actual length is.
-Scott
Hi Kyungmin, ----- Original Message ----- From: "Scott Wood" scottwood@freescale.com To: "apgmoorthy" moorthy.apg@samsung.com Cc: u-boot@lists.denx.de; rubini@unipv.it Sent: Friday, April 24, 2009 4:09 AM Subject: Re: [U-Boot] [PATCH 1/2] Fix OneNAND ipl to read CONFIG_SYS_MONITOR_LEN
apgmoorthy wrote:
Hi Scott,
On Tuesday, March 31, 2009 4:04 AM Scott Wood Wrote :
Note that there are a couple of board files (apollon and nmdk8815) that use the OneNAND loader that do not define CONFIG_SYS_MONITOR_LEN. I've added the maintainers to the Cc: list.
CONFIG_SYS_MONITOR_LEN is not defined in include/configs/apollon.h as of now.
This is done by the Post : [U-Boot] [PATCH 2/2] Fix OneNAND ipl to read CONFIG_SYS_MONITOR_LEN
What do you feel about getting this one inside u-boot-nand-flash ?
Such a change should go through the board maintainer, who knows better what the actual length is.
-Scott
Please provide your comments, Are you using Apollon board now days.
Thanks Amit
Hi, With OneNAND ipl reading CONFIG_SYS_MONITOR_LEN , Environment Area should start after CONFIG_SYS_MONITOR_LEN. Environment is made Bad Block cpapable too. These are done in the patch. And fix 'onenand test' command to skip Bootloader and Environment Blocks.
With Regards Moorthy
Makes Environment to start after CONFIG_SYS_MONITOR_LEN Environment is made to be Bad Block aware/capable. Fix 'onenand test' command to skip Bootloader and Environment Blocks.
Signed-off-by: Rohit Hagargundgi <h.rohit at samsung.com> Signed-off-by: Gangheyamoorthy <moorthy.apg at samsung.com> --- common/cmd_onenand.c | 12 ++++-- common/env_onenand.c | 96 +++++++++++++++++++++++++++++++++------------ include/configs/apollon.h | 2 +- include/onenand_uboot.h | 4 ++ 4 files changed, 84 insertions(+), 30 deletions(-)
diff --git a/common/cmd_onenand.c b/common/cmd_onenand.c index 5832ff8..c366935 100644 --- a/common/cmd_onenand.c +++ b/common/cmd_onenand.c @@ -206,6 +206,7 @@ static int onenand_block_test(u32 start, u32 size) u_char *buf; u_char *verify_buf; int ret; + int badblocklen = 0;
buf = malloc(blocksize); if (!buf) { @@ -219,13 +220,16 @@ static int onenand_block_test(u32 start, u32 size) return -1; }
+ /* Protect boot-loader and environment variables from badblock testing */ + while (start < CONFIG_SYS_MONITOR_LEN + CONFIG_ENV_SIZE + badblocklen) { + if (mtd->block_isbad(&onenand_mtd, start)) + badblocklen += blocksize; + start += blocksize; + } + start_block = start >> this->erase_shift; end_block = (start + size) >> this->erase_shift;
- /* Protect boot-loader from badblock testing */ - if (start_block < 2) - start_block = 2; - if (end_block > (mtd->size >> this->erase_shift)) end_block = mtd->size >> this->erase_shift;
diff --git a/common/env_onenand.c b/common/env_onenand.c index dbccc79..76e3aa8 100644 --- a/common/env_onenand.c +++ b/common/env_onenand.c @@ -39,6 +39,16 @@ extern uchar default_environment[];
#define ONENAND_ENV_SIZE(mtd) (mtd.writesize - ENV_HEADER_SIZE)
+/* + * User can give many blocks to environment variable partition + * through CONFIG_ENV_SIZE macro. + * The variables are written to first block in the partition. If this block + * goes bad, the successive block is used to store environment variables. + * + */ + +#define ONENAND_ENV_END (CONFIG_SYS_MONITOR_LEN + CONFIG_ENV_SIZE) + char *env_name_spec = "OneNAND";
#ifdef ENV_IS_EMBEDDED @@ -58,23 +68,31 @@ uchar env_get_char_spec(int index)
void env_relocate_spec(void) { - unsigned long env_addr; - int use_default = 0; + unsigned long env_addr = CONFIG_SYS_MONITOR_LEN; + int use_default = 1; size_t retlen; + int blocksize = onenand_mtd.erasesize;
- env_addr = CONFIG_ENV_ADDR; + /* Find environment block. */ + while (onenand_mtd.writesize && (env_addr < ONENAND_ENV_END)) { + if (onenand_block_isbad(&onenand_mtd, env_addr)) { + printf("Skip bad block at 0x%x\n", (u32)env_addr); + env_addr += blocksize; + continue; + }
- /* Check OneNAND exist */ - if (onenand_mtd.writesize) /* Ignore read fail */ onenand_read(&onenand_mtd, env_addr, onenand_mtd.writesize, &retlen, (u_char *) env_ptr); - else - onenand_mtd.writesize = MAX_ONENAND_PAGESIZE;
- if (crc32(0, env_ptr->data, ONENAND_ENV_SIZE(onenand_mtd)) != - env_ptr->crc) - use_default = 1; + if (crc32(0, env_ptr->data, ONENAND_ENV_SIZE(onenand_mtd)) == + env_ptr->crc) { + printf("OneNAND: Read environment from 0x%x\n", (u32)env_addr); + use_default = 0; + break; + } + env_addr += blocksize; + }
if (use_default) { memcpy(env_ptr->data, default_environment, @@ -89,31 +107,59 @@ void env_relocate_spec(void)
int saveenv(void) { - unsigned long env_addr = CONFIG_ENV_ADDR; + unsigned long env_addr = 0, badblocklen = 0; struct erase_info instr = { - .callback = NULL, + .callback = NULL, }; size_t retlen; + int blocksize = onenand_mtd.erasesize;
- instr.len = CONFIG_ENV_SIZE; - instr.addr = env_addr; - instr.mtd = &onenand_mtd; - if (onenand_erase(&onenand_mtd, &instr)) { - printf("OneNAND: erase failed at 0x%08lx\n", env_addr); + /* Skip bootloader area. */ + while (env_addr < CONFIG_SYS_MONITOR_LEN + badblocklen) { + if (onenand_block_isbad(&onenand_mtd, env_addr)) + badblocklen += blocksize; + env_addr += blocksize; + } + + /* Skip any bad blocks */ + while (onenand_block_isbad(&onenand_mtd, env_addr)) { + printf("Skip bad block at 0x%x\n", (u32)env_addr); + env_addr += blocksize; + } + + /* update crc */ + env_ptr->crc = + crc32(0, env_ptr->data, ONENAND_ENV_SIZE(onenand_mtd)); + +retry: + if (env_addr >= ONENAND_ENV_END) { + printf("OneNAND: Saving environment failed\n"); return 1; }
- /* update crc */ - env_ptr->crc = - crc32(0, env_ptr->data, ONENAND_ENV_SIZE(onenand_mtd)); + instr.len = blocksize; + instr.addr = env_addr; + instr.mtd = &onenand_mtd;
- if (onenand_write(&onenand_mtd, env_addr, onenand_mtd.writesize, &retlen, - (u_char *) env_ptr)) { - printf("OneNAND: write failed at 0x%08x\n", instr.addr); - return 2; + /* Erase the environment block */ + while (onenand_erase(&onenand_mtd, &instr)) { + onenand_block_markbad(&onenand_mtd, env_addr); + env_addr += blocksize; + instr.addr += blocksize; }
- return 0; + /* Write the environment variables*/ + if (onenand_write(&onenand_mtd, env_addr, onenand_mtd.writesize, + &retlen, (u_char *) env_ptr)) { + /* Mark block bad and try the next one */ + onenand_block_markbad(&onenand_mtd, env_addr); + env_addr += blocksize; + goto retry; + } + + printf("OneNAND: Saved environment to 0x%x\n", (u32)env_addr); + + return 0; }
int env_init(void) diff --git a/include/configs/apollon.h b/include/configs/apollon.h index 2e8198f..cf41e01 100644 --- a/include/configs/apollon.h +++ b/include/configs/apollon.h @@ -75,7 +75,7 @@ /* * Size of malloc() pool */ -#define CONFIG_ENV_SIZE SZ_128K /* Total Size of Environment Sector */ +#define CONFIG_ENV_SIZE SZ_256K /* Total Size of Environment Sector */ #define CONFIG_SYS_MALLOC_LEN (CONFIG_ENV_SIZE + SZ_1M) /* bytes reserved for initial data */ #define CONFIG_SYS_GBL_DATA_SIZE 128 diff --git a/include/onenand_uboot.h b/include/onenand_uboot.h index 49da9d0..dbac25b 100644 --- a/include/onenand_uboot.h +++ b/include/onenand_uboot.h @@ -38,6 +38,10 @@ extern int onenand_erase(struct mtd_info *mtd, struct erase_info *instr);
extern char *onenand_print_device_info(int device, int version);
+extern int onenand_block_isbad(struct mtd_info *mtd, loff_t ofs); + +extern int onenand_block_markbad(struct mtd_info *mtd, loff_t ofs); + /* S3C64xx */ extern void s3c64xx_onenand_init(struct mtd_info *); extern void s3c64xx_set_width_regs(struct onenand_chip *);
On Mon, Apr 13, 2009 at 06:34:52PM +0530, apgmoorthy wrote:
- /* Protect boot-loader and environment variables from badblock testing */
- while (start < CONFIG_SYS_MONITOR_LEN + CONFIG_ENV_SIZE + badblocklen) {
if (mtd->block_isbad(&onenand_mtd, start))badblocklen += blocksize;start += blocksize;- }
You need to start at zero to determine the number of bad blocks used by the boot image -- otherwise, if start points in the middle of the image, it is assuming that there were no bad blocks prior to that location.
Plus, if "start" was not block-aligned (is that allowed?), this should round up to the next block rather than jumping to the middle of the next block.
+#define ONENAND_ENV_END (CONFIG_SYS_MONITOR_LEN + CONFIG_ENV_SIZE)
char *env_name_spec = "OneNAND";
#ifdef ENV_IS_EMBEDDED @@ -58,23 +68,31 @@ uchar env_get_char_spec(int index)
void env_relocate_spec(void) {
- unsigned long env_addr;
- int use_default = 0;
- unsigned long env_addr = CONFIG_SYS_MONITOR_LEN;
As above, you have to check for bad blocks from the beginning. You do this in saveenv...
- int use_default = 1; size_t retlen;
- int blocksize = onenand_mtd.erasesize;
- env_addr = CONFIG_ENV_ADDR;
- /* Find environment block. */
- while (onenand_mtd.writesize && (env_addr < ONENAND_ENV_END)) {
if (onenand_block_isbad(&onenand_mtd, env_addr)) {printf("Skip bad block at 0x%x\n", (u32)env_addr);
Why not just use %lx?
/* update crc */env_ptr->crc =crc32(0, env_ptr->data, ONENAND_ENV_SIZE(onenand_mtd));
There's an extra space in the above indent.
return 0;
Here too.
<record state="broken"> Any consolidation we can do between env_nand and env_onenand would be appreciated... even if they don't share code yet, let's keep the logic similar. </record>
-Scott
On Apr 05, 2009 01:40 Jean-Christophe Wrote :
Your patch Fix OneNAND ipl to read CONFIG_SYS_MONITOR_LEN brake the appolon Could you take a look?
- Can you please do let me know , where exactly do you see a problem ?
With Regards Moorthy
participants (5)
-
Alessandro Rubini -
Amit Kumar Sharma -
apgmoorthy -
Scott Wood -
Wolfgang Denk