[coreboot-gerrit] Change in coreboot[master]: Add header file: psp.h, and modify code in psp.c to use internal func...

Tue Jul 24 23:09:35 CEST 2018

Martin Roth has posted comments on this change. ( https://review.coreboot.org/27619 )

Change subject: Add header file: psp.h, and modify code in psp.c to use internal functions rather than functions from PspBaseLib.
......................................................................

Patch Set 1:

(18 comments)

Lots of style comments.  Welcome to coreboot.  :)

https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.h
File src/soc/amd/common/block/psp/psp.h:

https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.h@4
PS1, Line 4: 2017
Is this a new file, or copied from somewhere else?

https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.h@34
PS1, Line 34: G		    
change to spaces to match the rest of the file?

https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.h@35
PS1, Line 35: 
Extra newline at the end.  This is what's causing the jenkins failure.

https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c
File src/soc/amd/common/block/psp/psp.c:

https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@12
PS1, Line 12:  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.	 See the
You probably didn't mean to make this change

https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@27
PS1, Line 27: 
extra line

https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@37
PS1, Line 37: static BOOLEAN psp_bar_init_early(VOID);
We don't generally have function definitions for static functions.

https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@38
PS1, Line 38: 
extra line

https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@46
PS1, Line 46: BOOLEAN
We don't really use BOOLEAN in coreboot.  It's only used in vendorcode

https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@49
PS1, Line 49: PspMmioSize;
No camelcase unless it's needed for interfacing with vendorcode please.

https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@61
PS1, Line 61: //
For multi-line comments, please use a style like this:
/*
 * Comment
 */

https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@96
PS1, Line 96:  0x34);
Does this need to be split to a separate line?

https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@110
PS1, Line 110: /**
             :   Return the PspMMIO MMIO location
             : 
             :   @param[in] PspMmio Pointer to Psp MMIO address
             : 
             :   @retval BOOLEAN  0: Error, 1 Success
             : **/
Same issue with the comment style, with the additional * for doxygen:

/**
 *
 *
 */

https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@117
PS1, Line 117: static BOOLEAN
             : get_psp_bar3_addr(
             : 	IN OUT   u32 *PspMmio
             : 	)
Single line?  We don't typically use BOOLEAN, or IN & OUT in coreboot code:

static int get_psp_bar3_addr(u32 psp_mmio_addr)
{

https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@134
PS1, Line 134: BIT12
You mention updating the magic numbers in a todo, but just changing this to a BAR3_HIDE #define would eliminate the need for the above comment.

https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@140
PS1, Line 140: __func__
We probably don't need to print out __func__ all over the place.  I'd just print the message and leave that off.  Generally you can find the correct function if you just search for the error message, and it makes the log a lot cleaner if you leave them out.

https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@148
PS1, Line 148: BIOS_DEBUG
You might want to look at what you're printing vs what's really useful.  Just printing the address of the PSP isn't really going to be useful most of the time, so you might want to change this to BIOS_SPEW.

On the other hand, when we fail above, that might be better as a BIOS_WARNING.

https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@187
PS1, Line 187: , base=0x%x...\
Here and above, do we care what base is if the function failed?

https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@287
PS1, Line 287: 	
You might want to make sure your editor isn't configured to change this for some reason.

-- 
To view, visit https://review.coreboot.org/27619
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2740ceb945736c6e413f7d0bd0c41a19e19c7d5a
Gerrit-Change-Number: 27619
Gerrit-PatchSet: 1
Gerrit-Owner: Charles Marslett <charles.marslett at amd.corp-partner.google.com>
Gerrit-Reviewer: Charles Marslett <charles.marslett at silverbackltd.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-CC: Martin Roth <martinroth at google.com>
Gerrit-Comment-Date: Tue, 24 Jul 2018 21:09:35 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180724/e8cba030/attachment.html>