Change in coreboot[master]: soc/amd/picasso/smu: only print time for actual command execution
Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41751 )
Change subject: soc/amd/picasso/smu: only print time for actual command execution ......................................................................
soc/amd/picasso/smu: only print time for actual command execution
When waiting for the SMU to be ready to accept a new command, the time spent waiting shouldn't be printed as command execution time.
Change-Id: I6b97b11cd9efae7029779ee2096d4f2224cecd72 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/picasso/smu.c 1 file changed, 5 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/41751/1
diff --git a/src/soc/amd/picasso/smu.c b/src/soc/amd/picasso/smu.c index cfe2240..7dd62ae 100644 --- a/src/soc/amd/picasso/smu.c +++ b/src/soc/amd/picasso/smu.c @@ -23,7 +23,7 @@ #define SMU_MESG_RESP_OK 0x01
/* returns SMU_MESG_RESP_OK, SMU_MESG_RESP_TIMEOUT or a negative number */ -static int32_t smu_poll_response(void) +static int32_t smu_poll_response(bool print_command_duration) { struct stopwatch sw; const long timeout_ms = 10 * MSECS_PER_SEC; @@ -34,7 +34,8 @@ do { result = smu_read32(REG_ADDR_MESG_RESP); if (result) { - printk(BIOS_SPEW, "SMU command consumed %ld msecs\n", + if (print_command_duration) + printk(BIOS_SPEW, "SMU command consumed %ld msecs\n", stopwatch_duration_usecs(&sw)); return result; } @@ -53,7 +54,7 @@ size_t i;
/* wait until SMU can process a new request; don't care if an old request failed */ - if (smu_poll_response() == SMU_MESG_RESP_TIMEOUT) + if (smu_poll_response(false) == SMU_MESG_RESP_TIMEOUT) return CB_ERR;
/* clear response register */ @@ -67,7 +68,7 @@ smu_write32(REG_ADDR_MESG_ID, id);
/* wait until SMU has processed the message and check if it was successful */ - if (smu_poll_response() != SMU_MESG_RESP_OK) + if (smu_poll_response(true) != SMU_MESG_RESP_OK) return CB_ERR;
/* copy returned values */
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41751 )
Change subject: soc/amd/picasso/smu: only print time for actual command execution ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41751/1/src/soc/amd/picasso/smu.c File src/soc/amd/picasso/smu.c:
https://review.coreboot.org/c/coreboot/+/41751/1/src/soc/amd/picasso/smu.c@3... PS1, Line 38: msecs This should be usec right?
Hello build bot (Jenkins), Raul Rangel,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41751
to look at the new patch set (#2).
Change subject: soc/amd/picasso/smu: only print time for actual command execution ......................................................................
soc/amd/picasso/smu: only print time for actual command execution
When waiting for the SMU to be ready to accept a new command, the time spent waiting shouldn't be printed as command execution time. Also fix the time unit in the print statement.
Change-Id: I6b97b11cd9efae7029779ee2096d4f2224cecd72 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/picasso/smu.c 1 file changed, 5 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/41751/2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41751 )
Change subject: soc/amd/picasso/smu: only print time for actual command execution ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41751/1/src/soc/amd/picasso/smu.c File src/soc/amd/picasso/smu.c:
https://review.coreboot.org/c/coreboot/+/41751/1/src/soc/amd/picasso/smu.c@3... PS1, Line 38: msecs
This should be usec right?
from the function name stopwatch_duration_usecs i'd assume that this should be usec. fixed.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41751 )
Change subject: soc/amd/picasso/smu: only print time for actual command execution ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41751/1/src/soc/amd/picasso/smu.c File src/soc/amd/picasso/smu.c:
https://review.coreboot.org/c/coreboot/+/41751/1/src/soc/amd/picasso/smu.c@3... PS1, Line 38: msecs
from the function name stopwatch_duration_usecs i'd assume that this should be usec. fixed.
Done
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41751 )
Change subject: soc/amd/picasso/smu: only print time for actual command execution ......................................................................
Patch Set 2: Code-Review+2
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41751 )
Change subject: soc/amd/picasso/smu: only print time for actual command execution ......................................................................
soc/amd/picasso/smu: only print time for actual command execution
When waiting for the SMU to be ready to accept a new command, the time spent waiting shouldn't be printed as command execution time. Also fix the time unit in the print statement.
Change-Id: I6b97b11cd9efae7029779ee2096d4f2224cecd72 Signed-off-by: Felix Held felix-coreboot@felixheld.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/41751 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Raul Rangel rrangel@chromium.org --- M src/soc/amd/picasso/smu.c 1 file changed, 5 insertions(+), 4 deletions(-)
Approvals: build bot (Jenkins): Verified Raul Rangel: Looks good to me, approved
diff --git a/src/soc/amd/picasso/smu.c b/src/soc/amd/picasso/smu.c index cfe2240..4a373ce 100644 --- a/src/soc/amd/picasso/smu.c +++ b/src/soc/amd/picasso/smu.c @@ -23,7 +23,7 @@ #define SMU_MESG_RESP_OK 0x01
/* returns SMU_MESG_RESP_OK, SMU_MESG_RESP_TIMEOUT or a negative number */ -static int32_t smu_poll_response(void) +static int32_t smu_poll_response(bool print_command_duration) { struct stopwatch sw; const long timeout_ms = 10 * MSECS_PER_SEC; @@ -34,7 +34,8 @@ do { result = smu_read32(REG_ADDR_MESG_RESP); if (result) { - printk(BIOS_SPEW, "SMU command consumed %ld msecs\n", + if (print_command_duration) + printk(BIOS_SPEW, "SMU command consumed %ld usecs\n", stopwatch_duration_usecs(&sw)); return result; } @@ -53,7 +54,7 @@ size_t i;
/* wait until SMU can process a new request; don't care if an old request failed */ - if (smu_poll_response() == SMU_MESG_RESP_TIMEOUT) + if (smu_poll_response(false) == SMU_MESG_RESP_TIMEOUT) return CB_ERR;
/* clear response register */ @@ -67,7 +68,7 @@ smu_write32(REG_ADDR_MESG_ID, id);
/* wait until SMU has processed the message and check if it was successful */ - if (smu_poll_response() != SMU_MESG_RESP_OK) + if (smu_poll_response(true) != SMU_MESG_RESP_OK) return CB_ERR;
/* copy returned values */
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41751 )
Change subject: soc/amd/picasso/smu: only print time for actual command execution ......................................................................
Patch Set 3:
Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/4258 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4257 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4256 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/4255
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Felix Held (Code Review)
-
Raul Rangel (Code Review)