Rob Barnes 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:
(10 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)
In my opinion, it would be simpler to have a table that matches the range of Mhz to standard MT/s va […]
I refactored the code to use a table of tCK. This allows accurate calculation of MHz and MT/s ranges for each speed bin. I had to include a second table for normalized MT/s values since there isn't a single way to derive this value from tCK.
What do you think about moving this code to device/dram/ddr4.[h|c]?
https://review.coreboot.org/c/coreboot/+/45177/3/src/soc/amd/picasso/dmi.c@2... PS3, Line 23: speed_mhz
I don't think all the speeds and MT/s values are really supported by DDR4 spec. […]
Done.
https://review.coreboot.org/c/coreboot/+/45177/3/src/soc/amd/picasso/dmi.c@2... PS3, Line 23: switch(speed_mhz) {
Would be nice to fix
Done
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. […]
There isn't a consistent convention. Actual max speed sometimes differs by 1 from the "normalized" speed. I decided to go with the normalized speed since that will be more familiar and expected by consumers of this data.
https://review.coreboot.org/c/coreboot/+/45177/3/src/soc/amd/picasso/dmi.c@3... PS3, Line 39: 1866
Why not 1867?
See other comment.
https://review.coreboot.org/c/coreboot/+/45177/3/src/soc/amd/picasso/dmi.c@5... PS3, Line 53: 3066
3067?
Removed.
https://review.coreboot.org/c/coreboot/+/45177/3/src/soc/amd/picasso/dmi.c@5... PS3, Line 59: 3866
3867?
Removed.
https://review.coreboot.org/c/coreboot/+/45177/3/src/soc/amd/picasso/dmi.c@7... PS3, Line 72: return 0;
Could you return 2*speed_mhz so that it "maybe" works? In any case, keep the printk so that if someo […]
I considered that. I decided to use 0 so it will be more obvious that something is wrong. Either way this code has been refactored.
https://review.coreboot.org/c/coreboot/+/45177/3/src/soc/amd/picasso/dmi.c@7... PS3, Line 74:
no need for this blank line
Done
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
Ah yes, 96 is the limit, not 80.