Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54990 )
Change subject: bitbang: Extend register_spi_bitbang_master() API with spi data
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
File bitbang_spi.c:
https://review.coreboot.org/c/flashrom/+/54990/comment/5cff25e5_2a184792
PS1, Line 164: mst.data = data;
: register_spi_master(&mst, NULL);
I feel like the removal of `mst.data = data;` and instead passing data directly into register_spi_master() is itself a patch. small granted but none the less technically not within the scope of this patch.
--
To view, visit https://review.coreboot.org/c/flashrom/+/54990
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I13e83ae74dbc3a3e79c84d1463683d360ff47bc0
Gerrit-Change-Number: 54990
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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Thu, 27 May 2021 10:19:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: David Bartley, Nico Huber.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54968 )
Change subject: flashchips: add support for Winbond W25Q01JV
......................................................................
Patch Set 1:
(6 comments)
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/54968/comment/e9563905_4d8bc47c
PS1, Line 17745: W25Q501V
Flash chip entries are sorted alphabetically by vendor and name. This one should be placed between `W25P80` and `W25Q128.V`.
https://review.coreboot.org/c/flashrom/+/54968/comment/df90eb13_fb1880d9
PS1, Line 17745: 5
The `5` seems spurious?
https://review.coreboot.org/c/flashrom/+/54968/comment/c8b5bb4b_ce6abeb1
PS1, Line 17757: /* Full chip erase is fastest, typically takes 200s */
Block erasers for the other flash chip entries are intentionally sorted in ascending block size. Please keep this ordering.
# Context
When writing, flashrom first reads the old flash chip contents and only erases/writes the blocks which have changed. This significantly reduces flashing time when only a part of the data needs to be rewritten. This behavior is also useful when using a layout file to only rewrite specific sections of the flash chip.
However, flashrom currently always uses the first block eraser, and only falls back to the other block erasers if there's an error. Dynamically choosing the best block erasers for each case would be great to have, but someone needs to implement it. Patches are welcome!
https://review.coreboot.org/c/flashrom/+/54968/comment/9aa66627_05ce4e8c
PS1, Line 17781: spi_prettyprint_status_register_plain
spi_prettyprint_status_register_bp3_srwd
https://review.coreboot.org/c/flashrom/+/54968/comment/bf58c60a_28a6a207
PS1, Line 17782: spi_disable_blockprotect
spi_disable_blockprotect_bp3_srwd
https://review.coreboot.org/c/flashrom/+/54968/comment/f6300f56_5c9ea10b
PS1, Line 17788:
nit: use only one blank line (there's two right now)
--
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: 1
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: Nico Huber <nico.h(a)gmx.de>
Gerrit-Comment-Date: Thu, 27 May 2021 09:32:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
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/+/52958 )
Change subject: buspirate_spi.c: Refactor singleton states into reentrant pattern
......................................................................
Patch Set 3:
(2 comments)
File buspirate_spi.c:
https://review.coreboot.org/c/flashrom/+/52958/comment/21f643f3_6ce3fdfa
PS3, Line 58: unsigned char **bp_commbuf
> I'd just pass a pointer to `struct bp_spi_data`
I was considering the idea to pass a pointer to `struct bp_spi_data` into buspirate_commbuf_grow but it didn't look great [to me] in init function.
Init function is quite long, and I wanted to use local variables for bp_commbuf and bp_commbufsize in init function to minimize diffs and noise. But at the same time, buspirate_commbuf_grow is called twice in the middle of init function, and each of those calls can change the pointer and the size.
So, if I would need to pass data struct into buspirate_commbuf_grow, and also using local variables during init, that would mean:
assigning local values into data,
calling buspirate_commbuf_grow,
and then assigning updated values from data into local variables.
https://review.coreboot.org/c/flashrom/+/52958/comment/f8d568a4_c90df3f2
PS3, Line 221: unsigned char *const bp_commbuf = bp_data->bp_commbuf;
> I thought that buspirate_commbuf_grow can change the pointer, and I can have this local variable as […]
As I learned today from C standard, realloc can change the pointer. And because I want this to be a constant pointer, I only can define it after the call to buspirate_commbuf_grow (because it does realloc).
--
To view, visit https://review.coreboot.org/c/flashrom/+/52958
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I418bbfff15fb126b042fbc9be09dbf59f4d243b8
Gerrit-Change-Number: 52958
Gerrit-PatchSet: 3
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: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Thu, 27 May 2021 02:43:39 +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
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/54907 )
Change subject: dummyflasher.c: Get rid of get_data_from_context()
......................................................................
dummyflasher.c: Get rid of get_data_from_context()
Relying on the global state 'dummy_buses_supported' to
determine the member master struct [mst.par or mst.spi]
is both buggy and ultimately unnecessary. It became
apparent after commit 4eef651ff503f81b77 just how fragile
this really was as the 'defaults' simultaneously selected
both buses causing get_data_from_context() to fall-though
however memory happened to workout by chance due to the
union. With the member master structs now being struct
fields the subtle bug is more apparent.
BUG=none
BRANCH=none
TEST=`./flashrom -r /tmp/fwupdater.apnSQQ -p dummy:emulate=VARIABLE_SIZE,image=test_update.sh.tmp.emu,size=8388608`
Change-Id: I07a34faf50ff0679cb3d6bc683142f82160010b1
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/54907
Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org>
Reviewed-by: Sam McNally <sammc(a)google.com>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
---
M dummyflasher.c
1 file changed, 2 insertions(+), 14 deletions(-)
Approvals:
build bot (Jenkins): Verified
Sam McNally: Looks good to me, approved
Anastasia Klimchuk: Looks good to me, but someone else must approve
diff --git a/dummyflasher.c b/dummyflasher.c
index 587fc61..b7cfab8 100644
--- a/dummyflasher.c
+++ b/dummyflasher.c
@@ -103,18 +103,6 @@
#endif
#endif
-static enum chipbustype dummy_buses_supported = BUS_NONE;
-
-static struct emu_data* get_data_from_context(const struct flashctx *flash)
-{
- if (dummy_buses_supported & BUS_NONSPI)
- return (struct emu_data *)flash->mst->par.data;
- else if (dummy_buses_supported & BUS_SPI)
- return (struct emu_data *)flash->mst->spi.data;
-
- return NULL; /* buses was set to BUS_NONE. */
-}
-
void *dummy_map(const char *descr, uintptr_t phys_addr, size_t len)
{
msg_pspew("%s: Mapping %s, 0x%zx bytes at 0x%0*" PRIxPTR "\n",
@@ -688,7 +676,7 @@
/* Convert the parameters to lowercase. */
tolower_string(bustext);
- dummy_buses_supported = BUS_NONE;
+ enum chipbustype dummy_buses_supported = BUS_NONE;
if (strstr(bustext, "parallel")) {
dummy_buses_supported |= BUS_PARALLEL;
msg_pdbg("Enabling support for %s flash.\n", "parallel");
@@ -1041,7 +1029,7 @@
int probe_variable_size(struct flashctx *flash)
{
unsigned int i;
- const struct emu_data *emu_data = get_data_from_context(flash);
+ const struct emu_data *emu_data = flash->mst->spi.data;
/* Skip the probing if we don't emulate this chip. */
if (!emu_data || emu_data->emu_chip != EMULATE_VARIABLE_SIZE)
--
To view, visit https://review.coreboot.org/c/flashrom/+/54907
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I07a34faf50ff0679cb3d6bc683142f82160010b1
Gerrit-Change-Number: 54907
Gerrit-PatchSet: 3
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
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: Namyoon Woo <namyoon(a)google.com>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: merged
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/54906 )
Change subject: dummyflasher.c: Inline data fetch for spi workers
......................................................................
dummyflasher.c: Inline data fetch for spi workers
The spi callbacks do not need par logic to fetch the
data field. Instead of going though get_data_from_context()
just fetch 'data' directly out of mst.spi.
This leads us towards a path of removing dummy_buses_supported
from global state.
BUG=none
BRANCH=none
TEST=`./flashrom -r /tmp/fwupdater.apnSQQ -p dummy:emulate=VARIABLE_SIZE,image=test_update.sh.tmp.emu,size=8388608`
Change-Id: I63b8d8861df75f52f241f09614146990fdfe59ed
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/54906
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Sam McNally <sammc(a)google.com>
Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M dummyflasher.c
1 file changed, 2 insertions(+), 2 deletions(-)
Approvals:
build bot (Jenkins): Verified
Sam McNally: Looks good to me, approved
Anastasia Klimchuk: Looks good to me, but someone else must approve
diff --git a/dummyflasher.c b/dummyflasher.c
index 4661f3d..587fc61 100644
--- a/dummyflasher.c
+++ b/dummyflasher.c
@@ -129,7 +129,7 @@
static int dummy_spi_write_256(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len)
{
- struct emu_data *emu_data = get_data_from_context(flash);
+ struct emu_data *emu_data = flash->mst->spi.data;
return spi_write_chunked(flash, buf, start, len,
emu_data->spi_write_256_chunksize);
}
@@ -570,7 +570,7 @@
unsigned char *readarr)
{
unsigned int i;
- struct emu_data *emu_data = get_data_from_context(flash);
+ struct emu_data *emu_data = flash->mst->spi.data;
if (!emu_data) {
msg_perr("No data in flash context!\n");
return 1;
--
To view, visit https://review.coreboot.org/c/flashrom/+/54906
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I63b8d8861df75f52f241f09614146990fdfe59ed
Gerrit-Change-Number: 54906
Gerrit-PatchSet: 2
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
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: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: merged
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/+/54890 )
Change subject: programmer: Introduce default shutdown function
......................................................................
Patch Set 1:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/54890/comment/6c871a4f_ec3cf9f3
PS1, Line 13: lot of par masters that
: will use it in the nearest future.
> Yes sure, my main goal was to get some comments, thank you for comment! […]
I have an update: there are 4 more customers that would be using default_shutdown function (bitbang spi masters):
CB:54994
CB:54995
CB:54996
CB:54998
So if you are saying this is a good direction, maybe we can consider going ahead with it?
If this goes after bitbangs, that would be 5 places to change - still fine. And then 10 par masters would use default shutdown straight away.
--
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: 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: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Thu, 27 May 2021 02:00:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment