Tegra2: implement Warmboot code and lp0_vec

Tegra2: implement Warmboot code and lp0_vec
Signed-off-by: Yen Lin <yelin@nvidia.com>;

BUG=None
TEST=Build and test on Seaboard with and without CONFIG_TEGRA2_LP0

Change-Id: If3b414b3163418ba3f92aafc22f41bb6dd4c8723

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=1c8d705

[Tom Warren, 2010-11-12 00:12:40]

A lot of this code is going to be hell to port to upstream U-Boot. Anton -
please tell me this is one of the features we can hold back from upstream (like
graphics init, etc.).

http://codereview.chromium.org/4841001/diff/1/arch/arm/cpu/arm_cortexa9/tegra...
File arch/arm/cpu/arm_cortexa9/tegra2/ap20.c (right):

http://codereview.chromium.org/4841001/diff/1/arch/arm/cpu/arm_cortexa9/tegra...
arch/arm/cpu/arm_cortexa9/tegra2/ap20.c:973: 
Why are you moving this to tegra2_start from cpu_start? If it's necessary for
the WB0 code to work, please put a comment in here as to why it has to be called
here. If not, then put it in another patch w/a justification. Thanks.

http://codereview.chromium.org/4841001/diff/1/arch/arm/cpu/arm_cortexa9/tegra...
File arch/arm/cpu/arm_cortexa9/tegra2/ap20.h (left):

http://codereview.chromium.org/4841001/diff/1/arch/arm/cpu/arm_cortexa9/tegra...
arch/arm/cpu/arm_cortexa9/tegra2/ap20.h:113: 
Why were these removed in this changelist? If they are not used, I happy to see
them go, but do it in another (cleanup) changelist, please.

http://codereview.chromium.org/4841001/diff/1/arch/arm/cpu/arm_cortexa9/tegra...
File arch/arm/cpu/arm_cortexa9/tegra2/warmboot_avp.S (right):

http://codereview.chromium.org/4841001/diff/1/arch/arm/cpu/arm_cortexa9/tegra...
arch/arm/cpu/arm_cortexa9/tegra2/warmboot_avp.S:65: 
This code is very ugly - it looks like we're using some macros from
'nvbl_assembly.h' to prettify the normal ARM asm mnemonics, but it just makes it
stand out as being different from all other .S files in U-Boot!  Please don't do
this - rewrite it using the correct ARM syntax.  This is the kind of craptastic
code I'm going to spend inordinate amounts of time rewriting just to get it to
pass muster @ u-boot@lists.denx.de.

http://codereview.chromium.org/4841001/diff/1/arch/arm/include/asm/arch-tegra...
File arch/arm/include/asm/arch-tegra2/nv_drf_asm.h (right):

http://codereview.chromium.org/4841001/diff/1/arch/arm/include/asm/arch-tegra...
arch/arm/include/asm/arch-tegra2/nv_drf_asm.h:55:
*-----------------------------------------------------------------------------
Nor should they - write assembler code in assembly!

Please fix this, also. There's no way we should be having any reference to MS C
in U-Boot or ARM code!

http://codereview.chromium.org/4841001/diff/1/arch/arm/include/asm/arch-tegra...
File arch/arm/include/asm/arch-tegra2/nvbl_assembly.h (right):

http://codereview.chromium.org/4841001/diff/1/arch/arm/include/asm/arch-tegra...
arch/arm/include/asm/arch-tegra2/nvbl_assembly.h:76: #define NVOS_IS_WINDOWS 1
Remove all references to Windows/WIN32 - this is U-Boot

http://codereview.chromium.org/4841001/diff/1/arch/arm/include/asm/arch-tegra...
arch/arm/include/asm/arch-tegra2/nvbl_assembly.h:128: 
This crap shouldn't be needed with our ARM gcc/ar/as/ld toolset. Please
rewrite/remove. It just makes the assembly code ugly & will never pass on
u-boot@lists.denx.de (U-Boot upstream).

http://codereview.chromium.org/4841001/diff/1/arch/arm/lib/board.c
File arch/arm/lib/board.c (right):

http://codereview.chromium.org/4841001/diff/1/arch/arm/lib/board.c#newcode68
arch/arm/lib/board.c:68: int prepare_wb_code(unsigned int, unsigned int);
You should put protos/forward declarations in a header file.

http://codereview.chromium.org/4841001/diff/1/arch/arm/lib/board.c#newcode326
arch/arm/lib/board.c:326: /* preparee the WB code to LP0 location */
'prepare'

http://codereview.chromium.org/4841001/diff/1/board/tegra2/common/crypto/aes_...
File board/tegra2/common/crypto/aes_ref.c (right):

http://codereview.chromium.org/4841001/diff/1/board/tegra2/common/crypto/aes_...
board/tegra2/common/crypto/aes_ref.c:33: 
If Karl wrote this, his copyright should be included.

http://codereview.chromium.org/4841001/diff/1/board/tegra2/seaboard/Makefile
File board/tegra2/seaboard/Makefile (right):

http://codereview.chromium.org/4841001/diff/1/board/tegra2/seaboard/Makefile#...
board/tegra2/seaboard/Makefile:43: 
I'd love to see this ifdef'd here so the WB0 code wouldn't be built unless
desired - great for troubleshooting, special/smaller/faster incarnations of
U-Boot, etc.

[jimmzhang, 2010-11-13 01:51:45]

On 2010/11/12 00:12:40, Tom Warren wrote:
> A lot of this code is going to be hell to port to upstream U-Boot. Anton -
> please tell me this is one of the features we can hold back from upstream
(like
> graphics init, etc.).
> 
>
http://codereview.chromium.org/4841001/diff/1/arch/arm/cpu/arm_cortexa9/tegra...
> File arch/arm/cpu/arm_cortexa9/tegra2/ap20.c (right):
> 
>
http://codereview.chromium.org/4841001/diff/1/arch/arm/cpu/arm_cortexa9/tegra...
> arch/arm/cpu/arm_cortexa9/tegra2/ap20.c:973: 
> Why are you moving this to tegra2_start from cpu_start? If it's necessary for
> the WB0 code to work, please put a comment in here as to why it has to be
called
> here. If not, then put it in another patch w/a justification. Thanks.
> 
>
http://codereview.chromium.org/4841001/diff/1/arch/arm/cpu/arm_cortexa9/tegra...
> File arch/arm/cpu/arm_cortexa9/tegra2/ap20.h (left):
> 
>
http://codereview.chromium.org/4841001/diff/1/arch/arm/cpu/arm_cortexa9/tegra...
> arch/arm/cpu/arm_cortexa9/tegra2/ap20.h:113: 
> Why were these removed in this changelist? If they are not used, I happy to
see
> them go, but do it in another (cleanup) changelist, please.
> 
>
http://codereview.chromium.org/4841001/diff/1/arch/arm/cpu/arm_cortexa9/tegra...
> File arch/arm/cpu/arm_cortexa9/tegra2/warmboot_avp.S (right):
> 
>
http://codereview.chromium.org/4841001/diff/1/arch/arm/cpu/arm_cortexa9/tegra...
> arch/arm/cpu/arm_cortexa9/tegra2/warmboot_avp.S:65: 
> This code is very ugly - it looks like we're using some macros from
> 'nvbl_assembly.h' to prettify the normal ARM asm mnemonics, but it just makes
it
> stand out as being different from all other .S files in U-Boot!  Please don't
do
> this - rewrite it using the correct ARM syntax.  This is the kind of
craptastic
> code I'm going to spend inordinate amounts of time rewriting just to get it to
> pass muster @ mailto:u-boot@lists.denx.de.
> 
>
http://codereview.chromium.org/4841001/diff/1/arch/arm/include/asm/arch-tegra...
> File arch/arm/include/asm/arch-tegra2/nv_drf_asm.h (right):
> 
>
http://codereview.chromium.org/4841001/diff/1/arch/arm/include/asm/arch-tegra...
> arch/arm/include/asm/arch-tegra2/nv_drf_asm.h:55:
> *-----------------------------------------------------------------------------
> Nor should they - write assembler code in assembly!
> 
> Please fix this, also. There's no way we should be having any reference to MS
C
> in U-Boot or ARM code!
> 
>
http://codereview.chromium.org/4841001/diff/1/arch/arm/include/asm/arch-tegra...
> File arch/arm/include/asm/arch-tegra2/nvbl_assembly.h (right):
> 
>
http://codereview.chromium.org/4841001/diff/1/arch/arm/include/asm/arch-tegra...
> arch/arm/include/asm/arch-tegra2/nvbl_assembly.h:76: #define NVOS_IS_WINDOWS 1
> Remove all references to Windows/WIN32 - this is U-Boot
> 
>
http://codereview.chromium.org/4841001/diff/1/arch/arm/include/asm/arch-tegra...
> arch/arm/include/asm/arch-tegra2/nvbl_assembly.h:128: 
> This crap shouldn't be needed with our ARM gcc/ar/as/ld toolset. Please
> rewrite/remove. It just makes the assembly code ugly & will never pass on
> mailto:u-boot@lists.denx.de (U-Boot upstream).
> 
> http://codereview.chromium.org/4841001/diff/1/arch/arm/lib/board.c
> File arch/arm/lib/board.c (right):
> 
> http://codereview.chromium.org/4841001/diff/1/arch/arm/lib/board.c#newcode68
> arch/arm/lib/board.c:68: int prepare_wb_code(unsigned int, unsigned int);
> You should put protos/forward declarations in a header file.
> 
> http://codereview.chromium.org/4841001/diff/1/arch/arm/lib/board.c#newcode326
> arch/arm/lib/board.c:326: /* preparee the WB code to LP0 location */
> 'prepare'
> 
>
http://codereview.chromium.org/4841001/diff/1/board/tegra2/common/crypto/aes_...
> File board/tegra2/common/crypto/aes_ref.c (right):
> 
>
http://codereview.chromium.org/4841001/diff/1/board/tegra2/common/crypto/aes_...
> board/tegra2/common/crypto/aes_ref.c:33: 
> If Karl wrote this, his copyright should be included.
> 
> http://codereview.chromium.org/4841001/diff/1/board/tegra2/seaboard/Makefile
> File board/tegra2/seaboard/Makefile (right):
> 
>
http://codereview.chromium.org/4841001/diff/1/board/tegra2/seaboard/Makefile#...
> board/tegra2/seaboard/Makefile:43: 
> I'd love to see this ifdef'd here so the WB0 code wouldn't be built unless
> desired - great for troubleshooting, special/smaller/faster incarnations of
> U-Boot, etc.

My understanding is the warmboot is equivalent to "Standby" in term of Windows.
If it is a feature that Chromeos must have (though user may turn on or off), as
part of firmware, we can't add the code in when user wants. We have to pre-build
it in. Defining as configurable module may only cause potential bugs and
confusion.

[robotboy, 2010-11-15 21:34:39]

Actaully, I think having this #definable makes sense.  The full ChromiumOS
firmware will use it.  But we want our changes to U-Boot to be useful to more
than just ChromiumOS.  In particular, it would be nice to be able to build
without this if/when we need to have a second stage U-Boot in the R/W filesystem
image.  Do you see this as a significant impediment to landing this code?

Thanks,
    Anton

[yelin, 2010-11-15 23:21:19]

http://codereview.chromium.org/4841001/diff/1/arch/arm/cpu/arm_cortexa9/tegra...
File arch/arm/cpu/arm_cortexa9/tegra2/ap20.c (right):

http://codereview.chromium.org/4841001/diff/1/arch/arm/cpu/arm_cortexa9/tegra...
arch/arm/cpu/arm_cortexa9/tegra2/ap20.c:973: 
On 2010/11/12 00:12:40, Tom Warren wrote:
> Why are you moving this to tegra2_start from cpu_start? If it's necessary for
> the WB0 code to work, please put a comment in here as to why it has to be
called
> here. If not, then put it in another patch w/a justification. Thanks.

Merge error. Revert it.

http://codereview.chromium.org/4841001/diff/1/arch/arm/cpu/arm_cortexa9/tegra...
File arch/arm/cpu/arm_cortexa9/tegra2/ap20.h (left):

http://codereview.chromium.org/4841001/diff/1/arch/arm/cpu/arm_cortexa9/tegra...
arch/arm/cpu/arm_cortexa9/tegra2/ap20.h:113: 
On 2010/11/12 00:12:40, Tom Warren wrote:
> Why were these removed in this changelist? If they are not used, I happy to
see
> them go, but do it in another (cleanup) changelist, please.

Done.

http://codereview.chromium.org/4841001/diff/1/arch/arm/cpu/arm_cortexa9/tegra...
File arch/arm/cpu/arm_cortexa9/tegra2/warmboot_avp.S (right):

http://codereview.chromium.org/4841001/diff/1/arch/arm/cpu/arm_cortexa9/tegra...
arch/arm/cpu/arm_cortexa9/tegra2/warmboot_avp.S:65: 
On 2010/11/12 00:12:40, Tom Warren wrote:
> This code is very ugly - it looks like we're using some macros from
> 'nvbl_assembly.h' to prettify the normal ARM asm mnemonics, but it just makes
it
> stand out as being different from all other .S files in U-Boot!  Please don't
do
> this - rewrite it using the correct ARM syntax.  This is the kind of
craptastic
> code I'm going to spend inordinate amounts of time rewriting just to get it to
> pass muster @ mailto:u-boot@lists.denx.de.

Done.

http://codereview.chromium.org/4841001/diff/1/arch/arm/include/asm/arch-tegra...
File arch/arm/include/asm/arch-tegra2/nv_drf_asm.h (right):

http://codereview.chromium.org/4841001/diff/1/arch/arm/include/asm/arch-tegra...
arch/arm/include/asm/arch-tegra2/nv_drf_asm.h:55:
*-----------------------------------------------------------------------------
On 2010/11/12 00:12:40, Tom Warren wrote:
> Nor should they - write assembler code in assembly!
> 
> Please fix this, also. There's no way we should be having any reference to MS
C
> in U-Boot or ARM code!

Done.

http://codereview.chromium.org/4841001/diff/1/arch/arm/include/asm/arch-tegra...
File arch/arm/include/asm/arch-tegra2/nvbl_assembly.h (right):

http://codereview.chromium.org/4841001/diff/1/arch/arm/include/asm/arch-tegra...
arch/arm/include/asm/arch-tegra2/nvbl_assembly.h:76: #define NVOS_IS_WINDOWS 1
On 2010/11/12 00:12:40, Tom Warren wrote:
> Remove all references to Windows/WIN32 - this is U-Boot

This file is gone.

http://codereview.chromium.org/4841001/diff/1/arch/arm/lib/board.c
File arch/arm/lib/board.c (right):

http://codereview.chromium.org/4841001/diff/1/arch/arm/lib/board.c#newcode68
arch/arm/lib/board.c:68: int prepare_wb_code(unsigned int, unsigned int);
On 2010/11/12 00:12:40, Tom Warren wrote:
> You should put protos/forward declarations in a header file.

Done.

http://codereview.chromium.org/4841001/diff/1/arch/arm/lib/board.c#newcode326
arch/arm/lib/board.c:326: /* preparee the WB code to LP0 location */
On 2010/11/12 00:12:40, Tom Warren wrote:
> 'prepare'

Done.

http://codereview.chromium.org/4841001/diff/1/board/tegra2/common/crypto/aes_...
File board/tegra2/common/crypto/aes_ref.c (right):

http://codereview.chromium.org/4841001/diff/1/board/tegra2/common/crypto/aes_...
board/tegra2/common/crypto/aes_ref.c:33: 
On 2010/11/12 00:12:40, Tom Warren wrote:
> If Karl wrote this, his copyright should be included.

Can not find any copyright from Karl on the web.

http://codereview.chromium.org/4841001/diff/1/board/tegra2/seaboard/Makefile
File board/tegra2/seaboard/Makefile (right):

http://codereview.chromium.org/4841001/diff/1/board/tegra2/seaboard/Makefile#...
board/tegra2/seaboard/Makefile:43: 
On 2010/11/12 00:12:40, Tom Warren wrote:
> I'd love to see this ifdef'd here so the WB0 code wouldn't be built unless
> desired - great for troubleshooting, special/smaller/faster incarnations of
> U-Boot, etc.

Done.

[Tom Warren, 2010-11-16 16:44:11]

Nicely done, Yen. Thanks.

Have you built without CONFIG_TEGRA2_LP0 defined to ensure it builds and works
as before?

Once that's confirmed, and my final comments are addressed, then LGTM.

http://codereview.chromium.org/4841001/diff/12001/arch/arm/cpu/arm_cortexa9/t...
File arch/arm/cpu/arm_cortexa9/tegra2/ap20.c (right):

http://codereview.chromium.org/4841001/diff/12001/arch/arm/cpu/arm_cortexa9/t...
arch/arm/cpu/arm_cortexa9/tegra2/ap20.c:944: *jtagReg = 192;
Please use #defined values from tegra2.h here, not magic numbers. 
 = (u32*)NV_ADDRESS_MAP_APB_MISC_BASE+JTAG_REG, etc.

And change the '192' to 0xC0 - it more clearly indicates which bits you are
setting (7&6).

Thanks.

http://codereview.chromium.org/4841001/diff/12001/include/configs/chromeos/te...
File include/configs/chromeos/tegra2/seaboard/recovery.h (right):

http://codereview.chromium.org/4841001/diff/12001/include/configs/chromeos/te...
include/configs/chromeos/tegra2/seaboard/recovery.h:42:
"platform_extras=mem=384M@0M nvmem=128M@384M mem=512M@512M "\
Cleaner to just put the #ifdef CONFIG_TEGRA2_LP0 here, around the 'lp0_vec='
line, but OK for now.

http://codereview.chromium.org/4841001/diff/12001/include/configs/tegra2-comm...
File include/configs/tegra2-common.h (right):

http://codereview.chromium.org/4841001/diff/12001/include/configs/tegra2-comm...
include/configs/tegra2-common.h:218: /* make sure the following has the same
value of TEGRA_LP0_DEFAULT_ADDR */
This set of #defines is awfully messy. Better to just put a #ifdef
CONFIG_TEGRA2_LP0 around the 'lp0_vec=' line below.

http://codereview.chromium.org/4841001/diff/12001/include/configs/tegra2-comm...
include/configs/tegra2-common.h:296: "${lp0_vec} " \
Shouldn't this be "lp0_vec=$(lp0_vec) "? And below in mmcboot.

[Tom Warren, 2010-11-16 17:51:16]

Yen - I'm seeing these build errors when applying your patch to
u-boot-next/v2010.09:

ap20.c: In function 'NvBlSaveSdramParams':
ap20.c:147: warning: implicit declaration of function 'memcpy'
../common/crypto/crypto.c: In function 'sign_data_block':
../common/crypto/crypto.c:325: warning: cast to pointer from integer of
different size

Please fix these.

[Tom Warren, 2010-11-16 18:34:22]

More ifdef's are needed in AP20.C, and the ifdefs in the Makefiles need to be
fixed - use 'ifdef ..' and _not_ #ifdef.

Thanks!

http://codereview.chromium.org/4841001/diff/12001/arch/arm/cpu/arm_cortexa9/t...
File arch/arm/cpu/arm_cortexa9/tegra2/Makefile (right):

http://codereview.chromium.org/4841001/diff/12001/arch/arm/cpu/arm_cortexa9/t...
arch/arm/cpu/arm_cortexa9/tegra2/Makefile:32: #ifdef CONFIG_TEGRA2_LP0
You can't use a # in front of a conditional like you do in C code - it just
becomes a comment/commented out.

Change these to plain 'ifdef CONFIG_TEGRA2_LP0'/'endif' (i.e. remove the #'s so
this is really conditional.

http://codereview.chromium.org/4841001/diff/12001/arch/arm/cpu/arm_cortexa9/t...
File arch/arm/cpu/arm_cortexa9/tegra2/ap20.c (right):

http://codereview.chromium.org/4841001/diff/12001/arch/arm/cpu/arm_cortexa9/t...
arch/arm/cpu/arm_cortexa9/tegra2/ap20.c:140: 
Please #ifdef this function with CONFIG_TEGRA2_LP0 so it isn't compiled in.

http://codereview.chromium.org/4841001/diff/12001/arch/arm/cpu/arm_cortexa9/t...
arch/arm/cpu/arm_cortexa9/tegra2/ap20.c:931: // Save Sdram params to PMC 2, 4,
and 24 for WB0
Please #ifdef CONFIG_TEGRA2_LP0 here, also.

http://codereview.chromium.org/4841001/diff/12001/board/tegra2/harmony/Makefile
File board/tegra2/harmony/Makefile (right):

http://codereview.chromium.org/4841001/diff/12001/board/tegra2/harmony/Makefi...
board/tegra2/harmony/Makefile:38: #ifdef CONFIG_TEGRA2_LP0
You can't use a # in front of a conditional like you do in C code - it just
becomes a comment/commented out. Change these to plain 'ifdef
CONFIG_TEGRA2_LP0'/'endif' (i.e. remove the #'s so this is really conditional.

http://codereview.chromium.org/4841001/diff/12001/board/tegra2/seaboard/Makefile
File board/tegra2/seaboard/Makefile (right):

http://codereview.chromium.org/4841001/diff/12001/board/tegra2/seaboard/Makef...
board/tegra2/seaboard/Makefile:41: #ifdef CONFIG_TEGRA2_LP0
You can't use a # in front of a conditional like you do in C code - it just
becomes a comment/commented out. Change these to plain 'ifdef
CONFIG_TEGRA2_LP0'/'endif' (i.e. remove the #'s so this is really conditional.

[yelin, 2010-11-16 23:41:04]

http://codereview.chromium.org/4841001/diff/12001/arch/arm/cpu/arm_cortexa9/t...
File arch/arm/cpu/arm_cortexa9/tegra2/Makefile (right):

http://codereview.chromium.org/4841001/diff/12001/arch/arm/cpu/arm_cortexa9/t...
arch/arm/cpu/arm_cortexa9/tegra2/Makefile:32: #ifdef CONFIG_TEGRA2_LP0
On 2010/11/16 18:34:27, Tom Warren wrote:
> You can't use a # in front of a conditional like you do in C code - it just
> becomes a comment/commented out.
> 
> Change these to plain 'ifdef CONFIG_TEGRA2_LP0'/'endif' (i.e. remove the #'s
so
> this is really conditional.

Done.

http://codereview.chromium.org/4841001/diff/12001/arch/arm/cpu/arm_cortexa9/t...
File arch/arm/cpu/arm_cortexa9/tegra2/ap20.c (right):

http://codereview.chromium.org/4841001/diff/12001/arch/arm/cpu/arm_cortexa9/t...
arch/arm/cpu/arm_cortexa9/tegra2/ap20.c:140: 
On 2010/11/16 18:34:27, Tom Warren wrote:
> Please #ifdef this function with CONFIG_TEGRA2_LP0 so it isn't compiled in.

Done.

http://codereview.chromium.org/4841001/diff/12001/arch/arm/cpu/arm_cortexa9/t...
arch/arm/cpu/arm_cortexa9/tegra2/ap20.c:931: // Save Sdram params to PMC 2, 4,
and 24 for WB0
On 2010/11/16 18:34:27, Tom Warren wrote:
> Please #ifdef CONFIG_TEGRA2_LP0 here, also.

Done.

http://codereview.chromium.org/4841001/diff/12001/arch/arm/cpu/arm_cortexa9/t...
arch/arm/cpu/arm_cortexa9/tegra2/ap20.c:944: *jtagReg = 192;
On 2010/11/16 16:44:11, Tom Warren wrote:
> Please use #defined values from tegra2.h here, not magic numbers. 
>  = (u32*)NV_ADDRESS_MAP_APB_MISC_BASE+JTAG_REG, etc.
> 
> And change the '192' to 0xC0 - it more clearly indicates which bits you are
> setting (7&6).
> 
> Thanks.

Done.

http://codereview.chromium.org/4841001/diff/12001/board/tegra2/harmony/Makefile
File board/tegra2/harmony/Makefile (right):

http://codereview.chromium.org/4841001/diff/12001/board/tegra2/harmony/Makefi...
board/tegra2/harmony/Makefile:38: #ifdef CONFIG_TEGRA2_LP0
On 2010/11/16 18:34:27, Tom Warren wrote:
> You can't use a # in front of a conditional like you do in C code - it just
> becomes a comment/commented out. Change these to plain 'ifdef
> CONFIG_TEGRA2_LP0'/'endif' (i.e. remove the #'s so this is really conditional.


Done.

http://codereview.chromium.org/4841001/diff/12001/board/tegra2/seaboard/Makefile
File board/tegra2/seaboard/Makefile (right):

http://codereview.chromium.org/4841001/diff/12001/board/tegra2/seaboard/Makef...
board/tegra2/seaboard/Makefile:41: #ifdef CONFIG_TEGRA2_LP0
On 2010/11/16 18:34:27, Tom Warren wrote:
> You can't use a # in front of a conditional like you do in C code - it just
> becomes a comment/commented out. Change these to plain 'ifdef
> CONFIG_TEGRA2_LP0'/'endif' (i.e. remove the #'s so this is really conditional.


Done.

http://codereview.chromium.org/4841001/diff/12001/include/configs/chromeos/te...
File include/configs/chromeos/tegra2/seaboard/recovery.h (right):

http://codereview.chromium.org/4841001/diff/12001/include/configs/chromeos/te...
include/configs/chromeos/tegra2/seaboard/recovery.h:42:
"platform_extras=mem=384M@0M nvmem=128M@384M mem=512M@512M "\
On 2010/11/16 16:44:11, Tom Warren wrote:
> Cleaner to just put the #ifdef CONFIG_TEGRA2_LP0 here, around the 'lp0_vec='
> line, but OK for now.

I was hoping I can do that way too. But, that does not work, because #ifdef can
not be inside the whole string.

http://codereview.chromium.org/4841001/diff/12001/include/configs/tegra2-comm...
File include/configs/tegra2-common.h (right):

http://codereview.chromium.org/4841001/diff/12001/include/configs/tegra2-comm...
include/configs/tegra2-common.h:218: /* make sure the following has the same
value of TEGRA_LP0_DEFAULT_ADDR */
On 2010/11/16 16:44:11, Tom Warren wrote:
> This set of #defines is awfully messy. Better to just put a #ifdef
> CONFIG_TEGRA2_LP0 around the 'lp0_vec=' line below.

No, I can not do that. The #ifdef can not be part of
CONFIG_DEFAULT_ENV_SETTINGS.

http://codereview.chromium.org/4841001/diff/12001/include/configs/tegra2-comm...
include/configs/tegra2-common.h:296: "${lp0_vec} " \
On 2010/11/16 16:44:11, Tom Warren wrote:
> Shouldn't this be "lp0_vec=$(lp0_vec) "? And below in mmcboot.

No. That is correct. It is the same way as "${mem} " about 5 lines above.

[JimmyZ.nvidia, 2010-11-19 19:39:32]

On 2010/11/16 23:41:04, yelin wrote:
>
http://codereview.chromium.org/4841001/diff/12001/arch/arm/cpu/arm_cortexa9/t...
> File arch/arm/cpu/arm_cortexa9/tegra2/Makefile (right):
> 
>
http://codereview.chromium.org/4841001/diff/12001/arch/arm/cpu/arm_cortexa9/t...
> arch/arm/cpu/arm_cortexa9/tegra2/Makefile:32: #ifdef CONFIG_TEGRA2_LP0
> On 2010/11/16 18:34:27, Tom Warren wrote:
> > You can't use a # in front of a conditional like you do in C code - it just
> > becomes a comment/commented out.
> > 
> > Change these to plain 'ifdef CONFIG_TEGRA2_LP0'/'endif' (i.e. remove the #'s
> so
> > this is really conditional.
> 
> Done.
> 
>
http://codereview.chromium.org/4841001/diff/12001/arch/arm/cpu/arm_cortexa9/t...
> File arch/arm/cpu/arm_cortexa9/tegra2/ap20.c (right):
> 
>
http://codereview.chromium.org/4841001/diff/12001/arch/arm/cpu/arm_cortexa9/t...
> arch/arm/cpu/arm_cortexa9/tegra2/ap20.c:140: 
> On 2010/11/16 18:34:27, Tom Warren wrote:
> > Please #ifdef this function with CONFIG_TEGRA2_LP0 so it isn't compiled in.
> 
> Done.
> 
>
http://codereview.chromium.org/4841001/diff/12001/arch/arm/cpu/arm_cortexa9/t...
> arch/arm/cpu/arm_cortexa9/tegra2/ap20.c:931: // Save Sdram params to PMC 2, 4,
> and 24 for WB0
> On 2010/11/16 18:34:27, Tom Warren wrote:
> > Please #ifdef CONFIG_TEGRA2_LP0 here, also.
> 
> Done.
> 
>
http://codereview.chromium.org/4841001/diff/12001/arch/arm/cpu/arm_cortexa9/t...
> arch/arm/cpu/arm_cortexa9/tegra2/ap20.c:944: *jtagReg = 192;
> On 2010/11/16 16:44:11, Tom Warren wrote:
> > Please use #defined values from tegra2.h here, not magic numbers. 
> >  = (u32*)NV_ADDRESS_MAP_APB_MISC_BASE+JTAG_REG, etc.
> > 
> > And change the '192' to 0xC0 - it more clearly indicates which bits you are
> > setting (7&6).
> > 
> > Thanks.
> 
> Done.
> 
>
http://codereview.chromium.org/4841001/diff/12001/board/tegra2/harmony/Makefile
> File board/tegra2/harmony/Makefile (right):
> 
>
http://codereview.chromium.org/4841001/diff/12001/board/tegra2/harmony/Makefi...
> board/tegra2/harmony/Makefile:38: #ifdef CONFIG_TEGRA2_LP0
> On 2010/11/16 18:34:27, Tom Warren wrote:
> > You can't use a # in front of a conditional like you do in C code - it just
> > becomes a comment/commented out. Change these to plain 'ifdef
> > CONFIG_TEGRA2_LP0'/'endif' (i.e. remove the #'s so this is really
conditional.
> 
> 
> Done.
> 
>
http://codereview.chromium.org/4841001/diff/12001/board/tegra2/seaboard/Makefile
> File board/tegra2/seaboard/Makefile (right):
> 
>
http://codereview.chromium.org/4841001/diff/12001/board/tegra2/seaboard/Makef...
> board/tegra2/seaboard/Makefile:41: #ifdef CONFIG_TEGRA2_LP0
> On 2010/11/16 18:34:27, Tom Warren wrote:
> > You can't use a # in front of a conditional like you do in C code - it just
> > becomes a comment/commented out. Change these to plain 'ifdef
> > CONFIG_TEGRA2_LP0'/'endif' (i.e. remove the #'s so this is really
conditional.
> 
> 
> Done.
> 
>
http://codereview.chromium.org/4841001/diff/12001/include/configs/chromeos/te...
> File include/configs/chromeos/tegra2/seaboard/recovery.h (right):
> 
>
http://codereview.chromium.org/4841001/diff/12001/include/configs/chromeos/te...
> include/configs/chromeos/tegra2/seaboard/recovery.h:42:
> "platform_extras=mem=384M@0M nvmem=128M@384M mem=512M@512M "\
> On 2010/11/16 16:44:11, Tom Warren wrote:
> > Cleaner to just put the #ifdef CONFIG_TEGRA2_LP0 here, around the 'lp0_vec='
> > line, but OK for now.
> 
> I was hoping I can do that way too. But, that does not work, because #ifdef
can
> not be inside the whole string.
> 
>
http://codereview.chromium.org/4841001/diff/12001/include/configs/tegra2-comm...
> File include/configs/tegra2-common.h (right):
> 
>
http://codereview.chromium.org/4841001/diff/12001/include/configs/tegra2-comm...
> include/configs/tegra2-common.h:218: /* make sure the following has the same
> value of TEGRA_LP0_DEFAULT_ADDR */
> On 2010/11/16 16:44:11, Tom Warren wrote:
> > This set of #defines is awfully messy. Better to just put a #ifdef
> > CONFIG_TEGRA2_LP0 around the 'lp0_vec=' line below.
> 
> No, I can not do that. The #ifdef can not be part of
> CONFIG_DEFAULT_ENV_SETTINGS.
> 
>
http://codereview.chromium.org/4841001/diff/12001/include/configs/tegra2-comm...
> include/configs/tegra2-common.h:296: "${lp0_vec} " \
> On 2010/11/16 16:44:11, Tom Warren wrote:
> > Shouldn't this be "lp0_vec=$(lp0_vec) "? And below in mmcboot.
> 
> No. That is correct. It is the same way as "${mem} " about 5 lines above.

LGTM

[robotboy, 2010-11-19 22:48:55]

There are a couple of files that need GPL headers.  There are also a number of
80 column wrapping issues.  The long macros I am less concerned about, but the
comments it would be nice to clean up.

Thanks,
    Anton

http://codereview.chromium.org/4841001/diff/24002/board/tegra2/common/crypto/...
File board/tegra2/common/crypto/crypto.h (right):

http://codereview.chromium.org/4841001/diff/24002/board/tegra2/common/crypto/...
board/tegra2/common/crypto/crypto.h:9: */
This needs to have a GPL v2 header.  Thanks.

http://codereview.chromium.org/4841001/diff/24002/board/tegra2/common/crypto/...
File board/tegra2/common/crypto/nvaes_ref.h (right):

http://codereview.chromium.org/4841001/diff/24002/board/tegra2/common/crypto/...
board/tegra2/common/crypto/nvaes_ref.h:9: */
This also needs to have a GPL v2 header.

[Tom Warren, 2010-11-22 20:11:15]

patch doesn't apply against u-boot-next v2010.09 TOT (2eacdfb) - looks like a
conflict with Rakesh's keyboard change to seaboard/recovery.h. Once that's done,
should be GTG.

[robotboy, 2010-11-25 00:09:06]

I'm sorry, I missed the latest patch set.  LGTM, I'll push.  Thanks.