[U-Boot] [PATCH v2 16/27] x86: Add basic support to queensbay platform and crownbay board

[Simon Glass]

Hi Bin,

On 11 December 2014 at 21:49, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Fri, Dec 12, 2014 at 11:53 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Bin,
>>
>
> [snip]
>
>>>>> diff --git a/arch/x86/cpu/queensbay/tnc_car.S b/arch/x86/cpu/queensbay/tnc_car.S
>>>>> new file mode 100644
>>>>> index 0000000..4f39e42
>>>>> --- /dev/null
>>>>> +++ b/arch/x86/cpu/queensbay/tnc_car.S
>>>>> @@ -0,0 +1,103 @@
>>>>> +/*
>>>>> + * Copyright (C) 2014, Bin Meng <bmeng.cn at gmail.com>
>>>>> + *
>>>>> + * SPDX-License-Identifier:    GPL-2.0+
>>>>> + */
>>>>> +
>>>>> +#include <config.h>
>>>>> +#include <asm/post.h>
>>>>> +
>>>>> +.globl car_init
>>>>> +car_init:
>>>>> +       /*
>>>>> +        * Note: ebp holds the BIST value (built-in self test) so far, but ebp
>>>>> +        * will be destroyed through the FSP call, thus we have to test the
>>>>> +        * BIST value here before we call into FSP.
>>>>> +        */
>>>>> +       test    %ebp, %ebp
>>>>> +       jz      car_init_start
>>>>> +       post_code(POST_BIST_FAILURE)
>>>>> +       jmp     die
>>>>> +
>>>>> +car_init_start:
>>>>> +       post_code(POST_CAR_START)
>>>>> +       lea     find_fsp_header_stack, %esp
>>>>> +       jmp     find_fsp_header
>>>>> +
>>>>> +find_fsp_header_ret:
>>>>> +       /* EAX points to FSP_INFO_HEADER */
>>>>> +       mov     %eax, %ebp
>>>>> +
>>>>> +       /* sanity test */
>>>>> +       cmp     $CONFIG_FSP_LOCATION, %eax
>>>>> +       jb      die
>>>>> +
>>>>> +       /* calculate TempRamInitEntry address */
>>>>> +       mov     0x30(%ebp), %eax
>>>>> +       add     0x1c(%ebp), %eax
>>>>> +
>>>>> +       /* call FSP TempRamInitEntry to setup temporary stack */
>>>>> +       lea     temp_ram_init_stack, %esp
>>>>> +       jmp     *%eax
>>>>> +
>>>>> +temp_ram_init_ret:
>>>>> +       addl    $4, %esp
>>>>> +       cmp     $0, %eax
>>>>> +       jz      continue
>>>>
>>>> Can we make this jnz put the error code below instead of it being in
>>>> the normal path flow?
>>>
>>> Sure.
>>>
>>>>> +       post_code(POST_CAR_FAILURE)
>>>>> +
>>>>> +die:
>>>>> +       hlt
>>>>> +       jmp     die
>>>>> +       hlt
>>>>> +
>>>>> +continue:
>>>>> +       post_code(POST_CAR_CPU_CACHE)
>>>>> +
>>>>> +       /*
>>>>> +        * The FSP TempRamInit initializes the ecx and edx registers to
>>>>> +        * point to a temporary but writable memory range (Cache-As-RAM).
>>>>> +        * ecx: the start of this temporary memory range,
>>>>> +        * edx: the end of this range.
>>>>> +        */
>>>>> +
>>>>> +       /* stack grows down from top of CAR */
>>>>> +       movl    %edx, %esp
>>>>> +
>>>>> +       movl    $CONFIG_FSP_TEMP_RAM_ADDR, %eax
>>>>> +       xorl    %edx, %edx
>>>>> +       xorl    %ecx, %ecx
>>>>> +       call    fsp_init
>>>>
>>>> Do we have to call fsp_init so early? Could we not call it from
>>>> cpu_init_f() or even later?
>>>
>>> Unfortunately yes. 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.
>>
>> But couldn't it be called from C with an assembler shim?
>>
>> My objective is to make the assembler code as short as possible and
>> call board_init_f() as soon as possible. So can we no call this later
>> in the init sequence?
>
> It may work from cpu_init_f() with an some inline assembly, but I am
> not sure and need do some experiments. Besides this, there is another
> issue that fsp_init() will setup another stack after the call using
> the fsp_init parameter stack_top, which means any data on the previous
> stack (on the CAR) gets lost (ie: global_data). I mentioned in one of
> the discussion thread before, that supposed FSP should support such
> scenario, however it does not work. I planned to look into this in the
> future. Perhaps we need leave this issue for now for this series. I
> will investigate it later. How do you think?

OK. Can you please add a TODO in the code and mention it in the commit
message also?

>
>>>
>>>>> +
>>>>> +.global fsp_init_done
>>>>> +fsp_init_done:
>>>>> +       /*
>>>>> +        * We come here from FspInit with eax pointing to the HOB list.
>>>>> +        * Save eax to esi temporarily.
>>>>> +        */
>>>>> +       movl    %eax, %esi
>>>>> +       /*
>>>>> +        * Re-initialize the ebp (BIST) to zero, as we already reach here
>>>>> +        * which means we passed BIST testing before.
>>>>> +        */
>>>>> +       xorl    %ebp, %ebp
>>>>> +       jmp     car_init_ret
>>>>> +
>>>>> +       .balign 4
>>>>> +find_fsp_header_stack:
>>>>> +       .long   find_fsp_header_ret
>>>>> +
>>>>> +       .balign 4
>>>>> +temp_ram_init_stack:
>>>>
>>>> Can we use temp_ram_init_vars or similar? This name makes it look like
>>>> it is a temporary stack.
>>>
>>> Yes, it is indeed a temporary ROM stack.
>>
>> It's not writeable though, right? So in what sense it is a stack?
>
> It is the ROM stack of the fsp_tempram_init call. See the codes below:
>
>         /* calculate TempRamInitEntry address */
>         mov     0x30(%ebp), %eax
>         add     0x1c(%ebp), %eax
>
>         /* call FSP TempRamInitEntry to setup temporary stack */
>         lea     temp_ram_init_stack, %esp
>         jmp     *%eax
>
> It needs to have the return address and fsp_tempram_init parameters on
> the stack.

OK, so really it is just parameters to the function, but in a strange
way. One wonders why they didn't just use registers.

Anyway,  I think the name should not be 'stack' as it is in ROM and so
cannot be written. To me that is confusing.

Regards,
Simon