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:
(5 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.
Ack
https://review.coreboot.org/c/coreboot/+/45177/4/src/soc/amd/picasso/dmi.c@3... PS4, Line 33: #define DDR4_MIN DDR4_1600 : #define DDR4_MAX DDR4_3200 : #
Can you put these in the enum, e.g. […]
Thanks for the tip.
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.
Ack
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[] = {
It does feel overly complicated. […]
I used tck because this is the value that's actually specified the spec. Starting with this value ensures there's no loss in precision. Also this pattern could be adopted for other memory types.
Based on the feedback, I will switch to the min,max,mts table and move to ddr4.h|c.
https://review.coreboot.org/c/coreboot/+/45177/4/src/soc/amd/picasso/dmi.c@1... PS4, Line 146: __attribute__ ((unused))
I would move all this into ddr4.c.
Yep, this was discussed in another comment.