[U-Boot] [PATCH v2 16/27] x86: Add basic support to queensbay platform and crownbay board
Bin Meng
bmeng.cn at gmail.com
Fri Dec 12 06:08:35 CET 2014
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.
Regards,
Bin
More information about the U-Boot
mailing list