Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40295 )
Change subject: soc/amd/common/psp: Add SmmInfo command ......................................................................
Patch Set 6:
(10 comments)
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/psp.h:
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/in... PS6, Line 17: nit: space
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... File src/soc/amd/common/block/psp/psp.c:
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 18: c2p What does c2p mean?
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 22: struct _p2c_buffer { : u8 buffer[P2C_BUFFER_MAXSIZE]; : } __attribute__((aligned(32))); : : static struct _p2c_buffer p2c_buffer; Could make this an anon stuct since it's only used once:
struct { u8 buffer[SIZE]; } __attribute__((aligned(32))) p2c_buffer;
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 31: set_smm_flag Can we make this function only get linked in the smm stage. Either a #ifdef around the method, or place it in a new file and add it to smm-y += smm-flag.c.
I don't like it silently returning.
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 38: clear_smm_flag Same
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 148: ENV_SMM Same, only link in SMM
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 156: 0xfffff000 #def the mask or comment what it means.
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 161: soc_fill_smm_reg_info
If there's a declaration for this function in a header, you can use a regular conditional instead of […]
+1
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 164: buffer.req.psp_smm_data_region = (uintptr_t)p2c_buffer.buffer; : buffer.req.psp_smm_data_length = sizeof(p2c_buffer); : : buffer.req.psp_mbox_smm_buffer_address = (uintptr_t)c2p_buffer.buffer; : buffer.req.psp_mbox_smm_flag_address = (uintptr_t)&smm_flag; nit: you could set these static fields in the buffer initializer above.
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... File src/soc/amd/common/block/psp/psp_def.h:
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 111: ENV_SMM Maybe move the buffer and the functions into a psp-smm file so you can avoid this #if.