Attention is currently required from: Nico Huber, Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Bernardo Perez Priego, Aaron Durbin. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37628 )
Change subject: soc/intel/common: Add romstage common stage file ......................................................................
Patch Set 20:
(14 comments)
File src/soc/intel/common/basecode/romstage/Kconfig:
https://review.coreboot.org/c/coreboot/+/37628/comment/a748297a_f7846a19 PS20, Line 5: for all when it's "for all", why have a Kconfig for it?
https://review.coreboot.org/c/coreboot/+/37628/comment/2bb0763d_3ec117ef PS20, Line 5: Platform platforms
File src/soc/intel/common/basecode/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/37628/comment/71b4b3bd_97091698 PS20, Line 3: e <acpi/acpi.h> : #include <arch/romstage.h> : #include <intelbasecode/romstage.h> : #include <console/console.h> : #include <intelblocks/cse.h> : #include <intelblocks/pmclib.h> : #include <intelblocks/smbus.h> : #include <soc/iomap.h> : #include <soc/romstage.h> : #include <fsp/util.h> : #include <cbmem.h> : #include <memory_info.h> : #include <soc/intel/common/smbios.h> : #include <soc/pm.h> nit: order
https://review.coreboot.org/c/coreboot/+/37628/comment/b1dfedb7_229b9ecf PS20, Line 18: nit: drop newline
https://review.coreboot.org/c/coreboot/+/37628/comment/49cc45ba_22655a4b PS20, Line 23: } nit: newline
https://review.coreboot.org/c/coreboot/+/37628/comment/2ca593f1_14543d18 PS20, Line 24: /* : Common functions implementation : */ nit: does this comment provide any value? I'd drop it
https://review.coreboot.org/c/coreboot/+/37628/comment/a330b10e_c1aac930 PS20, Line 27: cmn I stumbled upon this and only got what it means when looking at the path again, which won't help when looking at the path where functions may be used. Why not write it out?
https://review.coreboot.org/c/coreboot/+/37628/comment/09663d14_c5154d3c PS20, Line 44: /* Save the DIMM information for SMBIOS table 17 */ : static void save_dimm_info(void) : { : int node, channel, dimm, dimm_max, index; : size_t hob_size; : uint8_t ddr_type; : const CONTROLLER_INFO *ctrlr_info; : const CHANNEL_INFO *channel_info; : const DIMM_INFO *src_dimm; : struct dimm_info *dest_dimm; : struct memory_info *mem_info; : const MEMORY_INFO_DATA_HOB *meminfo_hob; : const uint8_t smbios_memory_info_guid[16] = : FSP_SMBIOS_MEMORY_INFO_GUID; : const uint8_t *serial_num; : const char *dram_part_num = NULL; : size_t dram_part_num_len = 0; : bool is_dram_part_overridden = false; : : /* Locate the memory info HOB, presence validated by raminit */ : meminfo_hob = fsp_find_extension_hob_by_guid( : smbios_memory_info_guid, : &hob_size); : if (meminfo_hob == NULL || hob_size == 0) { : printk(BIOS_ERR, "SMBIOS MEMORY_INFO_DATA_HOB not found\n"); : return; : } : : /* : * Allocate CBMEM area for DIMM information used to populate SMBIOS : * table 17 : */ : mem_info = cbmem_add(CBMEM_ID_MEMINFO, sizeof(*mem_info)); : if (mem_info == NULL) { : printk(BIOS_ERR, "CBMEM entry for DIMM info missing\n"); : return; : } : memset(mem_info, 0, sizeof(*mem_info)); : : /* Allow mainboard to override DRAM part number. */ : dram_part_num = mainboard_get_dram_part_num(); : if (dram_part_num) { : dram_part_num_len = strlen(dram_part_num); : is_dram_part_overridden = true; : } : : /* Save available DIMM information */ : index = 0; : dimm_max = ARRAY_SIZE(mem_info->dimm); : for (node = 0; node < MAX_NODE; node++) { : ctrlr_info = &meminfo_hob->Controller[node]; : for (channel = 0; channel < MAX_CH && index < dimm_max; : channel++) { : channel_info = &ctrlr_info->ChannelInfo[channel]; : if (channel_info->Status != CHANNEL_PRESENT) : continue; : : for (dimm = 0; dimm < MAX_DIMM && index < dimm_max; : dimm++) { : src_dimm = &channel_info->DimmInfo[dimm]; : dest_dimm = &mem_info->dimm[index]; : if (src_dimm->Status != DIMM_PRESENT) : continue; : : switch (meminfo_hob->MemoryType) { : case MRC_DDR_TYPE_DDR4: : ddr_type = MEMORY_TYPE_DDR4; : break; : case MRC_DDR_TYPE_DDR3: : ddr_type = MEMORY_TYPE_DDR3; : break; : case MRC_DDR_TYPE_LPDDR3: : ddr_type = MEMORY_TYPE_LPDDR3; : break; : default: : ddr_type = meminfo_hob->MemoryType; : break; : } : /* If there is no DRAM part number overridden by : * mainboard then use original one. */ : if (!is_dram_part_overridden) { : dram_part_num_len = sizeof(src_dimm->ModulePartNum); : dram_part_num = (const char *) : &src_dimm->ModulePartNum[0]; : } : : u8 memProfNum = meminfo_hob->MemoryProfile; : serial_num = src_dimm->SpdSave + : SPD_SAVE_OFFSET_SERIAL; : : /* Populate the DIMM information */ : dimm_info_fill(dest_dimm, : src_dimm->DimmCapacity, : ddr_type, : meminfo_hob->ConfiguredMemoryClockSpeed, : src_dimm->RankInDimm, : channel_info->ChannelId, : src_dimm->DimmId, : dram_part_num, : dram_part_num_len, : serial_num, : meminfo_hob->DataWidth, : meminfo_hob->VddVoltage[memProfNum], : meminfo_hob->EccSupport, : src_dimm->MfgId, : src_dimm->SpdModuleType); : index++; : } : } : } : mem_info->dimm_cnt = index; : printk(BIOS_DEBUG, "%d DIMMs found\n", mem_info->dimm_cnt); : }
Please do this in a separate commit. If it is done ahead, it would make […]
Yup, looks FSP2 specific to me on a first look. If the code matches for all FSP2.* platforms, I'd move that to `drivers/intel/fsp2_0/memory_init.c`.
https://review.coreboot.org/c/coreboot/+/37628/comment/91627063_087db3bf PS20, Line 169: /* : Weak Functions implementation : */ not sure if that's valid coreboot comment style; also add a newline b/c it's for a whole section, not the function
https://review.coreboot.org/c/coreboot/+/37628/comment/b4de09c6_b814d3ce PS20, Line 209: /* : Initialization functions : */ nit: same as above
https://review.coreboot.org/c/coreboot/+/37628/comment/868628d4_6d02e2fa PS20, Line 227: : /* : Main romstage function : */ nit: _entry already provides that information :)
https://review.coreboot.org/c/coreboot/+/37628/comment/5afa9123_66b32aa0 PS20, Line 235: romstage_init(); : : fsp_memory_init(s3wake); : : romstage_post_mem_init(); nit: why all these newlines?
https://review.coreboot.org/c/coreboot/+/37628/comment/8caff23a_a8e6fa5b PS20, Line 242: /* : Callback function for FSP memory initialization : */ nit: function name already provides that information
https://review.coreboot.org/c/coreboot/+/37628/comment/89b50c01_b37457df PS20, Line 248: nit: I'd drop that newline