EricR Lai has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39352 )
Change subject: lib/spd_bin: Extend LPDDR4 spd information ......................................................................
lib/spd_bin: Extend LPDDR4 spd information
Follow JEDEC 21-C to extend LPDDR4 spd information.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I68c9782c543afab4423296fa7ac1c078db5649c3 --- M src/include/spd_bin.h M src/lib/spd_bin.c 2 files changed, 9 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/39352/1
diff --git a/src/include/spd_bin.h b/src/include/spd_bin.h index f144b14..179d575 100644 --- a/src/include/spd_bin.h +++ b/src/include/spd_bin.h @@ -28,6 +28,7 @@ #define SPD_DRAM_LPDDR3_INTEL 0xF1 #define SPD_DRAM_LPDDR3_JEDEC 0x0F #define SPD_DRAM_DDR4 0x0C +#define SPD_DRAM_LPDDR4_JEDEC 0x10 #define SPD_DENSITY_BANKS 4 #define SPD_ADDRESSING 5 #define DDR3_ORGANIZATION 7 @@ -40,8 +41,6 @@ #define LPDDR3_SPD_PART_LEN 18 #define DDR4_SPD_PART_OFF 329 #define DDR4_SPD_PART_LEN 20 -#define LPDDR4_SPD_PART_OFF 329 -#define LPDDR4_SPD_PART_LEN 20
struct spd_block { u8 addr_map[CONFIG_DIMM_MAX]; /* 7 bit I2C addresses */ diff --git a/src/lib/spd_bin.c b/src/lib/spd_bin.c index 9e625b5..802c401 100644 --- a/src/lib/spd_bin.c +++ b/src/lib/spd_bin.c @@ -45,6 +45,8 @@ return "LPDDR3"; case SPD_DRAM_DDR4: return "DDR4"; + case SPD_DRAM_LPDDR4_JEDEC; + return "LPDDR4"; } return "UNKNOWN"; } @@ -62,7 +64,9 @@ if (index >= ARRAY_SIZE(ddr3_banks)) return -1; return ddr3_banks[index]; + /* DDR4 and LPDDR4 has the same bank definition */ case SPD_DRAM_DDR4: + case SPD_DRAM_LPDDR4_JEDEC; if (index >= ARRAY_SIZE(ddr4_banks)) return -1; return ddr4_banks[index]; @@ -73,7 +77,8 @@
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 }; + static const int spd_capmb[13] = { 1, 2, 4, 8, 16, 32, 64, 128, 48, + 96, 12, 24, 72 }; int index = spd[SPD_DENSITY_BANKS] & 0xf; if (index >= ARRAY_SIZE(spd_capmb)) return -1; @@ -145,6 +150,7 @@ spd_name[LPDDR3_SPD_PART_LEN] = 0; break; case SPD_DRAM_DDR4: + case SPD_DRAM_LPDDR4_JEDEC; memcpy(spd_name, &spd[DDR4_SPD_PART_OFF], DDR4_SPD_PART_LEN); spd_name[DDR4_SPD_PART_LEN] = 0; break; @@ -171,7 +177,7 @@ /* Module Part Number */ spd_get_name(spd, spd_name, type);
- printk(BIOS_INFO, "SPD: module part is %s\n", spd_name); + printk(BIOS_INFO, "SPD: module part number is %s\n", spd_name);
printk(BIOS_INFO, "SPD: banks %d, ranks %d, rows %d, columns %d, density %d Mb\n",
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39352 )
Change subject: lib/spd_bin: Extend LPDDR4 spd information ......................................................................
Patch Set 1:
Since Waddledoo use LPDDR4, I would like to add SPD information print in coreboot. Please help review it.
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39352
to look at the new patch set (#2).
Change subject: lib/spd_bin: Extend LPDDR4 spd information ......................................................................
lib/spd_bin: Extend LPDDR4 spd information
Follow JEDEC 21-C to extend LPDDR4 spd information.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I68c9782c543afab4423296fa7ac1c078db5649c3 --- M src/include/spd_bin.h M src/lib/spd_bin.c 2 files changed, 9 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/39352/2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39352 )
Change subject: lib/spd_bin: Extend LPDDR4 spd information ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39352/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39352/2//COMMIT_MSG@7 PS2, Line 7: spd SPD
https://review.coreboot.org/c/coreboot/+/39352/2//COMMIT_MSG@9 PS2, Line 9: spd SPD
https://review.coreboot.org/c/coreboot/+/39352/2/src/include/spd_bin.h File src/include/spd_bin.h:
https://review.coreboot.org/c/coreboot/+/39352/2/src/include/spd_bin.h@a44 PS2, Line 44: #define LPDDR4_SPD_PART_LEN 20 Why remove this?
https://review.coreboot.org/c/coreboot/+/39352/2/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/39352/2/src/lib/spd_bin.c@81 PS2, Line 81: 96, 12, 24, 72 }; More stuff fits on the line above in 96 characters.
https://review.coreboot.org/c/coreboot/+/39352/2/src/lib/spd_bin.c@180 PS2, Line 180: printk(BIOS_INFO, "SPD: module part number is %s\n", spd_name); Should be a separate commit.
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39352
to look at the new patch set (#3).
Change subject: lib/spd_bin: Extend LPDDR4 SPD information ......................................................................
lib/spd_bin: Extend LPDDR4 SPD information
Follow JEDEC 21-C to extend LPDDR4 SPD information.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I68c9782c543afab4423296fa7ac1c078db5649c3 --- M src/include/spd_bin.h M src/lib/spd_bin.c 2 files changed, 9 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/39352/3
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39352 )
Change subject: lib/spd_bin: Extend LPDDR4 SPD information ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39352/2/src/include/spd_bin.h File src/include/spd_bin.h:
https://review.coreboot.org/c/coreboot/+/39352/2/src/include/spd_bin.h@a44 PS2, Line 44: #define LPDDR4_SPD_PART_LEN 20
Why remove this?
Not used and this is same as DDR4, so remove it.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39352 )
Change subject: lib/spd_bin: Extend LPDDR4 SPD information ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39352/2/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/39352/2/src/lib/spd_bin.c@81 PS2, Line 81: 96, 12, 24, 72 };
More stuff fits on the line above in 96 characters.
Actually I want to cut this into two equal lines. Because it too long and hard to count it.. How do you think?
https://review.coreboot.org/c/coreboot/+/39352/2/src/lib/spd_bin.c@180 PS2, Line 180: printk(BIOS_INFO, "SPD: module part number is %s\n", spd_name);
Should be a separate commit.
I am okay with this. Will do later.
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39352
to look at the new patch set (#4).
Change subject: lib/spd_bin: Extend LPDDR4 SPD information ......................................................................
lib/spd_bin: Extend LPDDR4 SPD information
Follow JEDEC 21-C to extend LPDDR4 SPD information.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I68c9782c543afab4423296fa7ac1c078db5649c3 --- M src/include/spd_bin.h M src/lib/spd_bin.c 2 files changed, 8 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/39352/4
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39352 )
Change subject: lib/spd_bin: Extend LPDDR4 SPD information ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39352/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39352/2//COMMIT_MSG@7 PS2, Line 7: spd
SPD
Done
https://review.coreboot.org/c/coreboot/+/39352/2//COMMIT_MSG@9 PS2, Line 9: spd
SPD
Done
https://review.coreboot.org/c/coreboot/+/39352/2/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/39352/2/src/lib/spd_bin.c@81 PS2, Line 81: 96, 12, 24, 72 };
Actually I want to cut this into two equal lines. Because it too long and hard to count it.. […]
Done
https://review.coreboot.org/c/coreboot/+/39352/2/src/lib/spd_bin.c@180 PS2, Line 180: printk(BIOS_INFO, "SPD: module part number is %s\n", spd_name);
I am okay with this. Will do later.
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39352 )
Change subject: lib/spd_bin: Extend LPDDR4 SPD information ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/39352/4/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/39352/4/src/lib/spd_bin.c@67 PS4, Line 67: has have (yes, the comment above is wrong as well, I'll fix it in a separate commit)
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39352 )
Change subject: lib/spd_bin: Extend LPDDR4 SPD information ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39352/4/src/lib/spd_bin.c File src/lib/spd_bin.c:
https://review.coreboot.org/c/coreboot/+/39352/4/src/lib/spd_bin.c@67 PS4, Line 67: has
have (yes, the comment above is wrong as well, I'll fix it in a separate commit)
okay, thanks.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39352 )
Change subject: lib/spd_bin: Extend LPDDR4 SPD information ......................................................................
Patch Set 4:
(6 comments)
https://review.coreboot.org/c/coreboot/+/39352/4/src/include/spd_bin.h File src/include/spd_bin.h:
https://review.coreboot.org/c/coreboot/+/39352/4/src/include/spd_bin.h@31 PS4, Line 31: _JEDEC Do we need the suffix JEDEC? It was added to LPDDR3 because we had two types of SPDs one with Intel-defined custom ID for LPDDR3 and other with JEDEC-defined. But, LPDDR4 seems to have only one. So, we can skip the suffix _JEDEC just like SPD_DRAM_DDR3 or SPD_DRAM_DDR4.
https://review.coreboot.org/c/coreboot/+/39352/4/src/lib/spd_bin.c File src/lib/spd_bin.c:
PS4: I think this file is getting messier with all the checks for each new DRAM type that we add support for. Don't mean to push this on you, but I think we should switch to using a table which describes the attributes for each memory type and then have generic functions which use that table to return the right values. Then, any time a new memory type has to be added, it just needs another entry in the table.
https://review.coreboot.org/c/coreboot/+/39352/4/src/lib/spd_bin.c@63 PS4, Line 63: SPD_DRAM_LPDDR3_JEDEC I know this wasn't added here. But looking at 21-C, I think both LPDDR3 and LPDDR4 have similar definition for banks.
https://review.coreboot.org/c/coreboot/+/39352/4/src/lib/spd_bin.c@109 PS4, Line 109: is_memory_type_ddr4(dram_type) ? DDR4_ORGANIZATION : : DDR3_ORGANIZATION; This is not handled correctly for both LPDDR3 and LPDDR4.
https://review.coreboot.org/c/coreboot/+/39352/4/src/lib/spd_bin.c@120 PS4, Line 120: is_memory_type_ddr4(dram_type) ? DDR4_ORGANIZATION : : DDR3_ORGANIZATION; same here
https://review.coreboot.org/c/coreboot/+/39352/4/src/lib/spd_bin.c@131 PS4, Line 131: int busw_offset = is_memory_type_ddr4(dram_type) ? DDR4_BUS_DEV_WIDTH : : DDR3_BUS_DEV_WIDTH; and here
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39352 )
Change subject: lib/spd_bin: Extend LPDDR4 SPD information ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39352/4/src/lib/spd_bin.c File src/lib/spd_bin.c:
PS4:
I think this file is getting messier with all the checks for each new DRAM type that we add support […]
Most of the attributes are extend and backward compatible. We can try this on DDR5 to see it has a lot different or not.
https://review.coreboot.org/c/coreboot/+/39352/4/src/lib/spd_bin.c@109 PS4, Line 109: is_memory_type_ddr4(dram_type) ? DDR4_ORGANIZATION : : DDR3_ORGANIZATION;
This is not handled correctly for both LPDDR3 and LPDDR4.
I will add it :)
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39352 )
Change subject: lib/spd_bin: Extend LPDDR4 SPD information ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39352/4/src/include/spd_bin.h File src/include/spd_bin.h:
https://review.coreboot.org/c/coreboot/+/39352/4/src/include/spd_bin.h@31 PS4, Line 31: _JEDEC
Do we need the suffix JEDEC? It was added to LPDDR3 because we had two types of SPDs one with Intel- […]
I will clean up the LPDDR3 and LPDDR4 with new CL.
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Angel Pons, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39352
to look at the new patch set (#5).
Change subject: lib/spd_bin: Extend LPDDR4 SPD information ......................................................................
lib/spd_bin: Extend LPDDR4 SPD information
Follow JEDEC 21-C to extend LPDDR4 SPD information.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I68c9782c543afab4423296fa7ac1c078db5649c3 --- M src/include/spd_bin.h M src/lib/spd_bin.c 2 files changed, 8 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/39352/5
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39352 )
Change subject: lib/spd_bin: Extend LPDDR4 SPD information ......................................................................
Patch Set 5:
(6 comments)
https://review.coreboot.org/c/coreboot/+/39352/4/src/lib/spd_bin.c File src/lib/spd_bin.c:
PS4:
Most of the attributes are extend and backward compatible. […]
Done
https://review.coreboot.org/c/coreboot/+/39352/4/src/lib/spd_bin.c@63 PS4, Line 63: SPD_DRAM_LPDDR3_JEDEC
I know this wasn't added here. […]
Done
https://review.coreboot.org/c/coreboot/+/39352/4/src/lib/spd_bin.c@67 PS4, Line 67: has
okay, thanks.
Done
https://review.coreboot.org/c/coreboot/+/39352/4/src/lib/spd_bin.c@109 PS4, Line 109: is_memory_type_ddr4(dram_type) ? DDR4_ORGANIZATION : : DDR3_ORGANIZATION;
I will add it :)
Done
https://review.coreboot.org/c/coreboot/+/39352/4/src/lib/spd_bin.c@120 PS4, Line 120: is_memory_type_ddr4(dram_type) ? DDR4_ORGANIZATION : : DDR3_ORGANIZATION;
same here
Done
https://review.coreboot.org/c/coreboot/+/39352/4/src/lib/spd_bin.c@131 PS4, Line 131: int busw_offset = is_memory_type_ddr4(dram_type) ? DDR4_BUS_DEV_WIDTH : : DDR3_BUS_DEV_WIDTH;
and here
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39352 )
Change subject: lib/spd_bin: Extend LPDDR4 SPD information ......................................................................
Patch Set 5: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39352 )
Change subject: lib/spd_bin: Extend LPDDR4 SPD information ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39352/4/src/lib/spd_bin.c File src/lib/spd_bin.c:
PS4:
Done
Even if there are minor differences, I think we can still avoid changing the common functions every time a memory type is added. Just reduces change in logic and makes it easier to maintain as well. I agree we can take this as follow-up later.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39352 )
Change subject: lib/spd_bin: Extend LPDDR4 SPD information ......................................................................
lib/spd_bin: Extend LPDDR4 SPD information
Follow JEDEC 21-C to extend LPDDR4 SPD information.
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I68c9782c543afab4423296fa7ac1c078db5649c3 Reviewed-on: https://review.coreboot.org/c/coreboot/+/39352 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 2 files changed, 8 insertions(+), 3 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 f144b14..c78f7c3 100644 --- a/src/include/spd_bin.h +++ b/src/include/spd_bin.h @@ -28,6 +28,7 @@ #define SPD_DRAM_LPDDR3_INTEL 0xF1 #define SPD_DRAM_LPDDR3_JEDEC 0x0F #define SPD_DRAM_DDR4 0x0C +#define SPD_DRAM_LPDDR4 0x10 #define SPD_DENSITY_BANKS 4 #define SPD_ADDRESSING 5 #define DDR3_ORGANIZATION 7 @@ -40,8 +41,6 @@ #define LPDDR3_SPD_PART_LEN 18 #define DDR4_SPD_PART_OFF 329 #define DDR4_SPD_PART_LEN 20 -#define LPDDR4_SPD_PART_OFF 329 -#define LPDDR4_SPD_PART_LEN 20
struct spd_block { u8 addr_map[CONFIG_DIMM_MAX]; /* 7 bit I2C addresses */ diff --git a/src/lib/spd_bin.c b/src/lib/spd_bin.c index 25eb552..4bf77d2 100644 --- a/src/lib/spd_bin.c +++ b/src/lib/spd_bin.c @@ -45,6 +45,8 @@ return "LPDDR3"; case SPD_DRAM_DDR4: return "DDR4"; + case SPD_DRAM_LPDDR4: + return "LPDDR4"; } return "UNKNOWN"; } @@ -62,7 +64,9 @@ if (index >= ARRAY_SIZE(ddr3_banks)) return -1; return ddr3_banks[index]; + /* DDR4 and LPDDR4 has the same bank definition */ case SPD_DRAM_DDR4: + case SPD_DRAM_LPDDR4: if (index >= ARRAY_SIZE(ddr4_banks)) return -1; return ddr4_banks[index]; @@ -73,7 +77,8 @@
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 }; + static const int spd_capmb[13] = { 1, 2, 4, 8, 16, 32, 64, + 128, 48, 96, 12, 24, 72 }; int index = spd[SPD_DENSITY_BANKS] & 0xf; if (index >= ARRAY_SIZE(spd_capmb)) return -1; @@ -145,6 +150,7 @@ spd_name[LPDDR3_SPD_PART_LEN] = 0; break; case SPD_DRAM_DDR4: + case SPD_DRAM_LPDDR4: memcpy(spd_name, &spd[DDR4_SPD_PART_OFF], DDR4_SPD_PART_LEN); spd_name[DDR4_SPD_PART_LEN] = 0; break;