[coreboot-gerrit] [L] Change in coreboot[main]: mb/amd/crater: Add Crater mainboard support for Renoir/Cezanne SOC

[Felix Held (Code Review)]

Attention is currently required from: Ana Carolina Cabral, Anand Vaikar, Bao Zheng, Fred Reitberger, Jason Glenesk, Matt DeVillier.

Felix Held has posted comments on this change by Anand Vaikar. ( https://review.coreboot.org/c/coreboot/+/85749?usp=email )

Change subject: mb/amd/crater: Add Crater mainboard support for Renoir/Cezanne SOC
......................................................................

Patch Set 8:

(18 comments)

File src/mainboard/amd/crater/Kconfig:

https://review.coreboot.org/c/coreboot/+/85749/comment/24ad4303_7c02d175?usp... :
PS8, Line 33: if BOARD_AMD_CRATER_RENOIR
this if is redundant, since this config option is already inside a "if BOARD_AMD_CRATER_RENOIR" block. same for the 2 config options below

https://review.coreboot.org/c/coreboot/+/85749/comment/15e6fe79_6bbf5204?usp... :
PS8, Line 61: 	default "3rdparty/blobs/mainboard/amd/crater/EcSig_Crater.bin"
since this file isn't available in the public blobs repo, i'd say that we should have this in the .config used in the build instead of here in the kconfig

https://review.coreboot.org/c/coreboot/+/85749/comment/614f27a5_06046f2d?usp... :
PS8, Line 87: 
there should only be one empty line

https://review.coreboot.org/c/coreboot/+/85749/comment/47edcca6_ed1185c6?usp... :
PS8, Line 105: choice
             : 	prompt "I2C Speed Selection"
             : 	default SELECT_400KHz
             : 	help
             : 		Select Different speeds of I2C 400KHz / 1 MHz
             : 
             : config SELECT_400KHz
             : 	bool "400KHz"
             : 
             : config SELECT_1MHz
             : 	bool "1MHz"
             : 
             : endchoice
not needed, since the i2c speed can and should be configured in the devicetree

File src/mainboard/amd/crater/chromeos.c:

https://review.coreboot.org/c/coreboot/+/85749/comment/d8f3cd19_6eaf0658?usp... :
PS8, Line 17: Birman
Crater

File src/mainboard/amd/crater/chromeos_renoir.fmd:

PS8: 
not sure if the PSP_NVRAM and PSP_RPMC_NVRAM fmpa sections would easily fit into the fmap. if so, it might be good to add those; i'm fine with that being looked into in some follow-up patch

File src/mainboard/amd/crater/devicetree_renoir.cb:

https://review.coreboot.org/c/coreboot/+/85749/comment/309ebf86_9f2ab003?usp... :
PS8, Line 3: 
only one empty line

https://review.coreboot.org/c/coreboot/+/85749/comment/135b59cc_d9f03355?usp... :
PS8, Line 43: 	register "gpp_clk_config[0]" = "GPP_CLK_ON"
            : 	register "gpp_clk_config[1]" = "GPP_CLK_ON"
            : 	register "gpp_clk_config[2]" = "GPP_CLK_ON"
            : 	register "gpp_clk_config[3]" = "GPP_CLK_ON"
            : 	register "gpp_clk_config[4]" = "GPP_CLK_ON"
            : 	register "gpp_clk_config[5]" = "GPP_CLK_ON"
            : 	register "gpp_clk_config[6]" = "GPP_CLK_ON"
haven't checked with the schematic, but some of those probably aren't correct. feel free to keep it as it is for now and revisit this later, since this would be mainly an optimization to lower the power consumption

File src/mainboard/amd/crater/early_gpio.c:

https://review.coreboot.org/c/coreboot/+/85749/comment/e812dac4_fac03143?usp... :
PS8, Line 13:     
should be tab not spaces

https://review.coreboot.org/c/coreboot/+/85749/comment/091e1bcc_fe6ea363?usp... :
PS8, Line 70: };
i'd add an empty line between the array and the function here

File src/mainboard/amd/crater/ec.h:

https://review.coreboot.org/c/coreboot/+/85749/comment/3b45450c_1c235810?usp... :
PS8, Line 7: void crater_boardrevision(void);
i'd add an empty line between the last function and the #endif

File src/mainboard/amd/crater/ec.c:

https://review.coreboot.org/c/coreboot/+/85749/comment/c2b17876_3f9f551b?usp... :
PS8, Line 156: void crater_boardrevision(void)
hmm, i wonder if it would be cleaner if this file provides a function to get the board revision and have the revision-specific soc gpio configuration done in the mainboard code that configures the soc gpios

File src/mainboard/amd/crater/gpio.c:

PS8: 
this is probably missing some gpios

https://review.coreboot.org/c/coreboot/+/85749/comment/5be01867_8a458178?usp... :
PS8, Line 16:     
tab

File src/mainboard/amd/crater/port_descriptors_renoir.c:

https://review.coreboot.org/c/coreboot/+/85749/comment/a6e9af65_d2961bca?usp... :
PS8, Line 96: ec_set_ports(CRATER_EC_CMD, CRATER_EC_DATA);
            : 	BoardRev = ec_read(ECRAM_BOARDID_OFFSET + 0x3);
would be good to have a function in the mainboard's ec code for this

https://review.coreboot.org/c/coreboot/+/85749/comment/0c481978_4bdf38a5?usp... :
PS8, Line 115: void __weak mainboard_fsp_mainboard_params_init(FSP_M_CONFIG *m_config)
             : {
             : 
             : }
i don't see this function being needed or called, so it can likely be removed. same for the prototype in line 12

File src/soc/amd/cezanne/chip.h:

https://review.coreboot.org/c/coreboot/+/85749/comment/41fc7880_13d86a45?usp... :
PS8, Line 112: 	bool   acp_i2s_use_external_48mhz_osc;
this and the corresponding change in the cezanne soc code should be done in a separate patch before adding the mainboard

File src/soc/amd/cezanne/include/soc/gpio.h:

PS8: 
this change should also be split out into a separate patch, so that the mainboard patch doesn't need to touch any files outside of the mainboard directory