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

[Simon Glass]

Hi Bin,

On Dec 11, 2014 10:08 PM, "Bin Meng" <bmeng.cn at gmail.com> wrote:
>
> Hi Simon,
>
> On Fri, Dec 12, 2014 at 12:54 PM, Simon Glass <sjg at chromium.org> wrote:
> > 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?
>
> Yes, will do in v3.
>
> >>
> >>>>
> >>>>>> +
> >>>>>> +.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.
>
> Yes, FSP is not using registers to pass parameters but stack.
>
> > Anyway,  I think the name should not be 'stack' as it is in ROM and so
> > cannot be written. To me that is confusing.
>
> Maybe a better name of 'xxx_romstack'? I agree this might be confusing
> at first glance. I think I should add a comment block to explain the
> magic behind this, something like the stack is in ROM and not
> writable, so 'call' does not work and only 'jmp' will work with a
> handcrafted rom stack.

Yes sounds good.

Regards,
Simon