Change in coreboot[master]: Add header file: psp.h, and modify code in psp.c to use internal func...
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@3... 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@3... 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@1... 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@2... PS1, Line 27: extra line
https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@3... 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@3... PS1, Line 38: extra line
https://review.coreboot.org/#/c/27619/1/src/soc/amd/common/block/psp/psp.c@4... 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@4... 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@6... 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@9... 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@1... 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@1... 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@1... 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@1... 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@1... 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@1... 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@2... PS1, Line 287: You might want to make sure your editor isn't configured to change this for some reason.
-
Martin Roth (Code Review)