Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/19157 )
Change subject: amd/pi/00670F00: Add AMD PSP support for Stoney Ridge ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/#/c/19157/1/src/northbridge/amd/pi/00670F00/psp.... File src/northbridge/amd/pi/00670F00/psp.c:
Line 86: ; /* wait for initialized and no command in progress */
ok. one more thing to fix. dont' these parts have a constant tsc? dont' these parts have a constant tsc?
Yes. Everything Family 10h and later, I believe. This has probably been propagated over the years
https://review.coreboot.org/#/c/19157/14/src/northbridge/amd/pi/00670F00/psp... File src/northbridge/amd/pi/00670F00/psp.c:
Line 184
Don't we are care about being able to know if this command failed?
Short of putting it in the console log, I'm not sure what I would do with that info. Things like graphics aren't going to work properly if the notification fails or is missing. Resetting the PSP or retrying isn't in the spec, that I've found. The AMD code drops any command that fails.
Line 191
Why would we include u8 reserved[24] in the size?
I think my best argument is for consistency, i.e. all other commands will also use the sizes of their mbox_xxxxxx_buffer structures. Some will have reserved bytes and others will be larger and not need padding.
FWIW, the PSP seems to not check the value of .size for this particular call. Setting it to 0 before issuing the command still seemed to complete successfully. No info yet on a command that sends actual data to the PSP but is also <32 bytes.
https://review.coreboot.org/#/c/19157/14/src/northbridge/amd/pi/00670F00/psp... File src/northbridge/amd/pi/00670F00/psp.h:
PS14, Line 50: mbox
If this is really a hardware register then you shouldn't be using pointer f
Eww. Thanks for catching that. Wrong in AMD ref code.