Tim Wawrzynczak 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?
This is our current WIP understanding of the expectations (from reading EDS and info from Intel) Still waiting on MRC confirmation 😊
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 see 1 or 2 channel configurations for DDR5, from EDS #619501, section 5.1.2, do you see something different elsewhere?
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?
Done
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?
This is from the EDS #619501 "In mix topology implementation Ch0 will be SoDIMM and Ch1 MD"
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. […]
Good memory I had forgotten, that was a good suggestion, I'll see what it looks like here.
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?
Not yet no, this is the behavior we expect given all the information we have received so far (should be similar or same as TGL expectations); i.e. is WIP until we get confirmation from Intel
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?
Ah yes, my eyes were bleeding after writing 500 lines 😋