Hi Bin, On 22 January 2015 at 18:32, Bin Meng <[email protected]> wrote: > Hi Simon, > > On Fri, Jan 23, 2015 at 12:36 AM, Simon Glass <[email protected]> wrote: >> Hi Bin, >> >> On 21 January 2015 at 22:39, Bin Meng <[email protected]> wrote: >>> >>> Hi Simon, >>> >>> On Thu, Jan 22, 2015 at 1:02 PM, Simon Glass <[email protected]> wrote: >>> > Hi Bin, >>> > >>> > On 21 January 2015 at 21:45, Bin Meng <[email protected]> wrote: >>> >> Hi Simon, >>> >> >>> >> On Thu, Jan 22, 2015 at 11:42 AM, Simon Glass <[email protected]> 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 _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
