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

(3 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 */
> You can use struct stopwatch in timer.h. stopwatch_expired()
Difficult with AMD currently.  stopwatch uses timer_monotonic_get() but all AMD use the LAPIC timer and that version of timer_monotonic_get() isn't present in romstage.


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

PS12, Line 125: 
> This reads better with the parens, I think. I know the precedence has & hig
agree


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

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;
> There's no need for these typedefs nor for the indirection. I'm just very c
I sort of like typdefs but OK.

Have changed how the alignment is done.  The spec doesn't address alignment, but the reference code does it.  Nor does the reference code explain why the buffers are padded to a minimum of 32 bytes.  The comment seems intentional but no explanation is given.  I'm a little leery about changing that.

> There's no need for struct mbox_default_buffer...

I agree it looks funny the way it is but as new commands and their buffer definitions are added, it may look better.  An example that's not implemented yet will look something like:

 struct smm_trigger_info {
 	u64 address;
 	u32 address_type;
 	/* more stuff */
 };
 struct smm_req_buffer {
 	u64 smm_base;
 	/* more stuff */
 	struct smm_trigger_info smm_trig_info;
 };
 struct smm_info_buffer { /* larger than 32, BTW */
 	struct mbox_buffer_header header;
 	struct smm_req_buffer req;
 };

In general, I'd prefer the code match the spec even if it adds an extra layer in the structs.


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