Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Sergii Dmytruk. Nikolai Artemiev 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 24:
(1 comment)
File flashchips.c:
https://review.coreboot.org/c/flashrom/+/58477/comment/0b7c85ac_3998f79e PS20, Line 6353: .tb = {STATUS1, 5, RW}, /* Called BP3 in datasheet, acts like TB */ : .sec = {STATUS1, 6, RW}, /* Called BP4 in datasheet, acts like SEC */
Hmmm, this can indeed be a problem. I haven't had time to check datasheets yet.
Any thoughts about how this should be handled?
The main issue in my experience is that someone may add support for a new chip using bit names from the datasheet without realizing that the naming is suboptimal, e.g. they might call a bit BP3 when it acts like TB.
That leads to unnecessary duplication of range decoding functions that only differ in bit naming. It happened a lot in cros WP code and caused unnecessary range lookup tables to be added.
It might be sufficient to add a comment warning people to check the function of each bit, rather than directly using the datasheet names. We could also add some kind of test for it maybe. E.g. for every range decoding function, check that none of the BP bits act like TB/CMP/SEC. It would be easy to detect if a bit acts like TB or CMP, but SEC would be harder and I'm not sure if would be worth it overall.
Another option is to always use the datasheet's names, and avoid duplication by factoring out common logic from the range decoding functions. But that still leaves a few problems, e.g. names can change between datasheet revisions or between datasheets for different chips that share a single chip definition.
-
Nikolai Artemiev (Code Review)