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

12 Dec 2014

      Hi Bin,
On Dec 11, 2014 10:08 PM, "Bin Meng" bmeng.cn@gmail.com wrote:
...
Hi Simon,
On Fri, Dec 12, 2014 at 12:54 PM, Simon Glass sjg@chromium.org wrote:
...
Hi Bin,
On 11 December 2014 at 21:49, Bin Meng bmeng.cn@gmail.com wrote:
...
Hi Simon,
On Fri, Dec 12, 2014 at 11:53 AM, Simon Glass sjg@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@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