Attention is currently required from: Michał Żygowski, Angel Pons.
Hello Michał Żygowski, Angel Pons,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/flashrom/+/55650
to review the following change.
Change subject: [RFC] ich_descriptors: Don't base chipset detection on `freq_read`
......................................................................
[RFC] ich_descriptors: Don't base chipset detection on `freq_read`
Only warn if the `freq_read` setting looks odd but don't override
our previous guess. The `freq_read` check was taken from `ifdtool`
but seems less reliable than our own detection scheme.
Change-Id: I658d76ec2567d1d660a18d0b0ae71c744e603e8f
Signed-off-by: Nico Huber <nico.h(a)gmx.de>
---
M ich_descriptors.c
1 file changed, 3 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/50/55650/1
diff --git a/ich_descriptors.c b/ich_descriptors.c
index da49326..edc6bf4 100644
--- a/ich_descriptors.c
+++ b/ich_descriptors.c
@@ -984,7 +984,7 @@
case CHIPSET_400_SERIES_COMET_POINT:
case CHIPSET_GEMINI_LAKE:
/* `freq_read` was repurposed, so can't check on it any more. */
- return guess;
+ break;
case CHIPSET_100_SERIES_SUNRISE_POINT:
case CHIPSET_C620_SERIES_LEWISBURG:
case CHIPSET_APOLLO_LAKE:
@@ -993,19 +993,17 @@
"However, the read frequency isn't set to 17MHz (the only valid value).\n"
"Please report this message, the output of `ich_descriptors_tool` for\n"
"your descriptor and the output of `lspci -nn` to flashrom(a)flashrom.org\n\n");
- return CHIPSET_9_SERIES_WILDCAT_POINT;
}
- return guess;
+ break;
default:
if (component->modes.freq_read == 6) {
msg_pwarn("\nThe flash descriptor has the read frequency set to 17MHz. However,\n"
"it doesn't look like a Skylake/Sunrise Point compatible descriptor.\n"
"Please report this message, the output of `ich_descriptors_tool` for\n"
"your descriptor and the output of `lspci -nn` to flashrom(a)flashrom.org\n\n");
- return CHIPSET_100_SERIES_SUNRISE_POINT;
}
- return guess;
}
+ return guess;
}
/* len is the length of dump in bytes */
--
To view, visit https://review.coreboot.org/c/flashrom/+/55650
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I658d76ec2567d1d660a18d0b0ae71c744e603e8f
Gerrit-Change-Number: 55650
Gerrit-PatchSet: 1
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: newchange
Attention is currently required from: Nico Huber, Michał Żygowski.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55645 )
Change subject: ich_descriptors: Drop some unnecessary `else` after `return`
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/55645
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id739bc12832e3b441e8e7e1dcdcc4c05b260d7ad
Gerrit-Change-Number: 55645
Gerrit-PatchSet: 1
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Comment-Date: Thu, 17 Jun 2021 20:32:42 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Michał Żygowski.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55644 )
Change subject: ich_descriptors: Revise descriptor messages
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/55644/comment/4cc7b107_9aa82755
PS1, Line 9: Correct
I've seen both in Intel docs. However, I believe the IFD describes the flash more than it describes the firmware. Plus, this helps distinguish between IFD and IFWI.
--
To view, visit https://review.coreboot.org/c/flashrom/+/55644
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7f15780e03d2fa17ca6d8328275cae5af13ae424
Gerrit-Change-Number: 55644
Gerrit-PatchSet: 1
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Comment-Date: Thu, 17 Jun 2021 20:32:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Michał Żygowski, Angel Pons.
Hello Michał Żygowski, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/55647
to look at the new patch set (#2).
Change subject: ich_descriptors: Revise detection for chipsets w/ ICCRIBA
......................................................................
ich_descriptors: Revise detection for chipsets w/ ICCRIBA
Detection based on ICCRIBA and FMSBA became a little messy lately.
However, there's a new static difference: Since 300 series (Cannon
Point), there is an MDTBA field in FLUMAP1 that has always been 0
(reserved) before. Taking this into account, we can relax the checks
on ICCRIBA.
Change-Id: I587ad1abe390843d4a9e74431b6fc4b63f8ba512
Signed-off-by: Nico Huber <nico.h(a)gmx.de>
---
M ich_descriptors.c
M ich_descriptors.h
2 files changed, 26 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/47/55647/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/55647
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I587ad1abe390843d4a9e74431b6fc4b63f8ba512
Gerrit-Change-Number: 55647
Gerrit-PatchSet: 2
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Michał Żygowski, Angel Pons.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55577 )
Change subject: ich_descriptors.c: Fix PCH detection for Tiger Lake
......................................................................
Patch Set 1:
(1 comment)
File ich_descriptors.c:
https://review.coreboot.org/c/flashrom/+/55577/comment/d335c210_a4a756ae
PS1, Line 955: CHIPSET_300_SERIES_CANNON_POINT
> if it is not zero, then we have 300 series+ chipset
> right?
exactly.
The whole thing became a bit more complex, so I went ahead and
prepared things: CB:55647. Adding TGP on top should be much easier.
I'd say
if (ICCRIBA == 0x34)
return 300_SERIES
if ("CPU Soft Strap Offset from PMC Base" != 0x6c)
warn
return 500_series
I wonder if the field is really shifted by 2 bits or if it's
just Intels weird way to say the 2 least-significant bits are 0.
--
To view, visit https://review.coreboot.org/c/flashrom/+/55577
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib61d432ab4eaf00aa4eef50d2844940e73b5cad6
Gerrit-Change-Number: 55577
Gerrit-PatchSet: 1
Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Thu, 17 Jun 2021 20:26:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Michał Żygowski, Angel Pons.
Hello Michał Żygowski, Angel Pons,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/flashrom/+/55646
to review the following change.
Change subject: ich_descriptors: Refactor read_ich_descriptors_from_dump()
......................................................................
ich_descriptors: Refactor read_ich_descriptors_from_dump()
Process the "upper map" early as it doesn't depend on the descriptor
generation. This way, we can use it to guess the generation.
Change-Id: Ia2786b762ccefdce31b63397119bd89879e887ff
Signed-off-by: Nico Huber <nico.h(a)gmx.de>
---
M ich_descriptors.c
1 file changed, 17 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/46/55646/1
diff --git a/ich_descriptors.c b/ich_descriptors.c
index bf47ec0..4ae3a1b 100644
--- a/ich_descriptors.c
+++ b/ich_descriptors.c
@@ -1037,6 +1037,23 @@
desc->component.FLILL = dump[(getFCBA(&desc->content) >> 2) + 1];
desc->component.FLPB = dump[(getFCBA(&desc->content) >> 2) + 2];
+ /* upper map */
+ desc->upper.FLUMAP1 = dump[(UPPER_MAP_OFFSET >> 2) + 0];
+
+ /* VTL is 8 bits long. Quote from the Ibex Peak SPI programming guide:
+ * "Identifies the 1s based number of DWORDS contained in the VSCC
+ * Table. Each SPI component entry in the table is 2 DWORDS long." So
+ * the maximum of 255 gives us 127.5 SPI components(!?) 8 bytes each. A
+ * check ensures that the maximum offset actually accessed is available.
+ */
+ if (len < getVTBA(&desc->upper) + (desc->upper.VTL / 2 * 8))
+ return ICH_RET_OOB;
+
+ for (i = 0; i < desc->upper.VTL/2; i++) {
+ desc->upper.vscc_table[i].JID = dump[(getVTBA(&desc->upper) >> 2) + i * 2 + 0];
+ desc->upper.vscc_table[i].VSCC = dump[(getVTBA(&desc->upper) >> 2) + i * 2 + 1];
+ }
+
if (*cs == CHIPSET_ICH_UNKNOWN) {
*cs = guess_ich_chipset(&desc->content, &desc->component);
prettyprint_ich_chipset(*cs);
@@ -1056,23 +1073,6 @@
for (i = 0; i < nm; i++)
desc->master.FLMSTRs[i] = dump[(getFMBA(&desc->content) >> 2) + i];
- /* upper map */
- desc->upper.FLUMAP1 = dump[(UPPER_MAP_OFFSET >> 2) + 0];
-
- /* VTL is 8 bits long. Quote from the Ibex Peak SPI programming guide:
- * "Identifies the 1s based number of DWORDS contained in the VSCC
- * Table. Each SPI component entry in the table is 2 DWORDS long." So
- * the maximum of 255 gives us 127.5 SPI components(!?) 8 bytes each. A
- * check ensures that the maximum offset actually accessed is available.
- */
- if (len < getVTBA(&desc->upper) + (desc->upper.VTL / 2 * 8))
- return ICH_RET_OOB;
-
- for (i = 0; i < desc->upper.VTL/2; i++) {
- desc->upper.vscc_table[i].JID = dump[(getVTBA(&desc->upper) >> 2) + i * 2 + 0];
- desc->upper.vscc_table[i].VSCC = dump[(getVTBA(&desc->upper) >> 2) + i * 2 + 1];
- }
-
/* MCH/PROC (aka. North) straps */
if (len < getFMSBA(&desc->content) + desc->content.MSL * 4)
return ICH_RET_OOB;
--
To view, visit https://review.coreboot.org/c/flashrom/+/55646
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ia2786b762ccefdce31b63397119bd89879e887ff
Gerrit-Change-Number: 55646
Gerrit-PatchSet: 1
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: newchange
Attention is currently required from: Michał Żygowski, Angel Pons.
Hello Michał Żygowski, Angel Pons,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/flashrom/+/55644
to review the following change.
Change subject: ich_descriptors: Revise descriptor messages
......................................................................
ich_descriptors: Revise descriptor messages
Correct "firmware descriptor" to "flash descriptor". And also
move the check for peculiar descriptors and the message into an
inline function.
Change-Id: I7f15780e03d2fa17ca6d8328275cae5af13ae424
Signed-off-by: Nico Huber <nico.h(a)gmx.de>
---
M ich_descriptors.c
1 file changed, 13 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/44/55644/1
diff --git a/ich_descriptors.c b/ich_descriptors.c
index a6ac881..fc04a24 100644
--- a/ich_descriptors.c
+++ b/ich_descriptors.c
@@ -912,6 +912,13 @@
msg_pdbg2("\n");
}
+static inline void warn_peculiar_desc(const bool warn_if, const char *const name)
+{
+ if (!warn_if)
+ return;
+ msg_pwarn("Peculiar flash descriptor, assuming %s compatibility.\n", name);
+}
+
/*
* Guesses a minimum chipset version based on the maximum number of
* soft straps per generation.
@@ -930,11 +937,10 @@
else if (content->FLMAP2 == 0) {
if (content->ISL == 23)
return CHIPSET_GEMINI_LAKE;
- else if (content->ISL != 19)
- msg_pwarn("Peculiar firmware descriptor, assuming Apollo Lake compatibility.\n");
+ warn_peculiar_desc(content->ISL != 19, "Apollo Lake");
return CHIPSET_APOLLO_LAKE;
}
- msg_pwarn("Peculiar firmware descriptor, assuming Ibex Peak compatibility.\n");
+ warn_peculiar_desc(true, "Ibex Peak");
return CHIPSET_5_SERIES_IBEX_PEAK;
} else if (content->ICCRIBA < 0x31 && content->FMSBA < 0x30) {
if (content->MSL == 0 && content->ISL <= 17)
@@ -943,7 +949,7 @@
return CHIPSET_6_SERIES_COUGAR_POINT;
else if (content->MSL <= 1 && content->ISL <= 21)
return CHIPSET_8_SERIES_LYNX_POINT;
- msg_pwarn("Peculiar firmware descriptor, assuming Wildcat Point compatibility.\n");
+ warn_peculiar_desc(true, "Wildcat Point");
return CHIPSET_9_SERIES_WILDCAT_POINT;
} else if (content->ICCRIBA < 0x34) {
if (content->NM == 6)
@@ -956,7 +962,7 @@
else
return CHIPSET_300_SERIES_CANNON_POINT;
} else {
- msg_pwarn("Unknown firmware descriptor, assuming 300 series compatibility.\n");
+ msg_pwarn("Unknown flash descriptor, assuming 300 series compatibility.\n");
return CHIPSET_300_SERIES_CANNON_POINT;
}
}
@@ -982,7 +988,7 @@
case CHIPSET_C620_SERIES_LEWISBURG:
case CHIPSET_APOLLO_LAKE:
if (component->modes.freq_read != 6) {
- msg_pwarn("\nThe firmware descriptor looks like a Skylake/Sunrise Point descriptor.\n"
+ msg_pwarn("\nThe flash descriptor looks like a Skylake/Sunrise Point descriptor.\n"
"However, the read frequency isn't set to 17MHz (the only valid value).\n"
"Please report this message, the output of `ich_descriptors_tool` for\n"
"your descriptor and the output of `lspci -nn` to flashrom(a)flashrom.org\n\n");
@@ -991,7 +997,7 @@
return guess;
default:
if (component->modes.freq_read == 6) {
- msg_pwarn("\nThe firmware descriptor has the read frequency set to 17MHz. However,\n"
+ msg_pwarn("\nThe flash descriptor has the read frequency set to 17MHz. However,\n"
"it doesn't look like a Skylake/Sunrise Point compatible descriptor.\n"
"Please report this message, the output of `ich_descriptors_tool` for\n"
"your descriptor and the output of `lspci -nn` to flashrom(a)flashrom.org\n\n");
--
To view, visit https://review.coreboot.org/c/flashrom/+/55644
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7f15780e03d2fa17ca6d8328275cae5af13ae424
Gerrit-Change-Number: 55644
Gerrit-PatchSet: 1
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: newchange
Attention is currently required from: Nico Huber, Angel Pons.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55577 )
Change subject: ich_descriptors.c: Fix PCH detection for Tiger Lake
......................................................................
Patch Set 1:
(1 comment)
File ich_descriptors.c:
https://review.coreboot.org/c/flashrom/+/55577/comment/0a067069_c7b5b8b7
PS1, Line 955: CHIPSET_300_SERIES_CANNON_POINT
> Intel refers to it as TGP (also for Rocket Lake), so Tiger Point seems […]
FMSBA is just shifted 2 bits left in the register and occupies 10 bits, that is why it did not catch the `< 0x30` condition.
Right, FLUMAP[31:15] now contains some data. So basically what you mean is:
if it is not zero, then we have 300 series+ chipset
right?
--
To view, visit https://review.coreboot.org/c/flashrom/+/55577
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib61d432ab4eaf00aa4eef50d2844940e73b5cad6
Gerrit-Change-Number: 55577
Gerrit-PatchSet: 1
Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Thu, 17 Jun 2021 14:25:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment