Change in coreboot[master]: soc/intel/common/block/cse: Add helper function cse_send_global_reset

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/common/block/cse: Add helper function cse_send_global_reset
......................................................................

soc/intel/common/block/cse: Add helper function cse_send_global_reset

Sends GLOBAL_RESET_REQ cmd to CSE.
1. Check if CSE is enable from devicetree.cb
2. Ensure CSE in Normal Mode prior sending global reset command
3. If not in normal mode then send error status
4. If in normal mode then only call cse_request_global_reset() to
send global reset command

Signed-off-by: Subrata Banik 
Change-Id: I3668b473bec8d51f847908d11e2e25c485ec7a97
---
M src/soc/intel/common/block/cse/cse.c
M src/soc/intel/common/block/include/intelblocks/cse.h
2 files changed, 28 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/45341/1

diff --git a/src/soc/intel/common/block/cse/cse.c b/src/soc/intel/common/block/cse/cse.c
index 4b598e2..90afdcf 100644
--- a/src/soc/intel/common/block/cse/cse.c
+++ b/src/soc/intel/common/block/cse/cse.c
@@ -663,6 +663,25 @@
    return status;
 }

+int cse_send_global_reset(void)
+{
+	int status = -1;
+	union me_hfsts1 hfs1;
+
+	if (!is_cse_enabled())
+		goto ret;
+
+	/* Check ME operating mode */
+	hfs1.data = me_read_config32(PCI_ME_HFSTS1);
+	if (hfs1.fields.operation_mode)
+		goto ret;
+
+	/* ME should be in Normal Mode for this command */
+	status = cse_request_global_reset(GLOBAL_RESET);
+ret:
+	return status;
+}
+
 static bool cse_is_hmrfpo_enable_allowed(void)
 {
    /*
diff --git a/src/soc/intel/common/block/include/intelblocks/cse.h b/src/soc/intel/common/block/include/intelblocks/cse.h
index a67010c..9804b0d 100644
--- a/src/soc/intel/common/block/include/intelblocks/cse.h
+++ b/src/soc/intel/common/block/include/intelblocks/cse.h
@@ -126,7 +126,15 @@
  * Returns 0 on failure and 1 on success.
  */
 int cse_request_global_reset(enum rst_req_type rst_type);
-
+/*
+ * Sends GLOBAL_RESET_REQ cmd to CSE.
+ * 1. Check if CSE is enable from devicetree.cb
+ * 2. Ensure CSE in Normal Mode prior sending global reset command
+ * 3. If not in normal mode then send error status
+ * 4. If in normal mode then only call cse_request_global_reset() to send global reset command
+ * Returns 0 on failure and 1 on success.
+ */
+int cse_send_global_reset(void);
 /*
  * Sends HMRFPO_ENABLE command.
  * HMRFPO - Host ME Region Flash Protection Override.

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/common/block/cse: Add helper function cse_send_global_reset
......................................................................

Patch Set 1:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/45341/1/src/soc/intel/common/block/... 
PS1, Line 666: cse_send_global_reset
It is not very clear to me what the intention of adding this function is. Is it to provide a helper where the caller does not really have to provide GLOBAL_RESET as argument? The names of the two functions are so similar (request v/s send) that it does not really help understand the difference between the two. Maybe we should rename `cse_request_global_request()` to `cse_request_reset()` and this function as `cse_request_global_reset()`?

Also, I don't think we should have additional checks within this function. It should simply be a wrapper around `cse_request_global_request()`:

int cse_request_global_request(void)
{
    return cse_request_reset(GLOBAL_RESET);
}

The only additional check that you have in here is for `is_cse_enabled()`. That should be added to the above function instead of here.

https://review.coreboot.org/c/coreboot/+/45341/1/src/soc/intel/common/block/... 
PS1, Line 675: hfs1.data = me_read_config32(PCI_ME_HFSTS1);
             : 	if (hfs1.fields.operation_mode)
             : 		goto ret;
This check is already performed by `cse_is_global_reset_allowed()` in `cse_request_global_reset()`.

[Angel Pons (Code Review)]

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

Change subject: soc/intel/common/block/cse: Add helper function cse_send_global_reset
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/45341/1/src/soc/intel/common/block/... 
PS1, Line 666: cse_send_global_reset
> It is not very clear to me what the intention of adding this function is. […]

The purpose of this is to unify reset handling across SoCs. But I agree, this should just be a mere wrapper for `cse_request_reset`. If any checks are done here, why not do them for all reset types?

Change in coreboot[master]: soc/intel/common/block/cse: Refactor cse_request_global_reset() function

[Subrata Banik (Code Review)]

Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,

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

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

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

Change subject: soc/intel/common/block/cse: Refactor cse_request_global_reset() function
......................................................................

soc/intel/common/block/cse: Refactor cse_request_global_reset() function

List of changes:
1. Check if CSE is enable from devicetree.cb
2. Create helper function cse_request_reset()
3. Modify caller function argument cse_request_global_reset()

Signed-off-by: Subrata Banik 
Change-Id: I3668b473bec8d51f847908d11e2e25c485ec7a97
---
M src/soc/intel/cannonlake/reset.c
M src/soc/intel/common/block/cse/cse.c
M src/soc/intel/common/block/include/intelblocks/cse.h
M src/soc/intel/elkhartlake/reset.c
M src/soc/intel/icelake/reset.c
M src/soc/intel/jasperlake/reset.c
M src/soc/intel/skylake/me.c
M src/soc/intel/tigerlake/reset.c
8 files changed, 18 insertions(+), 13 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/45341/2

Change in coreboot[master]: soc/intel/common/block/cse: Refactor cse_request_global_reset() function

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/common/block/cse: Refactor cse_request_global_reset() function
......................................................................

Patch Set 2:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/45341/1/src/soc/intel/common/block/... 
PS1, Line 666: cse_send_global_reset
> The purpose of this is to unify reset handling across SoCs. […]

Ack

https://review.coreboot.org/c/coreboot/+/45341/1/src/soc/intel/common/block/... 
PS1, Line 675: hfs1.data = me_read_config32(PCI_ME_HFSTS1);
             : 	if (hfs1.fields.operation_mode)
             : 		goto ret;
> This check is already performed by `cse_is_global_reset_allowed()` in `cse_request_global_reset()`.

Ack

Change in coreboot[master]: soc/intel/common/block/cse: Refactor cse_request_global_reset() function

[Subrata Banik (Code Review)]

Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,

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

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

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

Change subject: soc/intel/common/block/cse: Refactor cse_request_global_reset() function
......................................................................

soc/intel/common/block/cse: Refactor cse_request_global_reset() function

List of changes:
1. Check if CSE is enable from devicetree.cb
2. Create helper function cse_request_reset()
3. Modify caller function argument cse_request_global_reset()

Signed-off-by: Subrata Banik 
Change-Id: I3668b473bec8d51f847908d11e2e25c485ec7a97
---
M src/soc/intel/cannonlake/reset.c
M src/soc/intel/common/block/cse/cse.c
M src/soc/intel/common/block/include/intelblocks/cse.h
M src/soc/intel/elkhartlake/reset.c
M src/soc/intel/icelake/reset.c
M src/soc/intel/jasperlake/reset.c
M src/soc/intel/skylake/me.c
M src/soc/intel/tigerlake/reset.c
8 files changed, 15 insertions(+), 13 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/45341/3

Change in coreboot[master]: soc/intel/common/block/cse: Refactor cse_request_global_reset() function

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/common/block/cse: Refactor cse_request_global_reset() function
......................................................................

Patch Set 3:

(3 comments)

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

https://review.coreboot.org/c/coreboot/+/45341/3/src/soc/intel/common/block/... 
PS3, Line 618: static
I don't think its a problem right now but if we ever have to support CSE only reset using this command, then we might have to add another helper or remove the static here.

https://review.coreboot.org/c/coreboot/+/45341/3/src/soc/intel/common/block/... 
PS3, Line 663: The reset type can be GLOBAL_RESET/CSE_RESET_ONLY
This does not apply to cse_request_global_reset(). I think this whole comment should stay with cse_request_reset above.

https://review.coreboot.org/c/coreboot/+/45341/3/src/soc/intel/common/block/... 
File src/soc/intel/common/block/include/intelblocks/cse.h:

https://review.coreboot.org/c/coreboot/+/45341/3/src/soc/intel/common/block/... 
PS3, Line 125:  The reset type can be one of the above defined reset type.
No longer true.

Change in coreboot[master]: soc/intel/common/block/cse: Refactor cse_request_global_reset() function

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/common/block/cse: Refactor cse_request_global_reset() function
......................................................................

Patch Set 3:

(3 comments)

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

https://review.coreboot.org/c/coreboot/+/45341/3/src/soc/intel/common/block/... 
PS3, Line 618: static
> I don't think its a problem right now but if we ever have to support CSE only reset using this comma […]

yes you are right. so far all SoC is just calling cse_request_global_reset, so we are good for now

https://review.coreboot.org/c/coreboot/+/45341/3/src/soc/intel/common/block/... 
PS3, Line 663: The reset type can be GLOBAL_RESET/CSE_RESET_ONLY
> This does not apply to cse_request_global_reset(). […]

Ack

https://review.coreboot.org/c/coreboot/+/45341/3/src/soc/intel/common/block/... 
File src/soc/intel/common/block/include/intelblocks/cse.h:

https://review.coreboot.org/c/coreboot/+/45341/3/src/soc/intel/common/block/... 
PS3, Line 125:  The reset type can be one of the above defined reset type.
> No longer true.

Ack

Change in coreboot[master]: soc/intel/common/block/cse: Refactor cse_request_global_reset() function

[Subrata Banik (Code Review)]

Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,

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

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

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

Change subject: soc/intel/common/block/cse: Refactor cse_request_global_reset() function
......................................................................

soc/intel/common/block/cse: Refactor cse_request_global_reset() function

List of changes:
1. Check if CSE is enable from devicetree.cb
2. Create helper function cse_request_reset()
3. Modify caller function argument cse_request_global_reset()

Signed-off-by: Subrata Banik 
Change-Id: I3668b473bec8d51f847908d11e2e25c485ec7a97
---
M src/soc/intel/cannonlake/reset.c
M src/soc/intel/common/block/cse/cse.c
M src/soc/intel/common/block/include/intelblocks/cse.h
M src/soc/intel/elkhartlake/reset.c
M src/soc/intel/icelake/reset.c
M src/soc/intel/jasperlake/reset.c
M src/soc/intel/skylake/me.c
M src/soc/intel/tigerlake/reset.c
8 files changed, 16 insertions(+), 18 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/45341/4

Change in coreboot[master]: soc/intel/common/block/cse: Refactor cse_request_global_reset() function

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/common/block/cse: Refactor cse_request_global_reset() function
......................................................................

Patch Set 4: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/45341/4/src/soc/intel/common/block/... 
File src/soc/intel/common/block/include/intelblocks/cse.h:

https://review.coreboot.org/c/coreboot/+/45341/4/src/soc/intel/common/block/... 
PS4, Line 123:  
with reset type GLOBAL_RESET?

Change in coreboot[master]: soc/intel/common/block/cse: Refactor cse_request_global_reset() function

[Subrata Banik (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Angel Pons, Patrick Rudolph,

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

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

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

Change subject: soc/intel/common/block/cse: Refactor cse_request_global_reset() function
......................................................................

soc/intel/common/block/cse: Refactor cse_request_global_reset() function

List of changes:
1. Check if CSE is enable from devicetree.cb
2. Create helper function cse_request_reset()
3. Modify caller function argument cse_request_global_reset()

Signed-off-by: Subrata Banik 
Change-Id: I3668b473bec8d51f847908d11e2e25c485ec7a97
---
M src/soc/intel/cannonlake/reset.c
M src/soc/intel/common/block/cse/cse.c
M src/soc/intel/common/block/include/intelblocks/cse.h
M src/soc/intel/elkhartlake/reset.c
M src/soc/intel/icelake/reset.c
M src/soc/intel/jasperlake/reset.c
M src/soc/intel/skylake/me.c
M src/soc/intel/tigerlake/reset.c
8 files changed, 17 insertions(+), 13 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/45341/5

Change in coreboot[master]: soc/intel/common/block/cse: Refactor cse_request_global_reset() function

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/common/block/cse: Refactor cse_request_global_reset() function
......................................................................

Patch Set 5: Code-Review+2

Change in coreboot[master]: soc/intel/common/block/cse: Refactor cse_request_global_reset() function

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/common/block/cse: Refactor cse_request_global_reset() function
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45341/4/src/soc/intel/common/block/... 
File src/soc/intel/common/block/include/intelblocks/cse.h:

https://review.coreboot.org/c/coreboot/+/45341/4/src/soc/intel/common/block/... 
PS4, Line 123:
> with reset type GLOBAL_RESET?

done

Change in coreboot[master]: soc/intel/common/block/cse: Refactor cse_request_global_reset() function

[Subrata Banik (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Angel Pons, Patrick Rudolph,

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

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

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

Change subject: soc/intel/common/block/cse: Refactor cse_request_global_reset() function
......................................................................

soc/intel/common/block/cse: Refactor cse_request_global_reset() function

List of changes:
1. Check if CSE is enable from devicetree.cb
2. Create helper function cse_request_reset()
3. Modify caller function argument cse_request_global_reset()

Signed-off-by: Subrata Banik 
Change-Id: I3668b473bec8d51f847908d11e2e25c485ec7a97
---
M src/soc/intel/alderlake/reset.c
M src/soc/intel/cannonlake/reset.c
M src/soc/intel/common/block/cse/cse.c
M src/soc/intel/common/block/include/intelblocks/cse.h
M src/soc/intel/elkhartlake/reset.c
M src/soc/intel/icelake/reset.c
M src/soc/intel/jasperlake/reset.c
M src/soc/intel/skylake/me.c
M src/soc/intel/tigerlake/reset.c
9 files changed, 18 insertions(+), 14 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/45341/7

Change in coreboot[master]: soc/intel/common/block/cse: Refactor cse_request_global_reset() function

[Werner Zeh (Code Review)]

Werner Zeh has removed Name of user not set #1003096 from this change.  ( https://review.coreboot.org/c/coreboot/+/45341 )

Change subject: soc/intel/common/block/cse: Refactor cse_request_global_reset() function
......................................................................

Removed reviewer null with the following votes:

* Verified+1 by Name of user not set (1003096)

Change in coreboot[master]: soc/intel/common/block/cse: Refactor cse_request_global_reset() function

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45341 )

Change subject: soc/intel/common/block/cse: Refactor cse_request_global_reset() function
......................................................................

Patch Set 7: Code-Review+1

Change in coreboot[master]: soc/intel/common/block/cse: Refactor cse_request_global_reset() function

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/common/block/cse: Refactor cse_request_global_reset() function
......................................................................

Patch Set 7:

@Furquan, is this good now, just added ADL support after romstage CL merged

Change in coreboot[master]: soc/intel/common/block/cse: Refactor cse_request_global_reset() function

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/common/block/cse: Refactor cse_request_global_reset() function
......................................................................

Patch Set 7: Code-Review+2

Change in coreboot[master]: soc/intel/common/block/cse: Refactor cse_request_global_reset() function

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/common/block/cse: Refactor cse_request_global_reset() function
......................................................................

Patch Set 7:

Thanks Furquan

Change in coreboot[master]: soc/intel/common/block/cse: Refactor cse_request_global_reset() function

[Angel Pons (Code Review)]

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

Change subject: soc/intel/common/block/cse: Refactor cse_request_global_reset() function
......................................................................

Patch Set 7: Code-Review+2

Change in coreboot[master]: soc/intel/common/block/cse: Refactor cse_request_global_reset() function

[Michael Niewöhner (Code Review)]

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

Change subject: soc/intel/common/block/cse: Refactor cse_request_global_reset() function
......................................................................

Patch Set 9:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45341/9//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/45341/9//COMMIT_MSG@10 
PS9, Line 10: enable
nit: enabled

Change in coreboot[master]: soc/intel/common/block/cse: Refactor cse_request_global_reset() function

[Michael Niewöhner (Code Review)]

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

Change subject: soc/intel/common/block/cse: Refactor cse_request_global_reset() function
......................................................................

Patch Set 9: Code-Review+1

Change in coreboot[master]: soc/intel/common/block/cse: Refactor cse_request_global_reset() function

[Subrata Banik (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Angel Pons, Michael Niewöhner, Patrick Rudolph, HAOUAS Elyes,

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

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

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

Change subject: soc/intel/common/block/cse: Refactor cse_request_global_reset() function
......................................................................

soc/intel/common/block/cse: Refactor cse_request_global_reset() function

List of changes:
1. Check if CSE is enabled from devicetree.cb
2. Create helper function cse_request_reset()
3. Modify caller function argument cse_request_global_reset()

Signed-off-by: Subrata Banik 
Change-Id: I3668b473bec8d51f847908d11e2e25c485ec7a97
---
M src/soc/intel/alderlake/reset.c
M src/soc/intel/cannonlake/reset.c
M src/soc/intel/common/block/cse/cse.c
M src/soc/intel/common/block/include/intelblocks/cse.h
M src/soc/intel/elkhartlake/reset.c
M src/soc/intel/icelake/reset.c
M src/soc/intel/jasperlake/reset.c
M src/soc/intel/skylake/me.c
M src/soc/intel/tigerlake/reset.c
9 files changed, 18 insertions(+), 14 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/45341/10

Change in coreboot[master]: soc/intel/common/block/cse: Refactor cse_request_global_reset() function

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/common/block/cse: Refactor cse_request_global_reset() function
......................................................................

Patch Set 10:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45341/9//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/45341/9//COMMIT_MSG@10 
PS9, Line 10: enable
> nit: enabled

Ack

Change in coreboot[master]: soc/intel/common/block/cse: Refactor cse_request_global_reset() function

[Michael Niewöhner (Code Review)]

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

Change subject: soc/intel/common/block/cse: Refactor cse_request_global_reset() function
......................................................................

Patch Set 10: Code-Review+2

Change in coreboot[master]: soc/intel/common/block/cse: Refactor cse_request_global_reset() function

[Subrata Banik (Code Review)]

Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45341 )

Change subject: soc/intel/common/block/cse: Refactor cse_request_global_reset() function
......................................................................

soc/intel/common/block/cse: Refactor cse_request_global_reset() function

List of changes:
1. Check if CSE is enabled from devicetree.cb
2. Create helper function cse_request_reset()
3. Modify caller function argument cse_request_global_reset()

Signed-off-by: Subrata Banik 
Change-Id: I3668b473bec8d51f847908d11e2e25c485ec7a97
Reviewed-on: https://review.coreboot.org/c/coreboot/+/45341
Reviewed-by: Michael Niewöhner 
Reviewed-by: HAOUAS Elyes 
Reviewed-by: Furquan Shaikh 
Reviewed-by: Angel Pons 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/alderlake/reset.c
M src/soc/intel/cannonlake/reset.c
M src/soc/intel/common/block/cse/cse.c
M src/soc/intel/common/block/include/intelblocks/cse.h
M src/soc/intel/elkhartlake/reset.c
M src/soc/intel/icelake/reset.c
M src/soc/intel/jasperlake/reset.c
M src/soc/intel/skylake/me.c
M src/soc/intel/tigerlake/reset.c
9 files changed, 18 insertions(+), 14 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved
  HAOUAS Elyes: Looks good to me, but someone else must approve
  Angel Pons: Looks good to me, approved
  Michael Niewöhner: Looks good to me, approved

diff --git a/src/soc/intel/alderlake/reset.c b/src/soc/intel/alderlake/reset.c
index d37ff54..1f7ea3c 100644
--- a/src/soc/intel/alderlake/reset.c
+++ b/src/soc/intel/alderlake/reset.c
@@ -11,7 +11,7 @@
 void do_global_reset(void)
 {
    /* Ask CSE to do the global reset */
-	if (cse_request_global_reset(GLOBAL_RESET))
+	if (cse_request_global_reset())
    	return;

/* global reset if CSE fail to reset */
diff --git a/src/soc/intel/cannonlake/reset.c b/src/soc/intel/cannonlake/reset.c
index d37ff54..1f7ea3c 100644
--- a/src/soc/intel/cannonlake/reset.c
+++ b/src/soc/intel/cannonlake/reset.c
@@ -11,7 +11,7 @@
 void do_global_reset(void)
 {
    /* Ask CSE to do the global reset */
-	if (cse_request_global_reset(GLOBAL_RESET))
+	if (cse_request_global_reset())
    	return;

/* global reset if CSE fail to reset */
diff --git a/src/soc/intel/common/block/cse/cse.c b/src/soc/intel/common/block/cse/cse.c
index 4b598e2..ef6db3d 100644
--- a/src/soc/intel/common/block/cse/cse.c
+++ b/src/soc/intel/common/block/cse/cse.c
@@ -616,9 +616,10 @@
 }

/*
- * Sends GLOBAL_RESET_REQ cmd to CSE.The reset type can be GLOBAL_RESET/CSE_RESET_ONLY.
+ * Sends GLOBAL_RESET_REQ cmd to CSE with reset type GLOBAL_RESET.
+ * Returns 0 on failure and 1 on success.
  */
-int cse_request_global_reset(enum rst_req_type rst_type)
+static int cse_request_reset(enum rst_req_type rst_type)
 {
    int status;
    struct mkhi_hdr reply;
@@ -644,7 +645,7 @@
    	return 0;
    }

-	if (!cse_is_global_reset_allowed()) {
+	if (!cse_is_global_reset_allowed() || !is_cse_enabled()) {
    	printk(BIOS_ERR, "HECI: CSE does not meet required prerequisites\n");
    	return 0;
    }
@@ -663,6 +664,11 @@
    return status;
 }

+int cse_request_global_reset(void)
+{
+	return cse_request_reset(GLOBAL_RESET);
+}
+
 static bool cse_is_hmrfpo_enable_allowed(void)
 {
    /*
diff --git a/src/soc/intel/common/block/include/intelblocks/cse.h b/src/soc/intel/common/block/include/intelblocks/cse.h
index a67010c..f554933 100644
--- a/src/soc/intel/common/block/include/intelblocks/cse.h
+++ b/src/soc/intel/common/block/include/intelblocks/cse.h
@@ -121,12 +121,10 @@
 };

/*
- * Sends GLOBAL_RESET_REQ cmd to CSE.
- * The reset type can be one of the above defined reset type.
+ * Sends GLOBAL_RESET_REQ cmd to CSE with reset type GLOBAL_RESET.
  * Returns 0 on failure and 1 on success.
  */
-int cse_request_global_reset(enum rst_req_type rst_type);
-
+int cse_request_global_reset(void);
 /*
  * Sends HMRFPO_ENABLE command.
  * HMRFPO - Host ME Region Flash Protection Override.
diff --git a/src/soc/intel/elkhartlake/reset.c b/src/soc/intel/elkhartlake/reset.c
index 107db5a..fe3d769 100644
--- a/src/soc/intel/elkhartlake/reset.c
+++ b/src/soc/intel/elkhartlake/reset.c
@@ -11,7 +11,7 @@
 void do_global_reset(void)
 {
    /* Ask CSE to do the global reset */
-	if (cse_request_global_reset(GLOBAL_RESET))
+	if (cse_request_global_reset())
    	return;

/* global reset if CSE fail to reset */
diff --git a/src/soc/intel/icelake/reset.c b/src/soc/intel/icelake/reset.c
index d37ff54..1f7ea3c 100644
--- a/src/soc/intel/icelake/reset.c
+++ b/src/soc/intel/icelake/reset.c
@@ -11,7 +11,7 @@
 void do_global_reset(void)
 {
    /* Ask CSE to do the global reset */
-	if (cse_request_global_reset(GLOBAL_RESET))
+	if (cse_request_global_reset())
    	return;

/* global reset if CSE fail to reset */
diff --git a/src/soc/intel/jasperlake/reset.c b/src/soc/intel/jasperlake/reset.c
index d37ff54..1f7ea3c 100644
--- a/src/soc/intel/jasperlake/reset.c
+++ b/src/soc/intel/jasperlake/reset.c
@@ -11,7 +11,7 @@
 void do_global_reset(void)
 {
    /* Ask CSE to do the global reset */
-	if (cse_request_global_reset(GLOBAL_RESET))
+	if (cse_request_global_reset())
    	return;

/* global reset if CSE fail to reset */
diff --git a/src/soc/intel/skylake/me.c b/src/soc/intel/skylake/me.c
index 0d6135f..b967d31 100644
--- a/src/soc/intel/skylake/me.c
+++ b/src/soc/intel/skylake/me.c
@@ -353,7 +353,7 @@
    	goto ret;

/* ME should be in Normal Mode for this command */
-	status = cse_request_global_reset(GLOBAL_RESET);
+	status = cse_request_global_reset();
 ret:
    return status;
 }
diff --git a/src/soc/intel/tigerlake/reset.c b/src/soc/intel/tigerlake/reset.c
index d37ff54..1f7ea3c 100644
--- a/src/soc/intel/tigerlake/reset.c
+++ b/src/soc/intel/tigerlake/reset.c
@@ -11,7 +11,7 @@
 void do_global_reset(void)
 {
    /* Ask CSE to do the global reset */
-	if (cse_request_global_reset(GLOBAL_RESET))
+	if (cse_request_global_reset())
    	return;

/* global reset if CSE fail to reset */

Change in coreboot[master]: soc/intel/common/block/cse: Refactor cse_request_global_reset() function

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45341 )

Change subject: soc/intel/common/block/cse: Refactor cse_request_global_reset() function
......................................................................

Patch Set 12:

Automatic boot test returned (PASS/FAIL/TOTAL): 8/1/9
"QEMU x86 q35/ich9" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/20037
"QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/20036
"QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/20035
"QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/20034
"QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/20033
"HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/20041
"HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/20040
"HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/20039
"HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/20038

Please note: This test is under development and might not be accurate at all!