Attention is currently required from: Tim Wawrzynczak, Nick Vaccaro, Arthur Heymans. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63518 )
Change subject: soc/intel/common/{sa, adl}: Add `finalize` operation for systemagent ......................................................................
Patch Set 7:
(1 comment)
File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/63518/comment/a074f0e4_90a7d1ae PS7, Line 327: if (!CONFIG(USE_FSP_NOTIFY_PHASE_END_OF_FIRMWARE)) : sa_lock_pam();
What about the lockdown_config() == CHIPSET_LOCKDOWN_COREBOOT on line 79? Shouldn't this code take that into account as well?
There potentially could be three cases as below:
1. SoC user decides to call FSP Notify Phase API but like to keep `common_config->chipset_lockdown = CHIPSET_LOCKDOWN_COREBOOT`
2. The SoC user decides to call FSP Notify Phase API and to keep `common_config->chipset_lockdown = CHIPSET_LOCKDOWN_FSP`
3. SoC user decides to skip FSP Notify Phase API
For #1:soc/intel/alderlake/finalize.c function sa_finalize will perform the PAM register lock.
For #2:FSP takes care of locking using FSP notify phase 2 API
For #3:This code change src/soc/intel/common/block/systemagent/systemagent.c#325 ensures to lockdown PAM registers using common code, so, we don't need to implement the SoC specific implementation going forward for MTL for example.
Also, PAM register attribute is RW/L, once programmed by IA common code as part of `.final` callback, SoC specific implementation doesn't have any significance.