Change in coreboot[master]: soc/amd/common/psp: Make common function to print status

[Felix Held (Code Review)]

Hello Raul Rangel, Marshall Dawson, Marshall Dawson, Eric Peers,

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

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

to review the following change.

Change subject: soc/amd/common/psp: Make common function to print status
......................................................................

soc/amd/common/psp: Make common function to print status

Consolidate commands' printing of status into one static function.

TEST: Verify PSP functionality on google/grunt

Change-Id: Id8abe0d1d4ac87f6d4f625593f47bf484729906f
Signed-off-by: Marshall Dawson 
Reviewed-on: https://chromium-review.googlesource.com/2020363
Reviewed-by: Raul E Rangel 
Reviewed-by: Eric Peers 
Tested-by: Eric Peers 
---
M src/soc/amd/common/block/psp/psp.c
1 file changed, 18 insertions(+), 18 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/39998/1

diff --git a/src/soc/amd/common/block/psp/psp.c b/src/soc/amd/common/block/psp/psp.c
index cf25fd1..0294422 100644
--- a/src/soc/amd/common/block/psp/psp.c
+++ b/src/soc/amd/common/block/psp/psp.c
@@ -226,6 +226,21 @@
 }

/*
+ * Print meaningful status to the console.  Caller only passes a pointer to a
+ * buffer if it's expected to contain its own status.
+ */
+static void print_cmd_status(int cmd_status, struct mbox_default_buffer *buffer)
+{
+	if (buffer && rd_resp_sts(buffer))
+		printk(BIOS_DEBUG, "buffer status=0x%x ", rd_resp_sts(buffer));
+
+	if (cmd_status)
+		printk(BIOS_DEBUG, "%s\n", status_to_string(cmd_status));
+	else
+		printk(BIOS_DEBUG, "OK\n");
+}
+
+/*
  * Notify the PSP that DRAM is present.  Upon receiving this command, the PSP
  * will load its OS into fenced DRAM that is not accessible to the x86 cores.
  */
@@ -243,13 +258,7 @@
    cmd_status = send_psp_command(MBOX_BIOS_CMD_DRAM_INFO, &buffer);

/* buffer's status shouldn't change but report it if it does */
-	if (rd_resp_sts(&buffer))
-		printk(BIOS_DEBUG, "buffer status=0x%x ",
-				rd_resp_sts(&buffer));
-	if (cmd_status)
-		printk(BIOS_DEBUG, "%s\n", status_to_string(cmd_status));
-	else
-		printk(BIOS_DEBUG, "OK\n");
+	print_cmd_status(cmd_status, &buffer);

return cmd_status;
 }
@@ -273,13 +282,7 @@
    cmd_status = send_psp_command(MBOX_BIOS_CMD_BOOT_DONE, &buffer);

/* buffer's status shouldn't change but report it if it does */
-	if (rd_resp_sts(&buffer))
-		printk(BIOS_DEBUG, "buffer status=0x%x ",
-				rd_resp_sts(&buffer));
-	if (cmd_status)
-		printk(BIOS_DEBUG, "%s\n", status_to_string(cmd_status));
-	else
-		printk(BIOS_DEBUG, "OK\n");
+	print_cmd_status(cmd_status, &buffer);
 }

/*
@@ -305,10 +308,7 @@
    /* Blob commands use the buffer registers as data, not pointer to buf */
    cmd_status = send_psp_command(type, addr);

-	if (cmd_status)
-		printk(BIOS_DEBUG, "%s\n", status_to_string(cmd_status));
-	else
-		printk(BIOS_DEBUG, "OK\n");
+	print_cmd_status(cmd_status, NULL);

return cmd_status;
 }

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39998 )

Change subject: soc/amd/common/psp: Make common function to print status
......................................................................

Patch Set 1: Code-Review+1

[Paul Menzel (Code Review)]

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

Change subject: soc/amd/common/psp: Make common function to print status
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/39998/1/src/soc/amd/common/block/ps... 
File src/soc/amd/common/block/psp/psp.c:

https://review.coreboot.org/c/coreboot/+/39998/1/src/soc/amd/common/block/ps... 
PS1, Line 240: 		printk(BIOS_DEBUG, "OK\n");
Use ternary operator?

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39998 )

Change subject: soc/amd/common/psp: Make common function to print status
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39998/1/src/soc/amd/common/block/ps... 
File src/soc/amd/common/block/psp/psp.c:

https://review.coreboot.org/c/coreboot/+/39998/1/src/soc/amd/common/block/ps... 
PS1, Line 240: 		printk(BIOS_DEBUG, "OK\n");
> Use ternary operator?

this would increase the code density a bit, but i don't think that this would improve the readability, since those two cases have different code paths (constant vs. function call).

[Paul Menzel (Code Review)]

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

Change subject: soc/amd/common/psp: Make common function to print status
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39998/1/src/soc/amd/common/block/ps... 
File src/soc/amd/common/block/psp/psp.c:

https://review.coreboot.org/c/coreboot/+/39998/1/src/soc/amd/common/block/ps... 
PS1, Line 240: 		printk(BIOS_DEBUG, "OK\n");
> this would increase the code density a bit, but i don't think that this would improve the readabilit […]

Ack

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39998 )

Change subject: soc/amd/common/psp: Make common function to print status
......................................................................

Patch Set 1: Code-Review+2

[Raul Rangel (Code Review)]

Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39998 )

Change subject: soc/amd/common/psp: Make common function to print status
......................................................................

Patch Set 2: Code-Review+2

[Felix Held (Code Review)]

Hello build bot (Jenkins), Raul Rangel, Marshall Dawson, Marshall Dawson, Paul Menzel, Angel Pons, Eric Peers,

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

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

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

Change subject: soc/amd/common/psp: Make common function to print status
......................................................................

soc/amd/common/psp: Make common function to print status

Consolidate commands' printing of status into one static function.

BUG=b:130660285
TEST: Verify PSP functionality on google/grunt

Change-Id: Id8abe0d1d4ac87f6d4f625593f47bf484729906f
Signed-off-by: Marshall Dawson 
Reviewed-on: https://chromium-review.googlesource.com/2020363
Reviewed-by: Raul E Rangel 
Reviewed-by: Eric Peers 
Tested-by: Eric Peers 
---
M src/soc/amd/common/block/psp/psp.c
1 file changed, 18 insertions(+), 18 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/39998/3

[Felix Held (Code Review)]

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39998 )

Change subject: soc/amd/common/psp: Make common function to print status
......................................................................

soc/amd/common/psp: Make common function to print status

Consolidate commands' printing of status into one static function.

BUG=b:130660285
TEST: Verify PSP functionality on google/grunt

Change-Id: Id8abe0d1d4ac87f6d4f625593f47bf484729906f
Signed-off-by: Marshall Dawson 
Reviewed-on: https://chromium-review.googlesource.com/2020363
Reviewed-by: Raul E Rangel 
Reviewed-by: Eric Peers 
Tested-by: Eric Peers 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/39998
Reviewed-by: Raul Rangel 
Reviewed-by: Paul Menzel 
Reviewed-by: Angel Pons 
Tested-by: build bot (Jenkins) 
---
M src/soc/amd/common/block/psp/psp.c
1 file changed, 18 insertions(+), 18 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Paul Menzel: Looks good to me, but someone else must approve
  Marshall Dawson: Looks good to me, but someone else must approve
  Raul Rangel: Looks good to me, approved
  Angel Pons: Looks good to me, approved

diff --git a/src/soc/amd/common/block/psp/psp.c b/src/soc/amd/common/block/psp/psp.c
index cf25fd1..0294422 100644
--- a/src/soc/amd/common/block/psp/psp.c
+++ b/src/soc/amd/common/block/psp/psp.c
@@ -226,6 +226,21 @@
 }

/*
+ * Print meaningful status to the console.  Caller only passes a pointer to a
+ * buffer if it's expected to contain its own status.
+ */
+static void print_cmd_status(int cmd_status, struct mbox_default_buffer *buffer)
+{
+	if (buffer && rd_resp_sts(buffer))
+		printk(BIOS_DEBUG, "buffer status=0x%x ", rd_resp_sts(buffer));
+
+	if (cmd_status)
+		printk(BIOS_DEBUG, "%s\n", status_to_string(cmd_status));
+	else
+		printk(BIOS_DEBUG, "OK\n");
+}
+
+/*
  * Notify the PSP that DRAM is present.  Upon receiving this command, the PSP
  * will load its OS into fenced DRAM that is not accessible to the x86 cores.
  */
@@ -243,13 +258,7 @@
    cmd_status = send_psp_command(MBOX_BIOS_CMD_DRAM_INFO, &buffer);

/* buffer's status shouldn't change but report it if it does */
-	if (rd_resp_sts(&buffer))
-		printk(BIOS_DEBUG, "buffer status=0x%x ",
-				rd_resp_sts(&buffer));
-	if (cmd_status)
-		printk(BIOS_DEBUG, "%s\n", status_to_string(cmd_status));
-	else
-		printk(BIOS_DEBUG, "OK\n");
+	print_cmd_status(cmd_status, &buffer);

return cmd_status;
 }
@@ -273,13 +282,7 @@
    cmd_status = send_psp_command(MBOX_BIOS_CMD_BOOT_DONE, &buffer);

/* buffer's status shouldn't change but report it if it does */
-	if (rd_resp_sts(&buffer))
-		printk(BIOS_DEBUG, "buffer status=0x%x ",
-				rd_resp_sts(&buffer));
-	if (cmd_status)
-		printk(BIOS_DEBUG, "%s\n", status_to_string(cmd_status));
-	else
-		printk(BIOS_DEBUG, "OK\n");
+	print_cmd_status(cmd_status, &buffer);
 }

/*
@@ -305,10 +308,7 @@
    /* Blob commands use the buffer registers as data, not pointer to buf */
    cmd_status = send_psp_command(type, addr);

-	if (cmd_status)
-		printk(BIOS_DEBUG, "%s\n", status_to_string(cmd_status));
-	else
-		printk(BIOS_DEBUG, "OK\n");
+	print_cmd_status(cmd_status, NULL);

return cmd_status;
 }

[9elements QA (Code Review)]

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

Change subject: soc/amd/common/psp: Make common function to print status
......................................................................

Patch Set 4:

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

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