[U-Boot] [PATCH v2 3/3] x86: fsp: Move FspInitEntry call to board_init_f()
Simon Glass
sjg at chromium.org
Fri Jun 5 18:10:30 CEST 2015
Hi Bin,
On 5 June 2015 at 09:57, Bin Meng <bmeng.cn at gmail.com> wrote:
> 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.
OK, then let's leave it as it is. I have added a CPU uclass and
perhaps we can try to remove some of the arch-specific init that way.
E.g. we could have an fsp_init() method.
Regards,
Simon
More information about the U-Boot
mailing list