Attention is currently required from: Nico Huber, Subrata Banik, Sridhar Siricilla, Patrick Rudolph. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61380 )
Change subject: soc/intel/common: Add support to control coreboot and Intel SoC features ......................................................................
Patch Set 10:
(5 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/61380/comment/0263ff91_4096625e PS10, Line 13: rebuilding the coreboot. `rebuilding coreboot.`
https://review.coreboot.org/c/coreboot/+/61380/comment/a04b6edc_3d81128c PS10, Line 17: The OEM section starts from offset:0xf00 till end of the Descriptor : Region(0xfff). Either bring up to the previous line or add a blank line between paragraphs
File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/61380/comment/ff2759a2_cc32cae7 PS10, Line 668: if (CONFIG(SOC_INTEL_COMMON_BLOCK_DEBUG_FEATURE) && CONFIG(SOC_INTEL_CSE_RW_UPDATE)) : return !is_debug_cse_fw_update_disable(); : : return CONFIG(SOC_INTEL_CSE_RW_UPDATE); suggestion, does this make the logic a little more clear?
``` if (!CONFIG(SOC_INTEL_CSE_RW_UPDATE)) return false;
if (CONFIG(SOC_INTEL_COMMON_BLOCK_DEBUG_FEATURE) return !is_debug_cse_fw_update_disable();
return true; ```
File src/soc/intel/common/block/debug/debug_feature.c:
https://review.coreboot.org/c/coreboot/+/61380/comment/2c16aa79_8a2c358c PS10, Line 14: /* Byte location: 0xF00 */ nit: this comment seems redundant, you have a well-named #define for this
https://review.coreboot.org/c/coreboot/+/61380/comment/6910d3c8_1ec5b1bb PS10, Line 28: pre_mem_debug.cse_fw_update_disable == 1; It seems the default value for the OEM section is 0xFFFF_FFFF, so if I am thinking correctly, on a normal image, as soon as you enable the DEBUG_FEATURE Kconfig, the CSE RW update will be disabled? As in there is extra work required (go and set that bit in the descriptor) to enable CSE RW update once you enable the Kconfig?