Attention is currently required from: Dinesh Gehlot, Eran Mitrani, Jakub Czapiga, Jayvik Desai, Kapil Porwal, Nick Vaccaro, Rishika Raj, Subrata Banik, Tarun.
Arthur Heymans 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/7a4d84d1_cf1abaea?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.
--
To view, visit
https://review.coreboot.org/c/coreboot/+/84205?usp=email
To unsubscribe, or for help writing mail filters, visit
https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ib3d4ed8032bb06b6d08fbc2dc4b697df88745243
Gerrit-Change-Number: 84205
Gerrit-PatchSet: 3
Gerrit-Owner: Arthur Heymans
arthur@aheymans.xyz
Gerrit-Reviewer: Dinesh Gehlot
digehlot@google.com
Gerrit-Reviewer: Eran Mitrani
mitrani@google.com
Gerrit-Reviewer: Jakub Czapiga
czapiga@google.com
Gerrit-Reviewer: Jayvik Desai
jayvik@google.com
Gerrit-Reviewer: Kapil Porwal
kapilporwal@google.com
Gerrit-Reviewer: Nick Vaccaro
nvaccaro@chromium.org
Gerrit-Reviewer: Rishika Raj
rishikaraj@google.com
Gerrit-Reviewer: Subrata Banik
subratabanik@google.com
Gerrit-Reviewer: Tarun
tstuli@gmail.com
Gerrit-Reviewer: build bot (Jenkins)
no-reply@coreboot.org
Gerrit-Attention: Jayvik Desai
jayvik@google.com
Gerrit-Attention: Rishika Raj
rishikaraj@google.com
Gerrit-Attention: Eran Mitrani
mitrani@google.com
Gerrit-Attention: Subrata Banik
subratabanik@google.com
Gerrit-Attention: Jakub Czapiga
czapiga@google.com
Gerrit-Attention: Kapil Porwal
kapilporwal@google.com
Gerrit-Attention: Dinesh Gehlot
digehlot@google.com
Gerrit-Attention: Nick Vaccaro
nvaccaro@chromium.org
Gerrit-Attention: Tarun
tstuli@gmail.com
Gerrit-Comment-Date: Wed, 04 Sep 2024 06:44:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik
subratabanik@google.com
Comment-In-Reply-To: Arthur Heymans
arthur@aheymans.xyz