Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48860 )
Change subject: soc/intel/alderlake: Refactor meminit code ......................................................................
soc/intel/alderlake: Refactor meminit code
Align ADL's memory initialization code with TGL; this allows mainboards to e.g. more easily align the CPU<->DRAM DQ pin mapping between the data structure and the schematic.
BUG=b:172978729 TEST=abuild intel/adlrvp
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: I2b75e856f1f2aa34bb7a91913147faf3037e7cfb --- M src/mainboard/intel/adlrvp/include/baseboard/variants.h M src/mainboard/intel/adlrvp/memory.c M src/mainboard/intel/adlrvp/romstage_fsp_params.c M src/soc/intel/alderlake/include/soc/meminit.h M src/soc/intel/alderlake/meminit.c 5 files changed, 501 insertions(+), 269 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/48860/1
diff --git a/src/mainboard/intel/adlrvp/include/baseboard/variants.h b/src/mainboard/intel/adlrvp/include/baseboard/variants.h index 9cb8640..9efa67e 100644 --- a/src/mainboard/intel/adlrvp/include/baseboard/variants.h +++ b/src/mainboard/intel/adlrvp/include/baseboard/variants.h @@ -29,5 +29,5 @@ void variant_configure_early_gpio_pads(void);
size_t variant_memory_sku(void); -const struct mb_cfg *variant_memory_params(void); +const struct adl_mem_config *variant_memory_params(void); #endif /*__BASEBOARD_VARIANTS_H__ */ diff --git a/src/mainboard/intel/adlrvp/memory.c b/src/mainboard/intel/adlrvp/memory.c index 80ec14a..2cfcac4 100644 --- a/src/mainboard/intel/adlrvp/memory.c +++ b/src/mainboard/intel/adlrvp/memory.c @@ -5,7 +5,7 @@ #include <baseboard/variants.h> #include <soc/romstage.h>
-static const struct mb_cfg ddr4_mem_config = { +static const struct ddr_mem_config ddr4_params = { /* Baseboard uses only 100ohm Rcomp resistors */ .rcomp_resistor = {100, 100, 100},
@@ -15,64 +15,97 @@ .dq_pins_interleaved = false,
.ect = true, /* Early Command Training */ - - .UserBd = BOARD_TYPE_MOBILE, +}; +static const struct adl_mem_config ddr4_mem_config = { + .mem_type = MEMTYPE_DDR4, + .ddr_cfg = &ddr4_params, };
-static const struct mb_cfg lpddr4_mem_config = { +static const struct lpx_common_mem_config lpddr4_params = { /* DQ byte map */ .dq_map = { - { 0, 2, 3, 1, 6, 7, 5, 4, 10, 8, 11, 9, 14, 12, 13, 15 }, - { 12, 8, 14, 10, 11, 13, 15, 9, 5, 0, 7, 3, 6, 2, 1, 4 }, - { 3, 0, 2, 1, 6, 5, 4, 7, 12, 13, 14, 15, 10, 9, 8, 11 }, - { 2, 6, 7, 1, 3, 4, 0, 5, 9, 13, 8, 15, 14, 11, 12, 10 }, - { 3, 0, 1, 2, 7, 4, 6, 5, 10, 8, 11, 9, 14, 13, 12, 15 }, - { 10, 12, 14, 8, 9, 13, 15, 11, 3, 7, 6, 2, 0, 4, 5, 1 }, - { 12, 15, 14, 13, 9, 10, 11, 8, 7, 4, 6, 5, 0, 1, 3, 2 }, - { 0, 2, 4, 3, 1, 6, 7, 5, 13, 9, 10, 11, 8, 12, 14, 15 }, + [0] = { { 0, 2, 3, 1, 6, 7, 5, 4 }, + { 10, 8, 11, 9, 14, 12, 13, 15 }, }, + [1] = { { 12, 8, 14, 10, 11, 13, 15, 9 }, + { 5, 0, 7, 3, 6, 2, 1, 4 }, }, + [2] = { { 3, 0, 2, 1, 6, 5, 4, 7 }, + { 12, 13, 14, 15, 10, 9, 8, 11 }, }, + [3] = { { 2, 6, 7, 1, 3, 4, 0, 5 }, + { 9, 13, 8, 15, 14, 11, 12, 10 }, }, + [4] = { { 3, 0, 1, 2, 7, 4, 6, 5 }, + { 10, 8, 11, 9, 14, 13, 12, 15 }, }, + [5] = { { 10, 12, 14, 8, 9, 13, 15, 11 }, + { 3, 7, 6, 2, 0, 4, 5, 1 }, }, + [6] = { { 12, 15, 14, 13, 9, 10, 11, 8 }, + { 7, 4, 6, 5, 0, 1, 3, 2 }, }, + [7] = { { 0, 2, 4, 3, 1, 6, 7, 5 }, + { 13, 9, 10, 11, 8, 12, 14, 15 }, }, },
/* DQS CPU<>DRAM map */ .dqs_map = { - { 0, 1 }, { 1, 0 }, { 0, 1 }, { 0, 1 }, { 0, 1 }, { 1, 0 }, { 1, 0 }, { 0, 1 } + [0] = { 0, 1 }, + [1] = { 1, 0 }, + [2] = { 0, 1 }, + [3] = { 0, 1 }, + [4] = { 0, 1 }, + [5] = { 1, 0 }, + [6] = { 1, 0 }, + [7] = { 0, 1 }, },
- .dq_pins_interleaved = false, - .ect = true, /* Early Command Training */ - - .UserBd = BOARD_TYPE_MOBILE, +}; +static const struct adl_mem_config lpddr4_mem_config = { + .mem_type = MEMTYPE_LP4X, + .lp4x_cfg = &lpddr4_params, };
-static const struct mb_cfg lp5_mem_config = { - +static const struct lpx_common_mem_config lpddr5_params = { /* DQ byte map */ .dq_map = { - { 3, 2, 1, 0, 5, 4, 6, 7, 15, 14, 12, 13, 8, 9, 10, 11 }, - { 0, 2, 3, 1, 5, 7, 4, 6, 14, 13, 15, 12, 8, 9, 11, 10 }, - { 1, 2, 0, 3, 4, 6, 5, 7, 15, 13, 12, 14, 9, 10, 8, 11 }, - { 2, 1, 3, 0, 7, 4, 5, 6, 13, 12, 15, 14, 9, 11, 8, 10 }, - { 1, 2, 3, 0, 6, 4, 5, 7, 15, 13, 14, 12, 10, 9, 8, 11 }, - { 1, 0, 3, 2, 6, 7, 4, 5, 14, 12, 15, 13, 8, 9, 10, 11 }, - { 0, 2, 1, 3, 4, 7, 5, 6, 12, 13, 15, 14, 9, 11, 10, 8 }, - { 3, 2, 1, 0, 5, 4, 6, 7, 13, 15, 11, 12, 10, 9, 14, 8 }, + [0] = { { 3, 2, 1, 0, 5, 4, 6, 7 }, + { 15, 14, 12, 13, 8, 9, 10, 11 }, }, + [1] = { { 0, 2, 3, 1, 5, 7, 4, 6 }, + { 14, 13, 15, 12, 8, 9, 11, 10 }, }, + [2] = { { 1, 2, 0, 3, 4, 6, 5, 7 }, + { 15, 13, 12, 14, 9, 10, 8, 11 }, }, + [3] = { { 2, 1, 3, 0, 7, 4, 5, 6 }, + { 13, 12, 15, 14, 9, 11, 8, 10 }, }, + [4] = { { 1, 2, 3, 0, 6, 4, 5, 7 }, + { 15, 13, 14, 12, 10, 9, 8, 11 }, }, + [5] = { { 1, 0, 3, 2, 6, 7, 4, 5 }, + { 14, 12, 15, 13, 8, 9, 10, 11 }, }, + [6] = { { 0, 2, 1, 3, 4, 7, 5, 6 }, + { 12, 13, 15, 14, 9, 11, 10, 8 }, }, + [7] = { { 3, 2, 1, 0, 5, 4, 6, 7 }, + { 13, 15, 11, 12, 10, 9, 14, 8 }, }, },
/* DQS CPU<>DRAM map */ .dqs_map = { - { 0, 1 }, { 0, 1 }, { 0, 1 }, { 0, 1 }, { 0, 1 }, { 0, 1 }, { 0, 1 }, { 0, 1 } + [0] = { 0, 1 }, + [1] = { 0, 1 }, + [2] = { 0, 1 }, + [3] = { 0, 1 }, + [4] = { 0, 1 }, + [5] = { 0, 1 }, + [6] = { 0, 1 }, + [7] = { 0, 1 }, },
- .dq_pins_interleaved = false, - .ect = false, /* Early Command Training */ - +}; +static const struct lp5x_mem_config lpddr5_config = { + .lpx_config = &lpddr5_params, .lp5_ccc_config = 0xff, - - .UserBd = BOARD_TYPE_MOBILE, +}; +static const struct adl_mem_config lpddr5_mem_config = { + .mem_type = MEMTYPE_LP5X, + .lp5x_cfg = &lpddr5_config, };
-static const struct mb_cfg ddr5_mem_config = { +static const struct ddr_mem_config ddr5_params = { /* Baseboard uses only 100ohm Rcomp resistors */ .rcomp_resistor = {100, 100, 100},
@@ -82,11 +115,13 @@ .dq_pins_interleaved = false,
.ect = true, /* Early Command Training */ - - .UserBd = BOARD_TYPE_MOBILE, +}; +static const struct adl_mem_config ddr5_mem_config = { + .mem_type = MEMTYPE_DDR5, + .ddr_cfg = &ddr5_params, };
-const struct mb_cfg *variant_memory_params(void) +const struct adl_mem_config *variant_memory_params(void) { int board_id = get_board_id();
@@ -100,7 +135,7 @@ case ADL_P_DDR5: return &ddr5_mem_config; case ADL_P_LP5: - return &lp5_mem_config; + return &lpddr5_mem_config; default: die("unsupported board id : 0x%x\n", board_id); } diff --git a/src/mainboard/intel/adlrvp/romstage_fsp_params.c b/src/mainboard/intel/adlrvp/romstage_fsp_params.c index 2f03cb4..da7103f 100644 --- a/src/mainboard/intel/adlrvp/romstage_fsp_params.c +++ b/src/mainboard/intel/adlrvp/romstage_fsp_params.c @@ -27,30 +27,29 @@
void mainboard_memory_init_params(FSPM_UPD *mupd) { - const struct mb_cfg *mem_config = variant_memory_params(); + const struct adl_mem_config *mem_config = variant_memory_params(); int board_id = get_board_id(); const bool half_populated = false;
const struct spd_info lp4_lp5_spd_info = { - .read_type = READ_SPD_CBFS, - .spd_spec.spd_index = get_spd_index(), + .topology = MEMORY_DOWN, + .md_spd_loc = SPD_CBFS, + .cbfs_index = get_spd_index(), };
const struct spd_info ddr4_ddr5_spd_info = { - .read_type = READ_SMBUS, - .spd_spec = { - .spd_smbus_address = { - [0] = 0xa0, - [1] = 0xa2, - [8] = 0xa4, - [9] = 0xa6, - }, + .topology = SODIMM, + .smbus_info = { + [0] = { .addr_dimm0 = 0xa0, .addr_dimm1 = 0xa2, }, + [1] = { .addr_dimm0 = 0xa4, .addr_dimm1 = 0xa6, }, }, };
switch (board_id) { case ADL_P_DDR4_1: case ADL_P_DDR4_2: + memcfg_init(&mupd->FspmConfig, mem_config, &ddr4_ddr5_spd_info, half_populated); + break; case ADL_P_DDR5: memcfg_init(&mupd->FspmConfig, mem_config, &ddr4_ddr5_spd_info, half_populated); break; diff --git a/src/soc/intel/alderlake/include/soc/meminit.h b/src/soc/intel/alderlake/include/soc/meminit.h index 36d0750..1e70d5c 100644 --- a/src/soc/intel/alderlake/include/soc/meminit.h +++ b/src/soc/intel/alderlake/include/soc/meminit.h @@ -7,65 +7,65 @@ #include <stdint.h> #include <fsp/soc_binding.h>
-#define BYTES_PER_CHANNEL 2 -#define BITS_PER_BYTE 8 -#define DQS_PER_CHANNEL 2 +#define BITS_PER_BYTE 8
-/* 64-bit Channel identification */ -enum { - DDR_CH0, - DDR_CH1, - DDR_CH2, - DDR_CH3, - DDR_CH4, - DDR_CH5, - DDR_CH6, - DDR_CH7, - DDR_NUM_CHANNELS -}; -/* Number of memory DIMM slots available on Alderlake board */ -#define NUM_DIMM_SLOT 16 +/* True for both LP4X and LP5X */ +#define LPX_CHANNELS 8 +#define LPX_BYTES_PER_CHANNEL 2
-struct spd_by_pointer { - size_t spd_data_len; - uintptr_t spd_data_ptr; +/* True for both DDR4 and DDR5 */ +#define DDR_CHANNELS 2 +#define DDR_BYTES_PER_CHANNEL 8 + +enum mem_topology { + MEMORY_DOWN, /* SPD sourced from CBFS or in-memory pointer */ + SODIMM, /* DDR4 supports reading SPD over SMBus */ + MIXED, /* DDR4 supports CH0 as SODIMM and CH1 as Memory Down */ };
-enum mem_info_read_type { - NOT_EXISTING, /* No memory in this slot */ - READ_SMBUS, /* Read on-module spd by SMBUS. */ - READ_SPD_CBFS, /* Find SPD file in CBFS. */ - READ_SPD_MEMPTR /* Find SPD data from pointer. */ +enum ddr_memtype { + MEMTYPE_DDR4, + MEMTYPE_LP4X, + MEMTYPE_DDR5, + MEMTYPE_LP5X, +}; + +/* Memory-down SPD location */ +enum md_spd_loc { + SPD_MEMPTR, + SPD_CBFS, };
struct spd_info { - enum mem_info_read_type read_type; - union spd_data_by { - /* To read on-module SPD when read_type is READ_SMBUS. */ - uint8_t spd_smbus_address[NUM_DIMM_SLOT]; - - /* To identify SPD file when read_type is READ_SPD_CBFS. */ - int spd_index; - - /* To find SPD data when read_type is READ_SPD_MEMPTR. */ - struct spd_by_pointer spd_data_ptr_info; - } spd_spec; -}; - -/* Board-specific memory configuration information */ -struct mb_cfg { - /* DQ mapping */ - uint8_t dq_map[DDR_NUM_CHANNELS][BYTES_PER_CHANNEL * BITS_PER_BYTE]; + enum mem_topology topology; + enum md_spd_loc md_spd_loc; + union { + /* Used for SPD_CBFS */ + uint8_t cbfs_index; + /* Used for SPD_MEMPTR */ + struct { + uintptr_t data_ptr; + size_t data_len; + }; + };
/* - * DQS CPU<>DRAM map. Each array entry represents a - * mapping of a dq bit on the CPU to the bit it's connected to on - * the memory part. The array index represents the dqs bit number - * on the memory part, and the values in the array represent which - * pin on the CPU that DRAM pin connects to. + * SPD info for SODIMM topology. + * Leave addr_dimmN as 0 for any DIMMs that are not populated. */ - uint8_t dqs_map[DDR_NUM_CHANNELS][DQS_PER_CHANNEL]; + struct { + /* SMBus address for DIMM0 within the channel. */ + uint8_t addr_dimm0; + /* SMBus address for DIMM1 within the channel. */ + uint8_t addr_dimm1; + } smbus_info[DDR_CHANNELS]; +};
+struct ddr_mem_config { + uint8_t dq_map[DDR_CHANNELS][DDR_BYTES_PER_CHANNEL][BITS_PER_BYTE]; + uint8_t dqs_map[DDR_CHANNELS][DDR_BYTES_PER_CHANNEL]; + uint8_t dq_pins_interleaved; + uint8_t ect; /* * Rcomp resistor values. These values represent the resistance in * ohms of the three rcomp resistors attached to the DDR_COMP_0, @@ -75,36 +75,29 @@
/* Rcomp target values. */ uint16_t rcomp_targets[5]; +};
- /* - * Dqs Pins Interleaved Setting. Enable/Disable Control - * TRUE = enable, FALSE = disable - */ - bool dq_pins_interleaved; +struct lpx_common_mem_config { + uint8_t dq_map[LPX_CHANNELS][LPX_BYTES_PER_CHANNEL][BITS_PER_BYTE]; + uint8_t dqs_map[LPX_CHANNELS][LPX_BYTES_PER_CHANNEL]; + uint8_t ect; +};
- /* - * Early Command Training Enable/Disable Control - * TRUE = enable, FALSE = disable - */ - bool ect; - - /* Board type */ - uint8_t UserBd; - - /* - * Command pins mapping for Controller Channel (ccc) - * lp5_ccc_config: Bitmask where bits [3:0] are Controller 0 Channel [3:0] and - * bits [7:4] are Controller 1 Channel [3:0] - * Bit value: 0 = ccc pin mapping is ascending, 1 = ccc pin mapping is descending. - */ +struct lp5x_mem_config { + const struct lpx_common_mem_config *lpx_config; uint8_t lp5_ccc_config; };
-/* - * Initialize default memory configurations for Alder Lake. - */ +struct adl_mem_config { + enum ddr_memtype mem_type; + union { + const struct ddr_mem_config *ddr_cfg; + const struct lpx_common_mem_config *lp4x_cfg; + const struct lp5x_mem_config *lp5x_cfg; + }; +};
-void memcfg_init(FSP_M_CONFIG *mem_cfg, const struct mb_cfg *board_cfg, - const struct spd_info *spd_info, bool half_populated); +void memcfg_init(FSP_M_CONFIG *mem_cfg, const struct adl_mem_config *board_cfg, + const struct spd_info *info, bool half_populated);
#endif /* _SOC_ALDERLAKE_MEMINIT_H_ */ diff --git a/src/soc/intel/alderlake/meminit.c b/src/soc/intel/alderlake/meminit.c index 8473ad8..d93469f 100644 --- a/src/soc/intel/alderlake/meminit.c +++ b/src/soc/intel/alderlake/meminit.c @@ -7,6 +7,14 @@ #include <spd_bin.h> #include <string.h>
+/* + * Translate DDR4/5 channel to FSP UPD index for the channel. + * Channel 0 -> Index 0 + * Channel 1 -> Index 4 + * All others indexes are unused. + */ +#define DDR_FSP_UPD_CHANNEL_IDX(x) ((x) * 4) + enum dimm_enable_options { ENABLE_BOTH_DIMMS = 0, DISABLE_DIMM0 = 1, @@ -14,172 +22,369 @@ DISABLE_BOTH_DIMMS = 3 };
-static void spd_read_from_cbfs(const struct spd_info *spd_info, - uintptr_t *spd_data_ptr, size_t *spd_data_len) +static enum dimm_enable_options get_dimm_cfg(uintptr_t dimm0, uintptr_t dimm1) +{ + if (dimm0 && dimm1) + return ENABLE_BOTH_DIMMS; + if (!dimm0 && !dimm1) + return DISABLE_BOTH_DIMMS; + if (!dimm1) + return DISABLE_DIMM1; + if (!dimm0) + die("Disabling only dimm0 is not supported!\n"); + + return DISABLE_BOTH_DIMMS; +} + +static void init_spd_upds(FSP_M_CONFIG *mem_cfg, int channel, uintptr_t spd_dimm0, + uintptr_t spd_dimm1) +{ + const enum dimm_enable_options dimm_cfg = get_dimm_cfg(spd_dimm0, spd_dimm1); + switch (channel) { + case 0: + mem_cfg->DisableDimmMc0Ch0 = (uint8_t)dimm_cfg; + mem_cfg->MemorySpdPtr00 = spd_dimm0; + mem_cfg->MemorySpdPtr01 = spd_dimm1; + break; + case 1: + mem_cfg->DisableDimmMc0Ch1 = (uint8_t)dimm_cfg; + mem_cfg->MemorySpdPtr02 = spd_dimm0; + mem_cfg->MemorySpdPtr03 = spd_dimm1; + break; + case 2: + mem_cfg->DisableDimmMc0Ch2 = (uint8_t)dimm_cfg; + mem_cfg->MemorySpdPtr04 = spd_dimm0; + mem_cfg->MemorySpdPtr05 = spd_dimm1; + break; + case 3: + mem_cfg->DisableDimmMc0Ch3 = (uint8_t)dimm_cfg; + mem_cfg->MemorySpdPtr06 = spd_dimm0; + mem_cfg->MemorySpdPtr07 = spd_dimm1; + break; + case 4: + mem_cfg->DisableDimmMc1Ch0 = (uint8_t)dimm_cfg; + mem_cfg->MemorySpdPtr08 = spd_dimm0; + mem_cfg->MemorySpdPtr09 = spd_dimm1; + break; + case 5: + mem_cfg->DisableDimmMc1Ch1 = (uint8_t)dimm_cfg; + mem_cfg->MemorySpdPtr10 = spd_dimm0; + mem_cfg->MemorySpdPtr11 = spd_dimm1; + break; + case 6: + mem_cfg->DisableDimmMc1Ch2 = (uint8_t)dimm_cfg; + mem_cfg->MemorySpdPtr12 = spd_dimm0; + mem_cfg->MemorySpdPtr13 = spd_dimm1; + break; + case 7: + mem_cfg->DisableDimmMc1Ch3 = (uint8_t)dimm_cfg; + mem_cfg->MemorySpdPtr14 = spd_dimm0; + mem_cfg->MemorySpdPtr15 = spd_dimm1; + break; + default: + die("Invalid channel %d\n", channel); + break; + } +} + +static void init_spd_upds_empty(FSP_M_CONFIG *mem_cfg, int channel) +{ + init_spd_upds(mem_cfg, channel, 0, 0); +} + +static void init_spd_upds_dimm0(FSP_M_CONFIG *mem_cfg, int channel, uintptr_t spd_dimm0) +{ + init_spd_upds(mem_cfg, channel, spd_dimm0, 0); +} + +static void init_dq_upds(FSP_M_CONFIG *mem_cfg, int byte_pair, const uint8_t *dq_byte0, + const uint8_t *dq_byte1) +{ + uint8_t *dq_upd; + + switch (byte_pair) { + case 0: + dq_upd = mem_cfg->DqMapCpu2DramCh0; + break; + case 1: + dq_upd = mem_cfg->DqMapCpu2DramCh1; + break; + case 2: + dq_upd = mem_cfg->DqMapCpu2DramCh2; + break; + case 3: + dq_upd = mem_cfg->DqMapCpu2DramCh3; + break; + case 4: + dq_upd = mem_cfg->DqMapCpu2DramCh4; + break; + case 5: + dq_upd = mem_cfg->DqMapCpu2DramCh5; + break; + case 6: + dq_upd = mem_cfg->DqMapCpu2DramCh6; + break; + case 7: + dq_upd = mem_cfg->DqMapCpu2DramCh7; + break; + default: + die("Invalid byte_pair: %d\n", byte_pair); + } + + if (dq_byte0 && dq_byte1) { + memcpy(dq_upd, dq_byte0, BITS_PER_BYTE); + memcpy(dq_upd + BITS_PER_BYTE, dq_byte1, BITS_PER_BYTE); + } else { + memset(dq_upd, 0, BITS_PER_BYTE * 2); + } +} + +static void init_dq_upds_empty(FSP_M_CONFIG *mem_cfg, int byte_pair) +{ + init_dq_upds(mem_cfg, byte_pair, NULL, NULL); +} + +static void init_dqs_upds(FSP_M_CONFIG *mem_cfg, int byte_pair, uint8_t dqs_byte0, + uint8_t dqs_byte1) +{ + uint8_t *dqs_upd; + + switch (byte_pair) { + case 0: + dqs_upd = mem_cfg->DqsMapCpu2DramCh0; + break; + case 1: + dqs_upd = mem_cfg->DqsMapCpu2DramCh1; + break; + case 2: + dqs_upd = mem_cfg->DqsMapCpu2DramCh2; + break; + case 3: + dqs_upd = mem_cfg->DqsMapCpu2DramCh3; + break; + case 4: + dqs_upd = mem_cfg->DqsMapCpu2DramCh4; + break; + case 5: + dqs_upd = mem_cfg->DqsMapCpu2DramCh5; + break; + case 6: + dqs_upd = mem_cfg->DqsMapCpu2DramCh6; + break; + case 7: + dqs_upd = mem_cfg->DqsMapCpu2DramCh7; + break; + default: + die("Invalid byte pair %d\n", byte_pair); + } + + dqs_upd[0] = dqs_byte0; + dqs_upd[1] = dqs_byte1; +} + +static void init_dqs_upds_empty(FSP_M_CONFIG *mem_cfg, int byte_pair) +{ + init_dqs_upds(mem_cfg, byte_pair, 0, 0); +} + +static void read_spd_from_cbfs(uint8_t index, uintptr_t *data, size_t *len) { struct region_device spd_rdev; - size_t spd_index = spd_info->spd_spec.spd_index;
- printk(BIOS_DEBUG, "SPD INDEX = %lu\n", spd_index); - if (get_spd_cbfs_rdev(&spd_rdev, spd_index) < 0) + printk(BIOS_DEBUG, "SPD index = %u\n", index); + if (get_spd_cbfs_rdev(&spd_rdev, index) < 0) die("spd.bin not found or incorrect index\n");
- *spd_data_len = region_device_sz(&spd_rdev); - /* Memory leak is ok since we have memory mapped boot media */ assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED));
- *spd_data_ptr = (uintptr_t)rdev_mmap_full(&spd_rdev); + *len = region_device_sz(&spd_rdev); + *data = (uintptr_t)rdev_mmap_full(&spd_rdev); }
-static void get_spd_data(const struct spd_info *spd_info, - uintptr_t *spd_data_ptr, size_t *spd_data_len) +static void read_md_spd(const struct spd_info *info, uintptr_t *data, size_t *len) { - if (spd_info->read_type == READ_SPD_MEMPTR) { - *spd_data_ptr = spd_info->spd_spec.spd_data_ptr_info.spd_data_ptr; - *spd_data_len = spd_info->spd_spec.spd_data_ptr_info.spd_data_len; - return; + switch (info->md_spd_loc) { + case SPD_MEMPTR: + *data = info->data_ptr; + *len = info->data_len; + break; + case SPD_CBFS: + read_spd_from_cbfs(info->cbfs_index, data, len); + break; + default: + die("Invalid location (%d) for Memory-down SPD\n", info->md_spd_loc); }
- if (spd_info->read_type == READ_SPD_CBFS) { - spd_read_from_cbfs(spd_info, spd_data_ptr, spd_data_len); - return; + print_spd_info((uint8_t *) *data); +} + +static void read_sodimm_spd(const struct spd_info *info, struct spd_block *blk) +{ + size_t i; + + blk->addr_map[0] = info->smbus_info[0].addr_dimm0; + blk->addr_map[1] = info->smbus_info[0].addr_dimm1; + blk->addr_map[2] = info->smbus_info[1].addr_dimm0; + blk->addr_map[3] = info->smbus_info[1].addr_dimm1; + + get_spd_smbus(blk); + + for (i = 0; i < ARRAY_SIZE(blk->addr_map); i++) { + if (blk->spd_array[i] != NULL) + print_spd_info((uint8_t *)blk->spd_array[i]); } }
-static void meminit_dq_dqs_map(FSP_M_CONFIG *mem_cfg, - const struct mb_cfg *board_cfg, - bool half_populated) +static void ddr_get_spd(int channel, const uintptr_t *spd_md_data, + const struct spd_block *spd_sodimm_blk, + const struct spd_info *info, + const bool half_populated, uintptr_t *spd_dimm0, + uintptr_t *spd_dimm1) { - memcpy(&mem_cfg->RcompResistor, &board_cfg->rcomp_resistor, - sizeof(mem_cfg->RcompResistor)); - - memcpy(&mem_cfg->RcompTarget, &board_cfg->rcomp_targets, - sizeof(mem_cfg->RcompTarget)); - - memcpy(&mem_cfg->DqMapCpu2DramCh0, &board_cfg->dq_map[DDR_CH0], - sizeof(board_cfg->dq_map[DDR_CH0])); - memcpy(&mem_cfg->DqsMapCpu2DramCh0, &board_cfg->dqs_map[DDR_CH0], - sizeof(board_cfg->dqs_map[DDR_CH0])); - - memcpy(&mem_cfg->DqMapCpu2DramCh1, &board_cfg->dq_map[DDR_CH1], - sizeof(board_cfg->dq_map[DDR_CH1])); - memcpy(&mem_cfg->DqsMapCpu2DramCh1, &board_cfg->dqs_map[DDR_CH1], - sizeof(board_cfg->dqs_map[DDR_CH1])); - - memcpy(&mem_cfg->DqMapCpu2DramCh2, &board_cfg->dq_map[DDR_CH2], - sizeof(board_cfg->dq_map[DDR_CH2])); - memcpy(&mem_cfg->DqsMapCpu2DramCh2, &board_cfg->dqs_map[DDR_CH2], - sizeof(board_cfg->dqs_map[DDR_CH2])); - - memcpy(&mem_cfg->DqMapCpu2DramCh3, &board_cfg->dq_map[DDR_CH3], - sizeof(board_cfg->dq_map[DDR_CH3])); - memcpy(&mem_cfg->DqsMapCpu2DramCh3, &board_cfg->dqs_map[DDR_CH3], - sizeof(board_cfg->dqs_map[DDR_CH3])); - - if (half_populated) - return; - - memcpy(&mem_cfg->DqMapCpu2DramCh4, &board_cfg->dq_map[DDR_CH4], - sizeof(board_cfg->dq_map[DDR_CH4])); - memcpy(&mem_cfg->DqsMapCpu2DramCh4, &board_cfg->dqs_map[DDR_CH4], - sizeof(board_cfg->dqs_map[DDR_CH4])); - - memcpy(&mem_cfg->DqMapCpu2DramCh5, &board_cfg->dq_map[DDR_CH5], - sizeof(board_cfg->dq_map[DDR_CH5])); - memcpy(&mem_cfg->DqsMapCpu2DramCh5, &board_cfg->dqs_map[DDR_CH5], - sizeof(board_cfg->dqs_map[DDR_CH5])); - - memcpy(&mem_cfg->DqMapCpu2DramCh6, &board_cfg->dq_map[DDR_CH6], - sizeof(board_cfg->dq_map[DDR_CH6])); - memcpy(&mem_cfg->DqsMapCpu2DramCh6, &board_cfg->dqs_map[DDR_CH6], - sizeof(board_cfg->dqs_map[DDR_CH6])); - - memcpy(&mem_cfg->DqMapCpu2DramCh7, &board_cfg->dq_map[DDR_CH7], - sizeof(board_cfg->dq_map[DDR_CH7])); - memcpy(&mem_cfg->DqsMapCpu2DramCh7, &board_cfg->dqs_map[DDR_CH7], - sizeof(board_cfg->dqs_map[DDR_CH7])); -} - -static void meminit_channels(FSP_M_CONFIG *mem_cfg, - const struct mb_cfg *board_cfg, - uintptr_t spd_data_ptr, - bool half_populated) -{ - uint8_t dimm_cfg = DISABLE_DIMM1; /* Use only DIMM0 */ - - /* Channel 0 */ - mem_cfg->DisableDimmMc0Ch0 = dimm_cfg; - mem_cfg->MemorySpdPtr00 = spd_data_ptr; - mem_cfg->MemorySpdPtr01 = 0; - - /* Channel 1 */ - mem_cfg->DisableDimmMc0Ch1 = dimm_cfg; - mem_cfg->MemorySpdPtr02 = spd_data_ptr; - mem_cfg->MemorySpdPtr03 = 0; - - /* Channel 2 */ - mem_cfg->DisableDimmMc0Ch2 = dimm_cfg; - mem_cfg->MemorySpdPtr04 = spd_data_ptr; - mem_cfg->MemorySpdPtr05 = 0; - - /* Channel 3 */ - mem_cfg->DisableDimmMc0Ch3 = dimm_cfg; - mem_cfg->MemorySpdPtr06 = spd_data_ptr; - mem_cfg->MemorySpdPtr07 = 0; - - if (half_populated) { - printk(BIOS_INFO, "%s: DRAM half-populated\n", __func__); - dimm_cfg = DISABLE_BOTH_DIMMS; - spd_data_ptr = 0; - } - - /* Channel 4 */ - mem_cfg->DisableDimmMc1Ch0 = dimm_cfg; - mem_cfg->MemorySpdPtr08 = spd_data_ptr; - mem_cfg->MemorySpdPtr09 = 0; - - /* Channel 5 */ - mem_cfg->DisableDimmMc1Ch1 = dimm_cfg; - mem_cfg->MemorySpdPtr10 = spd_data_ptr; - mem_cfg->MemorySpdPtr11 = 0; - - /* Channel 6 */ - mem_cfg->DisableDimmMc1Ch2 = dimm_cfg; - mem_cfg->MemorySpdPtr12 = spd_data_ptr; - mem_cfg->MemorySpdPtr13 = 0; - - /* Channel 7 */ - mem_cfg->DisableDimmMc1Ch3 = dimm_cfg; - mem_cfg->MemorySpdPtr14 = spd_data_ptr; - mem_cfg->MemorySpdPtr15 = 0; - - meminit_dq_dqs_map(mem_cfg, board_cfg, half_populated); -} - -/* Initialize onboard memory configurations for lpddr4x */ -void memcfg_init(FSP_M_CONFIG *mem_cfg, - const struct mb_cfg *board_cfg, - const struct spd_info *spd_info, - bool half_populated) -{ - if (spd_info->read_type == READ_SMBUS) { - for (int i = 0; i < NUM_DIMM_SLOT; i++) - mem_cfg->SpdAddressTable[i] = spd_info->spd_spec.spd_smbus_address[i]; - meminit_dq_dqs_map(mem_cfg, board_cfg, half_populated); + if (channel == 0) { + switch (info->topology) { + case MEMORY_DOWN: + *spd_dimm0 = 0; + *spd_dimm1 = *spd_md_data; + break; + case SODIMM: + case MIXED: /* For a mixed topology, channel 0 can only be SODIMM */ + *spd_dimm0 = (uintptr_t)spd_sodimm_blk->spd_array[0]; + *spd_dimm1 = (uintptr_t)spd_sodimm_blk->spd_array[1]; + break; + default: + die("Invalid memory topology on channel 0 (%d)\n", info->topology); + } + } else if (channel == 1) { + if (half_populated) { + *spd_dimm0 = *spd_dimm1 = 0; + } else { + switch (info->topology) { + case MEMORY_DOWN: + case MIXED: /* For a mixed topology, channel 1 can only be memory-down */ + *spd_dimm0 = *spd_md_data; + *spd_dimm1 = 0; + break; + case SODIMM: + *spd_dimm0 = (uintptr_t)spd_sodimm_blk->spd_array[2]; + *spd_dimm0 = (uintptr_t)spd_sodimm_blk->spd_array[3]; + break; + default: + die("Invalid memory topology on channel 1 (%d)\n", info->topology); + } + } } else { - size_t spd_data_len = 0; - uintptr_t spd_data_ptr = 0; + die("Invalid channel %d\n", channel); + } +}
- memset(&mem_cfg->SpdAddressTable, 0, sizeof(mem_cfg->SpdAddressTable)); - get_spd_data(spd_info, &spd_data_ptr, &spd_data_len); +/* If memory is half-populated, then upper half of the channels are required to be empty. */ +static bool lpx_channel_unpopulated(int channel, bool half_populated) +{ + return (half_populated && (channel >= (LPX_CHANNELS / 2))); +}
- mem_cfg->MemorySpdDataLen = spd_data_len; - meminit_channels(mem_cfg, board_cfg, spd_data_ptr, half_populated); +static void meminit_lpx(FSP_M_CONFIG *mem_cfg, const struct lpx_common_mem_config *board_cfg, + const struct spd_info *info, bool half_populated) +{ + size_t i; + size_t spd_len; + uintptr_t spd_data; + + if (info->topology != MEMORY_DOWN) + die("LPX only supports memory down topology\n"); + + /* LP4X does not support interleaving DQ pins */ + mem_cfg->DqPinsInterleaved = 0; + mem_cfg->ECT = board_cfg->ect; + + read_md_spd(info, &spd_data, &spd_len); + mem_cfg->MemorySpdDataLen = spd_len; + + for (i = 0; i < LPX_CHANNELS; ++i) { + if (lpx_channel_unpopulated(i, half_populated)) { + init_spd_upds_empty(mem_cfg, i); + init_dq_upds_empty(mem_cfg, i); + init_dqs_upds_empty(mem_cfg, i); + } else { + init_spd_upds_dimm0(mem_cfg, i, spd_data); + init_dq_upds(mem_cfg, i, board_cfg->dq_map[i][0], + board_cfg->dq_map[i][1]); + init_dqs_upds(mem_cfg, i, board_cfg->dqs_map[i][0], + board_cfg->dqs_map[i][1]); + } + } +} + +static void meminit_ddr(FSP_M_CONFIG *mem_cfg, const struct ddr_mem_config *board_cfg, + const struct spd_info *info, bool half_populated) +{ + uintptr_t spd_md_data; + size_t spd_md_len; + uintptr_t spd_dimm0 = 0; + uintptr_t spd_dimm1 = 0; + struct spd_block spd_sodimm_blk; + size_t i, b; + size_t index = 0; + + mem_cfg->ECT = board_cfg->ect; + mem_cfg->DqPinsInterleaved = board_cfg->dq_pins_interleaved; + + if (info->topology == MEMORY_DOWN || info->topology == MIXED) { + read_md_spd(info, &spd_md_data, &spd_md_len); + mem_cfg->MemorySpdDataLen = spd_md_len; }
- mem_cfg->Lp5CccConfig = board_cfg->lp5_ccc_config; - mem_cfg->ECT = board_cfg->ect; - mem_cfg->UserBd = board_cfg->UserBd; - mem_cfg->DqPinsInterleaved = board_cfg->dq_pins_interleaved; + if (info->topology == SODIMM || info->topology == MIXED) { + read_sodimm_spd(info, &spd_sodimm_blk); + if (info->topology == MIXED && + spd_md_len != spd_sodimm_blk.len) + die("Mixed topology SPDs don't match lengths\n"); + else + mem_cfg->MemorySpdDataLen = spd_sodimm_blk.len; + } + + /* Init SPD UPDs */ + for (i = 0; i < DDR_CHANNELS; ++i) { + ddr_get_spd(i, &spd_md_data, &spd_sodimm_blk, info, half_populated, &spd_dimm0, + &spd_dimm1); + init_spd_upds(mem_cfg, DDR_FSP_UPD_CHANNEL_IDX(i), spd_dimm0, spd_dimm1); + } + + /* Init DQ & DQS mapping UPDs */ + for (i = 0; i < DDR_CHANNELS; ++i) { + for (b = 0; b < DDR_BYTES_PER_CHANNEL; b += 2) { + if (half_populated && i == 1) { + init_dq_upds_empty(mem_cfg, index); + init_dqs_upds_empty(mem_cfg, index); + } else { + init_dq_upds(mem_cfg, index, board_cfg->dq_map[i][b], + board_cfg->dq_map[i][b + 1]); + init_dqs_upds(mem_cfg, index, board_cfg->dqs_map[i][b], + board_cfg->dqs_map[i][b + 1]); + } + ++index; + } + } +} + +void memcfg_init(FSP_M_CONFIG *mem_cfg, const struct adl_mem_config *board_cfg, + const struct spd_info *info, bool half_populated) +{ + switch (board_cfg->mem_type) { + case MEMTYPE_DDR4: + case MEMTYPE_DDR5: /* intentional fallthrough */ + meminit_ddr(mem_cfg, board_cfg->ddr_cfg, info, half_populated); + break; + case MEMTYPE_LP4X: + meminit_lpx(mem_cfg, board_cfg->lp4x_cfg, info, half_populated); + break; + case MEMTYPE_LP5X: + meminit_lpx(mem_cfg, board_cfg->lp5x_cfg->lpx_config, info, half_populated); + mem_cfg->Lp5CccConfig = board_cfg->lp5x_cfg->lp5_ccc_config; + break; + default: + die("Unsupported memory type (%d)\n", board_cfg->mem_type); + } }
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48860 )
Change subject: soc/intel/alderlake: Refactor meminit code ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/mem... File src/soc/intel/alderlake/meminit.c:
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/mem... PS1, Line 265: case MIXED: /* For a mixed topology, channel 1 can only be memory-down */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/mem... PS1, Line 274: die("Invalid memory topology on channel 1 (%d)\n", info->topology); line over 96 characters
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48860 )
Change subject: soc/intel/alderlake: Refactor meminit code ......................................................................
Patch Set 1:
(7 comments)
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/inc... File src/soc/intel/alderlake/include/soc/meminit.h:
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/inc... PS1, Line 12: /* True for both LP4X and LP5X */ Did we confirm FSP/MRC expectations for LPx and DDR?
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/inc... PS1, Line 16: True for both DDR4 and DDR5 I think it is not the same for DDR4 and DDR5. I think DDR5 channels are 4 instead of 2?
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/inc... PS1, Line 22: DDR4 DDRx since I believe it is true for DDR4 and DDR5?
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/inc... PS1, Line 23: DDR4 supports CH0 as SODIMM and CH1 as Memory Down Have we confirmed this for ADL?
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/inc... PS1, Line 65: uint8_t dq_map[DDR_CHANNELS][DDR_BYTES_PER_CHANNEL][BITS_PER_BYTE]; : uint8_t dqs_map[DDR_CHANNELS][DDR_BYTES_PER_CHANNEL]; Angel had posted this earlier on one of Subrata's CLs and I think it makes sense. Instead of having a single [][][] table for dq_map, it might be much easier for mainboards to fill something like:
» struct { » » uint8_t dq0[BITS_PER_BYTE]; » » uint8_t dq1[BITS_PER_BYTE]; » » uint8_t dq2[BITS_PER_BYTE]; » » uint8_t dq3[BITS_PER_BYTE]; » » uint8_t dq4[BITS_PER_BYTE]; » » uint8_t dq5[BITS_PER_BYTE]; » » uint8_t dq6[BITS_PER_BYTE]; » » uint8_t dq7[BITS_PER_BYTE]; » } dq_map[DDR_CHANNELS];
» struct { » » uint8_t dqs0; » » uint8_t dqs1; » » uint8_t dqs2; » » uint8_t dqs3; » » uint8_t dqs4; » » uint8_t dqs5; » » uint8_t dqs6; » » uint8_t dqs7; » } dqs_map[DDR_CHANNELS];
Above is for DDR4. For LP4x/LP5x, it would be something like:
» struct { » » uint8_t dq0[BITS_PER_BYTE]; » » uint8_t dq1[BITS_PER_BYTE]; » } dq_map[LPX_CHANNELS];
» struct { » » uint8_t dqs0; » » uint8_t dqs1; » } dqs_map[LPX_CHANNELS];
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/mem... File src/soc/intel/alderlake/meminit.c:
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/mem... PS1, Line 11: Translate DDR4/5 channel to FSP UPD index for the channel. Have we confirmed this for ADL?
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/mem... PS1, Line 377: /* intentional fallthrough */ Isn't this supposed to be after the above case?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48860 )
Change subject: soc/intel/alderlake: Refactor meminit code ......................................................................
Patch Set 1:
(7 comments)
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/inc... File src/soc/intel/alderlake/include/soc/meminit.h:
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/inc... PS1, Line 12: /* True for both LP4X and LP5X */
Did we confirm FSP/MRC expectations for LPx and DDR?
This is our current WIP understanding of the expectations (from reading EDS and info from Intel) Still waiting on MRC confirmation 😊
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/inc... PS1, Line 16: True for both DDR4 and DDR5
I think it is not the same for DDR4 and DDR5. […]
I see 1 or 2 channel configurations for DDR5, from EDS #619501, section 5.1.2, do you see something different elsewhere?
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/inc... PS1, Line 22: DDR4
DDRx since I believe it is true for DDR4 and DDR5?
Done
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/inc... PS1, Line 23: DDR4 supports CH0 as SODIMM and CH1 as Memory Down
Have we confirmed this for ADL?
This is from the EDS #619501 "In mix topology implementation Ch0 will be SoDIMM and Ch1 MD"
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/inc... PS1, Line 65: uint8_t dq_map[DDR_CHANNELS][DDR_BYTES_PER_CHANNEL][BITS_PER_BYTE]; : uint8_t dqs_map[DDR_CHANNELS][DDR_BYTES_PER_CHANNEL];
Angel had posted this earlier on one of Subrata's CLs and I think it makes sense. […]
Good memory I had forgotten, that was a good suggestion, I'll see what it looks like here.
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/mem... File src/soc/intel/alderlake/meminit.c:
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/mem... PS1, Line 11: Translate DDR4/5 channel to FSP UPD index for the channel.
Have we confirmed this for ADL?
Not yet no, this is the behavior we expect given all the information we have received so far (should be similar or same as TGL expectations); i.e. is WIP until we get confirmation from Intel
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/mem... PS1, Line 377: /* intentional fallthrough */
Isn't this supposed to be after the above case?
Ah yes, my eyes were bleeding after writing 500 lines 😋
Hello build bot (Jenkins), Furquan Shaikh, Subrata Banik, Balaji Manigandan, Patrick Rudolph, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48860
to look at the new patch set (#2).
Change subject: soc/intel/alderlake: Refactor meminit code ......................................................................
soc/intel/alderlake: Refactor meminit code
Align ADL's memory initialization code with TGL; this allows mainboards to e.g. more easily align the CPU<->DRAM DQ pin mapping between the data structure and the schematic.
BUG=b:172978729 TEST=abuild intel/adlrvp
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: I2b75e856f1f2aa34bb7a91913147faf3037e7cfb --- M src/mainboard/intel/adlrvp/include/baseboard/variants.h M src/mainboard/intel/adlrvp/memory.c M src/mainboard/intel/adlrvp/romstage_fsp_params.c M src/soc/intel/alderlake/include/soc/meminit.h M src/soc/intel/alderlake/meminit.c 5 files changed, 517 insertions(+), 259 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/48860/2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48860 )
Change subject: soc/intel/alderlake: Refactor meminit code ......................................................................
Patch Set 2:
Angel, took your suggestion from https://review.coreboot.org/c/coreboot/+/48077/4/src/mainboard/intel/adlrvp/...
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48860 )
Change subject: soc/intel/alderlake: Refactor meminit code ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/mem... File src/soc/intel/alderlake/meminit.c:
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/mem... PS2, Line 265: case MIXED: /* For a mixed topology, channel 1 can only be memory-down */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/mem... PS2, Line 274: die("Invalid memory topology on channel 1 (%d)\n", info->topology); line over 96 characters
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48860 )
Change subject: soc/intel/alderlake: Refactor meminit code ......................................................................
Patch Set 2:
(8 comments)
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/inc... File src/soc/intel/alderlake/include/soc/meminit.h:
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/inc... PS2, Line 94: /* : * Command pins mapping for Controller Channel (ccc) : * lp5_ccc_config: Bitmask where bits [3:0] are Controller 0 Channel [3:0] and : * bits [7:4] are Controller 1 Channel [3:0] : * Bit value: 0 = ccc pin mapping is ascending, 1 = ccc pin mapping is descending. : */ Probably retain comment?
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/inc... PS2, Line 63: struct { nit: It would be helpful to have a comment indicating what the index and value means.
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/inc... PS2, Line 87: /* : * Rcomp resistor values. These values represent the resistance in : * ohms of the three rcomp resistors attached to the DDR_COMP_0, : * DDR_COMP_1, and DDR_COMP_2 pins on the DRAM. : */ : uint16_t rcomp_resistor[3]; : : /* Rcomp target values. */ : uint16_t rcomp_targets[5]; Not for this change, but we should confirm with Intel if these still need to be filled in. I think I had seen some bug/CL from Intel where some of these could be dropped.
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/inc... File src/soc/intel/alderlake/include/soc/meminit.h:
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/inc... PS1, Line 12: /* True for both LP4X and LP5X */
This is our current WIP understanding of the expectations (from reading EDS and info from Intel) Sti […]
As per EDS Vol 1 (619501) section 5.1.5, this looks correct.
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/inc... PS1, Line 16: True for both DDR4 and DDR5
I see 1 or 2 channel configurations for DDR5, from EDS #619501, section 5.1. […]
See section 5.1.5. DDR4 is 2 channels total and DDR5 is 4 channels.
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/inc... PS1, Line 23: DDR4 supports CH0 as SODIMM and CH1 as Memory Down
This is from the EDS #619501 "In mix topology implementation Ch0 will be SoDIMM and Ch1 MD"
SG.
BTW, I noticed that the section 5.1.2 was removed from revision 0.7 of the doc, whereas it is present in revision 0.65 of the doc. Can you please raise a bug against Intel to know why this was dropped and what is the latest supported configuration by ADL.
I see that in revision 0.65 there are some differences in the half-populated channels for LPx compared to TGL i.e. TGL had higher channels empty whereas ADL seems to have lower channels empty. This means that we will have to update `lpx_channel_unpopulated()` accordingly. Same for DDR.
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/mem... File src/soc/intel/alderlake/meminit.c:
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/mem... PS1, Line 11: Translate DDR4/5 channel to FSP UPD index for the channel.
Not yet no, this is the behavior we expect given all the information we have received so far (should […]
Given that DDR5 is not the same as DDR4 as per EDS for the channel configuration, this might differ.
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/mem... File src/soc/intel/alderlake/meminit.c:
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/mem... PS2, Line 248: *spd_dimm0 = 0; DIMM0 is always populated. Here dimm0 should be spd_md_data and dimm1 should be empty.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48860 )
Change subject: soc/intel/alderlake: Refactor meminit code ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/mem... File src/soc/intel/alderlake/meminit.c:
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/mem... PS2, Line 100: init_dq_upds BTW, what do you think about this:
Define new dq_map and dqs_map structures within meminit.c that represent the 8-byte pairs without taking into account any specific memory technology.
struct dq_map { uint8_t dq0[BITS_PER_BYTE]; uint8_t dq1[BITS_PER_BYTE]; };
struct dqs_map { uint8_t dqs0; uint8_t dqs1; };
And those can be used to pass in the dq_map and dqs_map by all the meminit_* functions.
#define TOTAL_BYTE_PAIRS 8
static void init_dq_table(FSP_M_CONFIG *mem_cfg, struct dq_map *dq_map_ptr, bool half_populated) { int total_pairs = half_populated ? TOTAL_BYTE_PAIRS / 2 : TOTAL_BYTE_PAIRS;
for (i = 0; i < total_pairs; i++, dq_map_ptr++) { init_dq_upds(mem_cfg, i, dq_map_ptr.dq0, dq_map_ptr.dq1); }
for (; i < TOTAL_BYTE_PAIRS; i++) { init_dq_upds_empty(mem_cfg, i); } }
This should reduce the calls in `meminit_lpx()` and `meminit_ddr()` to:
init_dq_table(mem_cfg, board_cfg->dq_map, half_populated); init_dqs_table(mem_cfg, board_cfg->dqs_map, half_populated);
Probably cleans up the functions and makes it easier to read? Also, if we really have to support a different channel configuration for DDR5, it won't result in a lot of additional code.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48860 )
Change subject: soc/intel/alderlake: Refactor meminit code ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/mem... File src/soc/intel/alderlake/meminit.c:
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/mem... PS2, Line 100: init_dq_upds
BTW, what do you think about this: […]
Above can be further simplified by eliminating init_dq_upds()/init_dqs_upds() completely too.
static void *fspm_dq_upd_ptrs[] = { &mem_cfg->DqMapCpu2DramCh0, &mem_cfg->DqMapCpu2DramCh1, &mem_cfg->DqMapCpu2DramCh2, &mem_cfg->DqMapCpu2DramCh3, &mem_cfg->DqMapCpu2DramCh4, &mem_cfg->DqMapCpu2DramCh5, &mem_cfg->DqMapCpu2DramCh6, &mem_cfg->DqMapCpu2DramCh7, };
for (i = 0; i < total_pairs; i++, dq_map_ptr++) { void *upd_ptr = fspm_dq_upd_ptrs[i];
memcpy(upd_ptr, dq_map_ptr.dq0, BITS_PER_BYTE); memcpy((char *)upd_ptr + BITS_PER_BYTE, dq_map_ptr.dq1, BITS_PER_BYTE); }
for (; i < TOTAL_BYTE_PAIRS; i++) { void *upd_ptr = fspm_dq_upd_ptrs[i];
memset(upd_ptr, 0, 2 * BITS_PER_BYTE); }
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48860 )
Change subject: soc/intel/alderlake: Refactor meminit code ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/mem... File src/soc/intel/alderlake/meminit.c:
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/mem... PS2, Line 272: break; 0/1? I think typo here, right?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48860 )
Change subject: soc/intel/alderlake: Refactor meminit code ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/mem... File src/soc/intel/alderlake/meminit.c:
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/mem... PS2, Line 39: init_spd_upds I have been thinking about this. What do you think about - probably flipping this around to handle the channels so that it can be used by any meminit* functions irrespective of the memory technology.
struct mem_channel_info { struct channel_to_spd_upd_masks { uint8_t dimm0_full_channel_mask; uint8_t dimm1_full_channel_mask; uint8_t dimm0_half_channel_mask; uint8_t dimm1_half_channel_mask; }; struct mixed_topology_masks { uint8_t memory_down_mask; uint8_t sodimm_mask; } } mem_channel_info[] = { [MEMTYPE_LP4X] = { .dimm0_full_channel_mask = 0xff, /* DIMM0 for all 8 channels are populated. */ .dimm1_full_channel_mask = 0x0, /* DIMM1 is not supported. */ .dimm0_half_channel_mask = 0xf, /* Lower half of the channels are populated. */ .dimm1_half_channel_mask = 0x0, /* DIMM1 is not supported. */ .memory_down_mask = 0x0, /* Mixed topology is not supported. */ .sodimm_mask = 0x0, /* Mixed topology is not supported. */ }, [MEMTYPE_LP5X] = { ... }, [MEMTYPE_DDR4] = { .dimm0_full_channel_mask = 0x11, /* Channel 0 <-> SPD index 0, Channel 1 <-> SPD index 4 */ .dimm1_full_channel_mask = 0x11, /* Channel 0 <-> SPD index 0, Channel 1 <-> SPD index 4 */ .dimm0_half_channel_mask = 0x1, /* Only channel 0 is populated. */ .dimm1_half_channel_mask = 0x1, /* Only channel 0 is populated. */ .memory_down_mask = 0x1, /* In mixed mode, channel 0 can only be memory down. */ .sodimm_mask = 0x10, /* In mixed mode, channel 1 can only be sodimm. */ }, [MEMTYPE_DDR5] = { ... }, };
and then we can have a init_spd() function:
static void init_spd(struct spd_info *spd_info, int max_channels, int mem_type, bool half_populated) { struct mem_channel_info *channel_info = &mem_channel_info[mem_type]; static uint32_t *dimm0_spd_ptrs = { &mem_cfg->MemorySpdPtr00, &mem_cfg->MemorySpdPtr02, ... }; static uint32_t *dimm1_spd_ptrs = { &mem_cfg->MemorySpdPtr01, &mem_cfg->MemorySpdPtr03, ... };
if (MIXED OR MD topology) // Read MD SPD if (MIXED OR SODIMM topology) // Read SODIMM SPD
if (MIXED) // ensure lengths of md and sodimm spd are same
if (half_populated) { // use half channel masks } else { // use full channel masks }
for (i = 0; i < max_channels; i++) { dimm0_ptr = &dimm0_spd_ptrs[i]; dimm1_ptr = &dimm1_spd_ptrs[i]; disable_dimm_ptr = &disable_dimm_spd_ptrs[i];
dimm0_spd = get_spd(...); dimm1_spd = get_spd(...);
disable_dimm_mask = 0;
if (BIT(i) & dimm0_channel_spd_mask) { *dimm0_ptr = dimm0_spd; } else { *dimm0_ptr = 0; disable_dimm_mask |= DIMM0; }
if (BIT(i) & dimm1_channel_spd_mask) { *dimm1_ptr = dimm1_spd; } else { *dimm1_ptr = 0; disable_dimm_mask |= DIMM1; }
*disable_dimm_ptr = disable_dimm_mask; } }
static uintptr_t get_spd(int channel, uint32_t md_spd, uint32_t sodimm_spd, int topology, uint8_t md_mask, uint8_t sodimm_mask) { if (topology == MIXED) { if (md_mask & BIT(channel)) topology = MEMORY_DOWN; else if (sodimm_mask & BIT(channel)) topology = SODIMM; else die(...); }
if (//MEMORY DOWN) return md_spd; if (//SODIMM) return sodimm_spd; die(...); }
This probably doesn't reduce the overall code right now. But I am thinking of this change for 2 reasons: 1. Adding support for new memory types would be light weight. Adding a table for DDR5 for example instead of having multiple functions to format the inputs.
2. Long term, I am envisioning that this can be moved into common code for Intel SoCs so that we don't have to repeat this effort for every new generation. But, before we get there we need to arrive at an API that is flexible enough to work for any memory technology and FSP UPD expectations.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48860 )
Change subject: soc/intel/alderlake: Refactor meminit code ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/48860/2/src/mainboard/intel/adlrvp/... File src/mainboard/intel/adlrvp/memory.c:
https://review.coreboot.org/c/coreboot/+/48860/2/src/mainboard/intel/adlrvp/... PS2, Line 32: [3] = { .dq0 = { 2, 6, 7, 1, 3, 4, 0, 5 }, : .dq1 = { 9, 13, 8, 15, 14, 11, 12, 10 }, }, : [4] = { .dq0 = { 3, 0, 1, 2, 7, 4, 6, 5 }, nit: add an extra space on these three lines to align the values
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/inc... File src/soc/intel/alderlake/include/soc/meminit.h:
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/inc... PS2, Line 94: /* : * Command pins mapping for Controller Channel (ccc) : * lp5_ccc_config: Bitmask where bits [3:0] are Controller 0 Channel [3:0] and : * bits [7:4] are Controller 1 Channel [3:0] : * Bit value: 0 = ccc pin mapping is ascending, 1 = ccc pin mapping is descending. : */
Probably retain comment?
Yes, please.
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/inc... PS2, Line 122: const struct lp5x_mem_config *lp5x_cfg; Does this union work as intended regarding `lpx_common_mem_config`?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48860 )
Change subject: soc/intel/alderlake: Refactor meminit code ......................................................................
Patch Set 2:
@Furquan, AFAIK, new FSP doesn't need DQ map for SODIMM memory. Maybe we can remove it? It will probe and mapping by MRC. @Subrata, Am I right?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48860 )
Change subject: soc/intel/alderlake: Refactor meminit code ......................................................................
Patch Set 2:
Patch Set 2:
@Furquan, AFAIK, new FSP doesn't need DQ map for SODIMM memory. Maybe we can remove it? It will probe and mapping by MRC.
That can be done at the mainboard level, right? i.e. skip DQ map. SoC code would just copy all 0s in that case.
@Subrata, Am I right?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48860 )
Change subject: soc/intel/alderlake: Refactor meminit code ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
@Furquan, AFAIK, new FSP doesn't need DQ map for SODIMM memory. Maybe we can remove it? It will probe and mapping by MRC.
That can be done at the mainboard level, right? i.e. skip DQ map. SoC code would just copy all 0s in that case.
@Subrata, Am I right?
I mean we can skip update dq map in meminit_ddr. SG?
Tim Wawrzynczak has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/48860 )
Change subject: soc/intel/alderlake: Refactor meminit code ......................................................................
Abandoned
replaced by 49043 and friends
-
Angel Pons (Code Review) -
build bot (Jenkins) (Code Review)
-
EricR Lai (Code Review)
-
Furquan Shaikh (Code Review)
-
Tim Wawrzynczak (Code Review)