[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:

(9 comments)

https://review.coreboot.org/#/c/19157/14/src/northbridge/amd/pi/00670F00/psp.c
File src/northbridge/amd/pi/00670F00/psp.c:

Line 90: static void wr_mbox_cmd_resp(struct psp_mbox *mbox, void *buffer)
> Buffer is not flushed to DRAM prior to sending PSP a command to potentially
That's not mentioned in the spec.  I assume it's snooped like any other transaction.  I think that would be a pretty bad design if not.


Line 102: 	long usec_wait = 10000000; /* arbitrary 10 seconds */
> coverage may want 'long int'
coverity?  I infer it's OK.  I don't see any other variables done that way, and intptr_t is typedef of long.


Line 123: 	return PSPSTS_INIT_TIMEOUT;
> At the callsite, only not-equal-to zero is tested for, and we never explici
Yes.  Thanks.


Line 140: 	if (rd_mbox_sts(mbox) & STATUS_HALT) {
> IMHO we should not use enum with bitwise boolean ops. Don't know if coverag
> Don't know if coverage will be happy about this.

I don't know.  I can identify other examples, e.g. in vboot.  Can you point me to more info on this?


Line 150: 	if (wait_initialized(mbox)) {
> Code should not break if we simply change order of enums, we make assumptio
There are lots of examples of doing it like this, but I think it's a very reasonable change.


Line 192: 	buffer.header.status = 0; /* PSP does not report status for this cmd */
> I can see patchset CarrizoPI uses this status field as return value from Ps
It seems like send_psp_command and printk below are keeping this from getting optimized out.  Then rd_resp_sts is treating it as volatile.  I think it's OK here.


Line 196: 					status_to_string(cmd_status));
> If we really get no status, no need to print it either?
I kind of like seeing that the status=0.  There's no string associated with that which will show up in the console (just a trailing space).


https://review.coreboot.org/#/c/19157/14/src/northbridge/amd/pi/00670F00/psp.h
File src/northbridge/amd/pi/00670F00/psp.h:

Line 51: };
> Should we have __attribute__ ((packed)) here as well?
yes


Line 87: #define PSP_DEV dev_find_slot(0, PCI_DEVFN(PSP_PCI_DEV, PSP_PCI_DEV))
> PCI_DEVFN(PSP_PCI_DEV, PSP_PCI_FN)
right - copy/paste fail


-- 
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)
Gerrit-HasComments: Yes