From mboxrd@z Thu Jan  1 00:00:00 1970
From: Wolfgang Denk <wd@denx.de>
Date: Mon, 02 May 2011 17:27:00 +0200
Subject: [U-Boot] [PATCH v9 05/10] nds32/ag101: lowlevel_init.S of ag101
In-Reply-To: <1304342712-17120-5-git-send-email-macpaul@andestech.com>
References: <1302180333-25372-1-git-send-email-macpaul@andestech.com>
	<1304342712-17120-5-git-send-email-macpaul@andestech.com>
Message-ID: <20110502152700.53274BB0D9@gemini.denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Dear Macpaul Lin,

In message <1304342712-17120-5-git-send-email-macpaul@andestech.com> you wrote:
> lowlevel_init.S is a peripheral initial procedure of ag101.
> It configures onboard dram, clock, and power settings.
> It also prepars the dram environment before moving u-boot
> from rom and flash into dram.
> 
> This version of lowlevel_init.S also replace hardcode value
> by MARCO defines from the GPL version andesboot for better
> code quality.
> 
> Signed-off-by: Macpaul Lin <macpaul@andestech.com>
> 
> Changes from v1-v4:
>   - Code clean up and formatting style.
> Changes from v5-v6
>   - Change hard code value into MARCO definitions.
>   - ftsmc010
>     - Fix FTSMC020_TPR_AT2 from 1 to 3 (0xff3ff)
>   - ftsdmc021
>     - Fix hardcoded address of CR1, CR2, TR1, TR2, BANK0 registers.
>     - Fix the default configuration value of FTSDMC and FTSMC controller.
>   - Remove some ftpmu010 and flash probe code to C functions.
> Changes for v7-v9:
>   - No change.
> ---

Patch changelog belongs _below_ the "---" line, not above.

>  arch/nds32/cpu/n1213/ag101/lowlevel_init.S |  160 ++++++++++++++++++++++++++++
>  1 files changed, 160 insertions(+), 0 deletions(-)
>  create mode 100644 arch/nds32/cpu/n1213/ag101/lowlevel_init.S

This file alone makes no sense.  Pluease submit as a single commit
together with the other board code.

...
> +	/*
> +	 * relocation, copy ROM code to SDRAM (current at 0x10000000)
> +	 */
> +	li	$r4, CONFIG_SYS_RELO_ADDR		! 0x10000000
> +	li	$r5, 0x0
> +	la	$r1, relo_base
> +	sub	$r2, $r0, $r1
> +	sethi	$r6, hi20(andesboot_end)
> +	ori	$r6, $r6, lo12(andesboot_end)
> +	add	$r6, $r6, $r2
> +1:
> +	lwi	$r7, [$r5]
> +	swi	$r7, [$r4]
> +	addi	$r5, $r5, #4
> +	addi	$r4, $r4, #4
> +	blt	$r5, $r6, 1b

You are using  non-standard and completely undocumented CONFIG option
here, CONFIG_SYS_RELO_ADDR.  Please don't.  I think when adding new
architecture support we should avaid errors done with other
architectures in the past (see ARM, MIPS, ...).

Please implement proper relocation to a dynamically determined
address at the upper end of RAM. Keep in mind that the intention is
to have only a single lib/board.c isnatead of a collection of
slightly different arch-specific arch/*/board.c versions.

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
In an organization, each person rises to the level of his own  incom-
petency                                         - The Peter Principle