Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44815 )
Change subject: util/ifdtool: Identify chipset without platform name ......................................................................
util/ifdtool: Identify chipset without platform name
Able to uniquely identify the chipset without specifying the platform specific quirks (adl/cnl/icl/jsl/tgl etc.).
BUG=b:153888802 TEST=Able to dump FD contains correctly without specifying platform quirks on Hatch Platform.
ifdtool -d coreboot.rom
Without this CL : ICH Revision: 100 series Sunrise Point
With this CL : ICH Revision: 300 series Cannon Point/ 400 series Ice Point
Signed-off-by: Subrata Banik subrata.banik@intel.com Change-Id: I83763adb721e069343b19a10e503975ffa6abb24 --- M util/ifdtool/ifdtool.c M util/ifdtool/ifdtool.h 2 files changed, 42 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/44815/1
diff --git a/util/ifdtool/ifdtool.c b/util/ifdtool/ifdtool.c index e096864..1880900 100644 --- a/util/ifdtool/ifdtool.c +++ b/util/ifdtool/ifdtool.c @@ -59,6 +59,7 @@ "ICH8", "ICH9", "ICH10", + "Unknown PCH", "5 series Ibex Peak", "6 series Cougar Point", "7 series Panther Point", @@ -68,7 +69,10 @@ "8 series Wellsburg", "9 series Wildcat Point", "9 series Wildcat Point LP", - "100 series Sunrise Point", + "Gemini Lake: N5xxx, J5xxx, N4xxx, J4xxx", + "100/200 series Sunrise Point", + "300 series Cannon Point/ 400 series Ice Point", + "500 series Tiger Point", "C620 series Lewisburg", NULL }; @@ -161,14 +165,42 @@ return PTR_IN_RANGE(fmsba, image, size) ? fmsba : NULL; }
+static enum ich_chipset guess_ifd_2_chipset(const fpsba_t *fpsba) +{ + uint32_t pchstrp_22 = fpsba->pchstrp[22]; + uint32_t pchstrp_23 = fpsba->pchstrp[23]; + + /* Offset 0x5B is the last PCH descriptor record */ + if (pchstrp_23 == 0xFFFFFFFF) + return CHIPSET_N_J_SERIES; + + /* Offset 0x58 is PCH descriptor record is reserved */ + if (pchstrp_22 == 0x0) + return CHIPSET_300_400_SERIES_CANNON_ICE_POINT; + + /* Offset 0x58 bit [2:0] is reserved 0x4 and 0x5a bit [7:0] is reserved 0x58 */ + if (((pchstrp_22 & 0x07) == 0x4) && + ((pchstrp_22 & 0xFF0000) >> 16 == 0x58)) + return CHIPSET_500_SERIES_TIGER_POINT; + + return CHIPSET_PCH_UNKNOWN; +} + /* port from flashrom */ -static enum ich_chipset guess_ich_chipset(const fdbar_t *fdb) +static enum ich_chipset guess_ich_chipset(const fdbar_t *fdb, const fpsba_t *fpsba) { uint32_t iccriba = (fdb->flmap2 >> 16) & 0xff; uint32_t msl = (fdb->flmap2 >> 8) & 0xff; uint32_t isl = (fdb->flmap1 >> 24); uint32_t nm = (fdb->flmap1 >> 8) & 0x7; + int temp_chipset;
+ /* Check for IFD2 chipset type */ + temp_chipset = guess_ifd_2_chipset(fpsba); + if (temp_chipset != CHIPSET_PCH_UNKNOWN) + return temp_chipset; + + /* Rest for IFD1 chipset type */ if (iccriba == 0x00) { if (msl == 0 && isl <= 2) return CHIPSET_ICH8; @@ -192,7 +224,7 @@ } else if (nm == 6) { return CHIPSET_C620_SERIES_LEWISBURG; } else { - return CHIPSET_100_SERIES_SUNRISE_POINT; + return CHIPSET_100_200_SERIES_SUNRISE_POINT; } }
@@ -232,10 +264,11 @@ int read_freq; const fcba_t *fcba = find_fcba(image, size); const fdbar_t *fdb = find_fd(image, size); + const fpsba_t *fpsba = find_fpsba(image, size); if (!fcba || !fdb) exit(EXIT_FAILURE);
- chipset = guess_ich_chipset(fdb); + chipset = guess_ich_chipset(fdb, fpsba); /* TODO: port ifd_version and max_regions * against guess_ich_chipset() */ diff --git a/util/ifdtool/ifdtool.h b/util/ifdtool/ifdtool.h index 840d7fe..a113d29 100644 --- a/util/ifdtool/ifdtool.h +++ b/util/ifdtool/ifdtool.h @@ -23,6 +23,7 @@ CHIPSET_ICH8, CHIPSET_ICH9, CHIPSET_ICH10, + CHIPSET_PCH_UNKNOWN, CHIPSET_5_SERIES_IBEX_PEAK, CHIPSET_6_SERIES_COUGAR_POINT, CHIPSET_7_SERIES_PANTHER_POINT, @@ -34,9 +35,10 @@ CHIPSET_8_SERIES_WELLSBURG, CHIPSET_9_SERIES_WILDCAT_POINT, CHIPSET_9_SERIES_WILDCAT_POINT_LP, - CHIPSET_100_SERIES_SUNRISE_POINT, /* also 6th/7th gen Core i/o (LP) - * variants - */ + CHIPSET_N_J_SERIES, /* Gemini Lake: N5xxx, J5xxx, N4xxx, J4xxx */ + CHIPSET_100_200_SERIES_SUNRISE_POINT, /* 6th-7th gen Core i/o (LP) variants */ + CHIPSET_300_400_SERIES_CANNON_ICE_POINT, /* 8th-10th gen Core i/o (LP) variants */ + CHIPSET_500_SERIES_TIGER_POINT, /* 11th gen Core i/o (LP) variants onwards */ CHIPSET_C620_SERIES_LEWISBURG, };
Hello build bot (Jenkins), Stefan Reinauer,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44815
to look at the new patch set (#2).
Change subject: util/ifdtool: Identify chipset without platform name ......................................................................
util/ifdtool: Identify chipset without platform name
Able to uniquely identify the chipset without specifying the platform specific quirks (adl/cnl/icl/jsl/tgl etc.).
BUG=b:153888802 TEST=Able to dump FD contains correctly without specifying platform quirks on Hatch Platform.
ifdtool -d coreboot.rom
Without this CL : ICH Revision: 100 series Sunrise Point
With this CL : ICH Revision: 300 series Cannon Point/ 400 series Ice Point
Signed-off-by: Subrata Banik subrata.banik@intel.com Change-Id: I83763adb721e069343b19a10e503975ffa6abb24 --- M util/ifdtool/ifdtool.c M util/ifdtool/ifdtool.h 2 files changed, 42 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/44815/2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44815 )
Change subject: util/ifdtool: Identify chipset without platform name ......................................................................
Patch Set 3: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44815 )
Change subject: util/ifdtool: Identify chipset without platform name ......................................................................
util/ifdtool: Identify chipset without platform name
Able to uniquely identify the chipset without specifying the platform specific quirks (adl/cnl/icl/jsl/tgl etc.).
BUG=b:153888802 TEST=Able to dump FD contains correctly without specifying platform quirks on Hatch Platform.
ifdtool -d coreboot.rom
Without this CL : ICH Revision: 100 series Sunrise Point
With this CL : ICH Revision: 300 series Cannon Point/ 400 series Ice Point
Signed-off-by: Subrata Banik subrata.banik@intel.com Change-Id: I83763adb721e069343b19a10e503975ffa6abb24 Reviewed-on: https://review.coreboot.org/c/coreboot/+/44815 Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M util/ifdtool/ifdtool.c M util/ifdtool/ifdtool.h 2 files changed, 42 insertions(+), 7 deletions(-)
Approvals: build bot (Jenkins): Verified Tim Wawrzynczak: Looks good to me, approved
diff --git a/util/ifdtool/ifdtool.c b/util/ifdtool/ifdtool.c index 68e5b7b..2388ebc 100644 --- a/util/ifdtool/ifdtool.c +++ b/util/ifdtool/ifdtool.c @@ -59,6 +59,7 @@ "ICH8", "ICH9", "ICH10", + "Unknown PCH", "5 series Ibex Peak", "6 series Cougar Point", "7 series Panther Point", @@ -68,7 +69,10 @@ "8 series Wellsburg", "9 series Wildcat Point", "9 series Wildcat Point LP", - "100 series Sunrise Point", + "Gemini Lake: N5xxx, J5xxx, N4xxx, J4xxx", + "100/200 series Sunrise Point", + "300 series Cannon Point/ 400 series Ice Point", + "500 series Tiger Point", "C620 series Lewisburg", NULL }; @@ -161,14 +165,42 @@ return PTR_IN_RANGE(fmsba, image, size) ? fmsba : NULL; }
+static enum ich_chipset guess_ifd_2_chipset(const fpsba_t *fpsba) +{ + uint32_t pchstrp_22 = fpsba->pchstrp[22]; + uint32_t pchstrp_23 = fpsba->pchstrp[23]; + + /* Offset 0x5B is the last PCH descriptor record */ + if (pchstrp_23 == 0xFFFFFFFF) + return CHIPSET_N_J_SERIES; + + /* Offset 0x58 is PCH descriptor record is reserved */ + if (pchstrp_22 == 0x0) + return CHIPSET_300_400_SERIES_CANNON_ICE_POINT; + + /* Offset 0x58 bit [2:0] is reserved 0x4 and 0x5a bit [7:0] is reserved 0x58 */ + if (((pchstrp_22 & 0x07) == 0x4) && + ((pchstrp_22 & 0xFF0000) >> 16 == 0x58)) + return CHIPSET_500_SERIES_TIGER_POINT; + + return CHIPSET_PCH_UNKNOWN; +} + /* port from flashrom */ -static enum ich_chipset guess_ich_chipset(const fdbar_t *fdb) +static enum ich_chipset guess_ich_chipset(const fdbar_t *fdb, const fpsba_t *fpsba) { uint32_t iccriba = (fdb->flmap2 >> 16) & 0xff; uint32_t msl = (fdb->flmap2 >> 8) & 0xff; uint32_t isl = (fdb->flmap1 >> 24); uint32_t nm = (fdb->flmap1 >> 8) & 0x7; + int temp_chipset;
+ /* Check for IFD2 chipset type */ + temp_chipset = guess_ifd_2_chipset(fpsba); + if (temp_chipset != CHIPSET_PCH_UNKNOWN) + return temp_chipset; + + /* Rest for IFD1 chipset type */ if (iccriba == 0x00) { if (msl == 0 && isl <= 2) return CHIPSET_ICH8; @@ -192,7 +224,7 @@ } else if (nm == 6) { return CHIPSET_C620_SERIES_LEWISBURG; } else { - return CHIPSET_100_SERIES_SUNRISE_POINT; + return CHIPSET_100_200_SERIES_SUNRISE_POINT; } }
@@ -232,10 +264,11 @@ int read_freq; const fcba_t *fcba = find_fcba(image, size); const fdbar_t *fdb = find_fd(image, size); + const fpsba_t *fpsba = find_fpsba(image, size); if (!fcba || !fdb) exit(EXIT_FAILURE);
- chipset = guess_ich_chipset(fdb); + chipset = guess_ich_chipset(fdb, fpsba); /* TODO: port ifd_version and max_regions * against guess_ich_chipset() */ diff --git a/util/ifdtool/ifdtool.h b/util/ifdtool/ifdtool.h index 840d7fe..a113d29 100644 --- a/util/ifdtool/ifdtool.h +++ b/util/ifdtool/ifdtool.h @@ -23,6 +23,7 @@ CHIPSET_ICH8, CHIPSET_ICH9, CHIPSET_ICH10, + CHIPSET_PCH_UNKNOWN, CHIPSET_5_SERIES_IBEX_PEAK, CHIPSET_6_SERIES_COUGAR_POINT, CHIPSET_7_SERIES_PANTHER_POINT, @@ -34,9 +35,10 @@ CHIPSET_8_SERIES_WELLSBURG, CHIPSET_9_SERIES_WILDCAT_POINT, CHIPSET_9_SERIES_WILDCAT_POINT_LP, - CHIPSET_100_SERIES_SUNRISE_POINT, /* also 6th/7th gen Core i/o (LP) - * variants - */ + CHIPSET_N_J_SERIES, /* Gemini Lake: N5xxx, J5xxx, N4xxx, J4xxx */ + CHIPSET_100_200_SERIES_SUNRISE_POINT, /* 6th-7th gen Core i/o (LP) variants */ + CHIPSET_300_400_SERIES_CANNON_ICE_POINT, /* 8th-10th gen Core i/o (LP) variants */ + CHIPSET_500_SERIES_TIGER_POINT, /* 11th gen Core i/o (LP) variants onwards */ CHIPSET_C620_SERIES_LEWISBURG, };
Attention is currently required from: Subrata Banik. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44815 )
Change subject: util/ifdtool: Identify chipset without platform name ......................................................................
Patch Set 5:
(1 comment)
File util/ifdtool/ifdtool.c:
https://review.coreboot.org/c/coreboot/+/44815/comment/a0513233_0c02a204 PS5, Line 199: temp_chipset = guess_ifd_2_chipset(fpsba); This assumes that the IFD is IFD2. A Lynx Point IFD is now wrongly detected as being a Gemini Lake IFD.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44815 )
Change subject: util/ifdtool: Identify chipset without platform name ......................................................................
Patch Set 5:
(1 comment)
File util/ifdtool/ifdtool.c:
https://review.coreboot.org/c/coreboot/+/44815/comment/931d398f_d704f3a7 PS5, Line 199: temp_chipset = guess_ifd_2_chipset(fpsba);
This assumes that the IFD is IFD2. […]
oops 😞 mostly because of https://review.coreboot.org/c/coreboot/+/44815/5/util/ifdtool/ifdtool.c#173 line.
Can you please check if is_platform_ifd_2() check can help prior calling guess_ifd_2_chipset?
Attention is currently required from: Subrata Banik. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44815 )
Change subject: util/ifdtool: Identify chipset without platform name ......................................................................
Patch Set 5:
(1 comment)
File util/ifdtool/ifdtool.c:
https://review.coreboot.org/c/coreboot/+/44815/comment/4a52b3d1_b611c722 PS5, Line 199: temp_chipset = guess_ifd_2_chipset(fpsba);
oops 😞 […]
It doesn't help, because `platform` isn't set.
Attention is currently required from: Subrata Banik. Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44815 )
Change subject: util/ifdtool: Identify chipset without platform name ......................................................................
Patch Set 5:
(2 comments)
File util/ifdtool/ifdtool.c:
https://review.coreboot.org/c/coreboot/+/44815/comment/c0c7f954_258fd352 PS5, Line 62: "Unknown PCH", What are these?
https://review.coreboot.org/c/coreboot/+/44815/comment/55bc1591_ae2e8345 PS5, Line 168: guess_ifd_2_chipset Is this a good time to get rid of the ifd 1 vs 2 distinction? It was pretty arbitrary and there are other platforms in between (ifd 1.5 if you will).
It would be interesting to learn how Intel is doing the versioning / distinction here for their own tools
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44815 )
Change subject: util/ifdtool: Identify chipset without platform name ......................................................................
Patch Set 5:
(1 comment)
File util/ifdtool/ifdtool.c:
https://review.coreboot.org/c/coreboot/+/44815/comment/95dfa1b4_13fa7d25 PS5, Line 168: guess_ifd_2_chipset
Is this a good time to get rid of the ifd 1 vs 2 distinction? It was pretty arbitrary and there are other platforms in between (ifd 1.5 if you will)
.
Agree with you, typically CNL onwards its very much IFWI 1.5 layout. We had some thought earlier to pass information from SoC Kconfig about type of IFWI layout rather modify IFD for each new SoC code. Finally as such check to make sure it also could able to cover newer chipsets.
It would be interesting to learn how Intel is doing the versioning / distinction here for their own tools
Honestly there is no common version control for the descriptor otherwise it would have easier to check only one parameter to make such decision.
I will try to dig this problem sometime next week and see if we have some more W/A to apply to fix this problem with Lynxpoint
Attention is currently required from: Subrata Banik. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44815 )
Change subject: util/ifdtool: Identify chipset without platform name ......................................................................
Patch Set 5:
(1 comment)
File util/ifdtool/ifdtool.c:
https://review.coreboot.org/c/coreboot/+/44815/comment/87b5e9ed_289764e5 PS5, Line 168: guess_ifd_2_chipset
Is this a good time to get rid of the ifd 1 vs 2 distinction? It was pretty arbitrary and there ar […]
Something that would help is to know the *length* of the soft strap lists. They usually change across generations and can be used to differentiate between IFDs.
Attention is currently required from: Subrata Banik. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44815 )
Change subject: util/ifdtool: Identify chipset without platform name ......................................................................
Patch Set 5:
(1 comment)
File util/ifdtool/ifdtool.c:
https://review.coreboot.org/c/coreboot/+/44815/comment/a0486ee0_c932fbce PS5, Line 168: guess_ifd_2_chipset
Something that would help is to know the *length* of the soft strap lists. […]
Can you please take a look into CB:54305, used the same logic to identify the chipsets.
if you can test out Lynx Point with that CL?
-
Angel Pons (Code Review) -
Patrick Georgi (Code Review)
-
Stefan Reinauer (Code Review)
-
Subrata Banik (Code Review)
-
Tim Wawrzynczak (Code Review)