[PATCH 1/5] x86: fsp: Allow skipping init code when chain loading

[Bin Meng]

Hi Simon,

On Tue, Feb 4, 2020 at 1:15 AM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Bin,
>
> On Mon, 3 Feb 2020 at 10:10, Bin Meng <bmeng.cn at gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Tue, Feb 4, 2020 at 12:20 AM Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi Bin,
> > >
> > > On Mon, 3 Feb 2020 at 04:05, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Sun, Dec 22, 2019 at 12:13 AM Simon Glass <sjg at chromium.org> wrote:
> > > > >
> > > > > When U-Boot is no the first-stage bootloader much of this code is not
> > > > > needed and can break booting. Add checks for this to the FSP code.
> > > > >
> > > > > Rather than checking for the amount of available SDRAM, just use 1GB in
> > > > > this situation, which should be safe. Using 2GB may run into a memory
> > > > > hole on some SoCs.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > ---
> > > > >
> > > > >  arch/x86/lib/fsp/fsp_dram.c     |  8 ++++++++
> > > > >  arch/x86/lib/fsp/fsp_graphics.c |  3 +++
> > > > >  arch/x86/lib/fsp2/fsp_dram.c    | 10 ++++++++++
> > > > >  arch/x86/lib/fsp2/fsp_init.c    |  2 +-
> > > > >  4 files changed, 22 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/arch/x86/lib/fsp/fsp_dram.c b/arch/x86/lib/fsp/fsp_dram.c
> > > > > index 9ce0ddf0d3..15e82de2fe 100644
> > > > > --- a/arch/x86/lib/fsp/fsp_dram.c
> > > > > +++ b/arch/x86/lib/fsp/fsp_dram.c
> > > > > @@ -44,6 +44,14 @@ int dram_init_banksize(void)
> > > > >         phys_addr_t low_end;
> > > > >         uint bank;
> > > > >
> > > > > +       if (!ll_boot_init()) {
> > > > > +               gd->bd->bi_dram[0].start = 0;
> > > > > +               gd->bd->bi_dram[0].size = gd->ram_size;
> > > > > +
> > > > > +               mtrr_add_request(MTRR_TYPE_WRBACK, 0, gd->ram_size);
> > > > > +               return 0;
> > > > > +       }
> > > > > +
> > > >
> > > > I wonder why this change is needed. When booting from other
> > > > bootloader, this dram_init_banksize() will not be called, no?
> > >
> > > How about this:
> > >
> > > It is useful to be able to boot the same x86 image on a device with or
> > > without a first-stage bootloader. For example, with chromebook_coral, it
> > > is helpful for testing to be able to boot the same U-Boot (complete with
> > > FSP) on bare metal and from coreboot. It allows checking of things like
> > > CPU speed, comparing registers, ACPI tables and the like.
> > >
> > > The idea is to change ll_boot_init() to false, and rebuild without changing
> > > anything else.
> >
> > This sounds like for a debugging purpose, instead of a supported
> > configuration. So when we boot from previous bootloader, we really
> > should use its configuration, for the coreboot case,
> > coreboot-x86_defconfig should be used, and we can compare the
> > registers, ACPI tables in that configuration, instead of "hacking"
> > current U-Boot codes like in this series.
>
> Well the problem is then that we cannot boot the same image with or
> without coreboot. Also there are various of device-tree things in
> coral that we need for the laptop to work properly. My idea is to
> detect coreboot and set ll_boot_init() dynamically if needed.

If so, why should we keep coreboot_defconfig for U-Boot as a supported target?

I am more interested in what you proposed here: "detect coreboot and
set ll_boot_init() dynamically if needed." The name ll_boot_init()
does not sound great. It's written like a function call but actually
it is a macro for true/false. Can we do something in the gd->flags to
indicate we are booting from previous stage bootloaders instead of
relying on ll_boot_init()?

Regards,
Bin