Attention is currently required from: Felix Singer, Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk.
Alexander Goncharov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66373 )
Change subject: tree: provide flashrom context into programmer_delay()
......................................................................
Patch Set 8:
(14 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/66373/comment/994f54ff_d57a7a01
PS7, Line 7: tree: provide flashrom context into programmer's delay
> It's not passed to the programmers anymore
Done
https://review.coreboot.org/c/flashrom/+/66373/comment/26595fc9_3253bd69
PS7, Line 9: Add the flashrom context to the delay function declaration, which
: located in `struct programmer_entry`.
> Doesn't apply anymore. Also, there is not *that* delay function. […]
Done
https://review.coreboot.org/c/flashrom/+/66373/comment/06fefbef_79b360a8
PS7, Line 14: TOPIC=register_master_api
> Also, if you don't have a huge patch series then a topic doesn't make much sense. […]
That topic was actual as long as this patch changed programmer delay API.
As I see this patch will be a small chain in the series of patches by Edward. He uses `programmer_handle_global` topic in gerrit. I hope he will add `TOPIC=` in his next patches.
https://review.coreboot.org/c/flashrom/+/66373/comment/585b99f4_386b3b32
PS7, Line 19: Ticket: https://ticket.coreboot.org/issues/391
> Same here. […]
Not directly. That task is blocked and I can't refactor programmer code to use own structure.
I think I should delete it anyway.
File amd_imc.c:
https://review.coreboot.org/c/flashrom/+/66373/comment/34cc27e8_edbe6770
PS7, Line 63: /* flashctx is not completely ready during init, so it's not
: * needed (NULL). */
> Same here
Done
File atavia.c:
https://review.coreboot.org/c/flashrom/+/66373/comment/d330225e_2836bd40
PS7, Line 93: /* flashctx is not needed (NULL) because atavia does
: * not use it in its delay function. */
> Same here
Done
File dediprog.c:
https://review.coreboot.org/c/flashrom/+/66373/comment/014bd7af_eeacfe89
PS7, Line 319: /* flashctx not needed (NULL) because dediprog does not use it in its
: * delay function. */
> Same here
Done
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/66373/comment/f3258508_fbeee20b
PS1, Line 258: /* return or exit? */
> If flashrom continues to execute code, than it will be unexpected behavior. So for now, use exit(). […]
Ack
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/66373/comment/028c31da_41e2cc94
PS5, Line 220: void programmer_delay(const struct flashctx *flash, unsigned int usecs)
> Currently the commit message doesn't give any hint why this is necessary. […]
Abandoned.
https://review.coreboot.org/c/flashrom/+/66373/comment/f60d28da_8f57acda
PS5, Line 222: if (!usecs)
: return;
:
: if (!programmer->delay)
: return internal_delay(flash, usecs); // maybe don't pass context into internal? or pass NULL?
:
: if (!flash) {
: msg_perr("%s called but flashctx is not valid!\n"
: "Please report a bug at flashrom(a)flashrom.org\n", __func__);
: exit(1);
: }
: programmer->delay(flash, usecs);
: }
:
> leave all this diff out this patch and have it purely just about plumbing flashctx though the contro […]
I got you! Done.
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/66373/comment/6e8fafb4_f8ba90a7
PS6, Line 226: return internal_delay(flash, usecs);
> If we put CB:67393 on top of this patch then `programmer_delay()` has been largely dealt with from t […]
This patch originally did a bit more, so yeah. Thanks for the clarification!
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/66373/comment/b8f2b905_387086ce
PS7, Line 878: flashctx is not needed (NULL) because ichspi does not use
: * it in its delay function. */
> Same here
Done
File pony_spi.c:
https://review.coreboot.org/c/flashrom/+/66373/comment/26007511_72fe1242
PS7, Line 248: Also flashctx is not
: * completely ready during init anyway.
> This doesn't add much value. So please remove.
Done
File raiden_debug_spi.c:
https://review.coreboot.org/c/flashrom/+/66373/comment/f200c8e1_bf745acb
PS7, Line 1015: * flashctx not needed (NULL) because raiden_debug_spi does not
: * use it in its delay function. Also flashctx is not
: * completely ready during init anyway. */
> Same here
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/66373
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ibb0bce26ce2052853ee52158d7ba742967a9e229
Gerrit-Change-Number: 66373
Gerrit-PatchSet: 8
Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Wed, 05 Oct 2022 20:04:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk, Alexander Goncharov.
Hello Felix Singer, build bot (Jenkins), Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/66373
to look at the new patch set (#8).
Change subject: tree: provide flashrom context into programmer_delay()
......................................................................
tree: provide flashrom context into programmer_delay()
Modify the `programmer_delay` function signature to allow passing
the flashrom context. This is an important step to remove the global
state. Programmers uses internal delay can provide NULL as a context.
TOPIC=programmer_handle_global
TEST=builds
Change-Id: Ibb0bce26ce2052853ee52158d7ba742967a9e229
Signed-off-by: Alexander Goncharov <chat(a)joursoir.net>
---
M 82802ab.c
M amd_imc.c
M at45db.c
M atavia.c
M bitbang_spi.c
M cli_classic.c
M dediprog.c
M dummyflasher.c
M edi.c
M en29lv640b.c
M flashrom.c
M ichspi.c
M include/flash.h
M it87spi.c
M jedec.c
M nicintel_eeprom.c
M pony_spi.c
M raiden_debug_spi.c
M s25f.c
M spi25.c
M spi25_statusreg.c
M sst28sf040.c
M stm50.c
M w29ee011.c
M w39.c
M wbsio_spi.c
26 files changed, 126 insertions(+), 106 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/73/66373/8
--
To view, visit https://review.coreboot.org/c/flashrom/+/66373
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ibb0bce26ce2052853ee52158d7ba742967a9e229
Gerrit-Change-Number: 66373
Gerrit-PatchSet: 8
Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-MessageType: newpatchset
Attention is currently required from: Shreeyash .
Alexander Goncharov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66225 )
Change subject: stlinkv3_spi: Bug fix, Initialize uninitialized pointers
......................................................................
Patch Set 2:
(4 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/66225/comment/b13f2119_88ef031d
PS2, Line 7: Bug fix, Initialize uninitialized pointers
How about `fix false-positive compiler error`?
https://review.coreboot.org/c/flashrom/+/66225/comment/47e0cde9_29b83bb9
PS2, Line 9: on line 510
After rebasing, the line number is not correct. I suggest deleting it.
Patchset:
PS2:
Please have a look at CB:67700 (especially on unresolved comments). This patch does the same, so it's a clone.
You should rewrite the commit message to point out that it's false-positive case.
> Because then it's not a fix for anything in flashrom, certainly not a bug fix, but a workaround to assist limited path analysis. (C) Nico Huber
If you want, you can use the commit message from the patch I mentioned earlier.
File stlinkv3_spi.c:
https://review.coreboot.org/c/flashrom/+/66225/comment/aee760f5_769d9277
PS1, Line 501: stlinkv3_handle = usb_dev_get_by_vid_pid_serial(usb_ctx,
> I think both the issues have been 'fixed'. […]
Add the comment that Thomas suggests to you. But put it before the variable, otherwise 112-columns hard limit will be broken.
`/* Initialize stlinkv3_handle to NULL for suppressing scan-build false positive core.uninitialized.Branch */`
Thomas, can we get rid of `core.uninitialized.Branch`? It doesn't suppress the warning :D
--
To view, visit https://review.coreboot.org/c/flashrom/+/66225
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7ce156a9f7455434ea461560320c84660c93c02f
Gerrit-Change-Number: 66225
Gerrit-PatchSet: 2
Gerrit-Owner: Shreeyash <shreeyash335(a)gmail.com>
Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Miklós Márton <martonmiklosqdev(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Shreeyash <shreeyash335(a)gmail.com>
Gerrit-Comment-Date: Wed, 05 Oct 2022 19:06:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de>
Comment-In-Reply-To: Shreeyash <shreeyash335(a)gmail.com>
Gerrit-MessageType: comment
Alexander Goncharov has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/67700 )
Change subject: stlinkv3_spi: fix uninitialized pointer
......................................................................
Abandoned
Duplicate of CB:66225. It was introduced earlier, and owner is still interested.
--
To view, visit https://review.coreboot.org/c/flashrom/+/67700
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ibaf25f67186724d9045ade849026782c3eac4952
Gerrit-Change-Number: 67700
Gerrit-PatchSet: 2
Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: abandon
Attention is currently required from: Nico Huber, Thomas Heijligen, Aarya.
Hello build bot (Jenkins), Simon Buhrow, Thomas Heijligen,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/66104
to look at the new patch set (#62).
Change subject: flashrom.c: Add wrapper function to use the erase algorithm
......................................................................
flashrom.c: Add wrapper function to use the erase algorithm
Add a function to call the erase algorithm.
Change-Id: I29e3f2bd796759794184b125741a5abaac6f3ce8
Signed-off-by: Aarya Chaumal <aarya.chaumal(a)gmail.com>
---
M flashrom.c
M tests/chip_wp.c
2 files changed, 145 insertions(+), 288 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/04/66104/62
--
To view, visit https://review.coreboot.org/c/flashrom/+/66104
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I29e3f2bd796759794184b125741a5abaac6f3ce8
Gerrit-Change-Number: 66104
Gerrit-PatchSet: 62
Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Simon Buhrow
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Felix Singer, Nico Huber, Richard Hughes, Edward O'Callaghan, Daniel Campello, Anastasia Klimchuk.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64663 )
Change subject: libflashrom: Allow getting the progress_state from the flashctx
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
> Quick comment: There seems to be a misunderstanding. At least I read Anatasia's
> "We have a customer" as "We [as the flashrom project] have a customer". I could
> be wrong, though, idk.
Could be. We're used to hearing "customer" in situations where payments are involved, hence the confusion.
--
To view, visit https://review.coreboot.org/c/flashrom/+/64663
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I322bf56ff92f7b4d0ffc92768e9f0cdf7cb82010
Gerrit-Change-Number: 64663
Gerrit-PatchSet: 2
Gerrit-Owner: Richard Hughes <richard(a)hughsie.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Richard Hughes <richard(a)hughsie.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Wed, 05 Oct 2022 16:32:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
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: Daniel Campello <campello(a)chromium.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Edward O'Callaghan, Angel Pons, Nikolai Artemiev.
Jonathon Hall has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67695 )
Change subject: drivers: Move (un)map_flash_region to par/spi/opaque_master
......................................................................
Patch Set 9:
(3 comments)
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/67695/comment/b518865e_8f4dc70d
PS8, Line 222: (
> nit: `if (`
Done
https://review.coreboot.org/c/flashrom/+/67695/comment/314dc0ed_47876c73
PS8, Line 249: #if CONFIG_INTERNAL == 1
> drop this || move inside symbol and make the `master_uses_physmap()` always available (see comment b […]
Done
https://review.coreboot.org/c/flashrom/+/67695/comment/614e52db_6022478d
PS8, Line 869: #if CONFIG_INTERNAL == 1
> Not sure if `physical_memory` is always available. If it is, sure.
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/67695
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I9c3df6ae260bcdb246dfb0cd8e043919609b014b
Gerrit-Change-Number: 67695
Gerrit-PatchSet: 9
Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Thomas Heijligen <src(a)posteo.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-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Wed, 05 Oct 2022 12:36:27 +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>
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Jonathon Hall.
Hello build bot (Jenkins), Thomas Heijligen, Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/68092
to look at the new patch set (#2).
Change subject: flashrom.c: Remove custom mappers from opaque_master
......................................................................
flashrom.c: Remove custom mappers from opaque_master
No opaque masters have a custom mapper. The returned chipaddr is not
fed back into the read/write/erase functions, so this would only be
useful for side effects.
Change-Id: I36f05154edda371b51f8ff416f019837ff1c243d
Signed-off-by: Jonathon Hall <jonathon.hall(a)puri.sm>
---
M flashrom.c
M include/programmer.h
2 files changed, 17 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/92/68092/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/68092
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I36f05154edda371b51f8ff416f019837ff1c243d
Gerrit-Change-Number: 68092
Gerrit-PatchSet: 2
Gerrit-Owner: Jonathon Hall <jonathon.hall(a)puri.sm>
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: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Jonathon Hall <jonathon.hall(a)puri.sm>
Gerrit-MessageType: newpatchset