Johnny Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39625 )
Change subject: soc/intel/xeon_sp: Modify FSP-T code caching parameters ......................................................................
soc/intel/xeon_sp: Modify FSP-T code caching parameters
With a hard-coded CBFS_SIZE can solve the booting issue on Tioga Pass, a more ideal way can be explored in the future.
Change-Id: Ibba133d9f8fdfbdfae9a0e8e698356a3ca9ba424 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/soc/intel/xeon_sp/Kconfig M src/soc/intel/xeon_sp/bootblock/bootblock.c 2 files changed, 5 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/39625/1
diff --git a/src/soc/intel/xeon_sp/Kconfig b/src/soc/intel/xeon_sp/Kconfig index 94c0ac4..a9352275 100644 --- a/src/soc/intel/xeon_sp/Kconfig +++ b/src/soc/intel/xeon_sp/Kconfig @@ -122,5 +122,8 @@ hex default 0x80000
+config CBFS_SIZE + hex + default 0x1000000
endif ## SOC_INTEL_XEON_SP diff --git a/src/soc/intel/xeon_sp/bootblock/bootblock.c b/src/soc/intel/xeon_sp/bootblock/bootblock.c index 482f5b5..309a39c 100644 --- a/src/soc/intel/xeon_sp/bootblock/bootblock.c +++ b/src/soc/intel/xeon_sp/bootblock/bootblock.c @@ -30,8 +30,8 @@ .FsptCoreUpd = { .MicrocodeRegionBase = (UINT32)CONFIG_CPU_MICROCODE_CBFS_LOC, .MicrocodeRegionLength = (UINT32)CONFIG_CPU_MICROCODE_CBFS_LEN, - .CodeRegionBase = (uint32_t)(0x100000000ULL - CONFIG_ROM_SIZE), - .CodeRegionLength = (UINT32)CONFIG_ROM_SIZE, + .CodeRegionBase = (UINT32)(0x100000000ULL - CONFIG_CBFS_SIZE), + .CodeRegionLength = (UINT32)CONFIG_CBFS_SIZE, .Reserved1 = {0}, }, .FsptConfig = {
Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39625 )
Change subject: soc/intel/xeon_sp: Modify FSP-T code caching parameters ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39625/1/src/soc/intel/xeon_sp/bootb... File src/soc/intel/xeon_sp/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/39625/1/src/soc/intel/xeon_sp/bootb... PS1, Line 33: .CodeRegionBase = (UINT32)(0x100000000ULL - CONFIG_CBFS_SIZE), : .CodeRegionLength = (UINT32)CONFIG_CBFS_SIZE, The value we need here has nothing to do with CBFS. What you want to get here is size of BIOS IFD region, which is 16MB on TiogaPass. As the last resort, please rename this config variable to BIOS_REGION_SIZE and have it defined by mainboard code.
However, there may be a better alternative. Could you please try the following change and let me know if this works: 1. set CodeRegionBase to 0 2. set CodeRegionLength to 0
and then see if this resolves the booting issue.
This works on next generation platform well, and the caching is re-enabled when fast_spi_cache_bios_region() is called, right after FSP-T exit.
Hello build bot (Jenkins), Andrey Petrov, Anjaneya "Reddy" Chagam, Jonathan Zhang, David Hendricks, Jingle Hsu, Morgan Jang, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39625
to look at the new patch set (#2).
Change subject: soc/intel/xeon_sp: Modify FSP-T code caching parameters ......................................................................
soc/intel/xeon_sp: Modify FSP-T code caching parameters
Added variable CONFIG_BIOS_REGION_SIZE for this purpose.
Tested on OCP Tioga Pass.
Change-Id: Ibba133d9f8fdfbdfae9a0e8e698356a3ca9ba424 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/soc/intel/xeon_sp/bootblock/bootblock.c 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/39625/2
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39625 )
Change subject: soc/intel/xeon_sp: Modify FSP-T code caching parameters ......................................................................
Patch Set 2:
(1 comment)
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39625/1/src/soc/intel/xeon_sp/bootb... File src/soc/intel/xeon_sp/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/39625/1/src/soc/intel/xeon_sp/bootb... PS1, Line 33: .CodeRegionBase = (UINT32)(0x100000000ULL - CONFIG_CBFS_SIZE), : .CodeRegionLength = (UINT32)CONFIG_CBFS_SIZE,
The value we need here has nothing to do with CBFS. […]
With the same FSP after setting them to 0, it can boot to OS but sometimes it takes 7 minutes to boot, this depends on how FSP is implemented but 7 minutes is too long.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39625 )
Change subject: soc/intel/xeon_sp: Modify FSP-T code caching parameters ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39625/2/src/soc/intel/xeon_sp/bootb... File src/soc/intel/xeon_sp/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/39625/2/src/soc/intel/xeon_sp/bootb... PS2, Line 34: .CodeRegionLength = (UINT32)CONFIG_BIOS_REGION_SIZE, why not hardcode to 16MiB? Thats the maximum FSP-T can support. You need to align it to a power of two anyways and I don't see a problem marking the memory as cachable if only 8MiB are actually memory mapped.
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39625 )
Change subject: soc/intel/xeon_sp: Modify FSP-T code caching parameters ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39625/2/src/soc/intel/xeon_sp/bootb... File src/soc/intel/xeon_sp/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/39625/2/src/soc/intel/xeon_sp/bootb... PS2, Line 34: .CodeRegionLength = (UINT32)CONFIG_BIOS_REGION_SIZE,
why not hardcode to 16MiB? Thats the maximum FSP-T can support. […]
Do you mean like this? .CodeRegionBase = (UINT32)(0x100000000ULL - 0x1000000), .CodeRegionLength = 0x1000000, I'll wait for others' opinion a bit. Thanks.
Hello build bot (Jenkins), Andrey Petrov, Anjaneya "Reddy" Chagam, Jonathan Zhang, David Hendricks, Jingle Hsu, Morgan Jang, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39625
to look at the new patch set (#3).
Change subject: soc/intel/xeon_sp: Modify FSP-T code caching parameters ......................................................................
soc/intel/xeon_sp: Modify FSP-T code caching parameters
Added variable CONFIG_BIOS_REGION_SIZE for this purpose.
Tested on OCP Tioga Pass.
Change-Id: Ibba133d9f8fdfbdfae9a0e8e698356a3ca9ba424 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/soc/intel/xeon_sp/bootblock/bootblock.c 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/39625/3
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39625 )
Change subject: soc/intel/xeon_sp: Modify FSP-T code caching parameters ......................................................................
Patch Set 3:
We have CACHE_ROM_BASE/_SIZE (cpu/x86/mtrr.h) for such occasions. It's based on CONFIG_ROM_SIZE but I don't see how that would matter? We might mark a little too much as cacheable, but does it hurt if we never access those addresses?
Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39625 )
Change subject: soc/intel/xeon_sp: Modify FSP-T code caching parameters ......................................................................
Patch Set 3:
Patch Set 3:
We have CACHE_ROM_BASE/_SIZE (cpu/x86/mtrr.h) for such occasions. It's based on CONFIG_ROM_SIZE but I don't see how that would matter? We might mark a little too much as cacheable, but does it hurt if we never access those addresses?
you can't go with CACHE_ROM_SIZE because on 64mb system that overlaps with useful memory-mapped resources, e.g apic [ fecxxxxx ]
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39625 )
Change subject: soc/intel/xeon_sp: Modify FSP-T code caching parameters ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
We have CACHE_ROM_BASE/_SIZE (cpu/x86/mtrr.h) for such occasions. It's based on CONFIG_ROM_SIZE but I don't see how that would matter? We might mark a little too much as cacheable, but does it hurt if we never access those addresses?
you can't go with CACHE_ROM_SIZE because on 64mb system that overlaps with useful memory-mapped resources, e.g apic [ fecxxxxx ]
CACHE_ROM_SIZE is capped at 16MiB, so yes that seems the best solution so far.
Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39625 )
Change subject: soc/intel/xeon_sp: Modify FSP-T code caching parameters ......................................................................
Patch Set 3:
CACHE_ROM_SIZE is capped at 16MiB, so yes that seems the best solution so far.
well, we could also determine BIOS region size at build-time through IFD parsing
Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39625 )
Change subject: soc/intel/xeon_sp: Modify FSP-T code caching parameters ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39625/2/src/soc/intel/xeon_sp/bootb... File src/soc/intel/xeon_sp/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/39625/2/src/soc/intel/xeon_sp/bootb... PS2, Line 34: .CodeRegionLength = (UINT32)CONFIG_BIOS_REGION_SIZE,
Do you mean like this? […]
sounds like CACHE_ROM_SIZE will do the trick here. We can drop the mainboard patch then
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39625 )
Change subject: soc/intel/xeon_sp: Modify FSP-T code caching parameters ......................................................................
Patch Set 3: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/39625/3/src/soc/intel/xeon_sp/bootb... File src/soc/intel/xeon_sp/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/39625/3/src/soc/intel/xeon_sp/bootb... PS3, Line 33: CONFIG_BIOS_REGION_SIZE Is this value going to be used anywhere else? If it's only needed here, then please use a #define instead of Kconfig variable.
Hello build bot (Jenkins), Andrey Petrov, Anjaneya "Reddy" Chagam, Jonathan Zhang, David Hendricks, Jingle Hsu, Morgan Jang, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39625
to look at the new patch set (#4).
Change subject: soc/intel/xeon_sp: Modify FSP-T code caching parameters ......................................................................
soc/intel/xeon_sp: Modify FSP-T code caching parameters
Use CACHE_ROM_BASE and CACHE_ROM_SIZE for code caching parameters.
Tested on OCP Tioga Pass.
Change-Id: Ibba133d9f8fdfbdfae9a0e8e698356a3ca9ba424 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/soc/intel/xeon_sp/bootblock/bootblock.c 1 file changed, 3 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/39625/4
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39625 )
Change subject: soc/intel/xeon_sp: Modify FSP-T code caching parameters ......................................................................
Patch Set 4:
(3 comments)
Patch Set 3:
We have CACHE_ROM_BASE/_SIZE (cpu/x86/mtrr.h) for such occasions. It's based on CONFIG_ROM_SIZE but I don't see how that would matter? We might mark a little too much as cacheable, but does it hurt if we never access those addresses?
https://review.coreboot.org/c/coreboot/+/39625/1/src/soc/intel/xeon_sp/bootb... File src/soc/intel/xeon_sp/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/39625/1/src/soc/intel/xeon_sp/bootb... PS1, Line 33: .CodeRegionBase = (UINT32)(0x100000000ULL - CONFIG_CBFS_SIZE), : .CodeRegionLength = (UINT32)CONFIG_CBFS_SIZE,
With the same FSP after setting them to 0, it can boot to OS but sometimes it takes 7 minutes to boo […]
Done
https://review.coreboot.org/c/coreboot/+/39625/2/src/soc/intel/xeon_sp/bootb... File src/soc/intel/xeon_sp/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/39625/2/src/soc/intel/xeon_sp/bootb... PS2, Line 34: .CodeRegionLength = (UINT32)CONFIG_BIOS_REGION_SIZE,
sounds like CACHE_ROM_SIZE will do the trick here. […]
Done
https://review.coreboot.org/c/coreboot/+/39625/3/src/soc/intel/xeon_sp/bootb... File src/soc/intel/xeon_sp/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/39625/3/src/soc/intel/xeon_sp/bootb... PS3, Line 33: CONFIG_BIOS_REGION_SIZE
Is this value going to be used anywhere else? If it's only needed here, then please use a #define in […]
Done
Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39625 )
Change subject: soc/intel/xeon_sp: Modify FSP-T code caching parameters ......................................................................
Patch Set 4: Code-Review+2
Hello build bot (Jenkins), Andrey Petrov, Anjaneya "Reddy" Chagam, Jonathan Zhang, David Hendricks, Jingle Hsu, Morgan Jang, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39625
to look at the new patch set (#6).
Change subject: soc/intel/xeon_sp: Modify FSP-T code caching parameters ......................................................................
soc/intel/xeon_sp: Modify FSP-T code caching parameters
Use CACHE_ROM_BASE and CACHE_ROM_SIZE for code caching parameters.
Tested on OCP Tioga Pass.
Change-Id: Ibba133d9f8fdfbdfae9a0e8e698356a3ca9ba424 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com --- M src/soc/intel/xeon_sp/bootblock/bootblock.c 1 file changed, 3 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/39625/6
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39625 )
Change subject: soc/intel/xeon_sp: Modify FSP-T code caching parameters ......................................................................
Patch Set 6: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/39625/6/src/soc/intel/xeon_sp/bootb... File src/soc/intel/xeon_sp/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/39625/6/src/soc/intel/xeon_sp/bootb... PS6, Line 31: .MicrocodeRegionBase = (UINT32)CONFIG_CPU_MICROCODE_CBFS_LOC, : .MicrocodeRegionLength = (UINT32)CONFIG_CPU_MICROCODE_CBFS_LEN, : .CodeRegionBase = (UINT32)CACHE_ROM_BASE, : .CodeRegionLength = (UINT32)CACHE_ROM_SIZE, Why the casts?
Andrey Petrov has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39625 )
Change subject: soc/intel/xeon_sp: Modify FSP-T code caching parameters ......................................................................
soc/intel/xeon_sp: Modify FSP-T code caching parameters
Use CACHE_ROM_BASE and CACHE_ROM_SIZE for code caching parameters.
Tested on OCP Tioga Pass.
Change-Id: Ibba133d9f8fdfbdfae9a0e8e698356a3ca9ba424 Signed-off-by: Johnny Lin johnny_lin@wiwynn.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/39625 Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Andrey Petrov anpetrov@fb.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/xeon_sp/bootblock/bootblock.c 1 file changed, 3 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Andrey Petrov: Looks good to me, approved
diff --git a/src/soc/intel/xeon_sp/bootblock/bootblock.c b/src/soc/intel/xeon_sp/bootblock/bootblock.c index 6b2c488..dc88adc 100644 --- a/src/soc/intel/xeon_sp/bootblock/bootblock.c +++ b/src/soc/intel/xeon_sp/bootblock/bootblock.c @@ -19,6 +19,7 @@ #include <intelblocks/fast_spi.h> #include <soc/iomap.h> #include <console/console.h> +#include <cpu/x86/mtrr.h>
const FSPT_UPD temp_ram_init_params = { .FspUpdHeader = { @@ -29,8 +30,8 @@ .FsptCoreUpd = { .MicrocodeRegionBase = (UINT32)CONFIG_CPU_MICROCODE_CBFS_LOC, .MicrocodeRegionLength = (UINT32)CONFIG_CPU_MICROCODE_CBFS_LEN, - .CodeRegionBase = (uint32_t)(0x100000000ULL - CONFIG_ROM_SIZE), - .CodeRegionLength = (UINT32)CONFIG_ROM_SIZE, + .CodeRegionBase = (UINT32)CACHE_ROM_BASE, + .CodeRegionLength = (UINT32)CACHE_ROM_SIZE, .Reserved1 = {0}, }, .FsptConfig = {
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39625 )
Change subject: soc/intel/xeon_sp: Modify FSP-T code caching parameters ......................................................................
Patch Set 7:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1446 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1445 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1444
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Andrey Petrov (Code Review)
-
David Hendricks (Code Review)
-
Johnny Lin (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)