Attention is currently required from: Furquan Shaikh, Frans Hendriks, Patrick Rudolph, Aaron Durbin, Wim Vervoorn. Hello Furquan Shaikh, Aaron Durbin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/50350
to review the following change.
Change subject: spd_bin: Replace get_spd_cbfs_rdev() with spd_cbfs_map() ......................................................................
spd_bin: Replace get_spd_cbfs_rdev() with spd_cbfs_map()
In pursuit of the goal of eliminating the proliferation of raw region devices to represent CBFS files outside of the CBFS core code, this patch removes the get_spd_cbfs_rdev() API and instead replaces it with spd_cbfs_map() which will find and map the SPD file in one go and return a pointer to the relevant section. (This makes it impossible to unmap the mapping again, which all but one of the users didn't bother to do anyway since the API is only used on platforms with memory-mapped flash. Presumably this will stay that way in the future so this is not something worth worrying about.)
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: Iec7571bec809f2f0712e7a97b4c853b8b40702d1 --- M src/include/spd_bin.h M src/lib/spd_bin.c M src/mainboard/facebook/fbg1701/romstage.c M src/mainboard/google/kahlee/variants/baseboard/memory.c M src/mainboard/intel/icelake_rvp/romstage_fsp_params.c M src/mainboard/intel/kblrvp/romstage.c M src/mainboard/portwell/m107/romstage.c M src/mainboard/razer/blade_stealth_kbl/romstage.c M src/soc/intel/cannonlake/cnl_memcfg_init.c M src/soc/intel/common/block/memory/meminit.c M src/soc/intel/elkhartlake/meminit.c M src/soc/intel/jasperlake/meminit.c 12 files changed, 45 insertions(+), 69 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/50350/1
diff --git a/src/include/spd_bin.h b/src/include/spd_bin.h index 11a0084..973eb49 100644 --- a/src/include/spd_bin.h +++ b/src/include/spd_bin.h @@ -43,8 +43,7 @@ };
void print_spd_info(uint8_t spd[]); -/* Return 0 on success & -1 on failure */ -int get_spd_cbfs_rdev(struct region_device *spd_rdev, u8 spd_index); +uintptr_t spd_cbfs_map(u8 spd_index); void dump_spd_info(struct spd_block *blk); void get_spd_smbus(struct spd_block *blk);
diff --git a/src/lib/spd_bin.c b/src/lib/spd_bin.c index 863218c..d6815af 100644 --- a/src/lib/spd_bin.c +++ b/src/lib/spd_bin.c @@ -208,17 +208,16 @@ } }
-int get_spd_cbfs_rdev(struct region_device *spd_rdev, u8 spd_index) +uintptr_t spd_cbfs_map(u8 spd_index) { - struct cbfsf fh; + enum cbfs_type cbfs_type = CBFS_TYPE_SPD; + size_t size;
- uint32_t cbfs_type = CBFS_TYPE_SPD; + void *map = cbfs_type_map("spd.bin", &size, &cbfs_type); + if (!map || size < (spd_index + 1) * CONFIG_DIMM_SPD_SIZE) + return 0;
- if (cbfs_boot_locate(&fh, "spd.bin", &cbfs_type) < 0) - return -1; - cbfs_file_data(spd_rdev, &fh); - return rdev_chain(spd_rdev, spd_rdev, spd_index * CONFIG_DIMM_SPD_SIZE, - CONFIG_DIMM_SPD_SIZE); + return (uintptr_t)map + spd_index * CONFIG_DIMM_SPD_SIZE; }
#if CONFIG_DIMM_SPD_SIZE == 128 diff --git a/src/mainboard/facebook/fbg1701/romstage.c b/src/mainboard/facebook/fbg1701/romstage.c index f307f95..fd005a6 100644 --- a/src/mainboard/facebook/fbg1701/romstage.c +++ b/src/mainboard/facebook/fbg1701/romstage.c @@ -18,7 +18,6 @@ void mainboard_memory_init_params(struct romstage_params *params, MEMORY_INIT_UPD *memory_params) { - struct region_device spd_rdev; u8 spd_index = 0;
if (!CONFIG(ONBOARD_SAMSUNG_MEM)) { @@ -28,11 +27,10 @@ spd_index = 2; }
- if (get_spd_cbfs_rdev(&spd_rdev, spd_index) < 0) - die("spd.bin not found\n"); - memory_params->PcdMemoryTypeEnable = MEM_DDR3; - memory_params->PcdMemorySpdPtr = (uintptr_t)rdev_mmap_full(&spd_rdev); + memory_params->PcdMemorySpdPtr = spd_cbfs_map(spd_index); + if (!memory_params->PcdMemorySpdPtr) + die("spd.bin not found\n"); memory_params->PcdMemChannel0Config = 1; /* Memory down */ memory_params->PcdMemChannel1Config = 2; /* Disabled */ } diff --git a/src/mainboard/google/kahlee/variants/baseboard/memory.c b/src/mainboard/google/kahlee/variants/baseboard/memory.c index d3d81fd..3b3a3b7 100644 --- a/src/mainboard/google/kahlee/variants/baseboard/memory.c +++ b/src/mainboard/google/kahlee/variants/baseboard/memory.c @@ -22,25 +22,22 @@ int __weak variant_mainboard_read_spd(uint8_t spdAddress, char *buf, size_t len) { - struct region_device spd_rdev; u8 spd_index = variant_memory_sku();
printk(BIOS_INFO, "%s SPD index %d\n", __func__, spd_index);
- if (get_spd_cbfs_rdev(&spd_rdev, spd_index) < 0) { + void *spd = (void *)spd_cbfs_map(spd_index); + if (!spd) { printk(BIOS_ERR, "Error: spd.bin not found\n"); return -1; }
- if (len != region_device_sz(&spd_rdev)) { + if (len != CONFIG_DIMM_SPD_SIZE) { printk(BIOS_ERR, "Error: spd.bin is not the correct size\n"); return -1; }
- if (rdev_readat(&spd_rdev, buf, 0, len) != len) { - printk(BIOS_ERR, "Error: couldn't read spd.bin\n"); - return -1; - } + memcpy(buf, spd, len);
return 0; } diff --git a/src/mainboard/intel/icelake_rvp/romstage_fsp_params.c b/src/mainboard/intel/icelake_rvp/romstage_fsp_params.c index 97617cc..160f0a5 100644 --- a/src/mainboard/intel/icelake_rvp/romstage_fsp_params.c +++ b/src/mainboard/intel/icelake_rvp/romstage_fsp_params.c @@ -14,15 +14,12 @@ printk(BIOS_DEBUG, "spd index is 0x%x\n", spd_index);
if (spd_index > 0 && spd_index != 2) { - struct region_device spd_rdev; - - if (get_spd_cbfs_rdev(&spd_rdev, spd_index) < 0) - die("spd.bin not found\n"); - - mem_cfg->MemorySpdDataLen = region_device_sz(&spd_rdev); + mem_cfg->MemorySpdDataLen = CONFIG_DIMM_SPD_SIZE;
/* Memory leak is ok since we have memory mapped boot media */ - mem_cfg->MemorySpdPtr00 = (uintptr_t)rdev_mmap_full(&spd_rdev); + mem_cfg->MemorySpdPtr00 = spd_cbfs_map(spd_index); + if (!mem_cfg->MemorySpdPtr00) + die("spd.bin not found\n"); mem_cfg->MemorySpdPtr10 = mem_cfg->MemorySpdPtr00;
mem_cfg->SpdAddressTable[0] = 0x0; diff --git a/src/mainboard/intel/kblrvp/romstage.c b/src/mainboard/intel/kblrvp/romstage.c index 79206e0..bc4c6de 100644 --- a/src/mainboard/intel/kblrvp/romstage.c +++ b/src/mainboard/intel/kblrvp/romstage.c @@ -28,14 +28,12 @@ mainboard_fill_rcomp_strength_data(&mem_cfg->RcompTarget);
if (CONFIG(BOARD_INTEL_KBLRVP3)) { - struct region_device spd_rdev; - mem_cfg->DqPinsInterleaved = 0; - if (get_spd_cbfs_rdev(&spd_rdev, spd_index) < 0) - die("spd.bin not found\n"); - mem_cfg->MemorySpdDataLen = region_device_sz(&spd_rdev); + mem_cfg->MemorySpdDataLen = CONFIG_DIMM_SPD_SIZE; /* Memory leak is ok since we have memory mapped boot media */ - mem_cfg->MemorySpdPtr00 = (uintptr_t)rdev_mmap_full(&spd_rdev); + mem_cfg->MemorySpdPtr00 = spd_cbfs_map(spd_index); + if (!mem_cfg->MemorySpdPtr00) + die("spd.bin not found\n"); } else { /* CONFIG_BOARD_INTEL_KBLRVP7 and CONFIG_BOARD_INTEL_KBLRVP8 */ struct spd_block blk = { .addr_map = { 0x50, 0x51, 0x52, 0x53, }, diff --git a/src/mainboard/portwell/m107/romstage.c b/src/mainboard/portwell/m107/romstage.c index 3ba12f3..d6d4955 100644 --- a/src/mainboard/portwell/m107/romstage.c +++ b/src/mainboard/portwell/m107/romstage.c @@ -12,7 +12,6 @@ void mainboard_memory_init_params(struct romstage_params *params, MEMORY_INIT_UPD *memory_params) { - struct region_device spd_rdev; u8 spd_index = 0;
if (CONFIG(ONBOARD_MEM_MICRON)) @@ -20,11 +19,10 @@ else if (CONFIG(ONBOARD_MEM_KINGSTON)) spd_index = 2;
- if (get_spd_cbfs_rdev(&spd_rdev, spd_index) < 0) - die("spd.bin not found\n"); - memory_params->PcdMemoryTypeEnable = MEM_DDR3; - memory_params->PcdMemorySpdPtr = (uintptr_t)rdev_mmap_full(&spd_rdev); + memory_params->PcdMemorySpdPtr = spd_cbfs_map(spd_index); + if (!memory_params->PcdMemorySpdPtr) + die("spd.bin not found\n"); memory_params->PcdMemChannel0Config = 1; /* Memory down */ memory_params->PcdMemChannel1Config = 2; /* Disabled */ } diff --git a/src/mainboard/razer/blade_stealth_kbl/romstage.c b/src/mainboard/razer/blade_stealth_kbl/romstage.c index 92ced2e..02bfda48 100644 --- a/src/mainboard/razer/blade_stealth_kbl/romstage.c +++ b/src/mainboard/razer/blade_stealth_kbl/romstage.c @@ -30,15 +30,13 @@ mainboard_fill_rcomp_res_data(&mem_cfg->RcompResistor); mainboard_fill_rcomp_strength_data(&mem_cfg->RcompTarget);
- struct region_device spd_rdev; - mem_cfg->DqPinsInterleaved = 0; - if (get_spd_cbfs_rdev(&spd_rdev, spd_index) < 0) - die("spd.bin not found\n"); - mem_cfg->MemorySpdDataLen = region_device_sz(&spd_rdev); + mem_cfg->MemorySpdDataLen = CONFIG_DIMM_SPD_SIZE; /* Memory leak is ok since we have memory mapped boot media */ // TODO evaluate google/eve way of loading - mem_cfg->MemorySpdPtr00 = (uintptr_t)rdev_mmap_full(&spd_rdev); + mem_cfg->MemorySpdPtr00 = spd_cbfs_map(spd_index); + if (!mem_cfg->MemorySpdPtr00) + die("spd.bin not found\n"); mem_cfg->MemorySpdPtr10 = mem_cfg->MemorySpdPtr00;
mupd->FspmTestConfig.DmiVc1 = 1; diff --git a/src/soc/intel/cannonlake/cnl_memcfg_init.c b/src/soc/intel/cannonlake/cnl_memcfg_init.c index 46452a8..22c0e80 100644 --- a/src/soc/intel/cannonlake/cnl_memcfg_init.c +++ b/src/soc/intel/cannonlake/cnl_memcfg_init.c @@ -86,19 +86,17 @@ assert(mem_slot < NUM_DIMM_SLOT);
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) + spd_data_ptr = spd_cbfs_map(spd_index); + if (!spd_data_ptr); die("spd.bin not found or incorrect index\n");
- spd_data_len = region_device_sz(&spd_rdev); + spd_data_len = CONFIG_DIMM_SPD_SIZE;
/* 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; print_spd_info((unsigned char *)spd_data_ptr); } diff --git a/src/soc/intel/common/block/memory/meminit.c b/src/soc/intel/common/block/memory/meminit.c index 84987d6..7b8ef3a 100644 --- a/src/soc/intel/common/block/memory/meminit.c +++ b/src/soc/intel/common/block/memory/meminit.c @@ -39,7 +39,6 @@ { size_t ch; size_t num_phys_ch = soc_mem_cfg->num_phys_channels; - struct region_device spd_rdev; uintptr_t spd_data;
/* @@ -63,15 +62,14 @@
printk(BIOS_DEBUG, "SPD index = %zu\n", info->cbfs_index);
- if (get_spd_cbfs_rdev(&spd_rdev, info->cbfs_index) < 0) - die("SPD not found in CBFS or incorrect index!\n"); - /* Memory leak is ok as long as we have memory mapped boot media */ _Static_assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED), "Function assumes memory-mapped boot media");
- spd_data = (uintptr_t)rdev_mmap_full(&spd_rdev); - *spd_len = region_device_sz(&spd_rdev); + *spd_len = CONFIG_DIMM_SPD_SIZE; + spd_data = spd_cbfs_map(info->cbfs_index); + if (!spd_data) + die("SPD not found in CBFS or incorrect index!\n");
print_spd_info((uint8_t *)spd_data);
diff --git a/src/soc/intel/elkhartlake/meminit.c b/src/soc/intel/elkhartlake/meminit.c index f1f8270..d6de9a4 100644 --- a/src/soc/intel/elkhartlake/meminit.c +++ b/src/soc/intel/elkhartlake/meminit.c @@ -10,19 +10,17 @@ static void spd_read_from_cbfs(const struct spd_info *spd_info, uintptr_t *spd_data_ptr, size_t *spd_data_len) { - struct region_device spd_rdev; size_t spd_index = spd_info->spd_spec.spd_index;
printk(BIOS_DEBUG, "SPD INDEX = %lu\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); + *spd_data_len = CONFIG_DIMM_SPD_SIZE; + *spd_data_ptr = spd_cbfs_map(spd_index); + if (!*spd_data_ptr) + die("spd.bin not found or incorrect index\n"); }
static void get_spd_data(const struct spd_info *spd_info, uintptr_t *spd_data_ptr, diff --git a/src/soc/intel/jasperlake/meminit.c b/src/soc/intel/jasperlake/meminit.c index cd777ba..9ec1ffa 100644 --- a/src/soc/intel/jasperlake/meminit.c +++ b/src/soc/intel/jasperlake/meminit.c @@ -10,19 +10,17 @@ static void spd_read_from_cbfs(const struct spd_info *spd_info, uintptr_t *spd_data_ptr, size_t *spd_data_len) { - struct region_device spd_rdev; size_t spd_index = spd_info->spd_spec.spd_index;
printk(BIOS_DEBUG, "SPD INDEX = %lu\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); + *spd_data_len = CONFIG_DIMM_SPD_SIZE; + *spd_data_ptr = spd_cbfs_map(spd_index); + if (!*spd_data_ptr) + die("spd.bin not found or incorrect index\n"); }
static void get_spd_data(const struct spd_info *spd_info, uintptr_t *spd_data_ptr,