Hello Werner Zeh, Patrick Rudolph, Julius Werner, Huang Jin, Arthur Heymans, uwe taz, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/37084
to review the following change.
Change subject: Revert "lib/fmap: Disable pre-RAM cache for FSP 1.0" ......................................................................
Revert "lib/fmap: Disable pre-RAM cache for FSP 1.0"
This reverts commit 7fc928656e791064c46a4748f86466930bdf2de6.
Reason for revert: Removal of FSP 1.0 support with 4.11 release
Change-Id: I1f074ed7252331c268249f222d529addf8fc5c5c --- M src/arch/x86/car.ld M src/drivers/intel/fsp1_0/Kconfig M src/lib/Kconfig M src/lib/fmap.c M src/soc/rockchip/rk3288/Kconfig 5 files changed, 4 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/37084/1
diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld index 3680250..972cb52 100644 --- a/src/arch/x86/car.ld +++ b/src/arch/x86/car.ld @@ -58,10 +58,7 @@ #endif
TIMESTAMP(., 0x200) - -#if !CONFIG(NO_FMAP_CACHE) FMAP_CACHE(., FMAP_SIZE) -#endif
_car_ehci_dbg_info = .; /* Reserve sizeof(struct ehci_dbg_info). */ diff --git a/src/drivers/intel/fsp1_0/Kconfig b/src/drivers/intel/fsp1_0/Kconfig index 1a1d4f7..32a0777 100644 --- a/src/drivers/intel/fsp1_0/Kconfig +++ b/src/drivers/intel/fsp1_0/Kconfig @@ -15,7 +15,6 @@ bool default n select CAR_GLOBAL_MIGRATION - select NO_FMAP_CACHE # doesn't work with CAR_GLOBAL restrictions help Selected for Intel processors/platform combinations that use the Intel Firmware Support Package (FSP) 1.0 for initialization. diff --git a/src/lib/Kconfig b/src/lib/Kconfig index dd9974a..cb1e4a5 100644 --- a/src/lib/Kconfig +++ b/src/lib/Kconfig @@ -68,10 +68,3 @@ def_bool y
endif - -config NO_FMAP_CACHE - bool - help - If your platform really doesn't want to use an FMAP cache (e.g. due to - space constraints), you can select this to disable warnings and save - a bit more code. diff --git a/src/lib/fmap.c b/src/lib/fmap.c index 48aab8f..4b4179c 100644 --- a/src/lib/fmap.c +++ b/src/lib/fmap.c @@ -60,9 +60,6 @@
static void setup_preram_cache(struct mem_region_device *cache_mrdev) { - if (CONFIG(NO_FMAP_CACHE)) - return; - if (!ENV_ROMSTAGE_OR_BEFORE) { /* We get here if ramstage makes an FMAP access before calling cbmem_initialize(). We should avoid letting it come to that, @@ -73,10 +70,10 @@ }
if (REGION_SIZE(fmap_cache) == 0) { - /* If you see this you should add FMAP_CACHE() to your memlayout - (or select NO_FMAP_CACHE if you can't afford the 2K). */ - print_once(BIOS_ERR, - "ERROR: FMAP_CACHE enabled but no region provided!\n"); + /* If you see this you really want to add an FMAP_CACHE to your + memlayout, unless you absolutely can't affort the 2K. */ + print_once(BIOS_NOTICE, + "NOTE: Running without FMAP_CACHE, should add it!\n"); return; }
diff --git a/src/soc/rockchip/rk3288/Kconfig b/src/soc/rockchip/rk3288/Kconfig index 6a44ccd..3aebab9 100644 --- a/src/soc/rockchip/rk3288/Kconfig +++ b/src/soc/rockchip/rk3288/Kconfig @@ -29,7 +29,6 @@ select MAINBOARD_FORCE_NATIVE_VGA_INIT select HAVE_LINEAR_FRAMEBUFFER select NO_BOOTBLOCK_CONSOLE - select NO_FMAP_CACHE
if SOC_ROCKCHIP_RK3288
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37084 )
Change subject: Revert "lib/fmap: Disable pre-RAM cache for FSP 1.0" ......................................................................
Patch Set 1:
What about rockchip? Julius added it on purpose I guess?
Hello Werner Zeh, Patrick Rudolph, Julius Werner, Huang Jin, Arthur Heymans, uwe taz, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37084
to look at the new patch set (#2).
Change subject: Revert "lib/fmap: Disable pre-RAM cache for FSP 1.0" ......................................................................
Revert "lib/fmap: Disable pre-RAM cache for FSP 1.0"
This reverts commit 7fc928656e791064c46a4748f86466930bdf2de6.
Reason for revert: Removal of FSP 1.0 support with 4.11 release
Change-Id: I1f074ed7252331c268249f222d529addf8fc5c5c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld M src/lib/Kconfig M src/lib/fmap.c M src/soc/rockchip/rk3288/Kconfig 4 files changed, 4 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/37084/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37084 )
Change subject: Revert "lib/fmap: Disable pre-RAM cache for FSP 1.0" ......................................................................
Patch Set 2:
Patch Set 1:
What about rockchip? Julius added it on purpose I guess?
Then this gets -2 for the time being. I only read the title before hitting revert button :)
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37084 )
Change subject: Revert "lib/fmap: Disable pre-RAM cache for FSP 1.0" ......................................................................
Patch Set 2: Code-Review-2
Patch Set 2:
Patch Set 1:
What about rockchip? Julius added it on purpose I guess?
Then this gets -2 for the time being. I only read the title before hitting revert button :)
And obviously SOC_INTEL_GLK wants it too...
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37084 )
Change subject: Revert "lib/fmap: Disable pre-RAM cache for FSP 1.0" ......................................................................
Patch Set 2:
Patch Set 2: Code-Review-2
Patch Set 2:
Patch Set 1:
What about rockchip? Julius added it on purpose I guess?
Then this gets -2 for the time being. I only read the title before hitting revert button :)
And obviously SOC_INTEL_GLK wants it too...
Both GLK and rk3288 do it because of code size constraints.
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37084 )
Change subject: Revert "lib/fmap: Disable pre-RAM cache for FSP 1.0" ......................................................................
Abandoned
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37084 )
Change subject: Revert "lib/fmap: Disable pre-RAM cache for FSP 1.0" ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2: Code-Review-2
Patch Set 2:
Patch Set 1:
What about rockchip? Julius added it on purpose I guess?
Then this gets -2 for the time being. I only read the title before hitting revert button :)
And obviously SOC_INTEL_GLK wants it too...
Both GLK and rk3288 do it because of code size constraints.
We can fix GLK. Which stage failed to link with this?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37084 )
Change subject: Revert "lib/fmap: Disable pre-RAM cache for FSP 1.0" ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2: Code-Review-2
Patch Set 2:
Patch Set 1:
What about rockchip? Julius added it on purpose I guess?
Then this gets -2 for the time being. I only read the title before hitting revert button :)
And obviously SOC_INTEL_GLK wants it too...
Both GLK and rk3288 do it because of code size constraints.
We can fix GLK. Which stage failed to link with this?
https://review.coreboot.org/c/coreboot/+/36925 Bootblock it seems, which has a limit of 32KiB.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37084 )
Change subject: Revert "lib/fmap: Disable pre-RAM cache for FSP 1.0" ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2: Code-Review-2
Patch Set 2:
Patch Set 1:
What about rockchip? Julius added it on purpose I guess?
Then this gets -2 for the time being. I only read the title before hitting revert button :)
And obviously SOC_INTEL_GLK wants it too...
Both GLK and rk3288 do it because of code size constraints.
We can fix GLK. Which stage failed to link with this?
FYI: I just built meep w/o selecting no_fmap_cache:
$ ./util/abuild/abuild -c 48 -p none -v -t GOOGLE_MEEP Building config GOOGLE_MEEP_CROS Building GOOGLE_MEEP_CROS GOOGLE_MEEP (default configuration) Compiling GOOGLE_MEEP image on 48 cpus in parallel... GOOGLE_MEEP_CROS built successfully. (took 6s) All 1 tested configurations passed.
$ grep FMAP_CACHE coreboot-builds/GOOGLE_MEEP_CROS/config.h #define CONFIG_NO_FMAP_CACHE 0
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37084 )
Change subject: Revert "lib/fmap: Disable pre-RAM cache for FSP 1.0" ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2: Code-Review-2
Patch Set 2:
Patch Set 1:
What about rockchip? Julius added it on purpose I guess?
Then this gets -2 for the time being. I only read the title before hitting revert button :)
And obviously SOC_INTEL_GLK wants it too...
Both GLK and rk3288 do it because of code size constraints.
We can fix GLK. Which stage failed to link with this?
FYI: I just built meep w/o selecting no_fmap_cache:
$ ./util/abuild/abuild -c 48 -p none -v -t GOOGLE_MEEP Building config GOOGLE_MEEP_CROS Building GOOGLE_MEEP_CROS GOOGLE_MEEP (default configuration) Compiling GOOGLE_MEEP image on 48 cpus in parallel... GOOGLE_MEEP_CROS built successfully. (took 6s) All 1 tested configurations passed.
$ grep FMAP_CACHE coreboot-builds/GOOGLE_MEEP_CROS/config.h #define CONFIG_NO_FMAP_CACHE 0
I first saw the build failures in https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+... (visible to Googlers only, sorry). I was able to reproduce them on upstream master when I put up the config:
i386-elf-ld.bfd: section .id LMA [00000000ffffff48,00000000ffffff7f] overlaps section .text LMA [00000000ffff8000,00000000ffffffd7] i386-elf-ld.bfd: coreboot-builds/GOOGLE_MEEP_CROS/cbfs/fallback/bootblock.debug: section .id lma 0xffffff48 adjusted to 0xffffffd8 i386-elf-ld.bfd: coreboot-builds/GOOGLE_MEEP_CROS/cbfs/fallback/bootblock.debug: section .fit_pointer lma 0xffffffc0 adjusted to 0x100000010 i386-elf-ld.bfd: coreboot-builds/GOOGLE_MEEP_CROS/cbfs/fallback/bootblock.debug: section .reset lma 0xfffffff0 adjusted to 0x100000018
So there must be something else that saved memory again, and lots of it: fffffd98 T _etext ffffff4a R __id_start
Bisecting that, I found that the MIPS removal commit fixed it, which confused me, but alas, it removed 64bit int support in printf on all platforms (instead of disabling that on MIPS only), saving those precious bytes.
With https://review.coreboot.org/c/coreboot/+/37113 applied and fmap cache enabled, meep fails again.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37084 )
Change subject: Revert "lib/fmap: Disable pre-RAM cache for FSP 1.0" ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2: Code-Review-2
Patch Set 2:
> Patch Set 1: > > What about rockchip? Julius added it on purpose I guess?
Then this gets -2 for the time being. I only read the title before hitting revert button :)
And obviously SOC_INTEL_GLK wants it too...
Both GLK and rk3288 do it because of code size constraints.
We can fix GLK. Which stage failed to link with this?
FYI: I just built meep w/o selecting no_fmap_cache:
$ ./util/abuild/abuild -c 48 -p none -v -t GOOGLE_MEEP Building config GOOGLE_MEEP_CROS Building GOOGLE_MEEP_CROS GOOGLE_MEEP (default configuration) Compiling GOOGLE_MEEP image on 48 cpus in parallel... GOOGLE_MEEP_CROS built successfully. (took 6s) All 1 tested configurations passed.
$ grep FMAP_CACHE coreboot-builds/GOOGLE_MEEP_CROS/config.h #define CONFIG_NO_FMAP_CACHE 0
I first saw the build failures in https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+... (visible to Googlers only, sorry). I was able to reproduce them on upstream master when I put up the config:
i386-elf-ld.bfd: section .id LMA [00000000ffffff48,00000000ffffff7f] overlaps section .text LMA [00000000ffff8000,00000000ffffffd7] i386-elf-ld.bfd: coreboot-builds/GOOGLE_MEEP_CROS/cbfs/fallback/bootblock.debug: section .id lma 0xffffff48 adjusted to 0xffffffd8 i386-elf-ld.bfd: coreboot-builds/GOOGLE_MEEP_CROS/cbfs/fallback/bootblock.debug: section .fit_pointer lma 0xffffffc0 adjusted to 0x100000010 i386-elf-ld.bfd: coreboot-builds/GOOGLE_MEEP_CROS/cbfs/fallback/bootblock.debug: section .reset lma 0xfffffff0 adjusted to 0x100000018
So there must be something else that saved memory again, and lots of it: fffffd98 T _etext ffffff4a R __id_start
Bisecting that, I found that the MIPS removal commit fixed it, which confused me, but alas, it removed 64bit int support in printf on all platforms (instead of disabling that on MIPS only), saving those precious bytes.
With https://review.coreboot.org/c/coreboot/+/37113 applied and fmap cache enabled, meep fails again.
Thanks, Patrick. I'll look into it further.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37084 )
Change subject: Revert "lib/fmap: Disable pre-RAM cache for FSP 1.0" ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2: Code-Review-2
> Patch Set 2: > > > Patch Set 1: > > > > What about rockchip? Julius added it on purpose I guess? > > Then this gets -2 for the time being. I only read the title before hitting revert button :)
And obviously SOC_INTEL_GLK wants it too...
Both GLK and rk3288 do it because of code size constraints.
We can fix GLK. Which stage failed to link with this?
FYI: I just built meep w/o selecting no_fmap_cache:
$ ./util/abuild/abuild -c 48 -p none -v -t GOOGLE_MEEP Building config GOOGLE_MEEP_CROS Building GOOGLE_MEEP_CROS GOOGLE_MEEP (default configuration) Compiling GOOGLE_MEEP image on 48 cpus in parallel... GOOGLE_MEEP_CROS built successfully. (took 6s) All 1 tested configurations passed.
$ grep FMAP_CACHE coreboot-builds/GOOGLE_MEEP_CROS/config.h #define CONFIG_NO_FMAP_CACHE 0
I first saw the build failures in https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+... (visible to Googlers only, sorry). I was able to reproduce them on upstream master when I put up the config:
i386-elf-ld.bfd: section .id LMA [00000000ffffff48,00000000ffffff7f] overlaps section .text LMA [00000000ffff8000,00000000ffffffd7] i386-elf-ld.bfd: coreboot-builds/GOOGLE_MEEP_CROS/cbfs/fallback/bootblock.debug: section .id lma 0xffffff48 adjusted to 0xffffffd8 i386-elf-ld.bfd: coreboot-builds/GOOGLE_MEEP_CROS/cbfs/fallback/bootblock.debug: section .fit_pointer lma 0xffffffc0 adjusted to 0x100000010 i386-elf-ld.bfd: coreboot-builds/GOOGLE_MEEP_CROS/cbfs/fallback/bootblock.debug: section .reset lma 0xfffffff0 adjusted to 0x100000018
So there must be something else that saved memory again, and lots of it: fffffd98 T _etext ffffff4a R __id_start
Bisecting that, I found that the MIPS removal commit fixed it, which confused me, but alas, it removed 64bit int support in printf on all platforms (instead of disabling that on MIPS only), saving those precious bytes.
With https://review.coreboot.org/c/coreboot/+/37113 applied and fmap cache enabled, meep fails again.
Thanks, Patrick. I'll look into it further.
Phaser is sitting at 0x7c18 w/o 64-bit printf and 0x7e38 with 64-bit printf. Meep is sitting at 0x7d98 w/o 64-bit printf and blows up with it (assuming 0x220 bytes for 64-bit printf support). We can make bootblock bigger. 32KiB isn't a hard limit.
Big piece of that are chip_info and the device tree devs: (gdb) p/x 0xffffefa0 - 0xffffb1c0 $1 = 0x3de
Almost 1KiB worth.
Slimming that down would be helpful. Something to keep our eye on.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37084 )
Change subject: Revert "lib/fmap: Disable pre-RAM cache for FSP 1.0" ......................................................................
Patch Set 2:
We can make bootblock bigger. 32KiB isn't a hard limit.
A comment in Kconfig suggests otherwise? "# 32KiB bootblock is all that is mapped in by the CSE at top of 4GiB."
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37084 )
Change subject: Revert "lib/fmap: Disable pre-RAM cache for FSP 1.0" ......................................................................
Patch Set 2:
Patch Set 2:
We can make bootblock bigger. 32KiB isn't a hard limit.
A comment in Kconfig suggests otherwise? "# 32KiB bootblock is all that is mapped in by the CSE at top of 4GiB."
Let's go w/ that comment. It's probably more correct than my memory. :)
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
Kyösti Mälkki (Code Review)
-
Patrick Georgi (Code Review)
-
Werner Zeh (Code Review)