Change in coreboot[master]: util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards
Amanda Hwang has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48526 )
Change subject: util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards ......................................................................
util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards
Generates de-duplicated SPD files using a global memory part list provided by the mainboard in JSON format.
BUG=b:173132516
Change-Id: I4964ec28d74ab36c6b6f2e9dce6c923d1df95c84 Signed-off-by: Amanda Huang amanda_hwang@compal.corp-partner.google.com --- M util/spd_tools/lp4x/gen_spd.go 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/48526/1
diff --git a/util/spd_tools/lp4x/gen_spd.go b/util/spd_tools/lp4x/gen_spd.go index e63ca8d..94cfb8d 100644 --- a/util/spd_tools/lp4x/gen_spd.go +++ b/util/spd_tools/lp4x/gen_spd.go @@ -29,11 +29,13 @@
PlatformTGL = 0 PlatformJSL = 1 + PlatformADL = 0 )
var platformMap = map[string]int { "TGL": PlatformTGL, "JSL": PlatformJSL, + "ADL": PlatformADL, }
var currPlatform int
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48526 )
Change subject: util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48526/1/util/spd_tools/lp4x/gen_spd... File util/spd_tools/lp4x/gen_spd.go:
https://review.coreboot.org/c/coreboot/+/48526/1/util/spd_tools/lp4x/gen_spd... PS1, Line 38: PlatformADL Rather than adding another enum with same value as TGL, what do you think about reusing PlatformTGL here and adding a comment saying that ADL MRC has the exact same expectations as TGL MRC.
Another option is to update PlatformTGL to PlatformTGLADL to represent both platforms using a single enum.
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48526
to look at the new patch set (#2).
Change subject: util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards ......................................................................
util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards
Generates de-duplicated SPD files using a global memory part list provided by the mainboard in JSON format.
BUG=b:173132516
Change-Id: I4964ec28d74ab36c6b6f2e9dce6c923d1df95c84 Signed-off-by: Amanda Huang amanda_hwang@compal.corp-partner.google.com --- M util/spd_tools/lp4x/gen_spd.go 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/48526/2
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48526
to look at the new patch set (#3).
Change subject: util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards ......................................................................
util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards
Generates de-duplicated SPD files using a global memory part list provided by the mainboard in JSON format.
BUG=b:173132516
Change-Id: I4964ec28d74ab36c6b6f2e9dce6c923d1df95c84 Signed-off-by: Amanda Huang amanda_hwang@compal.corp-partner.google.com --- M util/spd_tools/lp4x/gen_spd.go 1 file changed, 7 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/48526/3
Amanda Hwang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48526 )
Change subject: util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48526/1/util/spd_tools/lp4x/gen_spd... File util/spd_tools/lp4x/gen_spd.go:
https://review.coreboot.org/c/coreboot/+/48526/1/util/spd_tools/lp4x/gen_spd... PS1, Line 38: PlatformADL
Rather than adding another enum with same value as TGL, what do you think about reusing PlatformTGL […]
Hi Furquan,
Updated patch set 3 based on your suggestion, thanks.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48526 )
Change subject: util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards ......................................................................
Patch Set 3: Code-Review+1
LGTM
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48526 )
Change subject: util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards ......................................................................
Patch Set 3:
Will add mem_list_variant if we decide the memory part for brya. So far, it include in the global_lp4x_mem_parts.json,already.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48526 )
Change subject: util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards ......................................................................
Patch Set 3:
Patch Set 3:
Will add mem_list_variant if we decide the memory part for brya. So far, it include in the global_lp4x_mem_parts.json,already.
We need to generate SPD files for soc/intel/alderlake. I am thinking that we should just move the SPDs to a common place rather than in each SoC. Not for this change, but I think that will help simplify the SPD generation and management.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48526 )
Change subject: util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards ......................................................................
Patch Set 3: Code-Review+1
Let's see what Tim thinks.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48526 )
Change subject: util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
Will add mem_list_variant if we decide the memory part for brya. So far, it include in the global_lp4x_mem_parts.json,already.
We need to generate SPD files for soc/intel/alderlake. I am thinking that we should just move the SPDs to a common place rather than in each SoC. Not for this change, but I think that will help simplify the SPD generation and management.
+1,maybe 3rdparty/blobs is the good place to put into?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48526 )
Change subject: util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
Patch Set 3:
Will add mem_list_variant if we decide the memory part for brya. So far, it include in the global_lp4x_mem_parts.json,already.
We need to generate SPD files for soc/intel/alderlake. I am thinking that we should just move the SPDs to a common place rather than in each SoC. Not for this change, but I think that will help simplify the SPD generation and management.
+1,maybe 3rdparty/blobs is the good place to put into?
I don't think it is 3rdparty since it is being generated by coreboot utils. Probably src/spd/. We should think about the hierarchy since we need flexibility in some cases. lp4x |_ jsl/ |_ tgl_adl/ ddr4/
LP4x has different SPDs for different platforms, whereas DDR4 is common even across AMD and Intel platforms.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48526 )
Change subject: util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
Patch Set 3:
Patch Set 3:
Will add mem_list_variant if we decide the memory part for brya. So far, it include in the global_lp4x_mem_parts.json,already.
We need to generate SPD files for soc/intel/alderlake. I am thinking that we should just move the SPDs to a common place rather than in each SoC. Not for this change, but I think that will help simplify the SPD generation and management.
+1,maybe 3rdparty/blobs is the good place to put into?
I don't think it is 3rdparty since it is being generated by coreboot utils. Probably src/spd/. We should think about the hierarchy since we need flexibility in some cases. lp4x |_ jsl/ |_ tgl_adl/ ddr4/
LP4x has different SPDs for different platforms, whereas DDR4 is common even across AMD and Intel platforms.
Maybe we can use another way to separate it rather than platform name? If we new platform use the tgl spd, we may need to extend the folder name? tgl_adl_xxl? But for now, I think it's fine.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48526 )
Change subject: util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
Patch Set 3:
Patch Set 3:
Patch Set 3:
Will add mem_list_variant if we decide the memory part for brya. So far, it include in the global_lp4x_mem_parts.json,already.
We need to generate SPD files for soc/intel/alderlake. I am thinking that we should just move the SPDs to a common place rather than in each SoC. Not for this change, but I think that will help simplify the SPD generation and management.
+1,maybe 3rdparty/blobs is the good place to put into?
I don't think it is 3rdparty since it is being generated by coreboot utils. Probably src/spd/. We should think about the hierarchy since we need flexibility in some cases. lp4x |_ jsl/ |_ tgl_adl/ ddr4/
LP4x has different SPDs for different platforms, whereas DDR4 is common even across AMD and Intel platforms.
Maybe we can use another way to separate it rather than platform name? If we new platform use the tgl spd, we may need to extend the folder name? tgl_adl_xxl? But for now, I think it's fine.
Yeah, that troubles me too. We can use "default" but then it becomes default for what? We can use default_intel/ jsl/
But, it is difficult to predict until we have more platforms that use it.
How about this: create a symlink for each platform that shares the directory -
lp4x |_ set1/ |_ set2/ |_ jsl --> set1 |_ tgl --> set2 |_ adl --> set3
That way mainboards can refer to src/spd/lp4x/adl and it will use the correct set.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48526 )
Change subject: util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
Patch Set 3:
Patch Set 3:
Patch Set 3:
Patch Set 3:
Will add mem_list_variant if we decide the memory part for brya. So far, it include in the global_lp4x_mem_parts.json,already.
We need to generate SPD files for soc/intel/alderlake. I am thinking that we should just move the SPDs to a common place rather than in each SoC. Not for this change, but I think that will help simplify the SPD generation and management.
+1,maybe 3rdparty/blobs is the good place to put into?
I don't think it is 3rdparty since it is being generated by coreboot utils. Probably src/spd/. We should think about the hierarchy since we need flexibility in some cases. lp4x |_ jsl/ |_ tgl_adl/ ddr4/
LP4x has different SPDs for different platforms, whereas DDR4 is common even across AMD and Intel platforms.
Maybe we can use another way to separate it rather than platform name? If we new platform use the tgl spd, we may need to extend the folder name? tgl_adl_xxl? But for now, I think it's fine.
Yeah, that troubles me too. We can use "default" but then it becomes default for what? We can use default_intel/ jsl/
But, it is difficult to predict until we have more platforms that use it.
How about this: create a symlink for each platform that shares the directory -
lp4x |_ set1/ |_ set2/ |_ jsl --> set1 |_ tgl --> set2 |_ adl --> set3
That way mainboards can refer to src/spd/lp4x/adl and it will use the correct set.
Symbol link is the good way and clear 👍 Agree!
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48526 )
Change subject: util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards ......................................................................
Patch Set 3: Code-Review+1
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48526 )
Change subject: util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards ......................................................................
Patch Set 3: Code-Review+2
ohhh no you should be very frightened of me giving a +2 go anything written in Go, I've never written a line of it before 😋 I'll have to take some time to look over this codebase 😊
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48526 )
Change subject: util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards ......................................................................
Patch Set 3:
Patch Set 3: Code-Review+2
ohhh no you should be very frightened of me giving a +2 go anything written in Go, I've never written a line of it before 😋 I'll have to take some time to look over this codebase 😊
hahah.. i wanted to make sure you got a chance to look at this (in case you had other ideas of handling this :))
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48526 )
Change subject: util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3: Code-Review+2
ohhh no you should be very frightened of me giving a +2 go anything written in Go, I've never written a line of it before 😋 I'll have to take some time to look over this codebase 😊
hahah.. i wanted to make sure you got a chance to look at this (in case you had other ideas of handling this :))
Just hoping we don't mysteriously run into compatibility issues between the two later... 💔
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48526 )
Change subject: util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
Patch Set 3: Code-Review+2
ohhh no you should be very frightened of me giving a +2 go anything written in Go, I've never written a line of it before 😋 I'll have to take some time to look over this codebase 😊
hahah.. i wanted to make sure you got a chance to look at this (in case you had other ideas of handling this :))
Just hoping we don't mysteriously run into compatibility issues between the two later... 💔
yeah. We need to get that document updated from Balaji to ensure the MRC team has signed off on it.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48526 )
Change subject: util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48526/1/util/spd_tools/lp4x/gen_spd... File util/spd_tools/lp4x/gen_spd.go:
https://review.coreboot.org/c/coreboot/+/48526/1/util/spd_tools/lp4x/gen_spd... PS1, Line 38: PlatformADL
Hi Furquan, […]
Done
Tim Wawrzynczak has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48526 )
Change subject: util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards ......................................................................
util: Modify LPDDR4 spd_tools to generate SPDs for ADL boards
Generates de-duplicated SPD files using a global memory part list provided by the mainboard in JSON format.
BUG=b:173132516
Change-Id: I4964ec28d74ab36c6b6f2e9dce6c923d1df95c84 Signed-off-by: Amanda Huang amanda_hwang@compal.corp-partner.google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/48526 Reviewed-by: EricR Lai ericr_lai@compal.corp-partner.google.com Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Nick Vaccaro nvaccaro@google.com Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M util/spd_tools/lp4x/gen_spd.go 1 file changed, 7 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, but someone else must approve Nick Vaccaro: Looks good to me, but someone else must approve EricR Lai: Looks good to me, but someone else must approve Tim Wawrzynczak: Looks good to me, approved
diff --git a/util/spd_tools/lp4x/gen_spd.go b/util/spd_tools/lp4x/gen_spd.go index e63ca8d..2575b5d 100644 --- a/util/spd_tools/lp4x/gen_spd.go +++ b/util/spd_tools/lp4x/gen_spd.go @@ -27,13 +27,14 @@ const ( SPDManifestFileName = "lp4x_spd_manifest.generated.txt"
- PlatformTGL = 0 + PlatformTGLADL = 0 PlatformJSL = 1 )
var platformMap = map[string]int { - "TGL": PlatformTGL, + "TGL": PlatformTGLADL, "JSL": PlatformJSL, + "ADL": PlatformTGLADL, }
var currPlatform int @@ -210,7 +211,7 @@
/* Returns density to encode as per Intel MRC expectations. */ func getMRCDensity(memAttribs *memAttributes) int { - if currPlatform == PlatformTGL { + if currPlatform == PlatformTGLADL { /* * Intel MRC on TGL expects density per logical channel to be encoded in * SPDIndexDensityBanks. Logical channel on TGL is an x16 channel. @@ -270,7 +271,7 @@
func encodeDiesPerPackage(memAttribs *memAttributes) byte { var dies int = 0 - if currPlatform == PlatformTGL { + if currPlatform == PlatformTGLADL { /* Intel MRC expects logical dies to be encoded for TGL. */ dies = memAttribs.ChannelsPerDie * memAttribs.RanksPerChannel * memAttribs.BitWidthPerChannel / 16 } else if currPlatform == PlatformJSL { @@ -330,7 +331,7 @@ )
func encodeBusWidth(memAttribs *memAttributes) byte { - if currPlatform == PlatformTGL { + if currPlatform == PlatformTGLADL { return SPDValueBusWidthTGL } else if currPlatform == PlatformJSL { return SPDValueBusWidthJSL @@ -895,7 +896,7 @@ }
func normalizeMemoryAttributes(memAttribs *memAttributes) { - if currPlatform == PlatformTGL { + if currPlatform == PlatformTGLADL { /* * TGL does not really use physical organization of dies per package when * generating the SPD. So, set it to 0 here so that deduplication ignores
-
Amanda Hwang (Code Review) -
EricR Lai (Code Review)
-
Furquan Shaikh (Code Review)
-
Nick Vaccaro (Code Review)
-
Tim Wawrzynczak (Code Review)