Change in coreboot[master]: mb/intel/saddlebrook: Enable Chipset_lockdown coreboot config

[PraveenX Hodagatta Pranesh (Code Review)]

PraveenX Hodagatta Pranesh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36451 )

Change subject: mb/intel/saddlebrook: Enable Chipset_lockdown coreboot config
......................................................................

mb/intel/saddlebrook: Enable Chipset_lockdown coreboot config

This patch enables lockdown configuration for saddlebrook platform

BUG=None
TEST=Boot to Linux on saddlebrook and verified MRC is restored on warm, cold,
    resume boot path's.

Change-Id: Ia324c118b0c8e72b66a757dee5be43ba79abbeab
Signed-off-by: Praveen Hodagatta Pranesh 
---
M src/mainboard/intel/saddlebrook/devicetree.cb
1 file changed, 5 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/36451/1

diff --git a/src/mainboard/intel/saddlebrook/devicetree.cb b/src/mainboard/intel/saddlebrook/devicetree.cb
index 385a4be..f6727ef 100644
--- a/src/mainboard/intel/saddlebrook/devicetree.cb
+++ b/src/mainboard/intel/saddlebrook/devicetree.cb
@@ -61,6 +61,11 @@

register "serirq_mode" = "SERIRQ_CONTINUOUS"

+	# Lock Down
+	register "common_soc_config" = "{
+	.chipset_lockdown = CHIPSET_LOCKDOWN_COREBOOT,
+	}"
+
    # VR Settings Configuration for 4 Domains
    #+----------------+-----------+-----------+-------------+----------+
    #| Domain/Setting |     SA    |    IA     | GT Unsliced |    GT    |

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36451 )

Change subject: mb/intel/saddlebrook: Enable Chipset_lockdown coreboot config
......................................................................

Patch Set 1: Code-Review+2

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36451 )

Change subject: mb/intel/saddlebrook: Enable Chipset_lockdown coreboot config
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

Again, does this mean the FSP path is broken? Then we should remove that
as well.

https://review.coreboot.org/c/coreboot/+/36451/1/src/mainboard/intel/saddleb... 
File src/mainboard/intel/saddlebrook/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/36451/1/src/mainboard/intel/saddleb... 
PS1, Line 66: 	.chipset_lockdown = CHIPSET_LOCKDOWN_COREBOOT,
Please, add a tab.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36451 )

Change subject: mb/intel/saddlebrook: Enable Chipset_lockdown coreboot config
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36451/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/36451/1//COMMIT_MSG@12 
PS1, Line 12: verified MRC is restored on warm, cold,
            : 	resume boot path's
What does that have to do with the lockdown?

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36451 )

Change subject: mb/intel/saddlebrook: Enable Chipset_lockdown coreboot config
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36451/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/36451/1//COMMIT_MSG@12 
PS1, Line 12: verified MRC is restored on warm, cold,
            : 	resume boot path's
> What does that have to do with the lockdown?

Lockdown can affect everything. Especially in the presence of undocumented
blobs. For instance, if locking happens too early, a register write may fail
silently. In this case, I'd assume that the MRC cache can't be written if
some flash locking is out of sync.

[PraveenX Hodagatta Pranesh (Code Review)]

Hello Michael Niewöhner, Teo Boon Tiong, build bot (Jenkins), Nico Huber, Patrick Georgi,

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

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

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

Change subject: mb/intel/saddlebrook: Enable Chipset_lockdown coreboot config
......................................................................

mb/intel/saddlebrook: Enable Chipset_lockdown coreboot config

This patch enables lockdown configuration for saddlebrook platform

BUG=None
TEST=Boot to Linux on saddlebrook and verified MRC is restored on warm, cold,
    resume boot path's.

Change-Id: Ia324c118b0c8e72b66a757dee5be43ba79abbeab
Signed-off-by: Praveen Hodagatta Pranesh 
---
M src/mainboard/intel/saddlebrook/devicetree.cb
1 file changed, 5 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/36451/2

[PraveenX Hodagatta Pranesh (Code Review)]

PraveenX Hodagatta Pranesh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36451 )

Change subject: mb/intel/saddlebrook: Enable Chipset_lockdown coreboot config
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/36451/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/36451/1//COMMIT_MSG@12 
PS1, Line 12: verified MRC is restored on warm, cold,
            : 	resume boot path's
> Lockdown can affect everything. Especially in the presence of undocumented […]

Nico is right. without this change MRC cache not able to update  and restore on second boot due to following error "SPI Transaction Error at Flash Offset 9e0000 HSFSTS".

https://review.coreboot.org/c/coreboot/+/36451/1/src/mainboard/intel/saddleb... 
File src/mainboard/intel/saddlebrook/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/36451/1/src/mainboard/intel/saddleb... 
PS1, Line 66: 	.chipset_lockdown = CHIPSET_LOCKDOWN_COREBOOT,
> Please, add a tab.

Done

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36451 )

Change subject: mb/intel/saddlebrook: Enable Chipset_lockdown coreboot config
......................................................................

Patch Set 2: Code-Review+2

[Patrick Georgi (Code Review)]

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

Change subject: mb/intel/saddlebrook: Enable Chipset_lockdown coreboot config
......................................................................

mb/intel/saddlebrook: Enable Chipset_lockdown coreboot config

This patch enables lockdown configuration for saddlebrook platform

BUG=None
TEST=Boot to Linux on saddlebrook and verified MRC is restored on warm, cold,
    resume boot path's.

Change-Id: Ia324c118b0c8e72b66a757dee5be43ba79abbeab
Signed-off-by: Praveen Hodagatta Pranesh 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/36451
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
---
M src/mainboard/intel/saddlebrook/devicetree.cb
1 file changed, 5 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved

diff --git a/src/mainboard/intel/saddlebrook/devicetree.cb b/src/mainboard/intel/saddlebrook/devicetree.cb
index 385a4be..7d7b58b 100644
--- a/src/mainboard/intel/saddlebrook/devicetree.cb
+++ b/src/mainboard/intel/saddlebrook/devicetree.cb
@@ -61,6 +61,11 @@

register "serirq_mode" = "SERIRQ_CONTINUOUS"

+	# Lock Down
+	register "common_soc_config" = "{
+		.chipset_lockdown = CHIPSET_LOCKDOWN_COREBOOT,
+	}"
+
    # VR Settings Configuration for 4 Domains
    #+----------------+-----------+-----------+-------------+----------+
    #| Domain/Setting |     SA    |    IA     | GT Unsliced |    GT    |