Attention is currently required from: Nico Huber, Anastasia Klimchuk.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54712 )
Change subject: sb600spi.c: Move sb600_spibar into spi data instead of being global
......................................................................
Patch Set 1:
(1 comment)
File sb600spi.c:
https://review.coreboot.org/c/flashrom/+/54712/comment/24a9d998_c2a0c02f
PS1, Line 610: free(data);
> rphysmap() is rather special. Actually many functions with an `r` prefix […]
Oh, I see now.
--
To view, visit https://review.coreboot.org/c/flashrom/+/54712
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ifaad0f0a2c0e956029d2df18ddcfd092515ca3c0
Gerrit-Change-Number: 54712
Gerrit-PatchSet: 1
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: Thu, 20 May 2021 09:58:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
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: Anastasia Klimchuk.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54712 )
Change subject: sb600spi.c: Move sb600_spibar into spi data instead of being global
......................................................................
Patch Set 1:
(1 comment)
File sb600spi.c:
https://review.coreboot.org/c/flashrom/+/54712/comment/92a98b88_e8a9802d
PS1, Line 610: free(data);
> `sb600_spibar` is not a pointer to dynamically-allocated memory, so it must not be free()'d. […]
rphysmap() is rather special. Actually many functions with an `r` prefix
in flashrom. AIUI, the `r` means that the effects of a successful call
shall be reversed once flashrom exits. There's some automatism that
registers shutdown functions for this.
In the long run, we should move the global state of shutdown registrations
into some kind of library context. We could also move more things into the
programmer-local shutdown functions, but that's independent. For now, there
is nothing that needs to be done for `r` function calls.
--
To view, visit https://review.coreboot.org/c/flashrom/+/54712
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ifaad0f0a2c0e956029d2df18ddcfd092515ca3c0
Gerrit-Change-Number: 54712
Gerrit-PatchSet: 1
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: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Thu, 20 May 2021 09:38:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
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, Anastasia Klimchuk.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54714 )
Change subject: sb600spi.c: Drop sb600_ prefix for spi data struct member
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/54714/comment/83b33a4a_9265b6fc
PS1, Line 9: sb600spi_
nit, pedantic: Strictly speaking, this prefix is only used in the name of the struct *type*. The variables of this type use `sb600_` as prefix.
--
To view, visit https://review.coreboot.org/c/flashrom/+/54714
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I001ae2044453d1bc205fa253ffb773ed993f57f8
Gerrit-Change-Number: 54714
Gerrit-PatchSet: 1
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: Thu, 20 May 2021 08:49:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Anastasia Klimchuk.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54712 )
Change subject: sb600spi.c: Move sb600_spibar into spi data instead of being global
......................................................................
Patch Set 1: Code-Review+2
(2 comments)
File sb600spi.c:
https://review.coreboot.org/c/flashrom/+/54712/comment/ee8e9bce_77d8eaad
PS1, Line 59: sb600_spibar
The `sb600_` prefix isn't needed. Oh, I see CB:54714 takes care of it.
https://review.coreboot.org/c/flashrom/+/54712/comment/f7dd2a91_3497be08
PS1, Line 610: free(data);
> not free'ed as its physical memory but technically it should be unmapped yes although I think everyo […]
`sb600_spibar` is not a pointer to dynamically-allocated memory, so it must not be free()'d. However, one should use physunmap() or some other function from physmap.c to properly clean up.
I'd do this in a separate commit.
--
To view, visit https://review.coreboot.org/c/flashrom/+/54712
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ifaad0f0a2c0e956029d2df18ddcfd092515ca3c0
Gerrit-Change-Number: 54712
Gerrit-PatchSet: 1
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: Thu, 20 May 2021 08:19:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Anastasia Klimchuk.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54713 )
Change subject: sb600spi.c: Make use of new register_spi_master() API
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/54713
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id555dc5e125309883a816e00afb26d7141fd870d
Gerrit-Change-Number: 54713
Gerrit-PatchSet: 1
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: Thu, 20 May 2021 08:11:06 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54042 )
Change subject: stlinkv3_spi.c: Refactor singleton states into reentrant pattern
......................................................................
Patch Set 2:
(1 comment)
File stlinkv3_spi.c:
https://review.coreboot.org/c/flashrom/+/54042/comment/c16cd3e3_4f1da503
PS1, Line 533: stlinkv3_spi_open(sck_freq_kHz, stlinkv3_handle
> I rebased on the top of CB:54190, ready for review, should look better than before. […]
(forgot to marked resolved, in the meaning this is ready for review)
--
To view, visit https://review.coreboot.org/c/flashrom/+/54042
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id044661b864b506028720ea809bc524f0640469f
Gerrit-Change-Number: 54042
Gerrit-PatchSet: 2
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: Miklós Márton <martonmiklosqdev(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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Thu, 20 May 2021 05:30:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
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: Richard Hughes, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Patrick Rudolph.
Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/49643 )
Change subject: libflashrom: Return progress state to the library user
......................................................................
Patch Set 8:
(4 comments)
Patchset:
PS4:
> Any thoughts on the splitting up idea suggested above Richard?
I think the context of adding support to programmers is useful to understand the needs of the framework for reporting progress; splitting this into two changes doesn't seem very useful.
File 82802ab.c:
https://review.coreboot.org/c/flashrom/+/49643/comment/abc60d8c_769ca106
PS8, Line 137: set_progress(flash, FLASHROM_PROGRESS_WRITE, i + 1, len);
Updating progress on every byte seems pretty costly. Does it matter? (Are the usual writes to this device small?) Perhaps set_progress itself should include a check to avoid costly too-frequent messages.
It looks like other programmers also update progress on every byte, so it probably should be addressed in set_progress (and apply to library users as well) or flashrom_progress_cb (applying only to flashrom CLI).
File cli_output.c:
https://review.coreboot.org/c/flashrom/+/49643/comment/f8c2a97c_ce7114c7
PS8, Line 85: msg_ginfo("[%s] %u%% complete", flashrom_progress_stage_to_string (stage), pc);
Would it make sense to terminate this with a \r to avoid scrolling terminals?
File libflashrom.h:
https://review.coreboot.org/c/flashrom/+/49643/comment/4128a208_2dbbc29b
PS8, Line 105: typedef void(flashrom_progress_callback)(struct flashrom_flashctx *flashctx, enum flashrom_progress_stage stage, size_t current, size_t total, void *user_data);
The progress will go 0-100 for every region being operated on, which might be surprising (but is probably still okay) when operating on multiple flash regions. Might be nice to also report the name of the current region, but I suspect that would be difficult to plumb through.
--
To view, visit https://review.coreboot.org/c/flashrom/+/49643
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7197572bb7f19e3bdb2bde855d70a0f50fd3854c
Gerrit-Change-Number: 49643
Gerrit-PatchSet: 8
Gerrit-Owner: Richard Hughes <richard(a)hughsie.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-CC: Richard Hughes <hughsient(a)gmail.com>
Gerrit-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Richard Hughes <richard(a)hughsie.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Thu, 20 May 2021 02:36:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: comment
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/54429 )
Change subject: Fix up handling of IFD chipsets
......................................................................
Fix up handling of IFD chipsets
When `CHIPSET_400_SERIES_COMET_POINT` got added, the `chipset_names`
table was not updated. Add the missing entry and reorder it to be
next to `CHIPSET_300_SERIES_CANNON_POINT` for consistency.
Change-Id: I4f4b31ecf91c432a2e82a92e274cb91ac166e635
Signed-off-by: Angel Pons <th3fanbus(a)gmail.com>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/54429
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org>
Reviewed-by: Sam McNally <sammc(a)google.com>
---
M ich_descriptors.c
M programmer.h
2 files changed, 3 insertions(+), 2 deletions(-)
Approvals:
build bot (Jenkins): Verified
Edward O'Callaghan: Looks good to me, approved
Sam McNally: Looks good to me, approved
diff --git a/ich_descriptors.c b/ich_descriptors.c
index c5f9f82..a6ac881 100644
--- a/ich_descriptors.c
+++ b/ich_descriptors.c
@@ -106,7 +106,8 @@
"5 series Ibex Peak", "6 series Cougar Point", "7 series Panther Point",
"8 series Lynx Point", "Baytrail", "8 series Lynx Point LP", "8 series Wellsburg",
"9 series Wildcat Point", "9 series Wildcat Point LP", "100 series Sunrise Point",
- "C620 series Lewisburg", "300 series Cannon Point", "Apollo Lake", "Gemini Lake",
+ "C620 series Lewisburg", "300 series Cannon Point", "400 series Comet Point",
+ "Apollo Lake", "Gemini Lake",
};
if (cs < CHIPSET_ICH8 || cs - CHIPSET_ICH8 + 1 >= ARRAY_SIZE(chipset_names))
cs = 0;
diff --git a/programmer.h b/programmer.h
index d3a7ba3..1a2e758 100644
--- a/programmer.h
+++ b/programmer.h
@@ -670,8 +670,8 @@
CHIPSET_100_SERIES_SUNRISE_POINT, /* also 6th/7th gen Core i/o (LP) variants */
CHIPSET_C620_SERIES_LEWISBURG,
CHIPSET_300_SERIES_CANNON_POINT,
- CHIPSET_APOLLO_LAKE,
CHIPSET_400_SERIES_COMET_POINT,
+ CHIPSET_APOLLO_LAKE,
CHIPSET_GEMINI_LAKE,
};
--
To view, visit https://review.coreboot.org/c/flashrom/+/54429
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I4f4b31ecf91c432a2e82a92e274cb91ac166e635
Gerrit-Change-Number: 54429
Gerrit-PatchSet: 2
Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: merged