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

[Andrew Bradford]

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.

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?

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.

Thanks!
-Andrew