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

(4 comments)

https://review.coreboot.org/#/c/19157/1/src/northbridge/amd/pi/00670F00/psp.c
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.c
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.h
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.


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