Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35402 )
Change subject: soc/intel/common/block/cse: Add boot partition related APIs ......................................................................
Patch Set 38:
(13 comments)
https://review.coreboot.org/c/coreboot/+/35402/38//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35402/38//COMMIT_MSG@17 PS38, Line 17: below APIs This patch is adding more than the 2 APIs to the header file. Also it would be good if this can be split into smaller patches.
https://review.coreboot.org/c/coreboot/+/35402/38/src/soc/intel/common/block... File src/soc/intel/common/block/cse/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35402/38/src/soc/intel/common/block... PS38, Line 4: bootblock-$(CONFIG_SOC_INTEL_COMMON_BLOCK_CSE) += cse_bp.c : romstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_CSE) += cse_bp.c We don't need this in bootblock and romstage
https://review.coreboot.org/c/coreboot/+/35402/38/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_bp.c:
https://review.coreboot.org/c/coreboot/+/35402/38/src/soc/intel/common/block... PS38, Line 103: force_cse_init What is the use of this variable?
I see that this function is getting called with force_cse_init value is being sent as false at 7 places and true in 1 place.Basically you want to update the partition info when you change it.
Why don't you just call the the update function, cse_update_boot_partition_info() after you change the partition in cse_set_next_boot_partition(). And since partition information should not change unless you change it using cse_set_next_boot_partition() you don't need to explicitly mention force_cse_init at 8 places.
https://review.coreboot.org/c/coreboot/+/35402/38/src/soc/intel/common/block... PS38, Line 105: cse_bp_init Can we just not use the "total_number_of_bp" field in cse_boot_partition_info to check if the partition info has been initialized or not. My understanding is that the number of partitions is always non-zero.
https://review.coreboot.org/c/coreboot/+/35402/38/src/soc/intel/common/block... PS38, Line 166: if (!cse_init_bp_info(true)) { : printk(BIOS_ERR, "Failed to update boot partition info\n"); : return false; : } just call the function to update the partition info, check comment above.
https://review.coreboot.org/c/coreboot/+/35402/38/src/soc/intel/common/block... PS38, Line 183: cse_init_bp_info(false) check comment on cse_init_bp_info()
https://review.coreboot.org/c/coreboot/+/35402/38/src/soc/intel/common/block... PS38, Line 192: cse_init_bp_info check comment on cse_init_bp_info()
https://review.coreboot.org/c/coreboot/+/35402/38/src/soc/intel/common/block... PS38, Line 201: cse_init_bp_info check comment on cse_init_bp_info()
https://review.coreboot.org/c/coreboot/+/35402/38/src/soc/intel/common/block... PS38, Line 210: cse_init_bp_info check comment on cse_init_bp_info()
https://review.coreboot.org/c/coreboot/+/35402/38/src/soc/intel/common/block... PS38, Line 221: cse_init_bp_info check comment on cse_init_bp_info()
https://review.coreboot.org/c/coreboot/+/35402/38/src/soc/intel/common/block... PS38, Line 232: cse_init_bp_info check comment on cse_init_bp_info()
https://review.coreboot.org/c/coreboot/+/35402/38/src/soc/intel/common/block... PS38, Line 304: cse_init_bp_info check comment on cse_init_bp_info()
https://review.coreboot.org/c/coreboot/+/35402/38/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35402/38/src/soc/intel/common/block... PS38, Line 166: cse_wait_opmode_soft_temp_disable Is this defined in this patch?