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

Simon Glass sjg at chromium.org
Wed Jul 1 23:10:50 CEST 2015


Hi Andrew,

On 1 July 2015 at 14:39, Andrew Bradford <andrew at bradfordembedded.com> wrote:
> Hi Bin,
>
> On 07/01 17:45, Bin Meng wrote:
>> Hi Andrew,
>>
>> On Mon, Jun 29, 2015 at 11:10 PM,  <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
>> >
>> > 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>
>> >
>>
>> Please add DECLARE_GLOBAL_DATA_PTR here.
>
> OK, will do in v2.
>
>> > @@ -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;
>>
>> Should the azalia config be moved to device tree too?
>
> Probably, yes.  Is there a good reason to *not* move it to the device tree
> along with the moves I'm making in this patch?
>
> If there's no objection, I'll work to include moving the azalia config
> to device tree in v2.

I'd prefer to put that in a separate patch. IMO it relates to that
particular device and probably should have its own device node,
completely separate from FSP. I think what you have already done has
tremendous value and we should get it in.

>
>> > -       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 */
>>
>> Do we need return error? Since you mentioned that if no device tree
>> settings the default FSP values will be used.
>
> Probably we don't need to return an error, I will remove the TODO in v2.
>
> At some point, if it is desired for runtime to know if the device tree
> settings or the built-in FSP settings were used, then it might be worth
> returning an error.
>
>> > +               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);
>> > +       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);
>> > +       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>;
>> > +               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>;
>> > +       };
>> > +
>> >         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
>>
>> Baytrail -> BayTrail. Please fix this globally.
>
> Should it be "Bay Trail" as per [1] when written out like this?
>
> [1]:http://ark.intel.com/products/codename/55844/Bay-Trail#@All
>
>> > +==============================
>> > +
>> > +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
>>
>> arch/x86/include/asm/ ...
>
> Yes, thank you for catching this! :)
> Will fix in v2.
>
>> > +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)
>> > --
>
> Thanks for taking the time to review this! :)
> -Andrew

Regards,
Simon


More information about the U-Boot mailing list