Hi Simon, On Fri, Dec 12, 2014 at 12:54 PM, Simon Glass <s...@chromium.org> wrote: > Hi Bin, > > On 11 December 2014 at 21:49, Bin Meng <bmeng...@gmail.com> wrote: >> Hi Simon, >> >> On Fri, Dec 12, 2014 at 11:53 AM, Simon Glass <s...@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...@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. Regards, Bin _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
