Attention is currently required from: Nico Huber, Thomas Walker. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52310 )
Change subject: Added support for Spansion/Cypress chip S25FL256L. ......................................................................
Patch Set 1:
(8 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/52310/comment/d1e017ff_a91c06e1 PS1, Line 7: Added support for Spansion/Cypress chip S25FL256L. Commit messages are written in present tense, do not end with a period and usually have a prefix. I'd suggest:
flashchips: Add Spansion/Cypress S25FL256L
https://review.coreboot.org/c/flashrom/+/52310/comment/69990759_7a45c3f5 PS1, Line 9: Testing has been conducted on ~60 individual chips and confirmed as working. One thing about write tests: flashrom compares the current contents of the flash chip to the provided data. If they are equal, flashrom does nothing.
For example, you may see something like this in a verbose log:
0x000000-0x000fff:S
The S tells us that the block was *S*kipped. To really test writes, one could use random data, e.g.
dd if=/dev/urandom bs=1M count=32 of=random.rom flashrom -p ... -w random.rom
If flashrom says VERIFIED, we're all good.
Patchset:
PS1: Welcome!
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/52310/comment/b82cff71_03b288fa PS1, Line 16387: | Please add spaces around `|`
https://review.coreboot.org/c/flashrom/+/52310/comment/af561af6_19e55bfd PS1, Line 16391: .block_erasers = Many block erasers are missing, and only the first entry is correct. This happened to work because flashrom only uses the other block erasers when the first one fails.
From the datasheet, the correct block erasers should be, in order:
.block_erasers = { { .eraseblocks = { {4 * 1024, 8192} }, .block_erase = spi_block_erase_21, }, { .eraseblocks = { {4 * 1024, 8192} }, .block_erase = spi_block_erase_20, }, { .eraseblocks = { {32 * 1024, 1024} }, .block_erase = spi_block_erase_53, }, { .eraseblocks = { {32 * 1024, 1024} }, .block_erase = spi_block_erase_52, }, { .eraseblocks = { {64 * 1024, 512} }, .block_erase = spi_block_erase_dc, }, { .eraseblocks = { {64 * 1024, 512} }, .block_erase = spi_block_erase_d8, }, { .eraseblocks = { {32 * 1024 * 1024, 1} }, .block_erase = spi_block_erase_60, }, { .eraseblocks = { {32 * 1024 * 1024, 1} }, .block_erase = spi_block_erase_c7, } },
Other chips place the 4BA (4-Byte Address) opcodes first, so I did the same here.
https://review.coreboot.org/c/flashrom/+/52310/comment/7fe33be1_b808e25f PS1, Line 16401: spi_prettyprint_status_register_plain spi_prettyprint_status_register_bp3_srwd
https://review.coreboot.org/c/flashrom/+/52310/comment/49e4ffce_f6adb287 PS1, Line 16402: spi_disable_blockprotect spi_disable_blockprotect_bp3_srwd
https://review.coreboot.org/c/flashrom/+/52310/comment/225fd020_3bb6432c PS1, Line 16406: }, Please use tabs for indentation
-
Angel Pons (Code Review)