Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Edward O'Callaghan.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56721 )
Change subject: flashrom: Return early from map_flash for >16MiB on x86
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
IIRC, the call to map_flash() is spurious for most chips (all SPI maybe?),
so I would prefer to avoid the call instead.
--
To view, visit https://review.coreboot.org/c/flashrom/+/56721
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I60244b6970118dbdfdbb5b8424fc0a8acd9def2e
Gerrit-Change-Number: 56721
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Sat, 31 Jul 2021 21:51:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/56721 )
Change subject: flashrom: Return early from map_flash for >16MiB on x86
......................................................................
flashrom: Return early from map_flash for >16MiB on x86
In the case of a flash chip bigger than 16 MiB, x86 chipsets are only
able to map the topmost 16 MiB of the chip into memory. To access the
rest, flashrom can use hardware sequencing, therefore return early
from `map_flash()` in this situation; this avoids messages like the
following in Linux kernel logs:
[ 57.715654] x86/PAT: flashrom:2805 conflicting memory types fe0000...
[ 57.726270] x86/PAT: memtype_reserve failed [mem 0xfe000000-0xffff...
BUG=b:185021901
TEST=On a system with a W25Q256.V (32MB), flashrom logs show:
$ flashrom -p host -r bios.bin
...
Found Winbond flash chip "W25Q256.V" (32768 kB, Programmer-specific) ...
Chipset unable to map >16 MiB of flash below 0xffffffff, falling back...
Reading flash... done.
SUCCESS
Signed-off-by: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Change-Id: I60244b6970118dbdfdbb5b8424fc0a8acd9def2e
---
M flashrom.c
1 file changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/56721/1
diff --git a/flashrom.c b/flashrom.c
index 3987bb9..dea96b7 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -697,6 +697,17 @@
return 0;
const chipsize_t size = flash->chip->total_size * 1024;
+
+#if IS_X86
+ /* No known x86 chipset maps more than 16 MiB of the flash into memory
+ * below 0xfffffff, therefore return early here. */
+ if (size > 16 * MiB) {
+ msg_cinfo("Chipset unable to map >16 MiB of flash below 0xffffffff,"
+ "falling back to hardware sequencing.\n");
+ return 1;
+ }
+#endif
+
uintptr_t base = flashbase ? flashbase : (0xffffffff - size + 1);
void *addr = programmer_map_flash_region(flash->chip->name, base, size);
if (addr == ERROR_PTR) {
--
To view, visit https://review.coreboot.org/c/flashrom/+/56721
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I60244b6970118dbdfdbb5b8424fc0a8acd9def2e
Gerrit-Change-Number: 56721
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-MessageType: newchange
Attention is currently required from: Edward O'Callaghan.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56466 )
Change subject: programmer.h: Minor tidy up
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
> Very likely not in most cases. […]
Hmmm, looking at this from another angle: I think we should just split the
file. That would make guarding implicit, right? e.g. if you some code doesn't
need USB stuff, it wouldn't include the usb header file.
If all these declarations can be clearly grouped, then I see no reason to have
them in a monolithic header file.
WDYT?
--
To view, visit https://review.coreboot.org/c/flashrom/+/56466
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7ceda32b66d17318d42b94a1fab621a9a926fa77
Gerrit-Change-Number: 56466
Gerrit-PatchSet: 2
Gerrit-Owner: 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: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Fri, 30 Jul 2021 14:29:01 +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>
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/+/56501 )
Change subject: tests: Add tests to erase a chip
......................................................................
Patch Set 6:
(1 comment)
Patchset:
PS2:
> I looked again, and realised I want to improve my solution a bit. […]
Some more explanations (sorry for spam :D)
My strong preference to have only one flashrom_test_dep in root meson.build , because if there are two dependencies this creates a "choice" and a question "which one should I choose for my new test? which one is the right one?" and a chance to pick wrong one. There should not be a choice in this situation, only one flashrom_test_dep with all compile args set up for test context.
A side-effect is that all test_outx and test_inx need to be wrapped for all tests executables, even if the test itself is not using them. Those wraps can be empty, but need to be present. Potentially later this can be improved, maybe wraps extracted into separate .c file. There are lots of wraps already, and there will be more (because there will be more tests!)
--
To view, visit https://review.coreboot.org/c/flashrom/+/56501
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6f74bfe4e02244d24d6c837cc3d551251e7b4898
Gerrit-Change-Number: 56501
Gerrit-PatchSet: 6
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-Comment-Date: Fri, 30 Jul 2021 06:12:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
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/+/56501 )
Change subject: tests: Add tests to erase a chip
......................................................................
Patch Set 6:
(1 comment)
Patchset:
PS2:
> Yes, I remember that when function is wrapped, the real one is meant to be available as __real_funct […]
I looked again, and realised I want to improve my solution a bit. I don't want any changes in root meson.build (reverted them), so now all changes are contained within tests/ Still two test executables, but both of them using the same flashrom_test_dep from root meson.build.
The thing which is not ideal: wraps for inx and outx are repeated in wrap_send_tests.c (they can do nothing, but need to be present). I am thinking how to make this better... but maybe this can be done later? (if everyone agrees of course).
Next thing I am going to split the patch into two.
--
To view, visit https://review.coreboot.org/c/flashrom/+/56501
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6f74bfe4e02244d24d6c837cc3d551251e7b4898
Gerrit-Change-Number: 56501
Gerrit-PatchSet: 6
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-Comment-Date: Fri, 30 Jul 2021 05:40:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Nico Huber, Angel Pons, Anastasia Klimchuk.
Hello build bot (Jenkins), Nico Huber, Edward O'Callaghan, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/56501
to look at the new patch set (#6).
Change subject: tests: Add tests to erase a chip
......................................................................
tests: Add tests to erase a chip
Two tests cover the code which performs do_erase operation.
First one works with fake chip and dummy programmer. Fake chip has all
operations defined, and a buffer to emulate chip memory.
Second one uses the chip which is closer to the real one, because
read/write/unlock/erase operations are real. The tests takes the
advantage of dummyflasher's capability of emulating a W25Q128.V chip.
BUG=b:181803212
TEST=builds and ninja test
Change-Id: I6f74bfe4e02244d24d6c837cc3d551251e7b4898
Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
A tests/chip.c
M tests/meson.build
M tests/tests.c
M tests/tests.h
A tests/wrap_send_tests.c
A tests/wrap_send_tests.h
6 files changed, 365 insertions(+), 34 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/01/56501/6
--
To view, visit https://review.coreboot.org/c/flashrom/+/56501
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I6f74bfe4e02244d24d6c837cc3d551251e7b4898
Gerrit-Change-Number: 56501
Gerrit-PatchSet: 6
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-MessageType: newpatchset
Attention is currently required from: Henry Sun, Tao Xia, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev, Karthik Ramasubramanian.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56546 )
Change subject: flashchips: Add MX25L12833F
......................................................................
Patch Set 1:
(2 comments)
Patchset:
PS1:
> I have uploaded the datasheet in the issue https://partnerissuetracker.corp.google. […]
This bug tracker is not public, though. Found it here:
https://www.mxic.com.tw/Lists/Datasheet/Attachments/7447/MX25L12833F,%203V,…
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/56546/comment/f86b40e7_3c0923b4
PS1, Line 8368: 512B
Looks like the 33F has a whole kilobyte?
--
To view, visit https://review.coreboot.org/c/flashrom/+/56546
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I14ab7e04f5209d2bcf34b0d2de9da2c01bf32d00
Gerrit-Change-Number: 56546
Gerrit-PatchSet: 1
Gerrit-Owner: Tao Xia <xiatao5(a)huaqin.corp-partner.google.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: Henry Sun <henrysun(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Ginger Zhang <zhangqingchun(a)huaqin.corp-partner.google.com>
Gerrit-CC: Krystal Han <hanlijing(a)huaqin.corp-partner.google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Weimin Wu <wuweimin(a)huaqin.corp-partner.google.com>
Gerrit-Attention: Henry Sun <henrysun(a)google.com>
Gerrit-Attention: Tao Xia <xiatao5(a)huaqin.corp-partner.google.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Thu, 29 Jul 2021 09:58:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Tao Xia <xiatao5(a)huaqin.corp-partner.google.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/56164 )
Change subject: ft2232_spi: Add FTDI search by description.
......................................................................
ft2232_spi: Add FTDI search by description.
This adds to the search-by-serial functionality with
search-by-description (product string). This is useful when e.g. one has
multiple FTDIs in a system and wants the serial numbers to reflect the
system-level serial number, and the description to reflect the
subcomponent names.
Tested manually by running with both serial and description searches, on
a machine with multiple FTDIs plugged in. Ensured that when two devices
with the same vid/pid/serial number are plugged in, description can
be used to differentiate.
Verifed no-description, no-serial, one FTDI plugged in base case works.
Original version of this code used the original single "arg" char*, but
on further thought, this wasn't worth the readability and functionality
losses. The new version with arg2 gets rid of several lines of code, the
gotos, and adds the ability to filter by both description and serial
simultaneously.
Change-Id: Ib4be23247995710900175f5f16e38db577ef08fa
Signed-off-by: Harry Johnson <johnsonh(a)waymo.com>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/56164
Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
---
M ft2232_spi.c
1 file changed, 6 insertions(+), 2 deletions(-)
Approvals:
build bot (Jenkins): Verified
Anastasia Klimchuk: Looks good to me, approved
diff --git a/ft2232_spi.c b/ft2232_spi.c
index d853b09..e32d7f8 100644
--- a/ft2232_spi.c
+++ b/ft2232_spi.c
@@ -319,7 +319,7 @@
*/
uint32_t divisor = DEFAULT_DIVISOR;
int f;
- char *arg;
+ char *arg, *arg2;
double mpsse_clk;
uint8_t cs_bits = 0x08;
@@ -499,8 +499,12 @@
}
arg = extract_programmer_param("serial");
- f = ftdi_usb_open_desc(&ftdic, ft2232_vid, ft2232_type, NULL, arg);
+ arg2 = extract_programmer_param("description");
+
+ f = ftdi_usb_open_desc(&ftdic, ft2232_vid, ft2232_type, arg2, arg);
+
free(arg);
+ free(arg2);
if (f < 0 && f != -5) {
msg_perr("Unable to open FTDI device: %d (%s)\n", f,
--
To view, visit https://review.coreboot.org/c/flashrom/+/56164
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib4be23247995710900175f5f16e38db577ef08fa
Gerrit-Change-Number: 56164
Gerrit-PatchSet: 6
Gerrit-Owner: johnsonh(a)waymo.com
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-CC: Andrew Bresticker <abrestic(a)google.com>
Gerrit-MessageType: merged