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 14:
(7 comments)
https://review.coreboot.org/#/c/19157/14/src/northbridge/amd/pi/00670F00/psp... File src/northbridge/amd/pi/00670F00/psp.c:
Line 102: long usec_wait = 10000000; /* arbitrary 10 seconds */
https://www.coreboot.org/Development_Guidelines#General_Guidelines https://www.coreboot.org/Development_Guidelines#General_Guidelines
good enough - done.
Line 140: if (rd_mbox_sts(mbox) & STATUS_HALT) {
Turns out this is all within standard, C enums are just ints.
Done - changed to #define
Line 150: if (wait_initialized(mbox)) {
Yeah, I forgot it's the same with AGESA_SUCCESS. So no real need for "fix"
Done
Line 170: if (rd_mbox_sts(mbox) & (STATUS_ERROR || STATUS_TERMINATED)) {
bitwise, not boolean OR
Done
Line 184: void psp_notify_dram(void)
The only issue is that we don't let the platform/mainboard set policy then
Done
Line 191: buffer.header.size = sizeof(struct mbox_default_buffer);
PI equivalent MBOX_DEFAULT_BUFFER is only the header without reserve, so si PI equivalent MBOX_DEFAULT_BUFFER is only the header without reserve, so size used here is not consistent with the reference.
I see now. Removed reserved bytes from mbox_default_buffer.
Line 196: status_to_string(cmd_status));
The status_to_string() is indexed with cmd_status that is not the same as b I think we just always show static "status=0" here, errors or no.
I don't think that's correct. cmd_status can report timeout, PSP-in-recovery, etc. It uses the return value from send_psp_command() which can be affected by buffer->header.status. I can jam the return and see an error message.
-
Marshall Dawson (Code Review)