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 73:
(13 comments)
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... File src/soc/intel/common/basecode/fw_update/Kconfig:
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 1: INTEL_CSE_UPDATE
Yes, CSE FW update is implemented without the INTEL_CSE_UPDATE Kconfig. […]
Done
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 13: ME_RW_VER_CBFS
It's being used as CBFS name for blob ME Image+Version
Done
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 18: ME_RW_FILE
Yes,implemented the same. […]
Done
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... File src/soc/intel/common/basecode/fw_update/cse_update.c:
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 41: #define ME_BP_SIGN_SIZE 4
Every CSE BP starts with fixed signature. […]
Done
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 43: /* ME RW's signature is valid */ : #define ME_RW_SIGN_VALID 0 : /* ME RW's Signature is not valid */ : #define ME_RW_SIGN_INVALID 1 :
The flags relevance to above explanation.
Done
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 68: CONFIG_ME_RW_VER_CBFS
The CBFS entry used to store CSE RW blob containing RW code and Version info.
Done
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 99: bp2_end_offset + 1
Yes,this is right -> end_offset - start_offset + 1;
Done
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 122: eturn -1;
If coreboot cann't talk to CSE, then do_global_reset() triggers guaranteed global reset. […]
We will not get into this scenario as we are going with GLOBAL RESET instead of CSE_RESET.
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 130: heci_reset
This is must after CSE goes for reset. It's to re-syncronize CSE and HOST for heci communication.
This is not required as we are not going with CSE_RESET to switch RO during FW update.
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 395: printk(BIOS_ERR, "me_fwu: Failed %s\n", __func__); : do_global_reset();
Just noticed set_recovery_mode_into_vbnv() is removed from hatch(mainstream code). […]
Done
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/block... PS67, Line 866: cse_enable_fw_sku_custom
I made small change to point E) […]
Done
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/block... PS67, Line 887: rec_mode && current_bp != BP1
do_global_reset() guarantees reset. So, EC/H1 help is not required here.
Done
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/block... PS67, Line 908: failed: : printk(BIOS_ERR, "HECI: Failed to enable CSE Firmware Custom SKU\n"); : do_global_reset();
What is your comment on triggering recovery through set_recovery_mode_into_vbnv(1) and followed […]
Done