[U-Boot] [PATCH] nand_spl: Fix large page nand_command()

[U-Boot] [PATCH] nand_spl: Fix large page nand_command()

[Alex]

From: Alex Waterman <awaterman@dawning.com>
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 <awaterman@dawning.com>
---
 nand_spl/nand_boot.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/nand_spl/nand_boot.c b/nand_spl/nand_boot.c
index 76b8566..4ef9aea 100644
--- a/nand_spl/nand_boot.c
+++ b/nand_spl/nand_boot.c
@@ -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;
     }
 
-- 
1.7.3.1

[U-Boot] [PATCH] nand_spl: Fix large page nand_command()

[Scott Wood]

On Tue, Apr 05, 2011 at 10:41:00AM -0400, Alex wrote:
> From: Alex Waterman <awaterman@dawning.com>
> 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 <awaterman@dawning.com>
> ---

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 |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/nand_spl/nand_boot.c b/nand_spl/nand_boot.c
> index 76b8566..4ef9aea 100644
> --- a/nand_spl/nand_boot.c
> +++ b/nand_spl/nand_boot.c
> @@ -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 )

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

[U-Boot] [PATCH v2] nand_spl: Fix large page nand_command()

[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

From: Alex Waterman <awaterman@dawning.com>
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 <awaterman@dawning.com>
---
 nand_spl/nand_boot.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/nand_spl/nand_boot.c b/nand_spl/nand_boot.c
index 76b8566..4a96878 100644
--- a/nand_spl/nand_boot.c
+++ b/nand_spl/nand_boot.c
@@ -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 

[U-Boot] [PATCH v2] nand_spl: Fix large page nand_command()

[Scott Wood]

On Wed, Apr 06, 2011 at 04:01:52PM -0400, Alex Waterman wrote:
> 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

[U-Boot] [PATCH v2] nand_spl: Fix large page nand_command()

[Wolfgang Denk]

Dear Alex Waterman,

In message <4D9CC6B0.6020608@dawning.com> 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:
> 
>  - Moved the offset calculation to outside of the OOB emulation code.
>  - Hopefully no more whitespace mangling.
> 
> Signed-off-by: Alex Waterman <awaterman@dawning.com>
> ---
>  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 <awaterman@dawning.com>
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

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

[U-Boot] [PATCH v2] nand_spl: Fix large page nand_command()

[Scott Wood]

On Sun, 1 May 2011 16:43:30 +0200
Wolfgang Denk <wd@denx.de> wrote:

> Dear Alex Waterman,
> 
> In message <4D9CC6B0.6020608@dawning.com> 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:
> > 
> >  - Moved the offset calculation to outside of the OOB emulation code.
> >  - Hopefully no more whitespace mangling.
> > 
> > Signed-off-by: Alex Waterman <awaterman@dawning.com>
> > ---
> >  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

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

-Scott