Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45343 )
Change subject: device/dram: Add method for converting MHz to MT/s ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45343/2/src/device/dram/ddr4.c File src/device/dram/ddr4.c:
https://review.coreboot.org/c/coreboot/+/45343/2/src/device/dram/ddr4.c@125 PS2, Line 125: >
Because this is how it's listed in the spec. Some precision will be lost if this isn't followed.
I think Aaron's question was to understand if there is a reason why we cannot keep both min and max either inclusive or exclusive in the code here.
Example: [DDR4_1600] = { .min_clock_mhz = 668, .max_clock_mhz = 800, .reported_mts = 1600 }, [DDR4_1866] = { .min_clock_mhz = 801, .max_clock_mhz = 934, .reported_mts = 1866 },
It would still comply with the spec, but the checks in code would be consistent.
https://review.coreboot.org/c/coreboot/+/45343/3/src/device/dram/ddr4.c File src/device/dram/ddr4.c:
https://review.coreboot.org/c/coreboot/+/45343/3/src/device/dram/ddr4.c@141 PS3, Line 141: ddr4_speed_attributes Why is this function required? It is doing two things: 1. Ensuring speed parameter passed in is within the range DDR4_MIN to DDR4_MAX 2. Returning the entry ddr4_speeds[speed]
This function is used only within ddr4_speed_mhz_to_reported_mts which has a loop over the range DDR4_MIN to DDR4_MAX. So, the check here isn't really necessary. I think we should drop this function and simply do this on line 155 below:
const struct ddr4_speed_attr *speed_attr = &ddr4_speeds[speed];