Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35403 )
Change subject: soc/intel/common/basecode: Implement CSE update flow ......................................................................
Patch Set 80:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35403/79//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35403/79//COMMIT_MSG@44 PS79, Line 44: Verified
TEST=
Done
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... File src/soc/intel/common/block/cse/Kconfig:
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... PS79, Line 24: "Name of CSE Region in FMAP"
our assumption as disucssued in earlier patches, FMAP entry for CSE region can be platfrom specific. […]
Done
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... File src/soc/intel/common/block/cse/custom_bp.c:
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... PS79, Line 29: 4
sizeof(uint32_t)
Done
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... PS79, Line 176: static void cse_trigger_recovery(uint8_t rec_sub_code) : { : if (CONFIG(VBOOT)) { : struct vb2_context *ctx; : ctx = vboot_get_context(); : vb2api_fail(ctx, VB2_RECOVERY_CSME_FAILURE, rec_sub_code); : vboot_save_data(ctx); : vboot_reboot(); : } else : do_global_reset(); : } :
Looking at the entire CL, I think addition of just this function can be put as a separate CL and can […]
Done
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... PS79, Line 225: return cse_get_bp_entry_version(RW, cse_bp_info);
Just place the contents of cse_get_bp_entry_version here?
It was added for code readability and one level of indirection as we want to extend the version check further.
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... PS79, Line 462: : if (boot_device_rw_subregion(&cse_rw_region, target_rdev) < 0) : return false;
I didn't understand your comment here. […]
Done
https://review.coreboot.org/c/coreboot/+/35403/79/src/soc/intel/common/block... PS79, Line 608: Layout of SOC_INTEL_CSE_RW_CBFS = [<CSE RW Version>(16 bytes) + <CSE RW Firmware>]
Yes. it's still under discussion. […]
Done