Attention is currently required from: Felix Singer, Nico Huber, Angel Pons. Michał Żygowski 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 3:
(36 comments)
File Makefile:
https://review.coreboot.org/c/flashrom/+/55715/comment/d8f44a3c_9d1a78eb PS1, Line 409: ifeq ($(CONFIG_TUXEDO_EC), yes) : UNSUPPORTED_FEATURES += CONFIG_TUXEDO_EC=yes : else : override CONFIG_TUXEDO_EC = no : endif
Any reason why this programmer wouldn't work with libpayload? I'm not asking you to test it, just re […]
Currently there is no coreboot (and libpayload) support for Tuxedo laptops, thus it is disabled here.
https://review.coreboot.org/c/flashrom/+/55715/comment/93bfbec4_4917de6c PS1, Line 839: override CONFIG_TUXEDO_EC = no
hmmm, but this programmer does not use libpci?
Probably many other also don't, like MEC1308 from what I can see in the code... Probably NEED_RAW_ACCESS would be enough. What I can do to verify it? Build with TUXEDO_EC enabled only?
https://review.coreboot.org/c/flashrom/+/55715/comment/54d15d5d_b7df6190 PS1, Line 903: # Disable internal DMI decoder to be able to locate DMI tables on EFI systems and match TUXEDO laptops
Why?
The TUXEDO laptops currenlty come with an UEFI firmware from IBV, no CSM support, so the only way to get the DMI table is to use the external decoder (checking F segment fails).
File flashrom.8.tmpl:
https://review.coreboot.org/c/flashrom/+/55715/comment/e1ddb0fa_bde5cb39 PS1, Line 1399: autoloading
What's autoloading?
I can only guess. Maybe it indicates whether the EC should automatically load the firmware update from flash after the operation.
https://review.coreboot.org/c/flashrom/+/55715/comment/18314bb6_8f7691e6 PS1, Line 1411: ite5570
How would the reader of the man page know when to enable IT5570 support?
The code automatically detects ITE5570, however it can be forcefully set as in the reference implementation.
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/55715/comment/fd81de44_9c4ba1ec PS1, Line 2109: #if CONFIG_TUXEDO_EC == 1
I can see why you'd want to do this, but tacking it right here feels odd.
I feel the same when I look at 40 lines back with internal programmer. Maybe we should introduce weak functions like: programmer_before_flash_access and programmer_before_image_write ?
File tuxedo_ec.c:
https://review.coreboot.org/c/flashrom/+/55715/comment/ca1dbc6b_ea76c25e PS1, Line 68: typedef struct
Typedefs are frowned upon: https://www.kernel.org/doc/html/latest/process/coding-style. […]
Done
https://review.coreboot.org/c/flashrom/+/55715/comment/9a6cad9d_a278e6dd PS1, Line 184: flashrom@flashrom.org
Personally, I wouldn't know what to do with such a report on the mailing list.
It just happens when you try to flash it two consecutive times in a very short interval. Not aware of any other circumstances when it may happen. We may as well ignore it and remove the report, because simply executing the flashrom again will give the correct result.
https://review.coreboot.org/c/flashrom/+/55715/comment/c7b890a8_241d79d2 PS1, Line 179: msg_pwarn("Querying EC ROM size returned unexpected result.\n" : "Probably the EC has just been flashed and the EC " : "RAM has been reset.\nYou may need to pass the flash" : " size via the programmer parameters or try again in" : " a while.\nIf this warning persists please email a " : "report to flashrom@flashrom.org\n");
Breaking user-visible strings makes it harder to grep for them. How about: […]
Okay. Too much tied to 80 chars per line.
https://review.coreboot.org/c/flashrom/+/55715/comment/de695876_fa295727 PS1, Line 205: ec_read_reg
Why `ec_read_reg` and not `tuxedo_ec_read_byte`?
Because reading the AC adapter state was always done with the hardcoded default register pair 0x62/0x66. Although the reference implementation allows to "change" the I/O port pair, it does not apply to all tranfers. That's just how it is...
https://review.coreboot.org/c/flashrom/+/55715/comment/bacf5b9f_bff6a1b0 PS1, Line 205: if (!ec_read_reg(0x10, ®_value)) { : msg_perr("Failed to query first byte of state register.\n"); : return false; : } : if (!ec_read_reg(0x10, ®_value)) { : msg_perr("Failed to query AC adapter state.\n"); : return false; : }
Do I understand correctly that reading this register results in a side-effect? How does one know whi […]
No idea about side effect. We simply read the register twice, replaying the process from reference implementation. If the read is not done twice, it reports incorrect AC adapter state. First byte of the status register i just a guess. Maybe it is simply a register that needs a read first to fill the value and read again to report the correct status.
https://review.coreboot.org/c/flashrom/+/55715/comment/0911c2d3_93e2347e PS1, Line 235: len % BYTES_PER_BLOCK != 0 && len < BYTES_PER_BLOCK
Are you sure this check is supposed to use an AND? Shouldn't it be checking that the length is a mul […]
It should be multiple of BYTES_PER_BLOCK
https://review.coreboot.org/c/flashrom/+/55715/comment/4d26e063_12a5bd88 PS1, Line 286: return tuxedo_ec_write_byte(ctx_data, data);
`break;` would be equivalent and easier to understand.
Done
https://review.coreboot.org/c/flashrom/+/55715/comment/8004345d_55f24901 PS1, Line 316: unsigned int
uint8_t ?
Done
https://review.coreboot.org/c/flashrom/+/55715/comment/295cedcf_3a85c31f PS1, Line 318: unsigned int
uint8_t ?
Done
https://review.coreboot.org/c/flashrom/+/55715/comment/04bd1329_ab192d61 PS1, Line 324: CHUNKS_PER_KBYTE * BYTES_PER_CHUNK
sizeof(ctx_data->first_kbyte)
Done
https://review.coreboot.org/c/flashrom/+/55715/comment/6f406bb1_544224a7 PS1, Line 361: CHUNKS_PER_KBYTE * BYTES_PER_CHUNK
sizeof(ctx_data->first_kbyte)
Done
https://review.coreboot.org/c/flashrom/+/55715/comment/40703442_188bdaea PS1, Line 384: offset
`start + i`?
Simplified it to something very clear and minimalistic.
https://review.coreboot.org/c/flashrom/+/55715/comment/0cd57a6c_27ae9634 PS1, Line 387: offset - ctx_data->autoload_offset != 1
Can these checks ever be true? The state is reset to `AUTOLOAD_NONE` already.
Simplified it to something very clear and minimalistic.
https://review.coreboot.org/c/flashrom/+/55715/comment/77430cc5_a7c251c3 PS1, Line 477: internal_sleep(15000 * 64);
No way to poll for completion?
Unfortunately no way that we are aware of.
https://review.coreboot.org/c/flashrom/+/55715/comment/22363910_528acab7 PS1, Line 504: ++to_chunk
This would make flashrom erase too much?
The check below should prevent it. Anyway we do not expect to have an erase operation other than 64KiB aligned block erase or multiple 64KiB block erase, so it may be removed as well.
https://review.coreboot.org/c/flashrom/+/55715/comment/004dc67c_510ee0ae PS1, Line 534: legnth
typo: length
Done
https://review.coreboot.org/c/flashrom/+/55715/comment/690b4a9f_5a70d73f PS1, Line 551: ctx_data->rom_size_in_kbytes
This is the only place where `rom_size_in_kbytes` is used. […]
Done
https://review.coreboot.org/c/flashrom/+/55715/comment/410b5820_236eef5f PS1, Line 555: * Erase operation must be done in one sway.
Doesn't seem to be the case for IT5570? The existing code doesn't allow making use of this, though.
Yes it doesn't seem, but in fact we can't make use of chunk erases to chop the update operation. If we mix read/erase/write operations, the flashing simply fails.
https://review.coreboot.org/c/flashrom/+/55715/comment/248b2642_299b19e7 PS1, Line 581: p = extract_programmer_param("type"); : if (p && strcmp(p, "ec")) { : msg_pdbg("%s(): tuxedo_ec only supports "ec" type devices\n", : __func__); : ret = false; : } : free(p);
Remove this, please. […]
Done
https://review.coreboot.org/c/flashrom/+/55715/comment/01a89701_9af57fb8 PS1, Line 713: if (id_length == 4)
If you zero-initialise the array, this "if" can be removed.
Done
https://review.coreboot.org/c/flashrom/+/55715/comment/de5884e1_3b1bdd6c PS1, Line 722: = NULL
Dead assignment
Done
https://review.coreboot.org/c/flashrom/+/55715/comment/eff8a08a_be7802db PS1, Line 734: if (!dmi_match("^InfinityBook S 14 Gen6$") && : !dmi_match("^InfinityBook S 15 Gen6$")) {
To ease adding support for more laptops in the future, put the strings in an array and loop over it?
Done
https://review.coreboot.org/c/flashrom/+/55715/comment/a0fcd2fd_0b099f2b PS1, Line 750: extra
extra in which sense?
Removed to avoid confusion. Extra probably meant the programmer state data like autoload, ite5570 support, etc.
https://review.coreboot.org/c/flashrom/+/55715/comment/d55b9748_feaf4521 PS1, Line 792: if (register_opaque_master(&programmer_opaque_tuxedo_ec, ctx_data)) : return 1; : : msg_pdbg("%s(): successfully initialized tuxedo_ec\n", __func__); : return 0;
Drop the print and return `register_opaque_master` directly?
Done
https://review.coreboot.org/c/flashrom/+/55715/comment/e4d70a34_360b3c5b PS1, Line 798: tuxedo_ec_init_exit: : tuxedo_ec_shutdown(ctx_data); : return 1; : : tuxedo_ec_init_exit_shutdown: : tuxedo_ec_shutdown(ctx_data); : return 1;
Am I missing something, or are these two paths identical?
Indeed.
https://review.coreboot.org/c/flashrom/+/55715/comment/3c5dabeb_62b3c256 PS1, Line 825: nto
typo: not
Done
https://review.coreboot.org/c/flashrom/+/55715/comment/63cb73af_80aa146f PS1, Line 826: Ifthe
missing a space
Done
https://review.coreboot.org/c/flashrom/+/55715/comment/518fc146_2b96e526 PS1, Line 827: If the current and the file's EC version is identical, : * return two (skip flashing)
Is there any reason to skip flashing if the EC version matches? flashrom already checks if the curre […]
I check the EC version because after the EC restarts, i.e. after `tuxedo_ec_shutdown` is called, the EC seems to reload the firmware from flash and writes a few bytes into it. I consider it pointless to flash e.g. 128K because of a few variable runtime bytes have been written by EC.
That's what I thought initially. But now when I looked at the autoload patching, there was a bug with the offset you pointed out so it incorrectly patched the file and probably resulted in such weird behavior. Removed.
https://review.coreboot.org/c/flashrom/+/55715/comment/4e97b4b2_453bf84c PS1, Line 832: Contects
typo: Contents
Done
https://review.coreboot.org/c/flashrom/+/55715/comment/3191ee40_6750b909 PS1, Line 904: .name = "tuxedo_ec", : .type = OTHER, : .devs.note = "Embedded Controller of TUXEDO laptops.\n", : .init = tuxedo_ec_init, : .map_flash_region = fallback_map, : .unmap_flash_region = fallback_unmap, : .delay = internal_delay,
One tab too many in the indentation?
Done