Hi Bin, On Dec 11, 2014 10:08 PM, "Bin Meng" <bmeng...@gmail.com> wrote: > > 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.
Yes sounds good. Regards, Simon
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
