Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk, Nikolai Artemiev, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58477 )
Change subject: flashchips: add writeprotect bit layout map to chips ......................................................................
Patch Set 21:
(4 comments)
File flash.h:
https://review.coreboot.org/c/flashrom/+/58477/comment/3db77e9e_1ce7f9ca PS21, Line 192: 0 Why give these numbers? Is there anything to sync with?
https://review.coreboot.org/c/flashrom/+/58477/comment/64a5e9cb_76f52562 PS21, Line 196: }; If we'd ignore the enum types, i.e. use `uint8_t` for all members, we could squeeze everything in 1B, saving 92% of the space.
https://review.coreboot.org/c/flashrom/+/58477/comment/614d3a06_c9c09d6e PS21, Line 283: */ We should generally try to find a better representation for the flash chips. The database is already too big for some use cases. With the current `struct reg_bit_map` definition, this bloats the database by 23%. Fairly compressable though.
You don't have to use common definitions with a pointer btw. I played with this once for the block erasers. The only diff in the database was
- .block_erasers = + .block_erasers = (const struct block_eraser[NUM_ERASEFUNCTIONS])
all over it. Together with `-fmerge-all-constants` GCC could join the common definitions.
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/58477/comment/d963df53_9cc6cd65 PS9, Line 6760: {STATUS1, 5, RW}
I got the bits mixed up, thanks for noticing!
You could, maybe should, extend selfcheck(), e.g. add something similar to selfcheck_eraseblocks(). Or add a unit test (but there were always plans to make the chip database more runtime flexible). We could definitely catch such issues.
-
Nico Huber (Code Review)