nand_spl: Fix large page nand_command()

PATCH Superseded Alex 3 participants 6 messages Apr 5, 2011 – May 3, 2011

Share & tools

Alex

author

2011-04-05 14:41 UTC #0 lore raw

PATCH 1/1 nand_spl: Fix large page nand_command()

From: Alex Waterman <[email protected]>
Date: Tue, 5 Apr 2011 10:32:38 -0400
Subject: [PATCH] nand_spl: Fix large page nand_command()

The large page nand_command() function addresses all chips by byte offset; when using a 16 bit chip the NAND_CMD_READOOB emulation would then address the OOB as though it were a byte offset instead of a word offset. This leads to addressing data that doesnt exist; to fix, simply divide the byte offset by two.

Signed-off-by Alex Waterman <[email protected]>

---
 nand_spl/nand_boot.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

nand_spl/nand_boot.c +4 -1

index 76b8566..4ef9aea 100644
@@ -86,7 +86,10 @@ static int nand_command(struct mtd_info *mtd, int block, int page, int offs, u8
 
     /* Emulate NAND_CMD_READOOB */
     if (cmd == NAND_CMD_READOOB) {
-        offs += CONFIG_SYS_NAND_PAGE_SIZE;
+        if ( this->options & NAND_BUSWIDTH_16 )
+            offs += (CONFIG_SYS_NAND_PAGE_SIZE >> 1);
+        else
+            offs += CONFIG_SYS_NAND_PAGE_SIZE;
         cmd = NAND_CMD_READ0;
     }

Signature · 1 line

1.7.3.1

Scott Wood

2011-04-05 18:42 UTC #1 lore raw

PATCH 1/1 nand_spl: Fix large page nand_command()

Quoted 4 lines · Alex

From: Alex Waterman <[email protected]>
Date: Tue, 5 Apr 2011 10:32:38 -0400
Subject: [PATCH] nand_spl: Fix large page nand_command()

Quoted 8 lines

The large page nand_command() function addresses all chips by byte offset; when using a 16 bit chip the NAND_CMD_READOOB emulation would then address the OOB as though it were a byte offset instead of a word offset. This leads to addressing data that doesnt exist; to fix, simply divide the byte offset by two.
Signed-off-by Alex Waterman <[email protected]>
---

Patch is whitespace-damaged, and does not apply. Please read:
http://www.denx.de/wiki/U-Boot/Patches

Also please make sure your full name is in the From: field, so that the git commit authorship information is complete.

nand_spl/nand_boot.c static int nand_command(struct mtd_info *mtd, int block, int page, int offs, u8 · line 89

Quoted 13 lines

 nand_spl/nand_boot.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

nand_spl/nand_boot.c +1 -1

index 76b8566..4ef9aea 100644
@@ -86,7 +86,10 @@ static int nand_command(struct mtd_info *mtd, int block, int page, int offs, u8
 
     /* Emulate NAND_CMD_READOOB */
     if (cmd == NAND_CMD_READOOB) {
-        offs += CONFIG_SYS_NAND_PAGE_SIZE;
+        if ( this->options & NAND_BUSWIDTH_16 )

No space after ( or before )

nand_spl/nand_boot.c static int nand_command(struct mtd_info *mtd, int block, int page, int offs, u8 · line 90

Quoted 1 line

nand_spl/nand_boot.c +1 -0

+            offs += (CONFIG_SYS_NAND_PAGE_SIZE >> 1);

Unnecessary parens

The semantics of this don't match what's done in the non-SPL code -- there the column/offs is assumed to be in bytes when passed to nand_command(), and dividing by two happens later (after the READOOB adjustment).

Don't we also need to fix nand_is_bad_block() to use a 16-bit access?

-Scott

Alex Waterman

author

2011-04-06 20:01 UTC #2 lore raw

PATCH v2 1/1 nand_spl: Fix large page nand_command()

Sorry, I screwed up the settings in Thunderbird, hopefully I fixed the white space/name issues now.

About the patch not applying, I used git format-patch and git apply to test the stuff locally, was that wrong?

Here is a revised patch, I hope this is better.

- Alex

From: Alex Waterman <[email protected]>
Date: Wed, 6 Apr 2011 15:09:57 -0400
Subject: [PATCH] nand_spl: Fix large page nand_command()

This patch changes the large page nand_command() routine to use a word offset instead of a byte offset. The 'offs' argument gets divided by 2 so that the offset passed to nand_command() is still by byte offset. Originally, the offset was not shifted and when too high an offset was requested the nand chip would attempt to read non-existent data.

Changes for v2:

- Moved the offset calculation to outside of the OOB emulation code.
- Hopefully no more whitespace mangling.

Signed-off-by Alex Waterman <[email protected]>

---
 nand_spl/nand_boot.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

nand_spl/nand_boot.c +4 -0

index 76b8566..4a96878 100644
@@ -90,6 +90,10 @@ static int nand_command(struct mtd_info *mtd, int block, int page, int offs, u8
 		cmd = NAND_CMD_READ0;
 	}
 
+	/* Shift the offset from byte addressing to word addressing. */
+	if (this->options & NAND_BUSWIDTH_16)
+		offs >>= 1;
+
 	/* Begin command latch cycle */
 	this->cmd_ctrl(mtd, cmd, NAND_CTRL_CLE | NAND_CTRL_CHANGE);
 	/* Set ALE and clear CLE to start address cycle */

-- 1.7.3.1

Scott Wood

2011-04-11 18:58 UTC #3 lore raw

PATCH v2 1/1 nand_spl: Fix large page nand_command()

Quoted 9 lines · Alex Waterman

Sorry, I screwed up the settings in Thunderbird, hopefully I fixed the white space/name issues now.
About the patch not applying, I used git format-patch and git apply to test the stuff locally, was that wrong?
Here is a revised patch, I hope this is better.
- Alex

Applied to u-boot-nand-flash

For future reference, anything which is not supposed to be part of the permanent changelog (including a description of changes from the previous version) should go below the --- line.

-Scott

Wolfgang Denk

2011-05-01 14:43 UTC #4 lore raw

PATCH v2 1/1 nand_spl: Fix large page nand_command()

In reply to Alex Waterman

Dear Alex Waterman,

Quoted 9 lines · In message <[email protected]> you wrote:

This patch changes the large page nand_command() routine to use a word offset instead of a byte offset. The 'offs' argument gets divided by 2 so that the offset passed to nand_command() is still by byte offset. Originally, the offset was not shifted and when too high an offset was requested the nand chip would attempt to read non-existent data.
Changes for v2:

Quoted 7 lines

- Moved the offset calculation to outside of the OOB emulation code.
- Hopefully no more whitespace mangling.
Signed-off-by Alex Waterman <[email protected]>
---
 nand_spl/nand_boot.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

Unfortunately this patch breaks building of the canyonlands_nand and glacier_nand configurations:

Configuring for canyonlands_nand - Board: canyonlands, Options: CANYONLANDS,NAND_U_BOOT,SYS_TEXT_BASE=0x01000000
ppc_6xx-ld: section .resetvec [e3003ffc -> e3003fff] overlaps section .bss.ndfc_cs [e3003ff4 -> e3004003] make[1]: *** [/work/wd/tmp-ppc/nand_spl/u-boot-spl] Error 1
make: *** [nand_spl] Error 2
make: *** Waiting for unfinished jobs....
ppc_6xx-size: '/work/wd/tmp-ppc/u-boot': No such file

Configuring for glacier_nand - Board: canyonlands, Options: GLACIER,NAND_U_BOOT,SYS_TEXT_BASE=0x01000000
ppc_6xx-ld: section .resetvec [e3003ffc -> e3003fff] overlaps section .bss.ndfc_cs [e3003ff4 -> e3004003] make[1]: *** [/work/wd/tmp-ppc/nand_spl/u-boot-spl] Error 1
make: *** [nand_spl] Error 2
make: *** Waiting for unfinished jobs....
ppc_6xx-size: '/work/wd/tmp-ppc/u-boot': No such file

Git bisect says:

65a9db7be0868be91ba81b9b5bf821de82e6d9b0 is the first bad commit commit 65a9db7be0868be91ba81b9b5bf821de82e6d9b0

Author: Alex Waterman <[email protected]>
Date:   Wed Apr 6 16:01:52 2011 -0400

nand_spl: Fix large page nand_command()

Can you please provide a fix? Thanks.

Best regards,

Wolfgang Denk

Signature · 5 lines

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 at denx.de
"Security is mostly a superstition. It does not  exist  in  nature...
Life is either a daring adventure or nothing."         - Helen Keller

Scott Wood

2011-05-03 16:08 UTC #5 lore raw

PATCH v2 1/1 nand_spl: Fix large page nand_command()

Quoted 30 lines · On Sun, 1 May 2011 16:43:30 +0200 Wolfgang Denk <[email protected]> wrote:

Dear Alex Waterman,
Quoted 9 lines · In message <[email protected]> you wrote:
This patch changes the large page nand_command() routine to use a word offset instead of a byte offset. The 'offs' argument gets divided by 2 so that the offset passed to nand_command() is still by byte offset. Originally, the offset was not shifted and when too high an offset was requested the nand chip would attempt to read non-existent data.
Changes for v2:
Quoted 7 lines
- Moved the offset calculation to outside of the OOB emulation code.
- Hopefully no more whitespace mangling.
Signed-off-by Alex Waterman <[email protected]>
---
 nand_spl/nand_boot.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
Unfortunately this patch breaks building of the canyonlands_nand and glacier_nand configurations:
Configuring for canyonlands_nand - Board: canyonlands, Options: CANYONLANDS,NAND_U_BOOT,SYS_TEXT_BASE=0x01000000
ppc_6xx-ld: section .resetvec [e3003ffc -> e3003fff] overlaps section .bss.ndfc_cs [e3003ff4 -> e3004003] make[1]: *** [/work/wd/tmp-ppc/nand_spl/u-boot-spl] Error 1
make: *** [nand_spl] Error 2
make: *** Waiting for unfinished jobs....
ppc_6xx-size: '/work/wd/tmp-ppc/u-boot': No such file

Quoted 6 lines

Configuring for glacier_nand - Board: canyonlands, Options: GLACIER,NAND_U_BOOT,SYS_TEXT_BASE=0x01000000
ppc_6xx-ld: section .resetvec [e3003ffc -> e3003fff] overlaps section .bss.ndfc_cs [e3003ff4 -> e3004003] make[1]: *** [/work/wd/tmp-ppc/nand_spl/u-boot-spl] Error 1
make: *** [nand_spl] Error 2
make: *** Waiting for unfinished jobs....
ppc_6xx-size: '/work/wd/tmp-ppc/u-boot': No such file

They build for me with GCC 4.5.1 -- probably right on the edge of the code size limit.

-Scott