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 14:
(24 comments)
https://review.coreboot.org/c/coreboot/+/35402/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35402/6//COMMIT_MSG@7 PS6, Line 7: APIs
No, these are news APIs and will be supported 14.0 onwards. […]
Done
https://review.coreboot.org/c/coreboot/+/35402/6//COMMIT_MSG@9 PS6, Line 9: cse
CSE
Done
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... File src/soc/intel/common/block/cse/Kconfig:
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 8: MAX_ME_PARTITIONS
CSE_MAX_BOOT_PARTITIONS
Done
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block/... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block/... PS6, Line 92: #define BUP_COMMON_GET_BOOT_PARTITION_INFO_CMD_REQ 0x1C : #define BUP_COMMON_SET_BOOT_PARTITION_CMD_REQ 0x1D
I think moving command ids to header file is not required!
Done
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block/... PS6, Line 779: +1
Boot Partition indexing starts from 0. […]
Done
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 72: MKHI_GROUP_ID_HMRFPO
I updated commit message.
Done
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 73:
Use tabs for consistency.
Done
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 86: #define ME_HFS_CWS_NORMAL 5 : #define ME_HFS_MODE_NORMAL 0 : #define ME_HFS_TEMP_DISABLE 3 : #define HECI_OP_MODE_SEC_OVERRIDE 5
I will address this in separate patch.
Done
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 91: *
Space before *
Done
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 91: Boot partition info and switching command Ids
Done.
Done
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 92:
Done
Done
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 93:
Done
Done
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 99:
Removed
Done
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 749: Partition Info resp Failed
Done
Moved the code into cse_bp.c, and made changes in it.
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 757: Partition
Done
Moved the code into cse_bp.c, and made changes in it.
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 770: struct set_boot_partition_info_resp { : struct mkhi_hdr hdr; : } __packed;
Done
Done
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 802: if (!cse_get_boot_partition_info(&resp)) : goto failed;
Removed
Done
https://review.coreboot.org/c/coreboot/+/35402/14/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_bp.c:
https://review.coreboot.org/c/coreboot/+/35402/14/src/soc/intel/common/block... PS14, Line 60: cse_bp_init The scope of the variable can be limited to function.
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 154:
Done
Done
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 160: typedef
Since we are using the enum type in boot_partition_info_entry sturcture, so typedef is required.
Done
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 161:
Done
Done
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 167: partition_fw_version
Done
Done
https://review.coreboot.org/c/coreboot/+/35402/12/src/soc/intel/common/block... PS12, Line 210: /* : * Next requested boot partition is updated when : * set_cse_next_boot_partition() is invoked. : */
I have modified the comment. The field always points to next boot partition of CSE. […]
Done
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block/... PS6, Line 212: /* Retrieves boot partitions Info CSE */
it could be done, but we would have to add a static/global structure to store the response which cou […]
Done
-
Sridhar Siricilla (Code Review)