Change in ...coreboot[master]: mb/google/hatch: Set UPD to unlock GPP_A12 to use FPMCU_RST

[Krishna P Bhat D (Code Review)]

Krishna P Bhat D has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32110


Change subject: mb/google/hatch: Set UPD to unlock GPP_A12 to use FPMCU_RST
......................................................................

mb/google/hatch: Set UPD to unlock GPP_A12 to use FPMCU_RST

GPP_A12 is being GPIO padlocked and cannot used in kernel. Unlock the
GPIO pads to export this pin in kernel to be used as FPMCU_RST.
GPP_A_12 has a Native3 (SX_EXIT_HOLDOFF#) mode, which allows to delay
resuming to S0. If this pad is not locked and platform was not initially
designed for this functionality, malware could reconfigure this pads
setting under OS (switch to Native3), which would make platform not able
to resume until G3 is applied. To prevent misuse of this pad,
re-configure this pad before entering S3 and S5 to guarantee that the
pad configuration is correct.

BUG=b:128686027

Change-Id: Iad9e8a209dc3f8ca0c994e8c1da329918409a1d4
Signed-off-by: Krishna Prasad Bhat <[e-mail]>
---
M src/mainboard/google/hatch/variants/baseboard/gpio.c
M src/soc/intel/cannonlake/fsp_params.c
2 files changed, 9 insertions(+), 1 deletion(-)



  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/32110/1

diff --git a/src/mainboard/google/hatch/variants/baseboard/gpio.c b/src/mainboard/google/hatch/variants/baseboard/gpio.c
index b974e49..8529d5916 100644
--- a/src/mainboard/google/hatch/variants/baseboard/gpio.c
+++ b/src/mainboard/google/hatch/variants/baseboard/gpio.c
@@ -411,8 +411,12 @@
 	return gpio_table;
 }
 
-/* Default GPIO settings before entering sleep. */
+/*
+ * Default GPIO settings before entering sleep. Configure A12: FPMCU_RST_ODL
+ * as GPO before entering sleep.
+ */
 static const struct pad_config default_sleep_gpio_table[] = {
+	PAD_CFG_GPO(GPP_A12, 1, DEEP), /* FPMCU_RST_ODL */
 };
 
 /*
@@ -421,6 +425,7 @@
  * turn off EN_PP3300_WWAN.
  */
 static const struct pad_config s5_sleep_gpio_table[] = {
+	PAD_CFG_GPO(GPP_A12, 1, DEEP), /* FPMCU_RST_ODL */
 	PAD_CFG_GPO(GPP_A18, 0, DEEP), /* EN_PP3300_WWAN */
 };
 
diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c
index 6173403..57d004f 100644
--- a/src/soc/intel/cannonlake/fsp_params.c
+++ b/src/soc/intel/cannonlake/fsp_params.c
@@ -328,6 +328,9 @@
 
 	/* Set TccActivationOffset */
 	tconfig->TccActivationOffset = config->tcc_offset;
+
+	/* Unlock all GPIO pads */
+	tconfig->PchUnlockGpioPads = 1;
 }
 
 /* Mainboard GPIO Configuration */

-- 
To view, visit https://review.coreboot.org/c/coreboot/+/32110
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Iad9e8a209dc3f8ca0c994e8c1da329918409a1d4
Gerrit-Change-Number: 32110
Gerrit-PatchSet: 1
Gerrit-Owner: Krishna P Bhat D <[e-mail]>
Gerrit-MessageType: newchange

Change in ...coreboot[master]: mb/google/hatch: Set UPD to unlock GPP_A12 to use as FPMCU_RST

[Krishna P Bhat D (Code Review)]

Hello Patrick Rudolph, 

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

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

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

Change subject: mb/google/hatch: Set UPD to unlock GPP_A12 to use as FPMCU_RST
......................................................................

mb/google/hatch: Set UPD to unlock GPP_A12 to use as FPMCU_RST

GPP_A12 is being GPIO padlocked and cannot used in kernel. Unlock the
GPIO pads to export this pin in kernel to be used as FPMCU_RST.
GPP_A_12 has a Native3 (SX_EXIT_HOLDOFF#) mode, which allows to delay
resuming to S0. If this pad is not locked and platform was not initially
designed for this functionality, malware could reconfigure this pads
setting under OS (switch to Native3), which would make platform not able
to resume until G3 is applied. To prevent misuse of this pad,
re-configure this pad before entering S3 and S5 to guarantee that the
pad configuration is correct.

BUG=b:128686027

Change-Id: Iad9e8a209dc3f8ca0c994e8c1da329918409a1d4
Signed-off-by: Krishna Prasad Bhat <[e-mail]>
---
M src/mainboard/google/hatch/variants/baseboard/gpio.c
M src/soc/intel/cannonlake/fsp_params.c
2 files changed, 9 insertions(+), 1 deletion(-)


  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/32110/2
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/32110
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Iad9e8a209dc3f8ca0c994e8c1da329918409a1d4
Gerrit-Change-Number: 32110
Gerrit-PatchSet: 2
Gerrit-Owner: Krishna P Bhat D <[e-mail]>
Gerrit-Reviewer: Krishna P Bhat D <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-MessageType: newpatchset

Change in ...coreboot[master]: mb/google/hatch: Set UPD to unlock GPP_A12 to use as FPMCU_RST

[Furquan Shaikh (Code Review)]

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

Change subject: mb/google/hatch: Set UPD to unlock GPP_A12 to use as FPMCU_RST
......................................................................


Patch Set 2:

Can you please split this into two changes -- one for SoC and other for mainboard?


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/32110
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Iad9e8a209dc3f8ca0c994e8c1da329918409a1d4
Gerrit-Change-Number: 32110
Gerrit-PatchSet: 2
Gerrit-Owner: Krishna P Bhat D <[e-mail]>
Gerrit-Reviewer: Furquan Shaikh <[e-mail]>
Gerrit-Reviewer: Krishna P Bhat D <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Rizwan Qureshi <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Thu, 28 Mar 2019 17:21:24 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in ...coreboot[master]: soc/intel/cannonlake: Set FSP UPD to unlock GPP_A12 to use as FPMCU_RST

[Krishna P Bhat D (Code Review)]

Hello Patrick Rudolph, Tim Wawrzynczak, Rizwan Qureshi, Shelley Chen, build bot (Jenkins), Furquan Shaikh, 

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

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

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

Change subject: soc/intel/cannonlake: Set FSP UPD to unlock GPP_A12 to use as FPMCU_RST
......................................................................

soc/intel/cannonlake: Set FSP UPD to unlock GPP_A12 to use as FPMCU_RST

GPP_A12 is being GPIO padlocked and cannot used in kernel. Unlock the
GPIO pads to export this pin in kernel to be used as FPMCU_RST.

BUG=b:128686027

Change-Id: Iad9e8a209dc3f8ca0c994e8c1da329918409a1d4
Signed-off-by: Krishna Prasad Bhat <[e-mail]>
---
M src/soc/intel/cannonlake/fsp_params.c
1 file changed, 3 insertions(+), 0 deletions(-)


  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/32110/3
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/32110
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Iad9e8a209dc3f8ca0c994e8c1da329918409a1d4
Gerrit-Change-Number: 32110
Gerrit-PatchSet: 3
Gerrit-Owner: Krishna P Bhat D <[e-mail]>
Gerrit-Reviewer: Furquan Shaikh <[e-mail]>
Gerrit-Reviewer: Krishna P Bhat D <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Rizwan Qureshi <[e-mail]>
Gerrit-Reviewer: Shelley Chen <[e-mail]>
Gerrit-Reviewer: Tim Wawrzynczak <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-MessageType: newpatchset

Change in ...coreboot[master]: soc/intel/cannonlake: Set FSP UPD to unlock GPP_A12 to use as FPMCU_RST

[Krishna P Bhat D (Code Review)]

Krishna P Bhat D has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32110 )

Change subject: soc/intel/cannonlake: Set FSP UPD to unlock GPP_A12 to use as FPMCU_RST
......................................................................


Patch Set 3:
> Patch Set 2:
>
> Can you please split this into two changes -- one for SoC and other for mainboard?

Done. The other CL is https://review.coreboot.org/c/coreboot/+/32111/


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/32110
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Iad9e8a209dc3f8ca0c994e8c1da329918409a1d4
Gerrit-Change-Number: 32110
Gerrit-PatchSet: 3
Gerrit-Owner: Krishna P Bhat D <[e-mail]>
Gerrit-Reviewer: Furquan Shaikh <[e-mail]>
Gerrit-Reviewer: Krishna P Bhat D <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Rizwan Qureshi <[e-mail]>
Gerrit-Reviewer: Shelley Chen <[e-mail]>
Gerrit-Reviewer: Tim Wawrzynczak <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Thu, 28 Mar 2019 17:49:42 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in ...coreboot[master]: soc/intel/cannonlake: Set FSP UPD to unlock GPP_A12 to use as FPMCU_RST

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/cannonlake: Set FSP UPD to unlock GPP_A12 to use as FPMCU_RST
......................................................................


Patch Set 3:

(3 comments)

https://review.coreboot.org/#/c/32110/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32110/3//COMMIT_MSG@7 
PS3, Line 7: GPP_A12
Isn't this all GPIOs?


https://review.coreboot.org/#/c/32110/3//COMMIT_MSG@7 
PS3, Line 7: FPMCU_RST
This doesn't apply to all CNL boards.


https://review.coreboot.org/#/c/32110/3//COMMIT_MSG@9 
PS3, Line 9: GPP_A12 is being GPIO padlocked and cannot used in kernel. Unlock the
           : GPIO pads to export this pin in kernel to be used as FPMCU_RST.
This is specific to hatch.



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/32110
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Iad9e8a209dc3f8ca0c994e8c1da329918409a1d4
Gerrit-Change-Number: 32110
Gerrit-PatchSet: 3
Gerrit-Owner: Krishna P Bhat D <[e-mail]>
Gerrit-Reviewer: Furquan Shaikh <[e-mail]>
Gerrit-Reviewer: Krishna P Bhat D <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Rizwan Qureshi <[e-mail]>
Gerrit-Reviewer: Shelley Chen <[e-mail]>
Gerrit-Reviewer: Tim Wawrzynczak <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Thu, 28 Mar 2019 23:35:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in ...coreboot[master]: soc/intel/cannonlake: Add FSP UPD to unlock GPIO pads in devicetree

[Krishna P Bhat D (Code Review)]

Hello Patrick Rudolph, Tim Wawrzynczak, Rizwan Qureshi, Shelley Chen, build bot (Jenkins), Furquan Shaikh, 

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

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

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

Change subject: soc/intel/cannonlake: Add FSP UPD to unlock GPIO pads in devicetree
......................................................................

soc/intel/cannonlake: Add FSP UPD to unlock GPIO pads in devicetree

FSP has a UPD to unlock all GPIO pads. This parameter is disabled by
default. Add a chip parameter so that GPIO pads can be unlocked on mainboard
level in devicetree and therefore this feature can be used if needed.

BUG=b:128686027

Change-Id: Iad9e8a209dc3f8ca0c994e8c1da329918409a1d4
Signed-off-by: Krishna Prasad Bhat <[e-mail]>
---
M src/soc/intel/cannonlake/chip.h
M src/soc/intel/cannonlake/fsp_params.c
2 files changed, 6 insertions(+), 0 deletions(-)


  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/32110/4
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/32110
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Iad9e8a209dc3f8ca0c994e8c1da329918409a1d4
Gerrit-Change-Number: 32110
Gerrit-PatchSet: 4
Gerrit-Owner: Krishna P Bhat D <[e-mail]>
Gerrit-Reviewer: Furquan Shaikh <[e-mail]>
Gerrit-Reviewer: Krishna P Bhat D <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Rizwan Qureshi <[e-mail]>
Gerrit-Reviewer: Shelley Chen <[e-mail]>
Gerrit-Reviewer: Tim Wawrzynczak <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-MessageType: newpatchset

Change in ...coreboot[master]: soc/intel/cannonlake: Add FSP UPD to unlock GPIO pads in devicetree

[Krishna P Bhat D (Code Review)]

Krishna P Bhat D has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32110 )

Change subject: soc/intel/cannonlake: Add FSP UPD to unlock GPIO pads in devicetree
......................................................................


Patch Set 3:

(3 comments)

https://review.coreboot.org/#/c/32110/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32110/3//COMMIT_MSG@7 
PS3, Line 7: FPMCU_RST
> This doesn't apply to all CNL boards.
> Since it does not apply to all CNL boards, we can do it in board specific devicetree.cb.

https://review.coreboot.org/#/c/32110/3//COMMIT_MSG@7 
PS3, Line 7: GPP_A12
> Isn't this all GPIOs?
> Yes. Have changed to indicate it is all GPIOs.

https://review.coreboot.org/#/c/32110/3//COMMIT_MSG@9 
PS3, Line 9: GPP_A12 is being GPIO padlocked and cannot used in kernel. Unlock the
           : GPIO pads to export this pin in kernel to be used as FPMCU_RST.
> This is specific to hatch.
> Have pushed a hatch mainboard specific patch here. https://review.coreboot.org/c/coreboot/+/32126

Change in ...coreboot[master]: soc/intel/cannonlake: Add FSP UPD to unlock GPIO pads in devicetree

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/cannonlake: Add FSP UPD to unlock GPIO pads in devicetree
......................................................................


Patch Set 4: Code-Review+2


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/32110
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Iad9e8a209dc3f8ca0c994e8c1da329918409a1d4
Gerrit-Change-Number: 32110
Gerrit-PatchSet: 4
Gerrit-Owner: Krishna P Bhat D <[e-mail]>
Gerrit-Reviewer: Furquan Shaikh <[e-mail]>
Gerrit-Reviewer: Krishna P Bhat D <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Rizwan Qureshi <[e-mail]>
Gerrit-Reviewer: Shelley Chen <[e-mail]>
Gerrit-Reviewer: Tim Wawrzynczak <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Sat, 30 Mar 2019 17:17:06 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

Change in ...coreboot[master]: soc/intel/cannonlake: Add FSP UPD to unlock GPIO pads in devicetree

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32110 )

Change subject: soc/intel/cannonlake: Add FSP UPD to unlock GPIO pads in devicetree
......................................................................

soc/intel/cannonlake: Add FSP UPD to unlock GPIO pads in devicetree

FSP has a UPD to unlock all GPIO pads. This parameter is disabled by
default. Add a chip parameter so that GPIO pads can be unlocked on mainboard
level in devicetree and therefore this feature can be used if needed.

BUG=b:128686027

Change-Id: Iad9e8a209dc3f8ca0c994e8c1da329918409a1d4
Signed-off-by: Krishna Prasad Bhat <[e-mail]>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32110
Tested-by: build bot (Jenkins) <[e-mail]>
Reviewed-by: Furquan Shaikh <[e-mail]>
---
M src/soc/intel/cannonlake/chip.h
M src/soc/intel/cannonlake/fsp_params.c
2 files changed, 6 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved



diff --git a/src/soc/intel/cannonlake/chip.h b/src/soc/intel/cannonlake/chip.h
index a81a7c1..7461d78 100644
--- a/src/soc/intel/cannonlake/chip.h
+++ b/src/soc/intel/cannonlake/chip.h
@@ -407,6 +407,9 @@
 	uint8_t DdiPortCDdc;
 	uint8_t DdiPortDDdc;
 	uint8_t DdiPortFDdc;
+
+	/* Unlock all GPIO Pads */
+	uint8_t PchUnlockGpioPads;
 };
 
 typedef struct soc_intel_cannonlake_config config_t;
diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c
index 25ee5e1..2688557 100644
--- a/src/soc/intel/cannonlake/fsp_params.c
+++ b/src/soc/intel/cannonlake/fsp_params.c
@@ -340,6 +340,9 @@
 
 	/* Set TccActivationOffset */
 	tconfig->TccActivationOffset = config->tcc_offset;
+
+	/* Unlock all GPIO pads */
+	tconfig->PchUnlockGpioPads = config->PchUnlockGpioPads;
 }
 
 /* Mainboard GPIO Configuration */

-- 
To view, visit https://review.coreboot.org/c/coreboot/+/32110
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Iad9e8a209dc3f8ca0c994e8c1da329918409a1d4
Gerrit-Change-Number: 32110
Gerrit-PatchSet: 5
Gerrit-Owner: Krishna P Bhat D <[e-mail]>
Gerrit-Reviewer: Furquan Shaikh <[e-mail]>
Gerrit-Reviewer: Krishna P Bhat D <[e-mail]>
Gerrit-Reviewer: Patrick Georgi <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Rizwan Qureshi <[e-mail]>
Gerrit-Reviewer: Shelley Chen <[e-mail]>
Gerrit-Reviewer: Tim Wawrzynczak <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-MessageType: merged