Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34616 )
Change subject: soc/intel/cnl: Only print ME status one time ......................................................................
soc/intel/cnl: Only print ME status one time
There were two hooks in the boot state machine which dumped the ME status to the debug UART, which is unnecessary. Removed the hook for the BS_OS_RESUME_CHECK state, leaving just BS_PAYLOAD_LOAD, which is called before FspNotifyEndOfFirmware, as required.
BUG=b:138463532 BRANCH=none TEST=Boot up, check cbmem to ensure the ME status messages are only printed one time.
Change-Id: I86bc6e33de4096f33023730ffabb25715c985de0 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/soc/intel/cannonlake/me.c 1 file changed, 0 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/34616/1
diff --git a/src/soc/intel/cannonlake/me.c b/src/soc/intel/cannonlake/me.c index c9748b7..79023d3 100644 --- a/src/soc/intel/cannonlake/me.c +++ b/src/soc/intel/cannonlake/me.c @@ -299,4 +299,3 @@ }
BOOT_STATE_INIT_ENTRY(BS_PAYLOAD_LOAD, BS_ON_ENTRY, dump_me_status, NULL); -BOOT_STATE_INIT_ENTRY(BS_OS_RESUME_CHECK, BS_ON_EXIT, dump_me_status, NULL);
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34616 )
Change subject: soc/intel/cnl: Only print ME status one time ......................................................................
Patch Set 1: Code-Review+1
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34616 )
Change subject: soc/intel/cnl: Only print ME status one time ......................................................................
Patch Set 1: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34616 )
Change subject: soc/intel/cnl: Only print ME status one time ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34616/1/src/soc/intel/cannonlake/me... File src/soc/intel/cannonlake/me.c:
https://review.coreboot.org/c/coreboot/+/34616/1/src/soc/intel/cannonlake/me... PS1, Line 302: I believe this was there to ensure that ME status gets printed on S3 resume. Did you check boot and S3 resume paths to ensure that the status gets printed?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34616 )
Change subject: soc/intel/cnl: Only print ME status one time ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34616/1/src/soc/intel/cannonlake/me... File src/soc/intel/cannonlake/me.c:
https://review.coreboot.org/c/coreboot/+/34616/1/src/soc/intel/cannonlake/me... PS1, Line 302:
I believe this was there to ensure that ME status gets printed on S3 resume. […]
It gets printed at boot-from-S5, but not from S3 resume in this case. Would we expect the data to change in between? Also, my understanding was that once HECI is disabled, this information is unreliable (?), and that happens when coreboot sends the EndOfFirmware notification, which is at payload load time in the S5 boot path.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34616 )
Change subject: soc/intel/cnl: Only print ME status one time ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34616/1/src/soc/intel/cannonlake/me... File src/soc/intel/cannonlake/me.c:
https://review.coreboot.org/c/coreboot/+/34616/1/src/soc/intel/cannonlake/me... PS1, Line 302:
It gets printed at boot-from-S5, but not from S3 resume in this case. Would we expect the data to change in between?
I believe it is more to ensure that the state can be captured coming out of boot as well as S3 which is generally helpful for debugging in case of failures.
Also, my understanding was that once HECI is disabled, this information is unreliable (?), and that happens when coreboot sends the EndOfFirmware notification, which is at payload load time in the S5 boot path.
EndOfFirmware notification is sent on resume as well:
BOOT_STATE_INIT_ENTRY(BS_OS_RESUME, BS_ON_ENTRY, fsp_notify_dummy, » » » » » » (void *) READY_TO_BOOT);
IIUC, HECI interface needs to be disabled again after resume from S3. +Subrata.
Hello Patrick Rudolph, Paul Fagerburg, Subrata Banik, Shelley Chen, Bora Guvendik, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34616
to look at the new patch set (#2).
Change subject: soc/intel/cnl: Only print ME status one time ......................................................................
soc/intel/cnl: Only print ME status one time
There were two hooks in the boot state machine which dumped the ME status to the debug UART, which is unnecessary. Removed the hook for the BS_OS_RESUME_CHECK state, leaving just BS_PAYLOAD_LOAD, which is called before FspNotifyEndOfFirmware, as required.
BUG=b:138463532 BRANCH=none TEST=Boot up, check cbmem to ensure the ME status messages are only printed one time.
Change-Id: I86bc6e33de4096f33023730ffabb25715c985de0 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/soc/intel/cannonlake/me.c 1 file changed, 0 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/34616/2
Hello Patrick Rudolph, Paul Fagerburg, Subrata Banik, Shelley Chen, Bora Guvendik, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34616
to look at the new patch set (#3).
Change subject: soc/intel/cnl: Only print ME status one time ......................................................................
soc/intel/cnl: Only print ME status one time
There were two hooks in the boot state machine which dumped the ME status to the debug UART, which is unnecessary. Removed the hook for the BS_OS_RESUME_CHECK state, leaving just BS_PAYLOAD_LOAD, which is called before FspNotifyEndOfFirmware, as required.
BUG=b:138463532 BRANCH=none TEST=Boot up, check cbmem to ensure the ME status messages are only printed one time. Resume to S3, ensure that ME status message are printed at resume-time as well.
Change-Id: I86bc6e33de4096f33023730ffabb25715c985de0 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/soc/intel/cannonlake/me.c 1 file changed, 0 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/34616/3
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34616 )
Change subject: soc/intel/cnl: Only print ME status one time ......................................................................
Patch Set 3: Code-Review+2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34616 )
Change subject: soc/intel/cnl: Only print ME status one time ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34616/1/src/soc/intel/cannonlake/me... File src/soc/intel/cannonlake/me.c:
https://review.coreboot.org/c/coreboot/+/34616/1/src/soc/intel/cannonlake/me... PS1, Line 302:
It gets printed at boot-from-S5, but not from S3 resume in this case. […]
Ok, changed to BS_OS_RESUME_CHECK, ON_EXIT. Doesn't the code in southbridge_smi_handler() effectively disable the HECI? So anytime the SMI handler runs from S3? I'll double check this.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34616 )
Change subject: soc/intel/cnl: Only print ME status one time ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34616/1/src/soc/intel/cannonlake/me... File src/soc/intel/cannonlake/me.c:
https://review.coreboot.org/c/coreboot/+/34616/1/src/soc/intel/cannonlake/me... PS1, Line 302:
Ok, changed to BS_OS_RESUME_CHECK, ON_EXIT. […]
It doesn't look like that happens. But I remember, from cold boot, the FSP disables HECI. So if it gets re-enabled sometime in S3 suspend/resume, it's probably still running and would need to be disabled again. Separate issue?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34616 )
Change subject: soc/intel/cnl: Only print ME status one time ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34616/1/src/soc/intel/cannonlake/me... File src/soc/intel/cannonlake/me.c:
https://review.coreboot.org/c/coreboot/+/34616/1/src/soc/intel/cannonlake/me... PS1, Line 302:
Ok, changed to BS_OS_RESUME_CHECK, ON_EXIT. […]
Are you referring to smihandler_soc_at_finalize()?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34616 )
Change subject: soc/intel/cnl: Only print ME status one time ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34616/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34616/3//COMMIT_MSG@10 PS3, Line 10: Removed the hook : for the BS_OS_RESUME_CHECK state, leaving just BS_PAYLOAD_LOAD, Isn't this flipped now?
Hello Patrick Rudolph, Paul Fagerburg, Subrata Banik, Shelley Chen, Bora Guvendik, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34616
to look at the new patch set (#4).
Change subject: soc/intel/cnl: Only print ME status one time ......................................................................
soc/intel/cnl: Only print ME status one time
There were two hooks in the boot state machine which dumped the ME status to the debug UART, which is unnecessary. Removed the hook for the BS_PAYLOAD_LOAD state, leaving just BS_OS_RESUME_CHECK, which is called before FspNotifyEndOfFirmware, as required.
BUG=b:138463532 BRANCH=none TEST=Boot up, check cbmem to ensure the ME status messages are only printed one time. Resume to S3, ensure that ME status message are printed at resume-time as well.
Change-Id: I86bc6e33de4096f33023730ffabb25715c985de0 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/soc/intel/cannonlake/me.c 1 file changed, 0 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/34616/4
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34616 )
Change subject: soc/intel/cnl: Only print ME status one time ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34616/1/src/soc/intel/cannonlake/me... File src/soc/intel/cannonlake/me.c:
https://review.coreboot.org/c/coreboot/+/34616/1/src/soc/intel/cannonlake/me... PS1, Line 302:
Are you referring to smihandler_soc_at_finalize()?
Yes, but it doesn't look like that gets called from S3 SMI handler.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34616 )
Change subject: soc/intel/cnl: Only print ME status one time ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34616/1/src/soc/intel/cannonlake/me... File src/soc/intel/cannonlake/me.c:
https://review.coreboot.org/c/coreboot/+/34616/1/src/soc/intel/cannonlake/me... PS1, Line 302:
Yes, but it doesn't look like that gets called from S3 SMI handler.
That is enabled only for CNL. WHL and CML do not use that. Controlled by HECI_DISABLE_USING_SMM
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34616 )
Change subject: soc/intel/cnl: Only print ME status one time ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34616/1/src/soc/intel/cannonlake/me... File src/soc/intel/cannonlake/me.c:
https://review.coreboot.org/c/coreboot/+/34616/1/src/soc/intel/cannonlake/me... PS1, Line 302:
That is enabled only for CNL. WHL and CML do not use that. […]
Ah ok. Thanks Furquan!
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34616 )
Change subject: soc/intel/cnl: Only print ME status one time ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34616/1/src/soc/intel/cannonlake/me... File src/soc/intel/cannonlake/me.c:
https://review.coreboot.org/c/coreboot/+/34616/1/src/soc/intel/cannonlake/me... PS1, Line 302:
Ah ok. […]
Ack
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34616 )
Change subject: soc/intel/cnl: Only print ME status one time ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34616/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34616/3//COMMIT_MSG@10 PS3, Line 10: Removed the hook : for the BS_OS_RESUME_CHECK state, leaving just BS_PAYLOAD_LOAD,
Isn't this flipped now?
Done
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34616 )
Change subject: soc/intel/cnl: Only print ME status one time ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34616/1/src/soc/intel/cannonlake/me... File src/soc/intel/cannonlake/me.c:
https://review.coreboot.org/c/coreboot/+/34616/1/src/soc/intel/cannonlake/me... PS1, Line 302:
Ack
HECI interface needs to be disabled again after resume from S3. +Subrata.
You are right furquan and we also would like to see ME status dump while resuming from S3 like kind of sanity
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34616 )
Change subject: soc/intel/cnl: Only print ME status one time ......................................................................
Patch Set 4: Code-Review+2
Hello Patrick Rudolph, Paul Fagerburg, Subrata Banik, Shelley Chen, Bora Guvendik, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34616
to look at the new patch set (#5).
Change subject: soc/intel/cnl: Only print ME status one time ......................................................................
soc/intel/cnl: Only print ME status one time
There were two hooks in the boot state machine which dumped the ME status to the debug UART, which is unnecessary. Removed the hook for the BS_OS_RESUME_CHECK state, leaving just BS_PAYLOAD_LOAD, which is called before FspNotifyEndOfFirmware, as required.
BUG=b:138463532 BRANCH=none TEST=Boot up, check cbmem to ensure the ME status messages are only printed one time.
Change-Id: I86bc6e33de4096f33023730ffabb25715c985de0 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/soc/intel/cannonlake/me.c 1 file changed, 0 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/34616/5
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34616 )
Change subject: soc/intel/cnl: Only print ME status one time ......................................................................
Patch Set 5: Code-Review+2
Furquan Shaikh has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34616 )
Change subject: soc/intel/cnl: Only print ME status one time ......................................................................
soc/intel/cnl: Only print ME status one time
There were two hooks in the boot state machine which dumped the ME status to the debug UART, which is unnecessary. Removed the hook for the BS_OS_RESUME_CHECK state, leaving just BS_PAYLOAD_LOAD, which is called before FspNotifyEndOfFirmware, as required.
BUG=b:138463532 BRANCH=none TEST=Boot up, check cbmem to ensure the ME status messages are only printed one time.
Change-Id: I86bc6e33de4096f33023730ffabb25715c985de0 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/34616 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Paul Fagerburg pfagerburg@chromium.org --- M src/soc/intel/cannonlake/me.c 1 file changed, 0 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Paul Fagerburg: Looks good to me, approved
diff --git a/src/soc/intel/cannonlake/me.c b/src/soc/intel/cannonlake/me.c index c9748b7..b8b4245d 100644 --- a/src/soc/intel/cannonlake/me.c +++ b/src/soc/intel/cannonlake/me.c @@ -298,5 +298,4 @@ hfsts6.fields.txt_support ? "YES" : "NO"); }
-BOOT_STATE_INIT_ENTRY(BS_PAYLOAD_LOAD, BS_ON_ENTRY, dump_me_status, NULL); BOOT_STATE_INIT_ENTRY(BS_OS_RESUME_CHECK, BS_ON_EXIT, dump_me_status, NULL);
-
Furquan Shaikh (Code Review) -
Paul Fagerburg (Code Review)
-
Shelley Chen (Code Review)
-
Subrata Banik (Code Review)
-
Tim Wawrzynczak (Code Review)