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

[Martin Roth (Code Review)]

Martin Roth 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:

(7 comments)

https://review.coreboot.org/#/c/19157/1//COMMIT_MSG
Commit Message:

PS1, Line 11: Move  amd_intcpuio()
Remove extra space and fix spelling of the function.
"Move amd_initcpuio()"


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

PS1, Line 59: baseptr = (uintptr_t)base;
Why assign to the intermediary variable?  just use base?


Line 86: 		;	/* wait for initialized and no command in progress */
add some sort of timeout here and below?


PS1, Line 101: PspLibPciWriteConfig(PCI_COMMAND, command_reg)
Add a comment?
/* Restore the original command_reg value */

Maybe explain why we need to do that.


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

PS1, Line 44: /* value violates BKDG but matches agesa code */
I don't understand this comment.  We're defining a struct, so we don't have a value yet.  Size?  location within the struct?


PS1, Line 65: #define BALIGN32(p)  ((void *) (((u32)(void *)(p) + 32) & ~0x1f))
Use one of the align macros in src/commonlib/include/commonlib/helpers.h?  Or if none of those work, put this there with the others?


PS1, Line 68: pspsts_success
Uppercase for all of these?


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