Change in coreboot[master]: src/soc/intel/(cnl,skl,icl): Remove, and call send_heci_reset_message...
Hello Rizwan Qureshi,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/35228
to review the following change.
Change subject: src/soc/intel/(cnl,skl,icl): Remove, and call send_heci_reset_message() defined in common code ......................................................................
src/soc/intel/(cnl,skl,icl): Remove, and call send_heci_reset_message() defined in common code
Removed send_heci_reset_message() from socs(cnl/skl/icl), and made changes to call function send_heci_reset_req_message() defined in the common code.
Change-Id: I691fc0610356ef1f64ffa7cc4fe7a39b1344cc16 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: sridhar sridhar.siricilla@intel.com --- M src/soc/intel/cannonlake/reset.c M src/soc/intel/icelake/reset.c M src/soc/intel/skylake/include/soc/me.h M src/soc/intel/skylake/me.c 4 files changed, 3 insertions(+), 166 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/35228/1
diff --git a/src/soc/intel/cannonlake/reset.c b/src/soc/intel/cannonlake/reset.c index e92396f3..e01c22c 100644 --- a/src/soc/intel/cannonlake/reset.c +++ b/src/soc/intel/cannonlake/reset.c @@ -22,65 +22,10 @@ #include <string.h> #include <soc/pci_devs.h>
-/* Reset Request */ -#define MKHI_GLOBAL_RESET 0x0b -#define MKHI_STATUS_SUCCESS 0 - -#define GR_ORIGIN_BIOS_MEM_INIT 0x01 -#define GR_ORIGIN_BIOS_POST 0x02 -#define GR_ORIGIN_MEBX 0x03 - -#define GLOBAL_RST_TYPE 0x01 - -#define BIOS_HOST_ADD 0x00 -#define HECI_MKHI_ADD 0x07 - -static int send_heci_reset_message(void) -{ - int status; - struct reset_reply { - u8 group_id; - u8 command; - u8 reserved; - u8 result; - } __packed reply; - struct reset_message { - u8 group_id; - u8 cmd; - u8 reserved; - u8 result; - u8 req_origin; - u8 reset_type; - } __packed; - struct reset_message msg = { - .cmd = MKHI_GLOBAL_RESET, - .req_origin = GR_ORIGIN_BIOS_POST, - .reset_type = GLOBAL_RST_TYPE - }; - size_t reply_size; - - heci_reset(); - - status = heci_send(&msg, sizeof(msg), BIOS_HOST_ADD, HECI_MKHI_ADD); - if (status != 1) - return -1; - - reply_size = sizeof(reply); - memset(&reply, 0, reply_size); - if (!heci_receive(&reply, &reply_size)) - return -1; - if (reply.result != MKHI_STATUS_SUCCESS) { - printk(BIOS_DEBUG, "Returned Mkhi Status is not success!\n"); - return -1; - } - printk(BIOS_DEBUG, "Heci receive success!\n"); - return 0; -} - void do_global_reset(void) { /* Ask CSE to do the global reset */ - if (!send_heci_reset_message()) + if (!send_heci_reset_req_message(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 470b5f4..d83b3ee 100644 --- a/src/soc/intel/icelake/reset.c +++ b/src/soc/intel/icelake/reset.c @@ -22,65 +22,10 @@ #include <string.h> #include <soc/pci_devs.h>
-/* Reset Request */ -#define MKHI_GLOBAL_RESET 0x0b -#define MKHI_STATUS_SUCCESS 0 - -#define GR_ORIGIN_BIOS_MEM_INIT 0x01 -#define GR_ORIGIN_BIOS_POST 0x02 -#define GR_ORIGIN_MEBX 0x03 - -#define GLOBAL_RST_TYPE 0x01 - -#define BIOS_HOST_ADD 0x00 -#define HECI_MKHI_ADD 0x07 - -static int send_heci_reset_message(void) -{ - int status; - struct reset_reply { - u8 group_id; - u8 command; - u8 reserved; - u8 result; - } __packed reply; - struct reset_message { - u8 group_id; - u8 cmd; - u8 reserved; - u8 result; - u8 req_origin; - u8 reset_type; - } __packed; - struct reset_message msg = { - .cmd = MKHI_GLOBAL_RESET, - .req_origin = GR_ORIGIN_BIOS_POST, - .reset_type = GLOBAL_RST_TYPE - }; - size_t reply_size; - - heci_reset(); - - status = heci_send(&msg, sizeof(msg), BIOS_HOST_ADD, HECI_MKHI_ADD); - if (status != 1) - return -1; - - reply_size = sizeof(reply); - memset(&reply, 0, reply_size); - if (!heci_receive(&reply, &reply_size)) - return -1; - if (reply.result != MKHI_STATUS_SUCCESS) { - printk(BIOS_DEBUG, "Returned Mkhi Status is not success!\n"); - return -1; - } - printk(BIOS_DEBUG, "Heci receive success!\n"); - return 0; -} - void do_global_reset(void) { /* Ask CSE to do the global reset */ - if (!send_heci_reset_message()) + if (!send_heci_reset_req_message(GLOBAL_RESET)) return;
/* global reset if CSE fail to reset */ diff --git a/src/soc/intel/skylake/include/soc/me.h b/src/soc/intel/skylake/include/soc/me.h index fbe5033..bec54d8 100644 --- a/src/soc/intel/skylake/include/soc/me.h +++ b/src/soc/intel/skylake/include/soc/me.h @@ -197,17 +197,8 @@
#define MKHI_GEN_GROUP_ID 0xff
-/* Reset Request */ -#define MKHI_GLOBAL_RESET 0x0b - #define MKHI_GET_FW_VERSION 0x02
-#define GR_ORIGIN_BIOS_MEM_INIT 0x01 -#define GR_ORIGIN_BIOS_POST 0x02 -#define GR_ORIGIN_MEBX 0x03 - -#define GLOBAL_RST_TYPE 0x01 - #define BIOS_HOST_ADD 0x00 #define HECI_MKHI_ADD 0x07
diff --git a/src/soc/intel/skylake/me.c b/src/soc/intel/skylake/me.c index e8d75e3..fde4b13 100644 --- a/src/soc/intel/skylake/me.c +++ b/src/soc/intel/skylake/me.c @@ -431,50 +431,6 @@ } }
-static int send_heci_reset_message(void) -{ - int status; - struct reset_reply { - u8 group_id; - u8 command; - u8 reserved; - u8 result; - } __packed reply; - struct reset_message { - u8 group_id; - u8 cmd; - u8 reserved; - u8 result; - u8 req_origin; - u8 reset_type; - } __packed; - struct reset_message msg = { - .cmd = MKHI_GLOBAL_RESET, - .req_origin = GR_ORIGIN_BIOS_POST, - .reset_type = GLOBAL_RST_TYPE - }; - size_t reply_size; - - heci_reset(); - - status = heci_send(&msg, sizeof(msg), BIOS_HOST_ADD, HECI_MKHI_ADD); - if (!status) - return -1; - - reply_size = sizeof(reply); - memset(&reply, 0, reply_size); - status = heci_receive(&reply, &reply_size); - if (!status) - return -1; - /* get reply result from HECI MSG */ - if (reply.result) { - printk(BIOS_DEBUG, "%s: Exit with Failure\n", __func__); - return -1; - } - printk(BIOS_DEBUG, "%s: Exit with Success\n", __func__); - return 0; -} - int send_global_reset(void) { int status = -1; @@ -486,7 +442,7 @@ goto ret;
/* ME should be in Normal Mode for this command */ - status = send_heci_reset_message(); + status = send_heci_reset_req_message(GLOBAL_RESET); ret: return status; }
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/(cnl,skl,icl): Remove, and call send_heci_reset_message() defined in common code ......................................................................
Patch Set 1:
can u add a test line ?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/(cnl,skl,icl): Remove, and call send_heci_reset_message() defined in common code ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35228/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35228/1//COMMIT_MSG@7 PS1, Line 7: Remove, and call send_heci_reset_message() defined in common code Make use of common https://review.coreboot.org/c/coreboot/+/26133() API
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/(cnl,skl,icl): Remove, and call send_heci_reset_message() defined in common code ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35228/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35228/1//COMMIT_MSG@7 PS1, Line 7: Remove, and call send_heci_reset_message() defined in common code
Make use of common https://review.coreboot. […]
bad link
Make use of common send_heci_reset_message() API
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/(cnl,skl,icl): Remove, and call send_heci_reset_message() defined in common code ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35228/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35228/2//COMMIT_MSG@7 PS2, Line 7: src/soc/intel/(cnl,skl,icl): Remove, and call send_heci_reset_message() defined in common code reduce the line length "src/soc/intel/*: call send_heci_reset_message() common code API"
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/(cnl,skl,icl): Remove, and call send_heci_reset_message() defined in common code ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35228/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35228/2//COMMIT_MSG@7 PS2, Line 7: src/soc/intel/(cnl,skl,icl): Remove, and call send_heci_reset_message() defined in common code
reduce the line length […]
add a test line
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35228
to look at the new patch set (#9).
Change subject: src/soc/intel/(cnl,skl,icl): Make use of common send_heci_reset_message() API ......................................................................
src/soc/intel/(cnl,skl,icl): Make use of common send_heci_reset_message() API
Removed send_heci_reset_message() from socs(cnl/skl/icl), and made changes to call function send_heci_reset_req_message() defined in the common code.
TEST=Verified on CML RVP & hatch board
Change-Id: I691fc0610356ef1f64ffa7cc4fe7a39b1344cc16 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- M src/soc/intel/cannonlake/reset.c M src/soc/intel/icelake/reset.c M src/soc/intel/skylake/include/soc/me.h M src/soc/intel/skylake/me.c 4 files changed, 3 insertions(+), 166 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/35228/9
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/(cnl,skl,icl): Make use of common send_heci_reset_message() API ......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35228/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35228/1//COMMIT_MSG@7 PS1, Line 7: Remove, and call send_heci_reset_message() defined in common code
bad link […]
Done
https://review.coreboot.org/c/coreboot/+/35228/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35228/2//COMMIT_MSG@7 PS2, Line 7: src/soc/intel/(cnl,skl,icl): Remove, and call send_heci_reset_message() defined in common code
add a test line
Done
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35228
to look at the new patch set (#14).
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common
Move send_heci_global_reset_req_message() into common code, and remove the same defination from socs(cnl/skl/icl), and made changes to referece to send_heci_reset_req_message().
TEST=Verified sending HECI global message on CML RVP & Hatch board
Change-Id: I691fc0610356ef1f64ffa7cc4fe7a39b1344cc16 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- 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/icelake/reset.c M src/soc/intel/skylake/include/soc/me.h M src/soc/intel/skylake/me.c 6 files changed, 76 insertions(+), 171 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/35228/14
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 14:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35228/14//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35228/14//COMMIT_MSG@10 PS14, Line 10: referece reference OR refer, in any case grammar update required
https://review.coreboot.org/c/coreboot/+/35228/14//COMMIT_MSG@10 PS14, Line 10: defination definition
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35228
to look at the new patch set (#15).
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common
send_heci_global_reset_req_message() is defined in multiple places, hence move it to common code.
TEST=Verified sending HECI global reset message on CML RVP & Hatch board
Change-Id: I691fc0610356ef1f64ffa7cc4fe7a39b1344cc16 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- 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/icelake/reset.c M src/soc/intel/skylake/include/soc/me.h M src/soc/intel/skylake/me.c 6 files changed, 76 insertions(+), 171 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/35228/15
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 15:
(2 comments)
Patch Set 14:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35228/14//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35228/14//COMMIT_MSG@10 PS14, Line 10: defination
definition
Done
https://review.coreboot.org/c/coreboot/+/35228/14//COMMIT_MSG@10 PS14, Line 10: referece
reference OR refer, in any case grammar update required
Done
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35228
to look at the new patch set (#16).
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common
send_heci_global_reset_req_message() is defined in multiple places, hence move it to common code.
TEST=Verified sending HECI global reset message on CML RVP & Hatch board
Change-Id: I691fc0610356ef1f64ffa7cc4fe7a39b1344cc16 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- 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/icelake/reset.c M src/soc/intel/skylake/include/soc/me.h M src/soc/intel/skylake/me.c 6 files changed, 76 insertions(+), 171 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/35228/16
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 16:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35228/16/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35228/16/src/soc/intel/common/block... PS16, Line 571: HOST_RESET_ONLY CSE_RESET_ONLY
https://review.coreboot.org/c/coreboot/+/35228/16/src/soc/intel/common/block... PS16, Line 573: u8 uint8_t, to be consistent with usage in the file.
https://review.coreboot.org/c/coreboot/+/35228/16/src/soc/intel/common/block... PS16, Line 573: send_heci_global_reset_req_message not sure I understood, if the API is termed for global reset , then why are we accepting reset type other than global reset?
or should it be renamed as send_heci_reset_req_message?
https://review.coreboot.org/c/coreboot/+/35228/16/src/soc/intel/common/block... PS16, Line 577: u8 group_id; : u8 command; : u8 reserved; : u8 result; : } __packed reply; : struct reset_message { : u8 group_id; : u8 cmd; : u8 reserved; : u8 result; : u8 req_origin; : u8 reset_type; same? Just to align with usage in file or it will be a mixed bag
https://review.coreboot.org/c/coreboot/+/35228/16/src/soc/intel/common/block... PS16, Line 598: (rst_type == GLOBAL_RESET) braces can go.
https://review.coreboot.org/c/coreboot/+/35228/16/src/soc/intel/common/block... PS16, Line 617: GBL RST Global Reset to be more clear
https://review.coreboot.org/c/coreboot/+/35228/16/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35228/16/src/soc/intel/common/block... PS16, Line 110: HOST_RESET_ONLY CSE_RESET_ONLY?
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 16:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35228/16/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35228/16/src/soc/intel/common/block... PS16, Line 571: HOST_RESET_ONLY
CSE_RESET_ONLY
Done
https://review.coreboot.org/c/coreboot/+/35228/16/src/soc/intel/common/block... PS16, Line 573: send_heci_global_reset_req_message
not sure I understood, if the API is termed for global reset , then why are we accepting reset type […]
The function named after HECI command GLOBAL RESET REQUEST
https://review.coreboot.org/c/coreboot/+/35228/16/src/soc/intel/common/block... PS16, Line 573: u8
uint8_t, to be consistent with usage in the file.
Done
https://review.coreboot.org/c/coreboot/+/35228/16/src/soc/intel/common/block... PS16, Line 577: u8 group_id; : u8 command; : u8 reserved; : u8 result; : } __packed reply; : struct reset_message { : u8 group_id; : u8 cmd; : u8 reserved; : u8 result; : u8 req_origin; : u8 reset_type;
same? Just to align with usage in file or it will be a mixed bag
Done
https://review.coreboot.org/c/coreboot/+/35228/16/src/soc/intel/common/block... PS16, Line 598: (rst_type == GLOBAL_RESET)
braces can go.
Can you be clear on your comment? thanks
https://review.coreboot.org/c/coreboot/+/35228/16/src/soc/intel/common/block... PS16, Line 617: GBL RST
Global Reset to be more clear
Done
https://review.coreboot.org/c/coreboot/+/35228/16/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35228/16/src/soc/intel/common/block... PS16, Line 110: HOST_RESET_ONLY
CSE_RESET_ONLY?
Done
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35228
to look at the new patch set (#18).
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common
send_heci_global_reset_req_message() is defined in multiple places, hence move it to common code.
TEST=Verified sending HECI global reset message on CML RVP & Hatch board
Change-Id: I691fc0610356ef1f64ffa7cc4fe7a39b1344cc16 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- 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/icelake/reset.c M src/soc/intel/skylake/include/soc/me.h M src/soc/intel/skylake/me.c 6 files changed, 76 insertions(+), 171 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/35228/18
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35228/18/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35228/18/src/soc/intel/common/block... PS18, Line 572: int send_heci_global_reset_req_message(uint8_t rst_type) Since this function supports all the HOST, CSE and GLOBAL reset types can we change/retain the earlier function name?
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 18:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35228/18//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35228/18//COMMIT_MSG@12 PS18, Line 12: Verified sending HECI global reset message on CML RVP & Hatch board verified on ICL and SKL or KBL too?
https://review.coreboot.org/c/coreboot/+/35228/16/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35228/16/src/soc/intel/common/block... PS16, Line 598: (rst_type == GLOBAL_RESET)
Done
I meant the braces around the conditions can go and not needed.
rst_type == HOST_RESET_ONLY || rst_type == CSE_RESET_ONLY
https://review.coreboot.org/c/coreboot/+/35228/18/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35228/18/src/soc/intel/common/block... PS18, Line 114: u8 declaration and definition needs to be aligned.
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 18:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35228/18//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35228/18//COMMIT_MSG@12 PS18, Line 12: Verified sending HECI global reset message on CML RVP & Hatch board
verified on ICL and SKL or KBL too?
Plans to verify on ICL/SKL.
https://review.coreboot.org/c/coreboot/+/35228/16/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35228/16/src/soc/intel/common/block... PS16, Line 617: GBL RST
Done
It requires brackets,otherwise bot will complains!
https://review.coreboot.org/c/coreboot/+/35228/18/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35228/18/src/soc/intel/common/block... PS18, Line 572: int send_heci_global_reset_req_message(uint8_t rst_type)
Since this function supports all the HOST, CSE and GLOBAL reset types can we change/retain the earli […]
The function is named to align with HECI command.
https://review.coreboot.org/c/coreboot/+/35228/18/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35228/18/src/soc/intel/common/block... PS18, Line 114: u8
declaration and definition needs to be aligned.
This is named as per HECI command.
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 18:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35228/18//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35228/18//COMMIT_MSG@12 PS18, Line 12: Verified sending HECI global reset message on CML RVP & Hatch board
Plans to verify on ICL/SKL.
Done
https://review.coreboot.org/c/coreboot/+/35228/18/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35228/18/src/soc/intel/common/block... PS18, Line 572: int send_heci_global_reset_req_message(uint8_t rst_type)
The function is named to align with HECI command.
Done
https://review.coreboot.org/c/coreboot/+/35228/18/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35228/18/src/soc/intel/common/block... PS18, Line 114: u8
This is named as per HECI command.
Done
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 18:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35228/18/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35228/18/src/soc/intel/common/block... PS18, Line 569: * Sends GLOBAL_RESET_REQ cmd to CSE.The reset can be GLOBAL_RESET/ reset type
https://review.coreboot.org/c/coreboot/+/35228/18/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35228/18/src/soc/intel/common/block... PS18, Line 114: u8
Done
This needs to be aligned with the declaration here -> https://review.coreboot.org/c/coreboot/+/35228/18/src/soc/intel/common/block...
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35228
to look at the new patch set (#19).
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common
send_heci_reset_req_message() is defined in multiple places, hence move it to common code.
TEST=Verified sending HECI global reset message on CML RVP & Hatch board
Change-Id: I691fc0610356ef1f64ffa7cc4fe7a39b1344cc16 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- 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/icelake/reset.c M src/soc/intel/skylake/include/soc/me.h M src/soc/intel/skylake/me.c 6 files changed, 76 insertions(+), 171 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/35228/19
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 19:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35228/18/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35228/18/src/soc/intel/common/block... PS18, Line 569: * Sends GLOBAL_RESET_REQ cmd to CSE.The reset can be GLOBAL_RESET/
reset type
Done
https://review.coreboot.org/c/coreboot/+/35228/18/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35228/18/src/soc/intel/common/block... PS18, Line 114: u8
This needs to be aligned with the declaration here -> https://review.coreboot. […]
Done
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35228
to look at the new patch set (#20).
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common
send_heci_reset_req_message() is defined in multiple places, hence move it to common code.
TEST=Verified sending HECI global reset message on CML RVP & Hatch board
Change-Id: I691fc0610356ef1f64ffa7cc4fe7a39b1344cc16 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- 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/icelake/reset.c M src/soc/intel/skylake/include/soc/me.h M src/soc/intel/skylake/me.c 6 files changed, 78 insertions(+), 171 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/35228/20
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35228
to look at the new patch set (#21).
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common
send_heci_reset_req_message() is defined in multiple places, hence move it to common code.
TEST=Verified sending HECI global reset message on CML RVP & Hatch board
Change-Id: I691fc0610356ef1f64ffa7cc4fe7a39b1344cc16 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- 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/icelake/reset.c M src/soc/intel/skylake/include/soc/me.h M src/soc/intel/skylake/me.c 6 files changed, 78 insertions(+), 171 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/35228/21
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 21:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35228/21//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35228/21//COMMIT_MSG@12 PS21, Line 12: TEST=Verified sending HECI global reset message on CML RVP & Hatch board Please verify this on ICL and SKL platforms.
https://review.coreboot.org/c/coreboot/+/35228/21/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35228/21/src/soc/intel/common/block... PS21, Line 583: Use space and not tab.
https://review.coreboot.org/c/coreboot/+/35228/21/src/soc/intel/common/block... PS21, Line 585: Use space and not tab for all members of this struct
https://review.coreboot.org/c/coreboot/+/35228/21/src/soc/intel/common/block... PS21, Line 618: print the rst_type
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35228
to look at the new patch set (#22).
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common
send_heci_reset_req_message() is defined in multiple places, hence move it to common code.
TEST=Verified on CMLRVP/Hatch/Soraka/Bobba boards.
Change-Id: I691fc0610356ef1f64ffa7cc4fe7a39b1344cc16 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- 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/icelake/reset.c M src/soc/intel/skylake/include/soc/me.h M src/soc/intel/skylake/me.c 6 files changed, 78 insertions(+), 171 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/35228/22
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 22:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35228/21//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35228/21//COMMIT_MSG@12 PS21, Line 12: TEST=Verified sending HECI global reset message on CML RVP & Hatch board
Please verify this on ICL and SKL platforms.
Verified on Soraka/Bobba/Hatch/CML RVP.
https://review.coreboot.org/c/coreboot/+/35228/21/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35228/21/src/soc/intel/common/block... PS21, Line 583:
Use space and not tab.
Done
https://review.coreboot.org/c/coreboot/+/35228/21/src/soc/intel/common/block... PS21, Line 585:
Use space and not tab for all members of this struct
Done
https://review.coreboot.org/c/coreboot/+/35228/21/src/soc/intel/common/block... PS21, Line 618:
print the rst_type
Not required as another debug log indicates reset type.
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 22:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35228/21/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35228/21/src/soc/intel/common/block... PS21, Line 618:
Not required as another debug log indicates reset type.
Done
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 23:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35228/23//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35228/23//COMMIT_MSG@12 PS23, Line 12: TEST=Verified on CMLRVP/Hatch/Soraka/Bobba boards. Was it validated in ICL aswell? if so could you mention that here.
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35228
to look at the new patch set (#24).
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common
send_heci_reset_req_message() is defined in multiple places, hence move it to common code.
TEST=Verified on CMLRVP/Hatch/Soraka/Bobba/Dragon Egg boards.
Change-Id: I691fc0610356ef1f64ffa7cc4fe7a39b1344cc16 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com --- 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/icelake/reset.c M src/soc/intel/skylake/include/soc/me.h M src/soc/intel/skylake/me.c 6 files changed, 78 insertions(+), 171 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/35228/24
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 23:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35228/21//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35228/21//COMMIT_MSG@12 PS21, Line 12: TEST=Verified sending HECI global reset message on CML RVP & Hatch board
Verified on Soraka/Bobba/Hatch/CML RVP.
Verified on Soraka/Bobba/Hatch/CML RVP/Dragon egg boards
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 24:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35228/23//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35228/23//COMMIT_MSG@12 PS23, Line 12: TEST=Verified on CMLRVP/Hatch/Soraka/Bobba boards.
Was it validated in ICL aswell? if so could you mention that here.
Yes it's validated on ICL (Dragonegg) board as well.
V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 24: Code-Review+2
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 24:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35228/23//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35228/23//COMMIT_MSG@12 PS23, Line 12: TEST=Verified on CMLRVP/Hatch/Soraka/Bobba boards.
Yes it's validated on ICL (Dragonegg) board as well.
Done
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common
send_heci_reset_req_message() is defined in multiple places, hence move it to common code.
TEST=Verified on CMLRVP/Hatch/Soraka/Bobba/Dragon Egg boards.
Change-Id: I691fc0610356ef1f64ffa7cc4fe7a39b1344cc16 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Signed-off-by: Sridhar Siricilla sridhar.siricilla@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35228 Reviewed-by: V Sowmya v.sowmya@intel.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- 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/icelake/reset.c M src/soc/intel/skylake/include/soc/me.h M src/soc/intel/skylake/me.c 6 files changed, 78 insertions(+), 171 deletions(-)
Approvals: build bot (Jenkins): Verified V Sowmya: Looks good to me, approved
diff --git a/src/soc/intel/cannonlake/reset.c b/src/soc/intel/cannonlake/reset.c index e92396f..e01c22c 100644 --- a/src/soc/intel/cannonlake/reset.c +++ b/src/soc/intel/cannonlake/reset.c @@ -22,65 +22,10 @@ #include <string.h> #include <soc/pci_devs.h>
-/* Reset Request */ -#define MKHI_GLOBAL_RESET 0x0b -#define MKHI_STATUS_SUCCESS 0 - -#define GR_ORIGIN_BIOS_MEM_INIT 0x01 -#define GR_ORIGIN_BIOS_POST 0x02 -#define GR_ORIGIN_MEBX 0x03 - -#define GLOBAL_RST_TYPE 0x01 - -#define BIOS_HOST_ADD 0x00 -#define HECI_MKHI_ADD 0x07 - -static int send_heci_reset_message(void) -{ - int status; - struct reset_reply { - u8 group_id; - u8 command; - u8 reserved; - u8 result; - } __packed reply; - struct reset_message { - u8 group_id; - u8 cmd; - u8 reserved; - u8 result; - u8 req_origin; - u8 reset_type; - } __packed; - struct reset_message msg = { - .cmd = MKHI_GLOBAL_RESET, - .req_origin = GR_ORIGIN_BIOS_POST, - .reset_type = GLOBAL_RST_TYPE - }; - size_t reply_size; - - heci_reset(); - - status = heci_send(&msg, sizeof(msg), BIOS_HOST_ADD, HECI_MKHI_ADD); - if (status != 1) - return -1; - - reply_size = sizeof(reply); - memset(&reply, 0, reply_size); - if (!heci_receive(&reply, &reply_size)) - return -1; - if (reply.result != MKHI_STATUS_SUCCESS) { - printk(BIOS_DEBUG, "Returned Mkhi Status is not success!\n"); - return -1; - } - printk(BIOS_DEBUG, "Heci receive success!\n"); - return 0; -} - void do_global_reset(void) { /* Ask CSE to do the global reset */ - if (!send_heci_reset_message()) + if (!send_heci_reset_req_message(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 223eab5..01b2050 100644 --- a/src/soc/intel/common/block/cse/cse.c +++ b/src/soc/intel/common/block/cse/cse.c @@ -69,10 +69,25 @@
#define HECI_OP_MODE_SEC_OVERRIDE 5
+/* Global Reset Command ID */ +#define MKHI_GLOBAL_RESET_REQ 0xb +#define MKHI_GROUP_ID_CBM 0 + +/* RST Origin */ +#define GR_ORIGIN_BIOS_POST 2 + static struct cse_device { uintptr_t sec_bar; } g_cse;
+/* HECI Message Header */ +struct mkhi_hdr { + uint8_t group_id; + uint8_t command:7; + uint8_t is_resp:1; + uint8_t rsvd; + uint8_t result; +} __packed; /* * Initialize the device with provided temporary BAR. If BAR is 0 use a * default. This is intended for pre-mem usage only where BARs haven't been @@ -558,6 +573,52 @@ return pci_read_config32(PCH_DEV_CSE, offset); }
+/* + * Sends GLOBAL_RESET_REQ cmd to CSE.The reset type can be GLOBAL_RESET/ + * HOST_RESET_ONLY/CSE_RESET_ONLY. + */ +int send_heci_reset_req_message(uint8_t rst_type) +{ + int status; + struct mkhi_hdr reply; + struct reset_message { + struct mkhi_hdr hdr; + uint8_t req_origin; + uint8_t reset_type; + } __packed; + struct reset_message msg = { + .hdr = { + .group_id = MKHI_GROUP_ID_CBM, + .command = MKHI_GLOBAL_RESET_REQ, + }, + .req_origin = GR_ORIGIN_BIOS_POST, + .reset_type = rst_type + }; + size_t reply_size; + + if (!((rst_type == GLOBAL_RESET) || + (rst_type == HOST_RESET_ONLY) || (rst_type == CSE_RESET_ONLY))) + return -1; + + heci_reset(); + + reply_size = sizeof(reply); + memset(&reply, 0, reply_size); + + printk(BIOS_DEBUG, "HECI: Global Reset(Type:%d) Command\n", rst_type); + if (rst_type == CSE_RESET_ONLY) + status = heci_send_receive(&msg, sizeof(msg), NULL, 0); + else + status = heci_send_receive(&msg, sizeof(msg), &reply, + &reply_size); + + if (status != 1) + return -1; + + printk(BIOS_DEBUG, "HECI: Global Reset success!\n"); + return 0; +} + #if ENV_RAMSTAGE
static void update_sec_bar(struct device *dev) diff --git a/src/soc/intel/common/block/include/intelblocks/cse.h b/src/soc/intel/common/block/include/intelblocks/cse.h index 30d17c8..1b08b4d 100644 --- a/src/soc/intel/common/block/include/intelblocks/cse.h +++ b/src/soc/intel/common/block/include/intelblocks/cse.h @@ -106,7 +106,19 @@ */ uint8_t wait_cse_sec_override_mode(void);
+/* + * Sends GLOBAL_RESET_REQ cmd to CSE.The reset type can be + * GLOBAL_RESET/HOST_RESET_ONLY/CSE_RESET_ONLY. + * Returns -1 on failure a 0 on success. + */ +int send_heci_reset_req_message(uint8_t rst_type); + #define BIOS_HOST_ADDR 0x00 #define HECI_MKHI_ADDR 0x07
+/* Command GLOBAL_RESET_REQ Reset Types */ +#define GLOBAL_RESET 1 +#define HOST_RESET_ONLY 2 +#define CSE_RESET_ONLY 3 + #endif // SOC_INTEL_COMMON_MSR_H diff --git a/src/soc/intel/icelake/reset.c b/src/soc/intel/icelake/reset.c index 470b5f4..d83b3ee 100644 --- a/src/soc/intel/icelake/reset.c +++ b/src/soc/intel/icelake/reset.c @@ -22,65 +22,10 @@ #include <string.h> #include <soc/pci_devs.h>
-/* Reset Request */ -#define MKHI_GLOBAL_RESET 0x0b -#define MKHI_STATUS_SUCCESS 0 - -#define GR_ORIGIN_BIOS_MEM_INIT 0x01 -#define GR_ORIGIN_BIOS_POST 0x02 -#define GR_ORIGIN_MEBX 0x03 - -#define GLOBAL_RST_TYPE 0x01 - -#define BIOS_HOST_ADD 0x00 -#define HECI_MKHI_ADD 0x07 - -static int send_heci_reset_message(void) -{ - int status; - struct reset_reply { - u8 group_id; - u8 command; - u8 reserved; - u8 result; - } __packed reply; - struct reset_message { - u8 group_id; - u8 cmd; - u8 reserved; - u8 result; - u8 req_origin; - u8 reset_type; - } __packed; - struct reset_message msg = { - .cmd = MKHI_GLOBAL_RESET, - .req_origin = GR_ORIGIN_BIOS_POST, - .reset_type = GLOBAL_RST_TYPE - }; - size_t reply_size; - - heci_reset(); - - status = heci_send(&msg, sizeof(msg), BIOS_HOST_ADD, HECI_MKHI_ADD); - if (status != 1) - return -1; - - reply_size = sizeof(reply); - memset(&reply, 0, reply_size); - if (!heci_receive(&reply, &reply_size)) - return -1; - if (reply.result != MKHI_STATUS_SUCCESS) { - printk(BIOS_DEBUG, "Returned Mkhi Status is not success!\n"); - return -1; - } - printk(BIOS_DEBUG, "Heci receive success!\n"); - return 0; -} - void do_global_reset(void) { /* Ask CSE to do the global reset */ - if (!send_heci_reset_message()) + if (!send_heci_reset_req_message(GLOBAL_RESET)) return;
/* global reset if CSE fail to reset */ diff --git a/src/soc/intel/skylake/include/soc/me.h b/src/soc/intel/skylake/include/soc/me.h index ef84f59..c1fdc81 100644 --- a/src/soc/intel/skylake/include/soc/me.h +++ b/src/soc/intel/skylake/include/soc/me.h @@ -173,20 +173,8 @@
#define MKHI_GEN_GROUP_ID 0xff
-/* Reset Request */ -#define MKHI_GLOBAL_RESET 0x0b - #define MKHI_GET_FW_VERSION 0x02
-#define GR_ORIGIN_BIOS_MEM_INIT 0x01 -#define GR_ORIGIN_BIOS_POST 0x02 -#define GR_ORIGIN_MEBX 0x03 - -#define GLOBAL_RST_TYPE 0x01 - -#define BIOS_HOST_ADD 0x00 -#define HECI_MKHI_ADD 0x07 - void intel_me_status(void); int send_global_reset(void);
diff --git a/src/soc/intel/skylake/me.c b/src/soc/intel/skylake/me.c index 5fc817f..dcde348 100644 --- a/src/soc/intel/skylake/me.c +++ b/src/soc/intel/skylake/me.c @@ -257,8 +257,8 @@ */ heci_reset();
- if (!heci_send(&fw_ver_msg, sizeof(fw_ver_msg), BIOS_HOST_ADD, - HECI_MKHI_ADD)) + if (!heci_send(&fw_ver_msg, sizeof(fw_ver_msg), BIOS_HOST_ADDR, + HECI_MKHI_ADDR)) goto failed;
if (!heci_receive(&resp, &resp_size)) @@ -437,50 +437,6 @@ } }
-static int send_heci_reset_message(void) -{ - int status; - struct reset_reply { - u8 group_id; - u8 command; - u8 reserved; - u8 result; - } __packed reply; - struct reset_message { - u8 group_id; - u8 cmd; - u8 reserved; - u8 result; - u8 req_origin; - u8 reset_type; - } __packed; - struct reset_message msg = { - .cmd = MKHI_GLOBAL_RESET, - .req_origin = GR_ORIGIN_BIOS_POST, - .reset_type = GLOBAL_RST_TYPE - }; - size_t reply_size; - - heci_reset(); - - status = heci_send(&msg, sizeof(msg), BIOS_HOST_ADD, HECI_MKHI_ADD); - if (!status) - return -1; - - reply_size = sizeof(reply); - memset(&reply, 0, reply_size); - status = heci_receive(&reply, &reply_size); - if (!status) - return -1; - /* get reply result from HECI MSG */ - if (reply.result) { - printk(BIOS_DEBUG, "%s: Exit with Failure\n", __func__); - return -1; - } - printk(BIOS_DEBUG, "%s: Exit with Success\n", __func__); - return 0; -} - int send_global_reset(void) { int status = -1; @@ -495,7 +451,7 @@ goto ret;
/* ME should be in Normal Mode for this command */ - status = send_heci_reset_message(); + status = send_heci_reset_req_message(GLOBAL_RESET); ret: return status; }
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 25:
(6 comments)
How much of this applies to small core? In all of this common code movement, I am afraid things are being generalized without looking at all platforms.
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 83: /* HECI Message Header */ : struct mkhi_hdr { : uint8_t group_id; : uint8_t command:7; : uint8_t is_resp:1; : uint8_t rsvd; : uint8_t result; : } __packed; If this is common across SoCs and has the same format, why not get rid of various definitions of this sprinkled across different SoCs?
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 577: The reset type can be GLOBAL_RESET/ : * HOST_RESET_ONLY/CSE_RESET_ONLY. Are these supported by ME on all platforms? I am looking at a spec which does not provide these different options at all
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 601: -1 All other functions return 0 on error and 1 on success. Why is this function different?
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 609: if (rst_type == CSE_RESET_ONLY) Why is the response type different for the reset types? Can you please point me to the spec#?
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 610: heci_send_receive(&msg, sizeof(msg), NULL, 0); Why not use heci_send()?
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 114: rst_type There should be a comment indicating that this rst_type is one of the definitions below. It might make sense to define a enum for these values. It will be easier to reference.
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 25:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 577: The reset type can be GLOBAL_RESET/ : * HOST_RESET_ONLY/CSE_RESET_ONLY.
Are these supported by ME on all platforms? I am looking at a spec which does not provide these diff […]
Yes, The spec does not mention about the different resets for this command. We are in the process of getting the documentation updated.
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 609: if (rst_type == CSE_RESET_ONLY)
Why is the response type different for the reset types? Can you please point me to the spec#?
for CSE_RESET_ONLY, there will not be a response.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 25:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 577: The reset type can be GLOBAL_RESET/ : * HOST_RESET_ONLY/CSE_RESET_ONLY.
Yes, The spec does not mention about the different resets for this command. […]
So, is it supported on all platforms? Or just some?
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 609: if (rst_type == CSE_RESET_ONLY)
for CSE_RESET_ONLY, there will not be a response.
So, I am a bit confused. In case of global reset, wouldn't a response mean that the global reset failed?
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 25:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 577: The reset type can be GLOBAL_RESET/ : * HOST_RESET_ONLY/CSE_RESET_ONLY.
So, is it supported on all platforms? Or just some?
Yes,to be more precise, it is supported from ME 6.0 onwards and also on TXE 3.0 and 4.0.
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 609: if (rst_type == CSE_RESET_ONLY)
So, I am a bit confused. […]
Aah, ok. There will be a response only when the GLOBAL_RESET request was not accepted, we do not have to wait for a response if a reset was initiated. In case of GLOBAL_RESET and HOST_ONLY_RESET, if the reset is initiated successfully the response doesn't matter anyway. However for CSE_ONLY_RESET in a successful case, the host side will be waiting for a response from CSE, which will never come (since CSE is being reset) and we will timeout eventually as a failure. The handling is different for CSE_ONLY_RESET to avoid the wait. I think we need to study the response in both cases, and also avoid the wait in case of successful CSE_ONLY_RESET. Need to to check if there is something in HFSTS registers which we can use to know that CSE is being reset and don't have to wait.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 25:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 577: The reset type can be GLOBAL_RESET/ : * HOST_RESET_ONLY/CSE_RESET_ONLY.
Yes,to be more precise, it is supported from ME 6.0 onwards and also on TXE 3.0 and 4.0.
How do those versions translate to the different platforms here?
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 609: if (rst_type == CSE_RESET_ONLY)
Aah, ok. […]
How does the host know that CSE_RESET request was accepted? How does it know when the reset was complete? I believe it is important for host to know that because future operations would depend on this?
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 25:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 83: /* HECI Message Header */ : struct mkhi_hdr { : uint8_t group_id; : uint8_t command:7; : uint8_t is_resp:1; : uint8_t rsvd; : uint8_t result; : } __packed;
If this is common across SoCs and has the same format, why not get rid of various definitions of thi […]
will be done as part of https://review.coreboot.org/c/coreboot/+/35545
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 577: The reset type can be GLOBAL_RESET/ : * HOST_RESET_ONLY/CSE_RESET_ONLY.
How do those versions translate to the different platforms here?
TXE is for small core and 4.0 is in APL/GLK ME xx series is for big core, e.g, SKL/KBL - 11.0, CNL - 12.0, ICL - 13.0, CML - 14.0, and so on.
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 609: if (rst_type == CSE_RESET_ONLY)
How does the host know that CSE_RESET request was accepted? How does it know when the reset was comp […]
correct, that is what I meant by "Need to to check if there is something in HFSTS registers which we can use to know that CSE is being reset and don't have to wait." Skip waiting part if we know that CSE is being reset.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 25:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 577: The reset type can be GLOBAL_RESET/ : * HOST_RESET_ONLY/CSE_RESET_ONLY.
TXE is for small core and 4.0 is in APL/GLK […]
So, all the three reset types are supported on ME 6.0+ and TXE 3.0+. Do I understand that correctly?
Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 25:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 601: -1
All other functions return 0 on error and 1 on success. […]
It is addressed in the patch: https://review.coreboot.org/c/coreboot/+/37584/1
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 610: heci_send_receive(&msg, sizeof(msg), NULL, 0);
Why not use heci_send()?
Yes, we can use heci_send(). Code change adddressed in the patch: https://review.coreboot.org/c/coreboot/+/37584/1
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 114: rst_type
There should be a comment indicating that this rst_type is one of the definitions below. […]
Addressed in https://review.coreboot.org/c/coreboot/+/37584/1
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35228 )
Change subject: src/soc/intel/{common,cnl,skl,icl}: Move global reset req function to common ......................................................................
Patch Set 25:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 577: The reset type can be GLOBAL_RESET/ : * HOST_RESET_ONLY/CSE_RESET_ONLY.
So, all the three reset types are supported on ME 6.0+ and TXE 3.0+. […]
Yes
https://review.coreboot.org/c/coreboot/+/35228/25/src/soc/intel/common/block... PS25, Line 609: if (rst_type == CSE_RESET_ONLY)
correct, that is what I meant by "Need to to check if there is something in HFSTS registers which we […]
there is Host Firmware Status Register #1 (HFSTS1)—Bits [3:0] Current Working State Values, which gives us the current state of the CSE. 0 Reset: The Intel® ME firmware is in a reset state. The firmware will exit this state within 1 millisecond. 1 Initializing: The Intel® ME firmware is initializing, the final functional state is NOT known at this time. The firmware will exit this state within 4 seconds.
The above values can be used to determine whether the CSE is resetting.
So here is what we can do to not miss out on the response for CSE_ONLY reset and also reduce the wait time for it: 1. Send the Global reset request with reset type as CSE_ONLY_RESET 2. Wait for 4 seconds or less for the CSE to enter the Initializing state 3. If the CSE enters this state, then consider that the CSE was reset and continue with teh rest of the flow without waiting for response 4. Else start receiving/waiting-for the response
-
Aamir Bohra (Code Review) -
Furquan Shaikh (Code Review)
-
Patrick Georgi (Code Review)
-
Rizwan Qureshi (Code Review)
-
Sridhar Siricilla (Code Review)
-
Subrata Banik (Code Review)
-
V Sowmya (Code Review)