Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45177 )
Change subject: soc/amd/picasso: Convert DDR4 MHz to MT/s correctly ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45177/4/src/soc/amd/picasso/dmi.c File src/soc/amd/picasso/dmi.c:
https://review.coreboot.org/c/coreboot/+/45177/4/src/soc/amd/picasso/dmi.c@1... PS4, Line 18: den macro parameters need parens around them.
https://review.coreboot.org/c/coreboot/+/45177/4/src/soc/amd/picasso/dmi.c@4... PS4, Line 41: static uint16_t _ddr4_tck_min_ps[] = { These arrays should be marked const as they shouldn't be mutable.
https://review.coreboot.org/c/coreboot/+/45177/4/src/soc/amd/picasso/dmi.c@5... PS4, Line 58: static uint16_t _ddr4_speed_normalized_mts[] = { I'm confused why we don't just have an object that has 3 fields:
min_clock_mhz, max_clock_mhz, reported_mts
Then we just need to see where the reported clock fits between min and max and use the reported_mts.
Also, this code should be put in a generic place. Unless I'm really missing something the current implementation seems overly complicated. What am I missing?