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 10:
(11 comments)
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 89: MKHI_BUP_COMMON_GROUP_ID
Can you please ensure that the naming is consistent. […]
Done
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
can we move these commands IDs and ones above into a separate header file and refer it from there? N […]
I think moving command ids to header file is not required!
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block/... PS6, Line 731: int
unsigned int?
Done
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block/... PS6, Line 760: set_cse_next_boot_partition
What exactly is this function doing? Under what conditions can this function be called? What are the […]
It sends HECI command to notify CSE about it's next boot partition to boot from on next reset. Whenever coreboot wants CSE to boot from certain partition(CSE valid boot partition ids are RO_BP1 (0), RW_BP2(1)), then this function be used. Global Reset( Global Reset or CSE Reset) should be issued after marking next boot partition of ME. This function must be used before EOP.
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block/... PS6, Line 760: int
same.
Done
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block/... PS6, Line 774: .group_id = MKHI_BUP_COMMON_GROUP_ID,
Move this to next line.
Done
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block/... PS6, Line 779: +1
Why add 1?
Boot Partition indexing starts from 0. We are referring partition names as BP1,BP2 & BP3, so as to align with the naming, I'm logging partition names accordingly.
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 143: #define RO_BP1 0 : #define RW_BP2 1 : #define RW_BP3 2
can we have it as enum as well would help it bundle under Boot partition context or add comment here […]
Done
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block/... PS6, Line 175: mkhi_hdr
This should be added as a separate CL. […]
Agreed.
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block/... PS6, Line 183: /* Boot Partition Info Flag Redundancy Enabled */ : #define BP_INFO_REDUNDANCY_EN (1 << 0) : : /* Boot Partition Info Flag Min. Recovery Mode Enabled */ : #define BP_INFO_MIN_RECO_MODE_EN (1 << 1) : : /* Boot Partition Info Flag Read only Config Enabled */ : #define BP_INFO_READ_ONLY_CFG (1 << 2)
It is not clear to me from the comments what the flags really mean.
In order to support CSE FW update functionality with resilience, the CSE FW logically divided into RO, and RW partitions. The RO will contain minimum firmware to boot the platform. RW will contain boot capability and other functionality.The minimal boot functionality is redundant in RO and RW. This is indicated by BP_INFO_REDUNDANCY_EN, and BP_INFO_MIN_RECO_MODE_EN. BP_INFO_READ_ONLY_CFG indicates HW protection to RO region.
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block/... PS6, Line 212: /* Retrieves boot partitions Info CSE */
Can you please add more details as to what this function really does. […]
The get_boot_partition_info_resp is utilized in multiple places at different points in the update flow, and having the response info available saves sending same command multiple times.
-
Sridhar Siricilla (Code Review)