Change in coreboot[master]: soc/intel/common: Check prerequisites for GLOBAL_RESET command

[Sridhar Siricilla (Code Review)]

Sridhar Siricilla has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38800 )

Change subject: soc/intel/common: Check prerequisites for GLOBAL_RESET command
......................................................................

soc/intel/common: Check prerequisites for GLOBAL_RESET command

Check prerequisites before sending GLOBAL RESET command to CSE.

TEST=Verified on hatch.

Change-Id: Ia583e4033f15ec20e942202fa78e7884cf370ce4
Signed-off-by: Sridhar Siricilla 
---
M src/soc/intel/common/block/cse/cse.c
1 file changed, 17 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/38800/1

diff --git a/src/soc/intel/common/block/cse/cse.c b/src/soc/intel/common/block/cse/cse.c
index 072dd05..735c0e1 100644
--- a/src/soc/intel/common/block/cse/cse.c
+++ b/src/soc/intel/common/block/cse/cse.c
@@ -607,6 +607,17 @@
    return pci_read_config32(PCH_DEV_CSE, offset);
 }

+static int cse_check_global_reset_prerequisites(void)
+{
+	if (cse_is_hfs1_cws_normal() && cse_is_hfs1_com_normal())
+		return 1;
+	else if (cse_is_hfs3_fw_sku_custom() && cse_is_hfs1_cws_normal()
+		&& (cse_is_hfs1_com_soft_temp_disable() || cse_is_hfs1_com_secover_mei_msg()))
+		return 1;
+
+	return 0;
+}
+
 /*
  * Sends GLOBAL_RESET_REQ cmd to CSE.The reset type can be GLOBAL_RESET/
  * HOST_RESET_ONLY/CSE_RESET_ONLY.
@@ -631,12 +642,18 @@
    size_t reply_size;

printk(BIOS_DEBUG, "HECI: Global Reset(Type:%d) Command\n", rst_type);
+
    if (!((rst_type == GLOBAL_RESET) ||
    	(rst_type == HOST_RESET_ONLY) || (rst_type == CSE_RESET_ONLY))) {
    	printk(BIOS_ERR, "HECI: Unsupported reset type is requested\n");
    	return 0;
    }

+	if (!cse_check_global_reset_prerequisites()) {
+		printk(BIOS_ERR, "HECI: CSE is not meeting required prerequisites\n");
+		return 0;
+	}
+
    heci_reset();

reply_size = sizeof(reply);

[Sridhar Siricilla (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Rizwan Qureshi, build bot (Jenkins), Nico Huber, Furquan Shaikh,

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

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

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

Change subject: soc/intel/common: Check prerequisites for GLOBAL_RESET command
......................................................................

soc/intel/common: Check prerequisites for GLOBAL_RESET command

Check prerequisites before sending GLOBAL RESET command to CSE.

TEST=Verified on hatch.

Change-Id: Ia583e4033f15ec20e942202fa78e7884cf370ce4
Signed-off-by: Sridhar Siricilla 
---
M src/soc/intel/common/block/cse/cse.c
1 file changed, 17 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/38800/3

[Sridhar Siricilla (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Rizwan Qureshi, build bot (Jenkins), Nico Huber, Furquan Shaikh,

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

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

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

Change subject: soc/intel/common: Check prerequisites for GLOBAL_RESET command
......................................................................

soc/intel/common: Check prerequisites for GLOBAL_RESET command

Check prerequisites before sending GLOBAL RESET command to CSE.

TEST=Verified on hatch.

Change-Id: Ia583e4033f15ec20e942202fa78e7884cf370ce4
Signed-off-by: Sridhar Siricilla 
---
M src/soc/intel/common/block/cse/cse.c
1 file changed, 17 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/38800/6

[Sridhar Siricilla (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Rizwan Qureshi, build bot (Jenkins), Nico Huber, Furquan Shaikh,

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

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

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

Change subject: soc/intel/common: Check prerequisites for GLOBAL_RESET command
......................................................................

soc/intel/common: Check prerequisites for GLOBAL_RESET command

Check prerequisites before sending GLOBAL RESET command to CSE.

TEST=Verified on hatch.

Change-Id: Ia583e4033f15ec20e942202fa78e7884cf370ce4
Signed-off-by: Sridhar Siricilla 
---
M src/soc/intel/common/block/cse/cse.c
1 file changed, 17 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/38800/7

[Rizwan Qureshi (Code Review)]

Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38800 )

Change subject: soc/intel/common: Check prerequisites for GLOBAL_RESET command
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38800/7/src/soc/intel/common/block/... 
File src/soc/intel/common/block/cse/cse.c:

https://review.coreboot.org/c/coreboot/+/38800/7/src/soc/intel/common/block/... 
PS7, Line 613: if (cse_is_hfs1_cws_normal() && cse_is_hfs1_com_normal())
             : 		return 1;
             : 	else if (cse_is_hfs3_fw_sku_custom() && cse_is_hfs1_cws_normal()
             : 		&& (cse_is_hfs1_com_soft_temp_disable() || cse_is_hfs1_com_secover_mei_msg()))
             : 		return 1;
             : 
             : 	return 0;
Add a comment to explain the prerequisites.

[Sridhar Siricilla (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Rizwan Qureshi, build bot (Jenkins), Nico Huber, Furquan Shaikh,

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

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

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

Change subject: soc/intel/common: Check prerequisites for GLOBAL_RESET command
......................................................................

soc/intel/common: Check prerequisites for GLOBAL_RESET command

Check prerequisites before sending GLOBAL RESET command to CSE.

TEST=Verified on hatch.

Change-Id: Ia583e4033f15ec20e942202fa78e7884cf370ce4
Signed-off-by: Sridhar Siricilla 
---
M src/soc/intel/common/block/cse/cse.c
1 file changed, 24 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/38800/8

[Paul Menzel (Code Review)]

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

Change subject: soc/intel/common: Check prerequisites for GLOBAL_RESET command
......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38800/8/src/soc/intel/common/block/... 
File src/soc/intel/common/block/cse/cse.c:

https://review.coreboot.org/c/coreboot/+/38800/8/src/soc/intel/common/block/... 
PS8, Line 622: else 
As the first branch returns, no `else` is needed.

[Sridhar Siricilla (Code Review)]

Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38800 )

Change subject: soc/intel/common: Check prerequisites for GLOBAL_RESET command
......................................................................

Patch Set 9:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38800/8/src/soc/intel/common/block/... 
File src/soc/intel/common/block/cse/cse.c:

https://review.coreboot.org/c/coreboot/+/38800/8/src/soc/intel/common/block/... 
PS8, Line 622: else
> As the first branch returns, no `else` is needed.

Done

[Sridhar Siricilla (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Rizwan Qureshi, build bot (Jenkins), Nico Huber, Furquan Shaikh,

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

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

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

Change subject: soc/intel/common: Check prerequisites for GLOBAL_RESET command
......................................................................

soc/intel/common: Check prerequisites for GLOBAL_RESET command

Check prerequisites before sending GLOBAL RESET command to CSE.

TEST=Verified on hatch.

Change-Id: Ia583e4033f15ec20e942202fa78e7884cf370ce4
Signed-off-by: Sridhar Siricilla 
---
M src/soc/intel/common/block/cse/cse.c
1 file changed, 24 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/38800/10

[Sridhar Siricilla (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Rizwan Qureshi, build bot (Jenkins), Nico Huber, Furquan Shaikh,

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

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

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

Change subject: soc/intel/common: Check prerequisites for GLOBAL_RESET command
......................................................................

soc/intel/common: Check prerequisites for GLOBAL_RESET command

Check prerequisites before sending GLOBAL RESET command to CSE.

TEST=Verified on hatch.

Change-Id: Ia583e4033f15ec20e942202fa78e7884cf370ce4
Signed-off-by: Sridhar Siricilla 
---
M src/soc/intel/common/block/cse/cse.c
1 file changed, 24 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/38800/11

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38800 )

Change subject: soc/intel/common: Check prerequisites for GLOBAL_RESET command
......................................................................

Patch Set 12: Code-Review+1

(4 comments)

https://review.coreboot.org/c/coreboot/+/38800/12/src/soc/intel/common/block... 
File src/soc/intel/common/block/cse/cse.c:

https://review.coreboot.org/c/coreboot/+/38800/12/src/soc/intel/common/block... 
PS12, Line 611: static int cse_check_global_reset_prerequisites(void)
Return a bool? Maybe rewrite the function name as a question:

static bool cse_is_global_reset_allowed(void)

https://review.coreboot.org/c/coreboot/+/38800/12/src/soc/intel/common/block... 
PS12, Line 614: HMRFPO ENABLE
Copypasta?

https://review.coreboot.org/c/coreboot/+/38800/12/src/soc/intel/common/block... 
PS12, Line 622: cse_is_hfs1_cws_normal()
This can be factored out:

if (!cse_is_hfs1_cws_normal())
    return false;

if (cse_is_hfs1_com_normal())
    return true;

if (cse_is_hfs3_fw_sku_custom()) {
    if (cse_is_hfs1_com_soft_temp_disable())
    	return true;
    if (cse_is_hfs1_com_secover_mei_msg())
    	return true;
 }

return false;

https://review.coreboot.org/c/coreboot/+/38800/12/src/soc/intel/common/block... 
PS12, Line 659: is not meeting
does not meet

[Sridhar Siricilla (Code Review)]

Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Rizwan Qureshi, Angel Pons, Subrata Banik, Patrick Rudolph,

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

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

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

Change subject: soc/intel/common: Check prerequisites for GLOBAL_RESET command
......................................................................

soc/intel/common: Check prerequisites for GLOBAL_RESET command

Check prerequisites before sending GLOBAL RESET command to CSE.

TEST=Verified on hatch.

Change-Id: Ia583e4033f15ec20e942202fa78e7884cf370ce4
Signed-off-by: Sridhar Siricilla 
---
M src/soc/intel/common/block/cse/cse.c
1 file changed, 25 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/38800/13

[Sridhar Siricilla (Code Review)]

Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38800 )

Change subject: soc/intel/common: Check prerequisites for GLOBAL_RESET command
......................................................................

Patch Set 13:

(4 comments)

https://review.coreboot.org/c/coreboot/+/38800/12/src/soc/intel/common/block... 
File src/soc/intel/common/block/cse/cse.c:

https://review.coreboot.org/c/coreboot/+/38800/12/src/soc/intel/common/block... 
PS12, Line 611: static int cse_check_global_reset_prerequisites(void)
> Return a bool? Maybe rewrite the function name as a question: […]

Renamed the function as suggested. Thanks

https://review.coreboot.org/c/coreboot/+/38800/12/src/soc/intel/common/block... 
PS12, Line 614: HMRFPO ENABLE
> Copypasta?

Fixed it,

https://review.coreboot.org/c/coreboot/+/38800/12/src/soc/intel/common/block... 
PS12, Line 622: cse_is_hfs1_cws_normal()
> This can be factored out: […]

Done

https://review.coreboot.org/c/coreboot/+/38800/12/src/soc/intel/common/block... 
PS12, Line 659: is not meeting
> does not meet

Ack

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38800 )

Change subject: soc/intel/common: Check prerequisites for GLOBAL_RESET command
......................................................................

Patch Set 13:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38800/12/src/soc/intel/common/block... 
File src/soc/intel/common/block/cse/cse.c:

https://review.coreboot.org/c/coreboot/+/38800/12/src/soc/intel/common/block... 
PS12, Line 622: cse_is_hfs1_cws_normal()
> Done

Um, not really: 'cse_is_hfs1_cws_normal()' check will not be taken into account on the custom FW path.

[Sridhar Siricilla (Code Review)]

Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Rizwan Qureshi, Angel Pons, Subrata Banik, Patrick Rudolph,

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

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

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

Change subject: soc/intel/common: Check prerequisites for GLOBAL_RESET command
......................................................................

soc/intel/common: Check prerequisites for GLOBAL_RESET command

Check prerequisites before sending GLOBAL RESET command to CSE.

TEST=Verified on hatch.

Change-Id: Ia583e4033f15ec20e942202fa78e7884cf370ce4
Signed-off-by: Sridhar Siricilla 
---
M src/soc/intel/common/block/cse/cse.c
1 file changed, 28 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/38800/14

[Sridhar Siricilla (Code Review)]

Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38800 )

Change subject: soc/intel/common: Check prerequisites for GLOBAL_RESET command
......................................................................

Patch Set 14:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38800/12/src/soc/intel/common/block... 
File src/soc/intel/common/block/cse/cse.c:

https://review.coreboot.org/c/coreboot/+/38800/12/src/soc/intel/common/block... 
PS12, Line 622: cse_is_hfs1_cws_normal()
> Um, not really: 'cse_is_hfs1_cws_normal()' check will not be taken into account on the custom FW pat […]

Ack

[Sridhar Siricilla (Code Review)]

Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38800 )

Change subject: soc/intel/common: Check prerequisites for GLOBAL_RESET command
......................................................................

Patch Set 14:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38800/7/src/soc/intel/common/block/... 
File src/soc/intel/common/block/cse/cse.c:

https://review.coreboot.org/c/coreboot/+/38800/7/src/soc/intel/common/block/... 
PS7, Line 613: if (cse_is_hfs1_cws_normal() && cse_is_hfs1_com_normal())
             : 		return 1;
             : 	else if (cse_is_hfs3_fw_sku_custom() && cse_is_hfs1_cws_normal()
             : 		&& (cse_is_hfs1_com_soft_temp_disable() || cse_is_hfs1_com_secover_mei_msg()))
             : 		return 1;
             : 
             : 	return 0;
> Add a comment to explain the prerequisites.

Done

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38800 )

Change subject: soc/intel/common: Check prerequisites for GLOBAL_RESET command
......................................................................

Patch Set 14: Code-Review+2

Thanks!

[Patrick Georgi (Code Review)]

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

Change subject: soc/intel/common: Check prerequisites for GLOBAL_RESET command
......................................................................

soc/intel/common: Check prerequisites for GLOBAL_RESET command

Check prerequisites before sending GLOBAL RESET command to CSE.

TEST=Verified on hatch.

Change-Id: Ia583e4033f15ec20e942202fa78e7884cf370ce4
Signed-off-by: Sridhar Siricilla 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/38800
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
---
M src/soc/intel/common/block/cse/cse.c
1 file changed, 28 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved

diff --git a/src/soc/intel/common/block/cse/cse.c b/src/soc/intel/common/block/cse/cse.c
index 041656a..648ec6a 100644
--- a/src/soc/intel/common/block/cse/cse.c
+++ b/src/soc/intel/common/block/cse/cse.c
@@ -608,6 +608,28 @@
    return pci_read_config32(PCH_DEV_CSE, offset);
 }

+static bool cse_is_global_reset_allowed(void)
+{
+	/*
+	 * Allow sending GLOBAL_RESET command only if:
+	 *  - CSE's current working state is Normal and current operation mode is Normal.
+	 *  - (or) CSE's current working state is normal and current operation mode can
+	 *    be Soft Temp Disable or Security Override Mode if CSE's Firmware SKU is
+	 *    Custom.
+	 */
+	if (!cse_is_hfs1_cws_normal())
+		return false;
+
+	if (cse_is_hfs1_com_normal())
+		return true;
+
+	if (cse_is_hfs3_fw_sku_custom()) {
+		if (cse_is_hfs1_com_soft_temp_disable() || cse_is_hfs1_com_secover_mei_msg())
+			return true;
+	}
+	return false;
+}
+
 /*
  * Sends GLOBAL_RESET_REQ cmd to CSE.The reset type can be GLOBAL_RESET/CSE_RESET_ONLY.
  */
@@ -631,11 +653,17 @@
    size_t reply_size;

printk(BIOS_DEBUG, "HECI: Global Reset(Type:%d) Command\n", rst_type);
+
    if (!(rst_type == GLOBAL_RESET || rst_type == CSE_RESET_ONLY)) {
    	printk(BIOS_ERR, "HECI: Unsupported reset type is requested\n");
    	return 0;
    }

+	if (!cse_is_global_reset_allowed()) {
+		printk(BIOS_ERR, "HECI: CSE does not meet required prerequisites\n");
+		return 0;
+	}
+
    heci_reset();

reply_size = sizeof(reply);