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 64:
(21 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 32: boot_partition_info_entry
I think this can be just name cse_boot_partition_entry or cse_bp_entry?
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 116: cse_send_set_boot_partition_info_cmd
The command name is SET_BOOT_PARTITION_INFO. […]
I will rename it as cse_send_set_bp_info()
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 161: cse_send_set_boot_partition_info_cmd
I am curious why CSE cannot just return a status in response to MKHI_SET_BOOT_PARTITION_CMD_REQ indi […]
If response strcut's result field indicates success, then it means CSE updated the next boot partition. Line #170 to 175 are just a redundant. I removed the code.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 223: boot_partition
Same comment as above. […]
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 284: me_rw_status
In fact, why is there a need to introduce a new ME RW status? I think this function can simply be: […]
The FW update driver checks for 3 conditions (ME_RW_VALID/INVALID/SIGN_MISMATCH). The partitcular function checks for RW_VALID/INVALID conditions.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 284: bool
Does this really need to be a bool? Can't we just return the state directly as the return value?
Yes, it requires since there is a check for error condition.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 313: ME: %s version = %d.%d.%d.%d
Each SoC has its own way of printing ME version. […]
All soc/cse_bp are logging cse version in same format(major.minor.hotfix.build). Currently, I see CSE version(one here, and another one in SoC) is being logged from 2 sources. I agree with you on having unified call which takes care of underlying ME Firmware SKU type, and logs firmware version. I will push new patchset accordingly.
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 214: BP_STATUS_MANIFEST_AND_INFO_OK
BP_STATUS_MANIFEST_AND_INFO_OK is no longer valid status code. I removed it.
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 216: I
nit: lowercase 'i'.
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 221: */
It would be good to mentioned that this is returned in response to MKHI command for GET_PARTITION_IN […]
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 222: bp_info_flags
I think it would be good to be consistent with the other enums i.e. […]
The flag represents value of cse_boot_partition_info struct. Hence the enum tag is named to match with it's struct.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 224: Boot Partition Info Flag
nit: All these are boot partition info flags and that is already indicated in the comment before the […]
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 232: RO
Done
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 239: BP_INFO_MIN_RECOV_MODE_EN
Why? Shouldn't this be BP_INFO_REDUNDANCY_EN?
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 253: ME_RW_SIGN_INVALID
ME_RW_INVALID/VALID are determined based on the boot partition status (BP_STATUS_GENERAL_FAILURE) Wh […]
These status codes are not required to be part of this file. I will use CSE FW update file itself.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 257: partition_fw_version
BP2 and BP3 always have same versions.But RO(BP1) and RW(BP2/BP3) can have different versions. […]
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 264: *
I see a number of functions being exposed here. […]
No, these functions won't be used outside of fw update driver. These functions will be used as helper functions by CSE FW Update driver.
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 267:
can/should?
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 270: Returns false on failure and true on success.
I agree with you on having common scheme for return values. […]
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 290: uint8_t
API returns member field "flags" of cse_boot_partition_info struct.
Done
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 316: bool
Error and disabled cases are handled differently in CSE FW Update library so modifying return values […]
Done
-
Sridhar Siricilla (Code Review)