HI Bin, On Tue, 26 Nov 2019 at 23:11, Bin Meng <bmeng...@gmail.com> wrote: > > Hi Simon, > > On Wed, Nov 27, 2019 at 1:08 AM Simon Glass <s...@chromium.org> wrote: > > > > Hi Bin, > > > > On Tue, 26 Nov 2019 at 01:36, Bin Meng <bmeng...@gmail.com> wrote: > > > > > > Hi Simon, > > > > > > On Mon, Nov 25, 2019 at 12:11 PM Simon Glass <s...@chromium.org> wrote: > > > > > > > > Add support for some important configuration options and FSP memory > > > > init. > > > > The memory init uses swizzle tables from the device tree. > > > > > > > > Support for the FSP_S binary is also included. > > > > > > > > Bootstage timing is used for both FSP_M and FSP_S and memory-mapped SPI > > > > reads. > > > > > > > > Signed-off-by: Simon Glass <s...@chromium.org> > > > > --- > > > > > > > > Changes in v5: > > > > - Drop SAFETY_MARGIN > > > > > > > > Changes in v4: > > > > - Add a LOG_CATEGORY for silicon init > > > > - Drop duplicate VBT file CONFIG > > > > - Enable HAVE_VBT for FSP2 also > > > > - Explain the 'twisty headers' comment > > > > - Fix FSP_M reference to refer to FSP_S in commit message > > > > - Fix comment on fsp_silicon_init() > > > > - Rename arch_fsp_s_preinit() to arch_fsps_preinit() > > > > - Rename get_coreboot_fsp() and add comments > > > > - Switch over to use pinctrl for pad init/config > > > > - Use lower-case pinctrl in arch_cpu_init_dm() > > > > > > > > Changes in v3: > > > > - Add a proper implementation of fsp_notify > > > > - Add an fsp: tag > > > > - Add bootstage timing for memory-mapped reads > > > > - Add fsp_locate_fsp to locate an fsp component > > > > - Add fspm_done() hook > > > > - Add support for FSP-S component and VBT > > > > - Simplify types for fsp_locate_fsp() > > > > - Switch mmap to use SPI instead of SPI flash > > > > > > > > Changes in v2: None > > > > > > > > arch/x86/Kconfig | 54 ++++++- > > > > arch/x86/include/asm/fsp2/fsp_api.h | 63 ++++++++ > > > > arch/x86/include/asm/fsp2/fsp_internal.h | 97 +++++++++++++ > > > > arch/x86/lib/fsp2/Makefile | 10 ++ > > > > arch/x86/lib/fsp2/fsp_common.c | 13 ++ > > > > arch/x86/lib/fsp2/fsp_dram.c | 77 ++++++++++ > > > > arch/x86/lib/fsp2/fsp_init.c | 174 +++++++++++++++++++++++ > > > > arch/x86/lib/fsp2/fsp_meminit.c | 97 +++++++++++++ > > > > arch/x86/lib/fsp2/fsp_silicon_init.c | 54 +++++++ > > > > arch/x86/lib/fsp2/fsp_support.c | 131 +++++++++++++++++ > > > > include/bootstage.h | 3 + > > > > 11 files changed, 770 insertions(+), 3 deletions(-) > > > > create mode 100644 arch/x86/include/asm/fsp2/fsp_api.h > > > > create mode 100644 arch/x86/include/asm/fsp2/fsp_internal.h > > > > create mode 100644 arch/x86/lib/fsp2/Makefile > > > > create mode 100644 arch/x86/lib/fsp2/fsp_common.c > > > > create mode 100644 arch/x86/lib/fsp2/fsp_dram.c > > > > create mode 100644 arch/x86/lib/fsp2/fsp_init.c > > > > create mode 100644 arch/x86/lib/fsp2/fsp_meminit.c > > > > create mode 100644 arch/x86/lib/fsp2/fsp_silicon_init.c > > > > create mode 100644 arch/x86/lib/fsp2/fsp_support.c > > > > > > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > > > > index 17a6fe6d3d..6bac5d5fe8 100644 > > > > --- a/arch/x86/Kconfig > > > > +++ b/arch/x86/Kconfig > > > > @@ -326,7 +326,7 @@ config X86_RAMTEST > > > > > > > > config FLASH_DESCRIPTOR_FILE > > > > string "Flash descriptor binary filename" > > > > - depends on HAVE_INTEL_ME > > > > + depends on HAVE_INTEL_ME || FSP_VERSION2 > > > > default "descriptor.bin" > > > > help > > > > The filename of the file to use as flash descriptor in the > > > > @@ -411,6 +411,54 @@ config FSP_ADDR > > > > The default base address of 0xfffc0000 indicates that the > > > > binary must > > > > be located at offset 0xc0000 from the beginning of a 1MB > > > > flash device. > > > > > > > > +if FSP_VERSION2 > > > > + > > > > +config FSP_FILE_T > > > > + string "Firmware-Support-Package binary filename (Temp RAM)" > > > > + default "fsp_t.bin" > > > > + help > > > > + The filename of the file to use for the temporary-RAM init > > > > phase from > > > > + the Firmware-Support-Package binary. Put this in the board > > > > directory. > > > > > > nits: Firmware Support Package (drop -) > > > > OK...the hyphens are actually correct I think, but perhaps make it > > hard to search for. > > > > [..] > > > > > > config VIDEO_FSP > > > > bool "Enable FSP framebuffer driver support" > > > > - depends on HAVE_VBT && DM_VIDEO > > > > + depends on (HAVE_VBT || FSP_VERSION2) && DM_VIDEO > > > > > > I think the original logic already satisfies the dependency > > > requirement, that we don't need explicitly list FSP_VERSION2 here. > > > > Yes, now that I don't need to add a new Kconfig I can drop this. > > > > [..] > > > > > > diff --git a/arch/x86/lib/fsp2/fsp_init.c b/arch/x86/lib/fsp2/fsp_init.c > > > > new file mode 100644 > > > > index 0000000000..bcc385e876 > > > > --- /dev/null > > > > +++ b/arch/x86/lib/fsp2/fsp_init.c > > > > @@ -0,0 +1,174 @@ > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > +/* > > > > + * Copyright 2019 Google LLC > > > > + */ > > > > + > > > > +#include <common.h> > > > > +#include <binman.h> > > > > +#include <binman_sym.h> > > > > +#include <cbfs.h> > > > > +#include <dm.h> > > > > +#include <init.h> > > > > +#include <spi.h> > > > > +#include <spl.h> > > > > +#include <spi_flash.h> > > > > +#include <asm/intel_pinctrl.h> > > > > +#include <dm/uclass-internal.h> > > > > +#include <asm/fsp2/fsp_internal.h> > > > > + > > > > +int arch_cpu_init_dm(void) > > > > +{ > > > > + struct udevice *dev; > > > > + ofnode node; > > > > + int ret; > > > > + > > > > + /* Make sure pads are set up early in U-Boot */ > > > > + if (spl_phase() != PHASE_BOARD_F) > > > > + return 0; > > > > + > > > > + /* Probe all pinctrl devices to set up the pads */ > > > > + ret = uclass_first_device_err(UCLASS_PINCTRL, &dev); > > > > + if (ret) > > > > + return log_msg_ret("no fsp pinctrl", ret); > > > > + node = ofnode_path("fsp"); > > > > + if (!ofnode_valid(node)) > > > > + return log_msg_ret("no fsp params", -EINVAL); > > > > + ret = pinctrl_config_pads_for_node(dev, node); > > > > + if (ret) > > > > + return log_msg_ret("pad config", ret); > > > > + > > > > + return ret; > > > > +} > > > > + > > > > +#if !defined(CONFIG_TPL_BUILD) > > > > +binman_sym_declare(ulong, intel_fsp_m, image_pos); > > > > +binman_sym_declare(ulong, intel_fsp_m, size); > > > > + > > > > +/** > > > > + * get_cbfs_fsp() - Obtain the FSP by looking up in CBFS > > > > + * > > > > + * This looks up an FSP in a CBFS. It is used mostly for testing, when > > > > booting > > > > + * U-Boot from a hybrid image containing coreboot as the first-stage > > > > bootloader. > > > > + * > > > > + * @type; Type to look up (only FSP_M supported at present) > > > > + * @map_base: Base memory address for mapped SPI > > > > + * @entry: Returns an entry containing the position of the FSP image > > > > + */ > > > > +static int get_cbfs_fsp(enum fsp_type_t type, ulong map_base, > > > > + struct binman_entry *entry) > > > > +{ > > > > + /* > > > > + * Use a hard-coded position of CBFS in the ROM for now. It > > > > would be > > > > + * possible to read the position using the FMAP in the ROM, but > > > > since > > > > + * this code is only used for development, it doesn't seem > > > > worth it. > > > > > > I don't understand why this code is only used for development. > > > > Because we don't use CBFS in U-Boot normally. This is just a way to > > meld coreboot and U-Boot for development purposes. I think it is > > useful enough to have in here and perhaps one day become more generic > > (perhaps when we do another embedded platform). > > > > My understanding is that U-Boot boots directly without coreboot's help > on Apollo Lake in this series. If this is used for development, what > benefit do we get?
Yes that's right...the benefit is that we can run coreboot's earlier setup code as a way of verifying that U-Boot does the right thing. > > Perhaps could you please describe it with more details about the > "hybrid" mode, ie: mixed coreboot and U-Boot. It's not as simple as > using U-Boot as the coreboot payload, I assume. In any case, the > hardcoded number won't work for other platform, I think. The hard-coded values only work for a particular platform, which is why I have the instructions on how to use them. You can modify coreboot to jump to U-Boot from various different stages, and validate that everything is working correctly. I could remove this code if that seems better, but I think it is useful perhaps for others. > > > > > > > + * Use the 'cbfstool <image> layout' command to get these > > > > values, e.g.: > > > > + * 'COREBOOT' (CBFS, size 1814528, offset 2117632) > > > > > > Does file_cbfs_find() and its friends in fs/cbfs.c help? > > > > That is for finding a CBFS file in a CBFS. We need to actually find > > the CBFS and there are unfortunately several of them in the image. > > > > The only really way would be to hard-code the section name and use the > > FMAP to find it. But we don't really use FMAP either, so that seems > > like a lot of code to add. > > > > [..] Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot