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

[Bin Meng]

Hi Andrew,

On Mon, Aug 17, 2015 at 8:51 PM, Andrew Bradford
<andrew at bradfordembedded.com> wrote:
> 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.
>

Thanks for letting us know.

> 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.
>

No problem. I guess I will be busy reviewing the ACPI support as well
as some other work currently on my hand this week. You can still work
on this when you have time (maybe next week?).

> 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>
>

Regards,
Bin