[coreboot-gerrit] [S] Change in coreboot[main]: soc/intel/{adl,mtl}: Don't set up SPD on LPDDRx

[Subrata Banik (Code Review)]

Attention is currently required from: Arthur Heymans, Dinesh Gehlot, Eran Mitrani, Jakub Czapiga, Jayvik Desai, Kapil Porwal, Nick Vaccaro, Rishika Raj, Tarun.

Subrata Banik has posted comments on this change by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/84205?usp=email )

Change subject: soc/intel/{adl,mtl}: Don't set up SPD on LPDDRx
......................................................................

Patch Set 3:

(1 comment)

File src/soc/intel/meteorlake/meminit.c:

https://review.coreboot.org/c/coreboot/+/84205/comment/4657f917_1adde10c?usp... :
PS2, Line 209: has_spd = true;
> > > > Can you help me understand the purpose of `has_spd`? Does it mean the mainboard has an SPD chip and we'll read the hex file using SMBUS?
> > >
> > > would need_spd be a better name? LPDDRx DRAM technology does not use SPD and the code for SPD in soc/intel/common is doing doing undefined behavior like overflowing an array.
> >
> > Yes, that name sounds better. I'd like to suggest adding a comment that explains the purpose of this variable. People often confuse the SPD chip with the SPD hex data file. The code should make it clear that the SPD chip isn't required to read the SPD hex file (need_spd).
>
> No actually it's about SPD data and this code is wrong. The soc/intel/common code is simply wrong for LPDDRx. The loop over channels and dimms per channel is overflowing. There are no DIMMs in LPDDRx which is what need to be addressed.

okay, so when you set `has_spd` or `need_spd`, are you saying that we will read SPD data over SMBUS and for lpddrx, we shall pass SPD hex file ?