[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/9379bb02_7a29c0cd?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).