Furquan Shaikh 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 6:
(11 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 Are these APIs supported by all platforms?
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. Line 74 is very different than line 77 and this defintion.
Also, can you please put all these group ids together.
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 expectations after this function is done? Does it need a reset?
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.
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block/... PS6, Line 779: +1 Why add 1?
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 147: MAX_ME_PARTITIONS Is this true for all platforms?
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block/... PS6, Line 150: BP_STATUS_SUCCESS = 0, : BP_STATUS_GENERAL_FAILURE = 1, : BP_STATUS_PARTITION_NOT_PRESENT = 2, : BP_STATUS_HOST_REQUEST_FOR_PARTITION_SWITCH = 3,
consistent spacing?
Can you please add comments indicating what each really means?
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. And all platforms using this should get rid of the redundant declarations.
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.
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.What is the return value? What is the input parameter? Is it necessary to expose get_boot_partition_info_resp?
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block/... PS6, Line 215: /* Sets CSE's next boot partition to boot */ what are the return values? What are the valid values for input parameter?
-
Furquan Shaikh (Code Review)