[coreboot-gerrit] Change in coreboot[master]: amd/pi/00670F00: Add AMD PSP support for Stoney Ridge

[Marshall Dawson (Code Review)]

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.c
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.


-- 
To view, visit https://review.coreboot.org/19157
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf111470b261d6516b878e88be471967abe681b3
Gerrit-PatchSet: 14
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Marc Jones <marc at marcjonesconsulting.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki at gmail.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd at gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth at google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-HasComments: Yes