Attention is currently required from: Vadim Bendebury, Edward O'Callaghan, Nikolai Artemiev. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64405 )
Change subject: Add W25Q512NW-IM ID to flashrom ......................................................................
Patch Set 1:
(3 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/64405/comment/65f15679_2e806b68 PS1, Line 15: C
Original-
Hmmm, so this and CL:3171890 have the same Change-Id, although they're on different Gerrit instances. This means we need to keep this Change-Id line as-is, and add the one for Original-Change-Id:
Original-Change-Id: I9debeda01d77444a5ebe9808ff80a337f320ef65 Original-Signed-off-by: Atul Dhudase adhudase@codeaurora.org Original-Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/+... Original-Reviewed-by: Shelley Chen shchen@chromium.org Original-Reviewed-by: Vadim Bendebury vbendeb@chromium.org Original-Tested-by: Shelley Chen shchen@chromium.org Original-Commit-Queue: Shelley Chen shchen@chromium.org (cherry picked from commit facb282e8939b8e4ad15d2478ed9ef86d98aed61) Change-Id: I9debeda01d77444a5ebe9808ff80a337f320ef65 Signed-off-by: Nikolai Artemiev nartemiev@google.com
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/64405/comment/ada4e639_51c00f63 PS1, Line 17973: .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_4BA | FEATURE_4BA_ENTER_WREN | FEATURE_4BA_READ | FEATURE_4BA_FAST_READ, The 4BA stuff looks suspicious... `FEATURE_4BA` implies `FEATURE_4BA_ENTER` which is mutually exclusive with `FEATUER_4BA_ENTER_WREN` (c.f. spi25.c function `spi_enter_exit_4ba()`). If I am looking at the right datasheet, it doesn't mention anything about WREN being needed before entering 4BA mode: https://imgur.com/6RVc3w4.png
Also, `FEATURE_4BA` implies `FEATURE_4BA_READ` and `FEATURE_4BA_FAST_READ`, so there's no need to explicitly mention them again.
TL;DR: Please simplify feature_bits to:
.feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_4BA,
https://review.coreboot.org/c/flashrom/+/64405/comment/a0d6afa5_a415b6f8 PS1, Line 17965: { : .vendor = "Winbond", : .name = "W25Q512NW-IM", : .bustype = BUS_SPI, : .manufacture_id = WINBOND_NEX_ID, : .model_id = WINBOND_NEX_W25Q512NW_IM, : .total_size = 64 * 1024, : .page_size = 256, : .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_4BA | FEATURE_4BA_ENTER_WREN | FEATURE_4BA_READ | FEATURE_4BA_FAST_READ, : .tested = TEST_OK_PREW, : .probe = probe_spi_rdid, : .probe_timing = TIMING_ZERO, : .block_erasers = : { : { : .eraseblocks = { {4 * 1024, 16384} }, : .block_erase = spi_block_erase_21, : }, { : .eraseblocks = { {4 * 1024, 16384} }, : .block_erase = spi_block_erase_20, : }, { : .eraseblocks = { {32 * 1024, 2048} }, : .block_erase = spi_block_erase_52, : }, { : .eraseblocks = { {64 * 1024, 1024} }, : .block_erase = spi_block_erase_dc, : }, { : .eraseblocks = { {64 * 1024, 1024} }, : .block_erase = spi_block_erase_d8, : }, { : .eraseblocks = { {64 * 1024 * 1024, 1} }, : .block_erase = spi_block_erase_60, : }, { : .eraseblocks = { {64 * 1024 * 1024, 1} }, : .block_erase = spi_block_erase_c7, : } : }, : .unlock = spi_disable_blockprotect, : .write = spi_chip_write_256, : .read = spi_chip_read, : .voltage = {1650, 1950}, : }, Please indent with tabs