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

(15 comments)

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

Line 53: 	GetPspBar3Addr(&base);
> This function has a return indicating success or not. Why is get_mbox_addre
Done


PS1, Line 59: baseptr = (uintptr_t)base;
> Why assign to the intermediary variable?  just use base?
IIRC, had trouble building and related to the reason for the stupid UINT32 above vs. just u32.


PS1, Line 69: u32
> uintptr_t
Done


PS1, Line 73: PspLibPciWriteConfig
> Why can't we use coreboot's native code?
Done


PS1, Line 76: mbox->mbox_status
> read32(&mbox->mbox_status) and throughout. Even better would be a wrapper:
Done


Line 86: 		;	/* wait for initialized and no command in progress */
> add some sort of timeout here and below?
Maybe.  Features start going away if we can't talk to the PSP, but it shouldn't be fatal if we've gotten here.

Do you know of any type of helper for that?  Seems like I've seen a timeout-waiting-for-bit function somewhere...


PS1, Line 95: mbox->mbox_status
> If the PCI_COMMAND_MEMORY was knocked down in the write previously why are 
Probably left over and not removed from previous implementation.  Still worked because the original command reg had the bits enabled.


PS1, Line 101: PspLibPciWriteConfig(PCI_COMMAND, command_reg)
> Add a comment?
Done, good enough?


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

PS1, Line 42: volatile
> Force the read of your structure in the code -- not in decorating fields of
Done


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
Reworded, "value" was a poor choice.  The BKDG calls offset 78 CmdRspBufAddrHi and 7c AddrLo.  However, it seems like a 64-bit pointer would be opposite.  Did I see that backwards?  I'd be OK taking the comment out.


Line 49:   u32 status;
> not tab indented
Done


PS1, Line 52: typedef struct {
            : 	mbox_buffer_header header;
            : } mbox_default_buffer;
            : 
            : typedef union _mbox_buffer {
            : 	mbox_default_buffer default_buff;
            : 	u8 reserved[32];
            : } mbox_buffer;
            : 
            : typedef struct {
            : 	mbox_buffer buffer[2];
            : } unaligned_mbox_buffer;
> What is with all of this indirection? I do not understand at all.
How about with comments added?


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?  
Done


PS1, Line 65: #define BALIGN32(p)  ((void *) (((u32)(void *)(p) + 32) & ~0x1f))
> Since mbox_buffer_header is already 32-bit aligned the unaligned_mbox_buffe
It's aligning to 32 bytes in the C file.  Use of this macro was copied from the PI package, although I haven't seen a requirement in the spec.  I just now noticed it's doing math on a void *.


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


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