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... 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... 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
-
Marshall Dawson (Code Review)