Change in flashrom[master]: ite_ec: Implement support for flashing ITE ECs found on TUXEDO laptops
Attention is currently required from: Felix Singer, Nico Huber, Paul Menzel, Angel Pons, Michael Niewöhner. Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 )
Change subject: ite_ec: Implement support for flashing ITE ECs found on TUXEDO laptops ......................................................................
Patch Set 6:
(12 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/55715/comment/2be3f4bf_1bfe778a PS4, Line 11: updates from IBV.
Can you please document the EC firmware version used, and give an example command?
Done
Patchset:
PS4:
Some SVID/SDID examples: […]
After a second thought, I think not all laptops with ITE ECs may have a distinguishable SVID/SDID set. In order to make this programmer more generic, leaving the DMI checks and will add DMI bypass option with force flag (however, we need to add a function to get the flash context somehow or pass it to programmer init)
PS4:
The IT8987E and IT5570E/IT5576E are a bit special. […]
Got some more information on the ITEString and found out what the autload patching is really doing. It sets some kind of EC Signature flag and changes the EC flash size to be mirrored
File Makefile:
https://review.coreboot.org/c/flashrom/+/55715/comment/4cdf808d_af21a7ca PS1, Line 839: override CONFIG_TUXEDO_EC = no
Probably many other also don't, like MEC1308 from what I can see in the code... […]
Done
File flashrom.8.tmpl:
https://review.coreboot.org/c/flashrom/+/55715/comment/df77aa1c_27184f2a PS4, Line 1376: The default is 1
`tuxedo_ec_init_ctx` is setting these defaults
Ack
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/55715/comment/625f9aa6_0e3504de PS1, Line 2109: #if CONFIG_TUXEDO_EC == 1
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.
Flashrom shouldn't even touch the hardware before content compatibility fails IMO. The approach with calling the function from programmer init is simple, but different programmers may need different arguments to be passed. I wonder how to address that. Maybe the programmer_entry structure should expose an optional method like `check_file` which would assess compatibility of the contents with the target hardware/flash.
File tuxedo_ec.c:
https://review.coreboot.org/c/flashrom/+/55715/comment/a2a66eb1_575f5b17 PS1, Line 205: ec_read_reg
Because reading the AC adapter state was always done with the hardcoded default register pair 0x62/0 […]
Ack
https://review.coreboot.org/c/flashrom/+/55715/comment/db21edde_89d9ffbd PS1, Line 477: internal_sleep(15000 * 64);
Unfortunately no way that we are aware of.
Ack
File tuxedo_ec.c:
https://review.coreboot.org/c/flashrom/+/55715/comment/743b23f2_538f6c30 PS4, Line 661: dmi_init(); : : if (!dmi_match("^TUXEDO$")) { : msg_perr("Not a TUXEDO device\n"); : return 1; : } : : for (i = 0; i < ARRAY_SIZE(tuxedo_supported_boards); i++) { : if (dmi_match(tuxedo_supported_boards[i])) { : match = true; : break; : } : } : : if (!match) { : msg_perr("TUXEDO EC programmer not yet supported on this device\n"); : return 1; : }
I was thinking about the same. Thanks for the piece of code. […]
Done
https://review.coreboot.org/c/flashrom/+/55715/comment/c7589d03_0a63061a PS4, Line 688: sizeof(struct tuxedo_ec_data)
sizeof(*ctx_data)
Done
https://review.coreboot.org/c/flashrom/+/55715/comment/9a6264a1_f6c1d0ba PS4, Line 740: uint8_t *const contents, char *buf
nit: I'd flip the parameter order to match the natural assignment semantics (leftmost parameter is t […]
Done
https://review.coreboot.org/c/flashrom/+/55715/comment/ad2f01bd_578f0f2c PS4, Line 822: if (strcmp(current_ec_project, new_ec_project)) { : msg_perr("Wrong EC project. This file can't be used on this machine\n"); : return 1; : } :
can we add a parameter for forcing here? One might use flashrom to switch from system76 ec to vendor […]
Should it be the programmer parameter or should I hook the global force flag?
-
Michał Żygowski (Code Review)