Change in coreboot[master]: mb/google/hatch: Drop use of SPD cache for puff-based variants
Matt DeVillier has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47899 )
Change subject: mb/google/hatch: Drop use of SPD cache for puff-based variants ......................................................................
mb/google/hatch: Drop use of SPD cache for puff-based variants
Commit 74109923 [mb/google/puff: add a region to cache SPD data] implemented a mechanism to cache SODIMM SPD data in a FMAP region, but it's actually slower than simply letting FSP (vs coreboot) read the SPD data via smbus, so drop it.
This has the side effect of removing the requirement for a RW_SPD_CACHE FMAP region, without which these boards would not boot at all - allowing the default FMAP to be used for non-ChromeOS builds.
Test: build/boot WYVERN variant, check boot times via cbmem: w/SPD caching: ~722 ms w/FSP reading: ~627 ms
Change-Id: I4e4db9ba9fdb0fa9fafb2645819b111d381b5756 Signed-off-by: Matt DeVillier matt.devillier@gmail.com --- M src/mainboard/google/hatch/romstage_spd_smbus.c 1 file changed, 5 insertions(+), 56 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/47899/1
diff --git a/src/mainboard/google/hatch/romstage_spd_smbus.c b/src/mainboard/google/hatch/romstage_spd_smbus.c index e697379..1db9c54 100644 --- a/src/mainboard/google/hatch/romstage_spd_smbus.c +++ b/src/mainboard/google/hatch/romstage_spd_smbus.c @@ -1,72 +1,21 @@ /* SPDX-License-Identifier: GPL-2.0-only */
#include <baseboard/variants.h> -#include <console/console.h> #include <soc/cnl_memcfg_init.h> #include <soc/romstage.h> -#include <spd_bin.h> -#include <spd_cache.h>
void mainboard_memory_init_params(FSPM_UPD *memupd) { struct cnl_mb_cfg memcfg; variant_memory_params(&memcfg);
- /* Read spd block to get memory config */ - struct spd_block blk = { - .addr_map = { 0x50, 0x52, }, - }; - - uint8_t *spd_cache; - size_t spd_cache_sz; - bool need_update_cache = false; - bool dimm_changed = true; - - /* load spd cache from RW_SPD_CACHE */ - if (load_spd_cache(&spd_cache, &spd_cache_sz) == CB_SUCCESS) { - if (!spd_cache_is_valid(spd_cache, spd_cache_sz)) { - printk(BIOS_WARNING, "Invalid SPD cache\n"); - } else { - dimm_changed = check_if_dimm_changed(spd_cache, &blk); - if (dimm_changed && memupd->FspmArchUpd.NvsBufferPtr != NULL) { - /* Set mrc_cache as invalid */ - printk(BIOS_INFO, "Set mrc_cache as invalid\n"); - memupd->FspmArchUpd.NvsBufferPtr = NULL; - } - } - need_update_cache = true; - } - - if (!dimm_changed) { - spd_fill_from_cache(spd_cache, &blk); - } else { - /* Access memory info through SMBUS. */ - get_spd_smbus(&blk); - - if (need_update_cache && update_spd_cache(&blk) == CB_ERR) - printk(BIOS_WARNING, "update SPD cache failed\n"); - } - - if (blk.spd_array[0] == NULL) { - memcfg.spd[0].read_type = NOT_EXISTING; - } else { - memcfg.spd[0].read_type = READ_SPD_MEMPTR; - memcfg.spd[0].spd_spec.spd_data_ptr_info.spd_data_len = blk.len; - memcfg.spd[0].spd_spec.spd_data_ptr_info.spd_data_ptr = (uintptr_t)blk.spd_array[0]; - } - + /* DDR4 SODIMMs located at CH0D0, CH1D0 */ + memcfg.spd[0].read_type = READ_SMBUS; + memcfg.spd[0].spd_spec.spd_smbus_address = 0xa0; memcfg.spd[1].read_type = NOT_EXISTING; - - if (blk.spd_array[1] == NULL) { - memcfg.spd[2].read_type = NOT_EXISTING; - } else { - memcfg.spd[2].read_type = READ_SPD_MEMPTR; - memcfg.spd[2].spd_spec.spd_data_ptr_info.spd_data_len = blk.len; - memcfg.spd[2].spd_spec.spd_data_ptr_info.spd_data_ptr = (uintptr_t)blk.spd_array[1]; - } - + memcfg.spd[2].read_type = READ_SMBUS; + memcfg.spd[2].spd_spec.spd_smbus_address = 0xa4; memcfg.spd[3].read_type = NOT_EXISTING; - dump_spd_info(&blk);
/* set to 2 VREF_CA goes to CH_A and VREF_DQ_B goes to CH_B. */ memcfg.vref_ca_config = 2;
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47899 )
Change subject: mb/google/hatch: Drop use of SPD cache for puff-based variants ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG@11 PS1, Line 11: it's actually slower than simply letting FSP (vs coreboot) : read the SPD data via smbus, so drop it. SPD cache is used not just to speed up boot, but also to decide when memory should be retrained if a DIMM is added/removed. Dropping this support will break that functionality.
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG@19 PS1, Line 19: Test: build/boot WYVERN variant, check boot times via cbmem: : w/SPD caching: ~722 ms : w/FSP reading: ~627 ms Adding Jamie Chen who had observed that the SPD cache brought boot time improvements as mentioned in the commit message:
"TEST=Build puff successfully and verified below two items. 1. To change memory DIMM can trigger retraining. 2. one DIMM save the boot time : 158ms two DIMM save the boot time : 265ms"
I don't understand why there is a difference from original implementation.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47899 )
Change subject: mb/google/hatch: Drop use of SPD cache for puff-based variants ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG@11 PS1, Line 11: it's actually slower than simply letting FSP (vs coreboot) : read the SPD data via smbus, so drop it.
SPD cache is used not just to speed up boot, but also to decide when memory should be retrained if a […]
but it doesn't - retraining works just fine either way
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG@19 PS1, Line 19: Test: build/boot WYVERN variant, check boot times via cbmem: : w/SPD caching: ~722 ms : w/FSP reading: ~627 ms
I don't understand why there is a difference from original implementation.
I spent some time adding RW_SPD_CACHE support to the default FMAP for use on another CML board, and saw worse boot times. Spun my tires for a bit, then decided to validate on puff by having FSP read the SPDs directly via smbus. ~100ms improvement. So I have no idea how this ever showed an improvement, unless early CML FSP had some horrible read times. But even reverting commit 74109923 and having coreboot read the SPDs via smbus shaved 30ms or so off the boot time. So the SPD cache was the worst performing of the 3 methods.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47899 )
Change subject: mb/google/hatch: Drop use of SPD cache for puff-based variants ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG@11 PS1, Line 11: it's actually slower than simply letting FSP (vs coreboot) : read the SPD data via smbus, so drop it.
but it doesn't - retraining works just fine either way
Isn't the serial number in SPD cache compared to decide whether memory should be retrained when a DIMM is added/removed: https://review.coreboot.org/cgit/coreboot.git/tree/src/lib/spd_cache.c?id=re...
That is where the original bug started i.e. on adding or removing a DIMM, memory retraining wasn't triggered and so the SPD cache was added.
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG@19 PS1, Line 19: Test: build/boot WYVERN variant, check boot times via cbmem: : w/SPD caching: ~722 ms : w/FSP reading: ~627 ms
I don't understand why there is a difference from original implementation. […]
Thanks for the numbers Matt. That is helpful.
I am just curious why SPD cache takes more time in practice. Looking at the code, it seems to be just passing in the memory-mapped address of the SPD cache region to FSP. So, ideally it should be just reading from that address, which I think would be faster than performing SMBUS operation. Did you check where exactly the boot time savings are? Is it all inside FSP-M?
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47899 )
Change subject: mb/google/hatch: Drop use of SPD cache for puff-based variants ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG@11 PS1, Line 11: it's actually slower than simply letting FSP (vs coreboot) : read the SPD data via smbus, so drop it.
Isn't the serial number in SPD cache compared to decide whether memory should be retrained when a DI […]
I'll have to retest swapping SODIMMs here on my Librem CML board and make sure MRC is invalidated/RAM training occurs, but I don't recall it being an issue.
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG@19 PS1, Line 19: Test: build/boot WYVERN variant, check boot times via cbmem: : w/SPD caching: ~722 ms : w/FSP reading: ~627 ms
Thanks for the numbers Matt. That is helpful. […]
a good chunk is FSP-M:
w/SPD cache: before ram initialization: 55,526 calling FspMemoryInit: 120,426 returning from FspMemoryInit: 39,648
w/o SPD cache: before ram initialization: 55,833 calling FspMemoryInit: 6,866 returning from FspMemoryInit: 40,483
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47899 )
Change subject: mb/google/hatch: Drop use of SPD cache for puff-based variants ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG@11 PS1, Line 11: it's actually slower than simply letting FSP (vs coreboot) : read the SPD data via smbus, so drop it.
I'll have to retest swapping SODIMMs here on my Librem CML board and make sure MRC is invalidated/RA […]
It would also be good to test just removing a DIMM or adding a DIMM (i.e. not swapping with a different SODIMM) to ensure memory training is triggered. The referenced bug on the original change talks about adding/removing not triggering a retrain.
Jamie/Edward - can you please comment on this too since you have debugged/tested this?
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG@19 PS1, Line 19: Test: build/boot WYVERN variant, check boot times via cbmem: : w/SPD caching: ~722 ms : w/FSP reading: ~627 ms
a good chunk is FSP-M: […]
Actually, it looks like the difference is before calling FSP-M? So, the ~120ms is spent doing something with the SPD cache in coreboot rather than in FSP-M?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47899 )
Change subject: mb/google/hatch: Drop use of SPD cache for puff-based variants ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG@19 PS1, Line 19: Test: build/boot WYVERN variant, check boot times via cbmem: : w/SPD caching: ~722 ms : w/FSP reading: ~627 ms
Actually, it looks like the difference is before calling FSP-M? So, the ~120ms is spent doing someth […]
This time difference is from platform_fsp_memory_init_params_cb() / mainboard_memory_init_params() and friends... so mb/google/hatch/romstage_spd_smbus.c, which implements the SPD cache is definitely suspicious here.
Jamie Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47899 )
Change subject: mb/google/hatch: Drop use of SPD cache for puff-based variants ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG@11 PS1, Line 11: it's actually slower than simply letting FSP (vs coreboot) : read the SPD data via smbus, so drop it.
It would also be good to test just removing a DIMM or adding a DIMM (i.e. […]
The original idea is the read speed of SPI rom should faster than reading from smbus and also solving the detection issue when adding or removing SODIMM. It's a risk if the MRC_CACHE won't be update when you change to another DIMM or adding a new DIMM.
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG@19 PS1, Line 19: Test: build/boot WYVERN variant, check boot times via cbmem: : w/SPD caching: ~722 ms : w/FSP reading: ~627 ms
This time difference is from platform_fsp_memory_init_params_cb() / mainboard_memory_init_params() a […]
Do a quick check with a puff variant project in my side. There are two SODIMM installed on the device. I saw w/SPD cache faster than using fsp to read spd 19ms when cold reboot, but slower 6ms when warm reboot. Hi Matt, Do you see the different between cold reboot and warm reboot?
With SPD cache 1265891 Cold reboot 1062252 warm reset
With this patch 1284142 Cold reboot 1056183 warm reset
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47899 )
Change subject: mb/google/hatch: Drop use of SPD cache for puff-based variants ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG@19 PS1, Line 19: Test: build/boot WYVERN variant, check boot times via cbmem: : w/SPD caching: ~722 ms : w/FSP reading: ~627 ms
Hi Matt, Do you see the different between cold reboot and warm reboot?
With SPD cache 1265891 Cold reboot 1062252 warm reset
With this patch 1284142 Cold reboot 1056183 warm reset
on a google/puff variant (wyvern), I'm seeing:
with SPD cache cold: 13,059,672 warm: 904,647
with patch cold: 13,214,845 warm: 897,934
for some reason, a cold reboot seems to always result in MRC cache needing to be updated. Need to look into that.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47899 )
Change subject: mb/google/hatch: Drop use of SPD cache for puff-based variants ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG@19 PS1, Line 19: Test: build/boot WYVERN variant, check boot times via cbmem: : w/SPD caching: ~722 ms : w/FSP reading: ~627 ms
Hi Matt, Do you see the different between cold reboot and warm reboot? […]
and on a Librem 14 (CML) development unit (w/2 sodimms):
with SPD cache cold: 634,xxx (occasionally 734,xxx) warm: 643,xxx
without SPD cache cold: 639,xxx warm: 637,xxx
<shrugs> maybe I'll just continue my patch to add the SPD cache as an option for the default FMAP
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47899 )
Change subject: mb/google/hatch: Drop use of SPD cache for puff-based variants ......................................................................
Patch Set 1: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47899 )
Change subject: mb/google/hatch: Drop use of SPD cache for puff-based variants ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
Conversation still ongoing.
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG@11 PS1, Line 11: it's actually slower than simply letting FSP (vs coreboot) : read the SPD data via smbus, so drop it.
The original idea is the read speed of SPI rom should faster than reading from smbus and also solvin […]
Shouldn't FSP-M have some logic to decide whether to do a full training?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47899 )
Change subject: mb/google/hatch: Drop use of SPD cache for puff-based variants ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG@11 PS1, Line 11: it's actually slower than simply letting FSP (vs coreboot) : read the SPD data via smbus, so drop it.
Shouldn't FSP-M have some logic to decide whether to do a full training?
Ideally yes. But, in practice, we observed that FSP-M does not do that. Hence, coreboot had to do it.
Matt DeVillier has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/47899 )
Change subject: mb/google/hatch: Drop use of SPD cache for puff-based variants ......................................................................
Abandoned
will rework approach to decouple feature from this specific board
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47899 )
Change subject: mb/google/hatch: Drop use of SPD cache for puff-based variants ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47899/1//COMMIT_MSG@11 PS1, Line 11: it's actually slower than simply letting FSP (vs coreboot) : read the SPD data via smbus, so drop it.
Ideally yes. But, in practice, we observed that FSP-M does not do that. […]
I'm sorta seeing that also on a Librem 14 development unit, so I think I'll probably end up using this there as well. So I'll drop this and add a new patch to add the RW_SPD_CACHE to the default fmap and decouple it from google/hatch
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47899 )
Change subject: mb/google/hatch: Drop use of SPD cache for puff-based variants ......................................................................
Patch Set 1:
evolved into CB:48520
-
Angel Pons (Code Review) -
Furquan Shaikh (Code Review)
-
Jamie Chen (Code Review)
-
Matt DeVillier (Code Review)
-
Tim Wawrzynczak (Code Review)