Change in ...coreboot[master]: soc/intel/cannonlake: Do not read SPD again if index hasn't changed
Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33137
Change subject: soc/intel/cannonlake: Do not read SPD again if index hasn't changed ......................................................................
soc/intel/cannonlake: Do not read SPD again if index hasn't changed
With the recent refactoring of memory configuration in CB:32513 ("soc/intel/cannonlake: Support different SPD read type for each slot"), meminit_cbfs_spd_index ends up reading SPD from CBFS for each slot. However, for mainboards that use the same SPD index for each slot this is unneccessary. This change adds a check to see if spd_data_ptr is not NULL and current spd index is the same as the last call to decide if SPD read from CBFS should be skipped.
TEST=Verified that SPD gets read only once on hatch.
Change-Id: I91963b55cea534c92207b2cd9f0caa96df8f222b Signed-off-by: Furquan Shaikh furquan@google.com --- M src/soc/intel/cannonlake/cnl_memcfg_init.c 1 file changed, 21 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/33137/1
diff --git a/src/soc/intel/cannonlake/cnl_memcfg_init.c b/src/soc/intel/cannonlake/cnl_memcfg_init.c index 4ebd997..d3e5e83 100644 --- a/src/soc/intel/cannonlake/cnl_memcfg_init.c +++ b/src/soc/intel/cannonlake/cnl_memcfg_init.c @@ -91,18 +91,29 @@ static void meminit_cbfs_spd_index(FSP_M_CONFIG *mem_cfg, int spd_index, uint8_t mem_slot) { - size_t spd_data_len; - uintptr_t spd_data_ptr; - struct region_device spd_rdev; + static size_t spd_data_len; + static uintptr_t spd_data_ptr; + static int last_spd_index;
assert(mem_slot < NUM_DIMM_SLOT); - printk(BIOS_DEBUG, "SPD INDEX = %d\n", spd_index); - if (get_spd_cbfs_rdev(&spd_rdev, spd_index) < 0) - die("spd.bin not found or incorrect index\n"); - spd_data_len = region_device_sz(&spd_rdev); - /* Memory leak is ok since we have memory mapped boot media */ - assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); - spd_data_ptr = (uintptr_t)rdev_mmap_full(&spd_rdev); + + if ((spd_data_ptr == 0) || (last_spd_index != spd_index)) { + struct region_device spd_rdev; + + printk(BIOS_DEBUG, "SPD INDEX = %d\n", spd_index); + + if (get_spd_cbfs_rdev(&spd_rdev, spd_index) < 0) + die("spd.bin not found or incorrect index\n"); + + spd_data_len = region_device_sz(&spd_rdev); + + /* Memory leak is ok since we have memory mapped boot media */ + assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); + + spd_data_ptr = (uintptr_t)rdev_mmap_full(&spd_rdev); + last_spd_index = spd_index; + } + meminit_spd_data(mem_cfg, mem_slot, spd_data_len, spd_data_ptr); }
Philip Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33137 )
Change subject: soc/intel/cannonlake: Do not read SPD again if index hasn't changed ......................................................................
Patch Set 1: Code-Review+1
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33137 )
Change subject: soc/intel/cannonlake: Do not read SPD again if index hasn't changed ......................................................................
Patch Set 1:
I don't understand the purpose of this; is reading CBFS data expensive in terms of time?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33137 )
Change subject: soc/intel/cannonlake: Do not read SPD again if index hasn't changed ......................................................................
Patch Set 1:
Patch Set 1:
I don't understand the purpose of this; is reading CBFS data expensive in terms of time?
It will definitely have to do more work. cbfs_locate() will parse all the files in CBFS until it finds the required file. Also, this work is completely redundant since it has already been done for the previous slot. I did a very quick timestamp check and it seems to add ~6-10ms.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33137 )
Change subject: soc/intel/cannonlake: Do not read SPD again if index hasn't changed ......................................................................
Patch Set 2: Code-Review+1
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33137 )
Change subject: soc/intel/cannonlake: Do not read SPD again if index hasn't changed ......................................................................
Patch Set 2: Code-Review+2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33137 )
Change subject: soc/intel/cannonlake: Do not read SPD again if index hasn't changed ......................................................................
Patch Set 2: Code-Review+2
Gotcha.
Furquan Shaikh has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33137 )
Change subject: soc/intel/cannonlake: Do not read SPD again if index hasn't changed ......................................................................
soc/intel/cannonlake: Do not read SPD again if index hasn't changed
With the recent refactoring of memory configuration in CB:32513 ("soc/intel/cannonlake: Support different SPD read type for each slot"), meminit_cbfs_spd_index ends up reading SPD from CBFS for each slot. However, for mainboards that use the same SPD index for each slot this is unneccessary. This change adds a check to see if spd_data_ptr is not NULL and current spd index is the same as the last call to decide if SPD read from CBFS should be skipped.
TEST=Verified that SPD gets read only once on hatch.
Change-Id: I91963b55cea534c92207b2cd9f0caa96df8f222b Signed-off-by: Furquan Shaikh furquan@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33137 Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-by: Philip Chen philipchen@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/cannonlake/cnl_memcfg_init.c 1 file changed, 21 insertions(+), 10 deletions(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Aaron Durbin: Looks good to me, approved Philip Chen: Looks good to me, but someone else must approve Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/soc/intel/cannonlake/cnl_memcfg_init.c b/src/soc/intel/cannonlake/cnl_memcfg_init.c index 4ebd997..d3e5e83 100644 --- a/src/soc/intel/cannonlake/cnl_memcfg_init.c +++ b/src/soc/intel/cannonlake/cnl_memcfg_init.c @@ -91,18 +91,29 @@ static void meminit_cbfs_spd_index(FSP_M_CONFIG *mem_cfg, int spd_index, uint8_t mem_slot) { - size_t spd_data_len; - uintptr_t spd_data_ptr; - struct region_device spd_rdev; + static size_t spd_data_len; + static uintptr_t spd_data_ptr; + static int last_spd_index;
assert(mem_slot < NUM_DIMM_SLOT); - printk(BIOS_DEBUG, "SPD INDEX = %d\n", spd_index); - if (get_spd_cbfs_rdev(&spd_rdev, spd_index) < 0) - die("spd.bin not found or incorrect index\n"); - spd_data_len = region_device_sz(&spd_rdev); - /* Memory leak is ok since we have memory mapped boot media */ - assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); - spd_data_ptr = (uintptr_t)rdev_mmap_full(&spd_rdev); + + if ((spd_data_ptr == 0) || (last_spd_index != spd_index)) { + struct region_device spd_rdev; + + printk(BIOS_DEBUG, "SPD INDEX = %d\n", spd_index); + + if (get_spd_cbfs_rdev(&spd_rdev, spd_index) < 0) + die("spd.bin not found or incorrect index\n"); + + spd_data_len = region_device_sz(&spd_rdev); + + /* Memory leak is ok since we have memory mapped boot media */ + assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); + + spd_data_ptr = (uintptr_t)rdev_mmap_full(&spd_rdev); + last_spd_index = spd_index; + } + meminit_spd_data(mem_cfg, mem_slot, spd_data_len, spd_data_ptr); }
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
Paul Menzel (Code Review)
-
Philip Chen (Code Review)
-
Tim Wawrzynczak (Code Review)