Attention is currently required from: Nico Huber, Subrata Banik, Tim Wawrzynczak, Patrick Rudolph. Sridhar Siricilla 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/64328438_5ebc080f PS10, Line 13: rebuilding the coreboot.
`rebuilding coreboot. […]
Ack
https://review.coreboot.org/c/coreboot/+/61380/comment/1e794313_1141667f 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
Ack
File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/61380/comment/9bf76d6e_3b4883a8 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? […]
Ack
File src/soc/intel/common/block/debug/debug_feature.c:
https://review.coreboot.org/c/coreboot/+/61380/comment/7afcbacf_b339a85d PS10, Line 14: /* Byte location: 0xF00 */
nit: this comment seems redundant, you have a well-named #define for this
If pre_mem_ft member list grows up, then it might be helpful!
https://review.coreboot.org/c/coreboot/+/61380/comment/c477f0ed_ecf71e7e 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 n […]
When DEBUG_FEATURE Kconfig is enabled, pre_mem_debug.cse_fw_update_disable becomes 0xFF, but expectation is 1 in order to disable the cse fw update. Please note each feature occupies 1 byte, not 1 bit.
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?
This is not intended, please see my above comment.