Change in coreboot[master]: lib/spd_bin: Extend DDR4 spd information

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

EricR Lai (Code Review)

2 Sep 2019 2 Sep '19

9:09 a.m.

EricR Lai has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

lib/spd_bin: Extend DDR4 spd information

From DDR4 SPD spec:

Byte 4 (0x004): SDRAM Density and Banks Bits [7, 6]: 00 = 0 (no bank groups) 01 = 1 (2 bank groups) 10 = 2 (4 bank groups) 11 = reserved

Bit [5, 4] : 00 = 2 (4 banks) 01 = 3 (8 banks) All others reserved

Separate DDR3 and DDR4 banks. And extened rows, cols and ranks.

Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 --- M src/lib/spd_bin.c 1 file changed, 5 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/35206/1

diff --git a/src/lib/spd_bin.c b/src/lib/spd_bin.c index 1598ff1..e2a811a 100644 --- a/src/lib/spd_bin.c +++ b/src/lib/spd_bin.c @@ -36,10 +36,11 @@ void print_spd_info(uint8_t spd[]) { const int spd_banks[8] = { 8, 16, 32, 64, -1, -1, -1, -1 }; + const int ddr4_banks[10] = { 4, 8, -1, -1, 8, 16, -1, -1, 16, 32 }; const int spd_capmb[8] = { 1, 2, 4, 8, 16, 32, 64, 128 }; - const int spd_rows[8] = {12, 13, 14, 15, 16, 17, -1, -1 }; - const int spd_cols[8] = { 9, 10, 11, 12, -1, -1, -1, -1 }; - const int spd_ranks[8] = { 1, 2, 3, 4, -1, -1, -1, -1 }; + const int spd_rows[8] = { 12, 13, 14, 15, 16, 17, 18, -1 }; + const int spd_cols[8] = { 9, 10, 11, 12, -1, -1, -1, -1 }; + const int spd_ranks[8] = { 1, 2, 3, 4, 5, 6, 7, 8 }; const int spd_devw[8] = { 4, 8, 16, 32, -1, -1, -1, -1 }; const int spd_busw[8] = { 8, 16, 32, 64, -1, -1, -1, -1 }; char spd_name[DDR4_SPD_PART_LEN+1] = { 0 }; @@ -74,6 +75,7 @@ memcpy(spd_name, &spd[DDR4_SPD_PART_OFF], DDR4_SPD_PART_LEN); spd_name[DDR4_SPD_PART_LEN] = 0; ranks = (spd[SPD_ORGANIZATION] >> 3) & 7; + banks = ddr4_banks[(spd[SPD_DENSITY_BANKS] >> 4) & 7]; devw = spd_devw[spd[12] & 7]; busw = spd_busw[spd[13] & 7]; break;

Attachments:

attachment.htm (text/html — 3.9 KB)

Show replies by date

EricR Lai (Code Review)

2 Sep 2 Sep

9:13 a.m.

Hello Julius Werner, Mathew King, Duncan Laurie, Furquan Shaikh,

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

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

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

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

lib/spd_bin: Extend DDR4 spd information

From DDR4 SPD spec:

Byte 4 (0x004): SDRAM Density and Banks Bits [7, 6]: 00 = 0 (no bank groups) 01 = 1 (2 bank groups) 10 = 2 (4 bank groups) 11 = reserved

Bit [5, 4] : 00 = 2 (4 banks) 01 = 3 (8 banks) All others reserved

Separate DDR3 and DDR4 banks. And extened rows, cols and ranks.

Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 --- M src/lib/spd_bin.c 1 file changed, 6 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/35206/2

EricR Lai (Code Review)

9:18 a.m.

Hello Julius Werner, Mathew King, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

lib/spd_bin: Extend DDR4 spd information

From DDR4 SPD spec:

Byte 4 (0x004): SDRAM Density and Banks Bits [7, 6]: 00 = 0 (no bank groups) 01 = 1 (2 bank groups) 10 = 2 (4 bank groups) 11 = reserved

Bit [5, 4] : 00 = 2 (4 banks) 01 = 3 (8 banks) All others reserved

Separate DDR3 and DDR4 banks. And extened capmb, rows, cols and ranks.

Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 --- M src/lib/spd_bin.c 1 file changed, 6 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/35206/3

EricR Lai (Code Review)

9:30 a.m.

Hello Julius Werner, Mathew King, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

lib/spd_bin: Extend DDR4 spd information

From DDR4 SPD spec:

Byte 4 (0x004): SDRAM Density and Banks Bits [7, 6]: 00 = 0 (no bank groups) 01 = 1 (2 bank groups) 10 = 2 (4 bank groups) 11 = reserved

Bit [5, 4] : 00 = 2 (4 banks) 01 = 3 (8 banks) All others reserved

Separate DDR3 and DDR4 banks. And extened capmb, rows, cols and ranks.

Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 --- M src/include/spd_bin.h M src/lib/spd_bin.c 2 files changed, 11 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/35206/4

EricR Lai (Code Review)

9:36 a.m.

Hello Julius Werner, Mathew King, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

lib/spd_bin: Extend DDR4 spd information

From DDR4 SPD spec:

Byte 4 (0x004): SDRAM Density and Banks Bits [7, 6]: 00 = 0 (no bank groups) 01 = 1 (2 bank groups) 10 = 2 (4 bank groups) 11 = reserved

Bit [5, 4] : 00 = 2 (4 banks) 01 = 3 (8 banks) All others reserved

Separate DDR3 and DDR4 banks. And extened capmb, rows, cols and ranks. Separate DDR3 and DDR4 ORGANIZATION/BUS_DEV_WIDTH offset.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/35206/5

EricR Lai (Code Review)

10:23 a.m.

Hello Julius Werner, Mathew King, Tim Wawrzynczak, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

lib/spd_bin: Extend DDR4 spd information

From DDR4 SPD spec:

Byte 4 (0x004): SDRAM Density and Banks Bits [7, 6]: 00 = 0 (no bank groups) 01 = 1 (2 bank groups) 10 = 2 (4 bank groups) 11 = reserved

Bit [5, 4] : 00 = 2 (4 banks) 01 = 3 (8 banks) All others reserved

Separate DDR3 and DDR4 banks. And extened capmb, rows, cols and ranks. Separate DDR3 and DDR4 ORGANIZATION/BUS_DEV_WIDTH offset.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/35206/6

Furquan Shaikh (Code Review)

11:06 p.m.

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

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35206/6/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/6/src/lib/spd_bin.c@37 PS6, Line 37: : const int spd_banks[8] = { 8, 16, 32, 64, -1, -1, -1, -1 }; : const int ddr4_banks[10] = { 4, 8, -1, -1, 8, 16, -1, -1, 16, 32 }; : const int spd_capmb[10] = { 1, 2, 4, 8, 16, 32, 64, 128, 48, 96 }; : const int spd_rows[8] = { 12, 13, 14, 15, 16, 17, 18, -1 }; : const int spd_cols[8] = { 9, 10, 11, 12, -1, -1, -1, -1 }; : const int spd_ranks[8] = { 1, 2, 3, 4, 5, 6, 7, 8 }; : const int spd_devw[8] = { 4, 8, 16, 32, -1, -1, -1, -1 }; : const int spd_busw[8] = { 8, 16, 32, 64, -1, -1, -1, -1 }; : char spd_name[DDR4_SPD_PART_LEN+1] = { 0 }; : : int banks = spd_banks[(spd[SPD_DENSITY_BANKS] >> 4) & 7]; : int capmb = spd_capmb[spd[SPD_DENSITY_BANKS] & 7] * 256; : int rows = spd_rows[(spd[SPD_ADDRESSING] >> 3) & 7]; : int cols = spd_cols[spd[SPD_ADDRESSING] & 7]; : int ranks = spd_ranks[(spd[DDR3_ORGANIZATION] >> 3) & 7]; : int devw = spd_devw[spd[DDR3_ORGANIZATION] & 7]; : int busw = spd_busw[spd[DDR3_BUS_DEV_WIDTH] & 7]; : I am wondering if we should reorganize this function to make it more clear. Right now, we read all these memory params assuming it is DDR3 and then re-read again if it turns out to be DDR4. How about having a function for each param that is required. Something like:

const char *spd_get_module_type_string(int dram_type) { switch (dram_type) { case SPD_DRAM_DDR3: return "DDR3"; case SPD_DRAM_LPDDR3_INTEL: case SPD_DRAM_LPDDR3_JEDEC: return "LPDDR3"; case SPD_DRAM_DDR4: return "DDR4"; } return "UNKNOWN"; }

int spd_get_banks(uint8_t spd[], int dram_type) { static const int spd_banks[8] = { ... }; int spd_banks_idx = (spd[SPD_DENSITY_BANKS] >> 4) & 7;

if (spd_banks_idx >= ARRAY_SIZE(spd_banks)) return -1;

return spd_banks[spd_banks_idx]; }

and so on. That way if a new memory type needs to be supported in the future, each function can be compared against the spec to decide what needs to be done for it. Thoughts?

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 6 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Mon, 02 Sep 2019 21:06:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

EricR Lai (Code Review)

3 Sep 3 Sep

5:39 a.m.

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 6:

...

Patch Set 6:

(1 comment)

Agree, I can work on this.

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 6 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 03 Sep 2019 03:39:04 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

EricR Lai (Code Review)

9:28 a.m.

Hello Julius Werner, Mathew King, Tim Wawrzynczak, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

lib/spd_bin: Extend DDR4 spd information

From DDR4 SPD spec:

Byte 4 (0x004): SDRAM Density and Banks Bits [7, 6]: 00 = 0 (no bank groups) 01 = 1 (2 bank groups) 10 = 2 (4 bank groups) 11 = reserved

Bit [5, 4] : 00 = 2 (4 banks) 01 = 3 (8 banks) All others reserved

Separate DDR3 and DDR4 banks. And extened capmb, rows, cols and ranks. Separate DDR3 and DDR4 ORGANIZATION/BUS_DEV_WIDTH offset.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/35206/7

build bot (Jenkins) (Code Review)

9:28 a.m.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 7:

(5 comments)

https://review.coreboot.org/c/coreboot/+/35206/7/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/7/src/lib/spd_bin.c@43 PS7, Line 43: switch (dram_type) { switch and case should be at the same indent

https://review.coreboot.org/c/coreboot/+/35206/7/src/lib/spd_bin.c@60 PS7, Line 60: if (is_memory_type_ddr4(dram_type)) that open brace { should be on the previous line

https://review.coreboot.org/c/coreboot/+/35206/7/src/lib/spd_bin.c@66 PS7, Line 66: else that open brace { should be on the previous line

https://review.coreboot.org/c/coreboot/+/35206/7/src/lib/spd_bin.c@66 PS7, Line 66: else else should follow close brace '}'

https://review.coreboot.org/c/coreboot/+/35206/7/src/lib/spd_bin.c@148 PS7, Line 148: printk(BIOS_INFO, "%s\n",spd_get_module_type_string(type)); space required after that ',' (ctx:VxV)

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 7 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 03 Sep 2019 07:28:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

EricR Lai (Code Review)

9:29 a.m.

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 7:

(1 comment)

...

Patch Set 6:

(1 comment)

@Furquan, I finished a draft version. IF you want make a change, let me know.

https://review.coreboot.org/c/coreboot/+/35206/6/src/lib/spd_bin.c File src/lib/spd_bin.c:

...

I am wondering if we should reorganize this function to make it more clear. […]

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 7 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 03 Sep 2019 07:29:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

EricR Lai (Code Review)

9:33 a.m.

Hello Julius Werner, Mathew King, Tim Wawrzynczak, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

lib/spd_bin: Extend DDR4 spd information

From DDR4 SPD spec:

Byte 4 (0x004): SDRAM Density and Banks Bits [7, 6]: 00 = 0 (no bank groups) 01 = 1 (2 bank groups) 10 = 2 (4 bank groups) 11 = reserved

Bit [5, 4] : 00 = 2 (4 banks) 01 = 3 (8 banks) All others reserved

Separate DDR3 and DDR4 banks. And extened capmb, rows, cols and ranks. Separate DDR3 and DDR4 ORGANIZATION/BUS_DEV_WIDTH offset.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/35206/8

build bot (Jenkins) (Code Review)

9:34 a.m.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 8:

(5 comments)

https://review.coreboot.org/c/coreboot/+/35206/8/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/8/src/lib/spd_bin.c@43 PS8, Line 43: switch (dram_type) { switch and case should be at the same indent

https://review.coreboot.org/c/coreboot/+/35206/8/src/lib/spd_bin.c@60 PS8, Line 60: if (is_memory_type_ddr4(dram_type)) that open brace { should be on the previous line

https://review.coreboot.org/c/coreboot/+/35206/8/src/lib/spd_bin.c@66 PS8, Line 66: else that open brace { should be on the previous line

https://review.coreboot.org/c/coreboot/+/35206/8/src/lib/spd_bin.c@66 PS8, Line 66: else else should follow close brace '}'

https://review.coreboot.org/c/coreboot/+/35206/8/src/lib/spd_bin.c@148 PS8, Line 148: printk(BIOS_INFO, "%s\n",spd_get_module_type_string(type)); space required after that ',' (ctx:VxV)

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 8 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 03 Sep 2019 07:34:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

EricR Lai (Code Review)

9:34 a.m.

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 8:

(4 comments)

https://review.coreboot.org/c/coreboot/+/35206/7/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/7/src/lib/spd_bin.c@43 PS7, Line 43: switch (dram_type) {

...

switch and case should be at the same indent

Done

https://review.coreboot.org/c/coreboot/+/35206/7/src/lib/spd_bin.c@60 PS7, Line 60: if (is_memory_type_ddr4(dram_type))

...

that open brace { should be on the previous line

Done

https://review.coreboot.org/c/coreboot/+/35206/7/src/lib/spd_bin.c@66 PS7, Line 66: else

...

else should follow close brace '}'

Done

https://review.coreboot.org/c/coreboot/+/35206/7/src/lib/spd_bin.c@148 PS7, Line 148: printk(BIOS_INFO, "%s\n",spd_get_module_type_string(type));

...

space required after that ',' (ctx:VxV)

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 8 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 03 Sep 2019 07:34:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: comment

EricR Lai (Code Review)

9:35 a.m.

Hello Julius Werner, Mathew King, Tim Wawrzynczak, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

lib/spd_bin: Extend DDR4 spd information

From DDR4 SPD spec:

Byte 4 (0x004): SDRAM Density and Banks Bits [7, 6]: 00 = 0 (no bank groups) 01 = 1 (2 bank groups) 10 = 2 (4 bank groups) 11 = reserved

Bit [5, 4] : 00 = 2 (4 banks) 01 = 3 (8 banks) All others reserved

Separate DDR3 and DDR4 banks. And extened capmb, rows, cols and ranks. Separate DDR3 and DDR4 ORGANIZATION/BUS_DEV_WIDTH offset.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/35206/9

build bot (Jenkins) (Code Review)

9:36 a.m.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 9:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35206/9/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/9/src/lib/spd_bin.c@64 PS9, Line 64: } else { else is not generally useful after a break or return

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 9 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 03 Sep 2019 07:36:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

EricR Lai (Code Review)

9:39 a.m.

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 9:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35206/9/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/9/src/lib/spd_bin.c@64 PS9, Line 64: } else {

...

else is not generally useful after a break or return

Ack

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 9 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 03 Sep 2019 07:39:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: comment

build bot (Jenkins) (Code Review)

9:47 a.m.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 10:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35206/10/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/10/src/lib/spd_bin.c@64 PS10, Line 64: } else { else is not generally useful after a break or return

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 10 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 03 Sep 2019 07:47:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

EricR Lai (Code Review)

10:12 a.m.

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 10:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35206/10/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/10/src/lib/spd_bin.c@64 PS10, Line 64: } else {

...

else is not generally useful after a break or return

Ack

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 10 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 03 Sep 2019 08:12:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: comment

Tim Wawrzynczak (Code Review)

5:27 p.m.

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 10:

(3 comments)

https://review.coreboot.org/c/coreboot/+/35206/10/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/10/src/lib/spd_bin.c@71 PS10, Line 71: uint8_t This is not expected to be modified, const ? Also see other spd_get_* below

https://review.coreboot.org/c/coreboot/+/35206/10/src/lib/spd_bin.c@73 PS10, Line 73: const What about making all of these local arrays static? (see others spd_get_*)

https://review.coreboot.org/c/coreboot/+/35206/10/src/lib/spd_bin.c@144 PS10, Line 144: printk(BIOS_INFO, "SPD: module type is "); : printk(BIOS_INFO, "%s\n", spd_get_module_type_string(type)); nit: I don't see a good reason to break this up into two calls to printk, can you leave it as one?

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 10 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 03 Sep 2019 15:27:23 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

EricR Lai (Code Review)

6:05 p.m.

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 10:

(3 comments)

https://review.coreboot.org/c/coreboot/+/35206/10/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/10/src/lib/spd_bin.c@71 PS10, Line 71: uint8_t

...

This is not expected to be modified, const ? Also see other spd_get_* below

ok, make sense to me.

https://review.coreboot.org/c/coreboot/+/35206/10/src/lib/spd_bin.c@73 PS10, Line 73: const

...

What about making all of these local arrays static? (see others spd_get_*)

I just keep the origin declaration, but static is fine.

https://review.coreboot.org/c/coreboot/+/35206/10/src/lib/spd_bin.c@144 PS10, Line 144: printk(BIOS_INFO, "SPD: module type is "); : printk(BIOS_INFO, "%s\n", spd_get_module_type_string(type));

...

nit: I don't see a good reason to break this up into two calls to printk, can you leave it as one?

I need warp the line anyway.

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 10 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 03 Sep 2019 16:05:23 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-MessageType: comment

EricR Lai (Code Review)

6:20 p.m.

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 10:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35206/10/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/10/src/lib/spd_bin.c@73 PS10, Line 73: const

...

I just keep the origin declaration, but static is fine.

@Tim, static const or just static?

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 10 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 03 Sep 2019 16:20:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: EricR Lai ericr_lai@compal.corp-partner.google.com Comment-In-Reply-To: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-MessageType: comment

Tim Wawrzynczak (Code Review)

6:32 p.m.

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 10:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35206/10/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/10/src/lib/spd_bin.c@73 PS10, Line 73: const

...

@Tim, static const or just static?

I would prefer static const

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 10 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 03 Sep 2019 16:32:23 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: EricR Lai ericr_lai@compal.corp-partner.google.com Comment-In-Reply-To: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-MessageType: comment

Furquan Shaikh (Code Review)

11:29 p.m.

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

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 10:

(2 comments)

https://review.coreboot.org/c/coreboot/+/35206/10/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/10/src/lib/spd_bin.c@60 PS10, Line 60: if (is_memory_type_ddr4(dram_type)) { : if (index >= ARRAY_SIZE(ddr4_banks)) : return -1; : return ddr4_banks[index]; : } else { Just use switch case here and in functions below for dram_type rather than having a special is_memory_type_ddr4() helper?

https://review.coreboot.org/c/coreboot/+/35206/10/src/lib/spd_bin.c@146 PS10, Line 146: /* Module Part Number */ : switch (type) { : case SPD_DRAM_DDR3: : memcpy(spd_name, &spd[DDR3_SPD_PART_OFF], DDR3_SPD_PART_LEN); : spd_name[DDR3_SPD_PART_LEN] = 0; : break; : case SPD_DRAM_LPDDR3_INTEL: : case SPD_DRAM_LPDDR3_JEDEC: : memcpy(spd_name, &spd[LPDDR3_SPD_PART_OFF], : LPDDR3_SPD_PART_LEN); : spd_name[LPDDR3_SPD_PART_LEN] = 0; : break; : case SPD_DRAM_DDR4: : memcpy(spd_name, &spd[DDR4_SPD_PART_OFF], DDR4_SPD_PART_LEN); : spd_name[DDR4_SPD_PART_LEN] = 0; : break; : default: : break; : } Have a helper for Module Part number as well?

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 10 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 03 Sep 2019 21:29:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

EricR Lai (Code Review)

4 Sep 4 Sep

6:30 a.m.

Hello Julius Werner, Mathew King, Tim Wawrzynczak, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

lib/spd_bin: Extend DDR4 spd information

From DDR4 SPD spec:

Byte 4 (0x004): SDRAM Density and Banks Bits [7, 6]: 00 = 0 (no bank groups) 01 = 1 (2 bank groups) 10 = 2 (4 bank groups) 11 = reserved

Bit [5, 4] : 00 = 2 (4 banks) 01 = 3 (8 banks) All others reserved

Separate DDR3 and DDR4 banks. And extened capmb, rows, cols and ranks. Separate DDR3 and DDR4 ORGANIZATION/BUS_DEV_WIDTH offset.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/35206/11

build bot (Jenkins) (Code Review)

6:31 a.m.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 11:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35206/11/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/11/src/lib/spd_bin.c@171 PS11, Line 171: printk(BIOS_INFO, "SPD: module type is %s\n", trailing whitespace

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 11 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Wed, 04 Sep 2019 04:31:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

EricR Lai (Code Review)

6:32 a.m.

Hello Julius Werner, Mathew King, Tim Wawrzynczak, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

lib/spd_bin: Extend DDR4 spd information

From DDR4 SPD spec:

Byte 4 (0x004): SDRAM Density and Banks Bits [7, 6]: 00 = 0 (no bank groups) 01 = 1 (2 bank groups) 10 = 2 (4 bank groups) 11 = reserved

Bit [5, 4] : 00 = 2 (4 banks) 01 = 3 (8 banks) All others reserved

Separate DDR3 and DDR4 banks. And extened capmb, rows, cols and ranks. Separate DDR3 and DDR4 ORGANIZATION/BUS_DEV_WIDTH offset.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/35206/12

build bot (Jenkins) (Code Review)

6:33 a.m.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 12:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35206/12/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/12/src/lib/spd_bin.c@171 PS12, Line 171: printk(BIOS_INFO, "SPD: module type is %s\n", trailing whitespace

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 12 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Wed, 04 Sep 2019 04:33:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

EricR Lai (Code Review)

6:34 a.m.

Hello Julius Werner, Mathew King, Tim Wawrzynczak, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

lib/spd_bin: Extend DDR4 spd information

From DDR4 SPD spec:

Byte 4 (0x004): SDRAM Density and Banks Bits [7, 6]: 00 = 0 (no bank groups) 01 = 1 (2 bank groups) 10 = 2 (4 bank groups) 11 = reserved

Bit [5, 4] : 00 = 2 (4 banks) 01 = 3 (8 banks) All others reserved

Separate DDR3 and DDR4 banks. And extened capmb, rows, cols and ranks. Separate DDR3 and DDR4 ORGANIZATION/BUS_DEV_WIDTH offset.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/35206/13

EricR Lai (Code Review)

6:43 a.m.

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 13:

(7 comments)

https://review.coreboot.org/c/coreboot/+/35206/10/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/10/src/lib/spd_bin.c@64 PS10, Line 64: } else {

...

Ack

Done

https://review.coreboot.org/c/coreboot/+/35206/10/src/lib/spd_bin.c@71 PS10, Line 71: uint8_t

...

ok, make sense to me.

Done

https://review.coreboot.org/c/coreboot/+/35206/10/src/lib/spd_bin.c@73 PS10, Line 73: const

...

I would prefer static const

Done

https://review.coreboot.org/c/coreboot/+/35206/10/src/lib/spd_bin.c@144 PS10, Line 144: printk(BIOS_INFO, "SPD: module type is "); : printk(BIOS_INFO, "%s\n", spd_get_module_type_string(type));

...

I need warp the line anyway.

Done

...

Have a helper for Module Part number as well?

Done

https://review.coreboot.org/c/coreboot/+/35206/11/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/11/src/lib/spd_bin.c@171 PS11, Line 171: printk(BIOS_INFO, "SPD: module type is %s\n",

...

trailing whitespace

Done

https://review.coreboot.org/c/coreboot/+/35206/12/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/12/src/lib/spd_bin.c@171 PS12, Line 171: printk(BIOS_INFO, "SPD: module type is %s\n",

...

trailing whitespace

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 13 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Wed, 04 Sep 2019 04:43:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: EricR Lai ericr_lai@compal.corp-partner.google.com Comment-In-Reply-To: Tim Wawrzynczak twawrzynczak@chromium.org Comment-In-Reply-To: build bot (Jenkins) no-reply@coreboot.org Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

EricR Lai (Code Review)

7:59 a.m.

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 13:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35206/10/src/lib/spd_bin.c File src/lib/spd_bin.c:

...

Just use switch case here and in functions below for dram_type rather than having a special is_memor […]

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 13 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Wed, 04 Sep 2019 05:59:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

Tim Wawrzynczak (Code Review)

5:08 p.m.

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 13:

(2 comments)

https://review.coreboot.org/c/coreboot/+/35206/13/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/13/src/lib/spd_bin.c@46 PS13, Line 46: case SPD_DRAM_LPDDR3_INTEL: I know I'm being nitpicky, but would you mind adding a comment indicating that the fallthrough of the different cases is intentional? /* intentional fallthrough */ or something like that?

https://review.coreboot.org/c/coreboot/+/35206/13/src/lib/spd_bin.c@61 PS13, Line 61: case SPD_DRAM_DDR3: : case SPD_DRAM_LPDDR3_INTEL: /* intentional fallthrough */ or something like that?

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 13 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Wed, 04 Sep 2019 15:08:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

EricR Lai (Code Review)

5:54 p.m.

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 13:

(2 comments)

https://review.coreboot.org/c/coreboot/+/35206/13/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/13/src/lib/spd_bin.c@46 PS13, Line 46: case SPD_DRAM_LPDDR3_INTEL:

...

I know I'm being nitpicky, but would you mind adding a comment indicating that the fallthrough of th […]

I think here is pretty clear by type name.

https://review.coreboot.org/c/coreboot/+/35206/13/src/lib/spd_bin.c@61 PS13, Line 61: case SPD_DRAM_DDR3: : case SPD_DRAM_LPDDR3_INTEL:

...

/* intentional fallthrough */ or something like that?

I can add some here, /* DDR3 and LPDDR3 has the same bank definition */

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 13 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Wed, 04 Sep 2019 15:54:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-MessageType: comment

Tim Wawrzynczak (Code Review)

5:58 p.m.

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 13:

(2 comments)

https://review.coreboot.org/c/coreboot/+/35206/13/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/13/src/lib/spd_bin.c@46 PS13, Line 46: case SPD_DRAM_LPDDR3_INTEL:

...

I think here is pretty clear by type name.

Ack

https://review.coreboot.org/c/coreboot/+/35206/13/src/lib/spd_bin.c@61 PS13, Line 61: case SPD_DRAM_DDR3: : case SPD_DRAM_LPDDR3_INTEL:

...

I can add some here, /* DDR3 and LPDDR3 has the same bank definition */

Ack

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 13 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Wed, 04 Sep 2019 15:58:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: EricR Lai ericr_lai@compal.corp-partner.google.com Comment-In-Reply-To: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-MessageType: comment

EricR Lai (Code Review)

5 Sep 5 Sep

5:51 a.m.

Hello Julius Werner, Mathew King, Tim Wawrzynczak, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

lib/spd_bin: Extend DDR4 spd information

From DDR4 SPD spec:

Byte 4 (0x004): SDRAM Density and Banks Bits [7, 6]: 00 = 0 (no bank groups) 01 = 1 (2 bank groups) 10 = 2 (4 bank groups) 11 = reserved

Bit [5, 4] : 00 = 2 (4 banks) 01 = 3 (8 banks) All others reserved

Separate DDR3 and DDR4 banks. And extened capmb, rows, cols and ranks. Separate DDR3 and DDR4 ORGANIZATION/BUS_DEV_WIDTH offset.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/35206/14

EricR Lai (Code Review)

5:52 a.m.

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 14:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35206/13/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/13/src/lib/spd_bin.c@61 PS13, Line 61: case SPD_DRAM_DDR3: : case SPD_DRAM_LPDDR3_INTEL:

...

Ack

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 14 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Thu, 05 Sep 2019 03:52:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: EricR Lai ericr_lai@compal.corp-partner.google.com Comment-In-Reply-To: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-MessageType: comment

EricR Lai (Code Review)

6:43 p.m.

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 14:

Any comment here? I am happy to modify it.

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 14 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Thu, 05 Sep 2019 16:43:06 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Tim Wawrzynczak (Code Review)

6:46 p.m.

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 14: Code-Review+1

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 14 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Thu, 05 Sep 2019 16:46:29 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Furquan Shaikh (Code Review)

7:02 p.m.

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

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 14: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/35206/14/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/14/src/lib/spd_bin.c@36 PS14, Line 36: _Bool nit: "bool" should work.

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 14 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Thu, 05 Sep 2019 17:02:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

EricR Lai (Code Review)

7:22 p.m.

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 14:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35206/14/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/14/src/lib/spd_bin.c@36 PS14, Line 36: _Bool

...

nit: "bool" should work.

There is no bool in payload. We discuss a little bit before. There is no stdbool.h.

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 14 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Thu, 05 Sep 2019 17:22:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

Furquan Shaikh (Code Review)

6 Sep 6 Sep

1:07 a.m.

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

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 14:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35206/14/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/14/src/lib/spd_bin.c@36 PS14, Line 36: _Bool

...

There is no bool in payload. We discuss a little bit before. There is no stdbool.h.

bool is present in coreboot proper.

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 14 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Thu, 05 Sep 2019 23:07:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: EricR Lai ericr_lai@compal.corp-partner.google.com Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

EricR Lai (Code Review)

5:27 a.m.

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 14:

...

Patch Set 14:

(1 comment)

Yes, but not in payload. If you are interested, you can try it. I am curious as well.

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 14 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Fri, 06 Sep 2019 03:27:09 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

EricR Lai (Code Review)

5:29 a.m.

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 14:

(1 comment)

...

Patch Set 14:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35206/14/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/14/src/lib/spd_bin.c@36 PS14, Line 36: _Bool

...

bool is present in coreboot proper.

OH, this is belong to coreboot, please ignore me, I can change it.

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 14 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Fri, 06 Sep 2019 03:29:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: EricR Lai ericr_lai@compal.corp-partner.google.com Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

EricR Lai (Code Review)

5:30 a.m.

Hello Julius Werner, Mathew King, Tim Wawrzynczak, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

lib/spd_bin: Extend DDR4 spd information

From DDR4 SPD spec:

Byte 4 (0x004): SDRAM Density and Banks Bits [7, 6]: 00 = 0 (no bank groups) 01 = 1 (2 bank groups) 10 = 2 (4 bank groups) 11 = reserved

Bit [5, 4] : 00 = 2 (4 banks) 01 = 3 (8 banks) All others reserved

Separate DDR3 and DDR4 banks. And extened capmb, rows, cols and ranks. Separate DDR3 and DDR4 ORGANIZATION/BUS_DEV_WIDTH offset.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/35206/15

EricR Lai (Code Review)

5:31 a.m.

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 15:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35206/14/src/lib/spd_bin.c File src/lib/spd_bin.c:

https://review.coreboot.org/c/coreboot/+/35206/14/src/lib/spd_bin.c@36 PS14, Line 36: _Bool

...

OH, this is belong to coreboot, please ignore me, I can change it.

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 15 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Fri, 06 Sep 2019 03:31:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: EricR Lai ericr_lai@compal.corp-partner.google.com Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

Furquan Shaikh (Code Review)

9:36 a.m.

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

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

Patch Set 15: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 15 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Fri, 06 Sep 2019 07:36:23 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Patrick Georgi (Code Review)

9 Sep 9 Sep

3:30 p.m.

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35206 )

Change subject: lib/spd_bin: Extend DDR4 spd information ......................................................................

lib/spd_bin: Extend DDR4 spd information

From DDR4 SPD spec:

Byte 4 (0x004): SDRAM Density and Banks Bits [7, 6]: 00 = 0 (no bank groups) 01 = 1 (2 bank groups) 10 = 2 (4 bank groups) 11 = reserved

Bit [5, 4] : 00 = 2 (4 banks) 01 = 3 (8 banks) All others reserved

Separate DDR3 and DDR4 banks. And extened capmb, rows, cols and ranks. Separate DDR3 and DDR4 ORGANIZATION/BUS_DEV_WIDTH offset.

Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Reviewed-on: https://review.coreboot.org/c/coreboot/+/35206 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/include/spd_bin.h M src/lib/spd_bin.c M src/mainboard/google/cyan/spd/spd.c 3 files changed, 128 insertions(+), 34 deletions(-)

Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved

diff --git a/src/include/spd_bin.h b/src/include/spd_bin.h index e0a50ff..3de60a9 100644 --- a/src/include/spd_bin.h +++ b/src/include/spd_bin.h @@ -31,8 +31,10 @@ #define SPD_DRAM_DDR4 0x0C #define SPD_DENSITY_BANKS 4 #define SPD_ADDRESSING 5 -#define SPD_ORGANIZATION 7 -#define SPD_BUS_DEV_WIDTH 8 +#define DDR3_ORGANIZATION 7 +#define DDR3_BUS_DEV_WIDTH 8 +#define DDR4_ORGANIZATION 12 +#define DDR4_BUS_DEV_WIDTH 13 #define DDR3_SPD_PART_OFF 128 #define DDR3_SPD_PART_LEN 18 #define LPDDR3_SPD_PART_OFF 128 diff --git a/src/lib/spd_bin.c b/src/lib/spd_bin.c index 1598ff1..62e600b 100644 --- a/src/lib/spd_bin.c +++ b/src/lib/spd_bin.c @@ -33,54 +33,146 @@ } }

-void print_spd_info(uint8_t spd[]) +static bool is_memory_type_ddr4(int dram_type) { - const int spd_banks[8] = { 8, 16, 32, 64, -1, -1, -1, -1 }; - const int spd_capmb[8] = { 1, 2, 4, 8, 16, 32, 64, 128 }; - const int spd_rows[8] = {12, 13, 14, 15, 16, 17, -1, -1 }; - const int spd_cols[8] = { 9, 10, 11, 12, -1, -1, -1, -1 }; - const int spd_ranks[8] = { 1, 2, 3, 4, -1, -1, -1, -1 }; - const int spd_devw[8] = { 4, 8, 16, 32, -1, -1, -1, -1 }; - const int spd_busw[8] = { 8, 16, 32, 64, -1, -1, -1, -1 }; - char spd_name[DDR4_SPD_PART_LEN+1] = { 0 }; + return (dram_type == SPD_DRAM_DDR4); +}

- int banks = spd_banks[(spd[SPD_DENSITY_BANKS] >> 4) & 7]; - int capmb = spd_capmb[spd[SPD_DENSITY_BANKS] & 7] * 256; - int rows = spd_rows[(spd[SPD_ADDRESSING] >> 3) & 7]; - int cols = spd_cols[spd[SPD_ADDRESSING] & 7]; - int ranks = spd_ranks[(spd[SPD_ORGANIZATION] >> 3) & 7]; - int devw = spd_devw[spd[SPD_ORGANIZATION] & 7]; - int busw = spd_busw[spd[SPD_BUS_DEV_WIDTH] & 7]; - - /* Module type */ - printk(BIOS_INFO, "SPD: module type is "); - switch (spd[SPD_DRAM_TYPE]) { +static const char *spd_get_module_type_string(int dram_type) +{ + switch (dram_type) { case SPD_DRAM_DDR3: - printk(BIOS_INFO, "DDR3\n"); - /* Module Part Number */ + return "DDR3"; + case SPD_DRAM_LPDDR3_INTEL: + case SPD_DRAM_LPDDR3_JEDEC: + return "LPDDR3"; + case SPD_DRAM_DDR4: + return "DDR4"; + } + return "UNKNOWN"; +} + +static int spd_get_banks(const uint8_t spd[], int dram_type) +{ + static const int ddr3_banks[4] = { 8, 16, 32, 64 }; + static const int ddr4_banks[10] = { 4, 8, -1, -1, 8, 16, -1, -1, 16, 32 }; + int index = (spd[SPD_DENSITY_BANKS] >> 4) & 0xf; + switch (dram_type) { + /* DDR3 and LPDDR3 has the same bank definition */ + case SPD_DRAM_DDR3: + case SPD_DRAM_LPDDR3_INTEL: + case SPD_DRAM_LPDDR3_JEDEC: + if (index >= ARRAY_SIZE(ddr3_banks)) + return -1; + return ddr3_banks[index]; + case SPD_DRAM_DDR4: + if (index >= ARRAY_SIZE(ddr4_banks)) + return -1; + return ddr4_banks[index]; + default: + return -1; + } +} + +static int spd_get_capmb(const uint8_t spd[]) +{ + static const int spd_capmb[10] = { 1, 2, 4, 8, 16, 32, 64, 128, 48, 96 }; + int index = spd[SPD_DENSITY_BANKS] & 0xf; + if (index >= ARRAY_SIZE(spd_capmb)) + return -1; + return spd_capmb[index] * 256; +} + +static int spd_get_rows(const uint8_t spd[]) +{ + static const int spd_rows[7] = { 12, 13, 14, 15, 16, 17, 18 }; + int index = (spd[SPD_ADDRESSING] >> 3) & 7; + if (index >= ARRAY_SIZE(spd_rows)) + return -1; + return spd_rows[index]; +} + +static int spd_get_cols(const uint8_t spd[]) +{ + static const int spd_cols[4] = { 9, 10, 11, 12 }; + int index = spd[SPD_ADDRESSING] & 7; + if (index >= ARRAY_SIZE(spd_cols)) + return -1; + return spd_cols[index]; +} + +static int spd_get_ranks(const uint8_t spd[], int dram_type) +{ + static const int spd_ranks[8] = { 1, 2, 3, 4, 5, 6, 7, 8 }; + int organ_offset = is_memory_type_ddr4(dram_type) ? DDR4_ORGANIZATION + : DDR3_ORGANIZATION; + int index = (spd[organ_offset] >> 3) & 7; + if (index >= ARRAY_SIZE(spd_ranks)) + return -1; + return spd_ranks[index]; +} + +static int spd_get_devw(const uint8_t spd[], int dram_type) +{ + static const int spd_devw[4] = { 4, 8, 16, 32 }; + int organ_offset = is_memory_type_ddr4(dram_type) ? DDR4_ORGANIZATION + : DDR3_ORGANIZATION; + int index = spd[organ_offset] & 7; + if (index >= ARRAY_SIZE(spd_devw)) + return -1; + return spd_devw[index]; +} + +static int spd_get_busw(const uint8_t spd[], int dram_type) +{ + static const int spd_busw[4] = { 8, 16, 32, 64 }; + int busw_offset = is_memory_type_ddr4(dram_type) ? DDR4_BUS_DEV_WIDTH + : DDR3_BUS_DEV_WIDTH; + int index = spd[busw_offset] & 7; + if (index >= ARRAY_SIZE(spd_busw)) + return -1; + return spd_busw[index]; +} + +static void spd_get_name(const uint8_t spd[], char spd_name[], int dram_type) +{ + switch (dram_type) { + case SPD_DRAM_DDR3: memcpy(spd_name, &spd[DDR3_SPD_PART_OFF], DDR3_SPD_PART_LEN); spd_name[DDR3_SPD_PART_LEN] = 0; break; case SPD_DRAM_LPDDR3_INTEL: case SPD_DRAM_LPDDR3_JEDEC: - printk(BIOS_INFO, "LPDDR3\n"); - /* Module Part Number */ memcpy(spd_name, &spd[LPDDR3_SPD_PART_OFF], LPDDR3_SPD_PART_LEN); spd_name[LPDDR3_SPD_PART_LEN] = 0; break; case SPD_DRAM_DDR4: - printk(BIOS_INFO, "DDR4\n"); memcpy(spd_name, &spd[DDR4_SPD_PART_OFF], DDR4_SPD_PART_LEN); spd_name[DDR4_SPD_PART_LEN] = 0; - ranks = (spd[SPD_ORGANIZATION] >> 3) & 7; - devw = spd_devw[spd[12] & 7]; - busw = spd_busw[spd[13] & 7]; break; default: - printk(BIOS_INFO, "Unknown (%02x)\n", spd[SPD_DRAM_TYPE]); break; } +} + +void print_spd_info(uint8_t spd[]) +{ + char spd_name[DDR4_SPD_PART_LEN+1] = { 0 }; + int type = spd[SPD_DRAM_TYPE]; + int banks = spd_get_banks(spd, type); + int capmb = spd_get_capmb(spd); + int rows = spd_get_rows(spd); + int cols = spd_get_cols(spd); + int ranks = spd_get_ranks(spd, type); + int devw = spd_get_devw(spd, type); + int busw = spd_get_busw(spd, type); + + /* Module type */ + printk(BIOS_INFO, "SPD: module type is %s\n", + spd_get_module_type_string(type)); + /* Module Part Number */ + spd_get_name(spd, spd_name, type);

printk(BIOS_INFO, "SPD: module part is %s\n", spd_name);

diff --git a/src/mainboard/google/cyan/spd/spd.c b/src/mainboard/google/cyan/spd/spd.c index 7c66e94..f73b9e6 100644 --- a/src/mainboard/google/cyan/spd/spd.c +++ b/src/mainboard/google/cyan/spd/spd.c @@ -122,9 +122,9 @@ const int spd_busw[8] = { 8, 16, 32, 64, -1, -1, -1, -1 };

int capmb = spd_capmb[spd[SPD_DENSITY_BANKS] & 7] * 256; - int ranks = spd_ranks[(spd[SPD_ORGANIZATION] >> 3) & 7]; - int devw = spd_devw[spd[SPD_ORGANIZATION] & 7]; - int busw = spd_busw[spd[SPD_BUS_DEV_WIDTH] & 7]; + int ranks = spd_ranks[(spd[DDR3_ORGANIZATION] >> 3) & 7]; + int devw = spd_devw[spd[DDR3_ORGANIZATION] & 7]; + int busw = spd_busw[spd[DDR3_BUS_DEV_WIDTH] & 7];

void *hob_list_ptr; EFI_HOB_GUID_TYPE *hob_ptr;

-- To view, visit https://review.coreboot.org/c/coreboot/+/35206 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I5f56975ce73d8ed2d4de7d9fd08e5ae86993e731 Gerrit-Change-Number: 35206 Gerrit-PatchSet: 16 Gerrit-Owner: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Duncan Laurie dlaurie@chromium.org Gerrit-Reviewer: EricR Lai ericr_lai@compal.corp-partner.google.com Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Mathew King mathewk@chromium.org Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Tim Wawrzynczak twawrzynczak@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: merged

1902

days inactive

1909

days old

coreboot-gerrit@coreboot.org

46 comments

5 participants

tags (0)

participants (5)

build bot (Jenkins) (Code Review)
EricR Lai (Code Review)
Furquan Shaikh (Code Review)
Patrick Georgi (Code Review)
Tim Wawrzynczak (Code Review)