[M] Change in coreboot[master]: soc/intel/cmn: Implement API to retrieve firmware partition information
Attention is currently required from: Subrata Banik, Kapil Porwal.
Dinesh Gehlot has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74005 )
Change subject: soc/intel/cmn: Implement API to retrieve firmware partition information ......................................................................
Patch Set 14:
(17 comments)
File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/74005/comment/544fbccf_3a7b4b16 PS9, Line 204: Two instances of 'fw_version' were allocated to store both the ISHC and CSE versions. : The first instance will store ISHC version and the second instance will store CSE version : */
follow the may char per line rule
Ack
https://review.coreboot.org/c/coreboot/+/74005/comment/6a297e98_5411b0dc PS9, Line 204: Two instances of 'fw_version' were allocated to store both the ISHC and CSE versions. : The first instance will store ISHC version and the second instance will store CSE version : */
follow the may char per line rule
old patch, comments already removed
File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/74005/comment/546aba04_3468c993 PS11, Line 204: saved_version = cbmem_add(CBMEM_ID_CSE_PARTITION_VERSION, sizeof(fw_partition_version));
ideally this should be cbmem_find() and if find is failing then you should add new entry isn't it. […]
Ack
https://review.coreboot.org/c/coreboot/+/74005/comment/bc7b8a2a_344d6185 PS11, Line 206: // Compare if saved CSE is same as current
follow /* */
Ack
https://review.coreboot.org/c/coreboot/+/74005/comment/03d1a829_ae4a2d6c PS11, Line 206: // Compare if saved CSE is same as current
follow /* */
Ack
https://review.coreboot.org/c/coreboot/+/74005/comment/7236f531_207cb5a0 PS11, Line 210: ishc_update_required
saved_version->ishc_version_update_done = false
Ack
https://review.coreboot.org/c/coreboot/+/74005/comment/ab6fcf4c_a9326aa0 PS11, Line 211: else
else { […]
Ack
https://review.coreboot.org/c/coreboot/+/74005/comment/51894785_19bce6d8 PS11, Line 211: else
else { […]
Ack
https://review.coreboot.org/c/coreboot/+/74005/comment/01f858b2_48ff812e PS11, Line 212: ishc_update_required
assuming ishc_version_update_done is already set to `true` from since last update?
Ack
File src/soc/intel/common/block/cse/cse_partition_version.c:
https://review.coreboot.org/c/coreboot/+/74005/comment/c8f8a20f_f099f281 PS11, Line 11: cbmem_entry_start
shouldn't this be just cbmem_find()
Ack
https://review.coreboot.org/c/coreboot/+/74005/comment/7c502e66_cf086fd4 PS11, Line 36: if (ret || resp.hdr.result)
if { […]
Ack
https://review.coreboot.org/c/coreboot/+/74005/comment/78ed4d79_55cf0268 PS11, Line 37: printk(BIOS_ERR, "HECI: ISHC version resp Failed: 0x%x\n", resp.hdr.result);
while failing, u should mark ishc_version_update_done is false to avoid getting any wrong value
The variable is removed in the latest patch.
https://review.coreboot.org/c/coreboot/+/74005/comment/ff90975a_b8a7763b PS11, Line 40: ishc_update_required
ishc_version_update_done=true
Ack
https://review.coreboot.org/c/coreboot/+/74005/comment/ba8fb5d6_9177f339 PS11, Line 44: BS_DEV_ENABLE
if we want to do this operation at early ramstage then `BS_PRE_DEVICE` is the correct BS
Ack
File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/74005/comment/3e0a22f1_9b0af6d9 PS11, Line 139: typedef
avoid typedef
Ack
https://review.coreboot.org/c/coreboot/+/74005/comment/28b930da_78e8a958 PS11, Line 140: ishc_update_required
ishc_version_update_done
Ack
https://review.coreboot.org/c/coreboot/+/74005/comment/2af8e3ea_64cdb75a PS11, Line 143: fw_partition_version
cse_fw_ish_version_info
Ack
-
Dinesh Gehlot (Code Review)