Attention is currently required from: Nico Huber, Angel Pons, Sergii Dmytruk. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58475 )
Change subject: spi25_statusreg: make register read/write functions generic ......................................................................
Patch Set 16:
(12 comments)
File flash.h:
https://review.coreboot.org/c/flashrom/+/58475/comment/23162502_2572ba7a PS15, Line 170: INVALID_REG = 0, : STATUS1, : STATUS2, : CONFIG1,
Please add entries in the commits that make use of them. Took me rather […]
Done. Yes, I had CONFIG1 for some Macronix chips, but I'll add it back later if it's needed.
File flash.h:
https://review.coreboot.org/c/flashrom/+/58475/comment/fcf847f4_b2c282fd PS14, Line 169: enum flash_reg { : INVALID_REG = 0, : STATUS1, : STATUS2, : CONFIG1, : }; : #define MAX_REGISTERS 4
`MAX_REGISTERS` is only used in `writeprotect.c` AFAICS, so it would belong […]
I've made it the last element, that should be a bit more robust. It's useful if you want to iterate over all registers that flashrom knows about, though I don't have another example of doing that outside writeprotect right now.
File spi25_statusreg.c:
https://review.coreboot.org/c/flashrom/+/58475/comment/3f1900ef_cc294e44 PS14, Line 51: int result = spi_send_multicommand(flash, cmds);
What do you mean? What could cause problems?
I think you're referring to the fact that it declares a variable in the middle of the function? There's quite a lot of other code like that, it shouldn't cause any new problems.
https://review.coreboot.org/c/flashrom/+/58475/comment/ffe275e5_24adef03 PS14, Line 126:
should be a tab
Done
https://review.coreboot.org/c/flashrom/+/58475/comment/15fbcf91_ecfe6b6c PS14, Line 134:
nit: double blank line
Done
https://review.coreboot.org/c/flashrom/+/58475/comment/8770b6c6_08bea653 PS14, Line 149:
nit: double blank line
Done
https://review.coreboot.org/c/flashrom/+/58475/comment/4585c851_7818ee9f PS14, Line 194: spi_write_register failed.
Would be good to indicate which register failed: […]
Done
https://review.coreboot.org/c/flashrom/+/58475/comment/1a0216b7_f39b55f1 PS14, Line 207: spi_write_register failed.
Ditto
Done
File spi25_statusreg.c:
https://review.coreboot.org/c/flashrom/+/58475/comment/4a646003_8ff8b64e PS15, Line 63: */
I have doubts that this is true for any modern chip. "allow running RDSR […]
I'm inclined to agree, this code dates back to 2010 in commit 174f55bdec62, which doesn't mention any specific chip, but says that some chips take >100ms, which implies that the initial delay wasn't doing anything useful even then.
We can probably just switch everything over to repeated polling, WDYT?
https://review.coreboot.org/c/flashrom/+/58475/comment/7d58e45b_0890c29c PS15, Line 84: *
Please drop this dangling asterisk or add a line break after the first […]
Done
https://review.coreboot.org/c/flashrom/+/58475/comment/3d71c81b_5975355c PS15, Line 89: }
Why not use a switch/case? It would make it easier to provide proper error […]
The next commit adds nested ifs for handling chips with different ways of writing the same register, and they cant be converted to a switch/case very cleanly.
https://review.coreboot.org/c/flashrom/+/58475/comment/f17825b5_821c2d14 PS15, Line 91: not supported by chip\
Or not supported by this function?
Done