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

[Simon Glass]

Hi Andrew,

On 27 May 2015 at 08:22, Andrew Bradford <andrew at bradfordembedded.com> wrote:
> Hi Simon & Bin,
>
> On 05/26 15:37, Simon Glass 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---------------
>> >> +void update_fsp_upd(struct upd_region *fsp_upd)
>> >> +{
>> >> +     struct memory_down_data *mem;
>> >> +
>> >> +     /*
>> >> +      * Configure everything here to avoid the poor hard-pressed user
>> >> +      * needing to run Intel's binary configuration tool. It may also allow
>> >> +      * us to support the 1GB single core variant easily.
>> >> +      *
>> >> +      * TODO(sjg at chromium.org): Move to device tree
>> >> +      */
>> >> +     fsp_upd->mrc_init_tseg_size = 8;
>> >> +     fsp_upd->mrc_init_mmio_size = 0x800;
>> >> +     fsp_upd->emmc_boot_mode = 0xff;
>> >> +     fsp_upd->enable_sdio = 1;
>> >> +     fsp_upd->enable_sdcard = 1;
>> >> +     fsp_upd->enable_hsuart0 = 1;
>> >> +     fsp_upd->azalia_config_ptr = (uint32_t)&azalia_config;
>> >> +     fsp_upd->enable_i2_c0 = 0;
>> >> +     fsp_upd->enable_i2_c2 = 0;
>> >> +     fsp_upd->enable_i2_c3 = 0;
>> >> +     fsp_upd->enable_i2_c4 = 0;
>> >> +     fsp_upd->enable_xhci = 0;
>> >> +     fsp_upd->igd_render_standby = 1;
>> >> +
>> >> +     mem = &fsp_upd->memory_params;
>> >> +     mem->enable_memory_down = 1;
>> >> +     mem->dram_speed = 1;
>> >> +     mem->dimm_width = 1;
>> >> +     mem->dimm_density = 2;
>> >> +     mem->dimm_tcl = 0xb;
>> >> +     mem->dimm_trpt_rcd = 0xb;
>> >> +     mem->dimm_twr = 0xc;
>> >> +     mem->dimm_twtr = 6;
>> >> +     mem->dimm_trrd = 6;
>> >> +     mem->dimm_trtp = 6;
>> >> +     mem->dimm_tfaw = 0x14;
>> >> +}
>> >
>> > 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.
>
> OK, if it doesn't work then that's frustrating.
>
> I do see that HOB 15 on my Valley Island board has the right GUID to be
> the FSP_BOOTLOADER_TEMPORARY_MEMORY_HOB and its size is 16408.  So that
> has me hopeful, but likely I'll run into similar things that you two
> have seen before.

Well if you can get access to the old global_data here then you can
memcpy it to a new place in DRAM. That helps. But I forgot about issue
3:

3. Driver model will have created a few devices in temporary RAM and
now they are gone.

The easiest solution is to run the board_init_f() sequence again from
the start, this time skipping the dram init. It's horrible but pretty
easy to implement. So you end up running dram_init() twice. The first
time it runs the FSP call and then calls board_init_f() (or returns a
special value such that the sequence restarts). The second time it
does nothing.

Better might be to redo just the early malloc and driver model init:
initf_malloc() and initf_dm(). That might work.

>
> Any pointers on why it was determined that the CAR RAM pointer doesn't
> work correctly so I can avoid spending too much time investigating
> things which have already been done?
>
>> 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.
>
> Assuming that we can recover the CAR RAM data, would it make sense to
> setup global data while in CAR so we can get access to the device tree
> and then call fsp_init(), and once we're running from main memory just
> set our global data pointer to the insides of the CAR RAM?

CAR becomes unavailable when U-Boot relocates, since we want to use
the cache for actual caching rather than for RAM. Currently this
happens in init_cache_f_r() for non-FSP platforms, and it would be
great if Intel could fix it so that this happens for FSP platforms too
(as the link to the new FSP spec suggests).

>
> Likely I'm missing a few steps, here.  But this feels a lot like how SPL
> works, except that u-boot is not in charge of how the relocation
> happens, we're just along for the ride.

Exactly.

Note that the device tree is actually available very early - see the
fdtdec_setup() call which is one of the first things in the init
sequence. The problem is just how to move the FSP init into that
sequence, so that global_data is available.

Regards,
Simon