[U-Boot] [PATCH v5 045/101] x86: fsp: Add FSP2 base support
Simon Glass
sjg at chromium.org
Thu Nov 28 02:22:45 UTC 2019
HI Bin,
On Tue, 26 Nov 2019 at 23:11, Bin Meng <bmeng.cn at gmail.com> wrote:
>
> Hi Simon,
>
> On Wed, Nov 27, 2019 at 1:08 AM Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Tue, 26 Nov 2019 at 01:36, Bin Meng <bmeng.cn at gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Mon, Nov 25, 2019 at 12:11 PM Simon Glass <sjg at 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 <sjg at 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
More information about the U-Boot
mailing list