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 59:
(8 comments)
I see comments from PS44 haven't been completely addressed yet.
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 37: boot_partition_status
This should be fixed width here since it is being used in a packed structure sent/received from CSE.
This hasn't been addressed yet?
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 64: static struct cse_boot_partition_info cse_bp_info;
"cse_bp_info" is made global to retain the boot partition state, and update it when only there a cha […]
Ack
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 116: cse_send_set_boot_partition_info_cmd
_info_ in the name is not correct. Just cse_send_set_bp(... […]
Not addressed?
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 137: switch_req.next_bp = partition_id;
Just set this when initialize switch_req above in L124?
Not addressed?
https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/block... PS44, Line 164: *
space before *
Not addressed?
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;
I think all of this can be just reduced to: […]
Not addressed?
https://review.coreboot.org/c/coreboot/+/35402/59/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_bp.c:
https://review.coreboot.org/c/coreboot/+/35402/59/src/soc/intel/common/block... PS59, Line 31: boot_partition_status Please see comment on patchset 44.
https://review.coreboot.org/c/coreboot/+/35402/59/src/soc/intel/common/block... PS59, Line 65: ; Since the memset is removed now, you would need the initialization here: = { .hdr.group_id = MKHI_GROUP_ID_BUP_COMMON, .hdr.command = MKHI_BUP_COMMON_GET_BOOT_PARTITION_INFO_CMD_REQ, .reserved = {0}, };