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

[Bin Meng]

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?

>>
>>>> +
>>>> +.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.

[snip]

Regards,
Bin