[PATCH 0/5] x86: Improve support for chain-loading U-Boot

[PATCH 0/5] x86: Improve support for chain-loading U-Boot

[Simon Glass]

This little series adds a few checks into the code to allow better
operation when booting a build from a previous-state loader such as
coreboot.

At present we have a 'coreboot' target, but sometimes it is useful to boot
a bare-metal target, such as coral, from coreboot. That allows comparison
of operation between the bare metal version doing all the init itself and
relying on coreboot for some init.


Simon Glass (5):
  x86: fsp: Allow skipping init code when chain loading
  x86: apl: Skip init code when chain loading
  x86: cpu: Skip init code when chain loading
  dm: Avoid initing built-in devices when chain loading
  pci: Avoid auto-config when chain loading

 arch/x86/cpu/apollolake/fsp_s.c |  2 ++
 arch/x86/cpu/cpu.c              |  4 +++-
 arch/x86/cpu/i386/interrupt.c   |  3 +++
 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 +-
 arch/x86/lib/init_helpers.c     |  3 +++
 common/board_r.c                |  3 +++
 drivers/pci/pci-uclass.c        |  4 ++--
 10 files changed, 38 insertions(+), 4 deletions(-)

-- 
2.24.1.735.g03f4e72817-goog

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

[Simon Glass]

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@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;
+	}
+
 	low_end = 0;
 	for (bank = 1, hdr = gd->arch.hob_list;
 	     bank < CONFIG_NR_DRAM_BANKS && !end_of_hob(hdr);
diff --git a/arch/x86/lib/fsp/fsp_graphics.c b/arch/x86/lib/fsp/fsp_graphics.c
index 226c7e66b3..98b762209f 100644
--- a/arch/x86/lib/fsp/fsp_graphics.c
+++ b/arch/x86/lib/fsp/fsp_graphics.c
@@ -78,6 +78,9 @@ static int fsp_video_probe(struct udevice *dev)
 	struct vesa_mode_info *vesa = &mode_info.vesa;
 	int ret;
 
+	if (!ll_boot_init())
+		return 0;
+
 	printf("Video: ");
 
 	/* Initialize vesa_mode_info structure */
diff --git a/arch/x86/lib/fsp2/fsp_dram.c b/arch/x86/lib/fsp2/fsp_dram.c
index 90a238a224..74835eebce 100644
--- a/arch/x86/lib/fsp2/fsp_dram.c
+++ b/arch/x86/lib/fsp2/fsp_dram.c
@@ -12,11 +12,18 @@
 #include <asm/fsp/fsp_support.h>
 #include <asm/fsp2/fsp_api.h>
 #include <asm/fsp2/fsp_internal.h>
+#include <linux/sizes.h>
 
 int dram_init(void)
 {
 	int ret;
 
+	if (!ll_boot_init()) {
+		/* Use a small and safe amount of 1GB */
+		gd->ram_size = SZ_1G;
+
+		return 0;
+	}
 	if (spl_phase() == PHASE_SPL) {
 #ifdef CONFIG_HAVE_ACPI_RESUME
 		bool s3wake = gd->arch.prev_sleep_state == ACPI_S3;
@@ -68,6 +75,9 @@ int dram_init(void)
 
 ulong board_get_usable_ram_top(ulong total_size)
 {
+	if (!ll_boot_init())
+		return gd->ram_size;
+
 #if CONFIG_IS_ENABLED(HANDOFF)
 	struct spl_handoff *ho = gd->spl_handoff;
 
diff --git a/arch/x86/lib/fsp2/fsp_init.c b/arch/x86/lib/fsp2/fsp_init.c
index da9bd6b45c..c7dc2ea257 100644
--- a/arch/x86/lib/fsp2/fsp_init.c
+++ b/arch/x86/lib/fsp2/fsp_init.c
@@ -23,7 +23,7 @@ int arch_cpu_init_dm(void)
 	int ret;
 
 	/* Make sure pads are set up early in U-Boot */
-	if (spl_phase() != PHASE_BOARD_F)
+	if (!ll_boot_init() || spl_phase() != PHASE_BOARD_F)
 		return 0;
 
 	/* Probe all pinctrl devices to set up the pads */
-- 
2.24.1.735.g03f4e72817-goog

[PATCH 2/5] x86: apl: Skip init code when chain loading

[Simon Glass]

When U-Boot is not the first-stage bootloader the FSP-S init must be
skipped. Update it to add a check.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 arch/x86/cpu/apollolake/fsp_s.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/cpu/apollolake/fsp_s.c b/arch/x86/cpu/apollolake/fsp_s.c
index 9804227f80..a3d147ec14 100644
--- a/arch/x86/cpu/apollolake/fsp_s.c
+++ b/arch/x86/cpu/apollolake/fsp_s.c
@@ -623,6 +623,8 @@ int arch_fsp_init_r(void)
 	struct udevice *dev, *itss;
 	int ret;
 
+	if (!ll_boot_init())
+		return 0;
 	/*
 	 * This must be called before any devices are probed. Put any probing
 	 * into arch_fsps_preinit() above.
-- 
2.24.1.735.g03f4e72817-goog

[PATCH 3/5] x86: cpu: Skip init code when chain loading

[Simon Glass]

When U-Boot is not the first-stage bootloader the interrupt and cache init
must be skipped, as well as init for various peripherals. Update the code
to add checks for this.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 arch/x86/cpu/cpu.c            | 4 +++-
 arch/x86/cpu/i386/interrupt.c | 3 +++
 arch/x86/lib/init_helpers.c   | 3 +++
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c
index d626e38fd1..d82305303a 100644
--- a/arch/x86/cpu/cpu.c
+++ b/arch/x86/cpu/cpu.c
@@ -238,8 +238,10 @@ int cpu_init_r(void)
 	struct udevice *dev;
 	int ret;
 
-	if (!ll_boot_init())
+	if (!ll_boot_init()) {
+		uclass_first_device(UCLASS_PCI, &dev);
 		return 0;
+	}
 
 	ret = x86_init_cpus();
 	if (ret)
diff --git a/arch/x86/cpu/i386/interrupt.c b/arch/x86/cpu/i386/interrupt.c
index 78aa51a3ea..c33ca165d8 100644
--- a/arch/x86/cpu/i386/interrupt.c
+++ b/arch/x86/cpu/i386/interrupt.c
@@ -261,6 +261,9 @@ int interrupt_init(void)
 	struct udevice *dev;
 	int ret;
 
+	if (!ll_boot_init())
+		return 0;
+
 	/* Try to set up the interrupt router, but don't require one */
 	ret = uclass_first_device_err(UCLASS_IRQ, &dev);
 	if (ret && ret != -ENODEV)
diff --git a/arch/x86/lib/init_helpers.c b/arch/x86/lib/init_helpers.c
index 5bb55e256f..d906b528b3 100644
--- a/arch/x86/lib/init_helpers.c
+++ b/arch/x86/lib/init_helpers.c
@@ -30,6 +30,9 @@ int init_cache_f_r(void)
 			return ret;
 	}
 
+	if (!ll_boot_init())
+		return 0;
+
 	/* Initialise the CPU cache(s) */
 	return init_cache();
 }
-- 
2.24.1.735.g03f4e72817-goog

[PATCH 4/5] dm: Avoid initing built-in devices when chain loading

[Simon Glass]

When U-Boot is not the first-stage bootloader we don't want to init
devices early during boot. Add a check to avoid this.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 common/board_r.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/common/board_r.c b/common/board_r.c
index e711de64b5..4e0dfac4fc 100644
--- a/common/board_r.c
+++ b/common/board_r.c
@@ -320,6 +320,9 @@ static int initr_dm_devices(void)
 {
 	int ret;
 
+	if (!ll_boot_init())
+		return 0;
+
 	if (IS_ENABLED(CONFIG_TIMER_EARLY)) {
 		ret = dm_timer_init();
 		if (ret)
-- 
2.24.1.735.g03f4e72817-goog

[PATCH 5/5] pci: Avoid auto-config when chain loading

[Simon Glass]

When U-Boot is not the first-stage bootloader we don't want to
re-configure the PCI devices, since this has already been done. Add a
check to avoid this.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 drivers/pci/pci-uclass.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
index 7308f612b6..6af97cd797 100644
--- a/drivers/pci/pci-uclass.c
+++ b/drivers/pci/pci-uclass.c
@@ -991,7 +991,7 @@ static int pci_uclass_post_probe(struct udevice *bus)
 	if (ret)
 		return ret;
 
-	if (CONFIG_IS_ENABLED(PCI_PNP) &&
+	if (CONFIG_IS_ENABLED(PCI_PNP) && ll_boot_init() &&
 	    (!hose->skip_auto_config_until_reloc ||
 	     (gd->flags & GD_FLG_RELOC))) {
 		ret = pci_auto_config_devices(bus);
@@ -1013,7 +1013,7 @@ static int pci_uclass_post_probe(struct udevice *bus)
 	 * Note we only call this 1) after U-Boot is relocated, and 2)
 	 * root bus has finished probing.
 	 */
-	if ((gd->flags & GD_FLG_RELOC) && (bus->seq == 0)) {
+	if ((gd->flags & GD_FLG_RELOC) && bus->seq == 0 && ll_boot_init()) {
 		ret = fsp_init_phase_pci();
 		if (ret)
 			return ret;
-- 
2.24.1.735.g03f4e72817-goog

[PATCH 0/5] x86: Improve support for chain-loading U-Boot

[Andy Shevchenko]

On Sat, Dec 21, 2019 at 6:13 PM Simon Glass <sjg@chromium.org> wrote:
>
> This little series adds a few checks into the code to allow better
> operation when booting a build from a previous-state loader such as
> coreboot.
>
> At present we have a 'coreboot' target, but sometimes it is useful to boot
> a bare-metal target, such as coral, from coreboot. That allows comparison
> of operation between the bare metal version doing all the init itself and
> relying on coreboot for some init.
>

Thank you for sharing. May I ask what devices and configurations you
have tested this on?

P.S. I'll be able to test my case after xmas vacations (mid January).

> Simon Glass (5):
>   x86: fsp: Allow skipping init code when chain loading
>   x86: apl: Skip init code when chain loading
>   x86: cpu: Skip init code when chain loading
>   dm: Avoid initing built-in devices when chain loading
>   pci: Avoid auto-config when chain loading
>
>  arch/x86/cpu/apollolake/fsp_s.c |  2 ++
>  arch/x86/cpu/cpu.c              |  4 +++-
>  arch/x86/cpu/i386/interrupt.c   |  3 +++
>  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 +-
>  arch/x86/lib/init_helpers.c     |  3 +++
>  common/board_r.c                |  3 +++
>  drivers/pci/pci-uclass.c        |  4 ++--
>  10 files changed, 38 insertions(+), 4 deletions(-)
>
> --
> 2.24.1.735.g03f4e72817-goog
>


-- 
With Best Regards,
Andy Shevchenko

[PATCH 0/5] x86: Improve support for chain-loading U-Boot

[Simon Glass]

Hi Andy,

On Sat, 21 Dec 2019 at 10:00, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> On Sat, Dec 21, 2019 at 6:13 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > This little series adds a few checks into the code to allow better
> > operation when booting a build from a previous-state loader such as
> > coreboot.
> >
> > At present we have a 'coreboot' target, but sometimes it is useful to boot
> > a bare-metal target, such as coral, from coreboot. That allows comparison
> > of operation between the bare metal version doing all the init itself and
> > relying on coreboot for some init.
> >
>
> Thank you for sharing. May I ask what devices and configurations you
> have tested this on?
>
> P.S. I'll be able to test my case after xmas vacations (mid January).

This is only on chromebook_coral. But I think it could work in a
similar way with any of the boards.

Merry Christmas!
Simon

[PATCH 0/5] x86: Improve support for chain-loading U-Boot

[Bin Meng]

Hi Andy,

On Sun, Dec 22, 2019 at 1:00 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sat, Dec 21, 2019 at 6:13 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > This little series adds a few checks into the code to allow better
> > operation when booting a build from a previous-state loader such as
> > coreboot.
> >
> > At present we have a 'coreboot' target, but sometimes it is useful to boot
> > a bare-metal target, such as coral, from coreboot. That allows comparison
> > of operation between the bare metal version doing all the init itself and
> > relying on coreboot for some init.
> >
>
> Thank you for sharing. May I ask what devices and configurations you
> have tested this on?
>
> P.S. I'll be able to test my case after xmas vacations (mid January).
>

Do you have any review comments to share?

Regards,
Bin

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

[Bin Meng]

Hi Simon,

On Sun, Dec 22, 2019 at 12:13 AM Simon Glass <sjg@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@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?

>         low_end = 0;
>         for (bank = 1, hdr = gd->arch.hob_list;
>              bank < CONFIG_NR_DRAM_BANKS && !end_of_hob(hdr);
> diff --git a/arch/x86/lib/fsp/fsp_graphics.c b/arch/x86/lib/fsp/fsp_graphics.c
> index 226c7e66b3..98b762209f 100644
> --- a/arch/x86/lib/fsp/fsp_graphics.c
> +++ b/arch/x86/lib/fsp/fsp_graphics.c
> @@ -78,6 +78,9 @@ static int fsp_video_probe(struct udevice *dev)
>         struct vesa_mode_info *vesa = &mode_info.vesa;
>         int ret;
>
> +       if (!ll_boot_init())
> +               return 0;
> +

the same question as above

>         printf("Video: ");
>
>         /* Initialize vesa_mode_info structure */
> diff --git a/arch/x86/lib/fsp2/fsp_dram.c b/arch/x86/lib/fsp2/fsp_dram.c
> index 90a238a224..74835eebce 100644
> --- a/arch/x86/lib/fsp2/fsp_dram.c
> +++ b/arch/x86/lib/fsp2/fsp_dram.c
> @@ -12,11 +12,18 @@
>  #include <asm/fsp/fsp_support.h>
>  #include <asm/fsp2/fsp_api.h>
>  #include <asm/fsp2/fsp_internal.h>
> +#include <linux/sizes.h>
>
>  int dram_init(void)
>  {
>         int ret;
>
> +       if (!ll_boot_init()) {
> +               /* Use a small and safe amount of 1GB */
> +               gd->ram_size = SZ_1G;
> +

ditto. Maybe we should explain on what configuration ll_boot_init() is
set to true / false?

> +               return 0;
> +       }
>         if (spl_phase() == PHASE_SPL) {
>  #ifdef CONFIG_HAVE_ACPI_RESUME
>                 bool s3wake = gd->arch.prev_sleep_state == ACPI_S3;
> @@ -68,6 +75,9 @@ int dram_init(void)
>
>  ulong board_get_usable_ram_top(ulong total_size)
>  {
> +       if (!ll_boot_init())
> +               return gd->ram_size;
> +
>  #if CONFIG_IS_ENABLED(HANDOFF)
>         struct spl_handoff *ho = gd->spl_handoff;
>
> diff --git a/arch/x86/lib/fsp2/fsp_init.c b/arch/x86/lib/fsp2/fsp_init.c
> index da9bd6b45c..c7dc2ea257 100644
> --- a/arch/x86/lib/fsp2/fsp_init.c
> +++ b/arch/x86/lib/fsp2/fsp_init.c
> @@ -23,7 +23,7 @@ int arch_cpu_init_dm(void)
>         int ret;
>
>         /* Make sure pads are set up early in U-Boot */
> -       if (spl_phase() != PHASE_BOARD_F)
> +       if (!ll_boot_init() || spl_phase() != PHASE_BOARD_F)
>                 return 0;
>
>         /* Probe all pinctrl devices to set up the pads */
> --

Regards,
Bin

[PATCH 3/5] x86: cpu: Skip init code when chain loading

[Bin Meng]

Hi Simon,

On Sun, Dec 22, 2019 at 12:13 AM Simon Glass <sjg@chromium.org> wrote:
>
> When U-Boot is not the first-stage bootloader the interrupt and cache init
> must be skipped, as well as init for various peripherals. Update the code
> to add checks for this.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  arch/x86/cpu/cpu.c            | 4 +++-
>  arch/x86/cpu/i386/interrupt.c | 3 +++
>  arch/x86/lib/init_helpers.c   | 3 +++
>  3 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c
> index d626e38fd1..d82305303a 100644
> --- a/arch/x86/cpu/cpu.c
> +++ b/arch/x86/cpu/cpu.c
> @@ -238,8 +238,10 @@ int cpu_init_r(void)
>         struct udevice *dev;
>         int ret;
>
> -       if (!ll_boot_init())
> +       if (!ll_boot_init()) {
> +               uclass_first_device(UCLASS_PCI, &dev);
>                 return 0;
> +       }
>
>         ret = x86_init_cpus();
>         if (ret)
> diff --git a/arch/x86/cpu/i386/interrupt.c b/arch/x86/cpu/i386/interrupt.c
> index 78aa51a3ea..c33ca165d8 100644
> --- a/arch/x86/cpu/i386/interrupt.c
> +++ b/arch/x86/cpu/i386/interrupt.c
> @@ -261,6 +261,9 @@ int interrupt_init(void)
>         struct udevice *dev;
>         int ret;
>
> +       if (!ll_boot_init())
> +               return 0;
> +

If you scroll down a little bit in this function, you see there is
already a ll_boot_init() check. We need some consolidation here.

>         /* Try to set up the interrupt router, but don't require one */
>         ret = uclass_first_device_err(UCLASS_IRQ, &dev);
>         if (ret && ret != -ENODEV)
> diff --git a/arch/x86/lib/init_helpers.c b/arch/x86/lib/init_helpers.c
> index 5bb55e256f..d906b528b3 100644
> --- a/arch/x86/lib/init_helpers.c
> +++ b/arch/x86/lib/init_helpers.c
> @@ -30,6 +30,9 @@ int init_cache_f_r(void)
>                         return ret;
>         }
>
> +       if (!ll_boot_init())
> +               return 0;

I am not convinced that we should add the ll_boot_init() check here.
This function already has a do_mtrr variable that is based on the
config options.

> +
>         /* Initialise the CPU cache(s) */
>         return init_cache();
>  }

Regards,
Bin

[PATCH 4/5] dm: Avoid initing built-in devices when chain loading

[Bin Meng]

Hi Simon,

On Sun, Dec 22, 2019 at 12:13 AM Simon Glass <sjg@chromium.org> wrote:
>
> When U-Boot is not the first-stage bootloader we don't want to init
> devices early during boot. Add a check to avoid this.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  common/board_r.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/common/board_r.c b/common/board_r.c
> index e711de64b5..4e0dfac4fc 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -320,6 +320,9 @@ static int initr_dm_devices(void)
>  {
>         int ret;
>
> +       if (!ll_boot_init())
> +               return 0;
> +

I can't think of a reason why dm_timer_init() cannot be called in this case.

>         if (IS_ENABLED(CONFIG_TIMER_EARLY)) {
>                 ret = dm_timer_init();
>                 if (ret)
> --

Regards,
Bin

[PATCH 5/5] pci: Avoid auto-config when chain loading

[Bin Meng]

Hi Simon,

On Sun, Dec 22, 2019 at 12:13 AM Simon Glass <sjg@chromium.org> wrote:
>
> When U-Boot is not the first-stage bootloader we don't want to
> re-configure the PCI devices, since this has already been done. Add a
> check to avoid this.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  drivers/pci/pci-uclass.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
> index 7308f612b6..6af97cd797 100644
> --- a/drivers/pci/pci-uclass.c
> +++ b/drivers/pci/pci-uclass.c
> @@ -991,7 +991,7 @@ static int pci_uclass_post_probe(struct udevice *bus)
>         if (ret)
>                 return ret;
>
> -       if (CONFIG_IS_ENABLED(PCI_PNP) &&
> +       if (CONFIG_IS_ENABLED(PCI_PNP) && ll_boot_init() &&

Doesn't the PCI_PNP option already cover such situation?

>             (!hose->skip_auto_config_until_reloc ||
>              (gd->flags & GD_FLG_RELOC))) {
>                 ret = pci_auto_config_devices(bus);
> @@ -1013,7 +1013,7 @@ static int pci_uclass_post_probe(struct udevice *bus)
>          * Note we only call this 1) after U-Boot is relocated, and 2)
>          * root bus has finished probing.
>          */
> -       if ((gd->flags & GD_FLG_RELOC) && (bus->seq == 0)) {
> +       if ((gd->flags & GD_FLG_RELOC) && bus->seq == 0 && ll_boot_init()) {

So CONFIG_HAVE_FSP already covers this. In such configuration, U-Boot
should not turn on CONFIG_HAVE_FSP hence there is no need to check
ll_boot_init().

>                 ret = fsp_init_phase_pci();
>                 if (ret)
>                         return ret;
> --

Regards,
Bin

[PATCH 0/5] x86: Improve support for chain-loading U-Boot

[Andy Shevchenko]

On Mon, Feb 3, 2020 at 1:02 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> On Sun, Dec 22, 2019 at 1:00 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Sat, Dec 21, 2019 at 6:13 PM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > This little series adds a few checks into the code to allow better
> > > operation when booting a build from a previous-state loader such as
> > > coreboot.
> > >
> > > At present we have a 'coreboot' target, but sometimes it is useful to boot
> > > a bare-metal target, such as coral, from coreboot. That allows comparison
> > > of operation between the bare metal version doing all the init itself and
> > > relying on coreboot for some init.
> > >
> >
> > Thank you for sharing. May I ask what devices and configurations you
> > have tested this on?
> >
> > P.S. I'll be able to test my case after xmas vacations (mid January).
> >
>
> Do you have any review comments to share?

Not for time being. Sorry, I'm busy with something else, but if there
is a branch with these patches available thru x86 U-Boot tree, it will
be easier to me to have a chance to test.


-- 
With Best Regards,
Andy Shevchenko

[PATCH 0/5] x86: Improve support for chain-loading U-Boot

[Simon Glass]

Hi Andy,

On Mon, 3 Feb 2020 at 05:30, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> On Mon, Feb 3, 2020 at 1:02 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > On Sun, Dec 22, 2019 at 1:00 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > >
> > > On Sat, Dec 21, 2019 at 6:13 PM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > This little series adds a few checks into the code to allow better
> > > > operation when booting a build from a previous-state loader such as
> > > > coreboot.
> > > >
> > > > At present we have a 'coreboot' target, but sometimes it is useful to boot
> > > > a bare-metal target, such as coral, from coreboot. That allows comparison
> > > > of operation between the bare metal version doing all the init itself and
> > > > relying on coreboot for some init.
> > > >
> > >
> > > Thank you for sharing. May I ask what devices and configurations you
> > > have tested this on?
> > >
> > > P.S. I'll be able to test my case after xmas vacations (mid January).
> > >
> >
> > Do you have any review comments to share?
>
> Not for time being. Sorry, I'm busy with something else, but if there
> is a branch with these patches available thru x86 U-Boot tree, it will
> be easier to me to have a chance to test.

This one is u-boot-dm/cor3chain-working

I always push to the -dm tree before sending patches out, but don't
always mention it in the cover letter!

The ACPI patches are at dm/coral-working

Regards,
Simon

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

[Simon Glass]

Hi Bin,

On Mon, 3 Feb 2020 at 04:05, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Sun, Dec 22, 2019 at 12:13 AM Simon Glass <sjg@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@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.

Regards,
Simon

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

[Bin Meng]

Hi Simon,

On Tue, Feb 4, 2020 at 12:20 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On Mon, 3 Feb 2020 at 04:05, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Sun, Dec 22, 2019 at 12:13 AM Simon Glass <sjg@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@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.

Regards,
Bin

[PATCH 3/5] x86: cpu: Skip init code when chain loading

[Simon Glass]

Hi Bin,

On Mon, 3 Feb 2020 at 04:11, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Sun, Dec 22, 2019 at 12:13 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > When U-Boot is not the first-stage bootloader the interrupt and cache init
> > must be skipped, as well as init for various peripherals. Update the code
> > to add checks for this.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  arch/x86/cpu/cpu.c            | 4 +++-
> >  arch/x86/cpu/i386/interrupt.c | 3 +++
> >  arch/x86/lib/init_helpers.c   | 3 +++
> >  3 files changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c
> > index d626e38fd1..d82305303a 100644
> > --- a/arch/x86/cpu/cpu.c
> > +++ b/arch/x86/cpu/cpu.c
> > @@ -238,8 +238,10 @@ int cpu_init_r(void)
> >         struct udevice *dev;
> >         int ret;
> >
> > -       if (!ll_boot_init())
> > +       if (!ll_boot_init()) {
> > +               uclass_first_device(UCLASS_PCI, &dev);
> >                 return 0;
> > +       }
> >
> >         ret = x86_init_cpus();
> >         if (ret)
> > diff --git a/arch/x86/cpu/i386/interrupt.c b/arch/x86/cpu/i386/interrupt.c
> > index 78aa51a3ea..c33ca165d8 100644
> > --- a/arch/x86/cpu/i386/interrupt.c
> > +++ b/arch/x86/cpu/i386/interrupt.c
> > @@ -261,6 +261,9 @@ int interrupt_init(void)
> >         struct udevice *dev;
> >         int ret;
> >
> > +       if (!ll_boot_init())
> > +               return 0;
> > +
>
> If you scroll down a little bit in this function, you see there is
> already a ll_boot_init() check. We need some consolidation here.
>
> >         /* Try to set up the interrupt router, but don't require one */
> >         ret = uclass_first_device_err(UCLASS_IRQ, &dev);
> >         if (ret && ret != -ENODEV)
> > diff --git a/arch/x86/lib/init_helpers.c b/arch/x86/lib/init_helpers.c
> > index 5bb55e256f..d906b528b3 100644
> > --- a/arch/x86/lib/init_helpers.c
> > +++ b/arch/x86/lib/init_helpers.c
> > @@ -30,6 +30,9 @@ int init_cache_f_r(void)
> >                         return ret;
> >         }
> >
> > +       if (!ll_boot_init())
> > +               return 0;
>
> I am not convinced that we should add the ll_boot_init() check here.
> This function already has a do_mtrr variable that is based on the
> config options.

Yes but as per my other explanation, we are not using those config
options. The idea is to boot an image as close as possible to the
bare-metal one.

Regards,
Simon

[PATCH 4/5] dm: Avoid initing built-in devices when chain loading

[Simon Glass]

Hi Bin,

On Mon, 3 Feb 2020 at 04:17, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Sun, Dec 22, 2019 at 12:13 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > When U-Boot is not the first-stage bootloader we don't want to init
> > devices early during boot. Add a check to avoid this.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  common/board_r.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/common/board_r.c b/common/board_r.c
> > index e711de64b5..4e0dfac4fc 100644
> > --- a/common/board_r.c
> > +++ b/common/board_r.c
> > @@ -320,6 +320,9 @@ static int initr_dm_devices(void)
> >  {
> >         int ret;
> >
> > +       if (!ll_boot_init())
> > +               return 0;
> > +
>
> I can't think of a reason why dm_timer_init() cannot be called in this case.

Perhaps we can drop this one, since we don't reset the timer base anymore.

>
> >         if (IS_ENABLED(CONFIG_TIMER_EARLY)) {
> >                 ret = dm_timer_init();
> >                 if (ret)
> > --

Regards,
SImon

[PATCH 5/5] pci: Avoid auto-config when chain loading

[Simon Glass]

Hi Bin,

On Mon, 3 Feb 2020 at 04:21, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Sun, Dec 22, 2019 at 12:13 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > When U-Boot is not the first-stage bootloader we don't want to
> > re-configure the PCI devices, since this has already been done. Add a
> > check to avoid this.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  drivers/pci/pci-uclass.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
> > index 7308f612b6..6af97cd797 100644
> > --- a/drivers/pci/pci-uclass.c
> > +++ b/drivers/pci/pci-uclass.c
> > @@ -991,7 +991,7 @@ static int pci_uclass_post_probe(struct udevice *bus)
> >         if (ret)
> >                 return ret;
> >
> > -       if (CONFIG_IS_ENABLED(PCI_PNP) &&
> > +       if (CONFIG_IS_ENABLED(PCI_PNP) && ll_boot_init() &&
>
> Doesn't the PCI_PNP option already cover such situation?
>
> >             (!hose->skip_auto_config_until_reloc ||
> >              (gd->flags & GD_FLG_RELOC))) {
> >                 ret = pci_auto_config_devices(bus);
> > @@ -1013,7 +1013,7 @@ static int pci_uclass_post_probe(struct udevice *bus)
> >          * Note we only call this 1) after U-Boot is relocated, and 2)
> >          * root bus has finished probing.
> >          */
> > -       if ((gd->flags & GD_FLG_RELOC) && (bus->seq == 0)) {
> > +       if ((gd->flags & GD_FLG_RELOC) && bus->seq == 0 && ll_boot_init()) {
>
> So CONFIG_HAVE_FSP already covers this. In such configuration, U-Boot
> should not turn on CONFIG_HAVE_FSP hence there is no need to check
> ll_boot_init().

Same comment as with patch 1. See if that explains things enough, and
if so I can send a v2.

Regards,
Simon

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

[Simon Glass]

Hi Bin,

On Mon, 3 Feb 2020 at 10:10, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Tue, Feb 4, 2020 at 12:20 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Mon, 3 Feb 2020 at 04:05, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Sun, Dec 22, 2019 at 12:13 AM Simon Glass <sjg@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@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.

Regards,
Simon

[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@chromium.org> wrote:
>
> Hi Bin,
>
> On Mon, 3 Feb 2020 at 10:10, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Tue, Feb 4, 2020 at 12:20 AM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Bin,
> > >
> > > On Mon, 3 Feb 2020 at 04:05, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Sun, Dec 22, 2019 at 12:13 AM Simon Glass <sjg@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@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

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

[Simon Glass]

Hi Bin,

On Mon, 3 Feb 2020 at 21:52, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Simon,
>
> On Tue, Feb 4, 2020 at 1:15 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Mon, 3 Feb 2020 at 10:10, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Tue, Feb 4, 2020 at 12:20 AM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Bin,
> > > >
> > > > On Mon, 3 Feb 2020 at 04:05, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On Sun, Dec 22, 2019 at 12:13 AM Simon Glass <sjg@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@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()?

That sounds like a good idea, although at present it is statically
configured. But we don't really care about code size here I think.

Regards,
Simon