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
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/55403 )
Change subject: lspcon: restart MPU on programmer shutdown
......................................................................
lspcon: restart MPU on programmer shutdown
Programmer initialization stops the on-chip MPU, and it was never
restarted. Leaving it stopped seems to prevent some display detection
from working, so implement restarting the MPU on programmer shutdown.
BUG=b:190359231
TEST=display hotplug works reliably after device communication
Change-Id: I66cd68f8f6905a2bfaf5b085bf08dcb218f42855
Reviewed-on: https://review.coreboot.org/c/flashrom/+/55403
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Angel Pons <th3fanbus(a)gmail.com>
Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org>
Reviewed-by: Sam McNally <sammc(a)google.com>
---
M lspcon_i2c_spi.c
1 file changed, 9 insertions(+), 6 deletions(-)
Approvals:
build bot (Jenkins): Verified
Angel Pons: Looks good to me, approved
Edward O'Callaghan: Looks good to me, approved
Sam McNally: Looks good to me, approved
diff --git a/lspcon_i2c_spi.c b/lspcon_i2c_spi.c
index 9f3bb9a..b590f1d 100644
--- a/lspcon_i2c_spi.c
+++ b/lspcon_i2c_spi.c
@@ -318,11 +318,13 @@
return ret;
}
-static int lspcon_i2c_spi_reset_mpu_stop(int fd)
+static int lspcon_i2c_spi_set_mpu_active(int fd, int running)
{
int ret = 0;
- ret |= lspcon_i2c_spi_write_register(fd, MPU, 0xc0); // cmd mode
- ret |= lspcon_i2c_spi_write_register(fd, MPU, 0x40); // stop mcu
+ // Cmd mode
+ ret |= lspcon_i2c_spi_write_register(fd, MPU, 0xc0);
+ // Stop or release MPU
+ ret |= lspcon_i2c_spi_write_register(fd, MPU, running ? 0 : 0x40);
return ret;
}
@@ -418,15 +420,16 @@
.write_aai = lspcon_i2c_spi_write_aai,
};
-/* TODO: MPU still stopped at this point, probably need to reset it. */
static int lspcon_i2c_spi_shutdown(void *data)
{
int ret = 0;
struct lspcon_i2c_spi_data *lspcon_data =
(struct lspcon_i2c_spi_data *)data;
int fd = lspcon_data->fd;
+
ret |= lspcon_i2c_spi_enable_write_protection(fd);
ret |= lspcon_i2c_spi_toggle_register_protection(fd, 0);
+ ret |= lspcon_i2c_spi_set_mpu_active(fd, 1);
i2c_close(fd);
free(data);
@@ -439,9 +442,9 @@
if (fd < 0)
return fd;
- int ret = lspcon_i2c_spi_reset_mpu_stop(fd);
+ int ret = lspcon_i2c_spi_set_mpu_active(fd, 0);
if (ret) {
- msg_perr("%s: call to reset_mpu_stop failed.\n", __func__);
+ msg_perr("%s: call to set_mpu_active failed.\n", __func__);
i2c_close(fd);
return ret;
}
--
To view, visit https://review.coreboot.org/c/flashrom/+/55403
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I66cd68f8f6905a2bfaf5b085bf08dcb218f42855
Gerrit-Change-Number: 55403
Gerrit-PatchSet: 3
Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: merged
Attention is currently required from: Peter Marheine.
Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55403 )
Change subject: lspcon: restart MPU on programmer shutdown
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/55403
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I66cd68f8f6905a2bfaf5b085bf08dcb218f42855
Gerrit-Change-Number: 55403
Gerrit-PatchSet: 2
Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Thu, 17 Jun 2021 00:28:29 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Sam McNally, Peter Marheine.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55403 )
Change subject: lspcon: restart MPU on programmer shutdown
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/55403
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I66cd68f8f6905a2bfaf5b085bf08dcb218f42855
Gerrit-Change-Number: 55403
Gerrit-PatchSet: 2
Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Thu, 17 Jun 2021 00:16:54 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
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/29983d9f_90cd1673
PS1, Line 955: CHIPSET_300_SERIES_CANNON_POINT
> It will be confusing, because 500 series is the Rocket Lake PCH and Tiger Point, well... […]
Intel refers to it as TGP (also for Rocket Lake), so Tiger Point seems
likely. It wouldn't be the first time they re-use code names.
CHIPSET_500_SERIES_TIGER_POINT
would be unambiguous and fits our naming scheme.
These if's are getting pretty weird and it seems like much luck that
FMSBA (or what is stored at that point now) is not `< 0x30`. Otherwise
we'd run into the other block above. However, there is a more distinctive
change: FLUMAP1[31:15] is not 0 anymore since 300 series. Checking for
this (FLUMAP >> 16 == 0) in line 948 could make further checks easier,
I guess. I've also noticed that a check for 400 series is missing
(probably not easy to identify).
--
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: Wed, 16 Jun 2021 21:17:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment