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? > >>> >>>>> + >>>>> +.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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
