October 2021 - flashrom-gerrit

Change in flashrom[master]: ich_descriptors: Add explicit checks for all chipsets
by Nico Huber (Code Review) 01 Oct '21

01 Oct '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/57793 ) Change subject: ich_descriptors: Add explicit checks for all chipsets ...................................................................... ich_descriptors: Add explicit checks for all chipsets This partially undoes changes made in commit cd9b7b427 (ich_descriptors: Normalize chipset detection) to re-add explicit matching of each chipset with one or more strap length values. Since ranges are checked explicitly, the `warn_if` parameter to warn_peculiar_desc() is no longer necessary and is removed. Change-Id: Ica49477492876810a6fa212768b1ab9e8c12001f Signed-off-by: David Hendricks <ddaveh(a)amazon.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/57793 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M ich_descriptors.c 1 file changed, 19 insertions(+), 10 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve diff --git a/ich_descriptors.c b/ich_descriptors.c index d56ade7..7927493 100644 --- a/ich_descriptors.c +++ b/ich_descriptors.c @@ -912,10 +912,8 @@ msg_pdbg2("\n"); } -static inline void warn_peculiar_desc(const bool warn_if, const char *const name) +static inline void warn_peculiar_desc(const char *const name) { - if (!warn_if) - return; msg_pwarn("Peculiar flash descriptor, assuming %s compatibility.\n", name); } @@ -933,15 +931,19 @@ return CHIPSET_ICH9; if (content->ISL <= 10) return CHIPSET_ICH10; + if (content->ISL <= 16) + return CHIPSET_5_SERIES_IBEX_PEAK; if (content->FLMAP2 == 0) { if (content->ISL == 19) return CHIPSET_APOLLO_LAKE; - warn_peculiar_desc(content->ISL != 23, "Gemini Lake"); + if (content->ISL == 23) + return CHIPSET_GEMINI_LAKE; + warn_peculiar_desc("Gemini Lake"); return CHIPSET_GEMINI_LAKE; } if (content->ISL <= 80) return CHIPSET_C620_SERIES_LEWISBURG; - warn_peculiar_desc(content->ISL != 16, "Ibex Peak"); + warn_peculiar_desc("Ibex Peak"); return CHIPSET_5_SERIES_IBEX_PEAK; } else if (upper->MDTBA == 0x00) { if (content->ICCRIBA < 0x31 && content->FMSBA < 0x30) { @@ -949,18 +951,25 @@ return CHIPSET_BAYTRAIL; if (content->MSL <= 1 && content->ISL <= 18) return CHIPSET_6_SERIES_COUGAR_POINT; - warn_peculiar_desc(content->MSL != 1 || content->ISL != 21, "Lynx Point"); + if (content->MSL <= 1 && content->ISL <= 21) + return CHIPSET_8_SERIES_LYNX_POINT; + warn_peculiar_desc("Lynx Point"); return CHIPSET_8_SERIES_LYNX_POINT; } if (content->NM == 6) { - warn_peculiar_desc(content->ICCRIBA > 0x34, "C620 series"); + if (content->ICCRIBA <= 0x34) + return CHIPSET_C620_SERIES_LEWISBURG; + warn_peculiar_desc("C620 series"); return CHIPSET_C620_SERIES_LEWISBURG; } - warn_peculiar_desc(content->ICCRIBA != 0x31, "100 series"); + if (content->ICCRIBA == 0x31) + return CHIPSET_100_SERIES_SUNRISE_POINT; + warn_peculiar_desc("100 series"); return CHIPSET_100_SERIES_SUNRISE_POINT; } else { - if (content->ICCRIBA != 0x34) - msg_pwarn("Unknown flash descriptor, assuming 300 series compatibility.\n"); + if (content->ICCRIBA == 0x34) + return CHIPSET_300_SERIES_CANNON_POINT; + msg_pwarn("Unknown flash descriptor, assuming 300 series compatibility.\n"); return CHIPSET_300_SERIES_CANNON_POINT; } } -- To view, visit https://review.coreboot.org/c/flashrom/+/57793 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ica49477492876810a6fa212768b1ab9e8c12001f Gerrit-Change-Number: 57793 Gerrit-PatchSet: 5 Gerrit-Owner: David Hendricks Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com> Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com> 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-MessageType: merged

1 0

Change in flashrom[master]: flashrom.8: carve out `csgpiol` into its own section
by Nico Huber (Code Review) 01 Oct '21

01 Oct '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/57893 ) Change subject: flashrom.8: carve out `csgpiol` into its own section ...................................................................... flashrom.8: carve out `csgpiol` into its own section Documentation for `csgpiol` was put into the generic programmer options section. Move it to its own section. Change-Id: Ic7379331d36b3068eacde5a983b4ccb3afc56c51 Signed-off-by: Michael Niewöhner <foss(a)mniewoehner.de> Reviewed-on: https://review.coreboot.org/c/flashrom/+/57893 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> --- M flashrom.8.tmpl 1 file changed, 11 insertions(+), 6 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl index 788e254..de93453 100644 --- a/flashrom.8.tmpl +++ b/flashrom.8.tmpl @@ -840,9 +840,9 @@ Flyswatter/Flyswatter 2. .sp An optional parameter specifies the controller -type, channel/interface/port and GPIO-based chip select it should support. For that you have to use the +type, channel/interface/port it should support. For that you have to use the .sp -.B " flashrom \-p ft2232_spi:type=model,port=interface,csgpiol=gpio" +.B " flashrom \-p ft2232_spi:type=model,port=interface" .sp syntax where .B model @@ -853,10 +853,7 @@ " or " google-servo-v2-legacy .B interface can be -.BR A ", " B ", " C ", or " D -and -.B csgpiol -can be a number between 0 and 3, denoting GPIOL0-GPIOL3 correspondingly. +.BR A ", " B ", " C ", or " D . The default model is .B 4232H the default interface is @@ -884,6 +881,14 @@ .B " flashrom \-p ft2232_spi:divisor=div" .sp syntax. +.sp +Using the parameter +.B csgpiol +an additional CS# pin can be chosen, where the value can be a number between 0 and 3, denoting GPIOL0-GPIOL3 +correspondingly. Example: +.sp +.B " flashrom \-p ft2232_spi:csgpiol=3" +.sp .SS .BR "serprog " programmer .IP -- To view, visit https://review.coreboot.org/c/flashrom/+/57893 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic7379331d36b3068eacde5a983b4ccb3afc56c51 Gerrit-Change-Number: 57893 Gerrit-PatchSet: 5 Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> 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: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: ft2232_spi: prevent use of reserved pins on some programmers
by Nico Huber (Code Review) 01 Oct '21

01 Oct '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/57809 ) Change subject: ft2232_spi: prevent use of reserved pins on some programmers ...................................................................... ft2232_spi: prevent use of reserved pins on some programmers On some programmers an output buffer needs to be enabled by pulling a gpio high/low. This gpio can not be used for `csgpiol`. Prevent this by printing an error. Change-Id: Ied450fa5ef358153adefec3beabc63a62c9f60cd Signed-off-by: Michael Niewöhner <foss(a)mniewoehner.de> Reviewed-on: https://review.coreboot.org/c/flashrom/+/57809 Reviewed-by: Nico Huber <nico.h(a)gmx.de> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M ft2232_spi.c 1 file changed, 12 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve diff --git a/ft2232_spi.c b/ft2232_spi.c index df156d6..110bdec 100644 --- a/ft2232_spi.c +++ b/ft2232_spi.c @@ -419,6 +419,9 @@ } free(arg); + /* Remember reserved pins before pindir gets modified. */ + const uint8_t rsv_bits = pindir & 0xf0; + arg = extract_programmer_param("port"); if (arg) { switch (toupper((unsigned char)*arg)) { @@ -478,6 +481,15 @@ return -2; } unsigned int pin = temp + 4; + + if (rsv_bits & 1 << pin) { + msg_perr("Error: Invalid GPIOL specified: \"%s\".\n" + "The pin is reserved on this programmer.\n", + arg); + free(arg); + return -2; + } + cs_bits |= 1 << pin; pindir |= 1 << pin; } -- To view, visit https://review.coreboot.org/c/flashrom/+/57809 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ied450fa5ef358153adefec3beabc63a62c9f60cd Gerrit-Change-Number: 57809 Gerrit-PatchSet: 9 Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> 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: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: flashchips.c: mark EN25F10 as TEST_OK_PREW
by Nico Huber (Code Review) 01 Oct '21

01 Oct '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/58031 ) Change subject: flashchips.c: mark EN25F10 as TEST_OK_PREW ...................................................................... flashchips.c: mark EN25F10 as TEST_OK_PREW As reported by Wolf Dieter Brandt in his e-mail from 09.Aug.2021. Change-Id: I0c19f84780e7fa3699fd706f8e105fc5937ba8bf Signed-off-by: Simon Buhrow <simon.buhrow(a)posteo.de> Reviewed-on: https://review.coreboot.org/c/flashrom/+/58031 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> --- M flashchips.c 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved diff --git a/flashchips.c b/flashchips.c index 3187a49..434c46d 100644 --- a/flashchips.c +++ b/flashchips.c @@ -4433,7 +4433,7 @@ .total_size = 128, .page_size = 256, .feature_bits = FEATURE_WRSR_WREN, - .tested = TEST_UNTESTED, + .tested = TEST_OK_PREW, .probe = probe_spi_rdid, .probe_timing = TIMING_ZERO, .block_erasers = -- To view, visit https://review.coreboot.org/c/flashrom/+/58031 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0c19f84780e7fa3699fd706f8e105fc5937ba8bf Gerrit-Change-Number: 58031 Gerrit-PatchSet: 2 Gerrit-Owner: Simon Buhrow 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: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: tests: Revise mock chip definition and usage
by Nico Huber (Code Review) 01 Oct '21

01 Oct '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/57437 ) Change subject: tests: Revise mock chip definition and usage ...................................................................... tests: Revise mock chip definition and usage This patch is doing few things: 1) Makes chip definitions static global so that they can be reused between test functions. 2) Promotes existing mock chip from 8KiB to 8MiB and eraseblocks are expanded accordingly. Old value of 8KiB was very small and it was confusing. Mock chip looks more realistic now. 3) Uses KiB and MiB macros from flash.h for mock chip definition 4) Renames CHIP_TOTAL_SIZE to MOCK_CHIP_SIZE to avoid confusion (there is also a W25Q128.V chip in the tests) 5) Makes chip definitions const so that every test can work on a fresh copy on the stack. BUG=b:181803212 TEST=builds and ninja test Change-Id: Ia9b5fc71e30610684e68e9aca9fb1970da8f840a Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/57437 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M tests/chip.c 1 file changed, 60 insertions(+), 117 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved diff --git a/tests/chip.c b/tests/chip.c index 2f6581f..54e14d2 100644 --- a/tests/chip.c +++ b/tests/chip.c @@ -21,11 +21,11 @@ #include "flash.h" #include "programmer.h" -#define CHIP_TOTAL_SIZE 8192 +#define MOCK_CHIP_SIZE (8*MiB) static struct { unsigned int unlock_calls; /* how many times unlock function was called */ - uint8_t buf[CHIP_TOTAL_SIZE]; /* buffer of total size of chip, to emulate a chip */ + uint8_t buf[MOCK_CHIP_SIZE]; /* buffer of total size of chip, to emulate a chip */ } g_chip_state = { .unlock_calls = 0, .buf = { 0 }, @@ -39,7 +39,7 @@ return 1; } - assert_in_range(start + len, 0, CHIP_TOTAL_SIZE); + assert_in_range(start + len, 0, MOCK_CHIP_SIZE); memcpy(buf, &g_chip_state.buf[start], len); return 0; @@ -53,7 +53,7 @@ return 1; } - assert_in_range(start + len, 0, CHIP_TOTAL_SIZE); + assert_in_range(start + len, 0, MOCK_CHIP_SIZE); memcpy(&g_chip_state.buf[start], buf, len); return 0; @@ -80,7 +80,7 @@ return 1; } - assert_in_range(blockaddr + blocklen, 0, CHIP_TOTAL_SIZE); + assert_in_range(blockaddr + blocklen, 0, MOCK_CHIP_SIZE); memset(&g_chip_state.buf[blockaddr], 0xff, blocklen); return 0; @@ -96,7 +96,7 @@ printf("Creating layout with one included region... "); assert_int_equal(0, flashrom_layout_new(layout)); /* One region which covers total size of chip. */ - assert_int_equal(0, flashrom_layout_add_region(*layout, 0, chip->total_size * 1024 - 1, "region")); + assert_int_equal(0, flashrom_layout_add_region(*layout, 0, chip->total_size * KiB - 1, "region")); assert_int_equal(0, flashrom_layout_include_region(*layout, "region")); flashrom_layout_set(flash, *layout); @@ -125,33 +125,60 @@ printf("done\n"); } +static const struct flashchip chip_8MiB = { + .vendor = "aklm", + .total_size = MOCK_CHIP_SIZE / KiB, + .tested = TEST_OK_PREW, + .read = read_chip, + .write = write_chip, + .unlock = unlock_chip, + .block_erasers = + {{ + /* All blocks within total size of the chip. */ + .eraseblocks = { {2 * MiB, 4} }, + .block_erase = block_erase_chip, + }}, +}; + +/* Setup the struct for W25Q128.V, all values come from flashchips.c */ +static const struct flashchip chip_W25Q128_V = { + .vendor = "aklm&dummyflasher", + .total_size = 16 * 1024, + .tested = TEST_OK_PREW, + .read = spi_chip_read, + .write = spi_chip_write_256, + .unlock = spi_disable_blockprotect, + .block_erasers = + { + { + .eraseblocks = { {4 * 1024, 4096} }, + .block_erase = spi_block_erase_20, + }, { + .eraseblocks = { {32 * 1024, 512} }, + .block_erase = spi_block_erase_52, + }, { + .eraseblocks = { {64 * 1024, 256} }, + .block_erase = spi_block_erase_d8, + }, { + .eraseblocks = { {16 * 1024 * 1024, 1} }, + .block_erase = spi_block_erase_60, + }, { + .eraseblocks = { {16 * 1024 * 1024, 1} }, + .block_erase = spi_block_erase_c7, + } + }, +}; + void erase_chip_test_success(void **state) { (void) state; /* unused */ - struct flashchip chip = { - .vendor = "aklm", - /* - * Total size less than 16 to skip some steps - * in flashrom.c#prepare_flash_access. - */ - .total_size = 8, - .tested = TEST_OK_PREW, - .read = read_chip, - .write = write_chip, - .unlock = unlock_chip, - .block_erasers = - {{ - /* All blocks within total size of the chip. */ - .eraseblocks = { {2 * 1024, 4} }, - .block_erase = block_erase_chip, - }}, - }; struct flashrom_flashctx flash = { 0 }; struct flashrom_layout *layout; + struct flashchip mock_chip = chip_8MiB; const char *param = ""; /* Default values for all params. */ - setup_chip(&flash, &layout, &chip, param); + setup_chip(&flash, &layout, &mock_chip, param); printf("Erase chip operation started.\n"); assert_int_equal(0, do_erase(&flash)); @@ -164,49 +191,16 @@ { (void) state; /* unused */ - struct flashchip chip = { - .vendor = "aklm&dummyflasher", - /* - * Setup the values for W25Q128.V because we ask dummyflasher - * to emulate this chip. All operations: read/write/unlock/erase - * are real, not mocks, and they are expected to be handled by - * dummyflasher. - */ - .total_size = 16 * 1024, - .tested = TEST_OK_PREW, - .read = spi_chip_read, - .write = spi_chip_write_256, - .unlock = spi_disable_blockprotect, - .block_erasers = - { - { - .eraseblocks = { {4 * 1024, 4096} }, - .block_erase = spi_block_erase_20, - }, { - .eraseblocks = { {32 * 1024, 512} }, - .block_erase = spi_block_erase_52, - }, { - .eraseblocks = { {64 * 1024, 256} }, - .block_erase = spi_block_erase_d8, - }, { - .eraseblocks = { {16 * 1024 * 1024, 1} }, - .block_erase = spi_block_erase_60, - }, { - .eraseblocks = { {16 * 1024 * 1024, 1} }, - .block_erase = spi_block_erase_c7, - } - }, - }; - struct flashrom_flashctx flash = { 0 }; struct flashrom_layout *layout; + struct flashchip mock_chip = chip_W25Q128_V; /* - * Dummyflasher is capable to emulate a chip, so we ask it to do this. + * Dummyflasher is capable to emulate W25Q128.V, so we ask it to do this. * Nothing to mock, dummy is taking care of this already. */ char *param_dup = strdup("bus=spi,emulate=W25Q128FV"); - setup_chip(&flash, &layout, &chip, param_dup); + setup_chip(&flash, &layout, &mock_chip, param_dup); printf("Erase chip operation started.\n"); assert_int_equal(0, do_erase(&flash)); @@ -221,30 +215,12 @@ { (void) state; /* unused */ - struct flashchip chip = { - .vendor = "aklm", - /* - * Total size less than 16 to skip some steps - * in flashrom.c#prepare_flash_access. - */ - .total_size = 8, - .tested = TEST_OK_PREW, - .read = read_chip, - .write = write_chip, - .unlock = unlock_chip, - .block_erasers = - {{ - /* All blocks within total size of the chip. */ - .eraseblocks = { {2 * 1024, 4} }, - .block_erase = block_erase_chip, - }}, - }; - struct flashrom_flashctx flash = { 0 }; struct flashrom_layout *layout; + struct flashchip mock_chip = chip_8MiB; const char *param = ""; /* Default values for all params. */ - setup_chip(&flash, &layout, &chip, param); + setup_chip(&flash, &layout, &mock_chip, param); const char *const filename = "read_chip.test"; @@ -259,49 +235,16 @@ { (void) state; /* unused */ - struct flashchip chip = { - .vendor = "aklm&dummyflasher", - /* - * Setup the values for W25Q128.V because we ask dummyflasher - * to emulate this chip. All operations: read/write/unlock/erase - * are real, not mocks, and they are expected to be handled by - * dummyflasher. - */ - .total_size = 16 * 1024, - .tested = TEST_OK_PREW, - .read = spi_chip_read, - .write = spi_chip_write_256, - .unlock = spi_disable_blockprotect, - .block_erasers = - { - { - .eraseblocks = { {4 * 1024, 4096} }, - .block_erase = spi_block_erase_20, - }, { - .eraseblocks = { {32 * 1024, 512} }, - .block_erase = spi_block_erase_52, - }, { - .eraseblocks = { {64 * 1024, 256} }, - .block_erase = spi_block_erase_d8, - }, { - .eraseblocks = { {16 * 1024 * 1024, 1} }, - .block_erase = spi_block_erase_60, - }, { - .eraseblocks = { {16 * 1024 * 1024, 1} }, - .block_erase = spi_block_erase_c7, - } - }, - }; - struct flashrom_flashctx flash = { 0 }; struct flashrom_layout *layout; + struct flashchip mock_chip = chip_W25Q128_V; /* - * Dummyflasher is capable to emulate a chip, so we ask it to do this. + * Dummyflasher is capable to emulate W25Q128.V, so we ask it to do this. * Nothing to mock, dummy is taking care of this already. */ char *param_dup = strdup("bus=spi,emulate=W25Q128FV"); - setup_chip(&flash, &layout, &chip, param_dup); + setup_chip(&flash, &layout, &mock_chip, param_dup); const char *const filename = "read_chip.test"; -- To view, visit https://review.coreboot.org/c/flashrom/+/57437 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia9b5fc71e30610684e68e9aca9fb1970da8f840a Gerrit-Change-Number: 57437 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-MessageType: merged

1 0

Change in flashrom[master]: tests: Revise mock chip definition and usage
by Nico Huber (Code Review) 01 Oct '21

01 Oct '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/+/57437 ) Change subject: tests: Revise mock chip definition and usage ...................................................................... Patch Set 5: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/57437 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia9b5fc71e30610684e68e9aca9fb1970da8f840a Gerrit-Change-Number: 57437 Gerrit-PatchSet: 5 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: 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, 01 Oct 2021 11:34:32 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Add tests to read from chip
by Nico Huber (Code Review) 01 Oct '21

01 Oct '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/57326 ) Change subject: tests: Add tests to read from chip ...................................................................... tests: Add tests to read from chip Two tests cover the code which performs do_read 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: Ia57781ebc670c7bd6197e56fe8a20651a425c756 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/57326 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M tests/chip.c M tests/io_mock.h M tests/meson.build M tests/tests.c M tests/tests.h 5 files changed, 146 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved diff --git a/tests/chip.c b/tests/chip.c index 4a61ff8..2f6581f 100644 --- a/tests/chip.c +++ b/tests/chip.c @@ -216,3 +216,100 @@ free(param_dup); } + +void read_chip_test_success(void **state) +{ + (void) state; /* unused */ + + struct flashchip chip = { + .vendor = "aklm", + /* + * Total size less than 16 to skip some steps + * in flashrom.c#prepare_flash_access. + */ + .total_size = 8, + .tested = TEST_OK_PREW, + .read = read_chip, + .write = write_chip, + .unlock = unlock_chip, + .block_erasers = + {{ + /* All blocks within total size of the chip. */ + .eraseblocks = { {2 * 1024, 4} }, + .block_erase = block_erase_chip, + }}, + }; + + struct flashrom_flashctx flash = { 0 }; + struct flashrom_layout *layout; + const char *param = ""; /* Default values for all params. */ + + setup_chip(&flash, &layout, &chip, param); + + const char *const filename = "read_chip.test"; + + printf("Read chip operation started.\n"); + assert_int_equal(0, do_read(&flash, filename)); + printf("Read chip operation done.\n"); + + teardown(&layout); +} + +void read_chip_with_dummyflasher_test_success(void **state) +{ + (void) state; /* unused */ + + struct flashchip chip = { + .vendor = "aklm&dummyflasher", + /* + * Setup the values for W25Q128.V because we ask dummyflasher + * to emulate this chip. All operations: read/write/unlock/erase + * are real, not mocks, and they are expected to be handled by + * dummyflasher. + */ + .total_size = 16 * 1024, + .tested = TEST_OK_PREW, + .read = spi_chip_read, + .write = spi_chip_write_256, + .unlock = spi_disable_blockprotect, + .block_erasers = + { + { + .eraseblocks = { {4 * 1024, 4096} }, + .block_erase = spi_block_erase_20, + }, { + .eraseblocks = { {32 * 1024, 512} }, + .block_erase = spi_block_erase_52, + }, { + .eraseblocks = { {64 * 1024, 256} }, + .block_erase = spi_block_erase_d8, + }, { + .eraseblocks = { {16 * 1024 * 1024, 1} }, + .block_erase = spi_block_erase_60, + }, { + .eraseblocks = { {16 * 1024 * 1024, 1} }, + .block_erase = spi_block_erase_c7, + } + }, + }; + + struct flashrom_flashctx flash = { 0 }; + struct flashrom_layout *layout; + /* + * Dummyflasher is capable to emulate a chip, so we ask it to do this. + * Nothing to mock, dummy is taking care of this already. + */ + char *param_dup = strdup("bus=spi,emulate=W25Q128FV"); + + setup_chip(&flash, &layout, &chip, param_dup); + + const char *const filename = "read_chip.test"; + + printf("Read chip operation started.\n"); + assert_int_equal(0, do_read(&flash, filename)); + printf("Read chip operation done.\n"); + + teardown(&layout); + + free(param_dup); +} diff --git a/tests/io_mock.h b/tests/io_mock.h index adb5f3b..a285e53 100644 --- a/tests/io_mock.h +++ b/tests/io_mock.h @@ -53,6 +53,9 @@ /* Linux I2C interface constants, avoiding linux/i2c-dev.h */ #define I2C_SLAVE 0x0703 +/* Always return success for tests. */ +#define S_ISREG(x) 0 + struct io_mock { void *state; diff --git a/tests/meson.build b/tests/meson.build index 53885a8..8019bd3 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -39,8 +39,14 @@ '-Wl,--wrap=write', '-Wl,--wrap=fopen', '-Wl,--wrap=fopen64', + '-Wl,--wrap=fwrite', + '-Wl,--wrap=fflush', '-Wl,--wrap=stat', '-Wl,--wrap=stat64', + '-Wl,--wrap=fstat', + '-Wl,--wrap=fstat64', + '-Wl,--wrap=fileno', + '-Wl,--wrap=fsync', '-Wl,--wrap=fread', '-Wl,--wrap=fgets', '-Wl,--wrap=fclose', diff --git a/tests/tests.c b/tests/tests.c index 5ca5b66..ac59470 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -154,6 +154,18 @@ return 0; } +int __wrap_fstat(int fd, void *buf) +{ + LOG_ME; + return 0; +} + +int __wrap_fstat64(int fd, void *buf) +{ + LOG_ME; + return 0; +} + char *__wrap_fgets(char *buf, int len, FILE *fp) { LOG_ME; @@ -170,6 +182,30 @@ return 0; } +size_t __wrap_fwrite(const void *ptr, size_t size, size_t nmemb, FILE *fp) +{ + LOG_ME; + return nmemb; +} + +int __wrap_fflush(FILE *fp) +{ + LOG_ME; + return 0; +} + +int __wrap_fileno(FILE *fp) +{ + LOG_ME; + return MOCK_HANDLE; +} + +int __wrap_fsync(int fd) +{ + LOG_ME; + return 0; +} + int __wrap_setvbuf(FILE *fp, char *buf, int type, size_t size) { LOG_ME; @@ -357,6 +393,8 @@ const struct CMUnitTest chip_tests[] = { cmocka_unit_test(erase_chip_test_success), cmocka_unit_test(erase_chip_with_dummyflasher_test_success), + cmocka_unit_test(read_chip_test_success), + cmocka_unit_test(read_chip_with_dummyflasher_test_success), }; ret |= cmocka_run_group_tests_name("chip.c tests", chip_tests, NULL, NULL); diff --git a/tests/tests.h b/tests/tests.h index 8e1b6da..c142b45 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -59,5 +59,7 @@ /* chip.c */ void erase_chip_test_success(void **state); void erase_chip_with_dummyflasher_test_success(void **state); +void read_chip_test_success(void **state); +void read_chip_with_dummyflasher_test_success(void **state); #endif /* TESTS_H */ 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/+/57326 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia57781ebc670c7bd6197e56fe8a20651a425c756 Gerrit-Change-Number: 57326 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-MessageType: merged

1 0

Change in flashrom[master]: tests: Extract setup and teardown for chip tests
by Nico Huber (Code Review) 01 Oct '21

01 Oct '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/57436 ) Change subject: tests: Extract setup and teardown for chip tests ...................................................................... tests: Extract setup and teardown for chip tests Steps to setup and teardown for a chip test are repeated for every test, so they can be extracted into their own functions. BUG=b:181803212 TEST=builds and ninja test Change-Id: If59315646f06344664df08b145866d9ce846d751 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/57436 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> --- M tests/chip.c 1 file changed, 47 insertions(+), 55 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved diff --git a/tests/chip.c b/tests/chip.c index 0cb05ad..4a61ff8 100644 --- a/tests/chip.c +++ b/tests/chip.c @@ -86,6 +86,45 @@ return 0; } +static void setup_chip(struct flashrom_flashctx *flash, struct flashrom_layout **layout, + struct flashchip *chip, const char *programmer_param) +{ + flash->chip = chip; + + g_chip_state.unlock_calls = 0; + + printf("Creating layout with one included region... "); + assert_int_equal(0, flashrom_layout_new(layout)); + /* One region which covers total size of chip. */ + assert_int_equal(0, flashrom_layout_add_region(*layout, 0, chip->total_size * 1024 - 1, "region")); + assert_int_equal(0, flashrom_layout_include_region(*layout, "region")); + + flashrom_layout_set(flash, *layout); + printf("done\n"); + + /* + * We need some programmer (any), and dummy is a very good one, + * because it doesn't need any mocking. So no extra complexity + * from a programmer side, and test can focus on working with the chip. + */ + printf("Dummyflasher initialising with param=\"%s\"... ", programmer_param); + assert_int_equal(0, programmer_init(&programmer_dummy, programmer_param)); + /* Assignment below normally happens while probing, but this test is not probing. */ + flash->mst = &registered_masters[0]; + printf("done\n"); +} + +static void teardown(struct flashrom_layout **layout) +{ + printf("Dummyflasher shutdown... "); + assert_int_equal(0, programmer_shutdown()); + printf("done\n"); + + printf("Releasing layout... "); + flashrom_layout_release(*layout); + printf("done\n"); +} + void erase_chip_test_success(void **state) { (void) state; /* unused */ @@ -108,43 +147,17 @@ .block_erase = block_erase_chip, }}, }; - struct flashrom_flashctx flash = { - .chip = &chip, - }; - + struct flashrom_flashctx flash = { 0 }; struct flashrom_layout *layout; + const char *param = ""; /* Default values for all params. */ - printf("Creating layout with one included region... "); - assert_int_equal(0, flashrom_layout_new(&layout)); - /* One region which covers total size of chip. */ - assert_int_equal(0, flashrom_layout_add_region(layout, 0, chip.total_size * 1024 - 1, "region")); - assert_int_equal(0, flashrom_layout_include_region(layout, "region")); - - flashrom_layout_set(&flash, layout); - printf("done\n"); - - /* - * We need some programmer (any), and dummy is a very good one, - * because it doesn't need any mocking. So no extra complexity - * from a programmer side, and test can focus on the code which - * erases the chip. - */ - const char *param = ""; /* default values for all params */ - printf("Dummyflasher initialising... "); - assert_int_equal(0, programmer_init(&programmer_dummy, param)); - printf("done\n"); + setup_chip(&flash, &layout, &chip, param); printf("Erase chip operation started.\n"); assert_int_equal(0, do_erase(&flash)); printf("Erase chip operation done.\n"); - printf("Dummyflasher shutdown... "); - assert_int_equal(0, programmer_shutdown()); - printf("done\n"); - - printf("Releasing layout... "); - flashrom_layout_release(layout); - printf("done\n"); + teardown(&layout); } void erase_chip_with_dummyflasher_test_success(void **state) @@ -184,43 +197,22 @@ } }, }; - struct flashrom_flashctx flash = { - .chip = &chip, - }; + struct flashrom_flashctx flash = { 0 }; struct flashrom_layout *layout; - - printf("Creating layout with one included region... "); - assert_int_equal(0, flashrom_layout_new(&layout)); - /* One region which covers total size of chip. */ - assert_int_equal(0, flashrom_layout_add_region(layout, 0, chip.total_size * 1024 - 1, "region")); - assert_int_equal(0, flashrom_layout_include_region(layout, "region")); - - flashrom_layout_set(&flash, layout); - printf("done\n"); - /* * Dummyflasher is capable to emulate a chip, so we ask it to do this. * Nothing to mock, dummy is taking care of this already. */ char *param_dup = strdup("bus=spi,emulate=W25Q128FV"); - printf("Dummyflasher initialising to emulate W25Q128FV... "); - assert_int_equal(0, programmer_init(&programmer_dummy, param_dup)); - /* Assignment below normally happens while probing, but this test is not probing. */ - flash.mst = &registered_masters[0]; - printf("done\n"); + + setup_chip(&flash, &layout, &chip, param_dup); printf("Erase chip operation started.\n"); assert_int_equal(0, do_erase(&flash)); printf("Erase chip operation done.\n"); - printf("Dummyflasher shutdown... "); - assert_int_equal(0, programmer_shutdown()); - printf("done\n"); - - printf("Releasing layout... "); - flashrom_layout_release(layout); - printf("done\n"); + teardown(&layout); free(param_dup); } 2 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/+/57436 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If59315646f06344664df08b145866d9ce846d751 Gerrit-Change-Number: 57436 Gerrit-PatchSet: 5 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-MessageType: merged

1 0

Change in flashrom[master]: nicintel_spi.c: Implement `set_sck_set_mosi` and `set_sck_get_miso`
by Nico Huber (Code Review) 01 Oct '21

01 Oct '21

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/49267 ) Change subject: nicintel_spi.c: Implement `set_sck_set_mosi` and `set_sck_get_miso` ...................................................................... nicintel_spi.c: Implement `set_sck_set_mosi` and `set_sck_get_miso` Tested on a 8086:1533 (i210 GbE), reads still return the same data. This cuts the time to read a Winbond W25Q80.V (1 MiB, SPI) from 66 seconds down to 48 seconds, i.e. a 37.5% increase in speed. Change-Id: I52a0ae5ff331ee2ed41652eb5c2ed7ebe7253d74 Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/49267 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Nico Huber <nico.h(a)gmx.de> --- M nicintel_spi.c 1 file changed, 27 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved diff --git a/nicintel_spi.c b/nicintel_spi.c index 2f4fd10..5dcdf7d 100644 --- a/nicintel_spi.c +++ b/nicintel_spi.c @@ -165,6 +165,19 @@ pci_mmio_writel(tmp, data->spibar + FLA); } +static void nicintel_bitbang_set_sck_set_mosi(int sck, int mosi, void *spi_data) +{ + struct nicintel_spi_data *data = spi_data; + uint32_t tmp; + + tmp = pci_mmio_readl(data->spibar + FLA); + tmp &= ~BIT(FL_SCK); + tmp &= ~BIT(FL_SI); + tmp |= (sck << FL_SCK); + tmp |= (mosi << FL_SI); + pci_mmio_writel(tmp, data->spibar + FLA); +} + static int nicintel_bitbang_get_miso(void *spi_data) { struct nicintel_spi_data *data = spi_data; @@ -175,10 +188,24 @@ return tmp; } +static int nicintel_bitbang_set_sck_get_miso(int sck, void *spi_data) +{ + struct nicintel_spi_data *data = spi_data; + uint32_t tmp; + + tmp = pci_mmio_readl(data->spibar + FLA); + tmp &= ~BIT(FL_SCK); + tmp |= (sck << FL_SCK); + pci_mmio_writel(tmp, data->spibar + FLA); + return (tmp >> FL_SO) & 0x1; +} + static const struct bitbang_spi_master bitbang_spi_master_nicintel = { .set_cs = nicintel_bitbang_set_cs, .set_sck = nicintel_bitbang_set_sck, .set_mosi = nicintel_bitbang_set_mosi, + .set_sck_set_mosi = nicintel_bitbang_set_sck_set_mosi, + .set_sck_get_miso = nicintel_bitbang_set_sck_get_miso, .get_miso = nicintel_bitbang_get_miso, .request_bus = nicintel_request_spibus, .release_bus = nicintel_release_spibus, -- To view, visit https://review.coreboot.org/c/flashrom/+/49267 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I52a0ae5ff331ee2ed41652eb5c2ed7ebe7253d74 Gerrit-Change-Number: 49267 Gerrit-PatchSet: 4 Gerrit-Owner: 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: Idwer Vollering <vidwer(a)gmail.com> Gerrit-CC: Shawn C <citypw(a)hardenedlinux.org> Gerrit-CC: Xiang W <wxjstz(a)126.com> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: mcp6x_spi.c: Implement `set_sck_set_mosi`
by Nico Huber (Code Review) 01 Oct '21

01 Oct '21

Attention is currently required from: Angel Pons. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/49266 ) Change subject: mcp6x_spi.c: Implement `set_sck_set_mosi` ...................................................................... Patch Set 3: -Code-Review (1 comment) Patchset: PS3: Meh, made the same mistake as in the parent commit... -- To view, visit https://review.coreboot.org/c/flashrom/+/49266 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie86cc9d4df6c7f0609c30ff00d6f2d77fbb8b836 Gerrit-Change-Number: 49266 Gerrit-PatchSet: 3 Gerrit-Owner: 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: Shawn C <citypw(a)hardenedlinux.org> Gerrit-CC: Xiang W <wxjstz(a)126.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Fri, 01 Oct 2021 11:26:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit October 2021