Paul Menzel 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 40:
(22 comments)
https://review.coreboot.org/c/coreboot/+/35402/40//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35402/40//COMMIT_MSG@11 PS40, Line 11: chrome Chromium OS?
https://review.coreboot.org/c/coreboot/+/35402/40//COMMIT_MSG@18 PS40, Line 18: the coreboot Remove the article.
https://review.coreboot.org/c/coreboot/+/35402/40//COMMIT_MSG@19 PS40, Line 19: partition(either BP1(RO) or BP2) Please add a space after *partition* before (.
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse_bp.c:
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 23: */ Please add a space before.
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 47: uint8_t Why not `unsigned int`?
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 64: bool Use CB_SUCCESS and friends?
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 89: printk(BIOS_ERR, "Get partition info resp failed:%d\n", info_resp.hdr.result); Please add a space after the colon.
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 105: Remove the blank line.
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 108: fail failed
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 135: Command(BP%d) Please add a space before (.
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 146: printk(BIOS_ERR, "Set Boot Partion Info Response Failed:%d\n", Ditto.
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 182: return false; Above you print an error.
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 305: No Number
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 308: =0x%x Please add a space.
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 172: Enabled lowercase
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 174: * So, CSE Image contains 3 partitions when redundancy is enabled Period/dot at the end?
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 180: Remove blank line.
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 183: * and it's a valid bootable partition. Fits on one line?
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 195: * not a bootable partition on it's own. One line?
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 202: Remove blank line.
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 262: }; Might this be available in other parts of coreboot already?
https://review.coreboot.org/c/coreboot/+/35402/40/src/soc/intel/common/block... PS40, Line 274: partition(BP1 Space.