Srinidhi N Kaushik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39040 )
Change subject: soc/intel/tigerlake: add Rcomp data structure ......................................................................
soc/intel/tigerlake: add Rcomp data structure
Add Rcomp data structure in meminit to support updating rcomp resistor and target values for DDR4 support.
BUG=none BRANCH=none TEST= build tglrvp flash and boot to kernel
Signed-off-by: Srinidhi N Kaushik srinidhi.n.kaushik@intel.com Change-Id: I3858cc56c838862fc61123c8b7dba11dbc40983c --- M src/soc/intel/tigerlake/include/soc/meminit_tgl.h 1 file changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/39040/1
diff --git a/src/soc/intel/tigerlake/include/soc/meminit_tgl.h b/src/soc/intel/tigerlake/include/soc/meminit_tgl.h old mode 100644 new mode 100755 index dd05418..c093304 --- a/src/soc/intel/tigerlake/include/soc/meminit_tgl.h +++ b/src/soc/intel/tigerlake/include/soc/meminit_tgl.h @@ -17,6 +17,7 @@ #define BITS_PER_BYTE 8 #define DQS_PER_CHANNEL 2 #define NUM_CHANNELS 8 +#define NUM_RCOMP_TARGETS 5
struct spd_by_pointer { size_t spd_data_len; @@ -61,6 +62,16 @@ uint8_t ect; };
+/* Rcomp configuration information */ +struct rcomp_cfg { + /* Rcomp Resistor */ + uint16_t rcomp_resistor; + + /* Rcomp Target */ + uint16_t rcomp_target[NUM_RCOMP_TARGETS]; + +}; + /* Initialize default memory configurations for dimm0-only lpddr4x */ void meminit_lpddr4x_dimm0(FSP_M_CONFIG *mem_cfg, const struct mb_lpddr4x_cfg *board_cfg,
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39040
to look at the new patch set (#2).
Change subject: soc/intel/tigerlake: add Rcomp data structure ......................................................................
soc/intel/tigerlake: add Rcomp data structure
Add Rcomp data structure in meminit to support updating rcomp resistor and target values for DDR4 support.
BUG=none BRANCH=none TEST= build tglrvp flash and boot to kernel
Signed-off-by: Srinidhi N Kaushik srinidhi.n.kaushik@intel.com Change-Id: I3858cc56c838862fc61123c8b7dba11dbc40983c --- M src/soc/intel/tigerlake/include/soc/meminit_tgl.h 1 file changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/39040/2
Srinidhi N Kaushik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39040 )
Change subject: soc/intel/tigerlake: add Rcomp data structure ......................................................................
Patch Set 2: Code-Review+1
Hello Raj Astekar, Patrick Rudolph, Nick Vaccaro, Wonkyu Kim, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39040
to look at the new patch set (#3).
Change subject: soc/intel/tigerlake: add Rcomp data structure ......................................................................
soc/intel/tigerlake: add Rcomp data structure
Add Rcomp data structure in meminit to support updating rcomp resistor and target values for DDR4 support.
BUG=none BRANCH=none TEST= build tglrvp flash and boot to kernel
Signed-off-by: Srinidhi N Kaushik srinidhi.n.kaushik@intel.com Change-Id: I3858cc56c838862fc61123c8b7dba11dbc40983c --- M src/soc/intel/tigerlake/include/soc/meminit_tgl.h 1 file changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/39040/3
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39040 )
Change subject: soc/intel/tigerlake: add Rcomp data structure ......................................................................
Patch Set 3: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39040 )
Change subject: soc/intel/tigerlake: add Rcomp data structure ......................................................................
Patch Set 3: Code-Review-1
(1 comment)
This does not match what was discussed here: b/142650263. Please see comment#18.
https://review.coreboot.org/c/coreboot/+/39040/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39040/3//COMMIT_MSG@9 PS3, Line 9: Add Rcomp data structure in meminit to support updating rcomp resistor : and target values for DDR4 support. Where is the SoC code that uses this information?
Hello Raj Astekar, Patrick Rudolph, Nick Vaccaro, Wonkyu Kim, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39040
to look at the new patch set (#4).
Change subject: soc/intel/tigerlake: add DDR4 support ......................................................................
soc/intel/tigerlake: add DDR4 support
Add functions in meminit to support DDR4 memory type.
BUG=none BRANCH=none TEST= build tglrvp flash and boot to kernel
Signed-off-by: Srinidhi N Kaushik srinidhi.n.kaushik@intel.com Change-Id: I3858cc56c838862fc61123c8b7dba11dbc40983c --- M src/soc/intel/tigerlake/include/soc/meminit_tgl.h M src/soc/intel/tigerlake/meminit_tgl.c 2 files changed, 42 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/39040/4
Srinidhi N Kaushik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39040 )
Change subject: soc/intel/tigerlake: add DDR4 support ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39040/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39040/3//COMMIT_MSG@9 PS3, Line 9: Add Rcomp data structure in meminit to support updating rcomp resistor : and target values for DDR4 support.
Where is the SoC code that uses this information?
Will create a function like meminit_ddr4x() in soc, currently it was done inline in fsp_params.c
Srinidhi N Kaushik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39040 )
Change subject: soc/intel/tigerlake: add DDR4 support ......................................................................
Patch Set 4: Code-Review+1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39040 )
Change subject: soc/intel/tigerlake: add DDR4 support ......................................................................
Patch Set 4:
Patch Set 3: Code-Review-1
(1 comment)
This does not match what was discussed here: b/142650263. Please see comment#18.
Still waiting for confirmation on this.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39040 )
Change subject: soc/intel/tigerlake: add DDR4 support ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/inc... PS4, Line 75: spd_addr_table So, SPD read over SMBUS is the only option for DDR4? Can't a board have SPD in CBFS? Or just provide a memptr to SPD? See enum on line 28.
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/mem... PS4, Line 160: /* Set SpdAddressTable to 0 when using MemorySpdPtr */ : mem_cfg->SpdAddressTable[0] = 0x0; : mem_cfg->SpdAddressTable[1] = 0x0; : mem_cfg->SpdAddressTable[2] = 0x0; : mem_cfg->SpdAddressTable[3] = 0x0; Just use memset?
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/mem... PS4, Line 175: blank line not required.
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/mem... PS4, Line 177: mem_cfg->SpdAddressTable[0] = mem_cfg_data->spd_addr_table[0]; : mem_cfg->SpdAddressTable[1] = mem_cfg_data->spd_addr_table[1]; : mem_cfg->SpdAddressTable[2] = mem_cfg_data->spd_addr_table[2]; : mem_cfg->SpdAddressTable[3] = mem_cfg_data->spd_addr_table[3]; Can we just use get_spd_smbus() to read out the SPD in coreboot and pass that using MemorySpdPtr00, etc.?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39040 )
Change subject: soc/intel/tigerlake: add DDR4 support ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39040/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39040/4//COMMIT_MSG@13 PS4, Line 13: TEST= build tglrvp flash and boot to kernel With what DDR4 configuration?
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/inc... PS4, Line 87: const struct mb_ddr4x_cfg *mem_cfg_data); Should fit on one line.
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/mem... PS4, Line 174: const struct mb_ddr4x_cfg *mem_cfg_data) Should fit on one line.
Srinidhi N Kaushik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39040 )
Change subject: soc/intel/tigerlake: add DDR4 support ......................................................................
Patch Set 4:
(6 comments)
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/inc... PS4, Line 75: spd_addr_table
So, SPD read over SMBUS is the only option for DDR4? Can't a board have SPD in CBFS? Or just provide […]
As far as I know this was the only option right now.
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/inc... PS4, Line 87: const struct mb_ddr4x_cfg *mem_cfg_data);
Should fit on one line.
Ack
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/mem... PS4, Line 160: /* Set SpdAddressTable to 0 when using MemorySpdPtr */ : mem_cfg->SpdAddressTable[0] = 0x0; : mem_cfg->SpdAddressTable[1] = 0x0; : mem_cfg->SpdAddressTable[2] = 0x0; : mem_cfg->SpdAddressTable[3] = 0x0;
Just use memset?
Ack
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/mem... PS4, Line 174: const struct mb_ddr4x_cfg *mem_cfg_data)
Should fit on one line.
Ack
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/mem... PS4, Line 175:
blank line not required.
Ack
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/mem... PS4, Line 177: mem_cfg->SpdAddressTable[0] = mem_cfg_data->spd_addr_table[0]; : mem_cfg->SpdAddressTable[1] = mem_cfg_data->spd_addr_table[1]; : mem_cfg->SpdAddressTable[2] = mem_cfg_data->spd_addr_table[2]; : mem_cfg->SpdAddressTable[3] = mem_cfg_data->spd_addr_table[3];
Can we just use get_spd_smbus() to read out the SPD in coreboot and pass that using MemorySpdPtr00, […]
I need look into how to refactor using the smbus library APIs. Can we do that in the next change set ?
Hello Raj Astekar, Patrick Rudolph, Subrata Banik, Nick Vaccaro, Wonkyu Kim, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39040
to look at the new patch set (#5).
Change subject: soc/intel/tigerlake: add DDR4 mem configuration ......................................................................
soc/intel/tigerlake: add DDR4 mem configuration
Add functions in meminit to support DDR4 memory configuration.
BUG=none BRANCH=none TEST= build tglrvp with DDR4 mem type flash and boot to kernel
Signed-off-by: Srinidhi N Kaushik srinidhi.n.kaushik@intel.com Change-Id: I3858cc56c838862fc61123c8b7dba11dbc40983c --- M src/soc/intel/tigerlake/include/soc/meminit_tgl.h M src/soc/intel/tigerlake/meminit_tgl.c 2 files changed, 31 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/39040/5
Hello Raj Astekar, Patrick Rudolph, Subrata Banik, Nick Vaccaro, Wonkyu Kim, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39040
to look at the new patch set (#6).
Change subject: soc/intel/tigerlake: add DDR4 mem configuration ......................................................................
soc/intel/tigerlake: add DDR4 mem configuration
Add functions in meminit to support DDR4 memory configuration.
BUG=none BRANCH=none TEST= build tglrvp with DDR4 mem type flash and boot to kernel
Signed-off-by: Srinidhi N Kaushik srinidhi.n.kaushik@intel.com Change-Id: I3858cc56c838862fc61123c8b7dba11dbc40983c --- M src/soc/intel/tigerlake/include/soc/meminit_tgl.h M src/soc/intel/tigerlake/meminit_tgl.c 2 files changed, 25 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/39040/6
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39040 )
Change subject: soc/intel/tigerlake: add DDR4 mem configuration ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39040/6/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39040/6/src/soc/intel/tigerlake/inc... PS6, Line 65: Rcomp This is not correct.
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/inc... PS4, Line 75: spd_addr_table
As far as I know this was the only option right now.
Wait. Are you saying that TGL FSP with DDR4 memory does not support passing in SPD data directly? i.e. setting of MemorySpdPtr* UPDs?
Srinidhi N Kaushik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39040 )
Change subject: soc/intel/tigerlake: add DDR4 mem configuration ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39040/6/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39040/6/src/soc/intel/tigerlake/inc... PS6, Line 65: Rcomp
This is not correct.
Ack
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/inc... PS4, Line 75: spd_addr_table
Wait. Are you saying that TGL FSP with DDR4 memory does not support passing in SPD data directly? i. […]
I am not 100% sure about this, I need to check with memory team. I will take an AR on this. I have seen other projects using MemorySpdPtr but in TGL case we have SpdAddressTable,hence we are using it. From the help text it seems like when we set SpdAddressTable to 0 MemorySpdPtr is used. But I am not clear on how to set the spd in CBFS for DDR4.
Hello Raj Astekar, Patrick Rudolph, Subrata Banik, Nick Vaccaro, Wonkyu Kim, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39040
to look at the new patch set (#7).
Change subject: soc/intel/tigerlake: add DDR4 mem configuration ......................................................................
soc/intel/tigerlake: add DDR4 mem configuration
Add functions in meminit to support DDR4 memory configuration.
BUG=none BRANCH=none TEST= build tglrvp with DDR4 mem type flash and boot to kernel
Signed-off-by: Srinidhi N Kaushik srinidhi.n.kaushik@intel.com Change-Id: I3858cc56c838862fc61123c8b7dba11dbc40983c --- M src/soc/intel/tigerlake/include/soc/meminit_tgl.h M src/soc/intel/tigerlake/meminit_tgl.c 2 files changed, 25 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/39040/7
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39040 )
Change subject: soc/intel/tigerlake: add DDR4 mem configuration ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/meminit_tgl.h:
https://review.coreboot.org/c/coreboot/+/39040/4/src/soc/intel/tigerlake/inc... PS4, Line 75: spd_addr_table
I am not 100% sure about this, I need to check with memory team. I will take an AR on this. […]
I don't see a reason why that would be any different than LPDDR4x memory type. You should be able to provide either SpdAddressTable or pointers to actual SPDs. Adding of SPDs to CBFS should not be any different here than what is done on previous platforms.
-
Furquan Shaikh (Code Review) -
Nick Vaccaro (Code Review)
-
Paul Menzel (Code Review)
-
Srinidhi N Kaushik (Code Review)