Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45177 )
Change subject: soc/amd/picasso: Use lookup table to convert MHz to MT/s ......................................................................
Patch Set 3:
(8 comments)
https://review.coreboot.org/c/coreboot/+/45177/3/src/soc/amd/picasso/dmi.c File src/soc/amd/picasso/dmi.c:
https://review.coreboot.org/c/coreboot/+/45177/3/src/soc/amd/picasso/dmi.c@2... PS3, Line 21: static uint16_t convert_speed_mhz_to_mts(uint16_t speed_mhz) If possible, I'd like to avoid the table. How about:
uint16_t tmp = speed_mhz; tmp *= 3; // Multiply so that we can round to tens later tmp = DIV_ROUND_CLOSEST(tmp, 10); // Round to normalize speeds tmp *= 10; // Restore the original value tmp *= 2; // Convert to MT/s return DIV_ROUND_CLOSEST(tmp, 3); // Divide with round to get the actual MT/s value
I've written it like this to explain what each step does, but it can be simplified:
const uint32_t mhz_x3 = DIV_ROUND_CLOSEST(speed_mhz * 3, 10) * 10;
return DIV_ROUND_CLOSEST(2 * mhz_x3, 3);
In any case, it would be good to have a comment explaining *why* we don't just double the value: to account for integer math rounding errors.
https://review.coreboot.org/c/coreboot/+/45177/3/src/soc/amd/picasso/dmi.c@2... PS3, Line 23: switch(speed_mhz) {
space required before the open parenthesis '('
Would be nice to fix
https://review.coreboot.org/c/coreboot/+/45177/3/src/soc/amd/picasso/dmi.c@3... PS3, Line 33: 1066 I see some inconsistencies as to which values should be rounded up. Shouldn't this be 1067?
https://review.coreboot.org/c/coreboot/+/45177/3/src/soc/amd/picasso/dmi.c@3... PS3, Line 39: 1866 Why not 1867?
https://review.coreboot.org/c/coreboot/+/45177/3/src/soc/amd/picasso/dmi.c@5... PS3, Line 53: 3066 3067?
https://review.coreboot.org/c/coreboot/+/45177/3/src/soc/amd/picasso/dmi.c@5... PS3, Line 59: 3866 3867?
https://review.coreboot.org/c/coreboot/+/45177/3/src/soc/amd/picasso/dmi.c@7... PS3, Line 74: no need for this blank line
https://review.coreboot.org/c/coreboot/+/45177/3/src/soc/amd/picasso/dmi.c@9... PS3, Line 91: convert_speed_mhz_to_mts(dmi17->ConfigSpeed); Nit: fits in a single line