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

Bin Meng bmeng.cn at gmail.com
Fri Jun 5 17:57:54 CEST 2015


Hi Simon,

On Fri, Jun 5, 2015 at 11:34 PM, Simon Glass <sjg at chromium.org> wrote:
> On 4 June 2015 at 04:28, Bin Meng <bmeng.cn at gmail.com> 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>
>> Acked-by: Simon Glass <sjg at chromium.org>
>> Tested-by: Andrew Bradford <andrew.bradford at kodakalaris.com>
>>
>
> Tested on Minnowboard MAX:
> Tested-by: Simon Glass <sjg at chromium.org>
>
>> ---
>>
>> Changes in v2: None
>>
>>  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 67c0098..5d8a96d 100644
>> --- a/arch/x86/include/asm/u-boot-x86.h
>> +++ b/arch/x86/include/asm/u-boot-x86.h
>> @@ -52,6 +52,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
>
> Can we drop the #ifdef here?

Yes, in v3.

>> +
>>  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
>
> Would it be possible to put this into arch_cpu_init()? Or do you think
> it is better to have a separate call?
>

I was thinking to do that in arch_cpu_init previously, but I changed
my idea mainly for:
- I need modify every cpu codes which uses FSP. So far there are two,
not a big deal :)
- Since x86_fsp_init() will jump back to call board_init_f() again, I
feel putting it as early as possible right after fdtdec_setup() could
save some boot time, like no need to execute trace_early_init() and
initf_malloc() which are meaningless, while still could gain access to
device tree.

>>  #ifdef CONFIG_TRACE
>>         trace_early_init,
>>  #endif
>> --

Regards,
Bin


More information about the U-Boot mailing list