[U-Boot] [RFC PATCH 2/2] x86: fsp: Move FspInitEntry call to board_init_f()

[Andrew Bradford]

Hi Bin,

On 06/01 20:31, Bin Meng wrote:
> The call to FspInitEntry is done in arch/x86/lib/fsp/fsp_car.S so far.
> It worked pretty well but looks not that good. Apart from doing too
> much work than just enabling CAR, it cannot read the configuration
> data from device tree at that time. Now we want to move it a little
> bit later as part of init_sequence_f[] being called by board_init_f().
> This way it looks and works better in the U-Boot initialization path.
> 
> Due to FSP's design, after calling FspInitEntry it will not return to
> its caller, instead it jumps to a continuation function which is given
> by bootloader with a new stack in system memory. The original stack in
> the CAR is gone, but its content is perserved by FSP and described by
> a bootloader temporary memory HOB. Technically we can recover anything
> we had before in the previous stack, but that is way too complicated.
> To make life much easier, in the FSP continuation routine we just
> simply call fsp_init_done() and jump back to car_init_ret() to redo
> the whole board_init_f() initialization, but this time with a non-zero
> HOB list pointer saved in U-Boot's global data so that we can bypass
> the FspInitEntry for the second time.
> 
> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
> 
> ---
> 
>  arch/x86/cpu/start.S              |  6 +++++-
>  arch/x86/include/asm/u-boot-x86.h |  4 ++++
>  arch/x86/lib/fsp/fsp_car.S        | 26 +++++---------------------
>  arch/x86/lib/fsp/fsp_common.c     |  8 ++++++++
>  common/board_f.c                  |  3 +++
>  5 files changed, 25 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/x86/cpu/start.S b/arch/x86/cpu/start.S
> index 2e5f9da..00e585e 100644
> --- a/arch/x86/cpu/start.S
> +++ b/arch/x86/cpu/start.S
> @@ -116,12 +116,16 @@ car_init_ret:
>  	rep	stosb
>  
>  #ifdef CONFIG_HAVE_FSP
> +	test	%esi, %esi
> +	jz	skip_hob
> +
>  	/* Store HOB list */
>  	movl	%esp, %edx
>  	addl	$GD_HOB_LIST, %edx
>  	movl	%esi, (%edx)
> -#endif
>  
> +skip_hob:
> +#endif
>  	/* Setup first parameter to setup_gdt, pointer to global_data */
>  	movl	%esp, %eax
>  
> diff --git a/arch/x86/include/asm/u-boot-x86.h b/arch/x86/include/asm/u-boot-x86.h
> index faa5182..9ee4104 100644
> --- a/arch/x86/include/asm/u-boot-x86.h
> +++ b/arch/x86/include/asm/u-boot-x86.h
> @@ -54,6 +54,10 @@ u32 isa_map_rom(u32 bus_addr, int size);
>  /* arch/x86/lib/... */
>  int video_bios_init(void);
>  
> +#ifdef CONFIG_HAVE_FSP
> +int x86_fsp_init(void);
> +#endif
> +
>  void	board_init_f_r_trampoline(ulong) __attribute__ ((noreturn));
>  void	board_init_f_r(void) __attribute__ ((noreturn));
>  
> diff --git a/arch/x86/lib/fsp/fsp_car.S b/arch/x86/lib/fsp/fsp_car.S
> index 5e09568..afbf3f9 100644
> --- a/arch/x86/lib/fsp/fsp_car.S
> +++ b/arch/x86/lib/fsp/fsp_car.S
> @@ -56,28 +56,10 @@ temp_ram_init_ret:
>  
>  	/* stack grows down from top of CAR */
>  	movl	%edx, %esp
> +	subl	$4, %esp
>  
> -	/*
> -	 * TODO:
> -	 *
> -	 * According to FSP architecture spec, the fsp_init() will not return
> -	 * to its caller, instead it requires the bootloader to provide a
> -	 * so-called continuation function to pass into the FSP as a parameter
> -	 * of fsp_init, and fsp_init() will call that continuation function
> -	 * directly.
> -	 *
> -	 * The call to fsp_init() may need to be moved out of the car_init()
> -	 * to cpu_init_f() with the help of some inline assembly codes.
> -	 * Note there is another issue that fsp_init() will setup another stack
> -	 * using the fsp_init parameter stack_top after DRAM is initialized,
> -	 * which means any data on the previous stack (on the CAR) gets lost
> -	 * (ie: U-Boot global_data). FSP is supposed to support such scenario,
> -	 * however it does not work. This should be revisited in the future.
> -	 */
> -	movl	$CONFIG_FSP_TEMP_RAM_ADDR, %eax
> -	xorl	%edx, %edx
> -	xorl	%ecx, %ecx
> -	call	fsp_init
> +	xor	%esi, %esi
> +	jmp	car_init_done
>  
>  .global fsp_init_done
>  fsp_init_done:
> @@ -86,6 +68,8 @@ fsp_init_done:
>  	 * Save eax to esi temporarily.
>  	 */
>  	movl	%eax, %esi
> +
> +car_init_done:
>  	/*
>  	 * Re-initialize the ebp (BIST) to zero, as we already reach here
>  	 * which means we passed BIST testing before.
> diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c
> index 001494d..5b25632 100644
> --- a/arch/x86/lib/fsp/fsp_common.c
> +++ b/arch/x86/lib/fsp/fsp_common.c
> @@ -46,3 +46,11 @@ void board_final_cleanup(void)
>  
>  	return;
>  }
> +
> +int x86_fsp_init(void)
> +{
> +	if (!gd->arch.hob_list)
> +		fsp_init(CONFIG_FSP_TEMP_RAM_ADDR, BOOT_FULL_CONFIG, NULL);
> +
> +	return 0;
> +}
> diff --git a/common/board_f.c b/common/board_f.c
> index fbbad1b..21be26f 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -753,6 +753,9 @@ static init_fnc_t init_sequence_f[] = {
>  #ifdef CONFIG_OF_CONTROL
>  	fdtdec_setup,
>  #endif
> +#if defined(CONFIG_X86) && defined(CONFIG_HAVE_FSP)
> +	x86_fsp_init,
> +#endif
>  #ifdef CONFIG_TRACE
>  	trace_early_init,
>  #endif

Overall, I like the idea of these two patches!

In my quick testing on my Valley Island, I now appear to have access to
the gd and device tree blob prior to calling the fsp to setup SDRAM.
This will be a huge help (assuming I'm testing correctly) for migrating
the memory configuration into the dts files. :)

I'll try to spend more time testing this week, sorry, I've been quite
busy so far with other things.

Thanks!
-Andrew