Change in coreboot[master]: soc/inte/common: Add support to control coreboot and Intel SoC features
Attention is currently required from: Nico Huber, Subrata Banik, Patrick Rudolph. Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61380 )
Change subject: soc/inte/common: Add support to control coreboot and Intel SoC features ......................................................................
Patch Set 2:
(1 comment)
File src/soc/intel/common/block/cse/cse_lite.c:
https://review.coreboot.org/c/coreboot/+/61380/comment/9d7f8e1b_afbd16e2 PS2, Line 666: is_cse_fw_update_enabled
- SOC_INTEL_COMMON_BLOCK_DEBUG_FEATURE & SOC_INTEL_CSE_RW_UPDATE are enabled -> decision is based […]
Do you mean `&` (bitwise) or `&&` (logical) ?
Here I am referring English word "and".
So, you will always have SOC_INTEL_CSE_RW_UPDATE on platform and if would like to disable RW update then will perform override from strap and select SOC_INTEL_COMMON_BLOCK_DEBUG_FEATURE ?
Need not be the case always. Initially we don't enable CSE firmware Update(SOC_INTEL_CSE_RW_UPDATE) at least beginning of the program. But, we would like to enable SOC_INTEL_COMMON_BLOCK_DEBUG_FEATURE from the beginning of the program and remove the SOC_INTEL_COMMON_BLOCK_DEBUG_FEATURE KConfig during FSI.
IMO, you don't need additional SOC_INTEL_COMMON_BLOCK_DEBUG_FEATURE to guard the soft strap read operation.
On a production system, descriptor region is RO, so SOC_INTEL_COMMON_BLOCK_DEBUG_FEATURE flag is not enabled at all on production systems. Hence, guard is required. Also, please note SOC_INTEL_COMMON_BLOCK_DEBUG_FEATURE will be used to control other SOC/coreboot features as well going forward.
Hence, I prefer the flow which I mentioned in my earlier comment. Agree?
-
Sridhar Siricilla (Code Review)