Hi Simon, On 06/30 12:13, Simon Glass wrote: > Hi Andrew, > > On 30 June 2015 at 10:58, Andrew Bradford <and...@bradfordembedded.com> wrote: > > > > Hi Simon, > > > > On 06/30 09:29, Simon Glass wrote: > > > +Bin > > > > > > Hi Andrew, > > > > > > On 29 June 2015 at 09:10, <and...@bradfordembedded.com> wrote: > > > > > > > > From: Andrew Bradford <andrew.bradf...@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.bradf...@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...@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(s...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot