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

[Simon Glass]

Hi Bin,

On 3 June 2015 at 06:59, Andrew Bradford <andrew at bradfordembedded.com> wrote:
> On 06/03 07:17, Bin Meng wrote:
>> Hi Andrew,
>>
>> On Wed, Jun 3, 2015 at 4:05 AM, Andrew Bradford
>> <andrew at bradfordembedded.com> wrote:
>> > 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.
>>
>> Yes, that's expected. Besides the device tree access, are you able to
>> boot the Valley Island successfully with these two patches?
>
> Yes! :)
>
> I'm able to boot and to extract data from the device tree prior to
> calling into the FSP.  So with a patchset like this one that you've
> proposed I could then implement moving the fsp_config vpd data into
> device tree to much more easily support additional baytrail boards.
>
> Tested-by: Andrew Bradford <andrew.bradford at kodakalaris.com>
>
> Thanks!
> -Andrew

This is a big step forward - congrats on getting this running!

I feel that ultimately the FSP init fits better in dram_init(), but by
that time we have emitted serial output and the 'second pass' logic
gets a lot more messy. So let's go with what you have for now.

Acked-by: Simon Glass <sjg at chromium.org>

Regards,
Simon