| Message ID | 1435590643-677-1-git-send-email-andrew@bradfordembedded.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
+Bin Hi Andrew, On 29 June 2015 at 09:10, <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> > --- > 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@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@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
Hi Simon, On 06/30 09:29, Simon Glass wrote: > +Bin > > Hi Andrew, > > On 29 June 2015 at 09:10, <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> > > --- > > 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@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@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? Yes, probably all the "enable" elements should be booleans, I'll fix in v2. > > > + 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 I use fdtdec_get_bool() then if a property is not called out in the device tree, won't it return 0? If this is the case, then I would just need to change my documentation to say that any boolean properties which you don't set to 1 will be set to disabled. That is kind of confusing since the FSP may be configured to enable a given boolean property but then by omission in the dts you disable it. Comparing that with the integer properties where using fdtdec_get_int() should have a sane default (I use what the FSP was configured with) and so by leaving one of those properties out of your dts then you just get the default from the FSP itself. I think the original changes to fsp_configs.c assumed that some of what the FSP was configured with was sane and OK for Minnowmax. This difference of operation isn't bad (it might even be a really good thing), per se, I will just need to ensure it's documented clearly. And I'd also like to get some input then on how to setup the Minnowmax dts file because there will need to be some boolean properties enabled which likely aren't right now in this patch and I do not have access to a Minnowmax board to test with. > > > + 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. OK, that makes sense, I'll adopt that for v2. > > + 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. Yes, that's a good idea. I'll adopt this style for v2. > > + 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. If I wanted to just start with driver model, would it be that much more work? Is there a good example that you'd recommend I learn from for this kind of driver model implementation? Sorry, driver model is new to me, although I've started looking at the docs and the demo implementation. Thanks for taking time to review this! :) -Andrew
Hi Andrew, On 30 June 2015 at 10:58, Andrew Bradford <andrew@bradfordembedded.com> wrote: > > Hi Simon, > > On 06/30 09:29, Simon Glass wrote: > > +Bin > > > > Hi Andrew, > > > > On 29 June 2015 at 09:10, <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> > > > --- > > > 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@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@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? > > Yes, probably all the "enable" elements should be booleans, I'll fix in > v2. > > > > > > + 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 I use fdtdec_get_bool() then if a property is not called out in the > device tree, won't it return 0? Yes. > > If this is the case, then I would just need to change my documentation > to say that any boolean properties which you don't set to 1 will be set > to disabled. That is kind of confusing since the FSP may be configured > to enable a given boolean property but then by omission in the dts you > disable it. Comparing that with the integer properties where using > fdtdec_get_int() should have a sane default (I use what the FSP was > configured with) and so by leaving one of those properties out of your > dts then you just get the default from the FSP itself. I think the > original changes to fsp_configs.c assumed that some of what the FSP was > configured with was sane and OK for Minnowmax. > > This difference of operation isn't bad (it might even be a really good > thing), per se, I will just need to ensure it's documented clearly. And > I'd also like to get some input then on how to setup the Minnowmax dts > file because there will need to be some boolean properties enabled which > likely aren't right now in this patch and I do not have access to a > Minnowmax board to test with. Yes I agree that could be confusing. So perhaps it is better as you have it now. You have a comment: "All properties are optional, if unspecified then the FSP's preconfigured choices +will be used." I suggest mentioning that normal boolean properties (where presence indicates 'true') cannot be used since it would then not be possible to override the default value with 0. > > > > > > + 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. > > OK, that makes sense, I'll adopt that for v2. > > > > + 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. > > Yes, that's a good idea. I'll adopt this style for v2. > > > > + 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. > > If I wanted to just start with driver model, would it be that much more > work? Is there a good example that you'd recommend I learn from for > this kind of driver model implementation? > > Sorry, driver model is new to me, although I've started looking at the > docs and the demo implementation. At present x86_fsp_init() is too early in the board_init_f() sequence for this to work (it needs to go after initf_dm(). I think this is something we should discuss with Bin. I think that arch_cpu_init() should be able to move to driver model (we have a CPU uclass now), but I haven't looked at the work involved. It's not required for what you want though. But assuming that is resolved, you could do something very simple, like these two patches which add a new uclass, and then a driver that uses it. http://patchwork.ozlabs.org/patch/487822/ http://patchwork.ozlabs.org/patch/487868/ So you could add an FSP uclass and either put the init code (contents of fsp_init() function) into the driver's normal probe() method or create a uclass method called init() and put the code there. Then, whenever you want to call it: struct udevice *dev; int ret; ret = uclass_get_device(UCLASS_FSP, 0, &dev); if (ret) .. handle error and this will call your driver's probe method. If you go the init() method way you also need: ret = fsp_init(dev); (with fsp_init() being implemented in your uclass) Given where things are I think your patch is a good step, and the driver model conversion can be a separate patch. > > Thanks for taking time to review this! :) > -Andrew Regards, Simon
Hi Simon, On 06/30 12:13, Simon Glass wrote: > Hi Andrew, > > On 30 June 2015 at 10:58, Andrew Bradford <andrew@bradfordembedded.com> wrote: > > > > Hi Simon, > > > > On 06/30 09:29, Simon Glass wrote: > > > +Bin > > > > > > Hi Andrew, > > > > > > On 29 June 2015 at 09:10, <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> > > > > --- > > > > 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@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@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? > > > > Yes, probably all the "enable" elements should be booleans, I'll fix in > > v2. > > > > > > > > > + 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 I use fdtdec_get_bool() then if a property is not called out in the > > device tree, won't it return 0? > > Yes. > > > > > If this is the case, then I would just need to change my documentation > > to say that any boolean properties which you don't set to 1 will be set > > to disabled. That is kind of confusing since the FSP may be configured > > to enable a given boolean property but then by omission in the dts you > > disable it. Comparing that with the integer properties where using > > fdtdec_get_int() should have a sane default (I use what the FSP was > > configured with) and so by leaving one of those properties out of your > > dts then you just get the default from the FSP itself. I think the > > original changes to fsp_configs.c assumed that some of what the FSP was > > configured with was sane and OK for Minnowmax. > > > > This difference of operation isn't bad (it might even be a really good > > thing), per se, I will just need to ensure it's documented clearly. And > > I'd also like to get some input then on how to setup the Minnowmax dts > > file because there will need to be some boolean properties enabled which > > likely aren't right now in this patch and I do not have access to a > > Minnowmax board to test with. > > Yes I agree that could be confusing. So perhaps it is better as you have it now. > > You have a comment: > > "All properties are optional, if unspecified then the FSP's > preconfigured choices > +will be used." > > I suggest mentioning that normal boolean properties (where presence > indicates 'true') cannot be used since it would then not be possible > to override the default value with 0. OK, will do! > > > > > > > > > > + 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. > > > > OK, that makes sense, I'll adopt that for v2. > > > > > > + 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. > > > > Yes, that's a good idea. I'll adopt this style for v2. > > > > > > + 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. > > > > If I wanted to just start with driver model, would it be that much more > > work? Is there a good example that you'd recommend I learn from for > > this kind of driver model implementation? > > > > Sorry, driver model is new to me, although I've started looking at the > > docs and the demo implementation. > > At present x86_fsp_init() is too early in the board_init_f() sequence > for this to work (it needs to go after initf_dm(). I think this is > something we should discuss with Bin. > > I think that arch_cpu_init() should be able to move to driver model > (we have a CPU uclass now), but I haven't looked at the work involved. > It's not required for what you want though. > > But assuming that is resolved, you could do something very simple, > like these two patches which add a new uclass, and then a driver that > uses it. > > http://patchwork.ozlabs.org/patch/487822/ > http://patchwork.ozlabs.org/patch/487868/ > > So you could add an FSP uclass and either put the init code (contents > of fsp_init() function) into the driver's normal probe() method or > create a uclass method called init() and put the code there. > > Then, whenever you want to call it: > > struct udevice *dev; > int ret; > > ret = uclass_get_device(UCLASS_FSP, 0, &dev); > if (ret) > .. handle error > > and this will call your driver's probe method. > > If you go the init() method way you also need: > > ret = fsp_init(dev); > > (with fsp_init() being implemented in your uclass) > > Given where things are I think your patch is a good step, and the > driver model conversion can be a separate patch. OK, thanks for the info. I think for now I'll hold off on doing the driver model conversion till a few more things are moved in that direction. I will send a v2 that fixes a few things, soon. Thanks! -Andrew
Hi Andrew, On Mon, Jun 29, 2015 at 11:10 PM, <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> > --- > 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@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. > @@ -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@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? > - 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. > + 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. > +============================== > + > +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/ ... > +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) > -- Regards, Bin
Hi Bin, On 07/01 17:45, Bin Meng wrote: > Hi Andrew, > > On Mon, Jun 29, 2015 at 11:10 PM, <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> > > --- > > 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@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@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. > > - 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
Hi Andrew, On 1 July 2015 at 14:39, Andrew Bradford <andrew@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@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> >> > --- >> > 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@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@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
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@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@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); + 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 +============================== + +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)