September 2024 - flashrom-gerrit

[M] Change in flashrom[main]: Complete and fix progress feature implementation for all operations
by Anastasia Klimchuk (Code Review) 19 Sep '24

19 Sep '24

Attention is currently required from: Aarya, Nikolai Artemiev, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change by Sergii Dmytruk. ( https://review.coreboot.org/c/flashrom/+/84102?usp=email ) Change subject: Complete and fix progress feature implementation for all operations ...................................................................... Patch Set 2: (1 comment) File en29lv640b.c: https://review.coreboot.org/c/flashrom/+/84102/comment/9b17c834_ccbff766?us… : PS1, Line 29: /* chunksize is 2 */ > This can also be a separate commit which updates the comment only. CB:84422 -- To view, visit https://review.coreboot.org/c/flashrom/+/84102?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: If1e40fc97f443c4f0c0501cef11cff1f3f84c051 Gerrit-Change-Number: 84102 Gerrit-PatchSet: 2 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> 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-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Thu, 19 Sep 2024 12:05:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[S] Change in flashrom[main]: dummyflasher: Enable higher frequency emulation
by Anastasia Klimchuk (Code Review) 19 Sep '24

19 Sep '24

Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84423?usp=email ) Change subject: dummyflasher: Enable higher frequency emulation ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: Note: need to ensure this change works together with CB:84102 and all tests pass at the end of a chain. -- To view, visit https://review.coreboot.org/c/flashrom/+/84423?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Iaca5d95f8f977bf0c2283c6458d8977e6ce70251 Gerrit-Change-Number: 84423 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 19 Sep 2024 12:04:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[M] Change in flashrom[main]: Complete and fix progress feature implementation for all operations
by Anastasia Klimchuk (Code Review) 19 Sep '24

19 Sep '24

Attention is currently required from: Aarya, Nikolai Artemiev, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change by Sergii Dmytruk. ( https://review.coreboot.org/c/flashrom/+/84102?usp=email ) Change subject: Complete and fix progress feature implementation for all operations ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: Still working on the tests, might need some time to get them work. I extracted few things into separate commits, but this one need some more time (and update to devel.rst) -- To view, visit https://review.coreboot.org/c/flashrom/+/84102?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: If1e40fc97f443c4f0c0501cef11cff1f3f84c051 Gerrit-Change-Number: 84102 Gerrit-PatchSet: 2 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> 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-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Thu, 19 Sep 2024 12:02:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[M] Change in flashrom[main]: Complete and fix progress feature implementation for all operations
by Anastasia Klimchuk (Code Review) 19 Sep '24

19 Sep '24

Attention is currently required from: Aarya, Nikolai Artemiev, Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change by Sergii Dmytruk. ( https://review.coreboot.org/c/flashrom/+/84102?usp=email ) Change subject: Complete and fix progress feature implementation for all operations ...................................................................... Patch Set 2: (1 comment) File dediprog.c: https://review.coreboot.org/c/flashrom/+/84102/comment/2ff90ccc_3b06d507?us… : PS1, Line 596: 256 > wow thanks for updating the comment! :) […] CB:84421 -- To view, visit https://review.coreboot.org/c/flashrom/+/84102?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: If1e40fc97f443c4f0c0501cef11cff1f3f84c051 Gerrit-Change-Number: 84102 Gerrit-PatchSet: 2 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> 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-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Thu, 19 Sep 2024 11:56:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[S] Change in flashrom[main]: dummyflasher: Enable higher frequency emulation
by Anastasia Klimchuk (Code Review) 19 Sep '24

19 Sep '24

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/84423?usp=email ) Change subject: dummyflasher: Enable higher frequency emulation ...................................................................... dummyflasher: Enable higher frequency emulation The patch also adds a section on a manpage to explain the freq parameter in dummyflasher. Change-Id: Iaca5d95f8f977bf0c2283c6458d8977e6ce70251 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M doc/classic_cli_manpage.rst M dummyflasher.c 2 files changed, 14 insertions(+), 5 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/23/84423/1 diff --git a/doc/classic_cli_manpage.rst b/doc/classic_cli_manpage.rst index e2ce684..ba77b7a 100644 --- a/doc/classic_cli_manpage.rst +++ b/doc/classic_cli_manpage.rst @@ -616,6 +616,15 @@ syntax where ``state`` is ``yes`` or ``no`` (default value). ``yes`` means active state of the pin implies that chip is write-protected (on real hardware the pin is usually negated, but not here). +**Frequency** + Frequency can be emulated in ``Hz`` (default), ``KHz``, or ``MHz`` (not case sensitive). + If ``freq`` parameter is passed in from command line, commands will delay for certain time before returning, + so that to emulate the requested frequency. + + The delay is calculated based on the assumption that we only work with bytes and transfer at 1 bit/Hz:: + + flashrom -p dummy:emulate=W25Q128FV,freq=64mhz + nic3com, nicrealtek, nicnatsemi, nicintel, nicintel_eeprom, nicintel_spi, gfxnvidia, ogp_spi, drkaiser, satasii, satamv, atahpt, atavia, atapromise, it8212 programmers ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/dummyflasher.c b/dummyflasher.c index cf4ca03..283e0c6 100644 --- a/dummyflasher.c +++ b/dummyflasher.c @@ -55,7 +55,7 @@ uint8_t emu_status_len; /* number of emulated status registers */ /* If "freq" parameter is passed in from command line, commands will delay * for this period before returning. */ - unsigned long int delay_us; + unsigned long long delay_ns; unsigned int emu_max_byteprogram_size; unsigned int emu_max_aai_size; unsigned int emu_jedec_se_size; @@ -901,7 +901,7 @@ msg_pspew(" 0x%02x", readarr[i]); msg_pspew("\n"); - default_delay((writecnt + readcnt) * emu_data->delay_us); + default_delay(((writecnt + readcnt) * emu_data->delay_ns) / 1000); return 0; } @@ -1128,7 +1128,7 @@ /* frequency to emulate in Hz (default), KHz, or MHz */ tmp = extract_programmer_param_str(cfg, "freq"); if (tmp) { - unsigned long int freq; + unsigned long long freq; char *units = tmp; char *end = tmp + strlen(tmp); @@ -1172,7 +1172,7 @@ return 1; } /* Assume we only work with bytes and transfer at 1 bit/Hz */ - data->delay_us = (1000000 * 8) / freq; + data->delay_ns = (1000000000ull * 8) / freq; } free(tmp); @@ -1402,7 +1402,7 @@ return 1; } data->emu_chip = EMULATE_NONE; - data->delay_us = 0; + data->delay_ns = 0; data->spi_write_256_chunksize = 256; msg_pspew("%s\n", __func__); -- To view, visit https://review.coreboot.org/c/flashrom/+/84423?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Iaca5d95f8f977bf0c2283c6458d8977e6ce70251 Gerrit-Change-Number: 84423 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[XS] Change in flashrom[main]: en29lv640b.c: Fix the comment about chunksize
by Anastasia Klimchuk (Code Review) 19 Sep '24

19 Sep '24

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/84422?usp=email ) Change subject: en29lv640b.c: Fix the comment about chunksize ...................................................................... en29lv640b.c: Fix the comment about chunksize Change-Id: I104611d42b301662e2c833498aca99c879685846 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M en29lv640b.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/22/84422/1 diff --git a/en29lv640b.c b/en29lv640b.c index 8a8d641..4d93844 100644 --- a/en29lv640b.c +++ b/en29lv640b.c @@ -26,7 +26,7 @@ * functions. */ -/* chunksize is 1 */ +/* chunksize is 2 */ int write_en29lv640b(struct flashctx *flash, const uint8_t *src, unsigned int start, unsigned int len) { unsigned int i; -- To view, visit https://review.coreboot.org/c/flashrom/+/84422?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I104611d42b301662e2c833498aca99c879685846 Gerrit-Change-Number: 84422 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[XS] Change in flashrom[main]: dediprog: Fix comment about usb transfer size
by Anastasia Klimchuk (Code Review) 19 Sep '24

19 Sep '24

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/84421?usp=email ) Change subject: dediprog: Fix comment about usb transfer size ...................................................................... dediprog: Fix comment about usb transfer size Change-Id: I1ad7f89b0a6c91907440e3897ac262bd82f846d5 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M dediprog.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/84421/1 diff --git a/dediprog.c b/dediprog.c index 734fcfa..aa3a1cf 100644 --- a/dediprog.c +++ b/dediprog.c @@ -593,7 +593,7 @@ static int dediprog_spi_bulk_write(struct flashctx *flash, const uint8_t *buf, unsigned int chunksize, unsigned int start, unsigned int len, uint8_t dedi_spi_cmd) { - /* USB transfer size must be 512, other sizes will NOT work at all. + /* USB transfer size must be 256, other sizes will NOT work at all. * chunksize is the real data size per USB bulk transfer. The remaining * space in a USB bulk transfer must be filled with 0xff padding. */ -- To view, visit https://review.coreboot.org/c/flashrom/+/84421?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I1ad7f89b0a6c91907440e3897ac262bd82f846d5 Gerrit-Change-Number: 84421 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[M] Change in flashrom[main]: Complete and fix progress feature implementation for all operations
by Anastasia Klimchuk (Code Review) 19 Sep '24

19 Sep '24

Attention is currently required from: Aarya, Anastasia Klimchuk, Nikolai Artemiev. Anastasia Klimchuk has uploaded a new patch set (#2) to the change originally created by Sergii Dmytruk. ( https://review.coreboot.org/c/flashrom/+/84102?usp=email ) The following approvals got outdated and were removed: Verified-1 by build bot (Jenkins) Change subject: Complete and fix progress feature implementation for all operations ...................................................................... Complete and fix progress feature implementation for all operations Original progress reporting implemented in CB:49643 and it has some issues, for example: size_t start_address = start; size_t end_address = len - start; End address is anything but length minus start address. update_progress(flash, FLASHROM_PROGRESS_READ, /*current*/ start - start_address + to_read, /*total*/ end_address); Total should just be length if that's how current value is computed. --- libflashrom needs to know total size ahead of time. That's init_progress() and changed update_progress(). It also needs to store the last current value to be able to update it. That's stage_progress in flashrom_flashctx. Measuring accurately amount of data which will be read/erased/written isn't easy because things can be skipped as optimizations. Current values are probably off, there are TODO/FIXME comments. --- CLI shares terminal with the rest of the code and has to maintain more state to handle that reasonably well. The progress doesn't just dump lots of stuff on the screen which probably won't fly because of CB:64668, but it's not hard to adjust this. --- A script to test the CLI: \#!/bin/bash t=${1:-rewW} shift if [[ $t =~ r ]]; then echo ">>> READ" ./flashrom -p dummy:emulate=W25Q128FV,freq=64mhz -r dump.rom --progress "$@" echo fi if [[ $t =~ e ]]; then echo ">>> ERASE" ./flashrom -p dummy:emulate=W25Q128FV,freq=64mhz -E --progress "$@" echo fi if [[ $t =~ w ]]; then echo ">>> WRITE (without erase)" dd if=/dev/zero of=zero.rom bs=1M count=16 2> /dev/null ./flashrom -p dummy:emulate=W25Q128FV,freq=64mhz -w zero.rom --progress "$@" echo fi if [[ $t =~ W ]]; then echo ">>> WRITE (with erase)" dd if=/dev/zero of=zero.rom bs=1M count=16 2> /dev/null dd if=/dev/random of=random.rom bs=1M count=16 2> /dev/null ./flashrom -p dummy:emulate=W25Q128FV,freq=64mhz,image=random.rom -w zero.rom --progress "$@" echo fi Change-Id: If1e40fc97f443c4f0c0501cef11cff1f3f84c051 Signed-off-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M 82802ab.c M at45db.c M cli_classic.c M cli_output.c M dediprog.c M en29lv640b.c M erasure_layout.c M flashrom.c M include/flash.h M it87spi.c M jedec.c M libflashrom.c M linux_mtd.c M parade_lspcon.c M realtek_mst_i2c_spi.c M spi.c M spi25.c M sst28sf040.c 18 files changed, 181 insertions(+), 38 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/02/84102/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/84102?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: If1e40fc97f443c4f0c0501cef11cff1f3f84c051 Gerrit-Change-Number: 84102 Gerrit-PatchSet: 2 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> 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-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>

1 0

[M] Change in flashrom[main]: Fix FEATURE_NO_ERASE chips and add test for them
by Anastasia Klimchuk (Code Review) 16 Sep '24

16 Sep '24

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/84203?usp=email ) Change subject: Fix FEATURE_NO_ERASE chips and add test for them ...................................................................... Fix FEATURE_NO_ERASE chips and add test for them New check was added to `check_block_eraser` in commit 0f389aea9e630c3b21547a5dd8dbe572a8502853 but it was not handling FEATURE_NO_ERASE chips. This patch fixes processing such chips and adds test to run write and verify with dummyflasher for FEATURE_NO_ERASE chips. Ticket: https://ticket.coreboot.org/issues/553 Change-Id: I582fe00da0715e9b5e92fcc9d15d5a90a2615117 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/84203 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Peter Marheine <pmarheine(a)chromium.org> --- M flashrom.c M include/flash.h M tests/chip.c M tests/tests.c M tests/tests.h 5 files changed, 80 insertions(+), 7 deletions(-) Approvals: build bot (Jenkins): Verified Peter Marheine: Looks good to me, approved diff --git a/flashrom.c b/flashrom.c index 01a41c4..e2c60f4 100644 --- a/flashrom.c +++ b/flashrom.c @@ -484,14 +484,15 @@ if (flash->mst->buses_supported & BUS_SPI) { const uint8_t *opcode = spi_get_opcode_from_erasefn(eraser.block_erase); - for (int i = 0; opcode[i]; i++) { - if (!spi_probe_opcode(flash, opcode[i])) { - if (log) - msg_cdbg("block erase function and layout found " - "but SPI master doesn't support the function. "); - return 1; + if (opcode) + for (int i = 0; opcode[i]; i++) { + if (!spi_probe_opcode(flash, opcode[i])) { + if (log) + msg_cdbg("block erase function and layout found " + "but SPI master doesn't support the function. "); + return 1; + } } - } } // TODO: Once erase functions are annotated with allowed buses, check that as well. return 0; diff --git a/include/flash.h b/include/flash.h index d9a20ad..d0e55af 100644 --- a/include/flash.h +++ b/include/flash.h @@ -148,6 +148,14 @@ * other flash chips, such as the ENE KB9012 internal flash, work the opposite way. */ #define FEATURE_ERASED_ZERO (1 << 18) +/* + * Feature indicates that the chip does not require erase before writing: + * write operations can set any bit to any value without first doing an erase, + * but bulk erase operations may still be supported. + * + * EEPROMs usually behave this way (compare to Flash, which requires erase), + * for example the ST M95M02. + */ #define FEATURE_NO_ERASE (1 << 19) #define FEATURE_WRSR_EXT2 (1 << 20) diff --git a/tests/chip.c b/tests/chip.c index 3e1ae3b..604d79e 100644 --- a/tests/chip.c +++ b/tests/chip.c @@ -27,6 +27,7 @@ #include <include/test.h> #include <stdio.h> +#include <stdlib.h> #include <string.h> #include "tests.h" @@ -132,6 +133,25 @@ }}, }; +/* Chip expected to be processed with dummyflasher, so using real op functions. */ +static const struct flashchip chip_no_erase = { + .vendor = "aklm&dummyflasher", + .total_size = 16 * 1024, + .tested = TEST_OK_PREW, + .read = SPI_CHIP_READ, + .write = SPI_CHIP_WRITE256, + .page_size = 256, + .feature_bits = FEATURE_NO_ERASE | FEATURE_ERASED_ZERO, + .block_erasers = + { + { + .eraseblocks = { {16 * 1024 * 1024, 1} }, + /* Special erase fn for chips without erase capability. */ + .block_erase = SPI_BLOCK_ERASE_EMULATION, + } + }, +}; + /* Setup the struct for W25Q128.V, all values come from flashchips.c */ static const struct flashchip chip_W25Q128_V = { .vendor = "aklm&dummyflasher", @@ -384,6 +404,48 @@ free(newcontents); } +void write_chip_feature_no_erase(void **state) +{ + (void) state; /* unused */ + + static struct io_mock_fallback_open_state data = { + .noc = 0, + .paths = { NULL }, + }; + const struct io_mock chip_io = { + .fallback_open_state = &data, + }; + + struct flashrom_flashctx flashctx = { 0 }; + struct flashrom_layout *layout; + + /* + * Tricking the dummyflasher by asking to emulate W25Q128FV but giving to it + * mock chip with FEATURE_NO_ERASE. + * As long as chip size is the same, this is fine. + */ + struct flashchip mock_chip = chip_no_erase; + const char *param_dup = "bus=spi,emulate=W25Q128FV"; + + setup_chip(&flashctx, &layout, &mock_chip, param_dup, &chip_io); + + /* See comment in write_chip_test_success */ + const char *const filename = "-"; + unsigned long size = mock_chip.total_size * 1024; + uint8_t *const newcontents = malloc(size); + assert_non_null(newcontents); + + printf("Write chip operation started.\n"); + assert_int_equal(0, read_buf_from_file(newcontents, size, filename)); + assert_int_equal(0, flashrom_image_write(&flashctx, newcontents, size, NULL)); + assert_int_equal(0, flashrom_image_verify(&flashctx, newcontents, size)); + printf("Write chip operation done.\n"); + + teardown(&layout); + + free(newcontents); +} + void write_nonaligned_region_with_dummyflasher_test_success(void **state) { (void) state; /* unused */ diff --git a/tests/tests.c b/tests/tests.c index 165390a..78f1b47 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -498,6 +498,7 @@ cmocka_unit_test(read_chip_with_dummyflasher_test_success), cmocka_unit_test(write_chip_test_success), cmocka_unit_test(write_chip_with_dummyflasher_test_success), + cmocka_unit_test(write_chip_feature_no_erase), cmocka_unit_test(write_nonaligned_region_with_dummyflasher_test_success), cmocka_unit_test(verify_chip_test_success), cmocka_unit_test(verify_chip_with_dummyflasher_test_success), diff --git a/tests/tests.h b/tests/tests.h index 8cf8be0..3536d1d 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -86,6 +86,7 @@ void read_chip_with_dummyflasher_test_success(void **state); void write_chip_test_success(void **state); void write_chip_with_dummyflasher_test_success(void **state); +void write_chip_feature_no_erase(void **state); void write_nonaligned_region_with_dummyflasher_test_success(void **state); void verify_chip_test_success(void **state); void verify_chip_with_dummyflasher_test_success(void **state); -- To view, visit https://review.coreboot.org/c/flashrom/+/84203?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I582fe00da0715e9b5e92fcc9d15d5a90a2615117 Gerrit-Change-Number: 84203 Gerrit-PatchSet: 7 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

[M] Change in flashrom[main]: More adequate progress
by Sergii Dmytruk (Code Review) 14 Sep '24

14 Sep '24

Attention is currently required from: Aarya, Anastasia Klimchuk, Nikolai Artemiev, Peter Marheine. Sergii Dmytruk has posted comments on this change by Sergii Dmytruk. ( https://review.coreboot.org/c/flashrom/+/84102?usp=email ) Change subject: More adequate progress ...................................................................... Patch Set 1: (7 comments) Patchset: PS1: > Sergii, thanks a lot for the patch, that's a great help. […] Sure, please go for it. I can review and provide more details as needed, just don't have time/need to finish this properly. Commit Message: https://review.coreboot.org/c/flashrom/+/84102/comment/7fa24368_d2e255aa?us… : PS1, Line 46: : CLI shares terminal with the rest of the code > What does it mean? I don't understand :( I just meant that flashrom can print log output at any point (like it prints `DONE something`) so CLI implementation can't assume that nothing interferes with its output. File dummyflasher.c: https://review.coreboot.org/c/flashrom/+/84102/comment/2af2d3e3_1145c745?us… : PS1, Line 58: unsigned long long delay_ns; > This is changing to nano seconds, is it to be able to set a more fine-grained delay? […] I think insufficient range was my first guess, before I noticed `(1000000 * 8) / freq` below. If frequency of 1Hz isn't necessary in 32-bit builds, then `long int` should be enough. 64MHz wasn't supported because `(1000000 * 8) / 64000000` results in zero delay (like any frequency above 8MHz). Type of `freq` below doesn't need to change in any case. File include/flash.h: https://review.coreboot.org/c/flashrom/+/84102/comment/74c4eb9a_2775229e?us… : PS1, Line 544: struct stage_progress { : size_t current; : size_t total; : }; > And can this go to libflashrom. […] This is not part of API, nothing in `libflashrom.h` uses this structure. https://review.coreboot.org/c/flashrom/+/84102/comment/8edbf977_85a786b5?us… : PS1, Line 587: struct stage_progress stage_progress[FLASHROM_PROGRESS_NR]; > Maybe this can go inside `struct flashrom_progress` ? Why do we need two structs on the same level, […] That would change API and ABI. This was added for internal use to keep track of progress, while `struct flashrom_progress` is a parameter for a user callback which can also be allocated on client side and changed at will, potentially messing with flashrom's view of things. File linux_mtd.c: https://review.coreboot.org/c/flashrom/+/84102/comment/4caa98f3_c3707e9c?us… : PS1, Line 298: update_progress(flash, FLASHROM_PROGRESS_ERASE, u + data->erasesize, len); > Why you removed this? Should it be […] It's for erasing which I think is handled uniformly in `erasure_layout.c`. File spi.c: https://review.coreboot.org/c/flashrom/+/84102/comment/a9799407_12dbdc0e?us… : PS1, Line 118: update_progress(flash, FLASHROM_PROGRESS_READ, start - start_address + to_read, end_address); > Why removing, should this be […] `flash->mst->spi.read` updates progress, keeping this line results in progress on reading reaching over 100%. This could be a single place to report read progress (like for erasing) and drop all similar calls in programmers, but granularity would likely be too coarse (erasing is different, there can be a command to erase the whole chip at once and you can only report it in one way: 0% -> 100%). -- To view, visit https://review.coreboot.org/c/flashrom/+/84102?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: If1e40fc97f443c4f0c0501cef11cff1f3f84c051 Gerrit-Change-Number: 84102 Gerrit-PatchSet: 1 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> 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-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Sat, 14 Sep 2024 16:33:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit September 2024