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

From: Andrew Bradford <andrew.bradford@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@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

Hi Andrew,

On Wed, Jul 8, 2015 at 3:16 AM,  <andrew@bradfordembedded.com> wrote:
> From: Andrew Bradford <andrew.bradford@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@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@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@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.

> +                                                    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().

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

> +All properties can be found within the `upd-region` struct in
> +arch/x86/include/asm/arch-baytrail/fsp/fsp-vpd.h, under the same names, and in
> +Intel's FSP Binary Configuration Tool for Bay Trail.
> +
> +Optional properties:
> +
> +- fsp,mrc-init-tseg-size
> +- fsp,mrc-init-mmio-size
> +- fsp,mrc-init-spd-addr1
> +- fsp,mrc-init-spd-addr2
> +- fsp,emmc-boot-mode
> +- fsp,enable-sdio
> +- fsp,enable-sdcard
> +- fsp,enable-hsuart0
> +- fsp,enable-hsuart1
> +- fsp,enable-spi
> +- fsp,enable-sata
> +- fsp,sata-mode
> +- fsp,enable-azalia
> +- fsp,enable-xhci
> +- fsp,enable-lpe
> +- fsp,lpss-sio-enable-pci-mode
> +- fsp,enable-dma0
> +- fsp,enable-dma1
> +- fsp,enable-i2-c0
> +- fsp,enable-i2-c1
> +- fsp,enable-i2-c2
> +- fsp,enable-i2-c3
> +- fsp,enable-i2-c4
> +- fsp,enable-i2-c5
> +- fsp,enable-i2-c6
> +- fsp,enable-pwm0
> +- fsp,enable-pwm1
> +- fsp,enable-hsi
> +- fsp,igd-dvmt50-pre-alloc
> +- fsp,aperture-size
> +- fsp,gtt-size
> +- fsp,serial-debug-port-address
> +- fsp,serial-debug-port-type
> +- fsp,mrc-debug-msg
> +- fsp,isp-enable
> +- fsp,scc-enable-pci-mode
> +- fsp,igd-render-standby
> +- fsp,txe-uma-enable
> +- fsp,os-selection
> +- fsp,emmc45-ddr50-enabled
> +- fsp,emmc45-hs200-enabled
> +- fsp,emmc45-retune-timer-value
> +- fsp,enable-memory-down
> +
> +- fsp,memory-down-params {
> +
> +       The following are only used if enable-memory-down is set, otherwise
> +       the FSP will use the DIMM's SPD information to configure the memory:
> +       - fsp,dram-speed
> +       - fsp,dram-type
> +       - fsp,dimm-0-enable
> +       - fsp,dimm-1-enable
> +       - fsp,dimm-width
> +       - fsp,dimm-density
> +       - fsp,dimm-bus-width
> +       - fsp,dimm-sides
> +       - fsp,dimm-tcl
> +       - fsp,dimm-trpt-rcd
> +       - fsp,dimm-twr
> +       - fsp,dimm-twtr
> +       - fsp,dimm-trrd
> +       - fsp,dimm-trtp
> +       - fsp,dimm-tfaw
> +};
> +
> +Example (from MinnowMax Dual Core):
> +-----------------------------------
> +
> +/ {
> +       ...
> +
> +       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,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>;
> +               };
> +       };
> +
> +       ...
> +};
> diff --git a/include/fdtdec.h b/include/fdtdec.h
> index 2323603..76d07eb 100644
> --- a/include/fdtdec.h
> +++ b/include/fdtdec.h
> @@ -183,6 +183,8 @@ enum fdt_compat_id {
>         COMPAT_SOCIONEXT_XHCI,          /* Socionext UniPhier xHCI */
>         COMPAT_INTEL_PCH,               /* Intel PCH */
>         COMPAT_INTEL_IRQ_ROUTER,        /* Intel Interrupt Router */
> +       COMPAT_INTEL_BAYTRAIL_FSP,      /* Intel Bay Trail FSP */
> +       COMPAT_INTEL_BAYTRAIL_FSP_MDP,  /* Intel Bay Trail FSP memory-down params */
>
>         COMPAT_COUNT,
>  };
> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> index 9877849..1e48b82 100644
> --- a/lib/fdtdec.c
> +++ b/lib/fdtdec.c
> @@ -76,6 +76,8 @@ static const char * const compat_names[COMPAT_COUNT] = {
>         COMPAT(SOCIONEXT_XHCI, "socionext,uniphier-xhci"),
>         COMPAT(COMPAT_INTEL_PCH, "intel,bd82x6x"),
>         COMPAT(COMPAT_INTEL_IRQ_ROUTER, "intel,irq-router"),
> +       COMPAT(COMPAT_INTEL_BAYTRAIL_FSP, "intel,baytrail-fsp"),
> +       COMPAT(COMPAT_INTEL_BAYTRAIL_FSP_MDP, "intel,baytrail-fsp-mdp"),
>  };
>
>  const char *fdtdec_get_compatible(enum fdt_compat_id id)
> --

Regards,
Bin

Hi Bin,

On 07/08 11:18, Bin Meng wrote:
> Hi Andrew,
> 
> On Wed, Jul 8, 2015 at 3:16 AM,  <andrew@bradfordembedded.com> wrote:
> > From: Andrew Bradford <andrew.bradford@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@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@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@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.

> > +All properties can be found within the `upd-region` struct in
> > +arch/x86/include/asm/arch-baytrail/fsp/fsp-vpd.h, under the same names, and in
> > +Intel's FSP Binary Configuration Tool for Bay Trail.
> > +
> > +Optional properties:
> > +
> > +- fsp,mrc-init-tseg-size
> > +- fsp,mrc-init-mmio-size
> > +- fsp,mrc-init-spd-addr1
> > +- fsp,mrc-init-spd-addr2
> > +- fsp,emmc-boot-mode
> > +- fsp,enable-sdio
> > +- fsp,enable-sdcard
> > +- fsp,enable-hsuart0
> > +- fsp,enable-hsuart1
> > +- fsp,enable-spi
> > +- fsp,enable-sata
> > +- fsp,sata-mode
> > +- fsp,enable-azalia
> > +- fsp,enable-xhci
> > +- fsp,enable-lpe
> > +- fsp,lpss-sio-enable-pci-mode
> > +- fsp,enable-dma0
> > +- fsp,enable-dma1
> > +- fsp,enable-i2-c0
> > +- fsp,enable-i2-c1
> > +- fsp,enable-i2-c2
> > +- fsp,enable-i2-c3
> > +- fsp,enable-i2-c4
> > +- fsp,enable-i2-c5
> > +- fsp,enable-i2-c6
> > +- fsp,enable-pwm0
> > +- fsp,enable-pwm1
> > +- fsp,enable-hsi
> > +- fsp,igd-dvmt50-pre-alloc
> > +- fsp,aperture-size
> > +- fsp,gtt-size
> > +- fsp,serial-debug-port-address
> > +- fsp,serial-debug-port-type
> > +- fsp,mrc-debug-msg
> > +- fsp,isp-enable
> > +- fsp,scc-enable-pci-mode
> > +- fsp,igd-render-standby
> > +- fsp,txe-uma-enable
> > +- fsp,os-selection
> > +- fsp,emmc45-ddr50-enabled
> > +- fsp,emmc45-hs200-enabled
> > +- fsp,emmc45-retune-timer-value
> > +- fsp,enable-memory-down
> > +
> > +- fsp,memory-down-params {
> > +
> > +       The following are only used if enable-memory-down is set, otherwise
> > +       the FSP will use the DIMM's SPD information to configure the memory:
> > +       - fsp,dram-speed
> > +       - fsp,dram-type
> > +       - fsp,dimm-0-enable
> > +       - fsp,dimm-1-enable
> > +       - fsp,dimm-width
> > +       - fsp,dimm-density
> > +       - fsp,dimm-bus-width
> > +       - fsp,dimm-sides
> > +       - fsp,dimm-tcl
> > +       - fsp,dimm-trpt-rcd
> > +       - fsp,dimm-twr
> > +       - fsp,dimm-twtr
> > +       - fsp,dimm-trrd
> > +       - fsp,dimm-trtp
> > +       - fsp,dimm-tfaw
> > +};
> > +
> > +Example (from MinnowMax Dual Core):
> > +-----------------------------------
> > +
> > +/ {
> > +       ...
> > +
> > +       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,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>;
> > +               };
> > +       };
> > +
> > +       ...
> > +};
> > diff --git a/include/fdtdec.h b/include/fdtdec.h
> > index 2323603..76d07eb 100644
> > --- a/include/fdtdec.h
> > +++ b/include/fdtdec.h
> > @@ -183,6 +183,8 @@ enum fdt_compat_id {
> >         COMPAT_SOCIONEXT_XHCI,          /* Socionext UniPhier xHCI */
> >         COMPAT_INTEL_PCH,               /* Intel PCH */
> >         COMPAT_INTEL_IRQ_ROUTER,        /* Intel Interrupt Router */
> > +       COMPAT_INTEL_BAYTRAIL_FSP,      /* Intel Bay Trail FSP */
> > +       COMPAT_INTEL_BAYTRAIL_FSP_MDP,  /* Intel Bay Trail FSP memory-down params */
> >
> >         COMPAT_COUNT,
> >  };
> > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> > index 9877849..1e48b82 100644
> > --- a/lib/fdtdec.c
> > +++ b/lib/fdtdec.c
> > @@ -76,6 +76,8 @@ static const char * const compat_names[COMPAT_COUNT] = {
> >         COMPAT(SOCIONEXT_XHCI, "socionext,uniphier-xhci"),
> >         COMPAT(COMPAT_INTEL_PCH, "intel,bd82x6x"),
> >         COMPAT(COMPAT_INTEL_IRQ_ROUTER, "intel,irq-router"),
> > +       COMPAT(COMPAT_INTEL_BAYTRAIL_FSP, "intel,baytrail-fsp"),
> > +       COMPAT(COMPAT_INTEL_BAYTRAIL_FSP_MDP, "intel,baytrail-fsp-mdp"),
> >  };
> >
> >  const char *fdtdec_get_compatible(enum fdt_compat_id id)
> > --
> 
> Regards,
> Bin

Thanks for taking the time to review! I appreciate it :)
-Andrew

Hi,

On 8 July 2015 at 05:30, Andrew Bradford <andrew@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@bradfordembedded.com> wrote:
>> > From: Andrew Bradford <andrew.bradford@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@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@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@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.

Re the Minnowmax settings, I can dump out the list of current settings
if you like.

>
>> > +All properties can be found within the `upd-region` struct in
>> > +arch/x86/include/asm/arch-baytrail/fsp/fsp-vpd.h, under the same names, and in
>> > +Intel's FSP Binary Configuration Tool for Bay Trail.
>> > +
>> > +Optional properties:
>> > +
>> > +- fsp,mrc-init-tseg-size
>> > +- fsp,mrc-init-mmio-size
>> > +- fsp,mrc-init-spd-addr1
>> > +- fsp,mrc-init-spd-addr2
>> > +- fsp,emmc-boot-mode
>> > +- fsp,enable-sdio
>> > +- fsp,enable-sdcard
>> > +- fsp,enable-hsuart0
>> > +- fsp,enable-hsuart1
>> > +- fsp,enable-spi
>> > +- fsp,enable-sata
>> > +- fsp,sata-mode
>> > +- fsp,enable-azalia
>> > +- fsp,enable-xhci
>> > +- fsp,enable-lpe
>> > +- fsp,lpss-sio-enable-pci-mode
>> > +- fsp,enable-dma0
>> > +- fsp,enable-dma1
>> > +- fsp,enable-i2-c0
>> > +- fsp,enable-i2-c1
>> > +- fsp,enable-i2-c2
>> > +- fsp,enable-i2-c3
>> > +- fsp,enable-i2-c4
>> > +- fsp,enable-i2-c5
>> > +- fsp,enable-i2-c6
>> > +- fsp,enable-pwm0
>> > +- fsp,enable-pwm1
>> > +- fsp,enable-hsi
>> > +- fsp,igd-dvmt50-pre-alloc
>> > +- fsp,aperture-size
>> > +- fsp,gtt-size
>> > +- fsp,serial-debug-port-address
>> > +- fsp,serial-debug-port-type
>> > +- fsp,mrc-debug-msg
>> > +- fsp,isp-enable
>> > +- fsp,scc-enable-pci-mode
>> > +- fsp,igd-render-standby
>> > +- fsp,txe-uma-enable
>> > +- fsp,os-selection
>> > +- fsp,emmc45-ddr50-enabled
>> > +- fsp,emmc45-hs200-enabled
>> > +- fsp,emmc45-retune-timer-value
>> > +- fsp,enable-memory-down
>> > +
>> > +- fsp,memory-down-params {
>> > +
>> > +       The following are only used if enable-memory-down is set, otherwise
>> > +       the FSP will use the DIMM's SPD information to configure the memory:
>> > +       - fsp,dram-speed
>> > +       - fsp,dram-type
>> > +       - fsp,dimm-0-enable
>> > +       - fsp,dimm-1-enable
>> > +       - fsp,dimm-width
>> > +       - fsp,dimm-density
>> > +       - fsp,dimm-bus-width
>> > +       - fsp,dimm-sides
>> > +       - fsp,dimm-tcl
>> > +       - fsp,dimm-trpt-rcd
>> > +       - fsp,dimm-twr
>> > +       - fsp,dimm-twtr
>> > +       - fsp,dimm-trrd
>> > +       - fsp,dimm-trtp
>> > +       - fsp,dimm-tfaw
>> > +};
>> > +
>> > +Example (from MinnowMax Dual Core):
>> > +-----------------------------------
>> > +
>> > +/ {
>> > +       ...
>> > +
>> > +       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,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>;
>> > +               };
>> > +       };
>> > +
>> > +       ...
>> > +};
>> > diff --git a/include/fdtdec.h b/include/fdtdec.h
>> > index 2323603..76d07eb 100644
>> > --- a/include/fdtdec.h
>> > +++ b/include/fdtdec.h
>> > @@ -183,6 +183,8 @@ enum fdt_compat_id {
>> >         COMPAT_SOCIONEXT_XHCI,          /* Socionext UniPhier xHCI */
>> >         COMPAT_INTEL_PCH,               /* Intel PCH */
>> >         COMPAT_INTEL_IRQ_ROUTER,        /* Intel Interrupt Router */
>> > +       COMPAT_INTEL_BAYTRAIL_FSP,      /* Intel Bay Trail FSP */
>> > +       COMPAT_INTEL_BAYTRAIL_FSP_MDP,  /* Intel Bay Trail FSP memory-down params */
>> >
>> >         COMPAT_COUNT,
>> >  };
>> > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
>> > index 9877849..1e48b82 100644
>> > --- a/lib/fdtdec.c
>> > +++ b/lib/fdtdec.c
>> > @@ -76,6 +76,8 @@ static const char * const compat_names[COMPAT_COUNT] = {
>> >         COMPAT(SOCIONEXT_XHCI, "socionext,uniphier-xhci"),
>> >         COMPAT(COMPAT_INTEL_PCH, "intel,bd82x6x"),
>> >         COMPAT(COMPAT_INTEL_IRQ_ROUTER, "intel,irq-router"),
>> > +       COMPAT(COMPAT_INTEL_BAYTRAIL_FSP, "intel,baytrail-fsp"),
>> > +       COMPAT(COMPAT_INTEL_BAYTRAIL_FSP_MDP, "intel,baytrail-fsp-mdp"),
>> >  };
>> >
>> >  const char *fdtdec_get_compatible(enum fdt_compat_id id)
>> > --
>>
>> Regards,
>> Bin
>
> Thanks for taking the time to review! I appreciate it :)
> -Andrew

Regards,
Simon

Hi Simon,

On 07/10 06:53, Simon Glass wrote:
> Hi,
> 
> On 8 July 2015 at 05:30, Andrew Bradford <andrew@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@bradfordembedded.com> wrote:
> >> > From: Andrew Bradford <andrew.bradford@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@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@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@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.

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.

> >
> >> > +All properties can be found within the `upd-region` struct in
> >> > +arch/x86/include/asm/arch-baytrail/fsp/fsp-vpd.h, under the same names, and in
> >> > +Intel's FSP Binary Configuration Tool for Bay Trail.
> >> > +
> >> > +Optional properties:
> >> > +
> >> > +- fsp,mrc-init-tseg-size
> >> > +- fsp,mrc-init-mmio-size
> >> > +- fsp,mrc-init-spd-addr1
> >> > +- fsp,mrc-init-spd-addr2
> >> > +- fsp,emmc-boot-mode
> >> > +- fsp,enable-sdio
> >> > +- fsp,enable-sdcard
> >> > +- fsp,enable-hsuart0
> >> > +- fsp,enable-hsuart1
> >> > +- fsp,enable-spi
> >> > +- fsp,enable-sata
> >> > +- fsp,sata-mode
> >> > +- fsp,enable-azalia
> >> > +- fsp,enable-xhci
> >> > +- fsp,enable-lpe
> >> > +- fsp,lpss-sio-enable-pci-mode
> >> > +- fsp,enable-dma0
> >> > +- fsp,enable-dma1
> >> > +- fsp,enable-i2-c0
> >> > +- fsp,enable-i2-c1
> >> > +- fsp,enable-i2-c2
> >> > +- fsp,enable-i2-c3
> >> > +- fsp,enable-i2-c4
> >> > +- fsp,enable-i2-c5
> >> > +- fsp,enable-i2-c6
> >> > +- fsp,enable-pwm0
> >> > +- fsp,enable-pwm1
> >> > +- fsp,enable-hsi
> >> > +- fsp,igd-dvmt50-pre-alloc
> >> > +- fsp,aperture-size
> >> > +- fsp,gtt-size
> >> > +- fsp,serial-debug-port-address
> >> > +- fsp,serial-debug-port-type
> >> > +- fsp,mrc-debug-msg
> >> > +- fsp,isp-enable
> >> > +- fsp,scc-enable-pci-mode
> >> > +- fsp,igd-render-standby
> >> > +- fsp,txe-uma-enable
> >> > +- fsp,os-selection
> >> > +- fsp,emmc45-ddr50-enabled
> >> > +- fsp,emmc45-hs200-enabled
> >> > +- fsp,emmc45-retune-timer-value
> >> > +- fsp,enable-memory-down
> >> > +
> >> > +- fsp,memory-down-params {
> >> > +
> >> > +       The following are only used if enable-memory-down is set, otherwise
> >> > +       the FSP will use the DIMM's SPD information to configure the memory:
> >> > +       - fsp,dram-speed
> >> > +       - fsp,dram-type
> >> > +       - fsp,dimm-0-enable
> >> > +       - fsp,dimm-1-enable
> >> > +       - fsp,dimm-width
> >> > +       - fsp,dimm-density
> >> > +       - fsp,dimm-bus-width
> >> > +       - fsp,dimm-sides
> >> > +       - fsp,dimm-tcl
> >> > +       - fsp,dimm-trpt-rcd
> >> > +       - fsp,dimm-twr
> >> > +       - fsp,dimm-twtr
> >> > +       - fsp,dimm-trrd
> >> > +       - fsp,dimm-trtp
> >> > +       - fsp,dimm-tfaw
> >> > +};
> >> > +
> >> > +Example (from MinnowMax Dual Core):
> >> > +-----------------------------------
> >> > +
> >> > +/ {
> >> > +       ...
> >> > +
> >> > +       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,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>;
> >> > +               };
> >> > +       };
> >> > +
> >> > +       ...
> >> > +};
> >> > diff --git a/include/fdtdec.h b/include/fdtdec.h
> >> > index 2323603..76d07eb 100644
> >> > --- a/include/fdtdec.h
> >> > +++ b/include/fdtdec.h
> >> > @@ -183,6 +183,8 @@ enum fdt_compat_id {
> >> >         COMPAT_SOCIONEXT_XHCI,          /* Socionext UniPhier xHCI */
> >> >         COMPAT_INTEL_PCH,               /* Intel PCH */
> >> >         COMPAT_INTEL_IRQ_ROUTER,        /* Intel Interrupt Router */
> >> > +       COMPAT_INTEL_BAYTRAIL_FSP,      /* Intel Bay Trail FSP */
> >> > +       COMPAT_INTEL_BAYTRAIL_FSP_MDP,  /* Intel Bay Trail FSP memory-down params */
> >> >
> >> >         COMPAT_COUNT,
> >> >  };
> >> > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> >> > index 9877849..1e48b82 100644
> >> > --- a/lib/fdtdec.c
> >> > +++ b/lib/fdtdec.c
> >> > @@ -76,6 +76,8 @@ static const char * const compat_names[COMPAT_COUNT] = {
> >> >         COMPAT(SOCIONEXT_XHCI, "socionext,uniphier-xhci"),
> >> >         COMPAT(COMPAT_INTEL_PCH, "intel,bd82x6x"),
> >> >         COMPAT(COMPAT_INTEL_IRQ_ROUTER, "intel,irq-router"),
> >> > +       COMPAT(COMPAT_INTEL_BAYTRAIL_FSP, "intel,baytrail-fsp"),
> >> > +       COMPAT(COMPAT_INTEL_BAYTRAIL_FSP_MDP, "intel,baytrail-fsp-mdp"),
> >> >  };
> >> >
> >> >  const char *fdtdec_get_compatible(enum fdt_compat_id id)
> >> > --
> >>
> >> Regards,
> >> Bin
> >
> > Thanks for taking the time to review! I appreciate it :)
> > -Andrew
> 
> Regards,
> Simon

Thanks,
Andrew

Hi Andrew,

On 10 July 2015 at 12:24, Andrew Bradford <andrew@bradfordembedded.com> wrote:
> Hi Simon,
>
> On 07/10 06:53, Simon Glass wrote:
>> Hi,
>>
>> On 8 July 2015 at 05:30, Andrew Bradford <andrew@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@bradfordembedded.com> wrote:
>> >> > From: Andrew Bradford <andrew.bradford@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@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@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@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

>
>> >
>> >> > +All properties can be found within the `upd-region` struct in
>> >> > +arch/x86/include/asm/arch-baytrail/fsp/fsp-vpd.h, under the same names, and in
>> >> > +Intel's FSP Binary Configuration Tool for Bay Trail.
>> >> > +
>> >> > +Optional properties:
>> >> > +
>> >> > +- fsp,mrc-init-tseg-size
>> >> > +- fsp,mrc-init-mmio-size
>> >> > +- fsp,mrc-init-spd-addr1
>> >> > +- fsp,mrc-init-spd-addr2
>> >> > +- fsp,emmc-boot-mode
>> >> > +- fsp,enable-sdio
>> >> > +- fsp,enable-sdcard
>> >> > +- fsp,enable-hsuart0
>> >> > +- fsp,enable-hsuart1
>> >> > +- fsp,enable-spi
>> >> > +- fsp,enable-sata
>> >> > +- fsp,sata-mode
>> >> > +- fsp,enable-azalia
>> >> > +- fsp,enable-xhci
>> >> > +- fsp,enable-lpe
>> >> > +- fsp,lpss-sio-enable-pci-mode
>> >> > +- fsp,enable-dma0
>> >> > +- fsp,enable-dma1
>> >> > +- fsp,enable-i2-c0
>> >> > +- fsp,enable-i2-c1
>> >> > +- fsp,enable-i2-c2
>> >> > +- fsp,enable-i2-c3
>> >> > +- fsp,enable-i2-c4
>> >> > +- fsp,enable-i2-c5
>> >> > +- fsp,enable-i2-c6
>> >> > +- fsp,enable-pwm0
>> >> > +- fsp,enable-pwm1
>> >> > +- fsp,enable-hsi
>> >> > +- fsp,igd-dvmt50-pre-alloc
>> >> > +- fsp,aperture-size
>> >> > +- fsp,gtt-size
>> >> > +- fsp,serial-debug-port-address
>> >> > +- fsp,serial-debug-port-type
>> >> > +- fsp,mrc-debug-msg
>> >> > +- fsp,isp-enable
>> >> > +- fsp,scc-enable-pci-mode
>> >> > +- fsp,igd-render-standby
>> >> > +- fsp,txe-uma-enable
>> >> > +- fsp,os-selection
>> >> > +- fsp,emmc45-ddr50-enabled
>> >> > +- fsp,emmc45-hs200-enabled
>> >> > +- fsp,emmc45-retune-timer-value
>> >> > +- fsp,enable-memory-down
>> >> > +
>> >> > +- fsp,memory-down-params {
>> >> > +
>> >> > +       The following are only used if enable-memory-down is set, otherwise
>> >> > +       the FSP will use the DIMM's SPD information to configure the memory:
>> >> > +       - fsp,dram-speed
>> >> > +       - fsp,dram-type
>> >> > +       - fsp,dimm-0-enable
>> >> > +       - fsp,dimm-1-enable
>> >> > +       - fsp,dimm-width
>> >> > +       - fsp,dimm-density
>> >> > +       - fsp,dimm-bus-width
>> >> > +       - fsp,dimm-sides
>> >> > +       - fsp,dimm-tcl
>> >> > +       - fsp,dimm-trpt-rcd
>> >> > +       - fsp,dimm-twr
>> >> > +       - fsp,dimm-twtr
>> >> > +       - fsp,dimm-trrd
>> >> > +       - fsp,dimm-trtp
>> >> > +       - fsp,dimm-tfaw
>> >> > +};
>> >> > +
>> >> > +Example (from MinnowMax Dual Core):
>> >> > +-----------------------------------
>> >> > +
>> >> > +/ {
>> >> > +       ...
>> >> > +
>> >> > +       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,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>;
>> >> > +               };
>> >> > +       };
>> >> > +
>> >> > +       ...
>> >> > +};
>> >> > diff --git a/include/fdtdec.h b/include/fdtdec.h
>> >> > index 2323603..76d07eb 100644
>> >> > --- a/include/fdtdec.h
>> >> > +++ b/include/fdtdec.h
>> >> > @@ -183,6 +183,8 @@ enum fdt_compat_id {
>> >> >         COMPAT_SOCIONEXT_XHCI,          /* Socionext UniPhier xHCI */
>> >> >         COMPAT_INTEL_PCH,               /* Intel PCH */
>> >> >         COMPAT_INTEL_IRQ_ROUTER,        /* Intel Interrupt Router */
>> >> > +       COMPAT_INTEL_BAYTRAIL_FSP,      /* Intel Bay Trail FSP */
>> >> > +       COMPAT_INTEL_BAYTRAIL_FSP_MDP,  /* Intel Bay Trail FSP memory-down params */
>> >> >
>> >> >         COMPAT_COUNT,
>> >> >  };
>> >> > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
>> >> > index 9877849..1e48b82 100644
>> >> > --- a/lib/fdtdec.c
>> >> > +++ b/lib/fdtdec.c
>> >> > @@ -76,6 +76,8 @@ static const char * const compat_names[COMPAT_COUNT] = {
>> >> >         COMPAT(SOCIONEXT_XHCI, "socionext,uniphier-xhci"),
>> >> >         COMPAT(COMPAT_INTEL_PCH, "intel,bd82x6x"),
>> >> >         COMPAT(COMPAT_INTEL_IRQ_ROUTER, "intel,irq-router"),
>> >> > +       COMPAT(COMPAT_INTEL_BAYTRAIL_FSP, "intel,baytrail-fsp"),
>> >> > +       COMPAT(COMPAT_INTEL_BAYTRAIL_FSP_MDP, "intel,baytrail-fsp-mdp"),
>> >> >  };
>> >> >
>> >> >  const char *fdtdec_get_compatible(enum fdt_compat_id id)
>> >> > --
>> >>
>> >> Regards,
>> >> Bin
>> >
>> > Thanks for taking the time to review! I appreciate it :)
>> > -Andrew
>>
>> Regards,
>> Simon
>
> Thanks,
> Andrew

Regards,
Simon

Hi Simon,

On 07/21 14:17, Simon Glass wrote:
> Hi Andrew,
> 
> On 10 July 2015 at 12:24, Andrew Bradford <andrew@bradfordembedded.com> wrote:
> > Hi Simon,
> >
> > On 07/10 06:53, Simon Glass wrote:
> >> Hi,
> >>
> >> On 8 July 2015 at 05:30, Andrew Bradford <andrew@bradfordembedded.com> wrote:
> >> > Hi Bin,
> >> >
> >> > On 07/08 11:18, Bin Meng wrote:

<snip>

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

<snip>

Thanks! :)

I may not be able to send v3 of this patch until next week due to other
obligations.

Thanks for your help!
-Andrew

Hi Andrew, Simon,

On Wed, Jul 22, 2015 at 4:17 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Andrew,
>
> On 10 July 2015 at 12:24, Andrew Bradford <andrew@bradfordembedded.com> wrote:
>> Hi Simon,
>>
>> On 07/10 06:53, Simon Glass wrote:
>>> Hi,
>>>
>>> On 8 July 2015 at 05:30, Andrew Bradford <andrew@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@bradfordembedded.com> wrote:
>>> >> > From: Andrew Bradford <andrew.bradford@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@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@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@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]

Regards,
Bin

Hi Bin,

On 29 July 2015 at 08:08, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Andrew, Simon,
>
> On Wed, Jul 22, 2015 at 4:17 AM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Andrew,
>>
>> On 10 July 2015 at 12:24, Andrew Bradford <andrew@bradfordembedded.com> wrote:
>>> Hi Simon,
>>>
>>> On 07/10 06:53, Simon Glass wrote:
>>>> Hi,
>>>>
>>>> On 8 July 2015 at 05:30, Andrew Bradford <andrew@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@bradfordembedded.com> wrote:
>>>> >> > From: Andrew Bradford <andrew.bradford@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@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@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@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