Change in coreboot[master]: soc/amd/picasso: Adjust for memory Mhz fraction

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Rob Barnes (Code Review)

8 Sep 2020 8 Sep '20

11:29 a.m.

Rob Barnes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45177 )

Change subject: soc/amd/picasso: Adjust for memory Mhz fraction ......................................................................

soc/amd/picasso: Adjust for memory Mhz fraction

Memory speed is given as an integer, but it some cases it has an implicit fractional speed. Specifically, speeds ending in 33 are implicitly 33.333 and speeds ending in 67 are implicitly 66.666. So when multiplying by 2 to get MT/s, the result needs to be rounded up or down by one to account for this implicit fraction.

BUG=b:167155849 TEST=None

Signed-off-by: Rob Barnes robbarnes@google.com Change-Id: Icc77c21932c68ee9f0ff0b8e35ae7b1a3732b322 --- M src/soc/amd/picasso/dmi.c 1 file changed, 10 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/45177/1

diff --git a/src/soc/amd/picasso/dmi.c b/src/soc/amd/picasso/dmi.c index 5189cd6..33c9e22 100644 --- a/src/soc/amd/picasso/dmi.c +++ b/src/soc/amd/picasso/dmi.c @@ -30,10 +30,20 @@ /** * TYPE17_DMI_INFO holds speed in MHz. * Change to MT/s by multiplying by 2. + * Round up/down by 1 to adjust for implict clock fraction. + * e.g. 1333 Mhz == 1333.333 Mhz == 2666.666 MT/s == 26667 MT/s */ dimm->configured_speed_mts = 2 * dmi17->ConfigSpeed; + if (dimm->configured_speed_mts % 100 == 66) + dimm->configured_speed_mts += 1; + else if (dimm->configured_speed_mts % 100 == 34) + dimm->configured_speed_mts -= 1;

dimm->max_speed_mts = 2 * dmi17->Speed; + if (dimm->max_speed_mts % 100 == 66) + dimm->max_speed_mts += 1; + else if (dimm->max_speed_mts % 100 == 34) + dimm->max_speed_mts -= 1;

dimm->rank_per_dimm = dmi17->Attributes;

Attachments:

attachment.htm (text/html — 3.6 KB)

Show replies by date

Rob Barnes (Code Review)

8 Sep 8 Sep

11:37 a.m.

Hello Raul Rangel, Jack Rosenthal,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/45177

to look at the new patch set (#2).

Change subject: soc/amd/picasso: Adjust for memory Mhz fraction ......................................................................

soc/amd/picasso: Adjust for memory Mhz fraction

Memory speed is given as an integer, but in some cases it has an implicit fractional speed. Specifically, speeds ending in 33 are implicitly 33.333 and speeds ending in 67 are implicitly 66.666. So when multiplying by 2 to get MT/s, the result needs to be rounded up or down by one to account for this implicit fraction.

BUG=b:167155849 TEST=None

Signed-off-by: Rob Barnes robbarnes@google.com Change-Id: Icc77c21932c68ee9f0ff0b8e35ae7b1a3732b322 --- M src/soc/amd/picasso/dmi.c 1 file changed, 10 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/45177/2

Jack Rosenthal (Code Review)

12:10 p.m.

Jack Rosenthal has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45177 )

Change subject: soc/amd/picasso: Adjust for memory Mhz fraction ......................................................................

Patch Set 2: Code-Review+1

I guess it's OK to do this firmware side, but I would rather see mosys tolerate this rounding condition in the future.

Furquan Shaikh (Code Review)

12:11 p.m.

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45177 )

Change subject: soc/amd/picasso: Adjust for memory Mhz fraction ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45177/2/src/soc/amd/picasso/dmi.c File src/soc/amd/picasso/dmi.c:

https://review.coreboot.org/c/coreboot/+/45177/2/src/soc/amd/picasso/dmi.c@3... PS2, Line 34: * e.g. 1333 Mhz == 1333.333 Mhz == 2666.666 MT/s == 26667 MT/s What if it is 1466MHz which comes out to be 2933MT/s. I think it might be better to just have a table that does the conversion instead of doing the math here? Or just handle it in mosys as Jack commented.

Rob Barnes (Code Review)

12:51 p.m.

Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45177 )

Change subject: soc/amd/picasso: Adjust for memory Mhz fraction ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45177/2/src/soc/amd/picasso/dmi.c File src/soc/amd/picasso/dmi.c:

https://review.coreboot.org/c/coreboot/+/45177/2/src/soc/amd/picasso/dmi.c@3... PS2, Line 34: * e.g. 1333 Mhz == 1333.333 Mhz == 2666.666 MT/s == 26667 MT/s

...

What if it is 1466MHz which comes out to be 2933MT/s. […]

It depends on what AGESA is returning in that case. Will it be returning 1466MHz or 1467MHz? The logic below can be updated to check for 32 and round up one for the 1466Mhz case.

If reporting the speed as 2666 MT/s is acceptable, then updating mosys is the way to go.

Furquan Shaikh (Code Review)

2:36 p.m.

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45177 )

Change subject: soc/amd/picasso: Adjust for memory Mhz fraction ......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/45177/2/src/soc/amd/picasso/dmi.c File src/soc/amd/picasso/dmi.c:

https://review.coreboot.org/c/coreboot/+/45177/2/src/soc/amd/picasso/dmi.c@3... PS2, Line 34: * e.g. 1333 Mhz == 1333.333 Mhz == 2666.666 MT/s == 26667 MT/s

...

It depends on what AGESA is returning in that case. Will it be returning 1466MHz or 1467MHz?

You will have to check AGESA to determine that.

...

The logic below can be updated to check for 32 and round up one for the 1466Mhz case.

That can work. I still think it might be just easier to have a table that looks up the provided speed value in MHz and returns the corresponding value in MT/s.

https://review.coreboot.org/c/coreboot/+/45177/2/src/soc/amd/picasso/dmi.c@3... PS2, Line 36: dimm->configured_speed_mts = 2 * dmi17->ConfigSpeed; : if (dimm->configured_speed_mts % 100 == 66) : dimm->configured_speed_mts += 1; : else if (dimm->configured_speed_mts % 100 == 34) : dimm->configured_speed_mts -= 1; Irrespective of whether you decide to go with adding check for 32 or using a table, I think this should be converted to a function: dimm->configured_speed_mts = convert_speed_mhz_to_mts(dmi17->ConfigSpeed); dimm->max_speed_mts = convert_speed_mhz_to_mts(dmi17->Speed);

Paul Menzel (Code Review)

9 Sep 9 Sep

7:45 a.m.

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45177 )

Change subject: soc/amd/picasso: Adjust for memory Mhz fraction ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45177/2//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/45177/2//COMMIT_MSG@7 PS2, Line 7: Mhz MHz

Rob Barnes (Code Review)

5:27 p.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Use lookup table to convert MHz to MT/s

Hello build bot (Jenkins), Raul Rangel, Jack Rosenthal, Paul Fagerburg,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/45177

to look at the new patch set (#3).

Change subject: soc/amd/picasso: Use lookup table to convert MHz to MT/s ......................................................................

soc/amd/picasso: Use lookup table to convert MHz to MT/s

Memory speed is given as an integer, but in some cases it has an implicit fractional speed, so simply multiplying by 2 is not sufficient. Using a lookup table instead. Values were derived from AGESA.h and public specs.

BUG=b:167155849 TEST=Boot ezkinil, check output of 'mosys memory spd print all'

Signed-off-by: Rob Barnes robbarnes@google.com Change-Id: Icc77c21932c68ee9f0ff0b8e35ae7b1a3732b322 --- M src/soc/amd/picasso/dmi.c 1 file changed, 63 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/45177/3

Rob Barnes (Code Review)

5:28 p.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Use lookup table to convert MHz to MT/s

Rob Barnes 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:

(3 comments)

https://review.coreboot.org/c/coreboot/+/45177/2//COMMIT_MSG Commit Message:

https://review.coreboot.org/c/coreboot/+/45177/2//COMMIT_MSG@7 PS2, Line 7: Mhz

...

MHz

Done

https://review.coreboot.org/c/coreboot/+/45177/2/src/soc/amd/picasso/dmi.c File src/soc/amd/picasso/dmi.c:

https://review.coreboot.org/c/coreboot/+/45177/2/src/soc/amd/picasso/dmi.c@3... PS2, Line 34: * e.g. 1333 Mhz == 1333.333 Mhz == 2666.666 MT/s == 26667 MT/s

...

...
It depends on what AGESA is returning in that case. Will it be returning 1466MHz or 1467MHz? […]

Done

...

Irrespective of whether you decide to go with adding check for 32 or using a table, I think this sho […]

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/45177 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc77c21932c68ee9f0ff0b8e35ae7b1a3732b322 Gerrit-Change-Number: 45177 Gerrit-PatchSet: 3 Gerrit-Owner: Rob Barnes robbarnes@google.com Gerrit-Reviewer: Jack Rosenthal jrosenth@chromium.org Gerrit-Reviewer: Paul Fagerburg pfagerburg@chromium.org Gerrit-Reviewer: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 10 Sep 2020 00:28:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Comment-In-Reply-To: Paul Menzel paulepanter@users.sourceforge.net Comment-In-Reply-To: Rob Barnes robbarnes@google.com Gerrit-MessageType: comment

Paul Fagerburg (Code Review)

5:42 p.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Use lookup table to convert MHz to MT/s

Paul Fagerburg 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:

(1 comment)

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@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 someone's looking at the console, they can see that there was an issue.

build bot (Jenkins) (Code Review)

5:49 p.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Use lookup table to convert MHz to MT/s

build bot (Jenkins) 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:

(1 comment)

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 23: switch(speed_mhz) { space required before the open parenthesis '('

Angel Pons (Code Review)

10 Sep 10 Sep

1:49 a.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Use lookup table to convert MHz to MT/s

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/45177 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc77c21932c68ee9f0ff0b8e35ae7b1a3732b322 Gerrit-Change-Number: 45177 Gerrit-PatchSet: 3 Gerrit-Owner: Rob Barnes robbarnes@google.com Gerrit-Reviewer: Jack Rosenthal jrosenth@chromium.org Gerrit-Reviewer: Paul Fagerburg pfagerburg@chromium.org Gerrit-Reviewer: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Angel Pons th3fanbus@gmail.com Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 10 Sep 2020 08:49:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: comment

Angel Pons (Code Review)

1:50 a.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Use lookup table to convert MHz to MT/s

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:

(1 comment)

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: […]

I'd also place this function in common code, so that other platforms can use it as well.

-- To view, visit https://review.coreboot.org/c/coreboot/+/45177 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc77c21932c68ee9f0ff0b8e35ae7b1a3732b322 Gerrit-Change-Number: 45177 Gerrit-PatchSet: 3 Gerrit-Owner: Rob Barnes robbarnes@google.com Gerrit-Reviewer: Jack Rosenthal jrosenth@chromium.org Gerrit-Reviewer: Paul Fagerburg pfagerburg@chromium.org Gerrit-Reviewer: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Angel Pons th3fanbus@gmail.com Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 10 Sep 2020 08:50:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

Furquan Shaikh (Code Review)

8:59 a.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Use lookup table to convert MHz to MT/s

Furquan Shaikh 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:

(2 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)

...

I'd also place this function in common code, so that other platforms can use it as well.

In my opinion, it would be simpler to have a table that matches the range of Mhz to standard MT/s values as defined by the DDR spec. It removes any scope for error and inconsistency. Also, easier to ensure that it aligns with the userspace tool expectations since other pieces would be aligning to the DDR spec as well.

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. I think we should only put in the supported values and print out an error for rest.

-- To view, visit https://review.coreboot.org/c/coreboot/+/45177 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc77c21932c68ee9f0ff0b8e35ae7b1a3732b322 Gerrit-Change-Number: 45177 Gerrit-PatchSet: 3 Gerrit-Owner: Rob Barnes robbarnes@google.com Gerrit-Reviewer: Jack Rosenthal jrosenth@chromium.org Gerrit-Reviewer: Paul Fagerburg pfagerburg@chromium.org Gerrit-Reviewer: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Angel Pons th3fanbus@gmail.com Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 10 Sep 2020 15:59:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

Rob Barnes (Code Review)

4:48 p.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Convert DDR4 MHz to MT/s correctly

Hello build bot (Jenkins), Raul Rangel, Jack Rosenthal, Paul Fagerburg,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/45177

to look at the new patch set (#4).

Change subject: soc/amd/picasso: Convert DDR4 MHz to MT/s correctly ......................................................................

soc/amd/picasso: Convert DDR4 MHz to MT/s correctly

Memory speed is given as an integer in MHz. In some cases it has an implicit fractional speed, so simply multiplying by 2 is not sufficient.

Instead tCK(avg) values from the JEDEC DDR4 spec are used to calculate accurate MHz ranges for each speed bin. The normalized MT/s value for the speed bin is then used for SMBIOS type 17.

BUG=b:167155849 TEST=Boot ezkinil, check output of 'mosys memory spd print all' and dmidecode -t17

Signed-off-by: Rob Barnes robbarnes@google.com Change-Id: Icc77c21932c68ee9f0ff0b8e35ae7b1a3732b322 --- M src/soc/amd/picasso/dmi.c 1 file changed, 144 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/45177/4

build bot (Jenkins) (Code Review)

4:48 p.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Convert DDR4 MHz to MT/s correctly

build bot (Jenkins) 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:

(1 comment)

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 134: for(enum ddr4_speed_grade speed = DDR4_MIN; speed <= DDR4_MAX; speed++) { space required before the open parenthesis '('

Rob Barnes (Code Review)

4:55 p.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Convert DDR4 MHz to MT/s correctly

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.

-- To view, visit https://review.coreboot.org/c/coreboot/+/45177 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc77c21932c68ee9f0ff0b8e35ae7b1a3732b322 Gerrit-Change-Number: 45177 Gerrit-PatchSet: 4 Gerrit-Owner: Rob Barnes robbarnes@google.com Gerrit-Reviewer: Jack Rosenthal jrosenth@chromium.org Gerrit-Reviewer: Paul Fagerburg pfagerburg@chromium.org Gerrit-Reviewer: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Angel Pons th3fanbus@gmail.com Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 10 Sep 2020 23:55:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: build bot (Jenkins) no-reply@coreboot.org Comment-In-Reply-To: Furquan Shaikh furquan@google.com Comment-In-Reply-To: Paul Fagerburg pfagerburg@chromium.org Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

Paul Fagerburg (Code Review)

4:56 p.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Convert DDR4 MHz to MT/s correctly

Paul Fagerburg 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: Code-Review+1

(1 comment)

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@3... PS4, Line 33: #define DDR4_MIN DDR4_1600 : #define DDR4_MAX DDR4_3200 : # Can you put these in the enum, e.g. DDR4_UNSPECIFIED, DDR4_1600, DDR4_MIN = DDR4_1600, ... DDR4_3200, DDR4_MAX = DDR4_3200 That way it's harder for them to get out of sync.

Aaron Durbin (Code Review)

11 Sep 11 Sep

8:16 a.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Convert DDR4 MHz to MT/s correctly

Aaron Durbin 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:

(3 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.

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.

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[] = { I'm confused why we don't just have an object that has 3 fields:

min_clock_mhz, max_clock_mhz, reported_mts

Then we just need to see where the reported clock fits between min and max and use the reported_mts.

Also, this code should be put in a generic place. Unless I'm really missing something the current implementation seems overly complicated. What am I missing?

Raul Rangel (Code Review)

8:17 a.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Convert DDR4 MHz to MT/s correctly

Raul Rangel 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:

(1 comment)

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 146: __attribute__ ((unused)) I would move all this into ddr4.c.

Angel Pons (Code Review)

8:48 a.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Convert DDR4 MHz to MT/s correctly

Angel Pons 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:

(1 comment)

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@5... PS4, Line 58: static uint16_t _ddr4_speed_normalized_mts[] = {

...

I'm confused why we don't just have an object that has 3 fields: […]

I proposed a table-less algorithm here: https://review.coreboot.org/c/coreboot/+/45177/3/src/soc/amd/picasso/dmi.c#2...

-- To view, visit https://review.coreboot.org/c/coreboot/+/45177 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc77c21932c68ee9f0ff0b8e35ae7b1a3732b322 Gerrit-Change-Number: 45177 Gerrit-PatchSet: 4 Gerrit-Owner: Rob Barnes robbarnes@google.com Gerrit-Reviewer: Jack Rosenthal jrosenth@chromium.org Gerrit-Reviewer: Paul Fagerburg pfagerburg@chromium.org Gerrit-Reviewer: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Angel Pons th3fanbus@gmail.com Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Fri, 11 Sep 2020 15:48:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Gerrit-MessageType: comment

Furquan Shaikh (Code Review)

8:53 a.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Convert DDR4 MHz to MT/s correctly

Furquan Shaikh 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:

(1 comment)

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@5... PS4, Line 58: static uint16_t _ddr4_speed_normalized_mts[] = {

...

I'm confused why we don't just have an object that has 3 fields: […]

It does feel overly complicated.

+1 to both -- using an object with 3 fields to determine the reported_mts rather than doing all the calculations here and moving this to be a common library function since there is nothing picasso specific.

-- To view, visit https://review.coreboot.org/c/coreboot/+/45177 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc77c21932c68ee9f0ff0b8e35ae7b1a3732b322 Gerrit-Change-Number: 45177 Gerrit-PatchSet: 4 Gerrit-Owner: Rob Barnes robbarnes@google.com Gerrit-Reviewer: Jack Rosenthal jrosenth@chromium.org Gerrit-Reviewer: Paul Fagerburg pfagerburg@chromium.org Gerrit-Reviewer: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Angel Pons th3fanbus@gmail.com Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Fri, 11 Sep 2020 15:53:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Gerrit-MessageType: comment

Furquan Shaikh (Code Review)

8:56 a.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Convert DDR4 MHz to MT/s correctly

Furquan Shaikh 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:

(1 comment)

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)

...

I refactored the code to use a table of tCK. […]

Yes, we should move this to ddr4.[c|h]. But, as mentioned on the latest patchset, I think the function can be simpler by having a table with min,max for Mhz and the expected MT/s for that range.

-- To view, visit https://review.coreboot.org/c/coreboot/+/45177 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc77c21932c68ee9f0ff0b8e35ae7b1a3732b322 Gerrit-Change-Number: 45177 Gerrit-PatchSet: 4 Gerrit-Owner: Rob Barnes robbarnes@google.com Gerrit-Reviewer: Jack Rosenthal jrosenth@chromium.org Gerrit-Reviewer: Paul Fagerburg pfagerburg@chromium.org Gerrit-Reviewer: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Angel Pons th3fanbus@gmail.com Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Fri, 11 Sep 2020 15:56:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Comment-In-Reply-To: Rob Barnes robbarnes@google.com Gerrit-MessageType: comment

Paul Fagerburg (Code Review)

9:35 a.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Convert DDR4 MHz to MT/s correctly

Paul Fagerburg 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: -Code-Review

It looks like the other reviewers want to move this code into a more common location, as well as make changes in implementation. When you do this, please add a unit test for the module as well.

Rob Barnes (Code Review)

11:52 a.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Convert DDR4 MHz to MT/s correctly

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.

-- To view, visit https://review.coreboot.org/c/coreboot/+/45177 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc77c21932c68ee9f0ff0b8e35ae7b1a3732b322 Gerrit-Change-Number: 45177 Gerrit-PatchSet: 4 Gerrit-Owner: Rob Barnes robbarnes@google.com Gerrit-Reviewer: Jack Rosenthal jrosenth@chromium.org Gerrit-Reviewer: Paul Fagerburg pfagerburg@chromium.org Gerrit-Reviewer: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Angel Pons th3fanbus@gmail.com Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Fri, 11 Sep 2020 18:52:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Raul Rangel rrangel@chromium.org Comment-In-Reply-To: Furquan Shaikh furquan@google.com Comment-In-Reply-To: Paul Fagerburg pfagerburg@chromium.org Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Gerrit-MessageType: comment

Rob Barnes (Code Review)

14 Sep 14 Sep

7:13 a.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Convert DDR4 MHz to MT/s correctly

Hello build bot (Jenkins), Raul Rangel, Jack Rosenthal, Paul Fagerburg,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/45177

to look at the new patch set (#5).

Change subject: soc/amd/picasso: Convert DDR4 MHz to MT/s correctly ......................................................................

soc/amd/picasso: Convert DDR4 MHz to MT/s correctly

Memory speed is given as an integer in MHz. In some cases it has an implicit fractional speed, so simply multiplying by 2 is not sufficient.

Use method from dram/ddr4.c instead.

BUG=b:167155849 TEST=Boot ezkinil, check output of 'mosys memory spd print all' and dmidecode -t17

Signed-off-by: Rob Barnes robbarnes@google.com Change-Id: Icc77c21932c68ee9f0ff0b8e35ae7b1a3732b322 --- M src/device/dram/Makefile.inc M src/soc/amd/picasso/dmi.c 2 files changed, 5 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/45177/5

Aaron Durbin (Code Review)

10:45 a.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Convert DDR4 MHz to MT/s correctly

Aaron Durbin 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 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45177/6/src/device/dram/Makefile.in... File src/device/dram/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/45177/6/src/device/dram/Makefile.in... PS6, Line 3: ramstage-y += ddr4.c ddr3.c ddr2.c ddr_common.c these changes should go w/ the previous dependent patch

-- To view, visit https://review.coreboot.org/c/coreboot/+/45177 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc77c21932c68ee9f0ff0b8e35ae7b1a3732b322 Gerrit-Change-Number: 45177 Gerrit-PatchSet: 6 Gerrit-Owner: Rob Barnes robbarnes@google.com Gerrit-Reviewer: Jack Rosenthal jrosenth@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Paul Fagerburg pfagerburg@chromium.org Gerrit-Reviewer: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Angel Pons th3fanbus@gmail.com Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 14 Sep 2020 17:45:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Rob Barnes (Code Review)

11:41 a.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Convert DDR4 MHz to MT/s correctly

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 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45177/6/src/device/dram/Makefile.in... File src/device/dram/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/45177/6/src/device/dram/Makefile.in... PS6, Line 3: ramstage-y += ddr4.c ddr3.c ddr2.c ddr_common.c

...

these changes should go w/ the previous dependent patch

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/45177 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc77c21932c68ee9f0ff0b8e35ae7b1a3732b322 Gerrit-Change-Number: 45177 Gerrit-PatchSet: 7 Gerrit-Owner: Rob Barnes robbarnes@google.com Gerrit-Reviewer: Jack Rosenthal jrosenth@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Paul Fagerburg pfagerburg@chromium.org Gerrit-Reviewer: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Angel Pons th3fanbus@gmail.com Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 14 Sep 2020 18:41:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Gerrit-MessageType: comment

Paul Fagerburg (Code Review)

2:42 p.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Convert DDR4 MHz to MT/s correctly

Paul Fagerburg 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 7: Code-Review+1

-- To view, visit https://review.coreboot.org/c/coreboot/+/45177 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc77c21932c68ee9f0ff0b8e35ae7b1a3732b322 Gerrit-Change-Number: 45177 Gerrit-PatchSet: 7 Gerrit-Owner: Rob Barnes robbarnes@google.com Gerrit-Reviewer: Jack Rosenthal jrosenth@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Paul Fagerburg pfagerburg@chromium.org Gerrit-Reviewer: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Angel Pons th3fanbus@gmail.com Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 14 Sep 2020 21:42:41 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Furquan Shaikh (Code Review)

4:53 p.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Convert DDR4 MHz to MT/s correctly

Furquan Shaikh 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 7: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/45177 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc77c21932c68ee9f0ff0b8e35ae7b1a3732b322 Gerrit-Change-Number: 45177 Gerrit-PatchSet: 7 Gerrit-Owner: Rob Barnes robbarnes@google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Jack Rosenthal jrosenth@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Paul Fagerburg pfagerburg@chromium.org Gerrit-Reviewer: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Angel Pons th3fanbus@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 14 Sep 2020 23:53:02 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Rob Barnes (Code Review)

15 Sep 15 Sep

9:35 a.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Convert DDR4 MHz to MT/s correctly

Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Patrick Georgi, Martin Roth, Jack Rosenthal, Paul Fagerburg,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/45177

to look at the new patch set (#9).

Change subject: soc/amd/picasso: Convert DDR4 MHz to MT/s correctly ......................................................................

soc/amd/picasso: Convert DDR4 MHz to MT/s correctly

Memory speed is given as an integer in MHz. In some cases it has an implicit fractional speed, so simply multiplying by 2 is not sufficient.

Use method from dram/ddr4.c instead.

BUG=b:167155849 TEST=Boot ezkinil, check output of 'mosys memory spd print all' and dmidecode -t17 BRANCH=Zork

Signed-off-by: Rob Barnes robbarnes@google.com Change-Id: Icc77c21932c68ee9f0ff0b8e35ae7b1a3732b322 --- M src/soc/amd/picasso/dmi.c 1 file changed, 3 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/45177/9

-- To view, visit https://review.coreboot.org/c/coreboot/+/45177 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc77c21932c68ee9f0ff0b8e35ae7b1a3732b322 Gerrit-Change-Number: 45177 Gerrit-PatchSet: 9 Gerrit-Owner: Rob Barnes robbarnes@google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Jack Rosenthal jrosenth@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Paul Fagerburg pfagerburg@chromium.org Gerrit-Reviewer: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Angel Pons th3fanbus@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Aaron Durbin (Code Review)

8:25 p.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Convert DDR4 MHz to MT/s correctly

Aaron Durbin 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 11:

(5 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)

...

Yes, we should move this to ddr4.[c|h]. […]

Ack

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

...

Ack

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 : #

...

Thanks for the tip.

Ack

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[] = {

...

Ack

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[] = {

...

I used tck because this is the value that's actually specified the spec. […]

Ack

-- To view, visit https://review.coreboot.org/c/coreboot/+/45177 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc77c21932c68ee9f0ff0b8e35ae7b1a3732b322 Gerrit-Change-Number: 45177 Gerrit-PatchSet: 11 Gerrit-Owner: Rob Barnes robbarnes@google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Jack Rosenthal jrosenth@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Paul Fagerburg pfagerburg@chromium.org Gerrit-Reviewer: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-CC: Angel Pons th3fanbus@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Wed, 16 Sep 2020 03:25:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Comment-In-Reply-To: Paul Fagerburg pfagerburg@chromium.org Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Comment-In-Reply-To: Rob Barnes robbarnes@google.com Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Gerrit-MessageType: comment

Aaron Durbin (Code Review)

8:25 p.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Convert DDR4 MHz to MT/s correctly

Aaron Durbin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45177 )

Change subject: soc/amd/picasso: Convert DDR4 MHz to MT/s correctly ......................................................................

soc/amd/picasso: Convert DDR4 MHz to MT/s correctly

Memory speed is given as an integer in MHz. In some cases it has an implicit fractional speed, so simply multiplying by 2 is not sufficient.

Use method from dram/ddr4.c instead.

BUG=b:167155849 TEST=Boot ezkinil, check output of 'mosys memory spd print all' and dmidecode -t17 BRANCH=Zork

Signed-off-by: Rob Barnes robbarnes@google.com Change-Id: Icc77c21932c68ee9f0ff0b8e35ae7b1a3732b322 Reviewed-on: https://review.coreboot.org/c/coreboot/+/45177 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/soc/amd/picasso/dmi.c 1 file changed, 3 insertions(+), 6 deletions(-)

Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Paul Fagerburg: Looks good to me, but someone else must approve

diff --git a/src/soc/amd/picasso/dmi.c b/src/soc/amd/picasso/dmi.c index 5189cd6..1133670 100644 --- a/src/soc/amd/picasso/dmi.c +++ b/src/soc/amd/picasso/dmi.c @@ -14,6 +14,7 @@ #include <lib.h> #include <dimm_info_util.h> #include <vendorcode/amd/fsp/picasso/dmi_info.h> +#include <device/dram/ddr4.h>

/** * Populate dimm_info using AGESA TYPE17_DMI_INFO. @@ -27,13 +28,9 @@

dimm->ddr_type = dmi17->MemoryType;

- /** - * TYPE17_DMI_INFO holds speed in MHz. - * Change to MT/s by multiplying by 2. - */ - dimm->configured_speed_mts = 2 * dmi17->ConfigSpeed; + dimm->configured_speed_mts = ddr4_speed_mhz_to_reported_mts(dmi17->ConfigSpeed);

- dimm->max_speed_mts = 2 * dmi17->Speed; + dimm->max_speed_mts = ddr4_speed_mhz_to_reported_mts(dmi17->Speed);

dimm->rank_per_dimm = dmi17->Attributes;

-- To view, visit https://review.coreboot.org/c/coreboot/+/45177 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc77c21932c68ee9f0ff0b8e35ae7b1a3732b322 Gerrit-Change-Number: 45177 Gerrit-PatchSet: 12 Gerrit-Owner: Rob Barnes robbarnes@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Jack Rosenthal jrosenth@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Paul Fagerburg pfagerburg@chromium.org Gerrit-Reviewer: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Angel Pons th3fanbus@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: merged

9elements QA (Code Review)

9:30 p.m.

New subject: Change in coreboot[master]: soc/amd/picasso: Convert DDR4 MHz to MT/s correctly

9elements QA 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 12:

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/19515 "QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19514 "QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/19513 "QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19512 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/19511 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19519 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19518 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19517 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19516

Please note: This test is under development and might not be accurate at all!

-- To view, visit https://review.coreboot.org/c/coreboot/+/45177 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc77c21932c68ee9f0ff0b8e35ae7b1a3732b322 Gerrit-Change-Number: 45177 Gerrit-PatchSet: 12 Gerrit-Owner: Rob Barnes robbarnes@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Jack Rosenthal jrosenth@chromium.org Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Paul Fagerburg pfagerburg@chromium.org Gerrit-Reviewer: Raul Rangel rrangel@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: 9elements QA hardwaretestrobot@gmail.com Gerrit-CC: Angel Pons th3fanbus@gmail.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Wed, 16 Sep 2020 04:30:39 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1523

days inactive

1531

days old

coreboot-gerrit@coreboot.org

33 comments

10 participants

tags (0)

participants (10)

9elements QA (Code Review)
Aaron Durbin (Code Review)
Angel Pons (Code Review)
build bot (Jenkins) (Code Review)
Furquan Shaikh (Code Review)
Jack Rosenthal (Code Review)
Paul Fagerburg (Code Review)
Paul Menzel (Code Review)
Raul Rangel (Code Review)
Rob Barnes (Code Review)