[U-Boot] [1/5]devkit8000 nand_spl: armv7 support nand_spl boot
Andreas Bießmann
andreas.devel at googlemail.com
Wed Jun 29 10:27:46 CEST 2011
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
More information about the U-Boot
mailing list