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 1:
(7 comments)
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 */ Did we confirm FSP/MRC expectations for LPx and DDR?
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/inc... PS1, Line 16: True for both DDR4 and DDR5 I think it is not the same for DDR4 and DDR5. I think DDR5 channels are 4 instead of 2?
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/inc... PS1, Line 22: DDR4 DDRx since I believe it is true for DDR4 and DDR5?
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 Have we confirmed this for ADL?
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/inc... PS1, Line 65: uint8_t dq_map[DDR_CHANNELS][DDR_BYTES_PER_CHANNEL][BITS_PER_BYTE]; : uint8_t dqs_map[DDR_CHANNELS][DDR_BYTES_PER_CHANNEL]; Angel had posted this earlier on one of Subrata's CLs and I think it makes sense. Instead of having a single [][][] table for dq_map, it might be much easier for mainboards to fill something like:
» struct { » » uint8_t dq0[BITS_PER_BYTE]; » » uint8_t dq1[BITS_PER_BYTE]; » » uint8_t dq2[BITS_PER_BYTE]; » » uint8_t dq3[BITS_PER_BYTE]; » » uint8_t dq4[BITS_PER_BYTE]; » » uint8_t dq5[BITS_PER_BYTE]; » » uint8_t dq6[BITS_PER_BYTE]; » » uint8_t dq7[BITS_PER_BYTE]; » } dq_map[DDR_CHANNELS];
» struct { » » uint8_t dqs0; » » uint8_t dqs1; » » uint8_t dqs2; » » uint8_t dqs3; » » uint8_t dqs4; » » uint8_t dqs5; » » uint8_t dqs6; » » uint8_t dqs7; » } dqs_map[DDR_CHANNELS];
Above is for DDR4. For LP4x/LP5x, it would be something like:
» struct { » » uint8_t dq0[BITS_PER_BYTE]; » » uint8_t dq1[BITS_PER_BYTE]; » } dq_map[LPX_CHANNELS];
» struct { » » uint8_t dqs0; » » uint8_t dqs1; » } dqs_map[LPX_CHANNELS];
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. Have we confirmed this for ADL?
https://review.coreboot.org/c/coreboot/+/48860/1/src/soc/intel/alderlake/mem... PS1, Line 377: /* intentional fallthrough */ Isn't this supposed to be after the above case?