Attention is currently required from: Nico Huber, Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Bernardo Perez Priego, Aaron Durbin.
14 comments:
File src/soc/intel/common/basecode/romstage/Kconfig:
Patch Set #20, Line 5: for all
when it's "for all", why have a Kconfig for it?
Patch Set #20, Line 5: Platform
platforms
File src/soc/intel/common/basecode/romstage/romstage.c:
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
nit: drop newline
nit: newline
/*
Common functions implementation
*/
nit: does this comment provide any value? I'd drop it
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?
/* 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`.
/*
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
/*
Initialization functions
*/
nit: same as above
/*
Main romstage function
*/
nit: _entry already provides that information :)
romstage_init();
fsp_memory_init(s3wake);
romstage_post_mem_init();
nit: why all these newlines?
/*
Callback function for FSP memory initialization
*/
nit: function name already provides that information
nit: I'd drop that newline
To view, visit change 37628. To unsubscribe, or for help writing mail filters, visit settings.