Attention is currently required from: Anastasia Klimchuk, Bill XIE, Peter Marheine.
Nikolai Artemiev has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84253?usp=email )
Change subject: ichspi: Restore Sector erase opcode after send command ......................................................................
Patch Set 1:
(3 comments)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/84253/comment/661d1ac2_06d8ba1a?usp... : PS1, Line 402: static OPCODE POSSIBLE_OPCODES[] = {
What is the meaning of this array ? It is called POSSIBLE_OPCODES , but for example in the logs of t […]
There's some logic in reprogram_opcode_on_the_fly() to handle the case where an opcode isn't found in this array (i.e. when lookup_spi_type() returns 0xFF). The array name is misleading since it doesn't contain all possible opcodes.
https://review.coreboot.org/c/flashrom/+/84253/comment/e3b78988_a3e33b48?usp... : PS1, Line 664: int oppos = 2; // use original JEDEC_BE_D8 offset
I have questions in my head […]
The choice of index 2 here does seem suboptimal. It might be good to change it in a different CL since it could possibly break something.
It's worth noting that newer CPUs don't support this code path (they use hwseq) so it's hard to test any changes thoroughly.
https://review.coreboot.org/c/flashrom/+/84253/comment/258fe60e_590e2c87?usp... : PS1, Line 1843: : static bool ich_spi_probe_opcode(const struct flashctx *flash, uint8_t opcode) : { : return find_opcode(curopcodes, opcode) >= 0; Rather than restoring JEDEC_SE after every ich_spi_send_command() call, we could add a check here to see if the opcode is in POSSIBLE_OPCODES and return true if it is.
That way, ich_spi_send_command will automatically reprogram the required opcode if it is missing and we don't have to restore JEDEC_SE after every command that reprograms it.
-
Nikolai Artemiev (Code Review)