[U-Boot] [PATCH v3] x86: baytrail: Configure FSP UPD from device tree

[Andrew Bradford]

Hi Bin,

On 08/07 08:11, Andrew Bradford wrote:
> Hi Bin,
> 
> On 08/07 08:23, Bin Meng wrote:
> > Hi Andrew,
> > 
> > On Fri, Aug 7, 2015 at 4:08 AM, Andrew Bradford
> > <andrew at bradfordembedded.com> wrote:
> > > From: Andrew Bradford <andrew.bradford at kodakalaris.com>
> > >
> > > Allow for configuration of FSP UPD from the device tree which will
> > > override any settings which the FSP was built with itself.
> > >
> > > Modify the MinnowMax and BayleyBay boards to transfer sensible UPD
> > > settings from the Intel FSPv4 Gold release to the respective dts files,
> > > with the condition that the memory-down parameters for MinnowMax are
> > > also used.
> > >
> > 
> > Thanks for updating BayleyBay dts as well :)
> 
> You're welcome :)
> My custom board actually boots with the Bayley Bay configuration, once I
> change out the dts to use your D0 stepping microcode patch, so that was
> a good check for me.
> 
> > > Signed-off-by: Andrew Bradford <andrew.bradford at kodakalaris.com>
> > > ---
> > 
> > Reviewed-by: Bin Meng <bmeng.cn at gmail.com>
> > Tested-by: Bin Meng <bmeng.cn at gmail.com>
> > 
> > But please see some comments/nits below.
> 
> Would it be best if I fix your comments/nits and send a v4?
> 
> > >
> > > Changes from v2:
> > > - Switch to using booleans in dts, where appropriate.
> > > - Add Bayley Bay dts modifications.
> > > - Clarify docs to show bool versus int properties.
> > > - Include enable-igt property from FSPv4.
> > >
> > > Changes from v1:
> > > - Use "-" rather than "_" in dt property names.
> > > - Use "Bay Trail" for the formal name of the Intel product family.
> > > - Use an "fsp," prefix for dt property names for clarity.
> > > - Fix minor code indentation issues.
> > > - Create a dt subnode for the memory-down-params.
> > > - Clarify documentation that dt overrides the FSP's config, so we don't
> > >   use booleans.
> > >
> > >  arch/x86/cpu/baytrail/fsp_configs.c                | 167 +++++++++++++++++----
> > >  arch/x86/dts/bayleybay.dts                         |  40 +++++
> > >  arch/x86/dts/minnowmax.dts                         |  58 +++++++
> > >  .../misc/intel,baytrail-fsp.txt                    | 158 +++++++++++++++++++
> > >  include/fdtdec.h                                   |   2 +
> > >  lib/fdtdec.c                                       |   2 +
> > >  6 files changed, 397 insertions(+), 30 deletions(-)
> > >  create mode 100644 doc/device-tree-bindings/misc/intel,baytrail-fsp.txt
> > >
> > > diff --git a/arch/x86/cpu/baytrail/fsp_configs.c b/arch/x86/cpu/baytrail/fsp_configs.c
> > > index 86b6926..1e5656f 100644
> > > --- a/arch/x86/cpu/baytrail/fsp_configs.c
> > > +++ b/arch/x86/cpu/baytrail/fsp_configs.c
> > > @@ -1,14 +1,18 @@
> > >  /*
> > >   * Copyright (C) 2013, Intel Corporation
> > >   * Copyright (C) 2014, Bin Meng <bmeng.cn at gmail.com>
> > > + * Copyright (C) 2015, Kodak Alaris, Inc
> > >   *
> > >   * SPDX-License-Identifier:    Intel
> > >   */
> > >
> > >  #include <common.h>
> > > +#include <fdtdec.h>
> > >  #include <asm/arch/fsp/azalia.h>
> > >  #include <asm/fsp/fsp_support.h>
> > >
> > > +DECLARE_GLOBAL_DATA_PTR;
> > > +
> > >  /* ALC262 Verb Table - 10EC0262 */
> > >  static const uint32_t verb_table_data13[] = {
> > >         /* Pin Complex (NID 0x11) */
> > > @@ -116,41 +120,144 @@ const struct pch_azalia_config azalia_config = {
> > >         .reset_wait_timer_us = 300
> > >  };
> > >
> > > +/**
> > > + * Override the FSP's UPD.
> > > + * If the device tree does not specify a integer setting, use the default
> > 
> > Nits: an integer
> > 
> > > + * provided in Intel's Baytrail_FSP_Gold4.tgz release FSP/BayleyBayFsp.bsf file.
> > > + */
> > >  void update_fsp_upd(struct upd_region *fsp_upd)
> > >  {
> > >         struct memory_down_data *mem;
> > > +       const void *blob = gd->fdt_blob;
> > > +       int node;
> > >
> > > -       /*
> > > -        * 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;
> > 
> > One general comment: I think we should replace these hardcoded numbers
> > with meaningful macros (eg: 0x800 means MMIO uses 2GB space, that's
> > why we see previous the pci does not work on a 4GB DIMM as FSP only
> > maps 2GB RAM per this setting). See Intel's Baytrail_FSP_Gold4.tgz
> > release FSP/BayleyBayFsp.bsf file, it has the details of how each
> > magic number maps to what meaning. Will you do a follow-up patch for
> > this? I can do that as well.
> 
> I can do this.

I'm sorry, but I have been busy with other tasks for the past week and
haven't gotten a chance to craft or send out a patch to turn the magic
numbers into #defines in a header.

If you are able to send a patch which #defines the magic numbers
quickly, I would greatly appreciate it, as I likely won't be able to get
a patch out until (at best) the end of this week.  I don't want to hold
anyone up with their development waiting for me to send such a patch.

My Bay Trail project has been put on-hold by my employer so I'm not able
to dedicate much time to it now.  Sorry.

<snip>

Thanks,
Andrew