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

Simon Glass sjg at chromium.org
Tue Jun 30 17:29:50 CEST 2015


+Bin

Hi Andrew,

On 29 June 2015 at 09:10, <andrew at bradfordembedded.com> wrote:
>
> From: Andrew Bradford <andrew.bradford at kodakalaris.com>
>
> Allow for configuration of FSP UPD from the device tree which will
> override any settings which the FSP was built with itself if the device
> tree settings exist, otherwise simply trust the FSP's defaults.
>
> Modifies the MinnowMax board to transfer the FSP UPD hard-coded settings
> to the MinnowMax dts.
>
> Signed-off-by: Andrew Bradford <andrew.bradford at kodakalaris.com>
> ---
>  arch/x86/cpu/baytrail/fsp_configs.c                | 183 +++++++++++++++++----
>  arch/x86/dts/minnowmax.dts                         |  27 +++
>  .../misc/intel,baytrail-fsp.txt                    | 113 +++++++++++++
>  include/fdtdec.h                                   |   1 +
>  lib/fdtdec.c                                       |   1 +
>  5 files changed, 295 insertions(+), 30 deletions(-)
>  create mode 100644 doc/device-tree-bindings/misc/intel,baytrail-fsp.txt

This is a big step forward in flexibility, thanks for sending this.

>
> diff --git a/arch/x86/cpu/baytrail/fsp_configs.c b/arch/x86/cpu/baytrail/fsp_configs.c
> index 86b6926..fd07eca 100644
> --- a/arch/x86/cpu/baytrail/fsp_configs.c
> +++ b/arch/x86/cpu/baytrail/fsp_configs.c
> @@ -1,11 +1,13 @@
>  /*
>   * Copyright (C) 2013, Intel Corporation
>   * Copyright (C) 2014, Bin Meng <bmeng.cn at gmail.com>
> + * Copyright (C) 2015, Kodak Alaris
>   *
>   * SPDX-License-Identifier:    Intel
>   */
>
>  #include <common.h>
> +#include <fdtdec.h>
>  #include <asm/arch/fsp/azalia.h>
>  #include <asm/fsp/fsp_support.h>
>
> @@ -116,41 +118,162 @@ const struct pch_azalia_config azalia_config = {
>         .reset_wait_timer_us = 300
>  };
>
> +/**
> + * Update the FSP's UPD.  The FSP itself can be configured for defaults to
> + * store in UPD through Intel's GUI configurator but likely a specific board
> + * will want to update these from u-boot, so allow for that via device tree.
> + * If the device tree does not specify a setting, trust the FSP's default.
> + */
>  void update_fsp_upd(struct upd_region *fsp_upd)
>  {
>         struct memory_down_data *mem;
> +       const void *blob = gd->fdt_blob;
> +       int node;
>
> -       /*
> -        * Configure everything here to avoid the poor hard-pressed user
> -        * needing to run Intel's binary configuration tool. It may also allow
> -        * us to support the 1GB single core variant easily.
> -        *
> -        * TODO(sjg at chromium.org): Move to device tree
> -        */
> -       fsp_upd->mrc_init_tseg_size = 8;
> -       fsp_upd->mrc_init_mmio_size = 0x800;
> -       fsp_upd->emmc_boot_mode = 0xff;
> -       fsp_upd->enable_sdio = 1;
> -       fsp_upd->enable_sdcard = 1;
> -       fsp_upd->enable_hsuart0 = 1;
>         fsp_upd->azalia_config_ptr = (uint32_t)&azalia_config;
> -       fsp_upd->enable_i2_c0 = 0;
> -       fsp_upd->enable_i2_c2 = 0;
> -       fsp_upd->enable_i2_c3 = 0;
> -       fsp_upd->enable_i2_c4 = 0;
> -       fsp_upd->enable_xhci = 0;
> -       fsp_upd->igd_render_standby = 1;
> +
> +       node = fdtdec_next_compatible(blob, 0, COMPAT_INTEL_BAYTRAIL_FSP);
> +       if (node < 0) {
> +               debug("%s: Cannot find FSP node\n", __func__);
> +               /* TODO: change return type for error indication */
> +               return;
> +       }
> +
> +       fsp_upd->mrc_init_tseg_size = fdtdec_get_int(blob, node,
> +                                                    "mrc_int_tseg_size",
> +                                                    fsp_upd->mrc_init_tseg_size);
> +       fsp_upd->mrc_init_mmio_size = fdtdec_get_int(blob, node,
> +                                                    "mrc_init_mmio_size",
> +                                                    fsp_upd->mrc_init_mmio_size);
> +       fsp_upd->mrc_init_spd_addr1 = fdtdec_get_int(blob, node,
> +                                                    "mrc_init_spd_addr1",
> +                                                    fsp_upd->mrc_init_spd_addr1);
> +       fsp_upd->mrc_init_spd_addr2 = fdtdec_get_int(blob, node,
> +                                                    "mrc_init_spd_addr2",
> +                                                    fsp_upd->mrc_init_spd_addr2);
> +       fsp_upd->emmc_boot_mode = fdtdec_get_int(blob, node, "emmc_boot_mode",
> +                                                fsp_upd->emmc_boot_mode);
> +       fsp_upd->enable_sdio = fdtdec_get_int(blob, node, "enable_sdio",
> +                                             fsp_upd->enable_sdio);

Shouldn't these be booleans?

> +       fsp_upd->enable_sdcard = fdtdec_get_int(blob, node, "enable_sdcard",
> +                                               fsp_upd->enable_sdcard);
> +       fsp_upd->enable_hsuart0 = fdtdec_get_int(blob, node, "enable_hsuart0",
> +                                                fsp_upd->enable_hsuart0);
> +       fsp_upd->enable_hsuart1 = fdtdec_get_int(blob, node, "enable_hsuart1",
> +                                                fsp_upd->enable_hsuart1);
> +       fsp_upd->enable_spi = fdtdec_get_int(blob, node, "enable_spi",
> +                                            fsp_upd->enable_spi);
> +       fsp_upd->enable_sata = fdtdec_get_int(blob, node, "enable_sata",
> +                                             fsp_upd->enable_sata);
> +       fsp_upd->enable_azalia = fdtdec_get_int(blob, node, "enable_azalia",
> +                                               fsp_upd->enable_azalia);
> +       fsp_upd->enable_xhci = fdtdec_get_int(blob, node, "enable_xhci",
> +                                             fsp_upd->enable_xhci);
> +       fsp_upd->enable_lpe = fdtdec_get_int(blob, node, "enable_lpe",
> +                                            fsp_upd->enable_lpe);
> +       fsp_upd->lpss_sio_enable_pci_mode = fdtdec_get_int(blob, node,
> +                                                          "lpss_sio_enable_pci_mode",
> +                                                          fsp_upd->lpss_sio_enable_pci_mode);
> +       fsp_upd->enable_dma0 = fdtdec_get_int(blob, node, "enable_dma0",
> +                                             fsp_upd->enable_dma0);
> +       fsp_upd->enable_dma1 = fdtdec_get_int(blob, node, "enable_dma1",
> +                                             fsp_upd->enable_dma1);
> +       fsp_upd->enable_i2_c0 = fdtdec_get_int(blob, node, "enable_i2_c0",
> +                                              fsp_upd->enable_i2_c0);
> +       fsp_upd->enable_i2_c1 = fdtdec_get_int(blob, node, "enable_i2_c1",
> +                                              fsp_upd->enable_i2_c1);
> +       fsp_upd->enable_i2_c2 = fdtdec_get_int(blob, node, "enable_i2_c2",
> +                                              fsp_upd->enable_i2_c2);
> +       fsp_upd->enable_i2_c3 = fdtdec_get_int(blob, node, "enable_i2_c3",
> +                                              fsp_upd->enable_i2_c3);
> +       fsp_upd->enable_i2_c4 = fdtdec_get_int(blob, node, "enable_i2_c4",
> +                                              fsp_upd->enable_i2_c4);
> +       fsp_upd->enable_i2_c5 = fdtdec_get_int(blob, node, "enable_i2_c5",
> +                                              fsp_upd->enable_i2_c5);
> +       fsp_upd->enable_i2_c6 = fdtdec_get_int(blob, node, "enable_i2_c6",
> +                                              fsp_upd->enable_i2_c6);
> +       fsp_upd->enable_pwm0 = fdtdec_get_int(blob, node, "enable_pwm0",
> +                                             fsp_upd->enable_pwm0);
> +       fsp_upd->enable_pwm1 = fdtdec_get_int(blob, node, "enable_pwm1",
> +                                             fsp_upd->enable_pwm1);
> +       fsp_upd->enable_hsi = fdtdec_get_int(blob, node, "enable_hsi",
> +                                           fsp_upd->enable_hsi);
> +       fsp_upd->igd_dvmt50_pre_alloc = fdtdec_get_int(blob, node,
> +                                                      "igd_dvmt50_pre_alloc",
> +                                                      fsp_upd->igd_dvmt50_pre_alloc);
> +       fsp_upd->aperture_size = fdtdec_get_int(blob, node, "aperture_size",
> +                                               fsp_upd->aperture_size);
> +       fsp_upd->gtt_size = fdtdec_get_int(blob, node, "gtt_size",
> +                                          fsp_upd->gtt_size);
> +       fsp_upd->serial_debug_port_address = fdtdec_get_int(blob, node,
> +                                                           "serial_debug_port_address",
> +                                                           fsp_upd->serial_debug_port_address);
> +       fsp_upd->serial_debug_port_type = fdtdec_get_int(blob, node,
> +                                                        "serial_debug_port_type",
> +                                                        fsp_upd->serial_debug_port_type);
> +       fsp_upd->mrc_debug_msg = fdtdec_get_int(blob, node, "mrc_debug_msg",
> +                                               fsp_upd->mrc_debug_msg);
> +       fsp_upd->isp_enable = fdtdec_get_int(blob, node, "isp_enable",
> +                                            fsp_upd->isp_enable);
> +       fsp_upd->scc_enable_pci_mode = fdtdec_get_int(blob, node,
> +                                                     "scc_enable_pci_mode",
> +                                                     fsp_upd->scc_enable_pci_mode);
> +       fsp_upd->igd_render_standby = fdtdec_get_int(blob, node,
> +                                                    "igd_render_standby",
> +                                                    fsp_upd->igd_render_standby);
> +       fsp_upd->txe_uma_enable = fdtdec_get_int(blob, node, "txe_uma_enable",
> +                                                fsp_upd->txe_uma_enable);
> +       fsp_upd->os_selection = fdtdec_get_int(blob, node, "os_selection",
> +                                              fsp_upd->os_selection);
> +       fsp_upd->emmc45_ddr50_enabled = fdtdec_get_int(blob, node,
> +                                                      "emmc45_ddr50_enabled",
> +                                                      fsp_upd->emmc45_ddr50_enabled);
> +       fsp_upd->emmc45_hs200_enabled = fdtdec_get_int(blob, node,
> +                                                      "emmc45_hs200_enabled",
> +                                                      fsp_upd->emmc45_hs200_enabled);
> +       fsp_upd->emmc45_retune_timer_value = fdtdec_get_int(blob, node,
> +                                                           "emmc45_retune_timer_value",
> +                                                           fsp_upd->emmc45_retune_timer_value);
> +       fsp_upd->igd_render_standby = fdtdec_get_int(blob, node,
> +                                                    "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,
> +                                                "enable_memory_down",
> +                                                mem->enable_memory_down);

Probably this should be fdtdec_get_bool().

> +       if (mem->enable_memory_down) {
> +               mem->dram_speed = fdtdec_get_int(blob, node, "dram_speed",
> +                                                mem->dram_speed);
> +               mem->dram_type = fdtdec_get_int(blob, node, "dram_type",
> +                                               mem->dram_type);
> +               mem->dimm_0_enable = fdtdec_get_int(blob, node, "dimm_0_enable",
> +                                                  mem->dimm_0_enable);
> +               mem->dimm_1_enable = fdtdec_get_int(blob, node, "dimm_1_enable",
> +                                                   mem->dimm_1_enable);
> +               mem->dimm_width = fdtdec_get_int(blob, node, "dimm_width",
> +                                                mem->dimm_width);
> +               mem->dimm_density = fdtdec_get_int(blob, node,
> +                                                  "dimm_density",
> +                                                  mem->dimm_density);
> +               mem->dimm_bus_width = fdtdec_get_int(blob, node,
> +                                                    "dimm_bus_width",
> +                                                    mem->dimm_bus_width);
> +               mem->dimm_sides = fdtdec_get_int(blob, node, "dimm_sides",
> +                                                mem->dimm_sides);
> +               mem->dimm_tcl = fdtdec_get_int(blob, node, "dimm_tcl",
> +                                              mem->dimm_tcl);
> +               mem->dimm_trpt_rcd = fdtdec_get_int(blob, node, "dimm_trpt_rcd",
> +                                                   mem->dimm_trpt_rcd);
> +               mem->dimm_twr = fdtdec_get_int(blob, node, "dimm_twr",
> +                                              mem->dimm_twr);
> +               mem->dimm_twtr = fdtdec_get_int(blob, node, "dimm_twtr",
> +                                               mem->dimm_twtr);
> +               mem->dimm_trrd = fdtdec_get_int(blob, node, "dimm_trrd",
> +                                               mem->dimm_trrd);
> +               mem->dimm_trtp = fdtdec_get_int(blob, node, "dimm_trtp",
> +                                               mem->dimm_trtp);
> +               mem->dimm_tfaw = fdtdec_get_int(blob, node, "dimm_tfaw",
> +                                               mem->dimm_tfaw);
> +       }
>  }
> diff --git a/arch/x86/dts/minnowmax.dts b/arch/x86/dts/minnowmax.dts
> index bd21bfb..bb76e69 100644
> --- a/arch/x86/dts/minnowmax.dts
> +++ b/arch/x86/dts/minnowmax.dts
> @@ -111,6 +111,33 @@
>
>         };
>
> +       fsp {
> +               compatible = "intel,baytrail-fsp";
> +               mrc_init_tseg_size = <8>;

We tend to use hyphen in device tree and reserve underscore for phandles.

So I think this should be mrc-init-tseg-size, and similarly for the
others. Also how about an fsp, prefix, so:

   fsp,mrc-init-tseg-size = <8>;

This indicates the context of the setting.

> +               mrc_init_mmio_size = <0x800>;
> +               emmc_boot_mode = <0xff>;
> +               enable_sdio = <1>;

You can just have

     fsp,enable-sdio;

meaning that it is true. The fdtdec_get_bool() function handles this.

> +               enable_sdcard = <1>;
> +               enable_hsuart0 = <1>;
> +               enable_i2_c0 = <0>;
> +               enable_i2_c2 = <0>;
> +               enable_i2_c3 = <0>;
> +               enable_i2_c4 = <0>;
> +               enable_xhci = <0>;
> +               igd_render_standby = <1>;
> +               enable_memory_down = <1>;

How about putting the memory parameters in a separate subnode of your
FSP node, like:

   memory-params {
       fsp,dram-speed = <1>;
...
   };

That groups them nicely into one area.

> +               dram_speed = <1>;
> +               dimm_width = <1>;
> +               dimm_density = <2>;
> +               dimm_tcl = <0xb>;
> +               dimm_trpt_rcd = <0xb>;
> +               dimm_twr = <0xc>;
> +               dimm_twtr = <6>;
> +               dimm_trrd = <6>;
> +               dimm_trtp = <6>;
> +               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..a9841e7
> --- /dev/null
> +++ b/doc/device-tree-bindings/misc/intel,baytrail-fsp.txt
> @@ -0,0 +1,113 @@
> +Intel Baytrail FSP UPD Binding
> +==============================
> +
> +The device tree node which describes the overriding of the Intel Baytrail FSP
> +UPD data for configuring the SoC.
> +
> +All properties are optional, if unspecified then the FSP's preconfigured choices
> +will be used.
> +
> +All properties can be found within the `upd_region` struct in
> +arch/x86/asm/arch-baytrail/fsp/fsp_vpd.h under the same names and in Intel's FSP
> +Binary Configuration Tool for Baytrail.
> +
> +Optional properties:
> +
> +- mrc_init_tseg_size
> +- mrc_init_mmio_size
> +- mrc_init_spd_addr1
> +- mrc_init_spd_addr2
> +- emmc_boot_mode
> +- enable_sdio
> +- enable_sdcard
> +- enable_hsuart0
> +- enable_hsuart1
> +- enable_spi
> +- enable_sata
> +- sata_mode
> +- enable_azalia
> +- enable_xhci
> +- enable_lpe
> +- lpss_sio_enable_pci_mode
> +- enable_dma0
> +- enable_dma1
> +- enable_i2_c0
> +- enable_i2_c1
> +- enable_i2_c2
> +- enable_i2_c3
> +- enable_i2_c4
> +- enable_i2_c5
> +- enable_i2_c6
> +- enable_pwm0
> +- enable_pwm1
> +- enable_hsi
> +- igd_dvmt50_pre_alloc
> +- aperture_size
> +- gtt_size
> +- serial_debug_port_address
> +- serial_debug_port_type
> +- mrc_debug_msg
> +- isp_enable
> +- scc_enable_pci_mode
> +- igd_render_standby
> +- txe_uma_enable
> +- os_selection
> +- emmc45_ddr50_enabled
> +- emmc45_hs200_enabled
> +- emmc45_retune_timer_value
> +- enable_memory_down
> +
> +       The following are only used if enable_memory_down is enabled, otherwise
> +       the FSP will use the DIMM's SPD information to configure the memory:
> +       - dram_speed
> +       - dram_type
> +       - dimm_0_enable
> +       - dimm_1_enable
> +       - dimm_width
> +       - dimm_density
> +       - dimm_bus_width
> +       - dimm_sides
> +       - dimm_tcl
> +       - dimm_trpt_rcd
> +       - dimm_twr
> +       - dimm_twtr
> +       - dimm_trrd
> +       - dimm_trtp
> +       - dimm_tfaw
> +
> +
> +Example (from MinnowMax Dual Core):
> +-----------------------------------
> +
> +/ {
> +       ...
> +
> +       fsp {
> +               compatible = "intel,baytrail-fsp";
> +               mrc_init_tseg_size = <8>;
> +               mrc_init_mmio_size = <0x800>;
> +               emmc_boot_mode = <0xff>;
> +               enable_sdio = <1>;
> +               enable_sdcard = <1>;
> +               enable_hsuart0 = <1>;
> +               enable_i2_c0 = <0>;
> +               enable_i2_c2 = <0>;
> +               enable_i2_c3 = <0>;
> +               enable_i2_c4 = <0>;
> +               enable_xhci = <0>;
> +               igd_render_standby = <1>;
> +               enable_memory_down = <1>;
> +               dram_speed = <1>;
> +               dimm_width = <1>;
> +               dimm_density = <2>;
> +               dimm_tcl = <0xb>;
> +               dimm_trpt_rcd = <0xb>;
> +               dimm_twr = <0xc>;
> +               dimm_twtr = <6>;
> +               dimm_trrd = <6>;
> +               dimm_trtp = <6>;
> +               dimm_tfaw = <0x14>;
> +       };
> +
> +       ...
> +};
> diff --git a/include/fdtdec.h b/include/fdtdec.h
> index 2323603..2b09cce 100644
> --- a/include/fdtdec.h
> +++ b/include/fdtdec.h
> @@ -183,6 +183,7 @@ 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 Baytrail FSP */
>
>         COMPAT_COUNT,
>  };
> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> index 9877849..c7a27ac 100644
> --- a/lib/fdtdec.c
> +++ b/lib/fdtdec.c
> @@ -76,6 +76,7 @@ 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"),
>  };
>
>  const char *fdtdec_get_compatible(enum fdt_compat_id id)
> --
> 1.9.1
>

At some point we could move this to driver model, but that can come later.

Regards,
Simon


More information about the U-Boot mailing list