Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45177 )
Change subject: soc/amd/picasso: Adjust for memory Mhz fraction ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45177/2/src/soc/amd/picasso/dmi.c File src/soc/amd/picasso/dmi.c:
https://review.coreboot.org/c/coreboot/+/45177/2/src/soc/amd/picasso/dmi.c@3... PS2, Line 34: * e.g. 1333 Mhz == 1333.333 Mhz == 2666.666 MT/s == 26667 MT/s
It depends on what AGESA is returning in that case. Will it be returning 1466MHz or 1467MHz?
You will have to check AGESA to determine that.
The logic below can be updated to check for 32 and round up one for the 1466Mhz case.
That can work. I still think it might be just easier to have a table that looks up the provided speed value in MHz and returns the corresponding value in MT/s.
https://review.coreboot.org/c/coreboot/+/45177/2/src/soc/amd/picasso/dmi.c@3... PS2, Line 36: dimm->configured_speed_mts = 2 * dmi17->ConfigSpeed; : if (dimm->configured_speed_mts % 100 == 66) : dimm->configured_speed_mts += 1; : else if (dimm->configured_speed_mts % 100 == 34) : dimm->configured_speed_mts -= 1; Irrespective of whether you decide to go with adding check for 32 or using a table, I think this should be converted to a function: dimm->configured_speed_mts = convert_speed_mhz_to_mts(dmi17->ConfigSpeed); dimm->max_speed_mts = convert_speed_mhz_to_mts(dmi17->Speed);