Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40463 )
Change subject: mb/google/volteer: add generic SPDs ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40463/16/src/mainboard/google/volte... File src/mainboard/google/volteer/spd/SPD_LPDDR4X_200b_2R_32Gb_QDP_4266.spd.hex:
PS16:
Yes, Micron/Hynix (SPD_LPDDR4X_200b_2R_32Gb_QDP_4267) and Samsung (SPD_LPDDR4X_200b_2R_32Gb_QDP_4266) support different CAS Latencies and have different Min/Max SDRAM Cycle Times.
Have you confirmed with Intel that this is really correct? Did they observe any issues using the same SPD for the three vendors? Is there a bug where this was captured?
If you believe a longer filename that includes the other differentiable categories would lead to less confusion overall, I will rename all of the generic SPDs to include the three extra categories.
I am not concerned about longer or shorter names. I want us to understand the attributes that matter for differentiating memory parts. That is going to form the basis for how IDs are assigned and hence SPDs need to be generated accordingly. It doesn't look to me that there is a clear understanding of the actual attributes that matter.
So far, the SPD_LPDDR4X_200b_2R_32Gb_QDP_4266.spd.hex file is the only one that runs into this filename clash issue, but we may see more down the road.
I would like to see an explanation from Intel why there is a difference and what attribute plays a role in that. We need to take that attribute into account for distinguishing the generic SPDs. Doing it this way is going to create a subset of generic but not really generic SPDs.