Meera Ravindranath has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39135 )
Change subject: src/soc/tigerlake: Add memory configuration support for Jasper Lake ......................................................................
Patch Set 17:
(7 comments)
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... PS14, Line 43: READ_SMBUS,
this is valid. […]
Ack
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... PS14, Line 73: meminit_lpddr4()
I think it is meminit_dq_dqs_map that is clearing the unused fields.
Ack. Done.
Also I see that it is set to 0 at the mainboard_config
So the the last 6 bytes of dq map for Ch1 and Ch2 corresponds to DIMM1 and DIMM3 respectively which are unused for JSL. Hence the value 0. So in any case we first clear all the 12 bytes of data and then assign the board values.
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... PS14, Line 117: uint8_t vref_ca_config;
vref_ca config is marked as not applicable for LPDDR4x.
Need to check on this from the MRC team and get back.
Do we still need to configure it? Atleast configured in the mainboard config.
The default UPD value is 2 and we are passing the same from mainboard. Hence removing it.
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/in... PS14, Line 132: )
i believe we have only 1 memory controller on JSL hence those concepts might not be applicable here
I second Subrata. We do not support 2 memory controllers on JSL and think half population won't be applicable here.
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_jsl.c:
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/me... PS14, Line 61: /
Nit: Fix the indentation.
Done
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/me... PS14, Line 82: /* Early Jasper Lake requires rcomp targets to be 0 */
Based on the comment, did you mean memset RcompTarget to 0. […]
This comment is confusing, hence removed it.And no, we don't memset it to 0. We pass the board values. Thanks for the finding.
https://review.coreboot.org/c/coreboot/+/39135/14/src/soc/intel/tigerlake/me... PS14, Line 93: mem_cfg->MemorySpdPtr01 = 0;
No need to set it to spd_data_ptr for LPDDR4x with 4 slots?
LPDDR4 supports 2 channels which means 4 DIMMs can be connected. Since we are connecting the DIMMs only on the 0th and 2nd slot, we don't have to set the spd data pointer to the other two slots.