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 71:
(12 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
Fine with build process. […]
Yes, CSE FW update is implemented without the INTEL_CSE_UPDATE Kconfig. In both above 2 cases, coreboot just ignores CSE FW update, switches to RW (if required) and continues to boot.
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 6: ADD_ME_RW_BINARY
Same here. This config will not be required.
Yes, this is not required. I removed the same.
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 18: ME_RW_FILE
In order to keep consistent naming, I think: […]
Yes,implemented the same. But I used below KConfigs for the same. I have used below KConfigs SOC_INTEL_CSE_RW_CBFS_NAME SOC_INTEL_CSE_RW_FILE
https://review.coreboot.org/c/coreboot/+/35403/2/src/soc/intel/common/baseco... File src/soc/intel/common/basecode/fw_update/cse_update.c:
https://review.coreboot.org/c/coreboot/+/35403/2/src/soc/intel/common/baseco... PS2, Line 377: Previous update is incomplete or RW Boot partition is corrupte
can we differentiate between the 2 possibilities here?
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 294: if (rec_mode) { : printk(BIOS_DEBUG, "me_fwu: Skip FW update in the recovery path\n"); : return; : }
Done
Done
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 317: if (get_cbfs_me_rw_ver(&me_rw_ver_cbfs) < 0) { : printk(BIOS_ERR, "me_fwu: Failed to get cbfs ME for update\n"); : goto failed; : } :
The same blob contains RW FW + Version.
Done
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 332: rdev_mmap
I organized the code as above but deviated little bit. Please check.
Done
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec... PS67, Line 346: if (!cse_check_rw_status(&me_rw_part_status)) { : printk(BIOS_ERR, "me_fwu: me rw status check fail\n"); : goto failed; : }
I pushed this code after recovery check. […]
Done
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();
No where in this path I see recovery being triggered. […]
May I know your comment on triggering recovery through set_recovery_mode_into_vbnv(1) API and followed do_global_reset()?
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
As mentioned here: https://review.coreboot. […]
I made small change to point E) Check if RW update is required. It can be based on two things: (i) RW's signature is not matching BTW,If RW signature is not valid, status id for RW partition is set to 1 (ii) If status ID for RW partition is 0 and RW version in boot info does not match RW version in CBFS. If either of the above is yes, update is required
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/block... PS67, Line 885: * TODO: Check to move this to bootblock */
It is not possible to move to bootblock. […]
Ack
https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/block... PS67, Line 887: rec_mode && current_bp != BP1
I mentioned this on https://review.coreboot. […]
do_global_reset() guarantees reset. So, EC/H1 help is not required here.
-
Sridhar Siricilla (Code Review)