Hello Marshall Dawson, Marshall Dawson, Eric Peers,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/40015
to review the following change.
Change subject: soc/amd/common/psp: Move definitions into a private file ......................................................................
soc/amd/common/psp: Move definitions into a private file
Declutter psp.h by removing internal details the caller doesn't need to know.
TEST: Verify PSP functionality on google/grunt
Change-Id: I2fb0ed1d2697c313fb8475e3f00482899e729130 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Reviewed-on: https://chromium-review.googlesource.com/2020366 Tested-by: Eric Peers epeers@google.com Reviewed-by: Eric Peers epeers@google.com --- M src/soc/amd/common/block/include/amdblocks/psp.h M src/soc/amd/common/block/psp/psp.c A src/soc/amd/common/block/psp/psp_def.h 3 files changed, 75 insertions(+), 62 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/40015/1
diff --git a/src/soc/amd/common/block/include/amdblocks/psp.h b/src/soc/amd/common/block/include/amdblocks/psp.h index 42c802d..d7f6169 100644 --- a/src/soc/amd/common/block/include/amdblocks/psp.h +++ b/src/soc/amd/common/block/include/amdblocks/psp.h @@ -18,63 +18,7 @@ /* Get the mailbox base address - specific to family of device. */ struct psp_mbox *soc_get_mbox_address(void);
-/* x86 to PSP commands */ -#define MBOX_BIOS_CMD_DRAM_INFO 0x01 -#define MBOX_BIOS_CMD_SMM_INFO 0x02 -#define MBOX_BIOS_CMD_SX_INFO 0x03 -#define MBOX_BIOS_CMD_RSM_INFO 0x04 -#define MBOX_BIOS_CMD_PSP_QUERY 0x05 -#define MBOX_BIOS_CMD_BOOT_DONE 0x06 -#define MBOX_BIOS_CMD_CLEAR_S3_STS 0x07 -#define MBOX_BIOS_CMD_S3_DATA_INFO 0x08 -#define MBOX_BIOS_CMD_NOP 0x09 -#define MBOX_BIOS_CMD_SMU_FW 0x19 -#define MBOX_BIOS_CMD_SMU_FW2 0x1a -#define MBOX_BIOS_CMD_ABORT 0xfe - -/* generic PSP interface status */ -#define STATUS_INITIALIZED 0x1 -#define STATUS_ERROR 0x2 -#define STATUS_TERMINATED 0x4 -#define STATUS_HALT 0x8 -#define STATUS_RECOVERY 0x10 - -/* psp_mbox consists of hardware registers beginning at PSPx000070 - * mbox_command: BIOS->PSP command, cleared by PSP when complete - * mbox_status: BIOS->PSP interface status - * cmd_response: pointer to command/response buffer - */ -struct psp_mbox { - u32 mbox_command; - u32 mbox_status; - u64 cmd_response; /* definition conflicts w/BKDG but matches agesa */ -} __packed; - -/* command/response format, BIOS builds this in memory - * mbox_buffer_header: generic header - * mbox_buffer: command-specific buffer format - * - * AMD reference code aligns and pads all buffers to 32 bytes. - */ -struct mbox_buffer_header { - u32 size; /* total size of buffer */ - u32 status; /* command status, filled by PSP if applicable */ -} __packed; - -/* - * command-specific buffer definitions: see NDA document #54267 - * The following commands need a buffer definition if they are to be used. - * All other commands will work with the default buffer. - * MBOX_BIOS_CMD_SMM_INFO MBOX_BIOS_CMD_PSP_QUERY - * MBOX_BIOS_CMD_SX_INFO MBOX_BIOS_CMD_S3_DATA_INFO - * MBOX_BIOS_CMD_RSM_INFO - */ - -struct mbox_default_buffer { /* command-response buffer unused by command */ - struct mbox_buffer_header header; -} __attribute__((packed, aligned(32))); - -/* send_psp_command() error codes */ +/* BIOS-to-PSP functions return 0 if successful, else negative value */ #define PSPSTS_SUCCESS 0 #define PSPSTS_NOBASE 1 #define PSPSTS_HALTED 2 @@ -87,11 +31,6 @@ #define PSPSTS_INVALID_NAME 8 #define PSPSTS_INVALID_BLOB 9
-#define PSP_INIT_TIMEOUT 10000 /* 10 seconds */ -#define PSP_CMD_TIMEOUT 1000 /* 1 second */ - -/* BIOS-to-PSP functions return 0 if successful, else negative value */ - int psp_notify_dram(void);
/* diff --git a/src/soc/amd/common/block/psp/psp.c b/src/soc/amd/common/block/psp/psp.c index 339e0e2..921775e 100644 --- a/src/soc/amd/common/block/psp/psp.c +++ b/src/soc/amd/common/block/psp/psp.c @@ -22,6 +22,7 @@ #include <console/console.h> #include <device/pci_ops.h> #include <amdblocks/psp.h> +#include "psp_def.h" #include <soc/iomap.h> #include <soc/northbridge.h>
diff --git a/src/soc/amd/common/block/psp/psp_def.h b/src/soc/amd/common/block/psp/psp_def.h new file mode 100644 index 0000000..4b3ca6a --- /dev/null +++ b/src/soc/amd/common/block/psp/psp_def.h @@ -0,0 +1,73 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* This file is part of the coreboot project. */ + +#ifndef __AMD_PSP_DEF_H__ +#define __AMD_PSP_DEF_H__ + +#include <types.h> + +/* x86 to PSP commands */ +#define MBOX_BIOS_CMD_DRAM_INFO 0x01 +#define MBOX_BIOS_CMD_SMM_INFO 0x02 +#define MBOX_BIOS_CMD_SX_INFO 0x03 +#define MBOX_BIOS_CMD_RSM_INFO 0x04 +#define MBOX_BIOS_CMD_PSP_QUERY 0x05 +#define MBOX_BIOS_CMD_BOOT_DONE 0x06 +#define MBOX_BIOS_CMD_CLEAR_S3_STS 0x07 +#define MBOX_BIOS_CMD_S3_DATA_INFO 0x08 +#define MBOX_BIOS_CMD_NOP 0x09 +#define MBOX_BIOS_CMD_SMU_FW 0x19 +#define MBOX_BIOS_CMD_SMU_FW2 0x1a +#define MBOX_BIOS_CMD_ABORT 0xfe + +/* generic PSP interface status */ +#define STATUS_INITIALIZED 0x1 +#define STATUS_ERROR 0x2 +#define STATUS_TERMINATED 0x4 +#define STATUS_HALT 0x8 +#define STATUS_RECOVERY 0x10 + +/* psp_mbox consists of hardware registers beginning at PSPx000070 + * mbox_command: BIOS->PSP command, cleared by PSP when complete + * mbox_status: BIOS->PSP interface status + * cmd_response: pointer to command/response buffer + */ +struct psp_mbox { + u32 mbox_command; + u32 mbox_status; + u64 cmd_response; /* definition conflicts w/BKDG but matches agesa */ +} __packed; + +/* command/response format, BIOS builds this in memory + * mbox_buffer_header: generic header + * mbox_buffer: command-specific buffer format + * + * AMD reference code aligns and pads all buffers to 32 bytes. + */ +struct mbox_buffer_header { + u32 size; /* total size of buffer */ + u32 status; /* command status, filled by PSP if applicable */ +} __packed; + +/* + * command-specific buffer definitions: see NDA document #54267 + * The following commands need a buffer definition if they are to be used. + * All other commands will work with the default buffer. + * MBOX_BIOS_CMD_SMM_INFO MBOX_BIOS_CMD_PSP_QUERY + * MBOX_BIOS_CMD_SX_INFO MBOX_BIOS_CMD_S3_DATA_INFO + * MBOX_BIOS_CMD_RSM_INFO + */ + +struct mbox_default_buffer { /* command-response buffer unused by command */ + struct mbox_buffer_header header; +} __attribute__((packed, aligned(32))); + +struct mbox_cmd_sx_info_buffer { + struct mbox_buffer_header header; + u8 sleep_type; +} __attribute__((packed, aligned(32))); + +#define PSP_INIT_TIMEOUT 10000 /* 10 seconds */ +#define PSP_CMD_TIMEOUT 1000 /* 1 second */ + +#endif /* __AMD_PSP_DEF_H__ */
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40015 )
Change subject: soc/amd/common/psp: Move definitions into a private file ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/40015/2/src/soc/amd/common/block/ps... File src/soc/amd/common/block/psp/psp.c:
https://review.coreboot.org/c/coreboot/+/40015/2/src/soc/amd/common/block/ps... PS2, Line 25: #include "psp_def.h" nit: Maybe put the "" includes below the <> includes ?
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40015 )
Change subject: soc/amd/common/psp: Move definitions into a private file ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/40015/2/src/soc/amd/common/block/ps... File src/soc/amd/common/block/psp/psp.c:
https://review.coreboot.org/c/coreboot/+/40015/2/src/soc/amd/common/block/ps... PS2, Line 25: #include "psp_def.h"
nit: Maybe put the "" includes below the <> includes ?
+1
Hello build bot (Jenkins), Raul Rangel, Marshall Dawson, Marshall Dawson, Angel Pons, Eric Peers,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40015
to look at the new patch set (#3).
Change subject: soc/amd/common/psp: Move definitions into a private file ......................................................................
soc/amd/common/psp: Move definitions into a private file
Declutter psp.h by removing internal details the caller doesn't need to know.
TEST: Verify PSP functionality on google/grunt
Change-Id: I2fb0ed1d2697c313fb8475e3f00482899e729130 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Reviewed-on: https://chromium-review.googlesource.com/2020366 Tested-by: Eric Peers epeers@google.com Reviewed-by: Eric Peers epeers@google.com --- M src/soc/amd/common/block/include/amdblocks/psp.h M src/soc/amd/common/block/psp/psp.c A src/soc/amd/common/block/psp/psp_def.h 3 files changed, 75 insertions(+), 62 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/40015/3
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40015 )
Change subject: soc/amd/common/psp: Move definitions into a private file ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40015/2/src/soc/amd/common/block/ps... File src/soc/amd/common/block/psp/psp.c:
https://review.coreboot.org/c/coreboot/+/40015/2/src/soc/amd/common/block/ps... PS2, Line 25: #include "psp_def.h"
+1
Done
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40015 )
Change subject: soc/amd/common/psp: Move definitions into a private file ......................................................................
Patch Set 3: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40015 )
Change subject: soc/amd/common/psp: Move definitions into a private file ......................................................................
Patch Set 3: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40015 )
Change subject: soc/amd/common/psp: Move definitions into a private file ......................................................................
Patch Set 3: Code-Review+2
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/+/40015
to look at the new patch set (#4).
Change subject: soc/amd/common/psp: Move definitions into a private file ......................................................................
soc/amd/common/psp: Move definitions into a private file
Declutter psp.h by removing internal details the caller doesn't need to know.
BUG=b:130660285 TEST: Verify PSP functionality on google/grunt
Change-Id: I2fb0ed1d2697c313fb8475e3f00482899e729130 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Reviewed-on: https://chromium-review.googlesource.com/2020366 Tested-by: Eric Peers epeers@google.com Reviewed-by: Eric Peers epeers@google.com --- M src/soc/amd/common/block/include/amdblocks/psp.h M src/soc/amd/common/block/psp/psp.c A src/soc/amd/common/block/psp/psp_def.h 3 files changed, 75 insertions(+), 62 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/40015/4
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40015 )
Change subject: soc/amd/common/psp: Move definitions into a private file ......................................................................
soc/amd/common/psp: Move definitions into a private file
Declutter psp.h by removing internal details the caller doesn't need to know.
BUG=b:130660285 TEST: Verify PSP functionality on google/grunt
Change-Id: I2fb0ed1d2697c313fb8475e3f00482899e729130 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Reviewed-on: https://chromium-review.googlesource.com/2020366 Tested-by: Eric Peers epeers@google.com Reviewed-by: Eric Peers epeers@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/40015 Reviewed-by: Raul Rangel rrangel@chromium.org Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/amd/common/block/include/amdblocks/psp.h M src/soc/amd/common/block/psp/psp.c A src/soc/amd/common/block/psp/psp_def.h 3 files changed, 75 insertions(+), 62 deletions(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: 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/include/amdblocks/psp.h b/src/soc/amd/common/block/include/amdblocks/psp.h index 42c802d..d7f6169 100644 --- a/src/soc/amd/common/block/include/amdblocks/psp.h +++ b/src/soc/amd/common/block/include/amdblocks/psp.h @@ -18,63 +18,7 @@ /* Get the mailbox base address - specific to family of device. */ struct psp_mbox *soc_get_mbox_address(void);
-/* x86 to PSP commands */ -#define MBOX_BIOS_CMD_DRAM_INFO 0x01 -#define MBOX_BIOS_CMD_SMM_INFO 0x02 -#define MBOX_BIOS_CMD_SX_INFO 0x03 -#define MBOX_BIOS_CMD_RSM_INFO 0x04 -#define MBOX_BIOS_CMD_PSP_QUERY 0x05 -#define MBOX_BIOS_CMD_BOOT_DONE 0x06 -#define MBOX_BIOS_CMD_CLEAR_S3_STS 0x07 -#define MBOX_BIOS_CMD_S3_DATA_INFO 0x08 -#define MBOX_BIOS_CMD_NOP 0x09 -#define MBOX_BIOS_CMD_SMU_FW 0x19 -#define MBOX_BIOS_CMD_SMU_FW2 0x1a -#define MBOX_BIOS_CMD_ABORT 0xfe - -/* generic PSP interface status */ -#define STATUS_INITIALIZED 0x1 -#define STATUS_ERROR 0x2 -#define STATUS_TERMINATED 0x4 -#define STATUS_HALT 0x8 -#define STATUS_RECOVERY 0x10 - -/* psp_mbox consists of hardware registers beginning at PSPx000070 - * mbox_command: BIOS->PSP command, cleared by PSP when complete - * mbox_status: BIOS->PSP interface status - * cmd_response: pointer to command/response buffer - */ -struct psp_mbox { - u32 mbox_command; - u32 mbox_status; - u64 cmd_response; /* definition conflicts w/BKDG but matches agesa */ -} __packed; - -/* command/response format, BIOS builds this in memory - * mbox_buffer_header: generic header - * mbox_buffer: command-specific buffer format - * - * AMD reference code aligns and pads all buffers to 32 bytes. - */ -struct mbox_buffer_header { - u32 size; /* total size of buffer */ - u32 status; /* command status, filled by PSP if applicable */ -} __packed; - -/* - * command-specific buffer definitions: see NDA document #54267 - * The following commands need a buffer definition if they are to be used. - * All other commands will work with the default buffer. - * MBOX_BIOS_CMD_SMM_INFO MBOX_BIOS_CMD_PSP_QUERY - * MBOX_BIOS_CMD_SX_INFO MBOX_BIOS_CMD_S3_DATA_INFO - * MBOX_BIOS_CMD_RSM_INFO - */ - -struct mbox_default_buffer { /* command-response buffer unused by command */ - struct mbox_buffer_header header; -} __attribute__((packed, aligned(32))); - -/* send_psp_command() error codes */ +/* BIOS-to-PSP functions return 0 if successful, else negative value */ #define PSPSTS_SUCCESS 0 #define PSPSTS_NOBASE 1 #define PSPSTS_HALTED 2 @@ -87,11 +31,6 @@ #define PSPSTS_INVALID_NAME 8 #define PSPSTS_INVALID_BLOB 9
-#define PSP_INIT_TIMEOUT 10000 /* 10 seconds */ -#define PSP_CMD_TIMEOUT 1000 /* 1 second */ - -/* BIOS-to-PSP functions return 0 if successful, else negative value */ - int psp_notify_dram(void);
/* diff --git a/src/soc/amd/common/block/psp/psp.c b/src/soc/amd/common/block/psp/psp.c index 7ec8d7b..c580803 100644 --- a/src/soc/amd/common/block/psp/psp.c +++ b/src/soc/amd/common/block/psp/psp.c @@ -24,6 +24,7 @@ #include <amdblocks/psp.h> #include <soc/iomap.h> #include <soc/northbridge.h> +#include "psp_def.h"
static const char *psp_status_nobase = "error: PSP BAR3 not assigned"; static const char *psp_status_halted = "error: PSP in halted state"; diff --git a/src/soc/amd/common/block/psp/psp_def.h b/src/soc/amd/common/block/psp/psp_def.h new file mode 100644 index 0000000..4b3ca6a --- /dev/null +++ b/src/soc/amd/common/block/psp/psp_def.h @@ -0,0 +1,73 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* This file is part of the coreboot project. */ + +#ifndef __AMD_PSP_DEF_H__ +#define __AMD_PSP_DEF_H__ + +#include <types.h> + +/* x86 to PSP commands */ +#define MBOX_BIOS_CMD_DRAM_INFO 0x01 +#define MBOX_BIOS_CMD_SMM_INFO 0x02 +#define MBOX_BIOS_CMD_SX_INFO 0x03 +#define MBOX_BIOS_CMD_RSM_INFO 0x04 +#define MBOX_BIOS_CMD_PSP_QUERY 0x05 +#define MBOX_BIOS_CMD_BOOT_DONE 0x06 +#define MBOX_BIOS_CMD_CLEAR_S3_STS 0x07 +#define MBOX_BIOS_CMD_S3_DATA_INFO 0x08 +#define MBOX_BIOS_CMD_NOP 0x09 +#define MBOX_BIOS_CMD_SMU_FW 0x19 +#define MBOX_BIOS_CMD_SMU_FW2 0x1a +#define MBOX_BIOS_CMD_ABORT 0xfe + +/* generic PSP interface status */ +#define STATUS_INITIALIZED 0x1 +#define STATUS_ERROR 0x2 +#define STATUS_TERMINATED 0x4 +#define STATUS_HALT 0x8 +#define STATUS_RECOVERY 0x10 + +/* psp_mbox consists of hardware registers beginning at PSPx000070 + * mbox_command: BIOS->PSP command, cleared by PSP when complete + * mbox_status: BIOS->PSP interface status + * cmd_response: pointer to command/response buffer + */ +struct psp_mbox { + u32 mbox_command; + u32 mbox_status; + u64 cmd_response; /* definition conflicts w/BKDG but matches agesa */ +} __packed; + +/* command/response format, BIOS builds this in memory + * mbox_buffer_header: generic header + * mbox_buffer: command-specific buffer format + * + * AMD reference code aligns and pads all buffers to 32 bytes. + */ +struct mbox_buffer_header { + u32 size; /* total size of buffer */ + u32 status; /* command status, filled by PSP if applicable */ +} __packed; + +/* + * command-specific buffer definitions: see NDA document #54267 + * The following commands need a buffer definition if they are to be used. + * All other commands will work with the default buffer. + * MBOX_BIOS_CMD_SMM_INFO MBOX_BIOS_CMD_PSP_QUERY + * MBOX_BIOS_CMD_SX_INFO MBOX_BIOS_CMD_S3_DATA_INFO + * MBOX_BIOS_CMD_RSM_INFO + */ + +struct mbox_default_buffer { /* command-response buffer unused by command */ + struct mbox_buffer_header header; +} __attribute__((packed, aligned(32))); + +struct mbox_cmd_sx_info_buffer { + struct mbox_buffer_header header; + u8 sleep_type; +} __attribute__((packed, aligned(32))); + +#define PSP_INIT_TIMEOUT 10000 /* 10 seconds */ +#define PSP_CMD_TIMEOUT 1000 /* 1 second */ + +#endif /* __AMD_PSP_DEF_H__ */
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40015 )
Change subject: soc/amd/common/psp: Move definitions into a private file ......................................................................
Patch Set 5:
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/2001 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2000 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1999
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Felix Held (Code Review)
-
Paul Menzel (Code Review)
-
Raul Rangel (Code Review)