Attention is currently required from: Nico Huber, Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Michael Niewöhner, Aaron Durbin. Bernardo Perez Priego 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 21:
(6 comments)
Patchset:
PS20:
Hi Bernardo, I think this is a nice idea. There is a lot of […]
Thanks for the feedback I will address this in upcoming commits.
File src/soc/intel/common/basecode/romstage/Kconfig:
https://review.coreboot.org/c/coreboot/+/37628/comment/14277298_8e9e97f4 PS20, Line 5: Platform
platforms
Ack
https://review.coreboot.org/c/coreboot/+/37628/comment/4607d7e8_bc2b9001 PS20, Line 5: for all
when it's "for all", why have a Kconfig for it?
Ack
File src/soc/intel/common/basecode/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/37628/comment/5fdf1d09_9d4389c5 PS20, Line 27: cmn
I stumbled upon this and only got what it means when looking at the path again, which won't help whe […]
Ack
https://review.coreboot.org/c/coreboot/+/37628/comment/ef9cfe36_20ac6d89 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); : }
Yup, looks FSP2 specific to me on a first look. If the code matches for all FSP2. […]
Thanks for the feedback I will address this in upcoming commits.
https://review.coreboot.org/c/coreboot/+/37628/comment/b5a4ff47_84545607 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, no […]
Ack