Philip Chen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32513
Change subject: soc/intel/cannonlake: Clean up cannonlake_memcfg_init ......................................................................
soc/intel/cannonlake: Clean up cannonlake_memcfg_init
The major changes include: (1) Add enum 'mem_info_read_type' to spd_info. (2) Add spd_into into cnl_mb_cfg. (3) Squash meminit_memcfg_spd().
BUG=b:124990009 TEST=build
Change-Id: I686a85996858204c20fd05ef24787a0487817c34 --- M src/mainboard/google/hatch/romstage.c M src/mainboard/google/hatch/variants/baseboard/memory.c M src/mainboard/google/sarien/romstage.c M src/mainboard/intel/coffeelake_rvp/memory.c M src/mainboard/intel/coffeelake_rvp/romstage.c M src/soc/intel/cannonlake/cnl_memcfg_init.c M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h 7 files changed, 90 insertions(+), 84 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/32513/1
diff --git a/src/mainboard/google/hatch/romstage.c b/src/mainboard/google/hatch/romstage.c index bdf951b..95db576 100644 --- a/src/mainboard/google/hatch/romstage.c +++ b/src/mainboard/google/hatch/romstage.c @@ -25,14 +25,8 @@ { struct cnl_mb_cfg memcfg;
- const struct spd_info spd = { - .spd_by_index = true, - .spd_spec.spd_index = variant_memory_sku(), - }; - variant_memory_params(&memcfg); - cannonlake_memcfg_init(&memupd->FspmConfig, - &memcfg, &spd); + cannonlake_memcfg_init(&memupd->FspmConfig, &memcfg); }
void mainboard_get_dram_part_num(const char **part_num, size_t *len) diff --git a/src/mainboard/google/hatch/variants/baseboard/memory.c b/src/mainboard/google/hatch/variants/baseboard/memory.c index b6a6615..8706332 100644 --- a/src/mainboard/google/hatch/variants/baseboard/memory.c +++ b/src/mainboard/google/hatch/variants/baseboard/memory.c @@ -20,6 +20,9 @@ #include <string.h>
static const struct cnl_mb_cfg baseboard_memcfg = { + .spd = { + .read_type = READ_SPD_CBFS}, + /* * The dqs_map arrays map the ddr4 pins to the SoC pins * for both channels. @@ -27,14 +30,14 @@ * the index = pin number on ddr4 part * the value = pin number on SoC */ - .dqs_map[DDR_CH0] = { 0, 1, 4, 5, 2, 3, 6, 7 }, - .dqs_map[DDR_CH1] = { 0, 1, 4, 5, 2, 3, 6, 7 }, + .dqs_map[DDR_CH0] = {0, 1, 4, 5, 2, 3, 6, 7}, + .dqs_map[DDR_CH1] = {0, 1, 4, 5, 2, 3, 6, 7},
/* Baseboard uses 121, 81 and 100 rcomp resistors */ - .rcomp_resistor = { 121, 81, 100 }, + .rcomp_resistor = {121, 81, 100},
/* Baseboard Rcomp target values */ - .rcomp_targets = { 100, 40, 20, 20, 26 }, + .rcomp_targets = {100, 40, 20, 20, 26},
/* Set CaVref config to 2 */ .vref_ca_config = 2, @@ -46,6 +49,7 @@ void __weak variant_memory_params(struct cnl_mb_cfg *bcfg) { memcpy(bcfg, &baseboard_memcfg, sizeof(baseboard_memcfg)); + bcfg->spd.spd_spec.spd_index = variant_memory_sku(); /* * GPP_F2 is the MEM_CH_SEL gpio, which is set to 1 for single * channel skus and 0 for dual channel skus. diff --git a/src/mainboard/google/sarien/romstage.c b/src/mainboard/google/sarien/romstage.c index e83cd4a..b66fd70 100644 --- a/src/mainboard/google/sarien/romstage.c +++ b/src/mainboard/google/sarien/romstage.c @@ -18,6 +18,14 @@ #include <soc/romstage.h>
static const struct cnl_mb_cfg memcfg = { + .spd = { + .read_type = READ_SMBUS, + .spd_spec = { + .spd_smbus_address[0] = 0xa0, + .spd_smbus_address[2] = 0xa4, + }, + }, + /* * The dqs_map arrays map the ddr4 pins to the SoC pins * for both channels. @@ -25,16 +33,16 @@ * the index = pin number on ddr4 part * the value = pin number on SoC */ - .dqs_map[DDR_CH0] = { 0, 1, 4, 5, 2, 3, 6, 7 }, - .dqs_map[DDR_CH1] = { 0, 1, 4, 5, 2, 3, 6, 7 }, + .dqs_map[DDR_CH0] = {0, 1, 4, 5, 2, 3, 6, 7}, + .dqs_map[DDR_CH1] = {0, 1, 4, 5, 2, 3, 6, 7},
/* Baseboard uses 121, 81 and 100 rcomp resistors */ - .rcomp_resistor = { 121, 81, 100 }, + .rcomp_resistor = {121, 81, 100},
/* * Baseboard Rcomp target values. */ - .rcomp_targets = { 100, 40, 20, 20, 26 }, + .rcomp_targets = {100, 40, 20, 20, 26},
/* Disable Early Command Training */ .ect = 0, @@ -45,12 +53,7 @@
void mainboard_memory_init_params(FSPM_UPD *memupd) { - const struct spd_info spd = { - .spd_smbus_address[0] = 0xa0, - .spd_smbus_address[2] = 0xa4 - }; - wilco_ec_romstage_init();
- cannonlake_memcfg_init(&memupd->FspmConfig, &memcfg, &spd); + cannonlake_memcfg_init(&memupd->FspmConfig, &memcfg); } diff --git a/src/mainboard/intel/coffeelake_rvp/memory.c b/src/mainboard/intel/coffeelake_rvp/memory.c index a13000c..a9695e8 100644 --- a/src/mainboard/intel/coffeelake_rvp/memory.c +++ b/src/mainboard/intel/coffeelake_rvp/memory.c @@ -20,23 +20,32 @@ #include <soc/cnl_memcfg_init.h>
static const struct cnl_mb_cfg baseboard_memcfg_cfg = { - /* + .spd = { + .read_type = READ_SMBUS, + .spd_spec = { + .spd_smbus_address[0] = 0xA0, + .spd_smbus_address[1] = 0xA2, + .spd_smbus_address[2] = 0xA4, + .spd_smbus_address[3] = 0xA6, + }, + }; +/* * The dqs_map arrays map the ddr4 pins to the SoC pins * for both channels. * * the index = pin number on ddr4 part * the value = pin number on SoC */ - .dqs_map[DDR_CH0] = { 0, 1, 3, 2, 4, 5, 6, 7 }, - .dqs_map[DDR_CH1] = { 1, 0, 4, 5, 2, 3, 6, 7 }, +.dqs_map[DDR_CH0] = {0, 1, 3, 2, 4, 5, 6, 7}, + .dqs_map[DDR_CH1] = {1, 0, 4, 5, 2, 3, 6, 7},
/* Baseboard uses 121, 81 and 100 rcomp resistors */ - .rcomp_resistor = { 121, 81, 100 }, + .rcomp_resistor = {121, 81, 100},
/* * Baseboard Rcomp target values. */ - .rcomp_targets = { 100, 40, 20, 20, 26 }, + .rcomp_targets = {100, 40, 20, 20, 26},
/* Baseboard is an interleaved design */ .dq_pins_interleaved = 1, @@ -46,7 +55,8 @@
/* Disable Early Command Training */ .ect = 0, -}; +} +;
const struct cnl_mb_cfg *__weak variant_memcfg_config(void) { diff --git a/src/mainboard/intel/coffeelake_rvp/romstage.c b/src/mainboard/intel/coffeelake_rvp/romstage.c index 1ab2d78..09ef148 100644 --- a/src/mainboard/intel/coffeelake_rvp/romstage.c +++ b/src/mainboard/intel/coffeelake_rvp/romstage.c @@ -20,13 +20,5 @@
void mainboard_memory_init_params(FSPM_UPD *memupd) { - const struct spd_info spd = { - .spd_smbus_address[0] = 0xA0, - .spd_smbus_address[1] = 0xA2, - .spd_smbus_address[2] = 0xA4, - .spd_smbus_address[3] = 0xA6, - }; - - cannonlake_memcfg_init(&memupd->FspmConfig, - variant_memcfg_config(), &spd); + cannonlake_memcfg_init(&memupd->FspmConfig, variant_memcfg_config()); } diff --git a/src/soc/intel/cannonlake/cnl_memcfg_init.c b/src/soc/intel/cannonlake/cnl_memcfg_init.c index db001b8..ad7a71c 100644 --- a/src/soc/intel/cannonlake/cnl_memcfg_init.c +++ b/src/soc/intel/cannonlake/cnl_memcfg_init.c @@ -20,7 +20,7 @@ #include <string.h>
static void meminit_memcfg(FSP_M_CONFIG *mem_cfg, - const struct cnl_mb_cfg *board_cfg) + const struct cnl_mb_cfg *board_cfg) { /* * DqByteMapChx expects 12 bytes of data, but the last 6 bytes @@ -29,29 +29,34 @@ */ memset(&mem_cfg->DqByteMapCh0, 0, sizeof(mem_cfg->DqByteMapCh0)); memcpy(&mem_cfg->DqByteMapCh0, &board_cfg->dq_map[DDR_CH0], - sizeof(board_cfg->dq_map[DDR_CH0])); + sizeof(board_cfg->dq_map[DDR_CH0]));
memset(&mem_cfg->DqByteMapCh1, 0, sizeof(mem_cfg->DqByteMapCh1)); memcpy(&mem_cfg->DqByteMapCh1, &board_cfg->dq_map[DDR_CH1], - sizeof(board_cfg->dq_map[DDR_CH1])); + sizeof(board_cfg->dq_map[DDR_CH1]));
memcpy(&mem_cfg->DqsMapCpu2DramCh0, &board_cfg->dqs_map[DDR_CH0], - sizeof(board_cfg->dqs_map[DDR_CH0])); + sizeof(board_cfg->dqs_map[DDR_CH0])); memcpy(&mem_cfg->DqsMapCpu2DramCh1, &board_cfg->dqs_map[DDR_CH1], - sizeof(board_cfg->dqs_map[DDR_CH1])); + sizeof(board_cfg->dqs_map[DDR_CH1]));
memcpy(&mem_cfg->RcompResistor, &board_cfg->rcomp_resistor, - sizeof(mem_cfg->RcompResistor)); + sizeof(mem_cfg->RcompResistor));
/* Early cannonlake requires rcomp targets to be 0 */ memcpy(&mem_cfg->RcompTarget, &board_cfg->rcomp_targets, - sizeof(mem_cfg->RcompTarget)); + sizeof(mem_cfg->RcompTarget)); }
-static void meminit_memcfg_spd(FSP_M_CONFIG *mem_cfg, - const struct cnl_mb_cfg *cnl_cfg, - size_t spd_data_len, uintptr_t spd_data_ptr) +/* + * Initialize default memory settings using spd data contained in a buffer. + */ +static void meminit_spd_data(FSP_M_CONFIG *mem_cfg, + const struct cnl_mb_cfg *cnl_cfg, + size_t spd_data_len, uintptr_t spd_data_ptr) { + assert(spd_data_ptr && spd_data_len); + mem_cfg->MemorySpdDataLen = spd_data_len;
if (cnl_cfg->channel_empty[0] == 0) @@ -62,31 +67,20 @@ }
/* - * Initialize default memory settings using spd data contained in a buffer. - */ -static void meminit_spd_data(FSP_M_CONFIG *mem_cfg, - const struct cnl_mb_cfg *cnl_cfg, - size_t spd_data_len, uintptr_t spd_data_ptr) -{ - assert(spd_data_ptr && spd_data_len); - meminit_memcfg_spd(mem_cfg, cnl_cfg, spd_data_len, spd_data_ptr); -} - -/* * Initialize default memory settings using the spd file specified by * spd_index. The spd_index is an index into the SPD_SOURCES array defined * in spd/Makefile.inc. */ static void meminit_cbfs_spd_index(FSP_M_CONFIG *mem_cfg, - const struct cnl_mb_cfg *cnl_cfg, - int spd_index) + const struct cnl_mb_cfg *cnl_cfg) { size_t spd_data_len; uintptr_t spd_data_ptr; struct region_device spd_rdev;
- printk(BIOS_DEBUG, "SPD INDEX = %d\n", spd_index); - if (get_spd_cbfs_rdev(&spd_rdev, spd_index) < 0) + printk(BIOS_DEBUG, "SPD INDEX = %d\n", + cnl_cfg->spd.spd_spec.spd_index); + if (get_spd_cbfs_rdev(&spd_rdev, cnl_cfg->spd.spd_spec.spd_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 */ @@ -97,10 +91,8 @@
/* Initialize onboard memory configurations for CannonLake */ void cannonlake_memcfg_init(FSP_M_CONFIG *mem_cfg, - const struct cnl_mb_cfg *cnl_cfg, - const struct spd_info *spd) + const struct cnl_mb_cfg *cnl_cfg) { - bool OnModuleSpd = false; /* Early Command Training Enabled */ mem_cfg->ECT = cnl_cfg->ect; mem_cfg->DqPinsInterleaved = cnl_cfg->dq_pins_interleaved; @@ -108,24 +100,26 @@
/* Spd pointer will only be used if all smbus slave address of memory * sockets on the platform is empty */ - for (int i = 0; i < ARRAY_SIZE(mem_cfg->SpdAddressTable); i++) { - if (spd->spd_smbus_address[i] != 0) { - mem_cfg->SpdAddressTable[i] = spd->spd_smbus_address[i]; - OnModuleSpd = true; + switch (cnl_cfg->spd.read_type) { + case READ_SMBUS: + for (int i = 0; i < ARRAY_SIZE(mem_cfg->SpdAddressTable); i++) { + if (cnl_cfg->spd.spd_spec.spd_smbus_address[i]) { + mem_cfg->SpdAddressTable[i] = cnl_cfg->spd. + spd_spec.spd_smbus_address[i]; + } } - } - - if (!OnModuleSpd) { - if (spd->spd_by_index) { - meminit_cbfs_spd_index(mem_cfg, cnl_cfg, - spd->spd_spec.spd_index); - } else { - meminit_spd_data(mem_cfg, cnl_cfg, - spd->spd_spec.spd_data_ptr_info.spd_data_len, - spd->spd_spec.spd_data_ptr_info.spd_data_ptr); - } + break; + case READ_SPD_CBFS: + meminit_cbfs_spd_index(mem_cfg, cnl_cfg); + break; + case READ_SPD_MEMPTR: + meminit_spd_data(mem_cfg, cnl_cfg, + cnl_cfg->spd.spd_spec.spd_data_ptr_info.spd_data_len, + cnl_cfg->spd.spd_spec.spd_data_ptr_info.spd_data_ptr); + break; + default: + printk(BIOS_DEBUG, "No valid way to read mem info."); }
meminit_memcfg(mem_cfg, cnl_cfg); - } diff --git a/src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h b/src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h index c602180..42d67a5 100644 --- a/src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h +++ b/src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h @@ -40,17 +40,27 @@ uintptr_t spd_data_ptr; };
+enum mem_info_read_type { + NOT_EXISTING, + READ_SMBUS, + READ_SPD_CBFS, + READ_SPD_MEMPTR +}; + struct spd_info { - bool spd_by_index; + enum mem_info_read_type read_type; union spd_data_by { int spd_index; struct spd_by_pointer spd_data_ptr_info; + uint8_t spd_smbus_address[4]; } spd_spec; - uint8_t spd_smbus_address[4]; };
/* Board-specific memory dq mapping information */ struct cnl_mb_cfg { + /* Parameters required to access DRAM info. */ + struct spd_info spd; + /* * For each channel, there are 3 sets of DQ byte mappings, * where each set has a package 0 and a package 1 value (package 0 @@ -122,7 +132,6 @@ * Initialize default memory configurations for CannonLake. */ void cannonlake_memcfg_init(FSP_M_CONFIG *mem_cfg, - const struct cnl_mb_cfg *cnl_cfg, - const struct spd_info *spd); + const struct cnl_mb_cfg *cnl_cfg);
#endif /* _SOC_CANNONLAKE_MEMCFG_INIT_H_ */
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Clean up cannonlake_memcfg_init ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32513/1/src/soc/intel/cannonlake/cnl_memcfg_... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/#/c/32513/1/src/soc/intel/cannonlake/cnl_memcfg_... PS1, Line 108: spd_spec.spd_smbus_address[i]; Avoid multiple line dereference - prefer 'cnl_cfg->spd.spd_spec.spd_smbus_address[i]'
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32513
to look at the new patch set (#2).
Change subject: soc/intel/cannonlake: Clean up cannonlake_memcfg_init ......................................................................
soc/intel/cannonlake: Clean up cannonlake_memcfg_init
The major changes include: (1) Add enum 'mem_info_read_type' to spd_info. (2) Move spd_into into cnl_mb_cfg. (3) Squash meminit_memcfg_spd().
BUG=b:124990009 BRANCH=none TEST=build
Change-Id: I686a85996858204c20fd05ef24787a0487817c34 Signed-off-by: Philip Chen philipchen@google.com --- M src/mainboard/google/hatch/romstage.c M src/mainboard/google/hatch/variants/baseboard/memory.c M src/mainboard/google/sarien/romstage.c M src/mainboard/intel/coffeelake_rvp/memory.c M src/mainboard/intel/coffeelake_rvp/romstage.c M src/soc/intel/cannonlake/cnl_memcfg_init.c M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h 7 files changed, 88 insertions(+), 82 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/32513/2
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32513
to look at the new patch set (#3).
Change subject: soc/intel/cannonlake: Clean up cannonlake_memcfg_init ......................................................................
soc/intel/cannonlake: Clean up cannonlake_memcfg_init
The major changes include: (1) Add enum 'mem_info_read_type' to spd_info. (2) Move spd_into into cnl_mb_cfg. (3) Squash meminit_memcfg_spd().
BUG=b:124990009 BRANCH=none TEST=none
Change-Id: I686a85996858204c20fd05ef24787a0487817c34 Signed-off-by: Philip Chen philipchen@google.com --- M src/mainboard/google/hatch/romstage.c M src/mainboard/google/hatch/variants/baseboard/memory.c M src/mainboard/google/sarien/romstage.c M src/mainboard/intel/coffeelake_rvp/memory.c M src/mainboard/intel/coffeelake_rvp/romstage.c M src/soc/intel/cannonlake/cnl_memcfg_init.c M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h 7 files changed, 88 insertions(+), 80 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/32513/3
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Clean up cannonlake_memcfg_init ......................................................................
Patch Set 3:
(1 comment)
thanks for working on this, please find the inline comment
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/include/soc... PS3, Line 62: struct spd_info spd; how do you support configurations where you have one SPD in CBFS and one stored on DDR4 EEPROM?
There should be one mem_info_read_type for each slot.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Clean up cannonlake_memcfg_init ......................................................................
Patch Set 3:
(6 comments)
https://review.coreboot.org/#/c/32513/3/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/baseboard/memory.c:
https://review.coreboot.org/#/c/32513/3/src/mainboard/google/hatch/variants/... PS3, Line 23: /* Access memory info using the spd file specified by spd_index */ : .spd = {.read_type = READ_SPD_CBFS}, I think its better to set this and spd_index in romstage.c since it will be the same for all hatch variants.
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/cnl_memcfg_... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/cnl_memcfg_... PS3, Line 106: if (!cnl_cfg->spd.spd_spec.spd_smbus_address[i]) : continue; Do we need this check? If the user is explicitly setting READ_SMBUS now, we should just copy whatever is set in spd_smbus_address.
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/cnl_memcfg_... PS3, Line 121: printk Probably add a die here? since this should not occur in production, but should be caught in development.
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/include/soc... PS3, Line 43: mem_info_read_type It would be helpful to add comments indicating what each type means.
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/include/soc... PS3, Line 52: spd_data_by A comment to explain how the data relates to the type would be helpful.
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/include/soc... PS3, Line 62: struct spd_info spd;
how do you support configurations where you have one SPD in CBFS and one stored on DDR4 EEPROM?
There should be one mem_info_read_type for each slot.
Currently, FSP does not support that. IIUC, if SMBUS address is provided, it does not use the spd data pointers: https://review.coreboot.org/cgit/coreboot.git/tree/src/vendorcode/intel/fsp/...
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Clean up cannonlake_memcfg_init ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/include/soc... PS3, Line 62: struct spd_info spd;
It's supported when you fetch the SPD in coreboot and point to it in heap. […]
Actually, looking back at the comments in the header file above, I think it is possible to do it with FSP i.e. each address entry is treated differently. In that case, we need to have spd_info for each slot rather than just read_type for each slot.
i.e. struct spd_info spd[MAX_SLOTS] which would be 4 for CNL and family.
Then each board needs to set the configuration it has.
e.g.: spd_info[] = { [0] = { .read_type = READ_SMBUS, .spd_smbus_address = 0xXX }, [1] = { .read_type = NOT_EXISTING }, [2] = { .read_type = READ_SPD_CBFS, .spd_index = X }, [3] = { .read_type = READ_SPD_MEMPTR, .spd_data_ptr_info = { ... } }, },
And then cannonlake_memcfg_init will have to address each slot based on the read_type for it.
In fact, cannonlake_memcfg_init also needs to set DisableDimmChannel0 and DisableDimmChannel1 as per the spd_info above i.e. if any read_type is NOT_EXISTING then that Dimm of the channel should be disabled.
Philip Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Clean up cannonlake_memcfg_init ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/include/soc... PS3, Line 62: struct spd_info spd;
Actually, looking back at the comments in the header file above, I think it is possible to do it wit […]
I don't think we can treat different slot independently. There is only one MemorySpdDataLen in FSPM_UPD, unless we can make sure MemorySpdDataLen is the same for each slot.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Clean up cannonlake_memcfg_init ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/include/soc... PS3, Line 62: struct spd_info spd;
I don't think we can treat different slot independently. […]
How is it supposed to work then? Please contact FSP developers, as those configurations are quite common.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Clean up cannonlake_memcfg_init ......................................................................
Patch Set 3:
(1 comment)
+Subrata +Rizwan
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/include/soc... PS3, Line 62: struct spd_info spd;
How is it supposed to work then? […]
As far as I can tell, the SPDs being used for all CNL boards have the same length for different memory types. You can add a die() in case that assumption is broken.
Philip Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Clean up cannonlake_memcfg_init ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/include/soc... PS3, Line 62: struct spd_info spd;
As far as I can tell, the SPDs being used for all CNL boards have the same length for different memo […]
SG. I'll upload a new patch shortly.
Hello Patrick Rudolph, Paul Fagerburg, Subrata Banik, Rizwan Qureshi, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32513
to look at the new patch set (#4).
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
soc/intel/cannonlake: Support different SPD read type for each slot
Also clean up cannonlake_memcfg_init.
The major changes include: (1) Add enum 'mem_info_read_type' to spd_info. (2) Add per-memory-slot spd_info to cnl_mb_cfg. (3) Setup memory config for each slot independently. (4) Squash meminit_memcfg_spd().
BUG=b:124990009 BRANCH=none TEST=none
Change-Id: I686a85996858204c20fd05ef24787a0487817c34 Signed-off-by: Philip Chen philipchen@google.com --- M src/mainboard/google/hatch/romstage.c M src/mainboard/google/hatch/variants/baseboard/memory.c M src/mainboard/google/hatch/variants/kohaku/memory.c M src/mainboard/google/sarien/romstage.c M src/mainboard/intel/coffeelake_rvp/memory.c M src/mainboard/intel/coffeelake_rvp/romstage.c M src/soc/intel/cannonlake/cnl_memcfg_init.c M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h 8 files changed, 164 insertions(+), 93 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/32513/4
Hello Patrick Rudolph, Paul Fagerburg, Subrata Banik, Rizwan Qureshi, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32513
to look at the new patch set (#5).
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
soc/intel/cannonlake: Support different SPD read type for each slot
Also clean up cannonlake_memcfg_init.
The major changes include: (1) Add enum 'mem_info_read_type' to spd_info. (2) Add per-memory-slot spd_info to cnl_mb_cfg. (3) Setup memory config for each slot independently. (4) Squash meminit_memcfg_spd().
BUG=b:124990009 BRANCH=none TEST=none
Change-Id: I686a85996858204c20fd05ef24787a0487817c34 Signed-off-by: Philip Chen philipchen@google.com --- M src/mainboard/google/hatch/romstage.c M src/mainboard/google/hatch/variants/baseboard/memory.c M src/mainboard/google/hatch/variants/kohaku/memory.c M src/mainboard/google/sarien/romstage.c M src/mainboard/intel/coffeelake_rvp/memory.c M src/mainboard/intel/coffeelake_rvp/romstage.c M src/soc/intel/cannonlake/cnl_memcfg_init.c M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h 8 files changed, 163 insertions(+), 93 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/32513/5
Philip Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/#/c/32513/3/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/baseboard/memory.c:
https://review.coreboot.org/#/c/32513/3/src/mainboard/google/hatch/variants/... PS3, Line 23: /* Access memory info using the spd file specified by spd_index */ : .spd = {.read_type = READ_SPD_CBFS},
I think its better to set this and spd_index in romstage. […]
I moved it to variant_memory_params() below because the per-slot spd info has dependence on whether single channel or not.
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/cnl_memcfg_... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/cnl_memcfg_... PS3, Line 106: if (!cnl_cfg->spd.spd_spec.spd_smbus_address[i]) : continue;
Do we need this check? If the user is explicitly setting READ_SMBUS now, we should just copy whateve […]
Removed
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/cnl_memcfg_... PS3, Line 121: printk
Probably add a die here? since this should not occur in production, but should be caught in developm […]
Done
Philip Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/include/soc... PS3, Line 43: mem_info_read_type
It would be helpful to add comments indicating what each type means.
Done
https://review.coreboot.org/#/c/32513/3/src/soc/intel/cannonlake/include/soc... PS3, Line 52: spd_data_by
A comment to explain how the data relates to the type would be helpful.
Done
Hello Patrick Rudolph, Paul Fagerburg, Subrata Banik, Rizwan Qureshi, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32513
to look at the new patch set (#6).
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
soc/intel/cannonlake: Support different SPD read type for each slot
Also clean up cannonlake_memcfg_init.
The major changes include: (1) Add enum 'mem_info_read_type' to spd_info. (2) Add per-memory-slot spd_info to cnl_mb_cfg. (3) Setup memory config for each slot independently. (4) Squash meminit_memcfg_spd().
BUG=b:124990009 BRANCH=none TEST=none
Change-Id: I686a85996858204c20fd05ef24787a0487817c34 Signed-off-by: Philip Chen philipchen@google.com --- M src/mainboard/google/hatch/romstage.c M src/mainboard/google/hatch/variants/baseboard/memory.c M src/mainboard/google/hatch/variants/kohaku/memory.c M src/mainboard/google/sarien/romstage.c M src/mainboard/intel/coffeelake_rvp/memory.c M src/mainboard/intel/coffeelake_rvp/romstage.c M src/soc/intel/cannonlake/cnl_memcfg_init.c M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h 8 files changed, 160 insertions(+), 93 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/32513/6
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/cnl_memcfg_... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/cnl_memcfg_... PS6, Line 77: break; see comment in cnl_memcfg_init. With channel_empty dropped, there's no need to pass struct cnl_mb_cfg *cnl_cfg any more.
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/cnl_memcfg_... PS6, Line 100: mem_slot add a define to the header for the amount of slots
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/include/soc... PS6, Line 130: * to indicate that the channel is populated. For example, can be removed, as spd now contains that information
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/#/c/32513/6/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/baseboard/memory.c:
https://review.coreboot.org/#/c/32513/6/src/mainboard/google/hatch/variants/... PS6, Line 60: for (int i = 0; i < ARRAY_SIZE(bcfg->spd); i++) { If bcfg->spd has more than 4 elements, the calculation to index into bcfg->channel_empty[] will read past the end of that array. Please use an assert() or a die() to protect against this failure case.
https://review.coreboot.org/#/c/32513/6/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/kohaku/memory.c:
https://review.coreboot.org/#/c/32513/6/src/mainboard/google/hatch/variants/... PS6, Line 90: for (int i = 0; i < ARRAY_SIZE(bcfg->spd); i++) { Same comment as for baseboard/memory.c line 60
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/include/soc... PS6, Line 67: struct spd_info spd[4]; The #define that Patrick Rudolph requested in cnl_memcfg_init.c line 60, please use that in the array definition here.
Philip Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/include/soc... PS6, Line 130: * to indicate that the channel is populated. For example,
can be removed, as spd now contains that information
Are you saying we can just remove channel_empty[] here? Furquan/Tim/Paul, if so, do we still need a strapping GPIO pin for single/dual channel memory skus?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/cnl_memcfg_... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/cnl_memcfg_... PS6, Line 144: No valid way to read mem info. Just put this in die?
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/include/soc... PS6, Line 130: * to indicate that the channel is populated. For example,
Are you saying we can just remove channel_empty[] here? Furquan/Tim/Paul, if so, do we still need a strapping GPIO pin for single/dual channel memory skus?
What Patrick meant is that spd_info is being filled for each slot which means that it already provides information about which channel is populated and which is not. However, in order to fill spd_info, you would still need the strapping GPIO on hatch.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/cnl_memcfg_... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/cnl_memcfg_... PS6, Line 82: printk(BIOS_EMERG, "nonexistent memory slot: %u", mem_slot); : die(""); Same thing here, just pass that message to die(); I don't think the slot number is necessary.
Hello Patrick Rudolph, Paul Fagerburg, Subrata Banik, Tim Wawrzynczak, Rizwan Qureshi, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32513
to look at the new patch set (#7).
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
soc/intel/cannonlake: Support different SPD read type for each slot
Also clean up cannonlake_memcfg_init.
The major changes include: (1) Add enum 'mem_info_read_type' to spd_info. (2) Add per-dimm-slot spd_info to cnl_mb_cfg. (3) Setup memory config for each slot independently. (4) Squash meminit_memcfg_spd().
BUG=chromium:960581, b:124990009 BRANCH=none TEST=none
Change-Id: I686a85996858204c20fd05ef24787a0487817c34 Signed-off-by: Philip Chen philipchen@google.com --- M src/mainboard/google/hatch/romstage.c M src/mainboard/google/hatch/variants/baseboard/memory.c M src/mainboard/google/hatch/variants/kohaku/memory.c M src/mainboard/google/sarien/romstage.c M src/mainboard/intel/coffeelake_rvp/memory.c M src/mainboard/intel/coffeelake_rvp/romstage.c M src/soc/intel/cannonlake/cnl_memcfg_init.c M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h 8 files changed, 163 insertions(+), 130 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/32513/7
Philip Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 6:
(8 comments)
https://review.coreboot.org/#/c/32513/6/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/baseboard/memory.c:
https://review.coreboot.org/#/c/32513/6/src/mainboard/google/hatch/variants/... PS6, Line 60: for (int i = 0; i < ARRAY_SIZE(bcfg->spd); i++) {
If bcfg->spd has more than 4 elements, the calculation to index into bcfg->channel_empty[] will read […]
Removed channel_empty.
https://review.coreboot.org/#/c/32513/6/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/kohaku/memory.c:
https://review.coreboot.org/#/c/32513/6/src/mainboard/google/hatch/variants/... PS6, Line 90: for (int i = 0; i < ARRAY_SIZE(bcfg->spd); i++) {
Same comment as for baseboard/memory. […]
Done
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/cnl_memcfg_... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/cnl_memcfg_... PS6, Line 77: break;
see comment in cnl_memcfg_init. […]
Done
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/cnl_memcfg_... PS6, Line 82: printk(BIOS_EMERG, "nonexistent memory slot: %u", mem_slot); : die("");
Same thing here, just pass that message to die(); I don't think the slot number is necessary.
Done
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/cnl_memcfg_... PS6, Line 100: mem_slot
add a define to the header for the amount of slots
Done
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/cnl_memcfg_... PS6, Line 144: No valid way to read mem info.
Just put this in die?
Done
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/include/soc... PS6, Line 67: struct spd_info spd[4];
The #define that Patrick Rudolph requested in cnl_memcfg_init. […]
Done
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/include/soc... PS6, Line 130: * to indicate that the channel is populated. For example,
Are you saying we can just remove channel_empty[] here? […]
I see. Thanks for the explanation.
Hello Patrick Rudolph, Paul Fagerburg, Subrata Banik, Tim Wawrzynczak, Rizwan Qureshi, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32513
to look at the new patch set (#8).
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
soc/intel/cannonlake: Support different SPD read type for each slot
Also clean up cannonlake_memcfg_init.
The major changes include: (1) Add enum 'mem_info_read_type' to spd_info. (2) Add per-dimm-slot spd_info to cnl_mb_cfg. (3) Setup memory config for each slot independently. (4) Squash meminit_memcfg_spd().
BUG=chromium:960581, b:124990009 BRANCH=none TEST=none
Change-Id: I686a85996858204c20fd05ef24787a0487817c34 Signed-off-by: Philip Chen philipchen@google.com --- M src/mainboard/google/hatch/romstage.c M src/mainboard/google/hatch/variants/baseboard/memory.c M src/mainboard/google/hatch/variants/kohaku/memory.c M src/mainboard/google/sarien/romstage.c M src/mainboard/intel/coffeelake_rvp/memory.c M src/mainboard/intel/coffeelake_rvp/romstage.c M src/soc/intel/cannonlake/cnl_memcfg_init.c M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h 8 files changed, 163 insertions(+), 130 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/32513/8
Philip Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:
https://review.coreboot.org/#/c/32513/6/src/soc/intel/cannonlake/include/soc... PS6, Line 130: * to indicate that the channel is populated. For example,
I see. Thanks for the explanation.
Done
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 8: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 8: Code-Review-1
(5 comments)
https://review.coreboot.org/#/c/32513/8/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/kohaku/memory.c:
https://review.coreboot.org/#/c/32513/8/src/mainboard/google/hatch/variants/... PS8, Line 66: __weak Why is weak added here?
https://review.coreboot.org/#/c/32513/8/src/mainboard/google/hatch/variants/... PS8, Line 80: for (int i = 0; i < NUM_DIMM_SLOT; i++) { : if (is_single_ch_mem && i >= (NUM_DIMM_SLOT / 2)) : bcfg->spd[i].read_type = NOT_EXISTING; : else { : bcfg->spd[i].read_type = READ_SPD_CBFS; : bcfg->spd[i].spd_spec.spd_index = mem_sku; : } This is not correct. Kohaku and Hatch need to just fill in spd[0] and spd[2]. spd[1] and spd[3] are always NOT_EXISTING. You can just do something like this:
bcfg->spd[0].read_type = READ_SPD_CBFS; bcfg->spd[0].spd_spec.spd_index = mem_sku;
if (!is_single_ch_mem) { bcfg->spd[2].read_type = READ_SPD_CBFS; bcfg->spd[2].spd_spec.spd_index = mem_sku; }
In fact, everything except memcpy can be done in mainboard_memory_init_params in romstage.c since it is the same for kohaku and hatch.
https://review.coreboot.org/#/c/32513/8/src/soc/intel/cannonlake/cnl_memcfg_... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/#/c/32513/8/src/soc/intel/cannonlake/cnl_memcfg_... PS8, Line 114: struct spd_info spdi; Just use a pointer? struct spd_info *spdi;
https://review.coreboot.org/#/c/32513/8/src/soc/intel/cannonlake/cnl_memcfg_... PS8, Line 131: cnl_cfg Why not just pass in spd_index here?
https://review.coreboot.org/#/c/32513/8/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:
https://review.coreboot.org/#/c/32513/8/src/soc/intel/cannonlake/include/soc... PS8, Line 62: MEMMPTR nit: MEMPTR
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 8: Code-Review-1
(2 comments)
https://review.coreboot.org/#/c/32513/8/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/kohaku/memory.c:
https://review.coreboot.org/#/c/32513/8/src/mainboard/google/hatch/variants/... PS8, Line 80: for (int i = 0; i < NUM_DIMM_SLOT; i++) { : if (is_single_ch_mem && i >= (NUM_DIMM_SLOT / 2)) : bcfg->spd[i].read_type = NOT_EXISTING; : else { : bcfg->spd[i].read_type = READ_SPD_CBFS; : bcfg->spd[i].spd_spec.spd_index = mem_sku; : }
This is not correct. Kohaku and Hatch need to just fill in spd[0] and spd[2]. […]
What Furquan said. Just make sure to add a comment to explain why spd[1] and spd[3] are never modified.
https://review.coreboot.org/#/c/32513/8/src/soc/intel/cannonlake/cnl_memcfg_... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/#/c/32513/8/src/soc/intel/cannonlake/cnl_memcfg_... PS8, Line 122: spdi = cnl_cfg->spd[i]; As Furquan suggested, use a pointer. This assignment is doing a bitwise-copy of the entire struct because spdi is declared as a struct instead of a pointer to a struct. It would be more efficient to have a pointer to a struct.
Hello Patrick Rudolph, Paul Fagerburg, Subrata Banik, Tim Wawrzynczak, Rizwan Qureshi, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32513
to look at the new patch set (#9).
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
soc/intel/cannonlake: Support different SPD read type for each slot
Also clean up cannonlake_memcfg_init.
The major changes include: (1) Add enum 'mem_info_read_type' to spd_info. (2) Add per-dimm-slot spd_info to cnl_mb_cfg. (3) Setup memory config for each slot independently. (4) Squash meminit_memcfg_spd().
BUG=chromium:960581, b:124990009 BRANCH=none TEST=none
Change-Id: I686a85996858204c20fd05ef24787a0487817c34 Signed-off-by: Philip Chen philipchen@google.com --- M src/mainboard/google/hatch/romstage.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/gpio.h M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/hatch/variants/baseboard/memory.c M src/mainboard/google/hatch/variants/kohaku/memory.c M src/mainboard/google/sarien/romstage.c M src/mainboard/intel/coffeelake_rvp/memory.c M src/mainboard/intel/coffeelake_rvp/romstage.c M src/soc/intel/cannonlake/cnl_memcfg_init.c M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h 10 files changed, 169 insertions(+), 161 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/32513/9
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/#/c/32513/9/src/mainboard/google/hatch/romstage.... File src/mainboard/google/hatch/romstage.c:
https://review.coreboot.org/#/c/32513/9/src/mainboard/google/hatch/romstage.... PS9, Line 57: /* trailing whitespace
https://review.coreboot.org/#/c/32513/9/src/mainboard/google/hatch/romstage.... PS9, Line 65: memcfg.spd[2].read_type = READ_SPD_CBFS; trailing whitespace
Hello Patrick Rudolph, Paul Fagerburg, Subrata Banik, Tim Wawrzynczak, Rizwan Qureshi, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32513
to look at the new patch set (#10).
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
soc/intel/cannonlake: Support different SPD read type for each slot
Also clean up cannonlake_memcfg_init.
The major changes include: (1) Add enum 'mem_info_read_type' to spd_info. (2) Add per-dimm-slot spd_info to cnl_mb_cfg. (3) Setup memory config for each slot independently. (4) Squash meminit_memcfg_spd().
BUG=chromium:960581, b:124990009 BRANCH=none TEST=none
Change-Id: I686a85996858204c20fd05ef24787a0487817c34 Signed-off-by: Philip Chen philipchen@google.com --- M src/mainboard/google/hatch/romstage.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/gpio.h M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/hatch/variants/baseboard/memory.c M src/mainboard/google/hatch/variants/kohaku/memory.c M src/mainboard/google/sarien/romstage.c M src/mainboard/intel/coffeelake_rvp/memory.c M src/mainboard/intel/coffeelake_rvp/romstage.c M src/soc/intel/cannonlake/cnl_memcfg_init.c M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h 10 files changed, 169 insertions(+), 161 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/32513/10
Philip Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 10:
(5 comments)
https://review.coreboot.org/#/c/32513/8/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/kohaku/memory.c:
https://review.coreboot.org/#/c/32513/8/src/mainboard/google/hatch/variants/... PS8, Line 66: __weak
Why is weak added here?
My fault over c&p. Just removed it.
https://review.coreboot.org/#/c/32513/8/src/mainboard/google/hatch/variants/... PS8, Line 80: for (int i = 0; i < NUM_DIMM_SLOT; i++) { : if (is_single_ch_mem && i >= (NUM_DIMM_SLOT / 2)) : bcfg->spd[i].read_type = NOT_EXISTING; : else { : bcfg->spd[i].read_type = READ_SPD_CBFS; : bcfg->spd[i].spd_spec.spd_index = mem_sku; : }
What Furquan said. […]
Fixed and removed the common code to romstage.c. Thanks for pointing out this error.
https://review.coreboot.org/#/c/32513/8/src/soc/intel/cannonlake/cnl_memcfg_... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/#/c/32513/8/src/soc/intel/cannonlake/cnl_memcfg_... PS8, Line 114: struct spd_info spdi;
Just use a pointer? struct spd_info *spdi;
Done
https://review.coreboot.org/#/c/32513/8/src/soc/intel/cannonlake/cnl_memcfg_... PS8, Line 122: spdi = cnl_cfg->spd[i];
As Furquan suggested, use a pointer. […]
Done
https://review.coreboot.org/#/c/32513/8/src/soc/intel/cannonlake/cnl_memcfg_... PS8, Line 131: cnl_cfg
Why not just pass in spd_index here?
Good idea. Fixed.
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 10: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/32513/10/src/mainboard/google/hatch/romstage... File src/mainboard/google/hatch/romstage.c:
https://review.coreboot.org/#/c/32513/10/src/mainboard/google/hatch/romstage... PS10, Line 55: is_single_ch_mem = gpio_get(GPP_F2); nit: please add a #define above (with GPIO_MEM_CONFIG_[0:3]) instead of using GPP_F2 here.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 10: Code-Review+1
Hello Patrick Rudolph, Paul Fagerburg, Subrata Banik, Tim Wawrzynczak, Rizwan Qureshi, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32513
to look at the new patch set (#11).
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
soc/intel/cannonlake: Support different SPD read type for each slot
Also clean up cannonlake_memcfg_init.
The major changes include: (1) Add enum 'mem_info_read_type' to spd_info. (2) Add per-dimm-slot spd_info to cnl_mb_cfg. (3) Setup memory config for each slot independently. (4) Squash meminit_memcfg_spd().
BUG=chromium:960581, b:124990009 BRANCH=none TEST=none
Change-Id: I686a85996858204c20fd05ef24787a0487817c34 Signed-off-by: Philip Chen philipchen@google.com --- M src/mainboard/google/hatch/romstage.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/gpio.h M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/hatch/variants/baseboard/memory.c M src/mainboard/google/hatch/variants/kohaku/memory.c M src/mainboard/google/sarien/romstage.c M src/mainboard/intel/coffeelake_rvp/memory.c M src/mainboard/intel/coffeelake_rvp/romstage.c M src/soc/intel/cannonlake/cnl_memcfg_init.c M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h 10 files changed, 175 insertions(+), 161 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/32513/11
Philip Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/#/c/32513/10/src/mainboard/google/hatch/romstage... File src/mainboard/google/hatch/romstage.c:
https://review.coreboot.org/#/c/32513/10/src/mainboard/google/hatch/romstage... PS10, Line 55: is_single_ch_mem = gpio_get(GPP_F2);
nit: please add a #define above (with GPIO_MEM_CONFIG_[0:3]) instead of using GPP_F2 here.
Done
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 11: Code-Review+1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 11: Code-Review+2
(2 comments)
https://review.coreboot.org/#/c/32513/11/src/mainboard/google/hatch/romstage... File src/mainboard/google/hatch/romstage.c:
https://review.coreboot.org/#/c/32513/11/src/mainboard/google/hatch/romstage... PS11, Line 37: memory_sku So, one of the reasons why this was in baseboard defined as a __weak function was to allow any variants to define this differently if they choose different GPIOs or use a different way for identifying memory strap. Currently, the variants we are working on use the mem strap GPIOs the same way, but may not have the single channel strap. Its okay if you want to change this later, but we might need something like variant_is_single_channel_mem() that basically returns false on the variants that do not support it and baseboard weak function that reads GPP_F2 and returns value appropriately.
https://review.coreboot.org/#/c/32513/11/src/soc/intel/cannonlake/cnl_memcfg... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/#/c/32513/11/src/soc/intel/cannonlake/cnl_memcfg... PS11, Line 95: assert(mem_slot < NUM_DIMM_SLOT); Is this really required?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/#/c/32513/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32513/11//COMMIT_MSG@19 PS11, Line 19: none Can you please test to make sure that hatch_whl, hatch and kohaku boot fine?
Philip Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 11:
(3 comments)
https://review.coreboot.org/#/c/32513/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32513/11//COMMIT_MSG@19 PS11, Line 19: none
Can you please test to make sure that hatch_whl, hatch and kohaku boot fine?
Sure
https://review.coreboot.org/#/c/32513/11/src/mainboard/google/hatch/romstage... File src/mainboard/google/hatch/romstage.c:
https://review.coreboot.org/#/c/32513/11/src/mainboard/google/hatch/romstage... PS11, Line 37: memory_sku
So, one of the reasons why this was in baseboard defined as a __weak function was to allow any varia […]
OK
https://review.coreboot.org/#/c/32513/11/src/soc/intel/cannonlake/cnl_memcfg... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/#/c/32513/11/src/soc/intel/cannonlake/cnl_memcfg... PS11, Line 95: assert(mem_slot < NUM_DIMM_SLOT);
Is this really required?
It helps in case that someone adds code in this file later and calls meminit_cbfs_spd_index() with an out-of-bound mem_slot. Anyway it's no harm to keep the assert here.
Hello Patrick Rudolph, Paul Fagerburg, Subrata Banik, Tim Wawrzynczak, Rizwan Qureshi, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32513
to look at the new patch set (#12).
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
soc/intel/cannonlake: Support different SPD read type for each slot
Also clean up cannonlake_memcfg_init.
The major changes include: (1) Add enum 'mem_info_read_type' to spd_info. (2) Add per-dimm-slot spd_info to cnl_mb_cfg. (3) Setup memory config for each slot independently. (4) Squash meminit_memcfg_spd().
BUG=chromium:960581, b:124990009 BRANCH=none TEST=none
Change-Id: I686a85996858204c20fd05ef24787a0487817c34 Signed-off-by: Philip Chen philipchen@google.com --- M src/mainboard/google/hatch/romstage.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/gpio.h M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/hatch/variants/baseboard/memory.c M src/mainboard/google/hatch/variants/kohaku/memory.c M src/mainboard/google/sarien/romstage.c M src/mainboard/intel/coffeelake_rvp/memory.c M src/mainboard/intel/coffeelake_rvp/romstage.c M src/soc/intel/cannonlake/cnl_memcfg_init.c M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h 10 files changed, 177 insertions(+), 161 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/32513/12
Hello Patrick Rudolph, Paul Fagerburg, Subrata Banik, Tim Wawrzynczak, Rizwan Qureshi, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32513
to look at the new patch set (#13).
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
soc/intel/cannonlake: Support different SPD read type for each slot
Also clean up cannonlake_memcfg_init.
The major changes include: (1) Add enum 'mem_info_read_type' to spd_info. (2) Add per-dimm-slot spd_info to cnl_mb_cfg. (3) Setup memory config for each slot independently. (4) Squash meminit_memcfg_spd().
BUG=chromium:960581, b:124990009 BRANCH=none TEST=boot hatch, hatch_whl, and kohaku
Change-Id: I686a85996858204c20fd05ef24787a0487817c34 Signed-off-by: Philip Chen philipchen@google.com --- M src/mainboard/google/hatch/romstage.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/gpio.h M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/hatch/variants/baseboard/memory.c M src/mainboard/google/hatch/variants/kohaku/memory.c M src/mainboard/google/sarien/romstage.c M src/mainboard/intel/coffeelake_rvp/memory.c M src/mainboard/intel/coffeelake_rvp/romstage.c M src/soc/intel/cannonlake/cnl_memcfg_init.c M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h 10 files changed, 177 insertions(+), 161 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/32513/13
Philip Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/#/c/32513/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32513/11//COMMIT_MSG@19 PS11, Line 19: none
Sure
Verified on hatch, hatch_whl, and kohaku.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 14: Code-Review+2
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 14:
(3 comments)
https://review.coreboot.org/#/c/32513/13/src/soc/intel/cannonlake/cnl_memcfg... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/#/c/32513/13/src/soc/intel/cannonlake/cnl_memcfg... PS13, Line 57: static size_t last_set_spd_data_len; You should explicitly initialize the variable to 0.
https://review.coreboot.org/#/c/32513/13/src/soc/intel/cannonlake/cnl_memcfg... PS13, Line 59: assert(spd_data_ptr && spd_data_len); Please explicitly test spd_data_len != 0 instead of relying on C language behavior to convert an integer from zero/non-zero to false/true.
https://review.coreboot.org/#/c/32513/13/src/soc/intel/cannonlake/cnl_memcfg... PS13, Line 61: if (last_set_spd_data_len && last_set_spd_data_len != spd_data_len) Please explicitly test last_set_spd_data_len != 0. Also, this could fail if last_set_spd_data_len is uninitialized and you happen to get something non-zero in that memory.
Philip Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/#/c/32513/13/src/soc/intel/cannonlake/cnl_memcfg... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/#/c/32513/13/src/soc/intel/cannonlake/cnl_memcfg... PS13, Line 57: static size_t last_set_spd_data_len;
You should explicitly initialize the variable to 0.
Wouldn't it be automatically initialized to 0 anyway?
Philip Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/#/c/32513/13/src/soc/intel/cannonlake/cnl_memcfg... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/#/c/32513/13/src/soc/intel/cannonlake/cnl_memcfg... PS13, Line 57: static size_t last_set_spd_data_len;
Wouldn't it be automatically initialized to 0 anyway?
Or you're worried about a potential compiler problem?
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/#/c/32513/13/src/soc/intel/cannonlake/cnl_memcfg... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/#/c/32513/13/src/soc/intel/cannonlake/cnl_memcfg... PS13, Line 57: static size_t last_set_spd_data_len;
Or you're worried about a potential compiler problem?
The variable is static, so it will have a definite memory address. If the compiler puts it into a zero-init section, then yes, it will be zero. But if it's in an uninitialized section of memory, the contents are undefined. It is safer to explicitly initialize it. If you initialize to 0 as part of the decl, it won't even result in any extra code in the executable.
Hello Patrick Rudolph, Paul Fagerburg, Subrata Banik, Tim Wawrzynczak, Rizwan Qureshi, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32513
to look at the new patch set (#15).
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
soc/intel/cannonlake: Support different SPD read type for each slot
Also clean up cannonlake_memcfg_init.
The major changes include: (1) Add enum 'mem_info_read_type' to spd_info. (2) Add per-dimm-slot spd_info to cnl_mb_cfg. (3) Setup memory config for each slot independently. (4) Squash meminit_memcfg_spd().
BUG=chromium:960581, b:124990009 BRANCH=none TEST=boot hatch, hatch_whl, and kohaku
Change-Id: I686a85996858204c20fd05ef24787a0487817c34 Signed-off-by: Philip Chen philipchen@google.com --- M src/mainboard/google/hatch/romstage.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/gpio.h M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/hatch/variants/baseboard/memory.c M src/mainboard/google/hatch/variants/kohaku/memory.c M src/mainboard/google/sarien/romstage.c M src/mainboard/intel/coffeelake_rvp/memory.c M src/mainboard/intel/coffeelake_rvp/romstage.c M src/soc/intel/cannonlake/cnl_memcfg_init.c M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h 10 files changed, 179 insertions(+), 162 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/32513/15
Philip Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 15:
(3 comments)
https://review.coreboot.org/#/c/32513/13/src/soc/intel/cannonlake/cnl_memcfg... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/#/c/32513/13/src/soc/intel/cannonlake/cnl_memcfg... PS13, Line 57: static size_t last_set_spd_data_len;
The variable is static, so it will have a definite memory address. […]
OK, SGTM.
https://review.coreboot.org/#/c/32513/13/src/soc/intel/cannonlake/cnl_memcfg... PS13, Line 59: assert(spd_data_ptr && spd_data_len);
Please explicitly test spd_data_len != 0 instead of relying on C language behavior to convert an int […]
Done
https://review.coreboot.org/#/c/32513/13/src/soc/intel/cannonlake/cnl_memcfg... PS13, Line 61: if (last_set_spd_data_len && last_set_spd_data_len != spd_data_len)
Please explicitly test last_set_spd_data_len != 0. […]
Done
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 15: Code-Review+1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 15: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/32513/13/src/soc/intel/cannonlake/cnl_memcfg... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/#/c/32513/13/src/soc/intel/cannonlake/cnl_memcfg... PS13, Line 57: static size_t last_set_spd_data_len;
OK, SGTM.
Static variables should end up in .bss which is initialized to 0. So, by default this will be set to 0.
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
Patch Set 15:
(1 comment)
Still +1, because I don't have +2 in this gerrit.
https://review.coreboot.org/#/c/32513/13/src/soc/intel/cannonlake/cnl_memcfg... File src/soc/intel/cannonlake/cnl_memcfg_init.c:
https://review.coreboot.org/#/c/32513/13/src/soc/intel/cannonlake/cnl_memcfg... PS13, Line 57: static size_t last_set_spd_data_len;
Static variables should end up in .bss which is initialized to 0. […]
The MISRA in me says to initialize it regardless. But Furquan is right; it will get initialized to 0 by the compiler that we're currently using.
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32513 )
Change subject: soc/intel/cannonlake: Support different SPD read type for each slot ......................................................................
soc/intel/cannonlake: Support different SPD read type for each slot
Also clean up cannonlake_memcfg_init.
The major changes include: (1) Add enum 'mem_info_read_type' to spd_info. (2) Add per-dimm-slot spd_info to cnl_mb_cfg. (3) Setup memory config for each slot independently. (4) Squash meminit_memcfg_spd().
BUG=chromium:960581, b:124990009 BRANCH=none TEST=boot hatch, hatch_whl, and kohaku
Change-Id: I686a85996858204c20fd05ef24787a0487817c34 Signed-off-by: Philip Chen philipchen@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32513 Reviewed-by: Paul Fagerburg pfagerburg@chromium.org Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/hatch/romstage.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/gpio.h M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/hatch/variants/baseboard/memory.c M src/mainboard/google/hatch/variants/kohaku/memory.c M src/mainboard/google/sarien/romstage.c M src/mainboard/intel/coffeelake_rvp/memory.c M src/mainboard/intel/coffeelake_rvp/romstage.c M src/soc/intel/cannonlake/cnl_memcfg_init.c M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h 10 files changed, 179 insertions(+), 162 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Paul Fagerburg: Looks good to me, but someone else must approve
diff --git a/src/mainboard/google/hatch/romstage.c b/src/mainboard/google/hatch/romstage.c index bdf951b..2c630a8 100644 --- a/src/mainboard/google/hatch/romstage.c +++ b/src/mainboard/google/hatch/romstage.c @@ -16,23 +16,63 @@ #include <baseboard/variants.h> #include <console/console.h> #include <ec/google/chromeec/ec.h> +#include <gpio.h> #include <memory_info.h> #include <soc/cnl_memcfg_init.h> #include <soc/romstage.h> #include <string.h>
+/* Memory configuration board straps */ +#define GPIO_MEM_CONFIG_0 GPP_F20 +#define GPIO_MEM_CONFIG_1 GPP_F21 +#define GPIO_MEM_CONFIG_2 GPP_F11 +#define GPIO_MEM_CONFIG_3 GPP_F22 + +/* + * GPIO_MEM_CH_SEL is set to 1 for single channel skus + * and 0 for dual channel skus. + */ +#define GPIO_MEM_CH_SEL GPP_F2 + +static int memory_sku(void) +{ + const gpio_t spd_gpios[] = { + GPIO_MEM_CONFIG_0, + GPIO_MEM_CONFIG_1, + GPIO_MEM_CONFIG_2, + GPIO_MEM_CONFIG_3, + }; + + return gpio_base2_value(spd_gpios, ARRAY_SIZE(spd_gpios)); +} + void mainboard_memory_init_params(FSPM_UPD *memupd) { struct cnl_mb_cfg memcfg; - - const struct spd_info spd = { - .spd_by_index = true, - .spd_spec.spd_index = variant_memory_sku(), - }; + int mem_sku; + int is_single_ch_mem;
variant_memory_params(&memcfg); - cannonlake_memcfg_init(&memupd->FspmConfig, - &memcfg, &spd); + mem_sku = memory_sku(); + /* + * GPP_F2 is the MEM_CH_SEL gpio, which is set to 1 for single + * channel skus and 0 for dual channel skus. + */ + is_single_ch_mem = gpio_get(GPIO_MEM_CH_SEL); + + /* + * spd[0]-spd[3] map to CH0D0, CH0D1, CH1D0, CH1D1 respectively. + * Dual-DIMM memory is not used in hatch family, so we only + * fill in spd_info for CH0D0 and CH1D0 here. + */ + memcfg.spd[0].read_type = READ_SPD_CBFS; + memcfg.spd[0].spd_spec.spd_index = mem_sku; + if (!is_single_ch_mem) { + memcfg.spd[2].read_type = READ_SPD_CBFS; + memcfg.spd[2].spd_spec.spd_index = mem_sku; + } + + cannonlake_memcfg_init(&memupd->FspmConfig, &memcfg); }
void mainboard_get_dram_part_num(const char **part_num, size_t *len) diff --git a/src/mainboard/google/hatch/variants/baseboard/include/baseboard/gpio.h b/src/mainboard/google/hatch/variants/baseboard/include/baseboard/gpio.h index 5d6311b..e83732c 100644 --- a/src/mainboard/google/hatch/variants/baseboard/include/baseboard/gpio.h +++ b/src/mainboard/google/hatch/variants/baseboard/include/baseboard/gpio.h @@ -22,13 +22,6 @@
#define GPIO_PCH_WP GPP_C20
-/* Memory configuration board straps */ -#define GPIO_MEM_CONFIG_0 GPP_F20 -#define GPIO_MEM_CONFIG_1 GPP_F21 -#define GPIO_MEM_CONFIG_2 GPP_F11 -#define GPIO_MEM_CONFIG_3 GPP_F22 - - /* EC wake pin is LAN_WAKE# */ #define GPE_EC_WAKE GPE0_LAN_WAK
diff --git a/src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h b/src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h index d41ad53..17bd5df 100644 --- a/src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h +++ b/src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h @@ -31,9 +31,6 @@ const struct pad_config *override_gpio_table(size_t *num); const struct pad_config *override_early_gpio_table(size_t *num);
-/* Return memory SKU for the board. */ -int variant_memory_sku(void); - /* Return board specific memory configuration */ void variant_memory_params(struct cnl_mb_cfg *bcfg);
diff --git a/src/mainboard/google/hatch/variants/baseboard/memory.c b/src/mainboard/google/hatch/variants/baseboard/memory.c index 580bdc9..bcfc49f 100644 --- a/src/mainboard/google/hatch/variants/baseboard/memory.c +++ b/src/mainboard/google/hatch/variants/baseboard/memory.c @@ -15,16 +15,15 @@
#include <baseboard/variants.h> #include <baseboard/gpio.h> -#include <gpio.h> #include <soc/cnl_memcfg_init.h> #include <string.h>
static const struct cnl_mb_cfg baseboard_memcfg = { /* Baseboard uses 121, 81 and 100 rcomp resistors */ - .rcomp_resistor = { 121, 81, 100 }, + .rcomp_resistor = {121, 81, 100},
/* Baseboard Rcomp target values */ - .rcomp_targets = { 100, 40, 20, 20, 26 }, + .rcomp_targets = {100, 40, 20, 20, 26},
/* Set CaVref config to 2 */ .vref_ca_config = 2, @@ -36,32 +35,4 @@ void __weak variant_memory_params(struct cnl_mb_cfg *bcfg) { memcpy(bcfg, &baseboard_memcfg, sizeof(baseboard_memcfg)); - /* - * GPP_F2 is the MEM_CH_SEL gpio, which is set to 1 for single - * channel skus and 0 for dual channel skus. - */ - if (gpio_get(GPP_F2) == 1) { - /* - * Single channel config: for Hatch, Channel 0 is - * always populated. - */ - bcfg->channel_empty[0] = 0; - bcfg->channel_empty[1] = 1; - } else { - /* Dual channel config: both channels populated. */ - bcfg->channel_empty[0] = 0; - bcfg->channel_empty[1] = 0; - } -} - -int __weak variant_memory_sku(void) -{ - const gpio_t spd_gpios[] = { - GPIO_MEM_CONFIG_0, - GPIO_MEM_CONFIG_1, - GPIO_MEM_CONFIG_2, - GPIO_MEM_CONFIG_3, - }; - - return gpio_base2_value(spd_gpios, ARRAY_SIZE(spd_gpios)); } diff --git a/src/mainboard/google/hatch/variants/kohaku/memory.c b/src/mainboard/google/hatch/variants/kohaku/memory.c index 27ae3d8..4901247 100644 --- a/src/mainboard/google/hatch/variants/kohaku/memory.c +++ b/src/mainboard/google/hatch/variants/kohaku/memory.c @@ -15,7 +15,6 @@
#include <baseboard/variants.h> #include <baseboard/gpio.h> -#include <gpio.h> #include <soc/cnl_memcfg_init.h> #include <string.h>
@@ -30,8 +29,8 @@ * the index = pin number on SoC * the value = pin number on lpddr3 part */ - .dqs_map[DDR_CH0] = { 0, 1, 3, 2, 5, 7, 6, 4 }, - .dqs_map[DDR_CH1] = { 1, 3, 2, 0, 5, 7, 6, 4 }, + .dqs_map[DDR_CH0] = {0, 1, 3, 2, 5, 7, 6, 4}, + .dqs_map[DDR_CH1] = {1, 3, 2, 0, 5, 7, 6, 4},
.dq_map[DDR_CH0] = { {0xf, 0xf0}, @@ -40,7 +39,7 @@ {0xf, 0x0}, {0xff, 0x0}, {0xff, 0x0} - }, + }, .dq_map[DDR_CH1] = { {0xf, 0xf0}, {0x0, 0xf0}, @@ -48,13 +47,13 @@ {0xf, 0x0}, {0xff, 0x0}, {0xff, 0x0} - }, + },
/* Kohaku uses 200, 80.6 and 162 rcomp resistors */ - .rcomp_resistor = { 200, 81, 162 }, + .rcomp_resistor = {200, 81, 162},
/* Kohaku Rcomp target values */ - .rcomp_targets = { 100, 40, 40, 23, 40 }, + .rcomp_targets = {100, 40, 40, 23, 40},
/* Set CaVref config to 0 for LPDDR3 */ .vref_ca_config = 0, @@ -66,20 +65,4 @@ void variant_memory_params(struct cnl_mb_cfg *bcfg) { memcpy(bcfg, &baseboard_memcfg, sizeof(baseboard_memcfg)); - /* - * GPP_F2 is the MEM_CH_SEL gpio, which is set to 1 for single - * channel skus and 0 for dual channel skus. - */ - if (gpio_get(GPP_F2) == 1) { - /* - * Single channel config: for kohaku, Channel 0 is - * always populated. - */ - bcfg->channel_empty[0] = 0; - bcfg->channel_empty[1] = 1; - } else { - /* Dual channel config: both channels populated. */ - bcfg->channel_empty[0] = 0; - bcfg->channel_empty[1] = 0; - } } diff --git a/src/mainboard/google/sarien/romstage.c b/src/mainboard/google/sarien/romstage.c index e83cd4a..20eee7f 100644 --- a/src/mainboard/google/sarien/romstage.c +++ b/src/mainboard/google/sarien/romstage.c @@ -18,6 +18,18 @@ #include <soc/romstage.h>
static const struct cnl_mb_cfg memcfg = { + /* Access memory info through SMBUS. */ + .spd[0] = { + .read_type = READ_SMBUS, + .spd_spec = {.spd_smbus_address = 0xa0}, + }, + .spd[1] = {.read_type = NOT_EXISTING}, + .spd[2] = { + .read_type = READ_SMBUS, + .spd_spec = {.spd_smbus_address = 0xa4}, + }, + .spd[3] = {.read_type = NOT_EXISTING}, + /* * The dqs_map arrays map the ddr4 pins to the SoC pins * for both channels. @@ -25,16 +37,16 @@ * the index = pin number on ddr4 part * the value = pin number on SoC */ - .dqs_map[DDR_CH0] = { 0, 1, 4, 5, 2, 3, 6, 7 }, - .dqs_map[DDR_CH1] = { 0, 1, 4, 5, 2, 3, 6, 7 }, + .dqs_map[DDR_CH0] = {0, 1, 4, 5, 2, 3, 6, 7}, + .dqs_map[DDR_CH1] = {0, 1, 4, 5, 2, 3, 6, 7},
/* Baseboard uses 121, 81 and 100 rcomp resistors */ - .rcomp_resistor = { 121, 81, 100 }, + .rcomp_resistor = {121, 81, 100},
/* * Baseboard Rcomp target values. */ - .rcomp_targets = { 100, 40, 20, 20, 26 }, + .rcomp_targets = {100, 40, 20, 20, 26},
/* Disable Early Command Training */ .ect = 0, @@ -45,12 +57,7 @@
void mainboard_memory_init_params(FSPM_UPD *memupd) { - const struct spd_info spd = { - .spd_smbus_address[0] = 0xa0, - .spd_smbus_address[2] = 0xa4 - }; - wilco_ec_romstage_init();
- cannonlake_memcfg_init(&memupd->FspmConfig, &memcfg, &spd); + cannonlake_memcfg_init(&memupd->FspmConfig, &memcfg); } diff --git a/src/mainboard/intel/coffeelake_rvp/memory.c b/src/mainboard/intel/coffeelake_rvp/memory.c index a13000c..b093a20 100644 --- a/src/mainboard/intel/coffeelake_rvp/memory.c +++ b/src/mainboard/intel/coffeelake_rvp/memory.c @@ -20,6 +20,23 @@ #include <soc/cnl_memcfg_init.h>
static const struct cnl_mb_cfg baseboard_memcfg_cfg = { + /* Access memory info through SMBUS. */ + .spd[0] = { + .read_type = READ_SMBUS, + .spd_spec = {.spd_smbus_address = 0xA0} + }, + .spd[1] = { + .read_type = READ_SMBUS, + .spd_spec = {.spd_smbus_address = 0xA2} + }, + .spd[2] = { + .read_type = READ_SMBUS, + .spd_spec = {.spd_smbus_address = 0xA4} + }, + .spd[3] = { + .read_type = READ_SMBUS, + .spd_spec = {.spd_smbus_address = 0xA6} + }, /* * The dqs_map arrays map the ddr4 pins to the SoC pins * for both channels. @@ -27,16 +44,16 @@ * the index = pin number on ddr4 part * the value = pin number on SoC */ - .dqs_map[DDR_CH0] = { 0, 1, 3, 2, 4, 5, 6, 7 }, - .dqs_map[DDR_CH1] = { 1, 0, 4, 5, 2, 3, 6, 7 }, + .dqs_map[DDR_CH0] = {0, 1, 3, 2, 4, 5, 6, 7}, + .dqs_map[DDR_CH1] = {1, 0, 4, 5, 2, 3, 6, 7},
/* Baseboard uses 121, 81 and 100 rcomp resistors */ - .rcomp_resistor = { 121, 81, 100 }, + .rcomp_resistor = {121, 81, 100},
/* * Baseboard Rcomp target values. */ - .rcomp_targets = { 100, 40, 20, 20, 26 }, + .rcomp_targets = {100, 40, 20, 20, 26},
/* Baseboard is an interleaved design */ .dq_pins_interleaved = 1, diff --git a/src/mainboard/intel/coffeelake_rvp/romstage.c b/src/mainboard/intel/coffeelake_rvp/romstage.c index 1ab2d78..09ef148 100644 --- a/src/mainboard/intel/coffeelake_rvp/romstage.c +++ b/src/mainboard/intel/coffeelake_rvp/romstage.c @@ -20,13 +20,5 @@
void mainboard_memory_init_params(FSPM_UPD *memupd) { - const struct spd_info spd = { - .spd_smbus_address[0] = 0xA0, - .spd_smbus_address[1] = 0xA2, - .spd_smbus_address[2] = 0xA4, - .spd_smbus_address[3] = 0xA6, - }; - - cannonlake_memcfg_init(&memupd->FspmConfig, - variant_memcfg_config(), &spd); + cannonlake_memcfg_init(&memupd->FspmConfig, variant_memcfg_config()); } diff --git a/src/soc/intel/cannonlake/cnl_memcfg_init.c b/src/soc/intel/cannonlake/cnl_memcfg_init.c index db001b8..4ebd997 100644 --- a/src/soc/intel/cannonlake/cnl_memcfg_init.c +++ b/src/soc/intel/cannonlake/cnl_memcfg_init.c @@ -20,7 +20,7 @@ #include <string.h>
static void meminit_memcfg(FSP_M_CONFIG *mem_cfg, - const struct cnl_mb_cfg *board_cfg) + const struct cnl_mb_cfg *board_cfg) { /* * DqByteMapChx expects 12 bytes of data, but the last 6 bytes @@ -29,47 +29,58 @@ */ memset(&mem_cfg->DqByteMapCh0, 0, sizeof(mem_cfg->DqByteMapCh0)); memcpy(&mem_cfg->DqByteMapCh0, &board_cfg->dq_map[DDR_CH0], - sizeof(board_cfg->dq_map[DDR_CH0])); + sizeof(board_cfg->dq_map[DDR_CH0]));
memset(&mem_cfg->DqByteMapCh1, 0, sizeof(mem_cfg->DqByteMapCh1)); memcpy(&mem_cfg->DqByteMapCh1, &board_cfg->dq_map[DDR_CH1], - sizeof(board_cfg->dq_map[DDR_CH1])); + sizeof(board_cfg->dq_map[DDR_CH1]));
memcpy(&mem_cfg->DqsMapCpu2DramCh0, &board_cfg->dqs_map[DDR_CH0], - sizeof(board_cfg->dqs_map[DDR_CH0])); + sizeof(board_cfg->dqs_map[DDR_CH0])); memcpy(&mem_cfg->DqsMapCpu2DramCh1, &board_cfg->dqs_map[DDR_CH1], - sizeof(board_cfg->dqs_map[DDR_CH1])); + sizeof(board_cfg->dqs_map[DDR_CH1]));
memcpy(&mem_cfg->RcompResistor, &board_cfg->rcomp_resistor, - sizeof(mem_cfg->RcompResistor)); + sizeof(mem_cfg->RcompResistor));
/* Early cannonlake requires rcomp targets to be 0 */ memcpy(&mem_cfg->RcompTarget, &board_cfg->rcomp_targets, - sizeof(mem_cfg->RcompTarget)); -} - -static void meminit_memcfg_spd(FSP_M_CONFIG *mem_cfg, - const struct cnl_mb_cfg *cnl_cfg, - size_t spd_data_len, uintptr_t spd_data_ptr) -{ - mem_cfg->MemorySpdDataLen = spd_data_len; - - if (cnl_cfg->channel_empty[0] == 0) - mem_cfg->MemorySpdPtr00 = spd_data_ptr; - - if (cnl_cfg->channel_empty[1] == 0) - mem_cfg->MemorySpdPtr10 = spd_data_ptr; + sizeof(mem_cfg->RcompTarget)); }
/* * Initialize default memory settings using spd data contained in a buffer. */ -static void meminit_spd_data(FSP_M_CONFIG *mem_cfg, - const struct cnl_mb_cfg *cnl_cfg, - size_t spd_data_len, uintptr_t spd_data_ptr) +static void meminit_spd_data(FSP_M_CONFIG *mem_cfg, uint8_t mem_slot, + size_t spd_data_len, uintptr_t spd_data_ptr) { - assert(spd_data_ptr && spd_data_len); - meminit_memcfg_spd(mem_cfg, cnl_cfg, spd_data_len, spd_data_ptr); + static size_t last_set_spd_data_len = 0; + + assert(spd_data_ptr != 0 && spd_data_len != 0); + + if (last_set_spd_data_len != 0 && + last_set_spd_data_len != spd_data_len) + die("spd data length disparity among slots"); + + mem_cfg->MemorySpdDataLen = spd_data_len; + last_set_spd_data_len = spd_data_len; + + switch (mem_slot) { + case 0: + mem_cfg->MemorySpdPtr00 = spd_data_ptr; + break; + case 1: + mem_cfg->MemorySpdPtr01 = spd_data_ptr; + break; + case 2: + mem_cfg->MemorySpdPtr10 = spd_data_ptr; + break; + case 3: + mem_cfg->MemorySpdPtr11 = spd_data_ptr; + break; + default: + die("nonexistent memory slot"); + } }
/* @@ -78,13 +89,13 @@ * in spd/Makefile.inc. */ static void meminit_cbfs_spd_index(FSP_M_CONFIG *mem_cfg, - const struct cnl_mb_cfg *cnl_cfg, - int spd_index) + int spd_index, uint8_t mem_slot) { size_t spd_data_len; uintptr_t spd_data_ptr; struct region_device spd_rdev;
+ assert(mem_slot < NUM_DIMM_SLOT); printk(BIOS_DEBUG, "SPD INDEX = %d\n", spd_index); if (get_spd_cbfs_rdev(&spd_rdev, spd_index) < 0) die("spd.bin not found or incorrect index\n"); @@ -92,40 +103,42 @@ /* 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); - meminit_spd_data(mem_cfg, cnl_cfg, spd_data_len, spd_data_ptr); + meminit_spd_data(mem_cfg, mem_slot, spd_data_len, spd_data_ptr); }
/* Initialize onboard memory configurations for CannonLake */ void cannonlake_memcfg_init(FSP_M_CONFIG *mem_cfg, - const struct cnl_mb_cfg *cnl_cfg, - const struct spd_info *spd) + const struct cnl_mb_cfg *cnl_cfg) { - bool OnModuleSpd = false; + const struct spd_info *spdi; + /* Early Command Training Enabled */ mem_cfg->ECT = cnl_cfg->ect; mem_cfg->DqPinsInterleaved = cnl_cfg->dq_pins_interleaved; mem_cfg->CaVrefConfig = cnl_cfg->vref_ca_config;
- /* Spd pointer will only be used if all smbus slave address of memory - * sockets on the platform is empty */ - for (int i = 0; i < ARRAY_SIZE(mem_cfg->SpdAddressTable); i++) { - if (spd->spd_smbus_address[i] != 0) { - mem_cfg->SpdAddressTable[i] = spd->spd_smbus_address[i]; - OnModuleSpd = true; + for (int i = 0; i < NUM_DIMM_SLOT; i++) { + spdi = &(cnl_cfg->spd[i]); + switch (spdi->read_type) { + case NOT_EXISTING: + break; + case READ_SMBUS: + mem_cfg->SpdAddressTable[i] = + spdi->spd_spec.spd_smbus_address; + break; + case READ_SPD_CBFS: + meminit_cbfs_spd_index(mem_cfg, + spdi->spd_spec.spd_index, i); + break; + case READ_SPD_MEMPTR: + meminit_spd_data(mem_cfg, i, + spdi->spd_spec.spd_data_ptr_info.spd_data_len, + spdi->spd_spec.spd_data_ptr_info.spd_data_ptr); + break; + default: + die("no valid way to read mem info"); } + + meminit_memcfg(mem_cfg, cnl_cfg); } - - if (!OnModuleSpd) { - if (spd->spd_by_index) { - meminit_cbfs_spd_index(mem_cfg, cnl_cfg, - spd->spd_spec.spd_index); - } else { - meminit_spd_data(mem_cfg, cnl_cfg, - spd->spd_spec.spd_data_ptr_info.spd_data_len, - spd->spd_spec.spd_data_ptr_info.spd_data_ptr); - } - } - - meminit_memcfg(mem_cfg, cnl_cfg); - } diff --git a/src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h b/src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h index e602a33..d5f6c39 100644 --- a/src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h +++ b/src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h @@ -23,6 +23,9 @@ /* Number of dq bits controlled per dqs */ #define DQ_BITS_PER_DQS 8
+/* Number of memory DIMM slots available on Cannonlake board */ +#define NUM_DIMM_SLOT 4 + /* * Number of memory packages, where a "package" represents a 64-bit solution. */ @@ -40,17 +43,32 @@ uintptr_t spd_data_ptr; };
+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. */ +}; + struct spd_info { - bool spd_by_index; + 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; + + /* 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; - uint8_t spd_smbus_address[4]; };
/* Board-specific memory dq mapping information */ struct cnl_mb_cfg { + /* Parameters required to access SPD for CH0D0/CH0D1/CH1D0/CH1D1. */ + struct spd_info spd[NUM_DIMM_SLOT]; + /* * For each channel, there are 6 sets of DQ byte mappings, * where each set has a package 0 and a package 1 value (package 0 @@ -107,26 +125,12 @@
/* Early Command Training Enabled */ uint8_t ect; - - /* - * Flags to indicate which channels are populated. We - * currently support single or dual channel configurations. - * Set 1 to indicate that the channel is not populated Set 0 - * to indicate that the channel is populated. For example, - * dual channel memory configuration would have both - * channel_empty[0] = 0 and channel_empty[1] = 0. Note that - * this flag is only used for soldered down DRAM where we get - * SPD data from CBFS. We need the value 0 to default to - * populated in order to support existing boards. - */ - uint8_t channel_empty[2]; };
/* * Initialize default memory configurations for CannonLake. */ void cannonlake_memcfg_init(FSP_M_CONFIG *mem_cfg, - const struct cnl_mb_cfg *cnl_cfg, - const struct spd_info *spd); + const struct cnl_mb_cfg *cnl_cfg);
#endif /* _SOC_CANNONLAKE_MEMCFG_INIT_H_ */
-
build bot (Jenkins) (Code Review) -
Furquan Shaikh (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Fagerburg (Code Review)
-
Philip Chen (Code Review)
-
Tim Wawrzynczak (Code Review)