[U-Boot] [PATCH] x86: baytrail: Configure FSP UPD from device tree
Andrew Bradford
andrew at bradfordembedded.com
Tue Jun 30 22:16:53 CEST 2015
Hi Simon,
On 06/30 12:13, Simon Glass wrote:
> Hi Andrew,
>
> On 30 June 2015 at 10:58, Andrew Bradford <andrew at bradfordembedded.com> wrote:
> >
> > Hi Simon,
> >
> > On 06/30 09:29, Simon Glass wrote:
> > > +Bin
> > >
> > > Hi Andrew,
> > >
> > > On 29 June 2015 at 09:10, <andrew at bradfordembedded.com> wrote:
> > > >
> > > > From: Andrew Bradford <andrew.bradford at kodakalaris.com>
> > > >
> > > > Allow for configuration of FSP UPD from the device tree which will
> > > > override any settings which the FSP was built with itself if the device
> > > > tree settings exist, otherwise simply trust the FSP's defaults.
> > > >
> > > > Modifies the MinnowMax board to transfer the FSP UPD hard-coded settings
> > > > to the MinnowMax dts.
> > > >
> > > > Signed-off-by: Andrew Bradford <andrew.bradford at kodakalaris.com>
> > > > ---
> > > > arch/x86/cpu/baytrail/fsp_configs.c | 183 +++++++++++++++++----
> > > > arch/x86/dts/minnowmax.dts | 27 +++
> > > > .../misc/intel,baytrail-fsp.txt | 113 +++++++++++++
> > > > include/fdtdec.h | 1 +
> > > > lib/fdtdec.c | 1 +
> > > > 5 files changed, 295 insertions(+), 30 deletions(-)
> > > > create mode 100644 doc/device-tree-bindings/misc/intel,baytrail-fsp.txt
> > >
> > > This is a big step forward in flexibility, thanks for sending this.
> > >
> > > >
> > > > diff --git a/arch/x86/cpu/baytrail/fsp_configs.c b/arch/x86/cpu/baytrail/fsp_configs.c
> > > > index 86b6926..fd07eca 100644
> > > > --- a/arch/x86/cpu/baytrail/fsp_configs.c
> > > > +++ b/arch/x86/cpu/baytrail/fsp_configs.c
> > > > @@ -1,11 +1,13 @@
> > > > /*
> > > > * Copyright (C) 2013, Intel Corporation
> > > > * Copyright (C) 2014, Bin Meng <bmeng.cn at gmail.com>
> > > > + * Copyright (C) 2015, Kodak Alaris
> > > > *
> > > > * SPDX-License-Identifier: Intel
> > > > */
> > > >
> > > > #include <common.h>
> > > > +#include <fdtdec.h>
> > > > #include <asm/arch/fsp/azalia.h>
> > > > #include <asm/fsp/fsp_support.h>
> > > >
> > > > @@ -116,41 +118,162 @@ const struct pch_azalia_config azalia_config = {
> > > > .reset_wait_timer_us = 300
> > > > };
> > > >
> > > > +/**
> > > > + * Update the FSP's UPD. The FSP itself can be configured for defaults to
> > > > + * store in UPD through Intel's GUI configurator but likely a specific board
> > > > + * will want to update these from u-boot, so allow for that via device tree.
> > > > + * If the device tree does not specify a setting, trust the FSP's default.
> > > > + */
> > > > void update_fsp_upd(struct upd_region *fsp_upd)
> > > > {
> > > > struct memory_down_data *mem;
> > > > + const void *blob = gd->fdt_blob;
> > > > + int node;
> > > >
> > > > - /*
> > > > - * Configure everything here to avoid the poor hard-pressed user
> > > > - * needing to run Intel's binary configuration tool. It may also allow
> > > > - * us to support the 1GB single core variant easily.
> > > > - *
> > > > - * TODO(sjg at chromium.org): Move to device tree
> > > > - */
> > > > - fsp_upd->mrc_init_tseg_size = 8;
> > > > - fsp_upd->mrc_init_mmio_size = 0x800;
> > > > - fsp_upd->emmc_boot_mode = 0xff;
> > > > - fsp_upd->enable_sdio = 1;
> > > > - fsp_upd->enable_sdcard = 1;
> > > > - fsp_upd->enable_hsuart0 = 1;
> > > > fsp_upd->azalia_config_ptr = (uint32_t)&azalia_config;
> > > > - fsp_upd->enable_i2_c0 = 0;
> > > > - fsp_upd->enable_i2_c2 = 0;
> > > > - fsp_upd->enable_i2_c3 = 0;
> > > > - fsp_upd->enable_i2_c4 = 0;
> > > > - fsp_upd->enable_xhci = 0;
> > > > - fsp_upd->igd_render_standby = 1;
> > > > +
> > > > + node = fdtdec_next_compatible(blob, 0, COMPAT_INTEL_BAYTRAIL_FSP);
> > > > + if (node < 0) {
> > > > + debug("%s: Cannot find FSP node\n", __func__);
> > > > + /* TODO: change return type for error indication */
> > > > + return;
> > > > + }
> > > > +
> > > > + fsp_upd->mrc_init_tseg_size = fdtdec_get_int(blob, node,
> > > > + "mrc_int_tseg_size",
> > > > + fsp_upd->mrc_init_tseg_size);
> > > > + fsp_upd->mrc_init_mmio_size = fdtdec_get_int(blob, node,
> > > > + "mrc_init_mmio_size",
> > > > + fsp_upd->mrc_init_mmio_size);
> > > > + fsp_upd->mrc_init_spd_addr1 = fdtdec_get_int(blob, node,
> > > > + "mrc_init_spd_addr1",
> > > > + fsp_upd->mrc_init_spd_addr1);
> > > > + fsp_upd->mrc_init_spd_addr2 = fdtdec_get_int(blob, node,
> > > > + "mrc_init_spd_addr2",
> > > > + fsp_upd->mrc_init_spd_addr2);
> > > > + fsp_upd->emmc_boot_mode = fdtdec_get_int(blob, node, "emmc_boot_mode",
> > > > + fsp_upd->emmc_boot_mode);
> > > > + fsp_upd->enable_sdio = fdtdec_get_int(blob, node, "enable_sdio",
> > > > + fsp_upd->enable_sdio);
> > >
> > > Shouldn't these be booleans?
> >
> > 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
More information about the U-Boot
mailing list