Attention is currently required from: Jason Glenesk, Raul Rangel, Martin L Roth, Marshall Dawson, Matt DeVillier, Paul Menzel, Arthur Heymans, Fred Reitberger, Felix Held.
ritul guru has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/65523 )
Change subject: soc/amd/common/psp: Support PSP SMI handler ......................................................................
Patch Set 16:
(10 comments)
File src/soc/amd/common/block/include/amdblocks/spi.h:
https://review.coreboot.org/c/coreboot/+/65523/comment/645168ff_f4a62b12 PS15, Line 34: register
i'd be a bit more specific here and say register bit instead of register, since other bits on that r […]
Ack
https://review.coreboot.org/c/coreboot/+/65523/comment/5e0f4344_f61d76c5 PS15, Line 33: /* : * Re-purpose unused SPI controller register for semaphore to synchronize SPI access : * between SMM and non-SMM software. : */ : #define SPI_SEMAPHORE 0xfc : #define SPI_SEM_LOCKED BIT(4)
please move this below the SPI_FIFO_DEPTH define to have the register definitions ordered by their o […]
Ack
File src/soc/amd/common/block/psp/psp_def.h:
https://review.coreboot.org/c/coreboot/+/65523/comment/45b53e9c_4ff38007 PS15, Line 26: /* PSP to x86 commands */ : #define MBOX_PSP_CMD_SPI_INFO 0x83 : #define MBOX_PSP_CMD_SPI_READ 0x84 : #define MBOX_PSP_CMD_SPI_WRITE 0x85 : #define MBOX_PSP_CMD_SPI_ERASE 0x86
i'd move this to line 36 below the 3 psp v1-only x86 to psp commands to not have the psp to x86 mail […]
Ack
https://review.coreboot.org/c/coreboot/+/65523/comment/6b90d0d5_2e8e8d51 PS15, Line 63: struct pspv2_bios_mbox { : u32 mbox_command; : union { : u32 val; : struct smi_mbox_status { : u32 checksum:8; : u32 checksum_en:1; : u32 reserved:22; : u32 command_ready:1; : } __packed fields; : }; : struct mbox_default_buffer buffer; : } __packed;
since the pspv2_bios_mbox struct is only used in the p2c mailbox access code, i'd move it to the psp […]
Ack
https://review.coreboot.org/c/coreboot/+/65523/comment/90782f73_484d1ad1 PS15, Line 91: #define SMI_TARGET_NVRAM 0 : #define SMI_TARGET_RPMC_NVRAM 5
i'd use an enum instead of the defines here
Ack
https://review.coreboot.org/c/coreboot/+/65523/comment/f96c3825_7feb0b3d PS15, Line 94: #define PSP_INIT_TIMEOUT 10000 /* 10 seconds */ : #define PSP_CMD_TIMEOUT 1000 /* 1 second */
those are already defined in line 142 and 143
Ack
File src/soc/amd/common/block/psp/psp_gen2.c:
PS15:
hm, i'd move the p2c mailbox access functions to a separate file to not have the c2p and p2c mailbox […]
Ack
https://review.coreboot.org/c/coreboot/+/65523/comment/804b9d92_ea64de31 PS15, Line 10:
not directly related to this patch, but this file is missing a types. […]
Ack
https://review.coreboot.org/c/coreboot/+/65523/comment/58396765_20e7d83c PS15, Line 147: u8
i'd use bool here
Ack
https://review.coreboot.org/c/coreboot/+/65523/comment/f09beda5_f9237815 PS15, Line 180: u32
since this returns an enum mbox_p2c_status value. […]
Ack
-
ritul guru (Code Review)