Attention is currently required from: Felix Singer, Michał Żygowski, Angel Pons, Michael Niewöhner. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 )
Change subject: tuxedo_ec: Implement support for flashing ECs on TUXEDO laptops ......................................................................
Patch Set 4:
(2 comments)
Patchset:
PS4: I think it's awesome that the DMI checks were there from the beginning. This is the way to go. Great work!
retrieve the chip ID at indices 0x20/0x21 and find a match in a table of supported ECs. This could also be used to handle the ITE IT5570E differences.
There is no standard for these registers, any chip could return the same as an ITE one. There is not even a guarantee that the 0x2e/0x2ef ports are used as a register/data pair. Probing such things is ok for a development tool like `inteltool`, who cares if the developer's machine resets/bricks? But flashrom is not a hacking tool, it's users are humble. There will be people who'll simply try the programmer.
or better the firmware id (e.g. the "ITE string" or sth else) because support for this firmware-based programming mechanism is implemented in EC fw
That's right, we would not only have to detect the chip without probing any non-standard ports, we'd also have to figure out what firmware it runs. How would one get this string without making assumptions that can be broken (e.g. by not using ITEs firmware framework)?
Maintaining a table of compatible boards is much likely the least work. Having said that, it might not be a bad idea to add additional checks after the board detection, i.e. when we know the board, we know that there is an ITE chips, can find the PMC pair, try to check for a com- patible firmware (currently the code starts with writes without knowing if the firmware is compatible; e.g. did somebody replace it with the System76 one?).
So basically we can rename the programmer to something like ite_ec. Any other suggestions?
I'd say `ite_ecfw.c`. After all it's not a driver for their chips but for their firmware framework?.
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/55715/comment/887b98de_7c92c5b1 PS1, Line 2109: #if CONFIG_TUXEDO_EC == 1
I feel the same when I look at 40 lines back with internal programmer. […]
The coreboot check above is a very bad rolemodel. It does things flashrom shouldn't do (look at the contents) and in a way (doing programmer-specific things in common code; a layering violation) that we try to get rid of.
I think it would be best to call _verify_file_project() from programmer init. The UI would then have to feed the file path (or contents) two times. If we really want such a check, flashrom (or the library) shouldn't offer any operation if the contents look incompatible.
Usually, we leave the decision if contents are valid to the user. Flashrom is supposed to be a tool that synchronizes flash contents with files, nothing more. However, I can understand the urge to check such things. If we decide to allow such checks and want to do them here, during writing, it should be a function pointer of the programmer.
Weak functions can be nice sometimes but are generally highly volatile. Many bugs in coreboot, for instance, are introduced because of weak functions. Anyway, it wouldn't work here unless we would do one flashrom build per programmer xD
NB. I think a lot of this could be avoided when people would write more specific CLIs. Using the all-in-one cli_classic might not be a good idea for EC flashing.