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

Simon Glass sjg at chromium.org
Wed Jul 29 16:23:51 CEST 2015


Hi Bin,

On 29 July 2015 at 08:08, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Andrew, Simon,
>
> On Wed, Jul 22, 2015 at 4:17 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Andrew,
>>
>> On 10 July 2015 at 12:24, Andrew Bradford <andrew at bradfordembedded.com> wrote:
>>> Hi Simon,
>>>
>>> On 07/10 06:53, Simon Glass wrote:
>>>> Hi,
>>>>
>>>> On 8 July 2015 at 05:30, Andrew Bradford <andrew at bradfordembedded.com> wrote:
>>>> > Hi Bin,
>>>> >
>>>> > On 07/08 11:18, Bin Meng wrote:
>>>> >> Hi Andrew,
>>>> >>
>>>> >> On Wed, Jul 8, 2015 at 3:16 AM,  <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 if the device
>>>> >> > tree settings exist, otherwise simply trust the FSP's defaults.
>>>> >> >
>>>> >> > Modifies the MinnowMax board to transfer the FSP UPD hard-coded settings
>>>> >> > to the MinnowMax dts.
>>>> >> >
>>>> >> > Signed-off-by: Andrew Bradford <andrew.bradford at kodakalaris.com>
>>>> >> > ---
>>>> >> >
>>>> >> > 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                | 188 +++++++++++++++++----
>>>> >> >  arch/x86/dts/minnowmax.dts                         |  30 ++++
>>>> >> >  .../misc/intel,baytrail-fsp.txt                    | 119 +++++++++++++
>>>> >> >  include/fdtdec.h                                   |   2 +
>>>> >> >  lib/fdtdec.c                                       |   2 +
>>>> >> >  5 files changed, 311 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..fce76e6 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,165 @@ const struct pch_azalia_config azalia_config = {
>>>> >> >         .reset_wait_timer_us = 300
>>>> >> >  };
>>>> >> >
>>>> >> > +/**
>>>> >> > + * Update the FSP's UPD.  The FSP itself can be configured for defaults to
>>>> >> > + * store in UPD through Intel's GUI configurator but likely a specific board
>>>> >> > + * will want to update these from u-boot, so allow for that via device tree.
>>>> >> > + * If the device tree does not specify a setting, trust the FSP's default.
>>>> >> > + */
>>>> >> >  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;
>>>> >> > -       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;
>>>> >> > +
>>>> >> > +       node = fdtdec_next_compatible(blob, 0, COMPAT_INTEL_BAYTRAIL_FSP);
>>>> >> > +       if (node < 0) {
>>>> >> > +               debug("%s: Cannot find FSP node\n", __func__);
>>>> >> > +               return;
>>>> >> > +       }
>>>> >> > +
>>>> >> > +       fsp_upd->mrc_init_tseg_size = fdtdec_get_int(blob, node,
>>>> >> > +                                                    "fsp,mrc-int-tseg-size",
>>>> >>
>>>> >> mrc-int? Guess it is mrc-init.
>>>> >
>>>> > Yes, thank you for catching this.  Will fix in v3.
>>>> >
>>>> >> > +                                                    fsp_upd->mrc_init_tseg_size);
>>>> >> > +       fsp_upd->mrc_init_mmio_size = fdtdec_get_int(blob, node,
>>>> >> > +                                                    "fsp,mrc-init-mmio-size",
>>>> >> > +                                                    fsp_upd->mrc_init_mmio_size);
>>>> >> > +       fsp_upd->mrc_init_spd_addr1 = fdtdec_get_int(blob, node,
>>>> >> > +                                                    "fsp,mrc-init-spd-addr1",
>>>> >> > +                                                    fsp_upd->mrc_init_spd_addr1);
>>>> >> > +       fsp_upd->mrc_init_spd_addr2 = fdtdec_get_int(blob, node,
>>>> >> > +                                                    "fsp,mrc-init-spd-addr2",
>>>> >> > +                                                    fsp_upd->mrc_init_spd_addr2);
>>>> >> > +       fsp_upd->emmc_boot_mode = fdtdec_get_int(blob, node, "fsp,emmc-boot-mode",
>>>> >> > +                                                fsp_upd->emmc_boot_mode);
>>>> >> > +       fsp_upd->enable_sdio = fdtdec_get_int(blob, node, "fsp,enable-sdio",
>>>> >> > +                                             fsp_upd->enable_sdio);
>>>> >>
>>>> >> I think we agreed that all these 'enable' should be accessed using
>>>> >> fdtdec_get_bool().
>>>> >
>>>> > I was under the impression that we decided to keep these as non-bool so
>>>> > that the FSP could be overridden as needed (or you can fully specify all
>>>> > the properties in dt if you want) as per Simon's comments [1] along with
>>>> > adding additional comments in this version of my patch to clarify this..
>>>> >
>>>> > [1]:http://lists.denx.de/pipermail/u-boot/2015-June/217802.html
>>>> >
>>>> > But I'm open to changing this to use bools.  Additional thoughts on this
>>>> > below...
>>>> >
>>>> >> > +       fsp_upd->enable_sdcard = fdtdec_get_int(blob, node, "fsp,enable-sdcard",
>>>> >> > +                                               fsp_upd->enable_sdcard);
>>>> >> > +       fsp_upd->enable_hsuart0 = fdtdec_get_int(blob, node, "fsp,enable-hsuart0",
>>>> >> > +                                                fsp_upd->enable_hsuart0);
>>>> >> > +       fsp_upd->enable_hsuart1 = fdtdec_get_int(blob, node, "fsp,enable-hsuart1",
>>>> >> > +                                                fsp_upd->enable_hsuart1);
>>>> >> > +       fsp_upd->enable_spi = fdtdec_get_int(blob, node, "fsp,enable-spi",
>>>> >> > +                                            fsp_upd->enable_spi);
>>>> >> > +       fsp_upd->enable_sata = fdtdec_get_int(blob, node, "fsp,enable-sata",
>>>> >> > +                                             fsp_upd->enable_sata);
>>>> >> > +       fsp_upd->enable_azalia = fdtdec_get_int(blob, node, "fsp,enable-azalia",
>>>> >> > +                                               fsp_upd->enable_azalia);
>>>> >> > +       fsp_upd->enable_xhci = fdtdec_get_int(blob, node, "fsp,enable-xhci",
>>>> >> > +                                             fsp_upd->enable_xhci);
>>>> >> > +       fsp_upd->enable_lpe = fdtdec_get_int(blob, node, "fsp,enable-lpe",
>>>> >> > +                                            fsp_upd->enable_lpe);
>>>> >> > +       fsp_upd->lpss_sio_enable_pci_mode = fdtdec_get_int(blob, node,
>>>> >> > +                                                          "fsp,lpss-sio-enable-pci-mode",
>>>> >> > +                                                          fsp_upd->lpss_sio_enable_pci_mode);
>>>> >> > +       fsp_upd->enable_dma0 = fdtdec_get_int(blob, node, "fsp,enable-dma0",
>>>> >> > +                                             fsp_upd->enable_dma0);
>>>> >> > +       fsp_upd->enable_dma1 = fdtdec_get_int(blob, node, "fsp,enable-dma1",
>>>> >> > +                                             fsp_upd->enable_dma1);
>>>> >> > +       fsp_upd->enable_i2_c0 = fdtdec_get_int(blob, node, "fsp,enable-i2-c0",
>>>> >> > +                                              fsp_upd->enable_i2_c0);
>>>> >> > +       fsp_upd->enable_i2_c1 = fdtdec_get_int(blob, node, "fsp,enable-i2-c1",
>>>> >> > +                                              fsp_upd->enable_i2_c1);
>>>> >> > +       fsp_upd->enable_i2_c2 = fdtdec_get_int(blob, node, "fsp,enable-i2-c2",
>>>> >> > +                                              fsp_upd->enable_i2_c2);
>>>> >> > +       fsp_upd->enable_i2_c3 = fdtdec_get_int(blob, node, "fsp,enable-i2-c3",
>>>> >> > +                                              fsp_upd->enable_i2_c3);
>>>> >> > +       fsp_upd->enable_i2_c4 = fdtdec_get_int(blob, node, "fsp,enable-i2-c4",
>>>> >> > +                                              fsp_upd->enable_i2_c4);
>>>> >> > +       fsp_upd->enable_i2_c5 = fdtdec_get_int(blob, node, "fsp,enable-i2-c5",
>>>> >> > +                                              fsp_upd->enable_i2_c5);
>>>> >> > +       fsp_upd->enable_i2_c6 = fdtdec_get_int(blob, node, "fsp,enable-i2-c6",
>>>> >> > +                                              fsp_upd->enable_i2_c6);
>>>> >> > +       fsp_upd->enable_pwm0 = fdtdec_get_int(blob, node, "fsp,enable-pwm0",
>>>> >> > +                                             fsp_upd->enable_pwm0);
>>>> >> > +       fsp_upd->enable_pwm1 = fdtdec_get_int(blob, node, "fsp,enable-pwm1",
>>>> >> > +                                             fsp_upd->enable_pwm1);
>>>> >> > +       fsp_upd->enable_hsi = fdtdec_get_int(blob, node, "fsp,enable-hsi",
>>>> >> > +                                            fsp_upd->enable_hsi);
>>>> >> > +       fsp_upd->igd_dvmt50_pre_alloc = fdtdec_get_int(blob, node,
>>>> >> > +                                                      "fsp,igd-dvmt50-pre-alloc",
>>>> >> > +                                                      fsp_upd->igd_dvmt50_pre_alloc);
>>>> >> > +       fsp_upd->aperture_size = fdtdec_get_int(blob, node, "fsp,aperture-size",
>>>> >> > +                                               fsp_upd->aperture_size);
>>>> >> > +       fsp_upd->gtt_size = fdtdec_get_int(blob, node, "fsp,gtt-size",
>>>> >> > +                                          fsp_upd->gtt_size);
>>>> >> > +       fsp_upd->serial_debug_port_address = fdtdec_get_int(blob, node,
>>>> >> > +                                                           "fsp,serial-debug-port-address",
>>>> >> > +                                                           fsp_upd->serial_debug_port_address);
>>>> >> > +       fsp_upd->serial_debug_port_type = fdtdec_get_int(blob, node,
>>>> >> > +                                                        "fsp,serial-debug-port-type",
>>>> >> > +                                                        fsp_upd->serial_debug_port_type);
>>>> >> > +       fsp_upd->mrc_debug_msg = fdtdec_get_int(blob, node, "fsp,mrc-debug-msg",
>>>> >> > +                                               fsp_upd->mrc_debug_msg);
>>>> >> > +       fsp_upd->isp_enable = fdtdec_get_int(blob, node, "fsp,isp-enable",
>>>> >> > +                                            fsp_upd->isp_enable);
>>>> >> > +       fsp_upd->scc_enable_pci_mode = fdtdec_get_int(blob, node,
>>>> >> > +                                                     "fsp,scc-enable-pci-mode",
>>>> >> > +                                                     fsp_upd->scc_enable_pci_mode);
>>>> >> > +       fsp_upd->igd_render_standby = fdtdec_get_int(blob, node,
>>>> >> > +                                                    "fsp,igd-render-standby",
>>>> >> > +                                                    fsp_upd->igd_render_standby);
>>>> >> > +       fsp_upd->txe_uma_enable = fdtdec_get_int(blob, node, "fsp,txe-uma-enable",
>>>> >> > +                                                fsp_upd->txe_uma_enable);
>>>> >> > +       fsp_upd->os_selection = fdtdec_get_int(blob, node, "fsp,os-selection",
>>>> >> > +                                              fsp_upd->os_selection);
>>>> >> > +       fsp_upd->emmc45_ddr50_enabled = fdtdec_get_int(blob, node,
>>>> >> > +                                                      "fsp,emmc45-ddr50-enabled",
>>>> >> > +                                                      fsp_upd->emmc45_ddr50_enabled);
>>>> >> > +       fsp_upd->emmc45_hs200_enabled = fdtdec_get_int(blob, node,
>>>> >> > +                                                      "fsp,emmc45-hs200-enabled",
>>>> >> > +                                                      fsp_upd->emmc45_hs200_enabled);
>>>> >> > +       fsp_upd->emmc45_retune_timer_value = fdtdec_get_int(blob, node,
>>>> >> > +                                                           "fsp,emmc45-retune-timer-value",
>>>> >> > +                                                           fsp_upd->emmc45_retune_timer_value);
>>>> >> > +       fsp_upd->igd_render_standby = fdtdec_get_int(blob, node,
>>>> >> > +                                                    "fsp,igd-render-standby",
>>>> >> > +                                                    fsp_upd->igd_render_standby);
>>>> >> >
>>>> >> >         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;
>>>> >> > +       mem->enable_memory_down = fdtdec_get_int(blob, node,
>>>> >> > +                                                "fsp,enable-memory-down",
>>>> >> > +                                                mem->enable_memory_down);
>>>> >> > +       node = fdtdec_next_compatible(blob, node,
>>>> >> > +                                     COMPAT_INTEL_BAYTRAIL_FSP_MDP);
>>>> >> > +       if (node < 0) {
>>>> >> > +               debug("%s: Cannot find FSP memory-down-params node\n", __func__);
>>>> >> > +       } else {
>>>> >> > +               mem->dram_speed = fdtdec_get_int(blob, node, "fsp,dram-speed",
>>>> >> > +                                                mem->dram_speed);
>>>> >> > +               mem->dram_type = fdtdec_get_int(blob, node, "fsp,dram-type",
>>>> >> > +                                               mem->dram_type);
>>>> >> > +               mem->dimm_0_enable = fdtdec_get_int(blob, node, "fsp,dimm-0-enable",
>>>> >> > +                                                   mem->dimm_0_enable);
>>>> >> > +               mem->dimm_1_enable = fdtdec_get_int(blob, node, "fsp,dimm-1-enable",
>>>> >> > +                                                   mem->dimm_1_enable);
>>>> >> > +               mem->dimm_width = fdtdec_get_int(blob, node, "fsp,dimm-width",
>>>> >> > +                                                mem->dimm_width);
>>>> >> > +               mem->dimm_density = fdtdec_get_int(blob, node,
>>>> >> > +                                                  "fsp,dimm-density",
>>>> >> > +                                                  mem->dimm_density);
>>>> >> > +               mem->dimm_bus_width = fdtdec_get_int(blob, node,
>>>> >> > +                                                    "fsp,dimm-bus-width",
>>>> >> > +                                                    mem->dimm_bus_width);
>>>> >> > +               mem->dimm_sides = fdtdec_get_int(blob, node, "fsp,dimm-sides",
>>>> >> > +                                                mem->dimm_sides);
>>>> >> > +               mem->dimm_tcl = fdtdec_get_int(blob, node, "fsp,dimm-tcl",
>>>> >> > +                                              mem->dimm_tcl);
>>>> >> > +               mem->dimm_trpt_rcd = fdtdec_get_int(blob, node, "fsp,dimm-trpt-rcd",
>>>> >> > +                                                   mem->dimm_trpt_rcd);
>>>> >> > +               mem->dimm_twr = fdtdec_get_int(blob, node, "fsp,dimm-twr",
>>>> >> > +                                              mem->dimm_twr);
>>>> >> > +               mem->dimm_twtr = fdtdec_get_int(blob, node, "fsp,dimm-twtr",
>>>> >> > +                                               mem->dimm_twtr);
>>>> >> > +               mem->dimm_trrd = fdtdec_get_int(blob, node, "fsp,dimm-trrd",
>>>> >> > +                                               mem->dimm_trrd);
>>>> >> > +               mem->dimm_trtp = fdtdec_get_int(blob, node, "fsp,dimm-trtp",
>>>> >> > +                                               mem->dimm_trtp);
>>>> >> > +               mem->dimm_tfaw = fdtdec_get_int(blob, node, "fsp,dimm-tfaw",
>>>> >> > +                                               mem->dimm_tfaw);
>>>> >> > +       }
>>>> >> >  }
>>>> >> > diff --git a/arch/x86/dts/minnowmax.dts b/arch/x86/dts/minnowmax.dts
>>>> >> > index 0e59b18..279d08d 100644
>>>> >> > --- a/arch/x86/dts/minnowmax.dts
>>>> >> > +++ b/arch/x86/dts/minnowmax.dts
>>>> >> > @@ -121,6 +121,36 @@
>>>> >> >                         0x01000000 0x0 0x2000 0x2000 0 0xe000>;
>>>> >> >         };
>>>> >> >
>>>> >> > +       fsp {
>>>> >> > +               compatible = "intel,baytrail-fsp";
>>>> >> > +               fsp,mrc-init-tseg-size = <8>;
>>>> >> > +               fsp,mrc-init-mmio-size = <0x800>;
>>>> >> > +               fsp,emmc-boot-mode = <0xff>;
>>>> >> > +               fsp,enable-sdio = <1>;
>>>> >> > +               fsp,enable-sdcard = <1>;
>>>> >> > +               fsp,enable-hsuart0 = <1>;
>>>> >> > +               fsp,enable-i2-c0 = <0>;
>>>> >> > +               fsp,enable-i2-c2 = <0>;
>>>> >> > +               fsp,enable-i2-c3 = <0>;
>>>> >> > +               fsp,enable-i2-c4 = <0>;
>>>> >> > +               fsp,enable-xhci = <0>;
>>>> >> > +               fsp,igd-render-standby = <1>;
>>>> >> > +               fsp,enable-memory-down = <1>;
>>>> >> > +               fsp,memory-down-params {
>>>> >> > +                       compatible = "intel,baytrail-fsp-mdp";
>>>> >> > +                       fsp,dram-speed = <1>;
>>>> >> > +                       fsp,dimm-width = <1>;
>>>> >> > +                       fsp,dimm-density = <2>;
>>>> >> > +                       fsp,dimm-tcl = <0xb>;
>>>> >> > +                       fsp,dimm-trpt-rcd = <0xb>;
>>>> >> > +                       fsp,dimm-twr = <0xc>;
>>>> >> > +                       fsp,dimm-twtr = <6>;
>>>> >> > +                       fsp,dimm-trrd = <6>;
>>>> >> > +                       fsp,dimm-trtp = <6>;
>>>> >> > +                       fsp,dimm-tfaw = <0x14>;
>>>> >> > +               };
>>>> >> > +       };
>>>> >> > +
>>>> >> >         spi {
>>>> >> >                 #address-cells = <1>;
>>>> >> >                 #size-cells = <0>;
>>>> >> > diff --git a/doc/device-tree-bindings/misc/intel,baytrail-fsp.txt b/doc/device-tree-bindings/misc/intel,baytrail-fsp.txt
>>>> >> > new file mode 100644
>>>> >> > index 0000000..979d646
>>>> >> > --- /dev/null
>>>> >> > +++ b/doc/device-tree-bindings/misc/intel,baytrail-fsp.txt
>>>> >> > @@ -0,0 +1,119 @@
>>>> >> > +Intel Bay Trail FSP UPD Binding
>>>> >> > +==============================
>>>> >> > +
>>>> >> > +The device tree node which describes the overriding of the Intel Bay Trail FSP
>>>> >> > +UPD data for configuring the SoC.
>>>> >> > +
>>>> >> > +All properties are optional, if a property is unspecified then the FSP's
>>>> >> > +preconfigured choices will be used.  For this reason, using normal boolean
>>>> >> > +properties is not desired as the lack of a boolean property would disable a
>>>> >> > +given property and force the user to include all properties they wish to enable.
>>>> >> > +
>>>> >>
>>>> >> My understanding is that we either use device tree or the FSP
>>>> >> defaults. So if we describe an FSP node in device tree, then we go
>>>> >> with device tree as U-Boot's configuration. We cannot use partial
>>>> >> configuration from device tree and partial from FSP defaults. That's
>>>> >> confusing.
>>>> >
>>>> > I agree that it could be confusing, even just in writing this patch and
>>>> > trying to test it I've run into the fact that in order to see if I'm
>>>> > properly overriding the FSP's configuration that I would need to build a
>>>> > bunch of different FSP configurations in Intel's Binary Configuration
>>>> > Tool.  I've not done this as it would be extremely time consuming.
>>>> >
>>>> > I'm open to making the properties into bools and then overriding all of
>>>> > the FSP's UPD fields with sane defaults if the device tree does not
>>>> > specify a non-bool property.  I now think this might be a more sane
>>>> > approach for the long run, but I would need some assistance to know what
>>>> > the proper settings are for MinnowMax as I do not have one to test with
>>>> > and I do not know the full backstory as to why the FSP that shipped with
>>>> > MinnowMax was found to need to have its UPD overridden prior to my
>>>> > patch.
>>>>
>>>> It is mostly that the FSP can be set to anything since it is
>>>> configured by a proprietary binary tool. Having configuration in two
>>>> places, particularly where one is uncertain, does seem confusing.
>>>>
>>>> So I think Bin's proposal is that we either override everything and
>>>> don't use the FSP config, or we override nothing and use the entire
>>>> FSP config.
>>>
>>> Yes, I agree, I think this will be the easiest to understand way.
>>>
>>> What I'll do for v3 is to make all of the "enable" properties into
>>> bools.
>>
>> OK
>>
>>>
>>> For the properties which are ints when the device tree does not have a
>>> specification for the property, like dimm-width, do you think it
>>> is better to trust the FSP's compiled in value or to simply set a sane
>>> default value in fdtdec_get_int()?
>>>
>>>> Re the Minnowmax settings, I can dump out the list of current settings
>>>> if you like.
>>>
>>> Yes, please.  This would be very helpful.
>>
>> Sorry this has taken a while. It is quite painful. Here is my manual
>> decoding of fsp.bin. I did it this way you can decode a different
>> version, which might have things in different places. You can follow
>> along with a hex editor. I suppose you could also write a little tool
>> to display the UPD values.
>>
>> UPD
>> FSP binary data
>>
>> see find_fsp_header()
>>
>> 0 struct fv_header
>> u16 ext_hdr_off at 0x38 = 0x0060
>> 0 + 0x60 = 0x60
>>
>> 0x60 has struct fv_ext_header
>> ext_hdr_size at 0x70 = 0x14
>> 0x60 + 0x14 = 0x74
>> round up to multiple of 8 bytes = 0x78
>>
>> 0x78 has struct ffs_file_header
>> 16 byte guid at 0x78  - FSP_GUID_DATA1, etc.
>>
>> sizeof(struct ffs_file_header) = 0x18
>> 0x78 + 0x18 = 0x90
>>
>> 0x90 has struct raw_section
>> ->type at 0x93 = 0x19
>> sizeof(struct raw_section) = 4
>> 0x90 + 4 = 0x94
>>
>> struct fsp_header at 0x94
>> 0x94 has 'FSPH'
>>
>> ->img_base at offset 0x1c = 0xb0
>> 0xb0 has 0xfffc0000
>>
>> ->cfg_region_off at offset 0x24 = 0xb8
>> 0xb8 has 0x9dac
>>
>> ->cfg_region_size at offset 0x28 = 0xbc
>> 0xbc has 0x36
>>
>> struct struct vpd_region at 0x9dac
>> ->sign at offset 0 = 0x9dac
>>   "VLYVIEW1"
>> ->img_rev at offset 8 = 0x9db4
>>   0x303
>>
>> ->upd_offset at offset 0xc = 9x9db8
>>   -0x1f494
>>
>> struct upd_region at 0x1f494
>> ->signature = "VLV2UPDR"
>> data as per struct upd_region
>>
>> extends from 0x1f494 to 0x1f596 as per struct upd_region (ends with
>> bytes 0xaa, 0x55)
>> 00000000  56 4c 56 32 55 50 44 52  e4 ff ff ff 00 00 00 00  |VLV2UPDR........|
>> 00000010  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
>> 00000020  01 00 00 08 a0 a2 02 01  01 00 01 01 01 01 01 00  |................|
>> 00000030  00 00 00 00 01 01 01 01  01 01 01 01 01 01 01 01  |................|
>> 00000040  01 01 00 02 02 02 f8 03  00 00 01 00 00 01 00 00  |................|
>> 00000050  04 01 00 08 00 00 00 00  00 00 00 00 00 00 00 00  |................|
>> 00000060  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
>> *
>> 000000f0  00 02 01 01 00 00 01 03  00 09 09 0a 05 04 05 14  |................|
>> 00000100  aa 55                                             |.U|
>> 00000102
>>
>
> Not sure if you already know, but if you installed the Intel Binary
> Configuration Tool (BCT), the FSP binary default UPD settings can be
> viewed easily by opening the shipped .bsf file within the FSP package
> from the BCT.
>
> [snip]

I did manage to get that running a while back but now I'm not sure how
now. On Ubuntu both the 32- and 64-bit versions crash with a X windows
error, and on Windows I recently tried it and it required Windows 7,
which I don't have in virtualbox (maybe I should try to figure that
out one day). It would be much more accessible if there were an open
source tool.

Regards,
Simon


More information about the U-Boot mailing list