Furquan Shaikh 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 18:
(8 comments)
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/jsl_memcfg_init.h:
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... PS7, Line 103: uint16_t rcomp_resistor[3];
In JSL, we can't auto configure it as the board values deviates and need to be passed in mainboard […]
Doesn't need to block this change, but can you please provide reference to where this information is captured? Do you have a DQ-DQS sheet for JSL similar to the one that is present for previous platforms?
https://review.coreboot.org/c/coreboot/+/39135/7/src/soc/intel/tigerlake/inc... PS7, Line 116: dq_pins_interleaved
Furquan, i understand your concern that interleaved might not required here as FSP default and core […]
Sure. Can you please file a bug to capture this?
BTW, as per PDG, Chapter 6, Table 9 Notes, it looks like interleaving is not supported.
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/meminit_jsl.h:
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/in... PS18, Line 73: 6 Why does this not have a macro of its own?
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/in... PS18, Line 106: Enabled That is not correct. It could be enabled/disabled based on board setting.
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/in... PS18, Line 110: UserBd Should there be an enum for what FSP expects?
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/in... PS18, Line 118: const struct mb_cfg *board_cfg, nit: Would this fit on the line above?
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 132: )
Hi Karthik, I understood what you are referring. let me try to explain my understanding. […]
I think what Karthik is referring to here is:
-- JSL has 2 logical channels say Ch0 and Ch1 -- This corresponds to: Logical Ch0: MemorySpdPtr00 --> spd_data_ptr, MemorySpdPtr01 --> NULL Logical Ch1: MemorySpdPtr10 --> spd_data_ptr, MemorySpdPtr11 --> NULL -- When channels are half-populated, then logical Ch1 would be left empty and logical Ch0 is still populated. Thus, the FSP configuration would look like: Logical Ch0: MemorySpdPtr00 --> spd_data_ptr, MemorySpdPtr01 --> NULL Logical Ch1: MemorySpdPtr10 --> NULL, MemorySpdPtr11 --> NULL
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/me... File src/soc/intel/tigerlake/meminit_jsl.c:
https://review.coreboot.org/c/coreboot/+/39135/18/src/soc/intel/tigerlake/me... PS18, Line 68: sizeof(board_cfg->dq_map[DDR_CH0]) I am confused by the comment above and the memset. Isn't this memcpy actually copying all the 12 bytes?