June 2021 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: tests: Move test environment header files into tests directory
by Angel Pons (Code Review) 11 Jun '21

11 Jun '21

Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55408 ) Change subject: tests: Move test environment header files into tests directory ...................................................................... Patch Set 1: Code-Review+2 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/55408/comment/04047176_13b5a1ab PS1, Line 8: I agree with the change, but I'd appreciate if you could add a sentence to explain why this is a good idea. -- To view, visit https://review.coreboot.org/c/flashrom/+/55408 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia42773b98b1eb6c65241aa559c0c8b4926bd0814 Gerrit-Change-Number: 55408 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-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 11 Jun 2021 11:19:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Move test environment header files into tests directory
by Nico Huber (Code Review) 11 Jun '21

11 Jun '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55408 ) Change subject: tests: Move test environment header files into tests directory ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/55408 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia42773b98b1eb6c65241aa559c0c8b4926bd0814 Gerrit-Change-Number: 55408 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-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-Comment-Date: Fri, 11 Jun 2021 11:01:27 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashchips: add support for Winbond W25Q01JV
by Nico Huber (Code Review) 11 Jun '21

11 Jun '21

Attention is currently required from: David Bartley, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54968 ) Change subject: flashchips: add support for Winbond W25Q01JV ...................................................................... Patch Set 4: (1 comment) File flashchips.c: https://review.coreboot.org/c/flashrom/+/54968/comment/e3c4e52b_071bba5d PS1, Line 17757: /* Full chip erase is fastest, typically takes 200s */ > Fair enough, I can start a thread on the mailing list exploring options. […] The problem is that there is no common use-case to tune the list for. Placing smaller blocks first is better for smaller changes, placing bigger blocks first is better for bigger changes. We don't know in advance what to expect. My idea how to re-arrange things: * Add an optional function to the programmer drivers that can check if a command is expected to succeed (or rather fail, the most obvious example is Intel's internal programmer that can be configured to only allow a specific set of SPI commands). With such a function, the list of block erasers to consider can be filtered at runtime. * Once we know the set of block erasers that is expected to work, we can apply a heuristic that chooses the biggest block that won't unnecessarily erase unchanged data. Or maybe with a threshold, e.g. prefer 64K over 4K if >80% of the 4K blocks would get erased. Well, maybe there is one low-hanging fruit: For the erase commandline option (-E), we could iterate over the list in reverse order? -- To view, visit https://review.coreboot.org/c/flashrom/+/54968 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If369409419332070c1fed96ce0e96b7cfe42c169 Gerrit-Change-Number: 54968 Gerrit-PatchSet: 4 Gerrit-Owner: David Bartley <andareed(a)gmail.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: David Bartley <andareed(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Fri, 11 Jun 2021 10:51:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: David Bartley <andareed(a)gmail.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: dummyflasher: Re-arrange includes
by Nico Huber (Code Review) 11 Jun '21

11 Jun '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/55359 ) Change subject: dummyflasher: Re-arrange includes ...................................................................... dummyflasher: Re-arrange includes Gather library includes at the top. Change-Id: Ib20a3245cae3206dca1d8f88f705ac3628473f43 Signed-off-by: Nico Huber <nico.huber(a)secunet.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/55359 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M dummyflasher.c 1 file changed, 2 insertions(+), 3 deletions(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Anastasia Klimchuk: Looks good to me, approved diff --git a/dummyflasher.c b/dummyflasher.c index 6a75506..049f9d5 100644 --- a/dummyflasher.c +++ b/dummyflasher.c @@ -18,15 +18,14 @@ #include <stdio.h> #include <ctype.h> #include <errno.h> +#include <sys/types.h> +#include <sys/stat.h> #include "flash.h" #include "chipdrivers.h" #include "programmer.h" #include "flashchips.h" #include "spi.h" -#include <sys/types.h> -#include <sys/stat.h> - enum emu_chip { EMULATE_NONE, EMULATE_ST_M25P10_RES, -- To view, visit https://review.coreboot.org/c/flashrom/+/55359 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib20a3245cae3206dca1d8f88f705ac3628473f43 Gerrit-Change-Number: 55359 Gerrit-PatchSet: 3 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: dummyflasher: Replace another case of `sizeof(struct ...)`
by Nico Huber (Code Review) 11 Jun '21

11 Jun '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/55358 ) Change subject: dummyflasher: Replace another case of `sizeof(struct ...)` ...................................................................... dummyflasher: Replace another case of `sizeof(struct ...)` Using the pointer's type instead ensures that we get the right size even if code changes in the future. Change-Id: If88ba394095f86c598dcc5cf1751e1c23b132d04 Signed-off-by: Nico Huber <nico.huber(a)secunet.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/55358 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M dummyflasher.c 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Anastasia Klimchuk: Looks good to me, approved diff --git a/dummyflasher.c b/dummyflasher.c index b75cddb..6a75506 100644 --- a/dummyflasher.c +++ b/dummyflasher.c @@ -947,7 +947,7 @@ { struct stat image_stat; - struct emu_data *data = calloc(1, sizeof(struct emu_data)); + struct emu_data *data = calloc(1, sizeof(*data)); if (!data) { msg_perr("Out of memory!\n"); return 1; -- To view, visit https://review.coreboot.org/c/flashrom/+/55358 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If88ba394095f86c598dcc5cf1751e1c23b132d04 Gerrit-Change-Number: 55358 Gerrit-PatchSet: 3 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: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: dummyflasher: Replace another case of `sizeof(struct ...)`
by Nico Huber (Code Review) 11 Jun '21

11 Jun '21

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55358 ) Change subject: dummyflasher: Replace another case of `sizeof(struct ...)` ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > I meant to say, thank you, it is an honour for me! You're welcome :) And let's say it was overdue anyway, the requirements for the reviewers group[1] are not very high: People who can +2 / -1 changes. Contributors with more activity than a one-off commit. My personal expectations may be higher, but you have exceeded those too anyway ;) You have shown that you are able and willing to read. A trait not common among people in that group Oo [1] https://review.coreboot.org/admin/groups -- To view, visit https://review.coreboot.org/c/flashrom/+/55358 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If88ba394095f86c598dcc5cf1751e1c23b132d04 Gerrit-Change-Number: 55358 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: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Fri, 11 Jun 2021 10:20:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: bitbang-spi.c: support clock polarity and phase
by Nico Huber (Code Review) 11 Jun '21

11 Jun '21

Attention is currently required from: Xiang W, Edward O'Callaghan, Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/49255 ) Change subject: bitbang-spi.c: support clock polarity and phase ...................................................................... Patch Set 28: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/49255/comment/1eede23b_3763e24d PS26, Line 11: may not be > This "may not be" is rather vague. […] I will not comment further on this review until this is answered. File bitbang_spi.c: https://review.coreboot.org/c/flashrom/+/49255/comment/88568544_ebde19db PS26, Line 121: programmer_delay(master->half_period); : bitbang_spi_set_sck_set_mosi(master, !cpol, (val >> i) & 1); : programmer_delay(master->half_period); : bitbang_spi_set_sck(master, cpol); > The hardware logic only requires that the data state is stable after the clock changes. […] Hmm, what can I say more? Please have a careful look at the diagrams in the Wikipedia article you linked and the datasheet of the chip you used for testing. Long story short, your (later corrected) order of signal changes is eventually correct. But your delays are 100% off. You also have to account for programmer drivers that change MOSI and CLK at once in bitbang_spi_set_sck_set_mosi(). If you continue trying to correct your code, please also include logic-analyzer traces before/after the patch and test programming speed as well, so no prior, precious optimizations to this code are lost. -- To view, visit https://review.coreboot.org/c/flashrom/+/49255 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I04c1dfe132d756119229b27c3cd611d1be1abc8d Gerrit-Change-Number: 49255 Gerrit-PatchSet: 28 Gerrit-Owner: Xiang W <wxjstz(a)126.com> 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: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Shawn C <citypw(a)hardenedlinux.org> Gerrit-Attention: Xiang W <wxjstz(a)126.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Fri, 11 Jun 2021 09:46:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Xiang W <wxjstz(a)126.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Do not build a test if its driver is not built
by Anastasia Klimchuk (Code Review) 11 Jun '21

11 Jun '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55295 ) Change subject: tests: Do not build a test if its driver is not built ...................................................................... Patch Set 2: (1 comment) File tests/init_shutdown.c: https://review.coreboot.org/c/flashrom/+/55295/comment/57330655_0e6c3643 PS1, Line 35: #if CONFIG_DUMMY == 1 : void dummy_init_and_shutdown_test_success(void **state) : { : run_lifecycle(state, PROGRAMMER_DUMMY, "bus=parallel+lpc+fwh+spi"); : } : #endif /* CONFIG_DUMMY */ > Right, this would actually benefit from removing the #if's from `programmer.h`. […] Ok, great, I will rebase and try - and so marking comment unresolved until this is done. I will be off for next week, then return back and do everything. -- To view, visit https://review.coreboot.org/c/flashrom/+/55295 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic1c48e41f658045a608f46636071f478ba646f77 Gerrit-Change-Number: 55295 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: 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-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Fri, 11 Jun 2021 06:16:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No 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

1 0

Change in flashrom[master]: tests: Move test environment header files into tests directory
by Anastasia Klimchuk (Code Review) 11 Jun '21

11 Jun '21

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/55408 ) Change subject: tests: Move test environment header files into tests directory ...................................................................... tests: Move test environment header files into tests directory BUG=b:181803212 TEST=ninja test -> all tests pass nm builddir/tests/flashrom_unit_tests.p/.._it85spi.c.o -> has symbols _test_calloc, _test_free, test_inb, test_outb Change-Id: Ia42773b98b1eb6c65241aa559c0c8b4926bd0814 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> --- R tests/hwaccess_x86_io_unittest.h R tests/unittest_env.h 2 files changed, 0 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/08/55408/1 diff --git a/hwaccess_x86_io_unittest.h b/tests/hwaccess_x86_io_unittest.h similarity index 100% rename from hwaccess_x86_io_unittest.h rename to tests/hwaccess_x86_io_unittest.h diff --git a/unittest_env.h b/tests/unittest_env.h similarity index 100% rename from unittest_env.h rename to tests/unittest_env.h -- To view, visit https://review.coreboot.org/c/flashrom/+/55408 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia42773b98b1eb6c65241aa559c0c8b4926bd0814 Gerrit-Change-Number: 55408 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: bitbang_spi.c: fix spi sequence in time and rename
by Xiang W (Code Review) 11 Jun '21

11 Jun '21

Attention is currently required from: Xiang W, Stefan Reinauer, Angel Pons. Hello build bot (Jenkins), Stefan Reinauer, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/49264 to look at the new patch set (#16). Change subject: bitbang_spi.c: fix spi sequence in time and rename ...................................................................... bitbang_spi.c: fix spi sequence in time and rename SPI write operation requires the change of mosi before the change of sck. For this reason, the interface set_sck_set_mosi was rename to set_mosi_set_sck. And the sequence in time bug was corrected. Change-Id: I107803afef044996c94ebfaf0b0e0040cea9f8c1 Signed-off-by: Xiang Wang <merle(a)hardenedlinux.org> --- M bitbang_spi.c M developerbox_spi.c M programmer.h 3 files changed, 9 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/64/49264/16 -- To view, visit https://review.coreboot.org/c/flashrom/+/49264 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I107803afef044996c94ebfaf0b0e0040cea9f8c1 Gerrit-Change-Number: 49264 Gerrit-PatchSet: 16 Gerrit-Owner: Xiang W <wxjstz(a)126.com> 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: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Shawn C <citypw(a)hardenedlinux.org> Gerrit-Attention: Xiang W <wxjstz(a)126.com> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newpatchset

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit June 2021