Attention is currently required from: Andrey Petrov, Arthur Heymans, Bora Guvendik, Jérémy Compostella, Ronak Kanabar, Shuo Liu.
Hello Andrey Petrov, Arthur Heymans, Bora Guvendik, Ronak Kanabar, Shuo Liu, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/81212?usp=email
to look at the new patch set (#3).
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: drivers/intel/fsp2_0: Support FSP-M execution from CBFS cache
......................................................................
drivers/intel/fsp2_0: Support FSP-M execution from CBFS cache
If SPINOR space is limited and Cache-As-RAM (CAR) is large enough, FSP
can be stored compressed, loaded in CBFS cache and executed from
there. It allows to reduce the FSP-M SPINOR footprint with a limited
boot time penalty. `FSP_EXECUTE_FROM_CBFS_CACHE' Kconfig can be set to
turn on this feature.
We performed some measurements on a Meteor Lake Rex board with a 2 MB
Cache-As-RAM (`DCACHE_RAM_SIZE' at 0x200000 and `DCACHE_RAM_BASE'
0xea000000) and a few adjustments in the FSP to enable CAR to RAM
PEIM (Pre-EFI Initialization Module) drivers migration.
1. Time impact
| Compression algorithm | LZ4 | LZMA |
|--------------------------+----------+----------|
| Decompress duration | 1.9 ms | 75.6 ms |
| Overall boot time impact | +15.9 ms | +77.4 ms |
The overall boot time impact increase is explained by FSP-M loading
being a bit faster (-18 ms) undermined by a longer Cache-As-RAM
setup (32 ms).
2. SPINOR impact
| CBFS file / Compression | LZ4 | LZMA |
|----------------------------+----------------+-----------------|
| romstage size | +4 KB (+3%) | +10 KB (+8%) |
| fspm.bin size | -313 KB (-37%) | -456 KB (-54%) |
|----------------------------+----------------+-----------------|
| Total Per slot | -309 KB | -446 KB |
| Total for SPINOR (3 slots) | -926 KB | -1339 KB |
BUG=b:329237541
TEST=Verified on rex with Cache-AS-RAM size of 2 MB
Change-Id: I8d42d765eb0ecf2e7a8fc6d8d15eb2df8975f6f2
Signed-off-by: Jeremy Compostella <jeremy.compostella(a)intel.com>
---
M src/drivers/intel/fsp2_0/Kconfig
M src/drivers/intel/fsp2_0/memory_init.c
2 files changed, 9 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/81212/3
--
To view, visit https://review.coreboot.org/c/coreboot/+/81212?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I8d42d765eb0ecf2e7a8fc6d8d15eb2df8975f6f2
Gerrit-Change-Number: 81212
Gerrit-PatchSet: 3
Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com>
Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com>
Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Arthur Heymans.
Jérémy Compostella has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81434?usp=email )
Change subject: arch/x86/bootblock.ld: Account for the .data section
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
Patchset:
PS1:
I am bumping up to +2 based on:
1. Arthur extra explanation
2. I concur to the fact that that initialized data are stored in SPINOR and there should be accounted for.
--
To view, visit https://review.coreboot.org/c/coreboot/+/81434?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I39abe499e5e9edbdacb1697c0a0fc347af3ef9c4
Gerrit-Change-Number: 81434
Gerrit-PatchSet: 1
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Tue, 26 Mar 2024 16:51:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Shelley Chen, Tanu Malhotra, Yuval Peress.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81418?usp=email )
Change subject: mb/google/brox: Configure ISH device based on FW_CONFIG
......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/81418/comment/57a52733_bdf8ef7b :
PS1, Line 15: None
> No bug #? You may be able to use 319164720 if you don't have something better.
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/81418?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I678416acd48e03ab77ae299beae6e295a688b8df
Gerrit-Change-Number: 81418
Gerrit-PatchSet: 2
Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: Tanu Malhotra <tanu.malhotra(a)intel.com>
Gerrit-Reviewer: Yuval Peress <peress(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Shelley Chen <shchen(a)google.com>
Gerrit-Attention: Yuval Peress <peress(a)google.com>
Gerrit-Attention: Tanu Malhotra <tanu.malhotra(a)intel.com>
Gerrit-Comment-Date: Tue, 26 Mar 2024 16:50:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Shelley Chen <shchen(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Andrey Petrov, Arthur Heymans, Bora Guvendik, Jérémy Compostella, Ronak Kanabar, Shuo Liu.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81212?usp=email )
Change subject: drivers/intel/fsp2_0: Support FSP-M execution from CBFS cache
......................................................................
Patch Set 2:
(4 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/81212/comment/620d0534_bb47598b :
PS2, Line 12: can
can be
https://review.coreboot.org/c/coreboot/+/81212/comment/7aa6d68b_1e117368 :
PS2, Line 25: | Overall boot time impact | +15.9 ms | +77.4 ms |
Where do the 14 ms difference for LZ4 come from?
https://review.coreboot.org/c/coreboot/+/81212/comment/1ad22eaf_03a5bb41 :
PS2, Line 29: \
Other way /?
File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/81212/comment/0adf4f02_ce604e42 :
PS2, Line 225: Select this value when FSP-M execute from CBFS cache
Please add a dot/period at the end of the sentence.
--
To view, visit https://review.coreboot.org/c/coreboot/+/81212?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I8d42d765eb0ecf2e7a8fc6d8d15eb2df8975f6f2
Gerrit-Change-Number: 81212
Gerrit-PatchSet: 2
Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com>
Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com>
Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Comment-Date: Tue, 26 Mar 2024 16:40:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Karthik Ramasubramanian, Tanu Malhotra, Yuval Peress.
Hello Shelley Chen, Tanu Malhotra, Yuval Peress, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/81418?usp=email
to look at the new patch set (#2).
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: mb/google/brox: Configure ISH device based on FW_CONFIG
......................................................................
mb/google/brox: Configure ISH device based on FW_CONFIG
ISH Firmware name needs to be configured only when full sensing
capabilities are enabled through ISH_ENABLE FW_CONFIG. Similarly dma
property needs to be added only when UFS is enabled through STORAGE_UFS
FW_CONFIG. Hence configure the ISH device at run-time based on
FW_CONFIG.
BUG=b:319164720
TEST=Build Brox BIOS image and boot to OS.
Change-Id: I678416acd48e03ab77ae299beae6e295a688b8df
Signed-off-by: Karthikeyan Ramasubramanian <kramasub(a)google.com>
---
M src/mainboard/google/brox/variants/brox/fw_config.c
M src/mainboard/google/brox/variants/brox/overridetree.cb
2 files changed, 14 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/81418/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/81418?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I678416acd48e03ab77ae299beae6e295a688b8df
Gerrit-Change-Number: 81418
Gerrit-PatchSet: 2
Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: Tanu Malhotra <tanu.malhotra(a)intel.com>
Gerrit-Reviewer: Yuval Peress <peress(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Yuval Peress <peress(a)google.com>
Gerrit-Attention: Tanu Malhotra <tanu.malhotra(a)intel.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Karthik Ramasubramanian, Tanu Malhotra, Yuval Peress.
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81418?usp=email )
Change subject: mb/google/brox: Configure ISH device based on FW_CONFIG
......................................................................
Patch Set 1:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/81418/comment/513dcf0e_c0d84c68 :
PS1, Line 15: None
No bug #? You may be able to use 319164720 if you don't have something better.
--
To view, visit https://review.coreboot.org/c/coreboot/+/81418?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I678416acd48e03ab77ae299beae6e295a688b8df
Gerrit-Change-Number: 81418
Gerrit-PatchSet: 1
Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: Tanu Malhotra <tanu.malhotra(a)intel.com>
Gerrit-Reviewer: Yuval Peress <peress(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Yuval Peress <peress(a)google.com>
Gerrit-Attention: Tanu Malhotra <tanu.malhotra(a)intel.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Tue, 26 Mar 2024 16:27:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Jérémy Compostella has submitted this change. ( https://review.coreboot.org/c/coreboot/+/81256?usp=email )
Change subject: drivers/intel/fsp2_0: Avoid unnecessary extra CBFS access
......................................................................
drivers/intel/fsp2_0: Avoid unnecessary extra CBFS access
fsp_mrc_version() function does not need to perform a CBFS access to
to get an address to the FSP-M blob as the caller,
do_fsp_memory_init(), already has it loaded. In addition to make the
code simpler, it avoids an unnecessary decompression of the FSP blob
if `FSP_COMPRESS_FSP_M_LZ4' or `FSP_COMPRESS_FSP_M_LZMA' are set.
TEST=Verified on Meteor Lake rex
Change-Id: If355b5811a09a0b76acc8a297db719d54caedc54
Signed-off-by: Jeremy Compostella <jeremy.compostella(a)intel.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/81256
Reviewed-by: Shuo Liu <shuo.liu(a)intel.com>
Reviewed-by: Paul Menzel <paulepanter(a)mailbox.org>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Arthur Heymans <arthur(a)aheymans.xyz>
Reviewed-by: Bora Guvendik <bora.guvendik(a)intel.com>
---
M src/drivers/intel/fsp2_0/memory_init.c
1 file changed, 3 insertions(+), 9 deletions(-)
Approvals:
Shuo Liu: Looks good to me, but someone else must approve
build bot (Jenkins): Verified
Paul Menzel: Looks good to me, but someone else must approve
Bora Guvendik: Looks good to me, approved
Arthur Heymans: Looks good to me, approved
diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c
index 27921c6..dc725c3 100644
--- a/src/drivers/intel/fsp2_0/memory_init.c
+++ b/src/drivers/intel/fsp2_0/memory_init.c
@@ -259,23 +259,17 @@
* MRC version is by reading the FSP_PRODUCDER_DATA_TABLES
* from the FSP-M binary (by parsing the FSP header).
*/
-static uint32_t fsp_mrc_version(void)
+static uint32_t fsp_mrc_version(const struct fsp_header *hdr)
{
uint32_t ver = 0;
#if CONFIG(MRC_CACHE_USING_MRC_VERSION)
- size_t fspm_blob_size;
- const char *fspm_cbfs = soc_select_fsp_m_cbfs();
- void *fspm_blob_file = cbfs_map(fspm_cbfs, &fspm_blob_size);
- if (!fspm_blob_file)
- return 0;
-
+ void *fspm_blob_file = (void *)(uintptr_t)hdr->image_base;
FSP_PRODUCER_DATA_TABLES *ft = fspm_blob_file + FSP_HDR_OFFSET;
FSP_PRODUCER_DATA_TYPE2 *table2 = &ft->FspProduceDataType2;
size_t mrc_version_size = sizeof(table2->MrcVersion);
for (size_t i = 0; i < mrc_version_size; i++) {
ver |= (table2->MrcVersion[i] << ((mrc_version_size - 1) - i) * 8);
}
- cbfs_unmap(fspm_blob_file);
#endif
return ver;
}
@@ -349,7 +343,7 @@
post_code(POSTCODE_MEM_PREINIT_PREP_START);
if (CONFIG(MRC_CACHE_USING_MRC_VERSION))
- version = fsp_mrc_version();
+ version = fsp_mrc_version(hdr);
else
version = fsp_memory_settings_version(hdr);
--
To view, visit https://review.coreboot.org/c/coreboot/+/81256?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: If355b5811a09a0b76acc8a297db719d54caedc54
Gerrit-Change-Number: 81256
Gerrit-PatchSet: 4
Gerrit-Owner: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com>
Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: merged