From 3bbfbd37e14edeef66cc875080624554a5ed8448 Mon Sep 17 00:00:00 2001 From: Angel Pons Date: Wed, 15 Oct 2025 16:52:32 +0200 Subject: [PATCH] nb/intel/haswell: Factor out `setup_sdram_meminfo()` Move the `setup_sdram_meminfo()` function to shared raminit code to deduplicate it as well as to allow native raminit to make use of it, which will be done in a follow-up. When consolidating the functions, the only functional difference is that the Broadwell MRC.bin path reports memory frequencies in MHz whereas the Haswell MRC.bin path reports them in MT/s. Since this data is used to populate SMBIOS tables, which expect memory frequencies in MT/s, using MT/s is the right choice. Given that SPD data is handled differently in the three RAM init implementations (Haswell MRC, Broadwell MRC, native raminit), we have to abstract the SPD data pointers a bit. This is done using an array of pointers. While we're at it, add some TODO comments to note limitations of the code. The idea is to fix those in follow-up commits. Change-Id: I1f81bf18a9e856d80f8e4d7bda65089e999957f6 Signed-off-by: Angel Pons Reviewed-on: https://review.coreboot.org/c/coreboot/+/89598 Reviewed-by: Matt DeVillier Reviewed-by: Nicholas Sudsgaard Tested-by: build bot (Jenkins) --- .../intel/haswell/broadwell_mrc/raminit.c | 102 ++------------- .../intel/haswell/haswell_mrc/raminit.c | 116 +++--------------- src/northbridge/intel/haswell/raminit.h | 4 + .../intel/haswell/raminit_shared.c | 97 +++++++++++++++ 4 files changed, 131 insertions(+), 188 deletions(-) diff --git a/src/northbridge/intel/haswell/broadwell_mrc/raminit.c b/src/northbridge/intel/haswell/broadwell_mrc/raminit.c index 9114c26783..98cfdcf412 100644 --- a/src/northbridge/intel/haswell/broadwell_mrc/raminit.c +++ b/src/northbridge/intel/haswell/broadwell_mrc/raminit.c @@ -155,96 +155,6 @@ static void sdram_initialize(struct pei_data *pei_data) report_memory_config(); } -static uint8_t nb_get_ecc_type(const uint32_t capid0_a) -{ - return capid0_a & CAPID_ECCDIS ? MEMORY_ARRAY_ECC_NONE : MEMORY_ARRAY_ECC_SINGLE_BIT; -} - -static uint16_t nb_slots_per_channel(const uint32_t capid0_a) -{ - return !(capid0_a & CAPID_DDPCD) + 1; -} - -static uint16_t nb_number_of_channels(const uint32_t capid0_a) -{ - return !(capid0_a & CAPID_PDCD) + 1; -} - -static uint32_t nb_max_chan_capacity_mib(const uint32_t capid0_a) -{ - uint32_t ddrsz; - - /* Values from documentation, which assume two DIMMs per channel */ - switch (CAPID_DDRSZ(capid0_a)) { - case 1: - ddrsz = 8192; - break; - case 2: - ddrsz = 2048; - break; - case 3: - ddrsz = 512; - break; - default: - ddrsz = 16384; - break; - } - - /* Account for the maximum number of DIMMs per channel */ - return (ddrsz / 2) * nb_slots_per_channel(capid0_a); -} - -static void setup_sdram_meminfo(struct pei_data *pei_data) -{ - unsigned int dimm_cnt = 0; - - struct memory_info *mem_info = cbmem_add(CBMEM_ID_MEMINFO, sizeof(*mem_info)); - if (!mem_info) - die("Failed to add memory info to CBMEM.\n"); - - memset(mem_info, 0, sizeof(struct memory_info)); - - const u32 ddr_frequency = (mchbar_read32(MC_BIOS_DATA) * 13333 * 2 + 50) / 100; - - for (unsigned int ch = 0; ch < NUM_CHANNELS; ch++) { - const u32 ch_conf = mchbar_read32(MAD_DIMM(ch)); - for (unsigned int slot = 0; slot < NUM_SLOTS; slot++) { - const u32 dimm_size = ((ch_conf >> (slot * 8)) & 0xff) * 256; - if (dimm_size) { - struct dimm_info *dimm = &mem_info->dimm[dimm_cnt]; - dimm->dimm_size = dimm_size; - dimm->ddr_type = MEMORY_TYPE_DDR3; - dimm->ddr_frequency = ddr_frequency; - dimm->rank_per_dimm = 1 + ((ch_conf >> (17 + slot)) & 1); - dimm->channel_num = ch; - dimm->dimm_num = slot; - dimm->bank_locator = ch * 2; - memcpy(dimm->serial, - &pei_data->spd_data[ch][slot][SPD_DDR3_SERIAL_NUM], - SPD_DDR3_SERIAL_LEN); - memcpy(dimm->module_part_number, - &pei_data->spd_data[ch][slot][SPD_DDR3_PART_NUM], - SPD_DDR3_PART_LEN); - dimm->mod_id = - (pei_data->spd_data[ch][slot][SPD_DDR3_MOD_ID2] << 8) | - (pei_data->spd_data[ch][slot][SPD_DDR3_MOD_ID1] & 0xff); - dimm->mod_type = SPD_DDR3_DIMM_TYPE_SO_DIMM; - dimm->bus_width = MEMORY_BUS_WIDTH_64; - dimm_cnt++; - } - } - } - mem_info->dimm_cnt = dimm_cnt; - - const uint32_t capid0_a = pci_read_config32(HOST_BRIDGE, CAPID0_A); - - const uint16_t channels = nb_number_of_channels(capid0_a); - - mem_info->ecc_type = nb_get_ecc_type(capid0_a); - mem_info->max_capacity_mib = channels * nb_max_chan_capacity_mib(capid0_a); - mem_info->number_of_devices = channels * nb_slots_per_channel(capid0_a); -} - #include /* Copy SPD data for on-board memory */ @@ -433,5 +343,15 @@ void perform_raminit(const bool s3resume) if (!s3resume) save_mrc_data(&pei_data); - setup_sdram_meminfo(&pei_data); + const uint8_t *spd_data[NUM_CHANNELS][NUM_SLOTS] = { + [0] = { + [0] = pei_data.spd_data[0][0], + [1] = pei_data.spd_data[0][1], + }, + [1] = { + [0] = pei_data.spd_data[1][0], + [1] = pei_data.spd_data[1][1], + }, + }; + setup_sdram_meminfo(spd_data); } diff --git a/src/northbridge/intel/haswell/haswell_mrc/raminit.c b/src/northbridge/intel/haswell/haswell_mrc/raminit.c index edd9b049d4..7db6a1397f 100644 --- a/src/northbridge/intel/haswell/haswell_mrc/raminit.c +++ b/src/northbridge/intel/haswell/haswell_mrc/raminit.c @@ -4,7 +4,6 @@ #include #include #include -#include #include #include #include @@ -183,101 +182,6 @@ static void sdram_initialize(struct pei_data *pei_data) report_memory_config(); } -static uint8_t nb_get_ecc_type(const uint32_t capid0_a) -{ - return capid0_a & CAPID_ECCDIS ? MEMORY_ARRAY_ECC_NONE : MEMORY_ARRAY_ECC_SINGLE_BIT; -} - -static uint16_t nb_slots_per_channel(const uint32_t capid0_a) -{ - return !(capid0_a & CAPID_DDPCD) + 1; -} - -static uint16_t nb_number_of_channels(const uint32_t capid0_a) -{ - return !(capid0_a & CAPID_PDCD) + 1; -} - -static uint32_t nb_max_chan_capacity_mib(const uint32_t capid0_a) -{ - uint32_t ddrsz; - - /* Values from documentation, which assume two DIMMs per channel */ - switch (CAPID_DDRSZ(capid0_a)) { - case 1: - ddrsz = 8192; - break; - case 2: - ddrsz = 2048; - break; - case 3: - ddrsz = 512; - break; - default: - ddrsz = 16384; - break; - } - - /* Account for the maximum number of DIMMs per channel */ - return (ddrsz / 2) * nb_slots_per_channel(capid0_a); -} - -static void setup_sdram_meminfo(struct pei_data *pei_data) -{ - struct memory_info *mem_info; - struct dimm_info *dimm; - int ch, d_num; - int dimm_cnt = 0; - - mem_info = cbmem_add(CBMEM_ID_MEMINFO, sizeof(struct memory_info)); - if (!mem_info) - die("Failed to add memory info to CBMEM.\n"); - - memset(mem_info, 0, sizeof(struct memory_info)); - - const u32 ddr_freq_mhz = (mchbar_read32(MC_BIOS_DATA) * 13333 * 2 + 50) / 100; - - for (ch = 0; ch < NUM_CHANNELS; ch++) { - const u32 ch_conf = mchbar_read32(MAD_DIMM(ch)); - /* DIMMs A/B */ - for (d_num = 0; d_num < NUM_SLOTS; d_num++) { - const u32 dimm_size = ((ch_conf >> (d_num * 8)) & 0xff) * 256; - if (dimm_size) { - const int index = ch * NUM_SLOTS + d_num; - dimm = &mem_info->dimm[dimm_cnt]; - dimm->dimm_size = dimm_size; - dimm->ddr_type = MEMORY_TYPE_DDR3; - dimm->ddr_frequency = ddr_freq_mhz * 2; /* In MT/s */ - dimm->rank_per_dimm = 1 + ((ch_conf >> (17 + d_num)) & 1); - dimm->channel_num = ch; - dimm->dimm_num = d_num; - dimm->bank_locator = ch * 2; - memcpy(dimm->serial, - &pei_data->spd_data[index][SPD_DDR3_SERIAL_NUM], - SPD_DDR3_SERIAL_LEN); - memcpy(dimm->module_part_number, - &pei_data->spd_data[index][SPD_DDR3_PART_NUM], - SPD_DDR3_PART_LEN); - dimm->mod_id = - (pei_data->spd_data[index][SPD_DDR3_MOD_ID2] << 8) | - (pei_data->spd_data[index][SPD_DDR3_MOD_ID1] & 0xff); - dimm->mod_type = SPD_DDR3_DIMM_TYPE_SO_DIMM; - dimm->bus_width = MEMORY_BUS_WIDTH_64; - dimm_cnt++; - } - } - } - mem_info->dimm_cnt = dimm_cnt; - - const uint32_t capid0_a = pci_read_config32(HOST_BRIDGE, CAPID0_A); - - const uint16_t channels = nb_number_of_channels(capid0_a); - - mem_info->ecc_type = nb_get_ecc_type(capid0_a); - mem_info->max_capacity_mib = channels * nb_max_chan_capacity_mib(capid0_a); - mem_info->number_of_devices = channels * nb_slots_per_channel(capid0_a); -} - /* Copy SPD data for on-board memory */ static void copy_spd(struct pei_data *pei_data, struct spd_info *spdi) { @@ -428,5 +332,23 @@ void perform_raminit(const bool s3resume) if (!s3resume) save_mrc_data(&pei_data); - setup_sdram_meminfo(&pei_data); + /* + * TODO: `setup_sdram_info()` uses SPD data to fill in various fields. However, + * even though Haswell MRC reads SPD data over SMBus, it does not pass the data + * back to coreboot. So, we currently only have SPD data for memory-down slots. + * + * We have to read the SPD data over SMBus again for coreboot to use it. But we + * can be smart and only read the bytes that `setup_sdram_meminfo()` needs. + */ + const uint8_t *spd_data[NUM_CHANNELS][NUM_SLOTS] = { + [0] = { + [0] = pei_data.spd_data[0], + [1] = pei_data.spd_data[1], + }, + [1] = { + [0] = pei_data.spd_data[2], + [1] = pei_data.spd_data[3], + }, + }; + setup_sdram_meminfo(spd_data); } diff --git a/src/northbridge/intel/haswell/raminit.h b/src/northbridge/intel/haswell/raminit.h index 3f13a03db5..2b80c24aba 100644 --- a/src/northbridge/intel/haswell/raminit.h +++ b/src/northbridge/intel/haswell/raminit.h @@ -4,7 +4,9 @@ #define NORTHBRIDGE_INTEL_HASWELL_RAMINIT_H #include + #include "chip.h" +#include "haswell.h" #define SPD_MEMORY_DOWN 0xff @@ -17,6 +19,8 @@ struct spd_info { void mb_get_spd_map(struct spd_info *spdi); void get_spd_info(struct spd_info *spdi, const struct northbridge_intel_haswell_config *cfg); +void setup_sdram_meminfo(const uint8_t *spd_data[NUM_CHANNELS][NUM_SLOTS]); + void perform_raminit(const bool s3resume); #endif /* NORTHBRIDGE_INTEL_HASWELL_RAMINIT_H */ diff --git a/src/northbridge/intel/haswell/raminit_shared.c b/src/northbridge/intel/haswell/raminit_shared.c index 90fe1145cc..7a87359b63 100644 --- a/src/northbridge/intel/haswell/raminit_shared.c +++ b/src/northbridge/intel/haswell/raminit_shared.c @@ -1,7 +1,13 @@ /* SPDX-License-Identifier: GPL-2.0-only */ +#include +#include +#include #include +#include + #include "chip.h" +#include "haswell.h" #include "raminit.h" void get_spd_info(struct spd_info *spdi, const struct northbridge_intel_haswell_config *cfg) @@ -14,3 +20,94 @@ void get_spd_info(struct spd_info *spdi, const struct northbridge_intel_haswell_ memcpy(spdi->addresses, cfg->spd_addresses, ARRAY_SIZE(spdi->addresses)); } } + +static uint8_t nb_get_ecc_type(const uint32_t capid0_a) +{ + return capid0_a & CAPID_ECCDIS ? MEMORY_ARRAY_ECC_NONE : MEMORY_ARRAY_ECC_SINGLE_BIT; +} + +static uint16_t nb_slots_per_channel(const uint32_t capid0_a) +{ + return !(capid0_a & CAPID_DDPCD) + 1; +} + +static uint16_t nb_number_of_channels(const uint32_t capid0_a) +{ + return !(capid0_a & CAPID_PDCD) + 1; +} + +static uint32_t nb_max_chan_capacity_mib(const uint32_t capid0_a) +{ + uint32_t ddrsz; + + /* Values from documentation, which assume two DIMMs per channel */ + switch (CAPID_DDRSZ(capid0_a)) { + case 1: + ddrsz = 8192; + break; + case 2: + ddrsz = 2048; + break; + case 3: + ddrsz = 512; + break; + default: + ddrsz = 16384; + break; + } + + /* Account for the maximum number of DIMMs per channel */ + return (ddrsz / 2) * nb_slots_per_channel(capid0_a); +} + +void setup_sdram_meminfo(const uint8_t *spd_data[NUM_CHANNELS][NUM_SLOTS]) +{ + struct memory_info *mem_info = cbmem_add(CBMEM_ID_MEMINFO, sizeof(*mem_info)); + if (!mem_info) + die("Failed to add memory info to CBMEM.\n"); + + memset(mem_info, 0, sizeof(struct memory_info)); + + /* TODO: This looks like an open-coded DIV_ROUND_CLOSEST() */ + const uint32_t ddr_freq_mhz = (mchbar_read32(MC_BIOS_DATA) * 13333 * 2 + 50) / 100; + + unsigned int dimm_cnt = 0; + for (unsigned int channel = 0; channel < NUM_CHANNELS; channel++) { + const uint32_t ch_conf = mchbar_read32(MAD_DIMM(channel)); + for (unsigned int slot = 0; slot < NUM_SLOTS; slot++) { + const uint32_t dimm_size = ((ch_conf >> (slot * 8)) & 0xff) * 256; + if (dimm_size) { + struct dimm_info *dimm = &mem_info->dimm[dimm_cnt]; + dimm->dimm_size = dimm_size; + /* TODO: Hardcoded, could also be LPDDR3 */ + dimm->ddr_type = MEMORY_TYPE_DDR3; + dimm->ddr_frequency = ddr_freq_mhz * 2; /* In MT/s */ + dimm->rank_per_dimm = 1 + ((ch_conf >> (17 + slot)) & 1); + dimm->channel_num = channel; + dimm->dimm_num = slot; + dimm->bank_locator = channel * 2; + memcpy(dimm->serial, + &spd_data[channel][slot][SPD_DDR3_SERIAL_NUM], + SPD_DDR3_SERIAL_LEN); + memcpy(dimm->module_part_number, + &spd_data[channel][slot][SPD_DDR3_PART_NUM], + SPD_DDR3_PART_LEN); + dimm->mod_id = + (spd_data[channel][slot][SPD_DDR3_MOD_ID2] << 8) | + (spd_data[channel][slot][SPD_DDR3_MOD_ID1] & 0xff); + /* TODO: Should be taken from SPD instead */ + dimm->mod_type = SPD_DDR3_DIMM_TYPE_SO_DIMM; + dimm->bus_width = MEMORY_BUS_WIDTH_64; + dimm_cnt++; + } + } + } + mem_info->dimm_cnt = dimm_cnt; + + const uint32_t capid0_a = pci_read_config32(HOST_BRIDGE, CAPID0_A); + const uint16_t num_channels = nb_number_of_channels(capid0_a); + + mem_info->ecc_type = nb_get_ecc_type(capid0_a); + mem_info->max_capacity_mib = num_channels * nb_max_chan_capacity_mib(capid0_a); + mem_info->number_of_devices = num_channels * nb_slots_per_channel(capid0_a); +}