Attention is currently required from: Nico Huber, Michał Żygowski.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56023 )
Change subject: board_enable.c: Add ME unlock function for TUXEDO laptops
......................................................................
Patch Set 1:
(2 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/56023/comment/aade2173_d0bfded7
PS1, Line 7: board_enable.c: Add ME unlock function for TUXEDO laptops
I'd appreciate if you could explain in the commit message what this "ME unlock" is about. Also missing is information on how to undo this unlocking (the code only implements unlocking).
Wild guess after a very quick glance: this code tells the EC to assert the HDA_SDO strap using a GPIO.
File board_enable.c:
https://review.coreboot.org/c/flashrom/+/56023/comment/0a6a6720_b9534deb
PS1, Line 2315: if (!arg && strcmp(arg, "yes"))
: return 0;
Hmmm, this check will call `strcmp()` only when `arg` is NULL. I don't think undefined behavior is what you wanted to do? 😄
If you want to bail out unless the param exists and equals `yes`, I would do:
char *arg = extract_programmer_param("unlockmeonly");
const bool proceed = arg && strcmp(arg, "yes") == 0;
free(arg);
if (!proceed)
return 0;
Note that `free()` is unconditionally called, even when `arg` is NULL. This is a non-issue because the C standard explicitly states the following about the `free()` function:
> If ptr is a null pointer, no action occurs.
--
To view, visit https://review.coreboot.org/c/flashrom/+/56023
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6bf1c40900aa674e3ea4f6add12dae8b73759fbb
Gerrit-Change-Number: 56023
Gerrit-PatchSet: 1
Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Comment-Date: Fri, 02 Jul 2021 09:45:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Paul Menzel.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55578 )
Change subject: Add Tiger Lake U Premium support
......................................................................
Patch Set 4:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/55578/comment/511ca77f_b02a1b59
PS3, Line 10: uknown
> unknown
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/55578
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I28f3b6fe9f8ce9e976a6808683f46b6f4ec72bdd
Gerrit-Change-Number: 55578
Gerrit-PatchSet: 4
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-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Comment-Date: Fri, 02 Jul 2021 08:24:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Michał Żygowski.
Hello build bot (Jenkins), Nico Huber, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/55578
to look at the new patch set (#4).
Change subject: Add Tiger Lake U Premium support
......................................................................
Add Tiger Lake U Premium support
Tiger Lake has very low ICCRIBA (TGL=0x11, CNL=0x34 and CML=0x34) and
detects as unknown chipset compatible with 300 series chipset. Add a
new enum CHIPSET_500_SERIES_TIGER_POINT and treat it identically to
CHIPSET_400_SERIES_COMET_POINT except the ICCRIBA in descriptor
guessing, pprint_freq and boot straps.
TEST=Flash BIOS region on Intel i5-1135G7
Signed-off-by: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Change-Id: I28f3b6fe9f8ce9e976a6808683f46b6f4ec72bdd
---
M chipset_enable.c
M ich_descriptors.c
M ichspi.c
M programmer.h
M util/ich_descriptors_tool/ich_descriptors_tool.c
5 files changed, 42 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/78/55578/4
--
To view, visit https://review.coreboot.org/c/flashrom/+/55578
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I28f3b6fe9f8ce9e976a6808683f46b6f4ec72bdd
Gerrit-Change-Number: 55578
Gerrit-PatchSet: 4
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-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Nico Huber, Anastasia Klimchuk.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54890 )
Change subject: programmer: Introduce default shutdown function
......................................................................
Patch Set 5: Code-Review+1
(5 comments)
Patchset:
PS5:
> Personally, I don't mind adding something that ends up being removed later, as long as it provides s […]
Agreed but I think a small [optional] variation as suggested inline puts the code slightly more inline with the future?
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/54890/comment/149aca10_67fea22b
PS5, Line 78: int default_shutdown(void *data)
`static int default_shutdown(void *data)`
https://review.coreboot.org/c/flashrom/+/54890/comment/687b7dc1_ab3dd9b4
PS5, Line 104: function;
`shutdown_fn[shutdown_fn_count].func = !function ? default_shutdown : function;`
File mcp6x_spi.c:
https://review.coreboot.org/c/flashrom/+/54890/comment/30eab7fa_84811336
PS5, Line 177: default_shutdown
just pass `NULL`
File programmer.h:
https://review.coreboot.org/c/flashrom/+/54890/comment/e6ce0cdd_efbd200e
PS5, Line 337: int default_shutdown(void *data);
no need.
--
To view, visit https://review.coreboot.org/c/flashrom/+/54890
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I8b20717ba549e12edffbc5d1643ff064a4f0c517
Gerrit-Change-Number: 54890
Gerrit-PatchSet: 5
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Fri, 02 Jul 2021 02:01:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Thomas Heijligen, Edward O'Callaghan, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55999 )
Change subject: layout: Turn overlap debug message into warning
......................................................................
Patch Set 1:
(1 comment)
File layout.c:
https://review.coreboot.org/c/flashrom/+/55999/comment/b8ebe0ef_40407d14
PS1, Line 269: warn
Does this message indicate an issue that has happened, or just information? Warning (at least to me) means there is a potential issue.
--
To view, visit https://review.coreboot.org/c/flashrom/+/55999
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ie969e0538d302ccd58d3fec3921265ed3621eaa5
Gerrit-Change-Number: 55999
Gerrit-PatchSet: 1
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Thu, 01 Jul 2021 23:53:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment