Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46250 )
Change subject: soc/intel/cannonlake: Improve memcfg comments ......................................................................
soc/intel/cannonlake: Improve memcfg comments
Instead of repeating what the FSP-M UPDs say, explain how to correctly configure these settings. DQ and DQS byte maps differ between actual Cannon Lake and Coffee Lake et alia, so skip these comments for now.
Change-Id: I1142ab500fd18176b174a5080f78c5c566c9ce25 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h 1 file changed, 29 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/46250/1
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 28af731..2e4dfcf 100644 --- a/src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h +++ b/src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h @@ -32,7 +32,7 @@
enum mem_info_read_type { NOT_EXISTING, /* No memory in this slot */ - READ_SMBUS, /* Read on-module spd by SMBUS. */ + READ_SMBUS, /* Read on-module spd by SMBUS */ READ_SPD_CBFS, /* Find spd file in CBFS. */ READ_SPD_MEMPTR /* Find spd data from pointer. */ }; @@ -40,20 +40,23 @@ 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. */ + /* 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. */ + /* 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. */ + /* To find spd data when read_type is READ_SPD_MEMPTR */ struct spd_by_pointer spd_data_ptr_info; } spd_spec; };
-/* Board-specific memory dq mapping information */ +/* Board-specific memory parameters */ struct cnl_mb_cfg { - /* Parameters required to access SPD for CH0D0/CH0D1/CH1D0/CH1D1. */ + /* + * Specify where to find the memory parameters for each slot, or the + * memory-down equivalent of a slot. Leave unpopulated slots blank. + */ struct spd_info spd[NUM_DIMM_SLOT];
/* @@ -83,39 +86,45 @@ uint8_t dqs_map[DDR_NUM_CHANNELS][DQ_BITS_PER_DQS];
/* - * 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. + * Rcomp resistor values in ohms. For socketed platforms, they are + * located on the CPU package itself and their values are always + * the same. For soldered CPUs, the resistors are on the mainboard, + * connected to the DDR_RCOMP_[2:0] pins of the processor. The PDG + * can provide the RCOMP resistor values for each memory topology. + * + * For socketed CPUs, the resistor values are: { 121, 75, 100 }; + * For soldered-down CPUs, check schematics or use the PDG values. */ uint16_t rcomp_resistor[3];
/* - * Rcomp target values. These will typically be the following - * values for Cannon Lake : { 80, 40, 40, 40, 30 } + * Rcomp target values. These depend on the platform topology and + * can be determined using Intel document #573387 or left blank. */ uint16_t rcomp_targets[5];
/* - * Indicates whether memory is interleaved. - * Set to 1 for an interleaved design, - * set to 0 for non-interleaved design. + * Indicates whether memory is interleaved. Refer to volume 1 of + * the datasheet for pictures of interleaved and non-interleaved + * configurations. Memory will never work with the wrong value. */ uint8_t dq_pins_interleaved;
/* - * VREF_CA configuration. - * Set to 0 VREF_CA goes to both CH_A and CH_B, - * set to 1 VREF_CA goes to CH_A and VREF_DQ_A goes to CH_B, - * set to 2 VREF_CA goes to CH_A and VREF_DQ_B goes to CH_B. + * VREF_CA configuration. For DDR3 and LPDDR, choose config 0. + * Config 1 is not meant to be used. For DDR4, use config 2. */ uint8_t vref_ca_config;
- /* Early Command Training Enabled */ + /* + * Early Command Training is a no-op for DDR4, and is necessary + * for LPDDR. If unsure, set to 1. MRC will skip ECT on DDR4. + */ uint8_t ect; };
/* - * Initialize default memory configurations for CannonLake. + * Write memory settings into the corresponding FSP-M UPDs. */ void cannonlake_memcfg_init(FSP_M_CONFIG *mem_cfg, const struct cnl_mb_cfg *cnl_cfg);
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46250 )
Change subject: soc/intel/cannonlake: Improve memcfg comments ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46250/1/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:
https://review.coreboot.org/c/coreboot/+/46250/1/src/soc/intel/cannonlake/in... PS1, Line 35: READ_SMBUS, /* Read on-module spd by SMBUS */ Not sure what the improvement here is? If you want to align on the periods, it would be easier to add one in the first line.
also s/by/via/?
https://review.coreboot.org/c/coreboot/+/46250/1/src/soc/intel/cannonlake/in... PS1, Line 102: or left blank FSP comment suggests that this is only true for CNL?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46250 )
Change subject: soc/intel/cannonlake: Improve memcfg comments ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46250/1/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:
https://review.coreboot.org/c/coreboot/+/46250/1/src/soc/intel/cannonlake/in... PS1, Line 35: READ_SMBUS, /* Read on-module spd by SMBUS */
Not sure what the improvement here is? If you want to align on the periods, […]
Uh, yes. I don't know what happened here. I think I wanted to write `SPD` in uppercase, decided against it, and didn't undo all the way back
https://review.coreboot.org/c/coreboot/+/46250/1/src/soc/intel/cannonlake/in... PS1, Line 102: or left blank
FSP comment suggests that this is only true for CNL?
Good point. I can try and see what happens.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46250 )
Change subject: soc/intel/cannonlake: Improve memcfg comments ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46250/1/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:
https://review.coreboot.org/c/coreboot/+/46250/1/src/soc/intel/cannonlake/in... PS1, Line 102: or left blank
Good point. I can try and see what happens.
fsp only has defaults for some SKUs and this even depends on which fsp base code was used (the older, platform-based code or the newer "onesilicon" based code like CML,ICL,TGL). Since this doesn't seem to be stable enough, let's drop "or left blank". What do you think?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46250 )
Change subject: soc/intel/cannonlake: Improve memcfg comments ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46250/1/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/cnl_memcfg_init.h:
https://review.coreboot.org/c/coreboot/+/46250/1/src/soc/intel/cannonlake/in... PS1, Line 102: or left blank
fsp only has defaults for some SKUs and this even depends on which fsp base code was used (the older […]
I'll have to rework the comment anyway. 573387 is not a good reference
Angel Pons has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/46250 )
Change subject: soc/intel/cannonlake: Improve memcfg comments ......................................................................
Abandoned
-
Angel Pons (Code Review) -
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)