Hello Marshall Dawson,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/40017
to review the following change.
Change subject: soc/amd/common/psp: make BootDone command conditional ......................................................................
soc/amd/common/psp: make BootDone command conditional
The common PSP code automatically sends the command, however for fam17h FSP also sends it in response to a Notify. The 2nd instance of the command generates a timeout error.
Change-Id: I819ab0b80d2a3cf6cd24f3e7170cae779dcc7cef Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Reviewed-on: https://chromium-review.googlesource.com/2020369 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/common/block/psp/psp.c 1 file changed, 7 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/40017/1
diff --git a/src/soc/amd/common/block/psp/psp.c b/src/soc/amd/common/block/psp/psp.c index b48ff30..3aa3b86 100644 --- a/src/soc/amd/common/block/psp/psp.c +++ b/src/soc/amd/common/block/psp/psp.c @@ -180,10 +180,12 @@ return cmd_status; }
+#if CONFIG(SOC_AMD_STONEYRIDGE) /* * Notify the PSP that the system is completing the boot process. Upon * receiving this command, the PSP will only honor commands where the buffer - * is in SMM space. + * is in SMM space. In FSP-based families, this is done by the FSP code and + * calling it a second time results in a hang. */ static void psp_notify_boot_done(void *unused) { @@ -201,6 +203,7 @@ /* buffer's status shouldn't change but report it if it does */ print_cmd_status(cmd_status, &buffer); } +#endif
/* Notify PSP the system is going to a sleep state. */ void psp_notify_sx_info(u8 sleep_type) @@ -276,5 +279,8 @@ return cmd_status; }
+#if CONFIG(SOC_AMD_STONEYRIDGE) +/* only run psp_notify_boot_done on non-FSP families */ BOOT_STATE_INIT_ENTRY(BS_PAYLOAD_BOOT, BS_ON_ENTRY, psp_notify_boot_done, NULL); +#endif
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40017 )
Change subject: soc/amd/common/psp: make BootDone command conditional ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/40017/2/src/soc/amd/common/block/ps... File src/soc/amd/common/block/psp/psp.c:
https://review.coreboot.org/c/coreboot/+/40017/2/src/soc/amd/common/block/ps... PS2, Line 188: a hang does it hang the system unrecoverably, or does it just time out?
Hello build bot (Jenkins), Raul Rangel, Marshall Dawson, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40017
to look at the new patch set (#3).
Change subject: soc/amd/common/psp: make BootDone command conditional ......................................................................
soc/amd/common/psp: make BootDone command conditional
The common PSP code automatically sends the command, however for fam17h FSP also sends it in response to a Notify. The 2nd instance of the command generates a timeout error.
Change-Id: I819ab0b80d2a3cf6cd24f3e7170cae779dcc7cef Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Reviewed-on: https://chromium-review.googlesource.com/2020369 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/common/block/psp/psp.c 1 file changed, 7 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/40017/3
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40017 )
Change subject: soc/amd/common/psp: make BootDone command conditional ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40017/2/src/soc/amd/common/block/ps... File src/soc/amd/common/block/psp/psp.c:
https://review.coreboot.org/c/coreboot/+/40017/2/src/soc/amd/common/block/ps... PS2, Line 188: a hang
does it hang the system unrecoverably, or does it just time out?
fixed
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40017 )
Change subject: soc/amd/common/psp: make BootDone command conditional ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40017/3/src/soc/amd/common/block/ps... File src/soc/amd/common/block/psp/psp.c:
https://review.coreboot.org/c/coreboot/+/40017/3/src/soc/amd/common/block/ps... PS3, Line 284: psp_notify_boot_done Can we move this from the common code into the stoney psp.c?
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40017 )
Change subject: soc/amd/common/psp: make BootDone command conditional ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40017/3/src/soc/amd/common/block/ps... File src/soc/amd/common/block/psp/psp.c:
https://review.coreboot.org/c/coreboot/+/40017/3/src/soc/amd/common/block/ps... PS3, Line 284: psp_notify_boot_done
Can we move this from the common code into the stoney psp. […]
good point; will do
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40017 )
Change subject: soc/amd/common/psp: make BootDone command conditional ......................................................................
Patch Set 3: Code-Review-1
Felix, I guess we need more time to chat :D Actually, I prefer coreboot and the common//psp code always own the communication with the PSP. That means we need to remove it from the FSP instead (or configurable). In the meantime, calling both ought not harm anything except the 2nd one will be ignored. I think you can abandon this patch.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40017 )
Change subject: soc/amd/common/psp: make BootDone command conditional ......................................................................
Patch Set 3:
Patch Set 3: Code-Review-1
Felix, I guess we need more time to chat :D Actually, I prefer coreboot and the common//psp code always own the communication with the PSP. That means we need to remove it from the FSP instead (or configurable). In the meantime, calling both ought not harm anything except the 2nd one will be ignored. I think you can abandon this patch.
ok. removing this from the FSP code and leaving it here would also be my preferred option
Felix Held has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/40017 )
Change subject: soc/amd/common/psp: make BootDone command conditional ......................................................................
Abandoned
problem will likely be fixed in FSP blob
-
Angel Pons (Code Review) -
Felix Held (Code Review)
-
Marshall Dawson (Code Review)
-
Raul Rangel (Code Review)