Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Michael Niewöhner, Bernardo Perez Priego, Aaron Durbin.
3 comments:
Patchset:
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:
/* 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*`.
Patch Set #20, Line 178: romstage_cmn_pch_init();
Non-empty weak functions are generally discouraged.
To view, visit change 37628. To unsubscribe, or for help writing mail filters, visit settings.