[U-Boot] x86 FSP - delayed SDRAM init?

[Simon Glass]

Hi Bin,

On 22 January 2015 at 18:32, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Fri, Jan 23, 2015 at 12:36 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Bin,
>>
>> On 21 January 2015 at 22:39, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>
>>> Hi Simon,
>>>
>>> On Thu, Jan 22, 2015 at 1:02 PM, Simon Glass <sjg at chromium.org> wrote:
>>> > Hi Bin,
>>> >
>>> > On 21 January 2015 at 21:45, Bin Meng <bmeng.cn at gmail.com> wrote:
>>> >> Hi Simon,
>>> >>
>>> >> On Thu, Jan 22, 2015 at 11:42 AM, Simon Glass <sjg at chromium.org> wrote:
>>> >>> Hi Bin,
>>> >>>
>>> >>> In the Baytrail FSP docs I see a note about the HOB passing back the
>>> >>> 'Boot Loader Temporary Memory Data HOB'. This seems to be a copy of
>>> >>> the entire temporary memory space. I wonder if we could recover struct
>>> >>> global_data from this?
>>> >>>
>>> >>
>>> >> Yes, I think so. And I have verified this temporary memory space
>>> >> indeed contains stack contents before fsp_init() from U-Boot shell on
>>> >> Crown Bay. But the overall process might be complicated. See below.
>>> >>
>>> >>> If so, then we could move the fsp_init stuff to dram_init(), perhaps?
>>> >>>
>>> >>> But perhaps this is a feature of only this FSP?
>>> >>>
>>> >>
>>> >> I believe this is a feature defined by the FSP architecture spec, so
>>> >> every FSP should support that.
>>> >>
>>> >> Technically it should be no problem to call fsp_init() from
>>> >> arch_cpu_init() or even later dram_init(). However, I would say doing
>>> >> so brings us more harm than good. The following points are what I
>>> >> thought about before:
>>> >>
>>> >> 1). fsp_init() takes one parameter 'stack_top' to setup another stack
>>> >> after DRAM is initialized. This means everything on the previous CAR
>>> >> stack will need to migrate to the new stack below 'stack_top'. This
>>> >> includes global data, early malloc pointers, arch_cpu_init() stack
>>> >> variables and its return address.
>>> >> 2). Copy previous global_data to the new places under stack_top, and
>>> >> fix up gd->arch.gd_addr.
>>> >> 3). The initf_malloc() is called before arch_cpu_init() so we need fix
>>> >> up the early malloc pointers manually (gd->malloc_base and
>>> >> gd->malloc_limit)
>>> >> 4). Fix up the stack variables and return address of arch_cpu_init()
>>> >> on the new stack.
>>> >> 5). On Tunnel Creek, if we call setup_gdt() in start.S, later
>>> >> fsp_init() in arch_cpu_init() will fail to bring up the thread 1
>>> >> (Tunnel Creek supports SMT), the reason of which is unknown to me yet
>>> >> (FSP is a black box). It looks to me that FSP is assuming GDT only
>>> >> contains two entries (32-bit 4G flat address) before calling
>>> >> fsp_init().
>>> >>
>>> >> I have not looked into this any further, so the above points might not
>>> >> be 100% right. I would say with these modifications, the codes are
>>> >> more difficult to understand.
>>> >
>>> > I don't disagree with any of this, but I've had a bit more of a look.
>>> >
>>> > How about something awful like this:
>>> >
>>> > - start.S
>>> > - do the initram thing
>>> > - call board_init_f()
>>> > - there is no hob pointer, so CPU init does very little
>>> > - it runs only to dram_init()
>>> > - dram_init() calls the fsp_init() and ends up back at another function
>>> > - that function sets up global_data again, calls board_init_f() again
>>> >    - passes a boot flag to suppress the banner the second time
>>> > - this time there is a hob pointer, so CPU init can complete
>>> > - things boot normally
>>> >
>>> > It is hideous. The question is which way is worse.
>>>
>>> So this way we avoid migrating the stack.
>>>
>>> > What we really need is to split fsp_init() into two parts:
>>> >
>>> > 1. Set up DRAM
>>> > 2. Turn off CAR (called when WE decide we no-longer need it)
>>> >
>>> > Then we could make this work as with the non-FSP mode.
>>> >
>>> > By joining the two, they have made it even harder than it should be.
>>> > Of course, no one wins with binary blobs, but the 'continuation
>>> > function' should have been a red flag when this design was done at
>>> > Intel.
>>>
>>> I agree. I wish the design done at Intel should have considered more
>>> use cases about the FSP integration into existing bootloaders. Even in
>>> coreboot, the FSP integration is not perfectly fit into the coreboot
>>> model.
>>>
>>> > BTW for me on minnowmax fsp_init() hangs. Since it is a binary blob I
>>> > am not sure how to debug it. There is no error printed / returned. I'm
>>> > not even sure how to make it output debug info. No post codes are
>>> > available. Sigh.
>>>
>>> I see coreboot has the minnow max board support already with Intel
>>> FSP. You can generate a coreboot for minnow max and program it onto
>>> the board to see how things go.
>>>
>>> Does this link help?
>>> http://review.coreboot.org/gitweb?p=coreboot.git;a=commitdiff;h=e6df041b8bf8e37debc0d6a871080b64eea7a372.
>>> It looks that the BayTrail FSP has several parameters configurable for
>>> DRAM.
>>
>> Thanks - yes I saw that code although didn't look at the particular
>> commit. It's just painful trying n random things to try to make a
>> black box behave. I must be getting less patient these days.
>>
>>>
>>> Here are the required changes for modifying the FSP manually:
>>> Enable Memory Down: Enabled
>>> DRAM Speed: 1066 MHz
>>> DIMM_DWidth: x16
>>> DIMM_Density: 4 Gbit (2GB Minnow Max) / 2 Gbit (1GB Minnow Max)
>>> tCL: 7
>>> tRP_tRCD: 7
>>> tWR: 8
>>> tRRD: 6
>>> tRTP: 4
>>> tFAW: 27
>>> Other FSP values can remain the same.
>>
>> Apart from the 27 that matches. I suppose that's another problem with
>> the FSP design. If memory init fails it should return with a useful
>> error, not just die because the API requires that it returns with a
>> new stack in DRAM. Also, specifying the address of the stack before
>> you know the memory layout is odd.
>>
>
> Agreed. I am not sure whether Intel wants to improve their design or
> not. The new stack after fsp_init() is indeed a nightmare which causes
> stack migration issues we are discussing here. Sigh ..

I've sent some feedback to the Minnowboard list.

Regards,
Simon