Attention is currently required from: Felix Singer, Michał Żygowski, Angel Pons, Michael Niewöhner.
2 comments:
Patchset:
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:
Patch Set #1, 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.
To view, visit change 55715. To unsubscribe, or for help writing mail filters, visit settings.