Hello Varun Joshi,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/39847
to review the following change.
Change subject: soc/intel: Memory config changes for Deltan ......................................................................
soc/intel: Memory config changes for Deltan
SODIMM Specific memory configuration
Signed-off-by: Varun Joshi varun.joshi@intel.corp-partner.google.com Change-Id: I47a5dcad3ee316871a6103b9d53ef7f6fc88d7d8 --- M src/soc/intel/tigerlake/include/soc/meminit_tgl.h M src/soc/intel/tigerlake/meminit_tgl.c 2 files changed, 69 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/39847/1
diff --git a/src/soc/intel/tigerlake/include/soc/meminit_tgl.h b/src/soc/intel/tigerlake/include/soc/meminit_tgl.h index 5573fb7..b4fbfe5 100644 --- a/src/soc/intel/tigerlake/include/soc/meminit_tgl.h +++ b/src/soc/intel/tigerlake/include/soc/meminit_tgl.h @@ -13,6 +13,7 @@ #include <fsp/soc_binding.h>
#define BYTES_PER_CHANNEL 2 +#define NUM_DIMM_SLOT 2 #define BITS_PER_BYTE 8 #define DQS_PER_CHANNEL 2 #define NUM_CHANNELS 8 @@ -24,6 +25,7 @@
enum mem_info_read_type { NOT_EXISTING, /* No memory in this channel */ + READ_SMBUS, /* Read on-module spd by SMBUS. */ READ_SPD_CBFS, /* Find spd file in CBFS. */ READ_SPD_MEMPTR /* Find spd data from pointer. */ }; @@ -31,6 +33,8 @@ struct spd_info { enum mem_info_read_type read_type; union spd_data_by { + /* To read on-module spd when read_type is READ_SMBUS. */ + uint8_t spd_smbus_address; /* To identify spd file when read_type is READ_SPD_CBFS. */ int spd_index;
@@ -41,6 +45,8 @@
/* Board-specific memory configuration information */ struct mb_lpddr4x_cfg { + /* Parameters required to access SPD for CH0D0/CH0D1/CH1D0/CH1D1. */ + struct spd_info spd[NUM_DIMM_SLOT]; /* DQ mapping */ uint8_t dq_map[NUM_CHANNELS][BYTES_PER_CHANNEL * BITS_PER_BYTE];
@@ -52,7 +58,24 @@ * pin on the CPU that DRAM pin connects to. */ uint8_t dqs_map[NUM_CHANNELS][DQS_PER_CHANNEL]; - + /* + * Rcomp resistor value. The value represent the resistance in + * ohms of the rcomp resistor attached to the DDR_COMP pins on the DRAM. + */ + uint16_t rcomp_resistor; + /* + * Indicates whether memory is interleaved. + * Set to 1 for an interleaved design, + * set to 0 for non-interleaved design. + */ + 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. + */ + uint8_t vref_ca_config; /* * Early Command Training Enable/Disable Control * 1 = enable, 0 = disable @@ -60,6 +83,10 @@ uint8_t ect; };
+/* Initialize default memory configurations for TigerLake */ +void tigerlake_memcfg_init(FSP_M_CONFIG *mem_cfg, + const struct mb_lpddr4x_cfg *tgl_cfg); + /* Initialize default memory configurations for dimm0-only lpddr4x */ void meminit_lpddr4x_dimm0(FSP_M_CONFIG *mem_cfg, const struct mb_lpddr4x_cfg *board_cfg, diff --git a/src/soc/intel/tigerlake/meminit_tgl.c b/src/soc/intel/tigerlake/meminit_tgl.c index a0e5107..2d01cc2 100644 --- a/src/soc/intel/tigerlake/meminit_tgl.c +++ b/src/soc/intel/tigerlake/meminit_tgl.c @@ -25,7 +25,7 @@ &_b_cfg->dq_map[_ch], \ sizeof(_b_cfg->dq_map[_ch])); \ memcpy(&_mem_cfg->DqsMapCpu2DramCh ## _ch, \ - &_b_cfg->dqs_map[_ch], \ + &_b_cfg->dqs_map[_ch], \ sizeof(_b_cfg->dqs_map[_ch])); \ } while (0)
@@ -161,3 +161,43 @@ mem_cfg->ECT = board_cfg->ect; mem_cfg->MrcSafeConfig = 0x1; } +static void meminit_memcfg(FSP_M_CONFIG *mem_cfg, + const struct mb_lpddr4x_cfg *board_cfg) +{ + MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 0); + MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 1); + MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 2); + MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 3); + MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 4); + MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 5); + MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 6); + MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 7); +} + +/* Initialize onboard memory configurations for TigerLake */ +void tigerlake_memcfg_init(FSP_M_CONFIG *mem_cfg, + const struct mb_lpddr4x_cfg *board_cfg) +{ + const struct spd_info *spdi; + + /* Early Command Training Enabled */ + mem_cfg->ECT = board_cfg->ect; + mem_cfg->RcompResistor = board_cfg->rcomp_resistor; + mem_cfg->DqPinsInterleaved = board_cfg->dq_pins_interleaved; + + for (int i = 0; i < NUM_DIMM_SLOT; i++) { + spdi = &(board_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; + default: + die("no valid way to read mem info"); + } + + meminit_memcfg(mem_cfg, board_cfg); + } +}
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel: Memory config changes for Deltan ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/1/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/1/src/soc/intel/tigerlake/mem... PS1, Line 190: switch (spdi->read_type) { switch and case should be at the same indent
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 2:
This change is ready for review.
Varun Joshi has uploaded a new patch set (#3) to the change originally created by Varun Joshi. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
soc/intel/tigerlake: Support to initialize Memory
Support to configure DDR4 memory variant which uses SMBus address. Added support to read SPD data from SMBUS. BUG=b: 151702387
Signed-off-by: Varun Joshi varun.joshi@intel.corp-partner.google.com Change-Id: I47a5dcad3ee316871a6103b9d53ef7f6fc88d7d8 --- M src/soc/intel/tigerlake/include/soc/meminit_tgl.h M src/soc/intel/tigerlake/meminit_tgl.c 2 files changed, 66 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/39847/3
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/3/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/3/src/soc/intel/tigerlake/mem... PS3, Line 194: break; Maybe we can follow CNL? case READ_SPD_CBFS: meminit_cbfs_spd_index(mem_cfg, spdi->spd_spec.spd_index, i); break;
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/3/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/3/src/soc/intel/tigerlake/mem... PS3, Line 194: break;
Maybe we can follow CNL? […]
Please see my comments on patchset 1.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 3:
Then we can combine the entry point in meminit_tgl. Currently only volteer is use TGL, right?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 3:
Patch Set 3:
Then we can combine the entry point in meminit_tgl. Currently only volteer is use TGL, right?
I think we should have entry points based on memory type. There are some differences in what each memory type supports or how it can be configured. Those entry points can do the work specific for that memory type and then call a common routine to do the stuff which is not different across different memories e.g. SPD setting.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
Then we can combine the entry point in meminit_tgl. Currently only volteer is use TGL, right?
I think we should have entry points based on memory type. There are some differences in what each memory type supports or how it can be configured. Those entry points can do the work specific for that memory type and then call a common routine to do the stuff which is not different across different memories e.g. SPD setting.
But we have one baseboard for two different type. Deltan is SODIMM and Deltaur is solderdown. In this case we need use two different entry in romstage by type.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
Patch Set 3:
Then we can combine the entry point in meminit_tgl. Currently only volteer is use TGL, right?
I think we should have entry points based on memory type. There are some differences in what each memory type supports or how it can be configured. Those entry points can do the work specific for that memory type and then call a common routine to do the stuff which is not different across different memories e.g. SPD setting.
But we have one baseboard for two different type. Deltan is SODIMM and Deltaur is solderdown. In this case we need use two different entry in romstage by type.
Oh won't be a problem, we use weak function for the entry. I am okay with this.
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/1/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39847/1/src/soc/intel/tigerlake/inc... PS1, Line 71: dq_pins_interleaved
IIRC, TGL did not allow interleaving of memory for LPDDR4x. […]
Is the new structure required?. can't we still keep "dq_pins_interleaved" here? for lpddr, dq_pins_interleaved is currently initialized to 0 in soc meminit_tgl.c as its not supported. Having a common *header structure for ddr4 and lpddr4 is not good?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39847/3//COMMIT_MSG@11 PS3, Line 11: BUG=b: 151702387 nit:no space here. b:151702387. And maybe wrap line before BUG?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/1/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39847/1/src/soc/intel/tigerlake/inc... PS1, Line 71: dq_pins_interleaved
Is the new structure required?. […]
Yes, I think its required. It allows keeping the interface cleaner as to what the expectations are w.r.t. different memory type initialization.
Selma Bensaid has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/1/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39847/1/src/soc/intel/tigerlake/inc... PS1, Line 87: void tigerlake_memcfg_init(FSP_M_CONFIG *mem_cfg, : const struct mb_lpddr4x_cfg *tgl_cfg);
What memory type is this for? I don't think lpddr4x supports SODIMM configuration. […]
this is a SO-DIMM DDR4 for Deltan
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/1/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39847/1/src/soc/intel/tigerlake/inc... PS1, Line 87: void tigerlake_memcfg_init(FSP_M_CONFIG *mem_cfg, : const struct mb_lpddr4x_cfg *tgl_cfg);
this is a SO-DIMM DDR4 for Deltan
Thanks Selma!
Hello Bora Guvendik, build bot (Jenkins), Tim Wawrzynczak, Selma Bensaid, Anil Kumar K, Varun Joshi, Bernardo Perez Priego, Patrick Rudolph, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39847
to look at the new patch set (#4).
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
soc/intel/tigerlake: Support to initialize Memory
Support to configure DDR4 memory variant which uses SMBus address. Added support to read SPD data from SMBUS. BUG=b:151702387
Signed-off-by: Varun Joshi varun.joshi@intel.corp-partner.google.com Change-Id: I47a5dcad3ee316871a6103b9d53ef7f6fc88d7d8 --- M src/soc/intel/tigerlake/include/soc/meminit_tgl.h M src/soc/intel/tigerlake/meminit_tgl.c 2 files changed, 90 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/39847/4
Hello Bora Guvendik, build bot (Jenkins), Tim Wawrzynczak, Selma Bensaid, Anil Kumar K, Varun Joshi, Bernardo Perez Priego, Patrick Rudolph, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39847
to look at the new patch set (#5).
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
soc/intel/tigerlake: Support to initialize Memory
Support to configure DDR4 memory variant which uses SMBus address. Added support to read SPD data from SMBUS.
BUG=b:151702387
Signed-off-by: Varun Joshi varun.joshi@intel.corp-partner.google.com Change-Id: I47a5dcad3ee316871a6103b9d53ef7f6fc88d7d8 --- M src/soc/intel/tigerlake/include/soc/meminit_tgl.h M src/soc/intel/tigerlake/meminit_tgl.c 2 files changed, 90 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/39847/5
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/inc... PS5, Line 45: * nit: space before comment end
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/inc... PS5, Line 64: */ same here
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... PS5, Line 22: #define MEM_INIT_CH_DQ_DQS_MAP(_mem_cfg, _b_cfg, _ch) \ : do { \ : memcpy(&_mem_cfg->DqMapCpu2DramCh ## _ch, \ : &_b_cfg->dq_map[_ch], \ : sizeof(_b_cfg->dq_map[_ch])); \ : memcpy(&_mem_cfg->DqsMapCpu2DramCh ## _ch, \ : &_b_cfg->dqs_map[_ch], \ : sizeof(_b_cfg->dqs_map[_ch])); \ : } while (0) Why is this a macro? Why can't it be a static function?
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... PS5, Line 89: MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 0); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 1); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 2); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 3); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 4); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 5); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 6); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 7 Can this just be a loop?
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... PS5, Line 194: NUM_DIMM_SLOT i
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... PS5, Line 22: #define MEM_INIT_CH_DQ_DQS_MAP(_mem_cfg, _b_cfg, _ch) \ : do { \ : memcpy(&_mem_cfg->DqMapCpu2DramCh ## _ch, \ : &_b_cfg->dq_map[_ch], \ : sizeof(_b_cfg->dq_map[_ch])); \ : memcpy(&_mem_cfg->DqsMapCpu2DramCh ## _ch, \ : &_b_cfg->dqs_map[_ch], \ : sizeof(_b_cfg->dqs_map[_ch])); \ : } while (0)
Why is this a macro? Why can't it be a static function?
Using #define, Because we have fspm named "DqMapCpu2DramCh0[16], DqMapCpu2DramCh1[16] ... DqMapCpu2Dram7[16]".
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... PS5, Line 89: MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 0); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 1); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 2); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 3); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 4); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 5); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 6); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 7
Can this just be a loop?
Could not do it with macro defined
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... PS5, Line 89: MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 0); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 1); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 2); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 3); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 4); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 5); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 6); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 7
Could not do it with macro defined
I don't think this is completely correct. DDR4 only supports 2 channels.
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... PS5, Line 179: meminit_ddr4x_sodimm I think we can reorganize this file slightly different to allow easier sharing and also enable support for different configurations. Let me see if I can rearrange the LPDDR4x code and you can rebase on top of it.
Hello Bora Guvendik, build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Selma Bensaid, Anil Kumar K, Varun Joshi, Bernardo Perez Priego, Patrick Rudolph, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39847
to look at the new patch set (#7).
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
soc/intel/tigerlake: Support to initialize Memory
Support to configure DDR4 memory variant which uses SMBus address. Added support to read SPD data from SMBUS.
BUG=b:151702387
Signed-off-by: Varun Joshi varun.joshi@intel.corp-partner.google.com Change-Id: I47a5dcad3ee316871a6103b9d53ef7f6fc88d7d8 --- M src/soc/intel/tigerlake/include/soc/meminit_tgl.h M src/soc/intel/tigerlake/meminit_tgl.c 2 files changed, 90 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/39847/7
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 7:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39847/1/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39847/1/src/soc/intel/tigerlake/inc... PS1, Line 63: * ohms of the rcomp resistor attached to the DDR_COMP pins on the DRAM.
More words fit on the line above.
Done
https://review.coreboot.org/c/coreboot/+/39847/1/src/soc/intel/tigerlake/inc... PS1, Line 65: uint16_t rcomp_resistor;
It was confirmed earlier that rcomp_resistor does not need to be changed from default. […]
Done
https://review.coreboot.org/c/coreboot/+/39847/1/src/soc/intel/tigerlake/inc... PS1, Line 71: dq_pins_interleaved
Yes, I think its required. […]
Done
https://review.coreboot.org/c/coreboot/+/39847/1/src/soc/intel/tigerlake/inc... PS1, Line 87: void tigerlake_memcfg_init(FSP_M_CONFIG *mem_cfg, : const struct mb_lpddr4x_cfg *tgl_cfg);
Thanks Selma!
Done
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/1/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39847/1/src/soc/intel/tigerlake/inc... PS1, Line 49: spd_info
spd_info was intentionally kept separate from mainboard memory configuration. […]
Done
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39847/3//COMMIT_MSG@11 PS3, Line 11: BUG=b: 151702387
nit:no space here. b:151702387. […]
Done
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... PS5, Line 194: NUM_DIMM_SLOT
i
Done
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... PS5, Line 179: meminit_ddr4x_sodimm
I think we can reorganize this file slightly different to allow easier sharing and also enable suppo […]
Okay
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... PS5, Line 179: meminit_ddr4x_sodimm
Okay
Here you go: https://review.coreboot.org/c/coreboot/+/39865
I have also added some basic header file changes to support DDR4 that you can build up on: https://review.coreboot.org/c/coreboot/+/39866
This is what I think you will have to do: a) Add ddr4_cfg structure b) Implement meminit_ddr4(...) In this function, you can set all params specific to DDR4 and then for SPD you will have to handle in following ways: i. As per EDS, if mixed configuration is used, ch0 should always be MD. Thus, if memory topology is mixed or MD, then you will have to use read_md_spd() to read out SPD information. Else, use read_smbus_spd()[You will have to implement this to read SPD via SMBus in coreboot]. Then, initialize channel1 by calling init_spd_upds(), init_dq_upds(), init_dqs_upds().
ii. For channel 2, if memory topology is MD, you can reuse SPD from above. Else, you will have to call read_smbus_spd() to read SPD. Then, initialize channel2 by calling init_spd_upds(), init_dq_upds(), init_dqs_upds().
For SODIMM configurations, each channel can support upto 2 DIMMs. So, you will have to ensure that you handle it appropriately and pass in right information into init_spd_upds().
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... PS5, Line 193: SpdAddressTable There is no need to set SpdAddressTable. Coreboot can read SPD data using SMBus and directly pass that information into FSP.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... PS5, Line 193: SpdAddressTable
There is no need to set SpdAddressTable. […]
You can make use of get_spd_smbus() [Reference: https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/common/bloc...]
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... PS5, Line 179: meminit_ddr4x_sodimm
Here you go: https://review.coreboot.org/c/coreboot/+/39865 […]
I am working on this, I saw you defined DIMMS_PER_CHANNEL to 2, shouldn't it be 1 for deltan?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... PS5, Line 179: meminit_ddr4x_sodimm
I am working on this, I saw you defined DIMMS_PER_CHANNEL to 2, shouldn't it be 1 for deltan?
Code added to SoC will be independent of one particular configuration. I agree that DIMMS_PER_CHANNEL are 1 for Deltan, however, TGL memory controller supports 2 DIMMs per channel. In the future, there can be a board that needs to use it. Hence, SoC code will have to account for that.
Here, board provided information will be useful in determining how to initialize different UPDs. i.e. for Deltan: topology = SODIMM, smbus_addr[0] --> Addr for ch0 smbus_addr[1] --> nil since deltan does not support dimm 1 on ch0 smbus_addr[2] --> Addr for ch1 smbus_addr[3] --> nil since deltan does not support dimm 1 on ch1.
SoC code will then have to use this information to fill the SPD data pointers accordingly. If it makes it easier to understand, you can also change smbus_addr to be something like:
struct { uint8_t addr_dimm0; uint8_t addr_dimm1; } smbus_info[DDR4_CHANNELS];
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... PS5, Line 179: meminit_ddr4x_sodimm
Code added to SoC will be independent of one particular configuration. […]
Correct, this what I mentioned before. And need change address to 0xa4 for ch1.
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... PS5, Line 179: meminit_ddr4x_sodimm
Code added to SoC will be independent of one particular configuration. […]
From your above comment(One on where you say ch0 should always be MD )If I understand correctly,for Deltan (which is not MD), we still use Channel 0 and Channel 1? Or we use channel 1 and channel 2?
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... PS5, Line 179: meminit_ddr4x_sodimm
From your above comment(One on where you say ch0 should always be MD )If I understand correctly,for […]
Deltan use CH0 and CH1. No MD in this project. But just one dimm for each channel.
Hello Bora Guvendik, build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Kevin Chowski, Selma Bensaid, Anil Kumar K, Varun Joshi, Bernardo Perez Priego, Patrick Rudolph, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39847
to look at the new patch set (#8).
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
soc/intel/tigerlake: Support to initialize Memory
Support to configure DDR4 memory variant which uses SMBus address. Added support to read SPD data from SMBUS.
BUG=b:151702387
Signed-off-by: Varun Joshi varun.joshi@intel.corp-partner.google.com Change-Id: I47a5dcad3ee316871a6103b9d53ef7f6fc88d7d8 --- M src/soc/intel/tigerlake/include/soc/meminit_tgl.h M src/soc/intel/tigerlake/meminit_tgl.c 2 files changed, 321 insertions(+), 138 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/39847/8
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 8:
(16 comments)
https://review.coreboot.org/c/coreboot/+/39847/8/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/8/src/soc/intel/tigerlake/mem... PS8, Line 233: static void read_smbus_spd(struct spd_block *blk, size_t *spd_len, uintptr_t *data0, uintptr_t *data1) line over 96 characters
https://review.coreboot.org/c/coreboot/+/39847/8/src/soc/intel/tigerlake/mem... PS8, Line 236: get_spd_smbus(blk); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39847/8/src/soc/intel/tigerlake/mem... PS8, Line 236: get_spd_smbus(blk); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39847/8/src/soc/intel/tigerlake/mem... PS8, Line 237: *spd_len = blk->len; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39847/8/src/soc/intel/tigerlake/mem... PS8, Line 237: *spd_len = blk->len; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39847/8/src/soc/intel/tigerlake/mem... PS8, Line 238: *data0 = (uintptr_t)blk->spd_array[0]; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39847/8/src/soc/intel/tigerlake/mem... PS8, Line 238: *data0 = (uintptr_t)blk->spd_array[0]; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39847/8/src/soc/intel/tigerlake/mem... PS8, Line 239: *data1 = (uintptr_t)blk->spd_array[2]; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39847/8/src/soc/intel/tigerlake/mem... PS8, Line 239: *data1 = (uintptr_t)blk->spd_array[2]; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39847/8/src/soc/intel/tigerlake/mem... PS8, Line 276: * 16 bits denoting DQ_PER_CHANNEL please, no space before tabs
https://review.coreboot.org/c/coreboot/+/39847/8/src/soc/intel/tigerlake/mem... PS8, Line 280: for(unsigned int i=0; i<DDR4_CHANNELS; i++) { spaces required around that '=' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/39847/8/src/soc/intel/tigerlake/mem... PS8, Line 280: for(unsigned int i=0; i<DDR4_CHANNELS; i++) { spaces required around that '<' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/39847/8/src/soc/intel/tigerlake/mem... PS8, Line 280: for(unsigned int i=0; i<DDR4_CHANNELS; i++) { space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/39847/8/src/soc/intel/tigerlake/mem... PS8, Line 281: for(int b=0; b<DQ_SODIMM_SLOT; b++) { spaces required around that '=' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/39847/8/src/soc/intel/tigerlake/mem... PS8, Line 281: for(int b=0; b<DQ_SODIMM_SLOT; b++) { spaces required around that '<' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/39847/8/src/soc/intel/tigerlake/mem... PS8, Line 281: for(int b=0; b<DQ_SODIMM_SLOT; b++) { space required before the open parenthesis '('
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 10:
(2 comments)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/39847/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39847/1//COMMIT_MSG@7 PS1, Line 7: soc/intel: Memory config changes for Deltan
Please make that a statement by adding a verb in imperative mood. […]
Done
https://review.coreboot.org/c/coreboot/+/39847/1//COMMIT_MSG@9 PS1, Line 9: SODIMM Specific memory configuration
Please elaborate. […]
Done
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 10:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39847/1/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/1/src/soc/intel/tigerlake/mem... PS1, Line 177: TigerLake
Tiger Lake
Done
https://review.coreboot.org/c/coreboot/+/39847/1/src/soc/intel/tigerlake/mem... PS1, Line 179: const struct mb_lpddr4x_cfg *board_cfg)
Fits in one line.
Done
https://review.coreboot.org/c/coreboot/+/39847/1/src/soc/intel/tigerlake/mem... PS1, Line 188: int
unsigned int i
Done
https://review.coreboot.org/c/coreboot/+/39847/1/src/soc/intel/tigerlake/mem... PS1, Line 203: }
I think the code here should be organized such that we don't duplicate a lot of stuff: […]
Done
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 10:
(7 comments)
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/inc... PS5, Line 45: *
nit: space before comment end
Done
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/inc... PS5, Line 64: */
same here
Done
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/inc... PS5, Line 64: */
same here
Done
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... PS5, Line 22: #define MEM_INIT_CH_DQ_DQS_MAP(_mem_cfg, _b_cfg, _ch) \ : do { \ : memcpy(&_mem_cfg->DqMapCpu2DramCh ## _ch, \ : &_b_cfg->dq_map[_ch], \ : sizeof(_b_cfg->dq_map[_ch])); \ : memcpy(&_mem_cfg->DqsMapCpu2DramCh ## _ch, \ : &_b_cfg->dqs_map[_ch], \ : sizeof(_b_cfg->dqs_map[_ch])); \ : } while (0)
Using #define, Because we have fspm named "DqMapCpu2DramCh0[16], DqMapCpu2DramCh1[16] ... […]
Done
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... PS5, Line 89: MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 0); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 1); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 2); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 3); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 4); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 5); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 6); : MEM_INIT_CH_DQ_DQS_MAP(mem_cfg, board_cfg, 7
I don't think this is completely correct. DDR4 only supports 2 channels.
Ack
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... PS5, Line 179: meminit_ddr4x_sodimm
Deltan use CH0 and CH1. No MD in this project. But just one dimm for each channel.
Ack
https://review.coreboot.org/c/coreboot/+/39847/5/src/soc/intel/tigerlake/mem... PS5, Line 193: SpdAddressTable
You can make use of get_spd_smbus() [Reference: https://review.coreboot.org/cgit/coreboot. […]
Done
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/10/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/10/src/soc/intel/tigerlake/me... PS10, Line 271: /*First set of Byte0- Byte 7 in Dqmap is through channel0, nits:Please fix the comment indent.
Furquan Shaikh has uploaded a new patch set (#11) to the change originally created by Varun Joshi. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
soc/intel/tigerlake: Support to initialize Memory
Support to configure DDR4 memory variant which uses SMBus address. Added support to read SPD data from SMBUS.
BUG=b:151702387
Signed-off-by: Varun Joshi varun.joshi@intel.corp-partner.google.com Change-Id: I47a5dcad3ee316871a6103b9d53ef7f6fc88d7d8 --- M src/soc/intel/tigerlake/include/soc/meminit_tgl.h M src/soc/intel/tigerlake/meminit_tgl.c 2 files changed, 95 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/39847/11
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 12:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39847/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39847/12//COMMIT_MSG@7 PS12, Line 7: soc/intel/tigerlake: Support to initialize Memory Add support to initialize memory
https://review.coreboot.org/c/coreboot/+/39847/12//COMMIT_MSG@10 PS12, Line 10: Added Add
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... PS12, Line 104: * DQS CPU<>DRAM map. Each array entry represents a : * mapping of a dq bit on the CPU to the bit it's connected to on : * the memory part. The array index represents the dqs bit number : * on the memory part, and the values in the array represent which : * pin on the CPU that DRAM pin connects to. Please re-flow for 80 or 96 characters per line.
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/me... PS12, Line 337: */ Please format the comment correctly by aligning the asterisks and adding a dot after an asterisk.
https://doc.coreboot.org/coding_style.html#commenting
Bora Guvendik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 12:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... PS12, Line 23: Please make it 3 tabs to be consistent with the defines above/
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... PS12, Line 100: mb_ddr4x_cfg ddr4x_cfg to have similar naming convention to lpddr4x_cfg above.
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... PS12, Line 102: DQ_PER_CHANNEL #define for this is missing.
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... PS12, Line 110: DQS_PER_CHANNEL #define for this is missing.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 12:
Patch Set 12:
(4 comments)
I think Varun need to integrate Furquan patches again.
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 12:
Patch Set 12:
Patch Set 12:
(4 comments)
I think Varun need to integrate Furquan patches again.
Yes, I'm working on it
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 12:
(10 comments)
https://review.coreboot.org/c/coreboot/+/39847/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39847/12//COMMIT_MSG@7 PS12, Line 7: soc/intel/tigerlake: Support to initialize Memory
Add support to initialize memory
Add support to initialize DDR4 memory
https://review.coreboot.org/c/coreboot/+/39847/12//COMMIT_MSG@9 PS12, Line 9: which uses SMBus address Why is this being restricted to SoDIMM memory only?
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... PS12, Line 102: dq_map This should actually be organized as: uint8_t dq_map[DDR4_CHANNELS][DDR4_BYTES_PER_CHANNEL][BITS_PER_BYTE]; where DDR4_CHANNELS is 2 and DDR4_BYTES_PER_CHANNEL is 8.
Also, can you please add a comment similar to https://review.coreboot.org/c/coreboot/+/39865/7/src/soc/intel/tigerlake/inc... to show how the array matches with the hardware mapping.
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... PS12, Line 110: dqs_map uint8_t dqs_map[DDR4_CHANNELS][DDR4_BYTES_PER_CHANNEL];
And comment showing the mapping : https://review.coreboot.org/c/coreboot/+/39865/7/src/soc/intel/tigerlake/inc...
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... PS12, Line 111: /* : * Rcomp resistor value. The value represent the resistance in ohms : * of the rcomp resistor attached to the DDR_COMP pins on the DRAM. : */ : uint16_t rcomp_resistor; This is not required even for DDR4. We had got a confirmation from Intel here: https://issuetracker.google.com/142650263#comment27
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... PS12, Line 139: _sodimm This function can be simply named: meminit_ddr4x. spd_info should provide information about the memory topology.
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/me... PS12, Line 308: unsigned const
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/me... PS12, Line 309: unsigned const
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/me... PS12, Line 320: struct spd_block blk = { : .addr_map[0] = spd->smbus_info[0].addr_dimm0, : .addr_map[1] = spd->smbus_info[0].addr_dimm1, : .addr_map[2] = spd->smbus_info[1].addr_dimm0, : .addr_map[3] = spd->smbus_info[1].addr_dimm1, : }; : : read_smbus_spd(&blk, &spd_len, &spd_data0, &spd_data1); This function should be able to handle all three memory topologies. I am thinking something like:
if ((info->topology == MEMORY_DOWN) || (info->topology == MIXED))
-------read_md_spd(info, &spd_md_data, &spd_md_len);
if ((info->topology == SODIMM) || (info->topology == MIXED)
-------read_sodimm_spd(info, &spd_sodimm_blk);
And then for each channel you will have to:
for (i = 0; i < DDR4_CHANNELS; i++) {
-------ddr4_get_spd(i, spd_md_data, *spd_sodimm_blk, info, half_populated ------->------->-------&spd_dimm0, &spd_dimm1); -------init_spd_upds(mem_cfg, i, spd_dimm0, spd_dimm1);
}
where ddr4_get_spd can fill in the right spd pointers based on:
Channel 0: - If mixed or memory down, use spd_md_data. In this case, spd_dimm1 will be 0. - If sodimm, use spd_sodimm_blk. In this case, spd_dimm0 will be blk->spd_array[0] and spd_dimm1 will be blk->spd_array[1];
Channel 1: - If half populated, spd_dimm0 and spd_dimm1 will be 0. - If mixed or sodimm, use spd_sodimm_blk. - If memory down, use spd_md_data.
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/me... PS12, Line 339: for (unsigned int i = 0; i < DDR4_CHANNELS; i++) { : for (int b = 0; b < DQ_SODIMM_SLOT; b++) { : init_dq_upds(mem_cfg, 4i+b, &board_cfg->dq_map[i][DQ_PER_CHANNEL*b]); : init_dqs_upds(mem_cfg, 4i+b, &board_cfg->dqs_map[i][DQS_PER_CHANNEL*b]); : } : } This will have to be updated as per latest changes.
Hello Bora Guvendik, build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Kevin Chowski, Selma Bensaid, Anil Kumar K, Varun Joshi, Bernardo Perez Priego, Patrick Rudolph, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39847
to look at the new patch set (#13).
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
soc/intel/tigerlake: Support to initialize Memory
Support to configure DDR4 memory variant which uses SMBus address. Added support to read SPD data from SMBUS.
BUG=b:151702387
Signed-off-by: Varun Joshi varun.joshi@intel.corp-partner.google.com Change-Id: I47a5dcad3ee316871a6103b9d53ef7f6fc88d7d8 --- M src/soc/intel/tigerlake/include/soc/meminit_tgl.h M src/soc/intel/tigerlake/meminit_tgl.c 2 files changed, 153 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/39847/13
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 13:
(25 comments)
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 306: if(channel == 0) { space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 307: if((info->topology == MEMORY_DOWN) || (info->topology == MIXED)) { space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 312: if((info->topology == SODIMM)) { Unnecessary parentheses - maybe == should be = ?
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 312: if((info->topology == SODIMM)) { space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 318: if(channel == 1) { space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 319: if(half_populated) { space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 324: if((info->topology == MEMORY_DOWN) || (info->topology == MIXED)) { space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 328: if((info->topology == SODIMM)) { Unnecessary parentheses - maybe == should be = ?
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 328: if((info->topology == SODIMM)) { space required before the open parenthesis '('
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 366: for (unsigned int i=0; i<DDR4_CHANNELS; i++) { spaces required around that '=' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 366: for (unsigned int i=0; i<DDR4_CHANNELS; i++) { spaces required around that '<' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 389: * DDR4x memory interface has 8 DQS pairs per channel. FSP UPDs for DQS Map expect a code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 390: * pair in each UPD. code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 391: * code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 392: * Thus, init_dqs_upds() needs to be called for dqs pair of each channel. code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 393: * DqsMapCpu2DramCh0 --> dqs_map[CHAN=0][0-1] code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 394: * DqsMapCpu2DramCh1 --> dqs_map[CHAN=0][2-3] code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 395: * DqsMapCpu2DramCh2 --> dqs_map[CHAN=0][4-5] code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 396: * DqsMapCpu2DramCh3 --> dqs_map[CHAN=0][6-7] code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 397: * DqsMapCpu2DramCh4 --> dqs_map[CHAN=1][0-1] code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 398: * DqsMapCpu2DramCh5 --> dqs_map[CHAN=1][2-3] code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 399: * DqsMapCpu2DramCh6 --> dqs_map[CHAN=1][4-5] code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 400: * DqsMapCpu2DramCh7 --> dqs_map[CHAN=1][6-7] code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 401: */ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 404: for (int b = 0; b < DDR4_BYTES_PER_CHANNEL; b+=2) { spaces required around that '+=' (ctx:VxV)
Hello Bora Guvendik, build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Kevin Chowski, Selma Bensaid, Anil Kumar K, Varun Joshi, Bernardo Perez Priego, Patrick Rudolph, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39847
to look at the new patch set (#14).
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
soc/intel/tigerlake: Support to initialize Memory
Support to configure DDR4 memory variant which uses SMBus address. Added support to read SPD data from SMBUS.
BUG=b:151702387
Signed-off-by: Varun Joshi varun.joshi@intel.corp-partner.google.com Change-Id: I47a5dcad3ee316871a6103b9d53ef7f6fc88d7d8 --- M src/soc/intel/tigerlake/include/soc/meminit_tgl.h M src/soc/intel/tigerlake/meminit_tgl.c 2 files changed, 153 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/39847/14
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 14:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39847/14/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/14/src/soc/intel/tigerlake/me... PS14, Line 356: if (( info->topology == MEMORY_DOWN ) || ( info->topology == MIXED )) { space prohibited after that open parenthesis '('
https://review.coreboot.org/c/coreboot/+/39847/14/src/soc/intel/tigerlake/me... PS14, Line 356: if (( info->topology == MEMORY_DOWN ) || ( info->topology == MIXED )) { space prohibited before that close parenthesis ')'
https://review.coreboot.org/c/coreboot/+/39847/14/src/soc/intel/tigerlake/me... PS14, Line 361: if ( info->topology == SODIMM ) { space prohibited after that open parenthesis '('
https://review.coreboot.org/c/coreboot/+/39847/14/src/soc/intel/tigerlake/me... PS14, Line 361: if ( info->topology == SODIMM ) { space prohibited before that close parenthesis ')'
https://review.coreboot.org/c/coreboot/+/39847/14/src/soc/intel/tigerlake/me... PS14, Line 366: for (unsigned int i = 0; i< DDR4_CHANNELS; i++) { spaces required around that '<' (ctx:VxW)
Hello Bora Guvendik, build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Kevin Chowski, Selma Bensaid, Anil Kumar K, Varun Joshi, Bernardo Perez Priego, Patrick Rudolph, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39847
to look at the new patch set (#15).
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
soc/intel/tigerlake: Support to initialize Memory
Support to configure DDR4 memory variant which uses SMBus address. Added support to read SPD data from SMBUS.
BUG=b:151702387
Signed-off-by: Varun Joshi varun.joshi@intel.corp-partner.google.com Change-Id: I47a5dcad3ee316871a6103b9d53ef7f6fc88d7d8 --- M src/soc/intel/tigerlake/include/soc/meminit_tgl.h M src/soc/intel/tigerlake/meminit_tgl.c 2 files changed, 153 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/39847/15
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 15:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 297: *spd_len = blk->len; *spd_len = blk->len; seems not needed, you can assign this after you get the blk.
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 300: static void ddr4_get_spd(unsigned int channel, uintptr_t *spd_md_data, The channel(variable i) is defined as DDR4_CHANNELS, we need try to avoid the hard code for the statement like if(channel == 0). You will miss something after future maintaining.
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 363: mem_cfg->MemorySpdDataLen = spd_sodimm_len; mem_cfg->MemorySpdDataLen=spd_sodimm_blk->len
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 15:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 297: *spd_len = blk->len;
*spd_len = blk->len; seems not needed, you can assign this after you get the blk.
Ack
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 300: static void ddr4_get_spd(unsigned int channel, uintptr_t *spd_md_data,
The channel(variable i) is defined as DDR4_CHANNELS, we need try to avoid the hard code for the stat […]
Ack
https://review.coreboot.org/c/coreboot/+/39847/13/src/soc/intel/tigerlake/me... PS13, Line 363: mem_cfg->MemorySpdDataLen = spd_sodimm_len;
mem_cfg->MemorySpdDataLen=spd_sodimm_blk->len
Ack
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 15:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/me... PS12, Line 308: unsigned
const
Done
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/me... PS12, Line 309: unsigned
const
Done
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/me... PS12, Line 320: struct spd_block blk = { : .addr_map[0] = spd->smbus_info[0].addr_dimm0, : .addr_map[1] = spd->smbus_info[0].addr_dimm1, : .addr_map[2] = spd->smbus_info[1].addr_dimm0, : .addr_map[3] = spd->smbus_info[1].addr_dimm1, : }; : : read_smbus_spd(&blk, &spd_len, &spd_data0, &spd_data1);
This function should be able to handle all three memory topologies. I am thinking something like: […]
Done
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/me... PS12, Line 337: */
Please format the comment correctly by aligning the asterisks and adding a dot after an asterisk. […]
Done
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/me... PS12, Line 339: for (unsigned int i = 0; i < DDR4_CHANNELS; i++) { : for (int b = 0; b < DQ_SODIMM_SLOT; b++) { : init_dq_upds(mem_cfg, 4i+b, &board_cfg->dq_map[i][DQ_PER_CHANNEL*b]); : init_dqs_upds(mem_cfg, 4i+b, &board_cfg->dqs_map[i][DQS_PER_CHANNEL*b]); : } : }
This will have to be updated as per latest changes.
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 15:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 312: if nit: else if
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 318: if nit: else if
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 328: if nit: else if
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 361: if nit: else if
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 318: if
nit: else if
yes, then use else to print some message.. channel out of config/support etc..
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 15:
(21 comments)
https://review.coreboot.org/c/coreboot/+/39847/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39847/12//COMMIT_MSG@7 PS12, Line 7: soc/intel/tigerlake: Support to initialize Memory
Add support to initialize DDR4 memory
This still needs to be addressed.
https://review.coreboot.org/c/coreboot/+/39847/15//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39847/15//COMMIT_MSG@9 PS15, Line 9: hich uses SMBus address not true anymore.
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... PS15, Line 99: /* DQ mapping */ I think it would be helpful to add some information like lpddr4x. Example:
/* * DQ CPU<>DRAM map: * DDR4 memory interface has 8 DQs per channel. Each DQ consists of 8 bits(1 * byte). Thus, dq_map is represented as DDR[1-0]_DQ[7-0][7:0], where * DDR[1-0] : DDR4 channel # * DQ[7-0] : DQ # within the channel * [7:0] : Bits within the DQ * * Index of the array represents DQ pin# on the CPU, whereas value in * the array represents DQ pin# on the memory part. */
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... PS15, Line 101: /* : * DQS CPU<>DRAM map. Each array entry represents a : * mapping of a dq bit on the CPU to the bit it's connected to on : * the memory part. The array index represents the dqs bit number : * on the memory part, and the values in the array represent which : * pin on the CPU that DRAM pin connects to. : */ /* * DQS CPU<>DRAM map: * DDR4 memory interface has 8 DQS pairs per channel. Thus, dqs_map is represented as * DDR[1-0]_DQS[7-0], where * DDR[1-0] : DDR4 channel # * DQS[7-0] : DQS # within the channel * * Index of the array represents DQS pin# on the CPU, whereas value in * the array represents DQ pin# on the memory part. */
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... PS15, Line 121: vref_ca_config This seems to be unused.
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... PS15, Line 131: SO-DIMM based Not true anymore.
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 307: if ((info->topology == MEMORY_DOWN) || (info->topology == MIXED)) { A comment here would be helpful indicating that for mixed topologies, ch0 can only be MD. Helps to tie things together.
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 310: : : if nit: else if (info->topology == SODIMM) { ... }
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 318: if nit: else if
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 321: return Using `else if` should avoid the need of adding early return here.
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 324: || (info->topology == MIXED) This is not correct. If mixed topology is used, channel1 has to be SODIMM. This check needs to go down along with SODIMM. Also, a comment for the mixed topology would be helpful.
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 342: = 0 I don't think these need to be set. It is initialized before being used.
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 343: = 0 same here.
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 349: = { : .addr_map[0] = info->smbus_info[0].addr_dimm0, : .addr_map[1] = info->smbus_info[0].addr_dimm1, : .addr_map[2] = info->smbus_info[1].addr_dimm0, : .addr_map[3] = info->smbus_info[1].addr_dimm1, : }; nit: Since you have a separate function read_sodimm_spd(), I think it makes sense to move setting of addr_map[3-0] to it. spd_sodimm_blk still needs to be passed in from this function to read_sodimm_spd() since it is used in this function later on.
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 363: mem_cfg->MemorySpdDataLen = spd_sodimm_len; How do you plan to ensure that MemorySpdDataLen is correct in MIXED topology?
if ((info->topology == MIXED) && (mem_cfg->MemorySpdDataLen != spd_sodimm_len)) die(...); else mem_cfg->MemorySpdDataLen = spd_sodimm_len;
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 373: DDR4x Just DDR4.
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 377: for dq pair for *every* dq pair
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 389: DDR4x Just DDR4
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 392: for dqs pair for *every* dqs pair
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 403: unsigned int i Can you please move this declaration to start of this function. No need to declare it in each for loop.
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 405: init_dq_upds(mem_cfg, 4i+b, board_cfg->dq_map[i][b], : board_cfg->dq_map[i][b+1]); : init_dqs_upds(mem_cfg, 4i+b, board_cfg->dqs_map[i][b], : board_cfg->dqs_map[i][b+1]); This will have to take into account empty v/s populated slots.
/* Channel 0 */ for (b = 0, i = 0; b < DDR4_BYTES_PER_CHANNEL; i++, b+=2) { init_dq_upds(mem_cfg, i, board_cfg->dq_map[0][b], board_cfg->dq_map[0][b+1]; init_dqs_upds(mem_cfg, i, board_cfg->dqs_map[0][b], board_cfg->dqs_map[0][b+1]; }
/* Channel 1 */ for (b = 0; b < DDR4_BYTES_PER_CHANNEL; i++, b+=2) { if (half_populated) { init_dq_upds_empty(mem_cfg, i); init_dqs_upds_empty(mem_cfg, i); } else { init_dq_upds(mem_cfg, i, board_cfg->dq_map[1][b], board_cfg->dq_map[1][b+1]; init_dqs_upds(mem_cfg, i, board_cfg->dqs_map[1][b], board_cfg->dqs_map[1][b+1]; } }
or, maybe:
index = 0; for (i = 0; i < DDR4_CHANNELS; i++) { for (int b = 0; b < DDR4_BYTES_PER_CHANNEL; b += 2) { if (half_populated && (i == 1)) { init_dq_upds_empty(mem_cfg, index); init_dqs_upds_empty(mem_cfg, index); } else { init_dq_upds(mem_cfg, index, board_cfg->dq_map[i][b], board_cfg->dq_map[i][b+1]; init_dqs_upds(mem_cfg, index, board_cfg->dqs_map[i][b], board_cfg->dqs_map[i][b+1]; } index++; } }
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Support to initialize Memory ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/16/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/16/src/soc/intel/tigerlake/me... PS16, Line 292: read_sodimm_spd To make this consistent with memory-down, it would be good to also add a call to printing out SPD information. I have moved the print to happen in read_md_spd() in my latest patchset.
Hello Bora Guvendik, build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Kevin Chowski, Selma Bensaid, Anil Kumar K, Varun Joshi, Bernardo Perez Priego, Patrick Rudolph, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39847
to look at the new patch set (#17).
Change subject: soc/intel/tigerlake: Add support to initialize Memory ......................................................................
soc/intel/tigerlake: Add support to initialize Memory
Support to configure DDR4 memory variant which uses SMBus address. Add support to read SPD data based on different memory topology.
BUG=b:151702387
Signed-off-by: Varun Joshi varun.joshi@intel.corp-partner.google.com Change-Id: I47a5dcad3ee316871a6103b9d53ef7f6fc88d7d8 --- M src/soc/intel/tigerlake/include/soc/meminit_tgl.h M src/soc/intel/tigerlake/meminit_tgl.c 2 files changed, 180 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/39847/17
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize Memory ......................................................................
Patch Set 17:
(10 comments)
https://review.coreboot.org/c/coreboot/+/39847/17/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/17/src/soc/intel/tigerlake/me... PS17, Line 320: else else should follow close brace '}'
https://review.coreboot.org/c/coreboot/+/39847/17/src/soc/intel/tigerlake/me... PS17, Line 321: printk(BIOS_INFO,"Undefined memory topology on Channel 0"); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/39847/17/src/soc/intel/tigerlake/me... PS17, Line 329: else if ((info->topology == MEMORY_DOWN)) { Unnecessary parentheses - maybe == should be = ?
https://review.coreboot.org/c/coreboot/+/39847/17/src/soc/intel/tigerlake/me... PS17, Line 333: else if ((info->topology == SODIMM) || (info->topology == MIXED)) { else should follow close brace '}'
https://review.coreboot.org/c/coreboot/+/39847/17/src/soc/intel/tigerlake/me... PS17, Line 337: else else should follow close brace '}'
https://review.coreboot.org/c/coreboot/+/39847/17/src/soc/intel/tigerlake/me... PS17, Line 338: printk(BIOS_DEBUG,"Undefined memory topology on channel 1"); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/39847/17/src/soc/intel/tigerlake/me... PS17, Line 340: else else should follow close brace '}'
https://review.coreboot.org/c/coreboot/+/39847/17/src/soc/intel/tigerlake/me... PS17, Line 341: printk(BIOS_DEBUG,"Unsupported channels"); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/39847/17/src/soc/intel/tigerlake/me... PS17, Line 367: if ((info->topology == MIXED) && (mem_cfg->MemorySpdDataLen != spd_sodimm_blk.len)) line over 96 characters
https://review.coreboot.org/c/coreboot/+/39847/17/src/soc/intel/tigerlake/me... PS17, Line 416: else { else should follow close brace '}'
Hello Bora Guvendik, build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Kevin Chowski, Selma Bensaid, Anil Kumar K, Varun Joshi, Bernardo Perez Priego, Patrick Rudolph, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39847
to look at the new patch set (#18).
Change subject: soc/intel/tigerlake: Add support to initialize Memory ......................................................................
soc/intel/tigerlake: Add support to initialize Memory
Support to configure DDR4 memory variant. -Add support to read SPD data based on different memory topology. -Initialize FSP UPD's for DQ and DQS mapping.
BUG=b:151702387
Signed-off-by: Varun Joshi varun.joshi@intel.corp-partner.google.com Change-Id: I47a5dcad3ee316871a6103b9d53ef7f6fc88d7d8 --- M src/soc/intel/tigerlake/include/soc/meminit_tgl.h M src/soc/intel/tigerlake/meminit_tgl.c 2 files changed, 181 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/39847/18
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize Memory ......................................................................
Patch Set 18:
(7 comments)
https://review.coreboot.org/c/coreboot/+/39847/18/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/18/src/soc/intel/tigerlake/me... PS18, Line 320: else else should follow close brace '}'
https://review.coreboot.org/c/coreboot/+/39847/18/src/soc/intel/tigerlake/me... PS18, Line 321: printk(BIOS_INFO,"Undefined memory topology on Channel 0"); space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/39847/18/src/soc/intel/tigerlake/me... PS18, Line 329: else if ((info->topology == MEMORY_DOWN)) { Unnecessary parentheses - maybe == should be = ?
https://review.coreboot.org/c/coreboot/+/39847/18/src/soc/intel/tigerlake/me... PS18, Line 333: else if ((info->topology == SODIMM) || (info->topology == MIXED)) { else should follow close brace '}'
https://review.coreboot.org/c/coreboot/+/39847/18/src/soc/intel/tigerlake/me... PS18, Line 337: else Statements should start on a tabstop
https://review.coreboot.org/c/coreboot/+/39847/18/src/soc/intel/tigerlake/me... PS18, Line 340: else Statements should start on a tabstop
https://review.coreboot.org/c/coreboot/+/39847/18/src/soc/intel/tigerlake/me... PS18, Line 417: else { Statements should start on a tabstop
Hello Bora Guvendik, build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Kevin Chowski, Selma Bensaid, Anil Kumar K, Varun Joshi, Bernardo Perez Priego, Patrick Rudolph, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39847
to look at the new patch set (#19).
Change subject: soc/intel/tigerlake: Add support to initialize Memory ......................................................................
soc/intel/tigerlake: Add support to initialize Memory
Support to configure DDR4 memory variant. -Add support to read SPD data based on different memory topology. -Initialize FSP UPD's for DQ and DQS mapping.
BUG=b:151702387
Signed-off-by: Varun Joshi varun.joshi@intel.corp-partner.google.com Change-Id: I47a5dcad3ee316871a6103b9d53ef7f6fc88d7d8 --- M src/soc/intel/tigerlake/include/soc/meminit_tgl.h M src/soc/intel/tigerlake/meminit_tgl.c 2 files changed, 170 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/39847/19
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize Memory ......................................................................
Patch Set 19:
(23 comments)
https://review.coreboot.org/c/coreboot/+/39847/12//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39847/12//COMMIT_MSG@7 PS12, Line 7: soc/intel/tigerlake: Support to initialize Memory
This still needs to be addressed.
Done
https://review.coreboot.org/c/coreboot/+/39847/12//COMMIT_MSG@9 PS12, Line 9: which uses SMBus address
Why is this being restricted to SoDIMM memory only?
Done
https://review.coreboot.org/c/coreboot/+/39847/12//COMMIT_MSG@10 PS12, Line 10: Added
Add
Done
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... PS15, Line 99: /* DQ mapping */
I think it would be helpful to add some information like lpddr4x. Example: […]
Done
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... PS15, Line 101: /* : * DQS CPU<>DRAM map. Each array entry represents a : * mapping of a dq bit on the CPU to the bit it's connected to on : * the memory part. The array index represents the dqs bit number : * on the memory part, and the values in the array represent which : * pin on the CPU that DRAM pin connects to. : */
/* […]
Done
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... PS15, Line 121: vref_ca_config
This seems to be unused.
Its Used in deltan, right?, its being configured to 0, since we have DDR_A_VREF_CA and DDR_B_VREF_CA in deltan schematics.
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... PS15, Line 131: SO-DIMM based
Not true anymore.
Done
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 307: if ((info->topology == MEMORY_DOWN) || (info->topology == MIXED)) {
A comment here would be helpful indicating that for mixed topologies, ch0 can only be MD. […]
Ack
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 312: if
nit: else if
Ack
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 310: : : if
nit: else if (info->topology == SODIMM) { […]
Ack
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 318: if
yes, then use else to print some message.. channel out of config/support etc..
Ack
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 318: if
nit: else if
Ack
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 321: return
Using `else if` should avoid the need of adding early return here.
Ack
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 324: || (info->topology == MIXED)
This is not correct. If mixed topology is used, channel1 has to be SODIMM. […]
Ack
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 328: if
nit: else if
Ack
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 342: = 0
I don't think these need to be set. It is initialized before being used.
It throws me warning that it can be used uninitialized in "init_spd_upds(mem_cfg, i, spd_dimm0, spd_dimm1);"
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 343: = 0
same here.
Ack
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 349: = { : .addr_map[0] = info->smbus_info[0].addr_dimm0, : .addr_map[1] = info->smbus_info[0].addr_dimm1, : .addr_map[2] = info->smbus_info[1].addr_dimm0, : .addr_map[3] = info->smbus_info[1].addr_dimm1, : };
nit: Since you have a separate function read_sodimm_spd(), I think it makes sense to move setting of […]
Ack
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 361: if
nit: else if
Ack
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 392: for dqs pair
for *every* dqs pair
Ack
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 403: unsigned int i
Can you please move this declaration to start of this function. […]
Ack
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 405: init_dq_upds(mem_cfg, 4i+b, board_cfg->dq_map[i][b], : board_cfg->dq_map[i][b+1]); : init_dqs_upds(mem_cfg, 4i+b, board_cfg->dqs_map[i][b], : board_cfg->dqs_map[i][b+1]);
This will have to take into account empty v/s populated slots. […]
Ack
https://review.coreboot.org/c/coreboot/+/39847/16/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/16/src/soc/intel/tigerlake/me... PS16, Line 292: read_sodimm_spd
To make this consistent with memory-down, it would be good to also add a call to printing out SPD in […]
Done
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize Memory ......................................................................
Patch Set 19:
(15 comments)
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 307: if ((info->topology == MEMORY_DOWN) || (info->topology == MIXED)) {
Ack
Done
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 312: if
Ack
Done
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 310: : : if
Ack
Done
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 318: if
Ack
Done
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 318: if
Ack
Done
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 321: return
Ack
Done
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 324: || (info->topology == MIXED)
Ack
Done
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 328: if
Ack
Done
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 349: = { : .addr_map[0] = info->smbus_info[0].addr_dimm0, : .addr_map[1] = info->smbus_info[0].addr_dimm1, : .addr_map[2] = info->smbus_info[1].addr_dimm0, : .addr_map[3] = info->smbus_info[1].addr_dimm1, : };
Ack
Done
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 361: if
Ack
Making it if ((info->topology == SODIMM) || (info->topology == MIXED)) and include Mixed SPD Data len check as mentioned by Furquan below.
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 373: DDR4x
Just DDR4.
Done
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 377: for dq pair
for *every* dq pair
Done
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 389: DDR4x
Just DDR4
Done
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 403: unsigned int i
Ack
Done
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 405: init_dq_upds(mem_cfg, 4i+b, board_cfg->dq_map[i][b], : board_cfg->dq_map[i][b+1]); : init_dqs_upds(mem_cfg, 4i+b, board_cfg->dqs_map[i][b], : board_cfg->dqs_map[i][b+1]);
Ack
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize Memory ......................................................................
Patch Set 19:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39847/19//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39847/19//COMMIT_MSG@7 PS19, Line 7: Memory DDR4 memory?
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 331: printk(BIOS_DEBUG, "Unsupported channels"); I think this is worth a die() call instead of just a print.
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 407: 4i+b 4 * i + b?
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 409: 4i+b 4 * i + b?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize Memory ......................................................................
Patch Set 19:
(15 comments)
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/in... PS19, Line 98: ddr4x_ ddr4
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... PS15, Line 121: vref_ca_config
Its Used in deltan, right?, its being configured to 0, since we have DDR_A_VREF_CA and DDR_B_VREF_CA […]
I meant it is unused in this patch. Don't you need to set some UPD based on this?
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 342: = 0
It throws me warning that it can be used uninitialized in "init_spd_upds(mem_cfg, i, spd_dimm0, spd_ […]
Probably because ddr4_get_spd() has a branch where spd_dimm0 and spd_dimm1 are not set. Not sure if you add die the compiler would be smart enough to understand that. Something to try.
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 296: , ;
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 297: , ;
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 298: , ;
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 299: , ;
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 303: uintptr_t const
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 304: struct const
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 318: BIOS_INFO This is not just info, it is an error. Probably die() like Tim suggested.
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 325: } else if ((info->topology == SODIMM) || (info->topology == MIXED)) { Comment would be helpful: /* For mixed topology, channel 1 can only be SODIMM */
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 329: printk(BIOS_DEBUG, "Undefined memory topology on channel 1"); Same here.
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 343: = 0 Not required.
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 407: 4i+b
4 * i + b?
Or just use index?
init_dq_upds(memcfg, index, ...);
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 409: 4i+b
4 * i + b?
Same here.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize Memory ......................................................................
Patch Set 19:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 409: 4i+b
Same here.
Actually I can't get init_dqs_upds meaning... In this function seems like do nothing? No return and no copy the upds. Weird to me.
Hello Bora Guvendik, build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Kevin Chowski, Selma Bensaid, Anil Kumar K, Varun Joshi, Bernardo Perez Priego, Patrick Rudolph, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39847
to look at the new patch set (#20).
Change subject: soc/intel/tigerlake: Add support to initialize Memory ......................................................................
soc/intel/tigerlake: Add support to initialize Memory
Support to configure DDR4 memory variant. -Add support to read SPD data based on different memory topology. -Initialize FSP UPD's for DQ and DQS mapping.
BUG=b:151702387
Signed-off-by: Varun Joshi varun.joshi@intel.corp-partner.google.com Change-Id: I47a5dcad3ee316871a6103b9d53ef7f6fc88d7d8 --- M src/soc/intel/tigerlake/include/soc/meminit_tgl.h M src/soc/intel/tigerlake/meminit_tgl.c 2 files changed, 164 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/39847/20
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize Memory ......................................................................
Patch Set 20:
(14 comments)
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... PS15, Line 101: /* : * DQS CPU<>DRAM map. Each array entry represents a : * mapping of a dq bit on the CPU to the bit it's connected to on : * the memory part. The array index represents the dqs bit number : * on the memory part, and the values in the array represent which : * pin on the CPU that DRAM pin connects to. : */
Done
No longer required
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/me... PS15, Line 363: mem_cfg->MemorySpdDataLen = spd_sodimm_len;
How do you plan to ensure that MemorySpdDataLen is correct in MIXED topology? […]
Done
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 296: ,
;
Done
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 297: ,
;
Done
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 299: ,
;
Done
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 303: uintptr_t
const
Done
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 304: struct
const
Done
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 318: BIOS_INFO
This is not just info, it is an error. Probably die() like Tim suggested.
Done
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 325: } else if ((info->topology == SODIMM) || (info->topology == MIXED)) {
Comment would be helpful: […]
Done
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 329: printk(BIOS_DEBUG, "Undefined memory topology on channel 1");
Same here.
Done
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 331: printk(BIOS_DEBUG, "Unsupported channels");
I think this is worth a die() call instead of just a print.
Done
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 343: = 0
Not required.
Done
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 407: 4i+b
Or just use index? […]
Done
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 409: 4i+b
Actually I can't get init_dqs_upds meaning... […]
It is initializing Fspm params.
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize Memory ......................................................................
Patch Set 20:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/in... PS19, Line 98: ddr4x_
ddr4
Done
Hello Bora Guvendik, build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Kevin Chowski, Selma Bensaid, Anil Kumar K, Varun Joshi, Bernardo Perez Priego, Patrick Rudolph, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39847
to look at the new patch set (#21).
Change subject: soc/intel/tigerlake: Add support to initialize Memory ......................................................................
soc/intel/tigerlake: Add support to initialize Memory
Support to configure DDR4 memory variant. -Add support to read SPD data based on different memory topology. -Initialize FSP UPD's for DQ and DQS mapping.
BUG=b:151702387
Signed-off-by: Varun Joshi varun.joshi@intel.corp-partner.google.com Change-Id: I47a5dcad3ee316871a6103b9d53ef7f6fc88d7d8 --- M src/soc/intel/tigerlake/include/soc/meminit_tgl.h M src/soc/intel/tigerlake/meminit_tgl.c 2 files changed, 168 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/39847/21
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize Memory ......................................................................
Patch Set 21:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 409: 4i+b
It is initializing Fspm params.
Done
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize Memory ......................................................................
Patch Set 21:
(19 comments)
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... PS15, Line 121: vref_ca_config
I meant it is unused in this patch. […]
Done
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... PS12, Line 23:
Please make it 3 tabs to be consistent with the defines above/
Done
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... PS12, Line 100: mb_ddr4x_cfg
ddr4x_cfg to have similar naming convention to lpddr4x_cfg above.
Done
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... PS12, Line 102: DQ_PER_CHANNEL
#define for this is missing.
Done
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... PS12, Line 102: dq_map
This should actually be organized as: […]
Done
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... PS12, Line 104: * DQS CPU<>DRAM map. Each array entry represents a : * mapping of a dq bit on the CPU to the bit it's connected to on : * the memory part. The array index represents the dqs bit number : * on the memory part, and the values in the array represent which : * pin on the CPU that DRAM pin connects to.
Please re-flow for 80 or 96 characters per line.
Done
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... PS12, Line 110: dqs_map
uint8_t dqs_map[DDR4_CHANNELS][DDR4_BYTES_PER_CHANNEL]; […]
Done
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... PS12, Line 110: DQS_PER_CHANNEL
#define for this is missing.
Done
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... PS12, Line 111: /* : * Rcomp resistor value. The value represent the resistance in ohms : * of the rcomp resistor attached to the DDR_COMP pins on the DRAM. : */ : uint16_t rcomp_resistor;
This is not required even for DDR4. We had got a confirmation from Intel here: https://issuetracker. […]
Done
https://review.coreboot.org/c/coreboot/+/39847/12/src/soc/intel/tigerlake/in... PS12, Line 139: _sodimm
This function can be simply named: meminit_ddr4x. […]
Done
https://review.coreboot.org/c/coreboot/+/39847/10/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/10/src/soc/intel/tigerlake/me... PS10, Line 271: /*First set of Byte0- Byte 7 in Dqmap is through channel0,
nits:Please fix the comment indent.
Done
https://review.coreboot.org/c/coreboot/+/39847/18/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/18/src/soc/intel/tigerlake/me... PS18, Line 320: else
else should follow close brace '}'
Done
https://review.coreboot.org/c/coreboot/+/39847/18/src/soc/intel/tigerlake/me... PS18, Line 321: printk(BIOS_INFO,"Undefined memory topology on Channel 0");
space required after that ',' (ctx:VxV)
Done
https://review.coreboot.org/c/coreboot/+/39847/18/src/soc/intel/tigerlake/me... PS18, Line 329: else if ((info->topology == MEMORY_DOWN)) {
Unnecessary parentheses - maybe == should be = ?
Done
https://review.coreboot.org/c/coreboot/+/39847/18/src/soc/intel/tigerlake/me... PS18, Line 333: else if ((info->topology == SODIMM) || (info->topology == MIXED)) {
else should follow close brace '}'
Done
https://review.coreboot.org/c/coreboot/+/39847/18/src/soc/intel/tigerlake/me... PS18, Line 337: else
Statements should start on a tabstop
Done
https://review.coreboot.org/c/coreboot/+/39847/18/src/soc/intel/tigerlake/me... PS18, Line 340: else
Statements should start on a tabstop
Done
https://review.coreboot.org/c/coreboot/+/39847/18/src/soc/intel/tigerlake/me... PS18, Line 417: else {
Statements should start on a tabstop
Done
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/19/src/soc/intel/tigerlake/me... PS19, Line 298: ,
;
Done
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize Memory ......................................................................
Patch Set 21:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39847/15//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39847/15//COMMIT_MSG@9 PS15, Line 9: hich uses SMBus address
not true anymore.
Done
https://review.coreboot.org/c/coreboot/+/39847/3/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/3/src/soc/intel/tigerlake/mem... PS3, Line 194: break;
Please see my comments on patchset 1.
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize Memory ......................................................................
Patch Set 21:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39847/21//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39847/21//COMMIT_MSG@7 PS21, Line 7: Memory nit: DDR4 memory
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... PS15, Line 121: vref_ca_config
Done
So, how would the UPD for this be set? Or is that not required?
https://review.coreboot.org/c/coreboot/+/39847/21/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/21/src/soc/intel/tigerlake/me... PS21, Line 299: print_spd_info((unsigned char *)blk->spd_array[0]); : print_spd_info((unsigned char *)blk->spd_array[1]); : print_spd_info((unsigned char *)blk->spd_array[2]); : print_spd_info((unsigned char *)blk->spd_array[3]); Doesn't this end up printing garbage if done unconditionally in case all DIMMs are not populated?
for (i = 0; i < ARRAY_SIZE(blk->addr_map); i++) { if (blk->addr_map[i]) print_spd_info((unsigned char *)blk->spd_array[i]); }
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize Memory ......................................................................
Patch Set 21:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/21/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/21/src/soc/intel/tigerlake/me... PS21, Line 299: print_spd_info((unsigned char *)blk->spd_array[0]); : print_spd_info((unsigned char *)blk->spd_array[1]); : print_spd_info((unsigned char *)blk->spd_array[2]); : print_spd_info((unsigned char *)blk->spd_array[3]);
Doesn't this end up printing garbage if done unconditionally in case all DIMMs are not populated? […]
This might. Can print in "ddr4_get_spd" where it is initialized?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize Memory ......................................................................
Patch Set 21:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/21/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/21/src/soc/intel/tigerlake/me... PS21, Line 299: print_spd_info((unsigned char *)blk->spd_array[0]); : print_spd_info((unsigned char *)blk->spd_array[1]); : print_spd_info((unsigned char *)blk->spd_array[2]); : print_spd_info((unsigned char *)blk->spd_array[3]);
This might. […]
That works. But, then you will have to add a special case for Memory down, since you would end up printing the same information multiple times. Adding the loop above should allow you to print the info of only the dimms that are populated based on addr_map?
Selma Bensaid has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize Memory ......................................................................
Patch Set 21:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39847/20/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/20/src/soc/intel/tigerlake/me... PS20, Line 318: " \n
https://review.coreboot.org/c/coreboot/+/39847/20/src/soc/intel/tigerlake/me... PS20, Line 330: " \n
https://review.coreboot.org/c/coreboot/+/39847/20/src/soc/intel/tigerlake/me... PS20, Line 332: " \n
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize Memory ......................................................................
Patch Set 21:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/21/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/21/src/soc/intel/tigerlake/me... PS21, Line 299: print_spd_info((unsigned char *)blk->spd_array[0]); : print_spd_info((unsigned char *)blk->spd_array[1]); : print_spd_info((unsigned char *)blk->spd_array[2]); : print_spd_info((unsigned char *)blk->spd_array[3]);
That works. […]
Yes that's correct too, I will add the loop.
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize Memory ......................................................................
Patch Set 21:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... PS15, Line 121: vref_ca_config
So, how would the UPD for this be set? Or is that not required?
Actually, its not required.
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize Memory ......................................................................
Patch Set 21:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39847/15/src/soc/intel/tigerlake/in... PS15, Line 121: vref_ca_config
Actually, its not required.
Done
Hello Bora Guvendik, build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Kevin Chowski, Selma Bensaid, Anil Kumar K, Varun Joshi, Bernardo Perez Priego, Patrick Rudolph, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39847
to look at the new patch set (#22).
Change subject: soc/intel/tigerlake: Add support to initialize DDR4 Memory ......................................................................
soc/intel/tigerlake: Add support to initialize DDR4 Memory
Support to configure DDR4 memory variant. -Add support to read SPD data based on different memory topology. -Initialize FSP UPD's for DQ and DQS mapping.
BUG=b:151702387
Signed-off-by: Varun Joshi varun.joshi@intel.corp-partner.google.com Change-Id: I47a5dcad3ee316871a6103b9d53ef7f6fc88d7d8 --- M src/soc/intel/tigerlake/include/soc/meminit_tgl.h M src/soc/intel/tigerlake/meminit_tgl.c 2 files changed, 172 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/39847/22
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize DDR4 Memory ......................................................................
Patch Set 22:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39847/20/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/20/src/soc/intel/tigerlake/me... PS20, Line 330: "
\n
Done
https://review.coreboot.org/c/coreboot/+/39847/20/src/soc/intel/tigerlake/me... PS20, Line 332: "
\n
Done
https://review.coreboot.org/c/coreboot/+/39847/21/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/21/src/soc/intel/tigerlake/me... PS21, Line 299: print_spd_info((unsigned char *)blk->spd_array[0]); : print_spd_info((unsigned char *)blk->spd_array[1]); : print_spd_info((unsigned char *)blk->spd_array[2]); : print_spd_info((unsigned char *)blk->spd_array[3]);
Yes that's correct too, I will add the loop.
Done
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize DDR4 Memory ......................................................................
Patch Set 22:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/22/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/22/src/soc/intel/tigerlake/me... PS22, Line 304: if(blk->addr_map[i]) space required before the open parenthesis '('
Hello Bora Guvendik, build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Kevin Chowski, Selma Bensaid, Anil Kumar K, Varun Joshi, Bernardo Perez Priego, Patrick Rudolph, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39847
to look at the new patch set (#23).
Change subject: soc/intel/tigerlake: Add support to initialize DDR4 Memory ......................................................................
soc/intel/tigerlake: Add support to initialize DDR4 Memory
Support to configure DDR4 memory variant. -Add support to read SPD data based on different memory topology. -Initialize FSP UPD's for DQ and DQS mapping.
BUG=b:151702387
Signed-off-by: Varun Joshi varun.joshi@intel.corp-partner.google.com Change-Id: I47a5dcad3ee316871a6103b9d53ef7f6fc88d7d8 --- M src/soc/intel/tigerlake/include/soc/meminit_tgl.h M src/soc/intel/tigerlake/meminit_tgl.c 2 files changed, 172 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/39847/23
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize DDR4 Memory ......................................................................
Patch Set 23:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/20/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/20/src/soc/intel/tigerlake/me... PS20, Line 318: "
\n
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize DDR4 Memory ......................................................................
Patch Set 23: Code-Review+2
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize DDR4 Memory ......................................................................
Patch Set 23:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/21//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39847/21//COMMIT_MSG@7 PS21, Line 7: Memory
nit: DDR4 memory
Done
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize DDR4 Memory ......................................................................
Patch Set 23:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/19//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39847/19//COMMIT_MSG@7 PS19, Line 7: Memory
DDR4 memory?
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize DDR4 Memory ......................................................................
Patch Set 23: Code-Review+2
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize DDR4 Memory ......................................................................
Patch Set 23:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/23/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/23/src/soc/intel/tigerlake/me... PS23, Line 410: if (half_populated && (i == 1)) { half_populated is means i%2? 1,3,5,7.. if we have should be empty?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize DDR4 Memory ......................................................................
Patch Set 23:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/23/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/23/src/soc/intel/tigerlake/me... PS23, Line 410: if (half_populated && (i == 1)) {
half_populated is means i%2? 1,3,5,7.. […]
DDR4 interface supports 2 channels. When configuration is half populated, then channel 1 is left empty. DQ/DQS UPDs 4,5,6,7 map to channel 1, hence they are left empty.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize DDR4 Memory ......................................................................
Patch Set 23: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/23/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/23/src/soc/intel/tigerlake/me... PS23, Line 410: if (half_populated && (i == 1)) {
DDR4 interface supports 2 channels. […]
oh, I misunderstood this. Thanks.
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize DDR4 Memory ......................................................................
Patch Set 23:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39847/23/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39847/23/src/soc/intel/tigerlake/me... PS23, Line 410: if (half_populated && (i == 1)) {
oh, I misunderstood this. Thanks.
Done
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize DDR4 Memory ......................................................................
Patch Set 23:
Patch Set 23: Code-Review+2
Thank You for all the help!!
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize DDR4 Memory ......................................................................
Patch Set 23:
Looks like there is a merge conflict that you need to resolve before it can be submitted.
Hello Bora Guvendik, build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Kevin Chowski, Selma Bensaid, Anil Kumar K, Varun Joshi, Bernardo Perez Priego, Patrick Rudolph, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39847
to look at the new patch set (#24).
Change subject: soc/intel/tigerlake: Add support to initialize DDR4 Memory ......................................................................
soc/intel/tigerlake: Add support to initialize DDR4 Memory
Support to configure DDR4 memory variant. -Add support to read SPD data based on different memory topology. -Initialize FSP UPD's for DQ and DQS mapping.
BUG=b:151702387
Signed-off-by: Varun Joshi varun.joshi@intel.corp-partner.google.com Change-Id: I47a5dcad3ee316871a6103b9d53ef7f6fc88d7d8 --- M src/soc/intel/tigerlake/include/soc/meminit.h M src/soc/intel/tigerlake/meminit.c 2 files changed, 172 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/39847/24
Varun Joshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize DDR4 Memory ......................................................................
Patch Set 24:
Patch Set 23:
Looks like there is a merge conflict that you need to resolve before it can be submitted.
Pushed a patch on top of your merged patches.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize DDR4 Memory ......................................................................
Patch Set 24: Code-Review+2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize DDR4 Memory ......................................................................
Patch Set 24: Code-Review+2
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize DDR4 Memory ......................................................................
Patch Set 24: Code-Review+2
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize DDR4 Memory ......................................................................
soc/intel/tigerlake: Add support to initialize DDR4 Memory
Support to configure DDR4 memory variant. -Add support to read SPD data based on different memory topology. -Initialize FSP UPD's for DQ and DQS mapping.
BUG=b:151702387
Signed-off-by: Varun Joshi varun.joshi@intel.corp-partner.google.com Change-Id: I47a5dcad3ee316871a6103b9d53ef7f6fc88d7d8 Reviewed-on: https://review.coreboot.org/c/coreboot/+/39847 Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-by: EricR Lai ericr_lai@compal.corp-partner.google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/tigerlake/include/soc/meminit.h M src/soc/intel/tigerlake/meminit.c 2 files changed, 172 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved EricR Lai: Looks good to me, approved Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/soc/intel/tigerlake/include/soc/meminit.h b/src/soc/intel/tigerlake/include/soc/meminit.h index a2fb3f4..3c4c16b 100644 --- a/src/soc/intel/tigerlake/include/soc/meminit.h +++ b/src/soc/intel/tigerlake/include/soc/meminit.h @@ -87,7 +87,44 @@ * the array represents DQ pin# on the memory part. */ uint8_t dqs_map[LPDDR4X_CHANNELS][LPDDR4X_BYTES_PER_CHANNEL]; + /* + * Early Command Training Enable/Disable Control + * 1 = enable, 0 = disable + */ + uint8_t ect; +};
+/* Board-specific memory configuration information for DDR4 memory variant */ +struct mb_ddr4_cfg { + /* + * DQ CPU<>DRAM map: + * DDR4 memory interface has 8 DQs per channel. Each DQ consists of 8 bits(1 + * byte). Thus, dq_map is represented as DDR[1-0]_DQ[7-0][7:0], where + * DDR[1-0] : DDR4 channel # + * DQ[7-0] : DQ # within the channel + * [7:0] : Bits within the DQ + * + * Index of the array represents DQ pin# on the CPU, whereas value in + * the array represents DQ pin# on the memory part. + */ + uint8_t dq_map[DDR4_CHANNELS][DDR4_BYTES_PER_CHANNEL][BITS_PER_BYTE]; + /* + * DQS CPU<>DRAM map: + * DDR4 memory interface has 8 DQS pairs per channel. Thus, dqs_map is represented as + * DDR[1-0]_DQS[7-0], where + * DDR[1-0] : DDR4 channel # + * DQS[7-0] : DQS # within the channel + * + * Index of the array represents DQS pin# on the CPU, whereas value in + * the array represents DQS pin# on the memory part. + */ + uint8_t dqs_map[DDR4_CHANNELS][DDR4_BYTES_PER_CHANNEL]; + /* + * Indicates whether memory is interleaved. + * Set to 1 for an interleaved design, + * set to 0 for non-interleaved design. + */ + uint8_t dq_pins_interleaved; /* * Early Command Training Enable/Disable Control * 1 = enable, 0 = disable @@ -97,5 +134,7 @@
void meminit_lpddr4x(FSP_M_CONFIG *mem_cfg, const struct lpddr4x_cfg *board_cfg, const struct spd_info *spd, bool half_populated); - +/* Initialize DDR4 memory configurations */ +void meminit_ddr4(FSP_M_CONFIG *mem_cfg, const struct mb_ddr4_cfg *board_cfg, + const struct spd_info *spd, const bool half_populated); #endif /* _SOC_TIGERLAKE_MEMINIT_H_ */ diff --git a/src/soc/intel/tigerlake/meminit.c b/src/soc/intel/tigerlake/meminit.c index bd9a4ff..7823cfe 100644 --- a/src/soc/intel/tigerlake/meminit.c +++ b/src/soc/intel/tigerlake/meminit.c @@ -296,3 +296,135 @@ board_cfg->dqs_map[i][1]); } } + +static void read_sodimm_spd(const struct spd_info *info, struct spd_block *blk) +{ + unsigned int i; + + blk->addr_map[0] = info->smbus_info[0].addr_dimm0; + blk->addr_map[1] = info->smbus_info[0].addr_dimm1; + blk->addr_map[2] = info->smbus_info[1].addr_dimm0; + blk->addr_map[3] = info->smbus_info[1].addr_dimm1; + + get_spd_smbus(blk); + + for (i = 0; i < ARRAY_SIZE(blk->addr_map); i++) { + if (blk->addr_map[i]) + print_spd_info((unsigned char *)blk->spd_array[i]); + } +} + +static void ddr4_get_spd(unsigned int channel, const uintptr_t *spd_md_data, + const struct spd_block *spd_sodimm_blk, + const struct spd_info *info, + const bool half_populated, uintptr_t *spd_dimm0, + uintptr_t *spd_dimm1) +{ + if (channel == 0) { + /* For mixed topology, channel 0 can only be Memory_Down */ + if ((info->topology == MEMORY_DOWN) || (info->topology == MIXED)) { + *spd_dimm0 = *spd_md_data; + *spd_dimm1 = 0; + } else if (info->topology == SODIMM) { + *spd_dimm0 = (uintptr_t)spd_sodimm_blk->spd_array[0]; + *spd_dimm1 = (uintptr_t)spd_sodimm_blk->spd_array[1]; + } else + die("Undefined memory topology on Channel 0.\n"); + } else if (channel == 1) { + if (half_populated) { + *spd_dimm0 = *spd_dimm1 = 0; + } else if (info->topology == MEMORY_DOWN) { + *spd_dimm0 = *spd_md_data; + *spd_dimm1 = 0; + /* For mixed topology, channel 1 can only be SODIMM */ + } else if ((info->topology == SODIMM) || (info->topology == MIXED)) { + *spd_dimm0 = (uintptr_t)spd_sodimm_blk->spd_array[2]; + *spd_dimm1 = (uintptr_t)spd_sodimm_blk->spd_array[3]; + } else + die("Undefined memory topology on channel 1.\n"); + } else + die("Unsupported channels.\n"); +} + +/* Initialize DDR4 memory configurations */ +void meminit_ddr4(FSP_M_CONFIG *mem_cfg, const struct mb_ddr4_cfg *board_cfg, + const struct spd_info *info, const bool half_populated) +{ + uintptr_t spd_md_data; + size_t spd_md_len; + uintptr_t spd_dimm0 = 0; + uintptr_t spd_dimm1 = 0; + struct spd_block spd_sodimm_blk; + unsigned int i; + unsigned int index = 0; + + /* Early Command Training Enabled */ + mem_cfg->ECT = board_cfg->ect; + mem_cfg->DqPinsInterleaved = board_cfg->dq_pins_interleaved; + + if ((info->topology == MEMORY_DOWN) || (info->topology == MIXED)) { + read_md_spd(info, &spd_md_data, &spd_md_len); + mem_cfg->MemorySpdDataLen = spd_md_len; + } + + if ((info->topology == SODIMM) || (info->topology == MIXED)) { + read_sodimm_spd(info, &spd_sodimm_blk); + if ((info->topology == MIXED) && + (mem_cfg->MemorySpdDataLen != spd_sodimm_blk.len)) + die("Mixed topology has incorrect length.\n"); + else + mem_cfg->MemorySpdDataLen = spd_sodimm_blk.len; + } + + for (i = 0; i < DDR4_CHANNELS; i++) { + ddr4_get_spd(i, &spd_md_data, &spd_sodimm_blk, info, + half_populated, &spd_dimm0, &spd_dimm1); + init_spd_upds(mem_cfg, i, spd_dimm0, spd_dimm1); + } + + /* + * DDR4 memory interface has 8 DQs per channel. Each DQ consists of 8 bits (1 + * byte). However, FSP UPDs for DQ Map expect a DQ pair (i.e. mapping for 2 bytes) in + * each UPD. + * + * Thus, init_dq_upds() needs to be called for every dq pair of each channel. + * DqMapCpu2DramCh0 --> dq_map[CHAN=0][0-1] + * DqMapCpu2DramCh1 --> dq_map[CHAN=0][2-3] + * DqMapCpu2DramCh2 --> dq_map[CHAN=0][4-5] + * DqMapCpu2DramCh3 --> dq_map[CHAN=0][6-7] + * DqMapCpu2DramCh4 --> dq_map[CHAN=1][0-1] + * DqMapCpu2DramCh5 --> dq_map[CHAN=1][2-3] + * DqMapCpu2DramCh6 --> dq_map[CHAN=1][4-5] + * DqMapCpu2DramCh7 --> dq_map[CHAN=1][6-7] + */ + + /* + * DDR4 memory interface has 8 DQS pairs per channel. FSP UPDs for DQS Map expect a + * pair in each UPD. + * + * Thus, init_dqs_upds() needs to be called for every dqs pair of each channel. + * DqsMapCpu2DramCh0 --> dqs_map[CHAN=0][0-1] + * DqsMapCpu2DramCh1 --> dqs_map[CHAN=0][2-3] + * DqsMapCpu2DramCh2 --> dqs_map[CHAN=0][4-5] + * DqsMapCpu2DramCh3 --> dqs_map[CHAN=0][6-7] + * DqsMapCpu2DramCh4 --> dqs_map[CHAN=1][0-1] + * DqsMapCpu2DramCh5 --> dqs_map[CHAN=1][2-3] + * DqsMapCpu2DramCh6 --> dqs_map[CHAN=1][4-5] + * DqsMapCpu2DramCh7 --> dqs_map[CHAN=1][6-7] + */ + + for (i = 0; i < DDR4_CHANNELS; i++) { + for (int b = 0; b < DDR4_BYTES_PER_CHANNEL; b += 2) { + if (half_populated && (i == 1)) { + init_dq_upds_empty(mem_cfg, index); + init_dqs_upds_empty(mem_cfg, index); + } else { + init_dq_upds(mem_cfg, index, board_cfg->dq_map[i][b], + board_cfg->dq_map[i][b+1]); + init_dqs_upds(mem_cfg, index, board_cfg->dqs_map[i][b], + board_cfg->dqs_map[i][b+1]); + } + index++; + } + } +}
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39847 )
Change subject: soc/intel/tigerlake: Add support to initialize DDR4 Memory ......................................................................
Patch Set 27:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/2228 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2227 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2226
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Bora Guvendik (Code Review)
-
build bot (Jenkins) (Code Review)
-
EricR Lai (Code Review)
-
Furquan Shaikh (Code Review)
-
Paul Menzel (Code Review)
-
Selma Bensaid (Code Review)
-
Tim Wawrzynczak (Code Review)
-
Varun Joshi (Code Review)