[U-Boot] [PATCH v2 22/22] x86: Add support for Intel Minnowboard Max

Mon Jun 1 14:41:36 CEST 2015

Hi Simon, Hi Andrew,

On Fri, May 29, 2015 at 9:54 PM, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Andrew, Simon,
>
> On Fri, May 29, 2015 at 2:09 AM, Andrew Bradford
> <andrew at bradfordembedded.com> wrote:
>> On 05/28 10:30, Simon Glass wrote:
>>> Hi Andrew,
>>>
>>> On 27 May 2015 at 08:39, Andrew Bradford <andrew at bradfordembedded.com> wrote:
>>> > On 05/27 13:19, Bin Meng wrote:
>>> >> Hi Simon,
>>> >>
>>> >> On Wed, May 27, 2015 at 5:37 AM, Simon Glass <sjg at chromium.org> wrote:
>>> >> > Hi Andrew,
>>> >> >
>>> >> > On 26 May 2015 at 13:52, Andrew Bradford <andrew at bradfordembedded.com> wrote:
>>> >> >> Hi Simon and Bin (sorry for bringing this back from the dead),
>>> >> >>
>>> >> >> But I have a question about fsp_configs.c down below:
>>> >> >>
>>> >> >> On 01/27 22:13, Simon Glass wrote:
>>> >> >> ------------->8---------------
>
> [snip]
>
>>> >> >>
>>> >> >> I am trying to move this fsp upd to use device tree as I am trying to
>>> >> >> create a patch set to add the Intel Valley Island E38xx board (which
>>> >> >> uses a SODIMM rather than memory down).  In doing so, I've found that
>>> >> >> global data doesn't seem to be available when update_fsp_upd() is called
>>> >> >> and generally it seems that gd->fdt_blob is used to get a reference to
>>> >> >> the flattened device tree.
>>> >> >>
>>> >> >> I'm not super familiar with device tree, but I was attempting to use
>>> >> >> fdtdec_next_compatible(gd->fdt_blob, 0, COMPAT_INTEL_BAYTRAIL_FSP) in a
>>> >> >> similar way that Quark does in my patchset (I've properly created the
>>> >> >> COMPAT_INTEL_BAYTRAIL_FSP define and some device tree nodes in my dts
>>> >> >> file).  When I call fdtdec_next_compatible() the board does something
>>> >> >> which I'm unable to debug (Valley Island does not have the early UART
>>> >> >> pins connected so I have no early UART capability) but things just seem
>>> >> >> to stop.
>>> >> >>
>>> >> >> In manually tracing the calls which lead to update_fsp_upd(), it seems
>>> >> >> that we haven't yet set up global data, so it makes sense that I can't
>>> >> >> reference it.  But the device tree should be available in NOR flash or
>>> >> >> in some other way which we can access in order to get the FSP UPD
>>> >> >> settings.
>>> >> >>
>>> >> >> Is there a simple way to access the device tree while it's still in NOR
>>> >> >> flash so I can avoid using gd?  Or can global data be setup prior to
>>> >> >> calling update_fsp_upd() (I believe we're still in CAR at this point)?
>>> >> >> Or am I misunderstanding something basic here?
>>> >> >>
>>> >> >> Did you have a rough outline of how this could be moved to device tree?
>>> >> >
>>> >> > This is a bit tricky. I would like to move fsp_init() later in the
>>> >> > init sequence (e.g. to board_init_f()). See this TODO in the code:
>>> >> >
>>> >> > /*
>>> >> > * TODO:
>>> >> > *
>>> >> > * 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.
>>> >> > *
>>> >> > * The call to fsp_init() may need to be moved out of the car_init()
>>> >> > * to cpu_init_f() with the help of some inline assembly codes.
>>> >> > * Note there is another issue that fsp_init() will setup another stack
>>> >> > * using the fsp_init parameter stack_top after DRAM is initialized,
>>> >> > * which means any data on the previous stack (on the CAR) gets lost
>>> >> > * (ie: U-Boot global_data). FSP is supposed to support such scenario,
>>> >> > * however it does not work. This should be revisited in the future.
>>> >> > */
>>> >> >
>>> >> > The primary issues are:
>>> >> > 1. The need to recover the global_data
>>> >> > 2. The need to change to a new stack
>>> >> >
>>> >> > Re 1, my reading of the HOB stuff is that it is supposed to provide
>>> >> > you with a pointer to the CAR RAM (all ~128KB of it) so that you can
>>> >> > go back and find your old stack (and in our case, global_data).
>>> >> >
>>> >> > Bin mentioned that this doesn't work - his is the comment above after
>>> >> > I asked him about it.
>>> >> >
>>> >> > But if it could be made to work, then we could delay the init.
>>> >> >
>>> >> > Re 2, U-Boot expects to change to a new stack when it wants to, which
>>> >> > is at the boundary of board_init_f() and board_init_r(). The Intel FSP
>>> >> > should not mandate a stack change over a C function call. IMO that is
>>> >> > just bad design. Dealing with it is not very easy, but one option is
>>> >> > to run with the new stack for the rest of the board_init_f() sequence
>>> >> > and then change stack again at the end of the sequence. Ick.
>>> >> >
>>> >> > To specifically address your problem, global_data is not available
>>> >> > until board_init_f() is called, and the device tree comes into
>>> >> > existence soon after. We could hack around it - e.g. with microcode we
>>> >> > find it in the device tree and stick a pointer to it in a special
>>> >> > place. But the real solution is to figure out how to move this
>>> >> > fsp_init() stuff to later in the sequence. For non-FSP boards we don't
>>> >> > have this problem - e.g. ivybridge does RAM init long after we have
>>> >> > global_data and device tree. Note it is still running from flash at
>>> >> > this point, but CAR is set up and that is where global_data resides.
>>> >> >
>>> >> > I'm interested to hear what you figure out.
>>> >> >
>>> >>
>>> >> I just noticed that Intel has released FSP specification v1.1 [1] in
>>> >> April. After a rough read of the 1.1 spec, it looks to me that Intel
>>> >> changed the fsp_init() design by breaking it down into 3 sub-routines:
>>> >> FspMemoryInit(), TempRamExit() and FspSiliconInit(). I feel this might
>>> >> be more logical to adapt U-Boot, but again I am not sure if the stack
>>> >> migration stuff is still there. So far I don't see any new FSP
>>> >> releases using the 1.1 spec.
>>> >>
>>> >> [1] https://www-ssl.intel.com/content/www/us/en/embedded/software/fsp/fsp-architecture-spec-v1-1.html
>>> >
>>> > There's also a very good overview of how to use an FSP v1.1 firmware at
>>> > [1].  It states that the problem in v1.0 for bootloaders was that when
>>> > you call FspInit() that temporary ram was torn down unconditionally.
>>> > Now, in v1.1, it says after calling FspMemoryInit() that control will be
>>> > given back to the bootloader running in the temporary ram (CAR?).  Then
>>> > the bootloader is responsible for migrating to main memory and to call
>>> > TempRamExit() so that temporary memory can be cleaned up.
>>> >
>>> > This sounds like what u-boot would want and what Simon described above,
>>> > for u-boot to be in charge of relocating from CAR to main memory, right?
>>> > If so, likely things will be much easier once there's a v1.1 FSP for
>>> > baytrail...
>>> >
>>>
>>> Indeed, care to ping them to find out when this might happen?
>>
>> I'm not sure, but I think Intel may have the source on Github [1].  I'm
>> starting to look through the code to see if it's actually useful for
>> E3800 parts or not, but a quick scan through the git log does show
>> changes to support FSP v1.1 and it appears to be actively developed...
>>
>> [1]:https://github.com/tianocore/edk2-IntelFspPkg
>>
>
> Looks like this is not the FSP source codes, but just the support
> codes to get FSP integrated into the EDKII TianCore. I doubt Intel
> will release any source codes of FSP.
>
>> Having a few other, more experienced, eyes on this to see if it's useful
>> would probably be good.
>>
>
> I will see if I can restart this FSP initialization sequence work next week.
>

I've reworked the FSP initialization sequence so that now it is called
by board_init_f(). I tested it on Crown Bay. Would you please try the
following two patches and see if it works on the MinnowMax board?

http://patchwork.ozlabs.org/patch/478943/
http://patchwork.ozlabs.org/patch/478944/

[snip]

Regards,
Bin