Sridhar Siricilla 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 66:
(13 comments)
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_bp.c:
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 116: cse_send_set_boot_partition_info_cmd
I will rename it as cse_send_set_bp_info()
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 161: cse_send_set_boot_partition_info_cmd
If response strcut's result field indicates success, then it means CSE updated the next boot partiti […]
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 254: if ((bp_info_flags & BP_INFO_MIN_RECOV_MODE_EN) != BP_INFO_MIN_RECOV_MODE_EN) { : printk(BIOS_INFO, "Minimum Recovery Mode is not enabled\n"); : *min_rec_mode_en = 0; : } else { : printk(BIOS_INFO, "Minimum Recovery Mode is enabled\n"); : *min_rec_mode_en = 1; : } : : return true;
Suggested code looks good but it does not consider error scenario!
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 284: bool
Yes, it requires since there is a check for error condition.
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 284: me_rw_status
The FW update driver checks for 3 conditions (ME_RW_VALID/INVALID/SIGN_MISMATCH). […]
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 295: bp2_status | bp3_status
If any boot partition is not valid, then it indicates RW partion is invalid.
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 313: ME: %s version = %d.%d.%d.%d
All soc/cse_bp are logging cse version in same format(major.minor.hotfix.build). […]
The below patch address your comment..
https://review.coreboot.org/c/coreboot/+/39023/2
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 222: bp_info_flags
The flag represents value of cse_boot_partition_info struct. […]
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 244: ME RW Status
This status is not returned in response to any command. […]
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 253: ME_RW_SIGN_INVALID
These status codes are not required to be part of this file. I will use CSE FW update file itself.
The code are used in CSE FW update driver only.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 264: *
No, these functions won't be used outside of fw update driver. […]
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 267: CSE Reset
Yes, GLOBAL RESET(GLOBAL_RESET_TYPE) is must to take effect of CSE's boot partition change. […]
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 313: enabled to support CSE FW Update
In the latest CSE FW, this feature is not a selectable setting(or option). […]
Done