Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48860 )
Change subject: soc/intel/alderlake: Refactor meminit code ......................................................................
Patch Set 2:
(8 comments)
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/inc... File src/soc/intel/alderlake/include/soc/meminit.h:
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/inc... PS2, Line 94: /* : * Command pins mapping for Controller Channel (ccc) : * lp5_ccc_config: Bitmask where bits [3:0] are Controller 0 Channel [3:0] and : * bits [7:4] are Controller 1 Channel [3:0] : * Bit value: 0 = ccc pin mapping is ascending, 1 = ccc pin mapping is descending. : */ Probably retain comment?
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/inc... PS2, Line 63: struct { nit: It would be helpful to have a comment indicating what the index and value means.
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/inc... PS2, Line 87: /* : * Rcomp resistor values. These values represent the resistance in : * ohms of the three rcomp resistors attached to the DDR_COMP_0, : * DDR_COMP_1, and DDR_COMP_2 pins on the DRAM. : */ : uint16_t rcomp_resistor[3]; : : /* Rcomp target values. */ : uint16_t rcomp_targets[5]; Not for this change, but we should confirm with Intel if these still need to be filled in. I think I had seen some bug/CL from Intel where some of these could be dropped.
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/inc... File src/soc/intel/alderlake/include/soc/meminit.h:
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/inc... PS1, Line 12: /* True for both LP4X and LP5X */
This is our current WIP understanding of the expectations (from reading EDS and info from Intel) Sti […]
As per EDS Vol 1 (619501) section 5.1.5, this looks correct.
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/inc... PS1, Line 16: True for both DDR4 and DDR5
I see 1 or 2 channel configurations for DDR5, from EDS #619501, section 5.1. […]
See section 5.1.5. DDR4 is 2 channels total and DDR5 is 4 channels.
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/inc... PS1, Line 23: DDR4 supports CH0 as SODIMM and CH1 as Memory Down
This is from the EDS #619501 "In mix topology implementation Ch0 will be SoDIMM and Ch1 MD"
SG.
BTW, I noticed that the section 5.1.2 was removed from revision 0.7 of the doc, whereas it is present in revision 0.65 of the doc. Can you please raise a bug against Intel to know why this was dropped and what is the latest supported configuration by ADL.
I see that in revision 0.65 there are some differences in the half-populated channels for LPx compared to TGL i.e. TGL had higher channels empty whereas ADL seems to have lower channels empty. This means that we will have to update `lpx_channel_unpopulated()` accordingly. Same for DDR.
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/mem... File src/soc/intel/alderlake/meminit.c:
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/mem... PS1, Line 11: Translate DDR4/5 channel to FSP UPD index for the channel.
Not yet no, this is the behavior we expect given all the information we have received so far (should […]
Given that DDR5 is not the same as DDR4 as per EDS for the channel configuration, this might differ.
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/mem... File src/soc/intel/alderlake/meminit.c:
https://review.coreboot.org/c/coreboot/+/48860/2/src/soc/intel/alderlake/mem... PS2, Line 248: *spd_dimm0 = 0; DIMM0 is always populated. Here dimm0 should be spd_md_data and dimm1 should be empty.