Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40295 )
Change subject: soc/amd/psp: Add SmmInfo command ......................................................................
Patch Set 7:
(9 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
Done
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?
moved the define for C2P_BUFFER_MAXSIZE that has a comment on this
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: […]
Done
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. […]
Done
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 38: clear_smm_flag
Same
Done
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 148: ENV_SMM
Same, only link in SMM
Done
https://review.coreboot.org/c/coreboot/+/40295/6/src/soc/amd/common/block/ps... PS6, Line 161: soc_fill_smm_reg_info
+1
tried this and the compiler complained that buffer.req has no member smm_reg_info. wontfix.
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.
Done
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.
Done