Rob Barnes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45343 )
Change subject: device/dram: Add method for converting MHz to MT/s ......................................................................
device/dram: Add method for converting MHz to MT/s
Add method for converting DDR4 speed in MHz to MT/s. Checks that MHz is within a speed grade range.
BUG=b:167155849 TEST=ddr4-test unit test
Change-Id: I1433f028afb794fe3e397b03f5bd0565494c8130 Signed-off-by: Rob Barnes robbarnes@google.com --- M src/device/dram/ddr4.c M src/include/device/dram/ddr4.h M tests/device/Makefile.inc A tests/device/ddr4-test.c 4 files changed, 143 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/45343/1
diff --git a/src/device/dram/ddr4.c b/src/device/dram/ddr4.c index 429a9d2..0d4c1e3 100644 --- a/src/device/dram/ddr4.c +++ b/src/device/dram/ddr4.c @@ -9,6 +9,53 @@ #include <smbios.h> #include <types.h>
+/** + * DDR4 speed attributes derived from JEDEC 79-4C tables 169 & 170 + * + * min_clock_mhz = 1000/max_tCk_avg(ns) + * max_clock_mhz = 1000/min_tCk_avg(ns) + * reported_mts = Standard reported DDR4 speed in MT/s + * May be 1 less than the actual max MT/s + */ +static const ddr4_speed_attr ddr4_speeds[] = { + [DDR4_UNSPECIFIED] = {}, + [DDR4_1600] = { + .min_clock_mhz = 667, + .max_clock_mhz = 800, + .reported_mts = 1600 + }, + [DDR4_1866]{ + .min_clock_mhz = 800, + .max_clock_mhz = 934, + .reported_mts = 1866 + }, + [DDR4_2133]{ + .min_clock_mhz = 934, + .max_clock_mhz = 1067, + .reported_mts = 2133 + }, + [DDR4_2400]{ + .min_clock_mhz = 1067, + .max_clock_mhz = 1200, + .reported_mts = 2400 + }, + [DDR4_2666]{ + .min_clock_mhz = 1200, + .max_clock_mhz = 1333, + .reported_mts = 2666 + }, + [DDR4_2933]{ + .min_clock_mhz = 1333, + .max_clock_mhz = 1466, + .reported_mts = 2933 + }, + [DDR4_3200]{ + .min_clock_mhz = 1466, + .max_clock_mhz = 1600, + .reported_mts = 3200 + } +}; + typedef enum { BLOCK_0, /* Base Configuration and DRAM Parameters */ BLOCK_1, @@ -68,6 +115,33 @@ } }
+/** + * Converts DDR4 clock speed in MHz to the standard reported speed in MT/s + */ +uint16_t ddr4_speed_mhz_to_reported_mts(uint16_t speed_mhz) +{ + for (enum ddr4_speed_grade speed = DDR4_MIN; speed <= DDR4_MAX; speed++) { + const ddr4_speed_attr speed_attr = ddr4_speed_attributes(speed); + if (speed_mhz > speed_attr.min_clock_mhz && + speed_mhz <= speed_attr.max_clock_mhz) { + return speed_attr.reported_mts; + } + } + printk(BIOS_ERR, "DDR4 speed of %d MHz is out of range", speed_mhz); + return 0; +} + +/** + * Gets DDR4 speed attributes for a given DDR4 speed grade + */ +const ddr4_speed_attr ddr4_speed_attributes(enum ddr4_speed_grade speed) +{ + if (speed < DDR4_MIN || speed > DDR4_MAX) { + printk(BIOS_ERR, "Invalid DDR4 speed grade"); + return ddr4_speeds[DDR4_UNSPECIFIED]; + } + return ddr4_speeds[speed]; +}
/** * \brief Decode the raw SPD data diff --git a/src/include/device/dram/ddr4.h b/src/include/device/dram/ddr4.h index f258fa9..75dbaf1 100644 --- a/src/include/device/dram/ddr4.h +++ b/src/include/device/dram/ddr4.h @@ -70,4 +70,27 @@ const u16 selected_freq, const dimm_attr *info);
+enum ddr4_speed_grade { + DDR4_UNSPECIFIED, + DDR4_1600, + DDR4_1866, + DDR4_2133, + DDR4_2400, + DDR4_2666, + DDR4_2933, + DDR4_3200, + DDR4_MIN = DDR4_1600, + DDR4_MAX = DDR4_3200 +}; + +typedef struct ddr4_speed_attr_st { + uint32_t min_clock_mhz; // exclusive + uint32_t max_clock_mhz; // inclusive + uint32_t reported_mts; +} ddr4_speed_attr; + +const ddr4_speed_attr ddr4_speed_attributes(enum ddr4_speed_grade speed); + +uint16_t ddr4_speed_mhz_to_reported_mts(uint16_t speed_mhz); + #endif /* DEVICE_DRAM_DDR4L_H */ diff --git a/tests/device/Makefile.inc b/tests/device/Makefile.inc index 10223c5..4626c3c 100644 --- a/tests/device/Makefile.inc +++ b/tests/device/Makefile.inc @@ -1,7 +1,12 @@ # SPDX-License-Identifier: GPL-2.0-only
tests-y += i2c-test +tests-y += ddr4-test
i2c-test-srcs += tests/device/i2c-test.c i2c-test-srcs += src/device/i2c.c i2c-test-mocks += platform_i2c_transfer + +ddr4-test-srcs += tests/device/ddr4-test.c +ddr4-test-srcs += tests/stubs/console.c +ddr4-test-srcs += src/device/dram/ddr4.c \ No newline at end of file diff --git a/tests/device/ddr4-test.c b/tests/device/ddr4-test.c new file mode 100644 index 0000000..0a9831d --- /dev/null +++ b/tests/device/ddr4-test.c @@ -0,0 +1,41 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <device/dram/ddr4.h> +#include <tests/test.h> + +static void ddr4_speed_mhz_to_mts_test(void **state) +{ + assert_int_equal(0, ddr4_speed_mhz_to_reported_mts(0)); + assert_int_equal(0, ddr4_speed_mhz_to_reported_mts(667)); + + assert_int_equal(1600, ddr4_speed_mhz_to_reported_mts(668)); + assert_int_equal(1600, ddr4_speed_mhz_to_reported_mts(800)); + + assert_int_equal(1866, ddr4_speed_mhz_to_reported_mts(801)); + assert_int_equal(1866, ddr4_speed_mhz_to_reported_mts(933)); + assert_int_equal(1866, ddr4_speed_mhz_to_reported_mts(934)); + + assert_int_equal(2133, ddr4_speed_mhz_to_reported_mts(1066)); + assert_int_equal(2133, ddr4_speed_mhz_to_reported_mts(1067)); + + assert_int_equal(2400, ddr4_speed_mhz_to_reported_mts(1200)); + + assert_int_equal(2666, ddr4_speed_mhz_to_reported_mts(1333)); + + assert_int_equal(2933, ddr4_speed_mhz_to_reported_mts(1466)); + + assert_int_equal(3200, ddr4_speed_mhz_to_reported_mts(1467)); + assert_int_equal(3200, ddr4_speed_mhz_to_reported_mts(1600)); + + assert_int_equal(0, ddr4_speed_mhz_to_reported_mts(1601)); + assert_int_equal(0, ddr4_speed_mhz_to_reported_mts(INT16_MAX)); +} + +int main(void) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test(ddr4_speed_mhz_to_mts_test) + }; + + return cmocka_run_group_tests(tests, NULL, NULL); +}
Martin Roth 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 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45343/1/src/device/dram/ddr4.c File src/device/dram/ddr4.c:
https://review.coreboot.org/c/coreboot/+/45343/1/src/device/dram/ddr4.c@24 PS1, Line 24: 800, I understand what's being done here, but it just seems odd that 801MHz would be classified as DDR4_1866. Is that actually what we want?
Rob Barnes 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 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45343/1/src/device/dram/ddr4.c File src/device/dram/ddr4.c:
https://review.coreboot.org/c/coreboot/+/45343/1/src/device/dram/ddr4.c@24 PS1, Line 24: 800,
I understand what's being done here, but it just seems odd that 801MHz would be classified as DDR4_1 […]
According to the standard, DDR4-1866 tck can range from 1.071ns to <1.25ns, or 934 MHz to >800Mhz. So, yes 801 MHz is within DDR4-1866 range. Whether this should be reported as 1866 MT/s or 1602 MT/s is another question. For SMBIOS, we decided to report 1866 MT/s because this is more common and the difference is unlikely to be meaningful.
Angel Pons 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 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45343/1/src/device/dram/ddr4.c File src/device/dram/ddr4.c:
https://review.coreboot.org/c/coreboot/+/45343/1/src/device/dram/ddr4.c@24 PS1, Line 24: 800,
According to the standard, DDR4-1866 tck can range from 1.071ns to <1.25ns, or 934 MHz to >800Mhz. […]
That would be false advertising, though. This is why I prefer a method that would simply round to the closest standard speed. I don't like table-based approaches because they use quite a bit of space and would need to be updated each time new speeds exist.
https://review.coreboot.org/c/coreboot/+/45343/1/src/device/dram/ddr4.c@27 PS1, Line 27: [DDR4_1866]{ How does this compile without an equals sign?
Hello build bot (Jenkins), Raul Rangel, Martin Roth, Furquan Shaikh, Patrick Georgi, Paul Fagerburg, Angel Pons, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45343
to look at the new patch set (#2).
Change subject: device/dram: Add method for converting MHz to MT/s ......................................................................
device/dram: Add method for converting MHz to MT/s
Add method for converting DDR4 speed in MHz to MT/s. Checks that MHz is within a speed grade range.
BUG=b:167155849 TEST=ddr4-test unit test
Change-Id: I1433f028afb794fe3e397b03f5bd0565494c8130 Signed-off-by: Rob Barnes robbarnes@google.com --- M src/device/dram/ddr4.c M src/include/device/dram/ddr4.h M tests/device/Makefile.inc A tests/device/ddr4-test.c 4 files changed, 143 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/45343/2
Rob Barnes 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 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45343/1/src/device/dram/ddr4.c File src/device/dram/ddr4.c:
https://review.coreboot.org/c/coreboot/+/45343/1/src/device/dram/ddr4.c@27 PS1, Line 27: [DDR4_1866]{
How does this compile without an equals sign?
Looks like it's an obsolete, but valid syntax for gcc: https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html
Filled in '=' signs.
Angel Pons 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 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45343/1/src/device/dram/ddr4.c File src/device/dram/ddr4.c:
https://review.coreboot.org/c/coreboot/+/45343/1/src/device/dram/ddr4.c@27 PS1, Line 27: [DDR4_1866]{
Looks like it's an obsolete, but valid syntax for gcc: https://gcc.gnu. […]
Oh no, GNU extensions! 😭
(GNU extensions aren't very well defined, which can cause problems in the long run)
Aaron Durbin 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 2:
(3 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: > Why is min exclusive and max inclusive?
https://review.coreboot.org/c/coreboot/+/45343/2/src/include/device/dram/ddr... File src/include/device/dram/ddr4.h:
https://review.coreboot.org/c/coreboot/+/45343/2/src/include/device/dram/ddr... PS2, Line 90: } ddr4_speed_attr; No typedefs, please. Also, this object does not need to be exposed out of the compilation unit. Same goes for the enum above.
https://review.coreboot.org/c/coreboot/+/45343/2/src/include/device/dram/ddr... PS2, Line 93: Please add comments here what the functions do.
Hello build bot (Jenkins), Raul Rangel, Martin Roth, Furquan Shaikh, Patrick Georgi, Paul Fagerburg, Angel Pons, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45343
to look at the new patch set (#3).
Change subject: device/dram: Add method for converting MHz to MT/s ......................................................................
device/dram: Add method for converting MHz to MT/s
Add method for converting DDR4 speed in MHz to MT/s. Checks that MHz is within a speed grade range.
BUG=b:167155849 TEST=ddr4-test unit test
Change-Id: I1433f028afb794fe3e397b03f5bd0565494c8130 Signed-off-by: Rob Barnes robbarnes@google.com --- M src/device/dram/Makefile.inc M src/device/dram/ddr4.c M src/include/device/dram/ddr4.h M tests/device/Makefile.inc A tests/device/ddr4-test.c 5 files changed, 147 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/45343/3
Rob Barnes 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:
(3 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: >
Why is min exclusive and max inclusive?
Because this is how it's listed in the spec. Some precision will be lost if this isn't followed.
https://review.coreboot.org/c/coreboot/+/45343/2/src/include/device/dram/ddr... File src/include/device/dram/ddr4.h:
https://review.coreboot.org/c/coreboot/+/45343/2/src/include/device/dram/ddr... PS2, Line 90: } ddr4_speed_attr;
No typedefs, please. Also, this object does not need to be exposed out of the compilation unit. […]
Done
https://review.coreboot.org/c/coreboot/+/45343/2/src/include/device/dram/ddr... PS2, Line 93:
Please add comments here what the functions do.
Done
Paul Fagerburg 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:
(1 comment)
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@69 PS3, Line 69: 1466 Should this one be 1467?
Rob Barnes 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:
(1 comment)
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@69 PS3, Line 69: 1466
Should this one be 1467?
Table 170, DDR4-2933: min_tck >= 0.682 ns, max_tck < 0.750 ns --> max_mhz<1466.28 min_mhz>=1333.33.
Are you seeing it reported as 1467 somewhere?
Paul Fagerburg 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:
(1 comment)
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@69 PS3, Line 69: 1466
Table 170, DDR4-2933: min_tck >= 0.682 ns, max_tck < 0.750 ns --> max_mhz<1466.28 min_mhz>=1333.33. […]
No, just questioning since 1067 will get doubled to 2133, should we not have 2933 include 1467 MHz and not stop at 1466 MHz?
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];
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Martin Roth, Patrick Georgi, Paul Fagerburg, Angel Pons, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45343
to look at the new patch set (#4).
Change subject: device/dram: Add method for converting MHz to MT/s ......................................................................
device/dram: Add method for converting MHz to MT/s
Add method for converting DDR4 speed in MHz to MT/s. Checks that MHz is within a speed grade range.
BUG=b:167155849 TEST=ddr4-test unit test
Change-Id: I1433f028afb794fe3e397b03f5bd0565494c8130 Signed-off-by: Rob Barnes robbarnes@google.com --- M src/device/dram/Makefile.inc M src/device/dram/ddr4.c M src/include/device/dram/ddr4.h M tests/device/Makefile.inc A tests/device/ddr4-test.c 5 files changed, 136 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/45343/4
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Martin Roth, Patrick Georgi, Paul Fagerburg, Angel Pons, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45343
to look at the new patch set (#5).
Change subject: device/dram: Add method for converting MHz to MT/s ......................................................................
device/dram: Add method for converting MHz to MT/s
Add method for converting DDR4 speed in MHz to MT/s. Checks that MHz is within a speed grade range.
BUG=b:167155849 TEST=ddr4-test unit test BRANCH=Zork
Change-Id: I1433f028afb794fe3e397b03f5bd0565494c8130 Signed-off-by: Rob Barnes robbarnes@google.com --- M src/device/dram/Makefile.inc M src/device/dram/ddr4.c M src/include/device/dram/ddr4.h M tests/device/Makefile.inc A tests/device/ddr4-test.c 5 files changed, 136 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/45343/5
Rob Barnes 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 4:
(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: >
I think Aaron's question was to understand if there is a reason why we cannot keep both min and max […]
Making the min inclusive will work fine in the current context. It does lose some precision that could matter if these values are used in another context.
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: […]
Right, this function is not required. It's a just a safe wrapper for accessing the ddr4_speeds array. I was attempting to generalize a bit since this is being added to the common ddr4 library. I'll simplify for this CL.
Aaron Durbin 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 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45343/5/src/device/dram/ddr4.c File src/device/dram/ddr4.c:
https://review.coreboot.org/c/coreboot/+/45343/5/src/device/dram/ddr4.c@144 PS5, Line 144: DDR4_MIN; speed <= DDR4_MAX Can be done using ARRAY_SIZE().
https://review.coreboot.org/c/coreboot/+/45343/5/src/device/dram/ddr4.c@145 PS5, Line 145: speed_attr = ddr4_speeds[speed] *speed_attr = &ddr4_speeds[speed]
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Martin Roth, Patrick Georgi, Paul Fagerburg, Angel Pons, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45343
to look at the new patch set (#6).
Change subject: device/dram: Add method for converting MHz to MT/s ......................................................................
device/dram: Add method for converting MHz to MT/s
Add method for converting DDR4 speed in MHz to MT/s. Checks that MHz is within a speed grade range.
BUG=b:167155849 TEST=ddr4-test unit test BRANCH=Zork
Change-Id: I1433f028afb794fe3e397b03f5bd0565494c8130 Signed-off-by: Rob Barnes robbarnes@google.com --- M src/device/dram/Makefile.inc M src/device/dram/ddr4.c M src/include/device/dram/ddr4.h M tests/device/Makefile.inc A tests/device/ddr4-test.c 5 files changed, 135 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/45343/6
Rob Barnes 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 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45343/1/src/device/dram/ddr4.c File src/device/dram/ddr4.c:
https://review.coreboot.org/c/coreboot/+/45343/1/src/device/dram/ddr4.c@24 PS1, Line 24: 800,
That would be false advertising, though. […]
Rounding to the closest standard speed grade does not follow the spec. e.g. 935 MHZ is explicitly in the 2133 speed grade range, not 1866.
Another approach would be to report the standard speed grade in the SMBIOS type 17 "max speed" field and the unrounded speed in the "configured speed" field.
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@69 PS3, Line 69: 1466
No, just questioning since 1067 will get doubled to 2133, should we not have 2933 include 1467 MHz a […]
I prefer to follow the spec. If this fails it should be fixed at the source of the data.
https://review.coreboot.org/c/coreboot/+/45343/5/src/device/dram/ddr4.c File src/device/dram/ddr4.c:
https://review.coreboot.org/c/coreboot/+/45343/5/src/device/dram/ddr4.c@144 PS5, Line 144: DDR4_MIN; speed <= DDR4_MAX
Can be done using ARRAY_SIZE().
Done
https://review.coreboot.org/c/coreboot/+/45343/5/src/device/dram/ddr4.c@145 PS5, Line 145: speed_attr = ddr4_speeds[speed]
*speed_attr = &ddr4_speeds[speed]
Done
Paul Fagerburg 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 6: Code-Review+1
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 6: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/45343/6/src/device/dram/ddr4.c File src/device/dram/ddr4.c:
https://review.coreboot.org/c/coreboot/+/45343/6/src/device/dram/ddr4.c@14 PS6, Line 14: DDR4_UNSPECIFIED nit: I am not sure what purpose this serves since we don't really use it for anything now except reserving an empty entry in ddr4_speeds[] table which I think is not really necessary.
https://review.coreboot.org/c/coreboot/+/45343/6/src/device/dram/ddr4.c@150 PS6, Line 150: DDR4 nit: Add "ERROR:" so that log parsing scripts catch this.
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Martin Roth, Patrick Georgi, Paul Fagerburg, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45343
to look at the new patch set (#7).
Change subject: device/dram: Add method for converting MHz to MT/s ......................................................................
device/dram: Add method for converting MHz to MT/s
Add method for converting DDR4 speed in MHz to MT/s. Checks that MHz is within a speed grade range.
BUG=b:167155849 TEST=ddr4-test unit test BRANCH=Zork
Change-Id: I1433f028afb794fe3e397b03f5bd0565494c8130 Signed-off-by: Rob Barnes robbarnes@google.com --- M src/device/dram/Makefile.inc M src/device/dram/ddr4.c M src/include/device/dram/ddr4.h M tests/device/Makefile.inc A tests/device/ddr4-test.c 5 files changed, 131 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/45343/7
Paul Fagerburg 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 7: Code-Review+2
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 7: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/45343/6/src/device/dram/ddr4.c File src/device/dram/ddr4.c:
https://review.coreboot.org/c/coreboot/+/45343/6/src/device/dram/ddr4.c@14 PS6, Line 14: DDR4_UNSPECIFIED
nit: I am not sure what purpose this serves since we don't really use it for anything now except res […]
Done
https://review.coreboot.org/c/coreboot/+/45343/6/src/device/dram/ddr4.c@150 PS6, Line 150: DDR4
nit: Add "ERROR:" so that log parsing scripts catch this.
Done
Aaron Durbin 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 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45343/1/src/device/dram/ddr4.c File src/device/dram/ddr4.c:
https://review.coreboot.org/c/coreboot/+/45343/1/src/device/dram/ddr4.c@24 PS1, Line 24: 800,
Rounding to the closest standard speed grade does not follow the spec. e.g. […]
Ack
Aaron Durbin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45343 )
Change subject: device/dram: Add method for converting MHz to MT/s ......................................................................
device/dram: Add method for converting MHz to MT/s
Add method for converting DDR4 speed in MHz to MT/s. Checks that MHz is within a speed grade range.
BUG=b:167155849 TEST=ddr4-test unit test BRANCH=Zork
Change-Id: I1433f028afb794fe3e397b03f5bd0565494c8130 Signed-off-by: Rob Barnes robbarnes@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/45343 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Paul Fagerburg pfagerburg@chromium.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/device/dram/Makefile.inc M src/device/dram/ddr4.c M src/include/device/dram/ddr4.h M tests/device/Makefile.inc A tests/device/ddr4-test.c 5 files changed, 131 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Paul Fagerburg: Looks good to me, approved
diff --git a/src/device/dram/Makefile.inc b/src/device/dram/Makefile.inc index f397a53..f7118db 100644 --- a/src/device/dram/Makefile.inc +++ b/src/device/dram/Makefile.inc @@ -1 +1,3 @@ romstage-y += ddr4.c ddr3.c ddr2.c ddr_common.c + +ramstage-y += ddr4.c ddr3.c ddr2.c ddr_common.c diff --git a/src/device/dram/ddr4.c b/src/device/dram/ddr4.c index 429a9d2..e670b20 100644 --- a/src/device/dram/ddr4.c +++ b/src/device/dram/ddr4.c @@ -9,6 +9,69 @@ #include <smbios.h> #include <types.h>
+enum ddr4_speed_grade { + DDR4_1600, + DDR4_1866, + DDR4_2133, + DDR4_2400, + DDR4_2666, + DDR4_2933, + DDR4_3200 +}; + +struct ddr4_speed_attr { + uint32_t min_clock_mhz; // inclusive + uint32_t max_clock_mhz; // inclusive + uint32_t reported_mts; +}; + +/** + * DDR4 speed attributes derived from JEDEC 79-4C tables 169 & 170 + * + * min_clock_mhz = 1000/max_tCk_avg(ns) + 1 + * Adding 1 to make minimum inclusive + * max_clock_mhz = 1000/min_tCk_avg(ns) + * reported_mts = Standard reported DDR4 speed in MT/s + * May be 1 less than the actual max MT/s + */ +static const struct ddr4_speed_attr ddr4_speeds[] = { + [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 + }, + [DDR4_2133] = { + .min_clock_mhz = 935, + .max_clock_mhz = 1067, + .reported_mts = 2133 + }, + [DDR4_2400] = { + .min_clock_mhz = 1068, + .max_clock_mhz = 1200, + .reported_mts = 2400 + }, + [DDR4_2666] = { + .min_clock_mhz = 1201, + .max_clock_mhz = 1333, + .reported_mts = 2666 + }, + [DDR4_2933] = { + .min_clock_mhz = 1334, + .max_clock_mhz = 1466, + .reported_mts = 2933 + }, + [DDR4_3200] = { + .min_clock_mhz = 1467, + .max_clock_mhz = 1600, + .reported_mts = 3200 + } +}; + typedef enum { BLOCK_0, /* Base Configuration and DRAM Parameters */ BLOCK_1, @@ -68,6 +131,21 @@ } }
+/** + * Converts DDR4 clock speed in MHz to the standard reported speed in MT/s + */ +uint16_t ddr4_speed_mhz_to_reported_mts(uint16_t speed_mhz) +{ + for (enum ddr4_speed_grade speed = 0; speed < ARRAY_SIZE(ddr4_speeds); speed++) { + const struct ddr4_speed_attr *speed_attr = &ddr4_speeds[speed]; + if (speed_mhz >= speed_attr->min_clock_mhz && + speed_mhz <= speed_attr->max_clock_mhz) { + return speed_attr->reported_mts; + } + } + printk(BIOS_ERR, "ERROR: DDR4 speed of %d MHz is out of range", speed_mhz); + return 0; +}
/** * \brief Decode the raw SPD data diff --git a/src/include/device/dram/ddr4.h b/src/include/device/dram/ddr4.h index d22d4bc..eac8bf8 100644 --- a/src/include/device/dram/ddr4.h +++ b/src/include/device/dram/ddr4.h @@ -69,4 +69,9 @@ const u16 selected_freq, const dimm_attr *info);
+/** + * Converts DDR4 clock speed in MHz to the standard reported speed in MT/s + */ +uint16_t ddr4_speed_mhz_to_reported_mts(uint16_t speed_mhz); + #endif /* DEVICE_DRAM_DDR4L_H */ diff --git a/tests/device/Makefile.inc b/tests/device/Makefile.inc index 10223c5..4626c3c 100644 --- a/tests/device/Makefile.inc +++ b/tests/device/Makefile.inc @@ -1,7 +1,12 @@ # SPDX-License-Identifier: GPL-2.0-only
tests-y += i2c-test +tests-y += ddr4-test
i2c-test-srcs += tests/device/i2c-test.c i2c-test-srcs += src/device/i2c.c i2c-test-mocks += platform_i2c_transfer + +ddr4-test-srcs += tests/device/ddr4-test.c +ddr4-test-srcs += tests/stubs/console.c +ddr4-test-srcs += src/device/dram/ddr4.c \ No newline at end of file diff --git a/tests/device/ddr4-test.c b/tests/device/ddr4-test.c new file mode 100644 index 0000000..0a9831d --- /dev/null +++ b/tests/device/ddr4-test.c @@ -0,0 +1,41 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <device/dram/ddr4.h> +#include <tests/test.h> + +static void ddr4_speed_mhz_to_mts_test(void **state) +{ + assert_int_equal(0, ddr4_speed_mhz_to_reported_mts(0)); + assert_int_equal(0, ddr4_speed_mhz_to_reported_mts(667)); + + assert_int_equal(1600, ddr4_speed_mhz_to_reported_mts(668)); + assert_int_equal(1600, ddr4_speed_mhz_to_reported_mts(800)); + + assert_int_equal(1866, ddr4_speed_mhz_to_reported_mts(801)); + assert_int_equal(1866, ddr4_speed_mhz_to_reported_mts(933)); + assert_int_equal(1866, ddr4_speed_mhz_to_reported_mts(934)); + + assert_int_equal(2133, ddr4_speed_mhz_to_reported_mts(1066)); + assert_int_equal(2133, ddr4_speed_mhz_to_reported_mts(1067)); + + assert_int_equal(2400, ddr4_speed_mhz_to_reported_mts(1200)); + + assert_int_equal(2666, ddr4_speed_mhz_to_reported_mts(1333)); + + assert_int_equal(2933, ddr4_speed_mhz_to_reported_mts(1466)); + + assert_int_equal(3200, ddr4_speed_mhz_to_reported_mts(1467)); + assert_int_equal(3200, ddr4_speed_mhz_to_reported_mts(1600)); + + assert_int_equal(0, ddr4_speed_mhz_to_reported_mts(1601)); + assert_int_equal(0, ddr4_speed_mhz_to_reported_mts(INT16_MAX)); +} + +int main(void) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test(ddr4_speed_mhz_to_mts_test) + }; + + return cmocka_run_group_tests(tests, NULL, NULL); +}
9elements QA 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 8:
Automatic boot test returned (PASS/FAIL/TOTAL): 8/1/9 "QEMU x86 q35/ich9" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19506 "QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19505 "QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/19504 "QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19503 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/19502 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19510 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19509 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19508 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19507
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Aaron Durbin (Code Review)
-
Angel Pons (Code Review)
-
Furquan Shaikh (Code Review)
-
Martin Roth (Code Review)
-
Paul Fagerburg (Code Review)
-
Rob Barnes (Code Review)