From mboxrd@z Thu Jan  1 00:00:00 1970
From: =?ISO-8859-1?Q?Andreas_Bie=DFmann?= <andreas.devel@googlemail.com>
Date: Wed, 29 Jun 2011 10:27:46 +0200
Subject: [U-Boot] [1/5]devkit8000 nand_spl: armv7 support nand_spl boot
In-Reply-To: <1309270480-31918-2-git-send-email-schwarz@corscience.de>
References: <1309270480-31918-1-git-send-email-schwarz@corscience.de>
	<1309270480-31918-2-git-send-email-schwarz@corscience.de>
Message-ID: <4E0AE202.6060708@gmail.com>
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 Simon Schwarz,

(this is a review of RFC for omap3 nand spl Simon does for his bachelor
thesis, there is one question in regarding the current 'SPL framework
redesign' discussion -> entry point for spl code).

Am 28.06.2011 16:14, schrieb simonschwarzcor at googlemail.com:

<snip long line>

> --
> 
> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
> index d91ae12..ebbfce4 100644
> --- a/arch/arm/cpu/armv7/start.S
> +++ b/arch/arm/cpu/armv7/start.S
> @@ -33,6 +33,11 @@
>  #include <config.h>
>  #include <version.h>
>  
> +/* prevent lowlevel init if this is not the preloader*/
> +#if defined(CONFIG_SPL) && !defined(CONFIG_PRELOADER)
> +#define CONFIG_SKIP_LOWLEVEL_INIT
> +#endif
> +

not here, use your board configuration header for this.

>  .globl _start
>  _start: b	reset
>  	ldr	pc, _undefined_instruction
> @@ -43,6 +48,17 @@ _start: b	reset
>  	ldr	pc, _irq
>  	ldr	pc, _fiq
>  
> +#ifdef CONFIG_PRELOADER
> +/* If in Preloader don't use interrupts...*/
> +_undefined_instruction: .word undefined_instruction
> +_software_interrupt:    .word _software_interrupt
> +_prefetch_abort:    .word _prefetch_abort
> +_data_abort:        .word data_abort
> +_not_used:      .word _not_used
> +_irq:           .word _irq
> +_fiq:           .word _fiq
> +_pad:           .word 0x12345678 /* now 16*4=64 */
> +#else
>  _undefined_instruction: .word undefined_instruction
>  _software_interrupt:	.word software_interrupt
>  _prefetch_abort:	.word prefetch_abort
> @@ -51,6 +67,7 @@ _not_used:		.word not_used
>  _irq:			.word irq
>  _fiq:			.word fiq
>  _pad:			.word 0x12345678 /* now 16*4=64 */
> +#endif /* CONFIG_PRELOADER */

why resetting the vector table? just do not enable irq at all should
also do the job, isn't it?

>  .global _end_vect
>  _end_vect:
>  
> @@ -85,6 +102,7 @@ _armboot_start:
>  /*
>   * These are defined in the board-specific linker script.
>   */
> +#ifndef CONFIG_PRELOADER

isn't that SPL specific?

>  .globl _bss_start_ofs
>  _bss_start_ofs:
>  	.word __bss_start - _start
> @@ -96,6 +114,16 @@ _bss_end_ofs:
>  .globl _end_ofs
>  _end_ofs:
>  	.word _end - _start
> +#else
> +/* preserved the _ofs because although there is no offset */
> +.globl _bss_start_ofs
> +_bss_start_ofs:
> +	.word __bss_start
> +
> +.globl _bss_end_ofs
> +_bss_end_ofs:
> +	.word __bss_end__
> +#endif
>  
>  #ifdef CONFIG_USE_IRQ
>  /* IRQ stack memory (calculated at run-time) */
> @@ -153,8 +181,15 @@ next:
>  	/* the mask ROM code should have PLL and others stable */
>  #ifndef CONFIG_SKIP_LOWLEVEL_INIT
>  	bl	cpu_init_crit
> -#endif
> -
> +#endif /* #ifdef CONFIG_PRELOADER */

ok to mark this #endif, but do not use  '/* #ifdef', it looks like
commented out ifdef.

> +
> +#ifdef CONFIG_PRELOADER
> +call_board_init_spl:
> +    ldr sp, =(CONFIG_SYS_INIT_SP_ADDR)
> +    bic sp, sp, #7 /* 8-byte alignment for ABI compliance */
> +    ldr r0,=0x00000000
> +    bl  board_init_spl

why a new interface for entry point? You could just use
call_board_init_f() even for spl (just do not compile lib/board.c and
provide another one for spl). -> this should be discussed in 'SPL
framework redesign'.

> +#else
>  /* Set stackpointer in internal RAM to call board_init_f */
>  call_board_init_f:
>  	ldr	sp, =(CONFIG_SYS_INIT_SP_ADDR)
> @@ -182,10 +217,8 @@ stack_setup:
>  	mov	sp, r4
>  
>  	adr	r0, _start
> -#ifndef CONFIG_PRELOADER

hmm ... that ifndef was intentionally there

>  	cmp	r0, r6
>  	beq	clear_bss		/* skip relocation */
> -#endif
>  	mov	r1, r6			/* r1 <- scratch for copy_loop */
>  	ldr	r3, _bss_start_ofs
>  	add	r2, r0, r3		/* r2 <- source end address	    */
> @@ -196,7 +229,6 @@ copy_loop:
>  	cmp	r0, r2			/* until source end address [r2]    */
>  	blo	copy_loop
>  
> -#ifndef CONFIG_PRELOADER
>  	/*
>  	 * fix .rel.dyn relocations
>  	 */
> @@ -248,7 +280,6 @@ clbss_l:str	r2, [r0]		/* clear loop...		    */
>  	add	r0, r0, #4
>  	cmp	r0, r1
>  	bne	clbss_l
> -#endif	/* #ifndef CONFIG_PRELOADER */
>  
>  /*
>   * We are done. Do not return, instead branch to second part of board
> @@ -265,16 +296,18 @@ jump_2_ram:
>  	/* jump to it ... */
>  	mov	pc, lr
>  
> -_board_init_r_ofs:
> -	.word board_init_r - _start
> -
>  _rel_dyn_start_ofs:
> -	.word __rel_dyn_start - _start
> +    .word __rel_dyn_start - _start
>  _rel_dyn_end_ofs:
> -	.word __rel_dyn_end - _start
> +    .word __rel_dyn_end - _start
>  _dynsym_start_ofs:
> -	.word __dynsym_start - _start
> +    .word __dynsym_start - _start
>  
> +_board_init_r_ofs:
> +	.word board_init_r - _start

these changes are not necessary!

> +#endif /* #ifndef CONFIG_PRELOADER */
> +
> +#ifndef CONFIG_SKIP_LOWLEVEL_INIT

this shouldn't be required here. If there is an error in start.S please
provide another patch fixing the error.

>  /*************************************************************************
>   *
>   * CPU_init_critical registers
> @@ -311,6 +344,8 @@ cpu_init_crit:
>  	bl	lowlevel_init		@ go setup pll,mux,memory
>  	mov	lr, ip			@ restore link
>  	mov	pc, lr			@ back to my caller
> +#endif /* CONFIG_SKIP_LOWLEVEL_INIT */
> +
>  /*
>   *************************************************************************
>   *
> @@ -437,6 +472,7 @@ cpu_init_crit:
>  /*
>   * exception handlers
>   */
> +#ifndef CONFIG_PRELOADER
>  	.align	5
>  undefined_instruction:
>  	get_bad_stack
> @@ -499,3 +535,14 @@ fiq:
>  	bl	do_fiq
>  
>  #endif
> +#endif /* CONFIG_PRELOADER */

#endif /* CONFIG_PRELOADER */ followed by #ifdef CONFIG_PRELOADER? ...
how about #else?

> +

no empty line then

> +#ifdef CONFIG_PRELOADER
> +	.align 	5
> +undefined_instruction:
> +	b undefined_instruction
> +
> +	.align	5
> +data_abort:
> +	b data_abort
> +#endif

I guess you want to save space by reducing the irq vector overhead. But
shouldn't this be some #ifdef CONFIG_USE_IRQ instead of CONFIG_PRELOADER?

> diff --git a/arch/arm/lib/reset.c b/arch/arm/lib/reset.c
> index 08e6acb..ad444cb 100644
> --- a/arch/arm/lib/reset.c
> +++ b/arch/arm/lib/reset.c
> @@ -44,8 +44,9 @@ int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  	puts ("resetting ...\n");
>  
>  	udelay (50000);				/* wait 50 ms */
> -
> +#ifndef CONFIG_PRELOADER
>  	disable_interrupts();
> +#endif

again, use CONFIG_USE_IRQ here would be better. But providing an empty
disable_interrupts() in either case you can drop these #ifndef here too.

>  	reset_cpu(0);
>  
>  	/*NOTREACHED*/

regards

Andreas Bie?mann