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