Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Michael Niewöhner, Bernardo Perez Priego, Aaron Durbin. Nico Huber 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:
(3 comments)
Patchset:
PS20: Hi Bernardo, I think this is a nice idea. There is a lot of of unnecessary divergence in the individual SoC code.
We need to be very careful with the design, though. We have to avoid the introduction of any patterns that could make future maintenance harder.
I've looked ahead into the next commit and find it quite hard to assess the impact of the common stage file because there's a lot of not directly related cleanup within the patches. It would be nice if any unrelated refactoring could be done in advance. Only when the changes directly related to the common stage file are clearly visible, we can discuss its merits.
File src/soc/intel/common/basecode/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/37628/comment/20f23eef_9167c977 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 it much easier to assess the impact of the actual topic, the common romstage file. It might be a good idea to put this into its own .c file.
Also, it seems FSP specific. If it is FSP but not chipset specific, it probably belongs into `drivers/intel/fsp*`.
https://review.coreboot.org/c/coreboot/+/37628/comment/7e6fd066_862903f7 PS20, Line 178: romstage_cmn_pch_init(); Non-empty weak functions are generally discouraged.