Attention is currently required from: Edward O'Callaghan, Angel Pons, Neill Corlett.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61288 )
Change subject: Add mediatek_i2c_spi interface.
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
Hi Neill, thanks for your patch.
I've some questions ahead of review:
* The basic SPI command interface looks a lot like the one of the mstarddc_spi driver. Have you looked into that? maybe there can be some code shared.
* There are two implementations to use either I2C or SMBus transfers. Are both paths tested? I only gave it a short glimpse, but it looked like the SMBus path would be doing something different.
* There are some hard-coded GPIO numbers. Is this a reference design or maybe specific to a particular board?
--
To view, visit https://review.coreboot.org/c/flashrom/+/61288
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I24adb14e7b4f7160e1c3ff941774064d5a81e820
Gerrit-Change-Number: 61288
Gerrit-PatchSet: 1
Gerrit-Owner: Neill Corlett <corlett(a)google.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: 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-Attention: Neill Corlett <corlett(a)google.com>
Gerrit-Comment-Date: Sun, 23 Jan 2022 15:14:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Paul Menzel, Peter Marheine.
Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61194 )
Change subject: hwaccess: fix build on non-x86 targets
......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS1:
> > I've now opted to put the x86 guards in the headers rather than in the files that include them bec […]
I've started working on an overhaul of the hwaccess code in CB:61275 and CB:61276. But this will need some time and testing due to the mess of #if. I'm fine with this patch as temporary solution.
--
To view, visit https://review.coreboot.org/c/flashrom/+/61194
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I20f122679c30340b2c73afd7419e79644ddc3c4e
Gerrit-Change-Number: 61194
Gerrit-PatchSet: 4
Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org>
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-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: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Sat, 22 Jan 2022 22:50:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de>
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/60070 )
Change subject: flashrom.c: Move do_*() helpers into cli_classic.c
......................................................................
Set Ready For Review
--
To view, visit https://review.coreboot.org/c/flashrom/+/60070
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If1112155e2421e0178fd73f847cbb80868387433
Gerrit-Change-Number: 60070
Gerrit-PatchSet: 5
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
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: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Sat, 22 Jan 2022 00:14:26 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Daniel Campello, Anastasia Klimchuk.
Hello build bot (Jenkins), Daniel Campello, Angel Pons, Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/59291
to look at the new patch set (#9).
Change subject: flashrom: Drop read_flash_to_file() usage
......................................................................
flashrom: Drop read_flash_to_file() usage
Aspire towards a goal of making cli_classic more of just
a user of libflashrom than having quasi-parallel paths in
flashrom.c
This converts remaining read_flash_to_file() usage to the
do_read() provider wrapper around libflashrom.
BUG=b:208132085
TEST=<TODO>
Change-Id: I4b690b688acf9d5deb46e8642a252a2132ea8c73
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M cli_classic.c
M flash.h
M flashrom.c
3 files changed, 2 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/91/59291/9
--
To view, visit https://review.coreboot.org/c/flashrom/+/59291
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I4b690b688acf9d5deb46e8642a252a2132ea8c73
Gerrit-Change-Number: 59291
Gerrit-PatchSet: 9
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: Daniel Campello <campello(a)chromium.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: 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-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Edward O'Callaghan.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/60074
to look at the new patch set (#4).
Change subject: build: Remove cli_classic need for internal symbols [WIP]
......................................................................
build: Remove cli_classic need for internal symbols [WIP]
cli_classic should now only use the libflashrom interface.
BUG=b:208132085
TEST=`make && meson/ninja`.
Change-Id: Ib04293c14f53c8e798c7d3e35483067520668161
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M meson.build
1 file changed, 0 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/60074/4
--
To view, visit https://review.coreboot.org/c/flashrom/+/60074
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib04293c14f53c8e798c7d3e35483067520668161
Gerrit-Change-Number: 60074
Gerrit-PatchSet: 4
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Nico Huber.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61286 )
Change subject: flashrom.c: Move {read,write}_buf_from_include_args() to layout
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> Isn't it CLI code, technically?
Yes, but it is something Anastasia wanted to see what it looked like in CB:60069 .
I'm more inclined to go with the simplicity of CB:60069 .
--
To view, visit https://review.coreboot.org/c/flashrom/+/61286
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I61d4460ea26e07b67c99baeae5cb3840c25cb913
Gerrit-Change-Number: 61286
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Comment-Date: Sat, 22 Jan 2022 00:08:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/60430 )
Change subject: flashrom: Convert do_read() into a libflashrom user
......................................................................
flashrom: Convert do_read() into a libflashrom user
Aspire towards a goal of making cli_classic more of just
a user of libflashrom than having quasi-parallel paths in
flashrom.c
This converts the do_read() provider wrapper into a pure
libflashrom user.
BUG=b:208132085
TEST=`$ sudo ./flashrom -p internal -r /tmp/bios.bin`
TEST=`$ sudo ./flashrom -p internal -l /tmp/layout -i FOO -r /tmp/foo.bin`
Change-Id: Id2addadb891c482ee3f69da806062d7a88776675
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/60430
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M flashrom.c
1 file changed, 18 insertions(+), 3 deletions(-)
Approvals:
build bot (Jenkins): Verified
Anastasia Klimchuk: Looks good to me, approved
diff --git a/flashrom.c b/flashrom.c
index 1cdddab..257112f 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -2192,13 +2192,28 @@
int do_read(struct flashctx *const flash, const char *const filename)
{
- if (prepare_flash_access(flash, true, false, false, false))
+ int ret;
+
+ unsigned long size = flash->chip->total_size * 1024;
+ unsigned char *buf = calloc(size, sizeof(unsigned char));
+ if (!buf) {
+ msg_gerr("Memory allocation failed!\n");
return 1;
+ }
- const int ret = read_flash_to_file(flash, filename);
+ ret = flashrom_image_read(flash, buf, size);
+ if (ret > 0)
+ goto free_out;
- finalize_flash_access(flash);
+ if (write_buf_to_include_args(flash, buf)) {
+ ret = 1;
+ goto free_out;
+ }
+ if (filename)
+ ret = write_buf_to_file(buf, size, filename);
+free_out:
+ free(buf);
return ret;
}
4 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
--
To view, visit https://review.coreboot.org/c/flashrom/+/60430
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id2addadb891c482ee3f69da806062d7a88776675
Gerrit-Change-Number: 60430
Gerrit-PatchSet: 6
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
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-MessageType: merged
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/60068 )
Change subject: cli_classic.c: Convert do_erase() to libflashrom call
......................................................................
cli_classic.c: Convert do_erase() to libflashrom call
Inline emergency_help_message() to cli_classic call site.
This leaves do_erase() a redudant wrapper and moves us a step
closer to cli_classic as a pure libflashrom user by using
flashrom_flash_erase().
BUG=b:208132085
TEST=`flashrom -E`
Change-Id: I8566164e7dbad69cf478b24208014f10fb99e4d0
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/60068
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Nico Huber <nico.h(a)gmx.de>
Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M cli_classic.c
M flash.h
M flashrom.c
3 files changed, 14 insertions(+), 21 deletions(-)
Approvals:
build bot (Jenkins): Verified
Nico Huber: Looks good to me, approved
Anastasia Klimchuk: Looks good to me, approved
diff --git a/cli_classic.c b/cli_classic.c
index 1b9c5ba..3cd78d4 100644
--- a/cli_classic.c
+++ b/cli_classic.c
@@ -840,8 +840,18 @@
ret = do_read(fill_flash, filename);
else if (extract_it)
ret = do_extract(fill_flash);
- else if (erase_it)
- ret = do_erase(fill_flash);
+ else if (erase_it) {
+ ret = flashrom_flash_erase(fill_flash);
+ /*
+ * FIXME: Do we really want the scary warning if erase failed?
+ * After all, after erase the chip is either blank or partially
+ * blank or it has the old contents. A blank chip won't boot,
+ * so if the user wanted erase and reboots afterwards, the user
+ * knows very well that booting won't work.
+ */
+ if (ret)
+ emergency_help_message();
+ }
else if (write_it)
ret = do_write(fill_flash, filename, referencefile);
else if (verify_it)
diff --git a/flash.h b/flash.h
index 391a2d4..11e6389 100644
--- a/flash.h
+++ b/flash.h
@@ -344,6 +344,7 @@
int read_flash_to_file(struct flashctx *flash, const char *filename);
int verify_range(struct flashctx *flash, const uint8_t *cmpbuf, unsigned int start, unsigned int len);
int need_erase(const uint8_t *have, const uint8_t *want, unsigned int len, enum write_granularity gran, const uint8_t erased_value);
+void emergency_help_message(void);
void print_version(void);
void print_buildinfo(void);
void print_banner(void);
@@ -355,7 +356,6 @@
void finalize_flash_access(struct flashctx *);
int do_read(struct flashctx *, const char *filename);
int do_extract(struct flashctx *);
-int do_erase(struct flashctx *);
int do_write(struct flashctx *, const char *const filename, const char *const referencefile);
int do_verify(struct flashctx *, const char *const filename);
int register_chip_restore(chip_restore_fn_cb_t func, struct flashctx *flash, uint8_t status);
diff --git a/flashrom.c b/flashrom.c
index dd443b9..1cdddab 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -1553,7 +1553,7 @@
"https://www.flashrom.org/Contact for details), thanks!\n");
}
-static void emergency_help_message(void)
+void emergency_help_message(void)
{
msg_gerr("Your flash chip is in an unknown state.\n");
#if CONFIG_INTERNAL == 1
@@ -2208,23 +2208,6 @@
return do_read(flash, NULL);
}
-int do_erase(struct flashctx *const flash)
-{
- const int ret = flashrom_flash_erase(flash);
-
- /*
- * FIXME: Do we really want the scary warning if erase failed?
- * After all, after erase the chip is either blank or partially
- * blank or it has the old contents. A blank chip won't boot,
- * so if the user wanted erase and reboots afterwards, the user
- * knows very well that booting won't work.
- */
- if (ret)
- emergency_help_message();
-
- return ret;
-}
-
int do_write(struct flashctx *const flash, const char *const filename, const char *const referencefile)
{
const size_t flash_size = flash->chip->total_size * 1024;
5 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
--
To view, visit https://review.coreboot.org/c/flashrom/+/60068
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I8566164e7dbad69cf478b24208014f10fb99e4d0
Gerrit-Change-Number: 60068
Gerrit-PatchSet: 7
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: Daniel Campello <campello(a)chromium.org>
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-MessageType: merged
Attention is currently required from: Thomas Heijligen.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61300 )
Change subject: Makeile: reenable internal programmer for mipsel
......................................................................
Patch Set 3: Code-Review+1
(1 comment)
Patchset:
PS3:
Looks good, but it's actually called MIPSEL (don't ask me why!).
(I still need to check default behavior, CONFIG_EVERYTHING etc.)
--
To view, visit https://review.coreboot.org/c/flashrom/+/61300
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ia607ea60c3d7d15fe231fa412595992dadc535ad
Gerrit-Change-Number: 61300
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
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: Thomas Heijligen <src(a)posteo.de>
Gerrit-Comment-Date: Fri, 21 Jan 2022 16:00:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Paul Menzel, Peter Marheine.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/61194 )
Change subject: hwaccess: fix build on non-x86 targets
......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS1:
> I've now opted to put the x86 guards in the headers rather than in the files that include them because otherwise the guards would need to be duplicated across three other files.
These three other files happen to be almost completely wrapped in `#if x86`,
right? It's just the #include that needs to be guarded as well, I guess
(beside the one call to rget_io_perms()). Let's better change those files
that are already a #if mess and not those that we cleaned up only just.
Mid-term we should separate the source code further, i.e. have x86 code
only in files compiled for x86, then we won't need no more #if.
--
To view, visit https://review.coreboot.org/c/flashrom/+/61194
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I20f122679c30340b2c73afd7419e79644ddc3c4e
Gerrit-Change-Number: 61194
Gerrit-PatchSet: 4
Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org>
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-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: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Fri, 21 Jan 2022 15:53:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de>
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: comment