Attention is currently required from: Angel Pons, Arthur Heymans, Christian Walter, Jincheng Li, Johnny Lin, Jonathan Zhang, Patrick Rudolph, Tim Chu.
Shuo Liu has posted comments on this change by Shuo Liu. ( https://review.coreboot.org/c/coreboot/+/83326?usp=email )
Change subject: soc/intel/xeon_sp/gnr: Add dimm_slot configuration ......................................................................
Patch Set 4:
(7 comments)
File src/mainboard/intel/avenuecity_crb/config/dimm_slot.c:
https://review.coreboot.org/c/coreboot/+/83326/comment/95c1cf3d_65e53742?usp... : PS4, Line 10: _DIMM_SLOT_CFG_STRUCT(0, 0, 0, "CPU0_DIMM_A1", "BANK 0", "CPU0_DIMM_A1_AssetTag"),
Maybe add a `TODO: add the rest of DIMM slots` comment?
Done
File src/mainboard/intel/avenuecity_crb/ramstage.c:
https://review.coreboot.org/c/coreboot/+/83326/comment/ed16d97e_15141001?usp... : PS4, Line 16: for (int i = 0; i < size; i++) : if (DIMM_SLOT_EQUAL(dimm_slot_config_table[i], : dimm->soc_num, dimm->channel_num, dimm->dimm_num)) { : const char *locator = dimm_slot_config_table[i].dev_locator; : t->device_locator = smbios_add_string(t->eos, locator); : locator = dimm_slot_config_table[i].bank_locator; : t->bank_locator = smbios_add_string(t->eos, locator); : }
I'd use braces: […]
Done
https://review.coreboot.org/c/coreboot/+/83326/comment/16ffa7c7_0155f4a1?usp... : PS4, Line 31: for (int i = 0; i < size; i++) : if (DIMM_SLOT_EQUAL(dimm_slot_config_table[i], : dimm->soc_num, dimm->channel_num, dimm->dimm_num)) { : const char *asset_tag = dimm_slot_config_table[i].asset_tag; : t->asset_tag = smbios_add_string(t->eos, asset_tag); : }
Same here: […]
Done
File src/mainboard/intel/avenuecity_crb/romstage.c:
https://review.coreboot.org/c/coreboot/+/83326/comment/b1f86b4f_bc9975c2?usp... : PS4, Line 50: for (int i = 0; i < size; i++) : if (DIMM_SLOT_EQUAL(dimm_slot_config_table[i], socket, channel, dimm)) : return true;
Same here: […]
Done
File src/mainboard/intel/beechnutcity_crb/config/dimm_slot.c:
https://review.coreboot.org/c/coreboot/+/83326/comment/217e4af3_19e0dd2f?usp... : PS4, Line 10: _DIMM_SLOT_CFG_STRUCT(0, 0, 0, "CPU0_DIMM_A1", "BANK 0", "CPU0_DIMM_A1_AssetTag"),
Maybe add a `TODO: add the rest of DIMM slots` comment?
Done
File src/mainboard/intel/beechnutcity_crb/romstage.c:
https://review.coreboot.org/c/coreboot/+/83326/comment/bb7fc6bc_ca4afebb?usp... : PS4, Line 50: for (int i = 0; i < size; i++) : if (DIMM_SLOT_EQUAL(dimm_slot_config_table[i], socket, channel, dimm)) : return true;
Done
File src/soc/intel/xeon_sp/gnr/include/soc/dimm_slot.h:
https://review.coreboot.org/c/coreboot/+/83326/comment/a3665bd9_0b1a1700?usp... : PS4, Line 21: #define _DIMM_SLOT_CFG_STRUCT(s, c, d, dl, bl, at) {\ : .socket = s,\ : .channel = c,\ : .dimm = d,\ : .dev_locator = dl,\ : .bank_locator = bl,\ : .asset_tag = at\ : }
I don't see why this macro would be needed. These two lines are equivalent: […]
Done