Attention is currently required from: DZ, Nikolai Artemiev.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81096?usp=email )
Change subject: flashchips: Add Macronix Flash EPN support ......................................................................
Patch Set 1:
(13 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/81096/comment/a35d804a_90ead2e8 : PS1, Line 9: We have tested read/write/erase and write protection function for following Macronix Flash. This is great, thank you so much! Could you please add info to commit message: which programmer you were using, and which operations you ran. Thanks!
Patchset:
PS1: Daniel thank you the patch! I appreciate you are updating so many chip definitions!
I have a suggestion, if you want, you can split this commit as one chip per commit. This way we can gradually review and submit them one by one. But you can keep it in one large commit, however in this case none of the chips will be submitted until all of them are ready.
I added comments, and then I will have another round of review after these ones are fixed.
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/81096/comment/f5276d8a_c964e083 : PS1, Line 6201: .tested = TEST_UNTESTED, This diff seems to be by mistake? That's Fudan chip, but also if the chip has been tested, you can't untest it :)
https://review.coreboot.org/c/flashrom/+/81096/comment/233b82cb_58f936a1 : PS1, Line 9008: MX25L12845E/MX25L12865E These two models (MX25L12845E and MX25L12865E) have no configuration register, not in datasheets, which means:
In the patch you update the current definition with `FEATURE_CFGR` and `reg_bits`, and now MX25L12845E and MX25L12865E do not match this new definition anymore.
The easiest option is to add MX25L12850F as a new definition (and it will be reusing the same model ID, which is fine), and add `FEATURE_CFGR` and `reg_bits` into it. Old definition remains the same.
Another option (harder, but more comprehensive) is to have old definition left for MX25L12845E and MX25L12865E, and create new definition with write-protect support for the remaining chips (including new one).
https://review.coreboot.org/c/flashrom/+/81096/comment/aa5ebaf1_6c51b52d : PS1, Line 9275: .tb = {CONFIG, 3, OTP} I didn't find configuration register in datasheets?
https://review.coreboot.org/c/flashrom/+/81096/comment/20b69955_7136025c : PS1, Line 9547: .eraseblocks = { {64 * 1024, 64} }, : .block_erase = SPI_BLOCK_ERASE_52, this command is used for 32K block
https://review.coreboot.org/c/flashrom/+/81096/comment/c2e74bfd_93878745 : PS1, Line 9564: {1650, 3600} datasheet says 2.65V-3.6V
https://review.coreboot.org/c/flashrom/+/81096/comment/9ece37cf_8bf93e73 : PS1, Line 9567: .srp = {STATUS1, 7, RW}, This bit is "Reserved" in datasheet
https://review.coreboot.org/c/flashrom/+/81096/comment/f69f8af8_6e243245 : PS1, Line 9594: .eraseblocks = { {64 * 1024, 64} }, : .block_erase = SPI_BLOCK_ERASE_52, this command is for 32K block
https://review.coreboot.org/c/flashrom/+/81096/comment/8bd19c68_f25e6897 : PS1, Line 9611: {1650, 3600}, datasheet says 2.7V-3.6V
https://review.coreboot.org/c/flashrom/+/81096/comment/e9da62a5_3d78bec9 : PS1, Line 10285: .name = "MX25R1635F", It seems like you moved MX25R1635F entry, and now it's out of alphabetical order. You need to return in back into its position in the list
File include/flashchips.h:
https://review.coreboot.org/c/flashrom/+/81096/comment/6535e38c_a2511122 : PS1, Line 538: #define MACRONIX_MX25L3239E 0x2536 for this and the other new lines: you need to keep the indentation between macro name and values, to keep value aligned
https://review.coreboot.org/c/flashrom/+/81096/comment/6860b225_eb3fe60b : PS1, Line 546: #define MACRONIX_MX25R4035F 0x2813 : #define MACRONIX_MX25R1635F 0x2815 it's a duplicate, the same macro exists already
-
Anastasia Klimchuk (Code Review)