Change in coreboot[master]: soc/intel/cannonlake: Allow coreboot to handle SPI lockdown

[Subrata Banik (Code Review)]

Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35299 )

Change subject: soc/intel/cannonlake: Allow coreboot to handle SPI lockdown
......................................................................

soc/intel/cannonlake: Allow coreboot to handle SPI lockdown

This patch disables FSP-S SPI lockdown UPDs and lets coreboot perform
SPI lockdown (i.e.flash register DLOCK, FLOCKDN, and WRSDIS before
end of post) in ramstage.

BUG=b:138200201
TEST=FSP debug build suggests those UPDs are disable now.

Change-Id: Id7a6b9859e058b9f1ec1bd45d2c388c02b8ac18c
Signed-off-by: Subrata Banik 
---
M src/soc/intel/cannonlake/fsp_params.c
1 file changed, 2 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35299/1

diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c
index 5cdddc6..9495f84 100644
--- a/src/soc/intel/cannonlake/fsp_params.c
+++ b/src/soc/intel/cannonlake/fsp_params.c
@@ -419,30 +419,24 @@
    	params->PchLockDownBiosLock = 0;
    	params->PchLockDownRtcMemoryLock = 0;
    	/*
-		 * TODO: Disable SpiFlashCfgLockDown config after FSP provides
-		 * dedicated UPD
-		 *
    	 * Skip SPI Flash Lockdown from inside FSP.
    	 * Making this config "0" means FSP won't set the FLOCKDN bit
    	 * of SPIBAR + 0x04 (i.e., Bit 15 of BIOS_HSFSTS_CTL).
    	 * So, it becomes coreboot's responsibility to set this bit
    	 * before end of POST for security concerns.
    	 */
-		// params->SpiFlashCfgLockDown = 0;
+		params->SpiFlashCfgLockDown = 0;
    } else {
    	tconfig->PchLockDownGlobalSmi = 1;
    	tconfig->PchLockDownBiosInterface = 1;
    	params->PchLockDownBiosLock = 1;
    	params->PchLockDownRtcMemoryLock = 1;
    	/*
-		 * TODO: Enable SpiFlashCfgLockDown config after FSP provides
-		 * dedicated UPD
-		 *
    	 * Enable SPI Flash Lockdown from inside FSP.
    	 * Making this config "1" means FSP will set the FLOCKDN bit
    	 * of SPIBAR + 0x04 (i.e., Bit 15 of BIOS_HSFSTS_CTL).
    	 */
-		// params->SpiFlashCfgLockDown = 1;
+		params->SpiFlashCfgLockDown = 1;
    }
 }

[Subrata Banik (Code Review)]

Hello Patrick Rudolph, Aamir Bohra, Tim Wawrzynczak, V Sowmya, build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/35299

to look at the new patch set (#2).

Change subject: soc/intel/cannonlake: Allow coreboot to handle SPI lockdown
......................................................................

soc/intel/cannonlake: Allow coreboot to handle SPI lockdown

This patch disables FSP-S SPI lockdown UPDs and lets coreboot perform
SPI lockdown (i.e.flash register DLOCK, FLOCKDN, and WRSDIS before
end of post) in ramstage.

BUG=b:138200201
TEST=FSP debug build suggests those UPDs are disable now.

Change-Id: Id7a6b9859e058b9f1ec1bd45d2c388c02b8ac18c
Signed-off-by: Subrata Banik 
---
M src/soc/intel/cannonlake/fsp_params.c
1 file changed, 6 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/35299/2

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35299 )

Change subject: soc/intel/cannonlake: Allow coreboot to handle SPI lockdown
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/35299/2/src/soc/intel/cannonlake/fs... 
File src/soc/intel/cannonlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/35299/2/src/soc/intel/cannonlake/fs... 
PS2, Line 421: #
Is the reason for using the preprocessor, that other SOC have different UPD?

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35299 )

Change subject: soc/intel/cannonlake: Allow coreboot to handle SPI lockdown
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35299/2/src/soc/intel/cannonlake/fs... 
File src/soc/intel/cannonlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/35299/2/src/soc/intel/cannonlake/fs... 
PS2, Line 421: #
> Is the reason for using the preprocessor, that other SOC have different UPD?

yes you got it right. Other SOCs like CNL, CFL, WHL doesn't have FSP UPD to skip SPI lockdown options and those products are in production now so code changes are not feasible hence added the support in CML and due to compilation issue guarding it.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35299 )

Change subject: soc/intel/cannonlake: Allow coreboot to handle SPI lockdown
......................................................................

Patch Set 2: Code-Review+2

[V Sowmya (Code Review)]

V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35299 )

Change subject: soc/intel/cannonlake: Allow coreboot to handle SPI lockdown
......................................................................

Patch Set 2: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35299 )

Change subject: soc/intel/cannonlake: Allow coreboot to handle SPI lockdown
......................................................................

soc/intel/cannonlake: Allow coreboot to handle SPI lockdown

This patch disables FSP-S SPI lockdown UPDs and lets coreboot perform
SPI lockdown (i.e.flash register DLOCK, FLOCKDN, and WRSDIS before
end of post) in ramstage.

BUG=b:138200201
TEST=FSP debug build suggests those UPDs are disable now.

Change-Id: Id7a6b9859e058b9f1ec1bd45d2c388c02b8ac18c
Signed-off-by: Subrata Banik 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/35299
Tested-by: build bot (Jenkins) 
Reviewed-by: Arthur Heymans 
Reviewed-by: Furquan Shaikh 
Reviewed-by: V Sowmya 
---
M src/soc/intel/cannonlake/fsp_params.c
1 file changed, 6 insertions(+), 8 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved
  Arthur Heymans: Looks good to me, but someone else must approve
  V Sowmya: Looks good to me, approved

diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c
index 06c556c..4038335 100644
--- a/src/soc/intel/cannonlake/fsp_params.c
+++ b/src/soc/intel/cannonlake/fsp_params.c
@@ -421,31 +421,29 @@
    	tconfig->PchLockDownBiosInterface = 0;
    	params->PchLockDownBiosLock = 0;
    	params->PchLockDownRtcMemoryLock = 0;
+#if CONFIG(SOC_INTEL_COMETLAKE)
    	/*
-		 * TODO: Disable SpiFlashCfgLockDown config after FSP provides
-		 * dedicated UPD
-		 *
    	 * Skip SPI Flash Lockdown from inside FSP.
    	 * Making this config "0" means FSP won't set the FLOCKDN bit
    	 * of SPIBAR + 0x04 (i.e., Bit 15 of BIOS_HSFSTS_CTL).
    	 * So, it becomes coreboot's responsibility to set this bit
    	 * before end of POST for security concerns.
    	 */
-		// params->SpiFlashCfgLockDown = 0;
+		params->SpiFlashCfgLockDown = 0;
+#endif
    } else {
    	tconfig->PchLockDownGlobalSmi = 1;
    	tconfig->PchLockDownBiosInterface = 1;
    	params->PchLockDownBiosLock = 1;
    	params->PchLockDownRtcMemoryLock = 1;
+#if CONFIG(SOC_INTEL_COMETLAKE)
    	/*
-		 * TODO: Enable SpiFlashCfgLockDown config after FSP provides
-		 * dedicated UPD
-		 *
    	 * Enable SPI Flash Lockdown from inside FSP.
    	 * Making this config "1" means FSP will set the FLOCKDN bit
    	 * of SPIBAR + 0x04 (i.e., Bit 15 of BIOS_HSFSTS_CTL).
    	 */
-		// params->SpiFlashCfgLockDown = 1;
+		params->SpiFlashCfgLockDown = 1;
+#endif
    }
 }