Attention is currently required from: Ana Carolina Cabral, Bao Zheng, Felix Held, Fred Reitberger, Jason Glenesk, Matt DeVillier.
Anand Vaikar 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 9:
(18 comments)
File src/mainboard/amd/crater/Kconfig:
https://review.coreboot.org/c/coreboot/+/85749/comment/1bfcb03f_1cfb5183?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" bloc […]
Done
https://review.coreboot.org/c/coreboot/+/85749/comment/c4049fc0_307b7c3a?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 . […]
Done
https://review.coreboot.org/c/coreboot/+/85749/comment/bd523f04_f2599b90?usp... : PS8, Line 87:
there should only be one empty line
Done
https://review.coreboot.org/c/coreboot/+/85749/comment/e0cd50c4_bbd0a813?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
Done
File src/mainboard/amd/crater/chromeos.c:
https://review.coreboot.org/c/coreboot/+/85749/comment/a3acac6e_bcafcdb0?usp... : PS8, Line 17: Birman
Crater
Done
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. […]
sure, will take it up in subsequent patch.
File src/mainboard/amd/crater/devicetree_renoir.cb:
https://review.coreboot.org/c/coreboot/+/85749/comment/dbde155f_168ef3f2?usp... : PS8, Line 3:
only one empty line
Done
https://review.coreboot.org/c/coreboot/+/85749/comment/5ea7090e_86b98ab0?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. […]
sure, will take it up in subsequent patch.
File src/mainboard/amd/crater/early_gpio.c:
https://review.coreboot.org/c/coreboot/+/85749/comment/d42798d9_b0250e48?usp... : PS8, Line 13:
should be tab not spaces
Done
https://review.coreboot.org/c/coreboot/+/85749/comment/c857a4b2_0b4bb71d?usp... : PS8, Line 70: };
i'd add an empty line between the array and the function here
Done
File src/mainboard/amd/crater/ec.h:
https://review.coreboot.org/c/coreboot/+/85749/comment/3a2be262_a9709062?usp... : PS8, Line 7: void crater_boardrevision(void);
i'd add an empty line between the last function and the #endif
Done
File src/mainboard/amd/crater/ec.c:
https://review.coreboot.org/c/coreboot/+/85749/comment/b0baf8f3_f5578e30?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 […]
sure will take it up in subsequent patches
File src/mainboard/amd/crater/gpio.c:
PS8:
this is probably missing some gpios
will address in subsequent patch by reviewing the code.
https://review.coreboot.org/c/coreboot/+/85749/comment/359b506a_e546457d?usp... : PS8, Line 16:
tab
Done
File src/mainboard/amd/crater/port_descriptors_renoir.c:
https://review.coreboot.org/c/coreboot/+/85749/comment/396596f5_45612cdd?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
will address this in subsequent patch
https://review.coreboot.org/c/coreboot/+/85749/comment/473c85ee_ec940ef2?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. […]
Done
File src/soc/amd/cezanne/chip.h:
https://review.coreboot.org/c/coreboot/+/85749/comment/8c4da447_03e718ab?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 […]
Done
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 […]
Done