Change in coreboot[master]: soc/intel/common/block/cse: Rename set_host_ready() to cse_set_host_r...

[Sridhar Siricilla (Code Review)]

Hello Rizwan Qureshi,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: soc/intel/common/block/cse: Rename set_host_ready() to cse_set_host_ready()
......................................................................

soc/intel/common/block/cse: Rename set_host_ready() to cse_set_host_ready()

Below changes are done:
1. Rename set_host_ready() function to cse_set_host_ready()
2. Additional debug messages are added

TEST=Build and Boot hatch board.

Change-Id: Icfcf1631cc37faacdea9ad84be55f5710104bad5
Signed-off-by: Rizwan Qureshi 
Signed-off-by: Sridhar Siricilla 
---
M src/soc/intel/common/block/cse/cse.c
M src/soc/intel/common/block/include/intelblocks/cse.h
2 files changed, 7 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/37282/1

diff --git a/src/soc/intel/common/block/cse/cse.c b/src/soc/intel/common/block/cse/cse.c
index f81411a..46ad8ce 100644
--- a/src/soc/intel/common/block/cse/cse.c
+++ b/src/soc/intel/common/block/cse/cse.c
@@ -272,7 +272,7 @@
 }

/* Makes the host ready to communicate with CSE */
-void set_host_ready(void)
+void cse_set_host_ready(void)
 {
    uint32_t csr;
    csr = read_host_csr();
@@ -286,10 +286,13 @@
 {
    struct stopwatch sw;
    stopwatch_init_msecs_expire(&sw, HECI_DELAY_READY);
+	printk(BIOS_ERR, "HECI: Wait 15 secs for CSE to enter SEC_OVERRIDE mode!\n");
    while (!check_cse_sec_override_mode()) {
    	udelay(HECI_DELAY);
-		if (stopwatch_expired(&sw))
+		if (stopwatch_expired(&sw)) {
+			printk(BIOS_ERR, "HECI: Timed out!\n");
    		return 0;
+		}
    }

return 1;
@@ -545,7 +548,7 @@

if (wait_heci_ready()) {
    	/* Device is back on its imaginary feet, clear reset */
-		set_host_ready();
+		cse_set_host_ready();
    	return 1;
    }

diff --git a/src/soc/intel/common/block/include/intelblocks/cse.h b/src/soc/intel/common/block/include/intelblocks/cse.h
index 4c00006..3c00b87 100644
--- a/src/soc/intel/common/block/include/intelblocks/cse.h
+++ b/src/soc/intel/common/block/include/intelblocks/cse.h
@@ -112,7 +112,7 @@

/* Makes the host ready to communicate with CSE*/
-void set_host_ready(void);
+void cse_set_host_ready(void);

/*
  * Polls for ME state 'HECI_OP_MODE_SEC_OVERRIDE' for 15 seconds.

[Sridhar Siricilla (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Paul Menzel, Rizwan Qureshi, V Sowmya, build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi,

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

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

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

Change subject: soc/intel/common/block/cse: Rename set_host_ready() to cse_set_host_ready()
......................................................................

soc/intel/common/block/cse: Rename set_host_ready() to cse_set_host_ready()

Below changes are done:
1. Rename set_host_ready() function to cse_set_host_ready()
2. Additional debug messages are added

TEST=Build and Boot hatch board.

Change-Id: Icfcf1631cc37faacdea9ad84be55f5710104bad5
Signed-off-by: Rizwan Qureshi 
Signed-off-by: Sridhar Siricilla 
---
M src/soc/intel/common/block/cse/cse.c
M src/soc/intel/common/block/include/intelblocks/cse.h
2 files changed, 7 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/37282/2

[Sridhar Siricilla (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Paul Menzel, Rizwan Qureshi, V Sowmya, build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi,

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

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

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

Change subject: soc/intel/common/block/cse: Rename set_host_ready() to cse_set_host_ready()
......................................................................

soc/intel/common/block/cse: Rename set_host_ready() to cse_set_host_ready()

Below changes are done:
1. Rename set_host_ready() function to cse_set_host_ready()
2. Additional debug messages are added

TEST=Build and Boot hatch board.

Change-Id: Icfcf1631cc37faacdea9ad84be55f5710104bad5
Signed-off-by: Rizwan Qureshi 
Signed-off-by: Sridhar Siricilla 
---
M src/soc/intel/common/block/cse/cse.c
M src/soc/intel/common/block/include/intelblocks/cse.h
2 files changed, 7 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/37282/3

[Nico Huber (Code Review)]

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

Change subject: soc/intel/common/block/cse: Rename set_host_ready() to cse_set_host_ready()
......................................................................

Patch Set 3:

(3 comments)

https://review.coreboot.org/c/coreboot/+/37282/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/37282/3//COMMIT_MSG@7 
PS3, Line 7: soc/intel/common/block/cse: Rename set_host_ready() to cse_set_host_ready()
The line is too long, 72 is the maximum and <65 is recommended.

The prefix doesn't have to be a full path, `intel/cse` is all that
matters.

https://review.coreboot.org/c/coreboot/+/37282/3//COMMIT_MSG@11 
PS3, Line 11: 2. Additional debug messages are added
These are not even in set_host_ready(), please find a better
summary line that doesn't hide changes. Or even better, don't
list unrelated changes but make them individual commits.

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

https://review.coreboot.org/c/coreboot/+/37282/3/src/soc/intel/common/block/... 
PS3, Line 289: BIOS_ERR
Not an error message.

Change in coreboot[master]: soc/intel/common: Rename set_host_ready() to cse_set_host_ready()

[Sridhar Siricilla (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Sridhar Siricilla, Paul Menzel, Rizwan Qureshi, V Sowmya, build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi,

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

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

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

Change subject: soc/intel/common: Rename set_host_ready() to cse_set_host_ready()
......................................................................

soc/intel/common: Rename set_host_ready() to cse_set_host_ready()

Below changes are done:
1. Rename set_host_ready() function to cse_set_host_ready()
2. Additional debug messages are added in wait_cse_sec_override_mode()

TEST=Build and Boot hatch board.

Change-Id: Icfcf1631cc37faacdea9ad84be55f5710104bad5
Signed-off-by: Rizwan Qureshi 
Signed-off-by: Sridhar Siricilla 
---
M src/soc/intel/common/block/cse/cse.c
M src/soc/intel/common/block/include/intelblocks/cse.h
2 files changed, 7 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/37282/4

Change in coreboot[master]: soc/intel/common: Rename set_host_ready() to cse_set_host_ready()

[Sridhar Siricilla (Code Review)]

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

Change subject: soc/intel/common: Rename set_host_ready() to cse_set_host_ready()
......................................................................

Patch Set 4:

(3 comments)

https://review.coreboot.org/c/coreboot/+/37282/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/37282/3//COMMIT_MSG@7 
PS3, Line 7: soc/intel/common/block/cse: Rename set_host_ready() to cse_set_host_ready()
> The line is too long, 72 is the maximum and <65 is recommended. […]

Done

https://review.coreboot.org/c/coreboot/+/37282/3//COMMIT_MSG@11 
PS3, Line 11: 2. Additional debug messages are added
> These are not even in set_host_ready(), please find a better […]

Done

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

https://review.coreboot.org/c/coreboot/+/37282/3/src/soc/intel/common/block/... 
PS3, Line 289: BIOS_ERR
> Not an error message.

Done

Change in coreboot[master]: soc/intel/common: Rename set_host_ready() to cse_set_host_ready()

[Nico Huber (Code Review)]

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

Change subject: soc/intel/common: Rename set_host_ready() to cse_set_host_ready()
......................................................................

Patch Set 4: Code-Review+1

Change in coreboot[master]: soc/intel/common: Rename set_host_ready() to cse_set_host_ready()

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/common: Rename set_host_ready() to cse_set_host_ready()
......................................................................

Patch Set 4:

(13 comments)

Some of the comments I provided might impact other SoC code. Please feel free to submit those changes as separate CLs.

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

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 288: HECI_DELAY_READY
Just curious: How was this 15 second delay/wait time determined? I don't find any reference to it in BWG.

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 297: 
Since you are adding debug prints, it might be helpful to have a print that says how much time it took for cse to be ready in override mode here.

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

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 61: HECI
MKHI

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 104: me
While you are cleaning up names, may be update this to cse_.

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 111: is_cse_enabled
For consistency: cse_is_enabled()

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 114: *
nit: space before *

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 121: wait_cse_sec_override_mode
For consistency: cse_wait_sec_override_mode()

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 128: send_heci_reset_req_message
I think this should be: cse_request_global_reset() to match what BWG says. But I don't see Host reset or CSE reset only options in it. Can you please point me to the right doc?

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 134: send_hmrfpo_enable_msg
For consistency: cse_hmrfpo_enable()

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 141: send_hmrfpo_get_status_msg
For consistency: cse_hmrfpo_get_status()

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 144: BIOS_HOST_ADDR
I think this should be HECI_HOST_ADDR for consistency.

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 147: /* Command GLOBAL_RESET_REQ Reset Types */
             : #define GLOBAL_RESET       1
             : #define HOST_RESET_ONLY    2
             : #define CSE_RESET_ONLY     3
These should be placed closer to send_heci_reset_req_message(). Actually, I think having an enum here might be helpful and then the param to send_heci_reset_req_message() can be of the enum type.

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 152: *
nit: space after *

Change in coreboot[master]: soc/intel/common: Rename funtions to have consistency naming

[Sridhar Siricilla (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Sridhar Siricilla, Paul Menzel, Rizwan Qureshi, V Sowmya, build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi,

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

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

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

Change subject: soc/intel/common: Rename funtions to have consistency naming
......................................................................

soc/intel/common: Rename funtions to have consistency naming

Below changes are done:
1. Rename below funtions to have consistency naming:
    set_host_ready() -> cse_set_host_ready()
        wait_cse_sec_override_mode() -> cse_wait_sec_override_mode()
    send_hmrfpo_enable_msg()     -> cse_hmrfpo_enable()
    send_hmrfpo_get_status_msg() -> cse_hmrfpo_get_status()

2. Additional debug messages are added in cse_wait_sec_override_mode().

TEST=Build and Boot hatch board.

Change-Id: Icfcf1631cc37faacdea9ad84be55f5710104bad5
Signed-off-by: Rizwan Qureshi 
Signed-off-by: Sridhar Siricilla 
---
M src/soc/intel/common/block/cse/cse.c
M src/soc/intel/common/block/include/intelblocks/cse.h
2 files changed, 17 insertions(+), 13 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/37282/6

Change in coreboot[master]: soc/intel/common: Rename funtions to have consistency naming

[Sridhar Siricilla (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Sridhar Siricilla, Paul Menzel, Rizwan Qureshi, V Sowmya, build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi,

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

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

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

Change subject: soc/intel/common: Rename funtions to have consistency naming
......................................................................

soc/intel/common: Rename funtions to have consistency naming

Below changes are done:
1. Rename below funtions to have consistency naming:
    set_host_ready() -> cse_set_host_ready()
        wait_cse_sec_override_mode() -> cse_wait_sec_override_mode()
    send_hmrfpo_enable_msg()     -> cse_hmrfpo_enable()
    send_hmrfpo_get_status_msg() -> cse_hmrfpo_get_status()

2. Additional debug messages are added in cse_wait_sec_override_mode().

TEST=Build and Boot hatch board.

Change-Id: Icfcf1631cc37faacdea9ad84be55f5710104bad5
Signed-off-by: Rizwan Qureshi 
Signed-off-by: Sridhar Siricilla 
---
M src/soc/intel/common/block/cse/cse.c
M src/soc/intel/common/block/include/intelblocks/cse.h
2 files changed, 17 insertions(+), 13 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/37282/7

Change in coreboot[master]: soc/intel/common: Rename functions to have consistency naming

[Sridhar Siricilla (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Sridhar Siricilla, Paul Menzel, Rizwan Qureshi, V Sowmya, build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi,

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

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

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

Change subject: soc/intel/common: Rename functions to have consistency naming
......................................................................

soc/intel/common: Rename functions to have consistency naming

Below changes are done:
1. Rename below funtions to have consistency naming:
    set_host_ready() -> cse_set_host_ready()
        wait_cse_sec_override_mode() -> cse_wait_sec_override_mode()
    send_hmrfpo_enable_msg()     -> cse_hmrfpo_enable()
    send_hmrfpo_get_status_msg() -> cse_hmrfpo_get_status()

2. Additional debug messages are added in cse_wait_sec_override_mode().

TEST=Build and Boot hatch board.

Change-Id: Icfcf1631cc37faacdea9ad84be55f5710104bad5
Signed-off-by: Rizwan Qureshi 
Signed-off-by: Sridhar Siricilla 
---
M src/soc/intel/common/block/cse/cse.c
M src/soc/intel/common/block/include/intelblocks/cse.h
2 files changed, 17 insertions(+), 13 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/37282/10

Change in coreboot[master]: soc/intel/common: Rename functions to have consistency naming

[Sridhar Siricilla (Code Review)]

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

Change subject: soc/intel/common: Rename functions to have consistency naming
......................................................................

Patch Set 12:

(13 comments)

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

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 288: HECI_DELAY_READY
> Just curious: How was this 15 second delay/wait time determined? I don't find any reference to it in […]

It's not available in BWG.
Used the same timeout as the one used for wait_heci_ready.

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 297:
> Since you are adding debug prints, it might be helpful to have a print that says how much time it to […]

Done

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

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 61: HECI
> MKHI

Done

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 104: me
> While you are cleaning up names, may be update this to cse_.

This will be addressed in the separate patch later as there are multiple references to the function.

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 111: is_cse_enabled
> For consistency: cse_is_enabled()

This will be addressed in separate patch later as this function being referenced in many places.

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 114: *
> nit: space before *

Done

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 121: wait_cse_sec_override_mode
> For consistency: cse_wait_sec_override_mode()

Done

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 128: send_heci_reset_req_message
> I think this should be: cse_request_global_reset() to match what BWG says. […]

Addressed naming part in the patch : https://review.coreboot.org/c/coreboot/+/37584
Currently, the Host reset/CSE reset only options are not included in the BWG guide.

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 134: send_hmrfpo_enable_msg
> For consistency: cse_hmrfpo_enable()

Done

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 141: send_hmrfpo_get_status_msg
> For consistency: cse_hmrfpo_get_status()

Done

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 144: BIOS_HOST_ADDR
> I think this should be HECI_HOST_ADDR for consistency.

It's a value of Host_Address field of Fixed Address MEI Header. So, macro rename is not required.

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 147: /* Command GLOBAL_RESET_REQ Reset Types */
             : #define GLOBAL_RESET       1
             : #define HOST_RESET_ONLY    2
             : #define CSE_RESET_ONLY     3
> These should be placed closer to send_heci_reset_req_message(). […]

The comment is addressed in the patch : https://review.coreboot.org/c/coreboot/+/36786

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 152: *
> nit: space after *

Addressed the change in in the https://review.coreboot.org/c/coreboot/+/38248/1

Change in coreboot[master]: soc/intel/common: Rename functions to have consistency naming

[Sridhar Siricilla (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Sridhar Siricilla, Paul Menzel, Rizwan Qureshi, V Sowmya, build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi,

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

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

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

Change subject: soc/intel/common: Rename functions to have consistency naming
......................................................................

soc/intel/common: Rename functions to have consistency naming

Below changes are done:
1. Rename below funtions to have consistency naming:
    set_host_ready() -> cse_set_host_ready()
        wait_cse_sec_override_mode() -> cse_wait_sec_override_mode()
    send_hmrfpo_enable_msg()     -> cse_hmrfpo_enable()
    send_hmrfpo_get_status_msg() -> cse_hmrfpo_get_status()

2. Additional debug messages are added in cse_wait_sec_override_mode().

TEST=Build and Boot hatch board.

Change-Id: Icfcf1631cc37faacdea9ad84be55f5710104bad5
Signed-off-by: Rizwan Qureshi 
Signed-off-by: Sridhar Siricilla 
---
M src/soc/intel/common/block/cse/cse.c
M src/soc/intel/common/block/include/intelblocks/cse.h
2 files changed, 17 insertions(+), 13 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/37282/14

Change in coreboot[master]: soc/intel/common: Rename functions to have consistency naming

[Sridhar Siricilla (Code Review)]

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

Change subject: soc/intel/common: Rename functions to have consistency naming
......................................................................

Patch Set 14:

(4 comments)

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

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 297:
> Done

Done

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

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 144: BIOS_HOST_ADDR
> It's a value of Host_Address field of Fixed Address MEI Header. So, macro rename is not required.

Done

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 147: /* Command GLOBAL_RESET_REQ Reset Types */
             : #define GLOBAL_RESET       1
             : #define HOST_RESET_ONLY    2
             : #define CSE_RESET_ONLY     3
> The comment is addressed in the patch : https://review.coreboot. […]

Done

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 152: *
> Addressed the change in in the https://review.coreboot. […]

Done

Change in coreboot[master]: soc/intel/common: Rename functions to have consistency naming

[Sridhar Siricilla (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Sridhar Siricilla, Paul Menzel, Rizwan Qureshi, V Sowmya, build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi,

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

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

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

Change subject: soc/intel/common: Rename functions to have consistency naming
......................................................................

soc/intel/common: Rename functions to have consistency naming

Below changes are done:
1. Rename below funtions to have consistency naming:
    set_host_ready() -> cse_set_host_ready()
        wait_cse_sec_override_mode() -> cse_wait_sec_override_mode()
    send_hmrfpo_enable_msg()     -> cse_hmrfpo_enable()
    send_hmrfpo_get_status_msg() -> cse_hmrfpo_get_status()

2. Additional debug messages are added in cse_wait_sec_override_mode().

TEST=Build and Boot hatch board.

Change-Id: Icfcf1631cc37faacdea9ad84be55f5710104bad5
Signed-off-by: Rizwan Qureshi 
Signed-off-by: Sridhar Siricilla 
---
M src/soc/intel/common/block/cse/cse.c
M src/soc/intel/common/block/include/intelblocks/cse.h
2 files changed, 17 insertions(+), 13 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/37282/15

Change in coreboot[master]: soc/intel/common: Rename functions to have consistency naming

[Sridhar Siricilla (Code Review)]

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

Change subject: soc/intel/common: Rename functions to have consistency naming
......................................................................

Patch Set 15:

(4 comments)

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

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 288: HECI_DELAY_READY
> It's not available in BWG. […]

Done

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

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 104: me
> This will be addressed in the separate patch later as there are multiple references to the function.

Done

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 111: is_cse_enabled
> This will be addressed in separate patch later as this function being referenced in many places.

Done

https://review.coreboot.org/c/coreboot/+/37282/4/src/soc/intel/common/block/... 
PS4, Line 128: send_heci_reset_req_message
> Addressed naming part in the patch : https://review.coreboot.org/c/coreboot/+/37584 […]

Done

Change in coreboot[master]: soc/intel/common: Rename functions to have consistency naming

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/common: Rename functions to have consistency naming
......................................................................

Patch Set 15: Code-Review+2

Change in coreboot[master]: soc/intel/common: Rename functions to have consistency naming

[Paul Menzel (Code Review)]

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

Change subject: soc/intel/common: Rename functions to have consistency naming
......................................................................

Patch Set 15:

(4 comments)

https://review.coreboot.org/c/coreboot/+/37282/15//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/37282/15//COMMIT_MSG@7 
PS15, Line 7: to have consistency naming
for consistent naming

https://review.coreboot.org/c/coreboot/+/37282/15//COMMIT_MSG@10 
PS15, Line 10: consistency
consistent

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

https://review.coreboot.org/c/coreboot/+/37282/15/src/soc/intel/common/block... 
PS15, Line 257: 15
HECI_DELAY

https://review.coreboot.org/c/coreboot/+/37282/15/src/soc/intel/common/block... 
PS15, Line 261: 			printk(BIOS_ERR, "HECI: Timed out!\n");
I know it’s in the above message already, but if only error level debugging is used, please also add HECI_DELAY here.

Change in coreboot[master]: soc/intel/common: Rename functions for consistent naming

[Sridhar Siricilla (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Sridhar Siricilla, Paul Menzel, Rizwan Qureshi, V Sowmya, build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi,

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

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

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

Change subject: soc/intel/common: Rename functions for consistent naming
......................................................................

soc/intel/common: Rename functions for consistent naming

Below changes are done:
1. Rename below functions to have consistent naming:
    set_host_ready() -> cse_set_host_ready()
        wait_cse_sec_override_mode() -> cse_wait_sec_override_mode()
    send_hmrfpo_enable_msg()     -> cse_hmrfpo_enable()
    send_hmrfpo_get_status_msg() -> cse_hmrfpo_get_status()

2. Additional debug messages are added in cse_wait_sec_override_mode().

TEST=Build and Boot hatch board.

Change-Id: Icfcf1631cc37faacdea9ad84be55f5710104bad5
Signed-off-by: Rizwan Qureshi 
Signed-off-by: Sridhar Siricilla 
---
M src/soc/intel/common/block/cse/cse.c
M src/soc/intel/common/block/include/intelblocks/cse.h
2 files changed, 19 insertions(+), 14 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/37282/16

Change in coreboot[master]: soc/intel/common: Rename functions for consistent naming

[Sridhar Siricilla (Code Review)]

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

Change subject: soc/intel/common: Rename functions for consistent naming
......................................................................

Patch Set 16:

(4 comments)

https://review.coreboot.org/c/coreboot/+/37282/15//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/37282/15//COMMIT_MSG@7 
PS15, Line 7: to have consistency naming
> for consistent naming

Done

https://review.coreboot.org/c/coreboot/+/37282/15//COMMIT_MSG@10 
PS15, Line 10: consistency
> consistent

Done

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

https://review.coreboot.org/c/coreboot/+/37282/15/src/soc/intel/common/block... 
PS15, Line 257: 15
> HECI_DELAY

Done

https://review.coreboot.org/c/coreboot/+/37282/15/src/soc/intel/common/block... 
PS15, Line 261: 			printk(BIOS_ERR, "HECI: Timed out!\n");
> I know it’s in the above message already, but if only error level debugging is used, please also add […]

Done

Change in coreboot[master]: soc/intel/common: Rename functions for consistent naming

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/common: Rename functions for consistent naming
......................................................................

Patch Set 17: Code-Review+2

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/37282/17/src/soc/intel/common/block... 
PS17, Line 293: printk(BIOS_DEBUG, "HECI: Wait %d secs for CSE to enter SEC_OVERRIDE mode!\n",
              : 			HECI_DELAY);
nit: Just curious if this message is really useful. You already have prints in both successful and error paths below.

Change in coreboot[master]: soc/intel/common: Rename functions for consistent naming

[Sridhar Siricilla (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Sridhar Siricilla, Paul Menzel, Rizwan Qureshi, V Sowmya, build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi,

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

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

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

Change subject: soc/intel/common: Rename functions for consistent naming
......................................................................

soc/intel/common: Rename functions for consistent naming

Below changes are done:
1. Rename below functions to have consistent naming:
    set_host_ready() -> cse_set_host_ready()
        wait_cse_sec_override_mode() -> cse_wait_sec_override_mode()
    send_hmrfpo_enable_msg()     -> cse_hmrfpo_enable()
    send_hmrfpo_get_status_msg() -> cse_hmrfpo_get_status()

2. Additional debug messages are added in cse_wait_sec_override_mode().

TEST=Build and Boot hatch board.

Change-Id: Icfcf1631cc37faacdea9ad84be55f5710104bad5
Signed-off-by: Rizwan Qureshi 
Signed-off-by: Sridhar Siricilla 
---
M src/soc/intel/common/block/cse/cse.c
M src/soc/intel/common/block/include/intelblocks/cse.h
2 files changed, 17 insertions(+), 14 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/37282/18

Change in coreboot[master]: soc/intel/common: Rename functions for consistent naming

[Sridhar Siricilla (Code Review)]

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

Change subject: soc/intel/common: Rename functions for consistent naming
......................................................................

Patch Set 18:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/37282/17/src/soc/intel/common/block... 
PS17, Line 293: printk(BIOS_DEBUG, "HECI: Wait %d secs for CSE to enter SEC_OVERRIDE mode!\n",
              : 			HECI_DELAY);
> nit: Just curious if this message is really useful. […]

Agreed,removed the debug statement.

Change in coreboot[master]: soc/intel/common: Rename functions for consistent naming

[Sridhar Siricilla (Code Review)]

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

Change subject: soc/intel/common: Rename functions for consistent naming
......................................................................

Patch Set 18:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/37282/17/src/soc/intel/common/block... 
PS17, Line 293: printk(BIOS_DEBUG, "HECI: Wait %d secs for CSE to enter SEC_OVERRIDE mode!\n",
              : 			HECI_DELAY);
> Agreed,removed the debug statement.

Done

Change in coreboot[master]: soc/intel/common: Rename functions for consistent naming

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/common: Rename functions for consistent naming
......................................................................

Patch Set 18: Code-Review+2

Change in coreboot[master]: soc/intel/common: Rename functions for consistent naming

[Patrick Georgi (Code Review)]

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

Change subject: soc/intel/common: Rename functions for consistent naming
......................................................................

soc/intel/common: Rename functions for consistent naming

Below changes are done:
1. Rename below functions to have consistent naming:
    set_host_ready() -> cse_set_host_ready()
        wait_cse_sec_override_mode() -> cse_wait_sec_override_mode()
    send_hmrfpo_enable_msg()     -> cse_hmrfpo_enable()
    send_hmrfpo_get_status_msg() -> cse_hmrfpo_get_status()

2. Additional debug messages are added in cse_wait_sec_override_mode().

TEST=Build and Boot hatch board.

Change-Id: Icfcf1631cc37faacdea9ad84be55f5710104bad5
Signed-off-by: Rizwan Qureshi 
Signed-off-by: Sridhar Siricilla 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/37282
Tested-by: build bot (Jenkins) 
Reviewed-by: Furquan Shaikh 
---
M src/soc/intel/common/block/cse/cse.c
M src/soc/intel/common/block/include/intelblocks/cse.h
2 files changed, 17 insertions(+), 14 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: 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 440b59b..8fbaba5 100644
--- a/src/soc/intel/common/block/cse/cse.c
+++ b/src/soc/intel/common/block/cse/cse.c
@@ -271,7 +271,7 @@
 }

/* Makes the host ready to communicate with CSE */
-void set_host_ready(void)
+void cse_set_host_ready(void)
 {
    uint32_t csr;
    csr = read_host_csr();
@@ -280,17 +280,20 @@
    write_host_csr(csr);
 }

-/* Polls for ME state 'HECI_OP_MODE_SEC_OVERRIDE' for 15 seconds */
-uint8_t wait_cse_sec_override_mode(void)
+/* Polls for ME mode ME_HFS1_COM_SECOVER_MEI_MSG for 15 seconds */
+uint8_t cse_wait_sec_override_mode(void)
 {
    struct stopwatch sw;
    stopwatch_init_msecs_expire(&sw, HECI_DELAY_READY);
    while (!cse_is_hfs1_com_secover_mei_msg()) {
    	udelay(HECI_DELAY);
-		if (stopwatch_expired(&sw))
+		if (stopwatch_expired(&sw)) {
+			printk(BIOS_ERR, "HECI: Timed out waiting for SEC_OVERRIDE mode!\n");
    		return 0;
+		}
    }
-
+	printk(BIOS_DEBUG, "HECI: CSE took %lu ms to enter security override mode\n",
+			stopwatch_duration_msecs(&sw));
    return 1;
 }

@@ -544,7 +547,7 @@

if (wait_heci_ready()) {
    	/* Device is back on its imaginary feet, clear reset */
-		set_host_ready();
+		cse_set_host_ready();
    	return 1;
    }

@@ -622,7 +625,7 @@
 }

/* Sends HMRFPO Enable command to CSE */
-int send_hmrfpo_enable_msg(void)
+int cse_hmrfpo_enable(void)
 {
    struct hmrfpo_enable_msg {
    	struct mkhi_hdr hdr;
@@ -682,7 +685,7 @@
  * Sends HMRFPO Get Status command to CSE to get the HMRFPO status.
  * The status can be DISABLES/LOCKED/ENABLED
  */
-int send_hmrfpo_get_status_msg(void)
+int cse_hmrfpo_get_status(void)
 {
    struct hmrfpo_get_status_msg {
    	struct mkhi_hdr hdr;
diff --git a/src/soc/intel/common/block/include/intelblocks/cse.h b/src/soc/intel/common/block/include/intelblocks/cse.h
index 4673070..6233f7d 100644
--- a/src/soc/intel/common/block/include/intelblocks/cse.h
+++ b/src/soc/intel/common/block/include/intelblocks/cse.h
@@ -51,7 +51,7 @@
    PCI_ME_HFSTS6 = 0x6C,
 };

-/* HECI Message Header */
+/* MKHI Message Header */
 struct mkhi_hdr {
    uint8_t group_id;
    uint8_t command:7;
@@ -103,14 +103,14 @@
  */
 bool is_cse_enabled(void);

-/* Makes the host ready to communicate with CSE*/
-void set_host_ready(void);
+/* Makes the host ready to communicate with CSE */
+void cse_set_host_ready(void);

/*
  * Polls for ME state 'HECI_OP_MODE_SEC_OVERRIDE' for 15 seconds.
  * Returns 0 on failure and 1 on success.
  */
-uint8_t wait_cse_sec_override_mode(void);
+uint8_t cse_wait_sec_override_mode(void);

/*
  * Sends GLOBAL_RESET_REQ cmd to CSE.The reset type can be
@@ -123,14 +123,14 @@
  * Send HMRFPO_ENABLE command.
  * returns 0 on failure and 1 on success.
  */
-int send_hmrfpo_enable_msg(void);
+int cse_hmrfpo_enable(void);

/*
  * Send HMRFPO_GET_STATUS command.
  * returns -1 on failure and 0 (DISABLED)/ 1 (LOCKED)/ 2 (ENABLED)
  * on success.
  */
-int send_hmrfpo_get_status_msg(void);
+int cse_hmrfpo_get_status(void);

/* Fixed Address MEI Header's Host Address field value */
 #define BIOS_HOST_ADDR	0x00

Change in coreboot[master]: soc/intel/common: Rename functions for consistent naming

[9elements QA (Code Review)]

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

Change subject: soc/intel/common: Rename functions for consistent naming
......................................................................

Patch Set 19:

Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3
Emulation targets:
EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/517
EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/516
EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/515

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