Attention is currently required from: Nico Huber, Michał Żygowski. Angel Pons 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 1:
(41 comments)
File Makefile:
https://review.coreboot.org/c/flashrom/+/55715/comment/dc9687ac_9ce8aa8c 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 remove this if there's no particular reason for it.
https://review.coreboot.org/c/flashrom/+/55715/comment/da05f5ed_876b75d3 PS1, Line 839: override CONFIG_TUXEDO_EC = no hmmm, but this programmer does not use libpci?
https://review.coreboot.org/c/flashrom/+/55715/comment/d8769ee6_62d5e9a5 PS1, Line 903: # Disable internal DMI decoder to be able to locate DMI tables on EFI systems and match TUXEDO laptops Why?
File flashrom.8.tmpl:
https://review.coreboot.org/c/flashrom/+/55715/comment/a5393331_c54e324f PS1, Line 648: .SS Was this removed on purpose?
https://review.coreboot.org/c/flashrom/+/55715/comment/69a4426a_d31bcf81 PS1, Line 1399: autoloading What's autoloading?
https://review.coreboot.org/c/flashrom/+/55715/comment/01531e8b_dea281c7 PS1, Line 1411: ite5570 How would the reader of the man page know when to enable IT5570 support?
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/55715/comment/f7933f94_db2063d4 PS1, Line 42: #if CONFIG_TUXEDO_EC == 1 I'd appreciate if we could avoid using preprocessor.
https://review.coreboot.org/c/flashrom/+/55715/comment/bc9eb3c6_9ac04a48 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.
File tuxedo_ec.c:
https://review.coreboot.org/c/flashrom/+/55715/comment/2bea369a_7368e169 PS1, Line 68: typedef struct Typedefs are frowned upon: https://www.kernel.org/doc/html/latest/process/coding-style.html#typedefs
https://review.coreboot.org/c/flashrom/+/55715/comment/a87e95b9_99fce5f1 PS1, Line 184: flashrom@flashrom.org Personally, I wouldn't know what to do with such a report on the mailing list.
https://review.coreboot.org/c/flashrom/+/55715/comment/3c9d2b16_ea6ced3f 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:
"Querying EC ROM size returned unexpected result.\n" "Probably the EC has just been flashed and the EC RAM has been reset.\n" "You may need to pass the flash size via the programmer parameters or try again in a while.\n" "If this warning persists please email a report to flashrom@flashrom.org\n"
https://review.coreboot.org/c/flashrom/+/55715/comment/cf1d674d_5df7f5e7 PS1, Line 205: ec_read_reg Why `ec_read_reg` and not `tuxedo_ec_read_byte`?
https://review.coreboot.org/c/flashrom/+/55715/comment/42ac0440_6399bf29 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 which is the first byte?
https://review.coreboot.org/c/flashrom/+/55715/comment/f8211bb5_f7813961 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 multiple of `BYTES_PER_BLOCK`?
if (!len || len % BYTES_PER_BLOCK != 0) {
https://review.coreboot.org/c/flashrom/+/55715/comment/26360eae_af77017d PS1, Line 286: return tuxedo_ec_write_byte(ctx_data, data); `break;` would be equivalent and easier to understand.
https://review.coreboot.org/c/flashrom/+/55715/comment/0959c5c0_39a08140 PS1, Line 316: unsigned int uint8_t ?
https://review.coreboot.org/c/flashrom/+/55715/comment/034d8966_16164f54 PS1, Line 318: unsigned int uint8_t ?
https://review.coreboot.org/c/flashrom/+/55715/comment/b36e6ff3_2957ea64 PS1, Line 324: CHUNKS_PER_KBYTE * BYTES_PER_CHUNK sizeof(ctx_data->first_kbyte)
https://review.coreboot.org/c/flashrom/+/55715/comment/4dfbc001_8529b8a6 PS1, Line 361: CHUNKS_PER_KBYTE * BYTES_PER_CHUNK sizeof(ctx_data->first_kbyte)
https://review.coreboot.org/c/flashrom/+/55715/comment/6d2a8098_7beb2773 PS1, Line 384: offset `start + i`?
https://review.coreboot.org/c/flashrom/+/55715/comment/0a032c47_8bd7f0f1 PS1, Line 387: offset - ctx_data->autoload_offset != 1 Can these checks ever be true? The state is reset to `AUTOLOAD_NONE` already.
https://review.coreboot.org/c/flashrom/+/55715/comment/47b58b86_62653769 PS1, Line 433: 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 multiple of `BYTES_PER_BLOCK`?
if (!len || len % BYTES_PER_BLOCK != 0) {
https://review.coreboot.org/c/flashrom/+/55715/comment/33cb1117_82a6511c PS1, Line 477: internal_sleep(15000 * 64); No way to poll for completion?
https://review.coreboot.org/c/flashrom/+/55715/comment/647dcced_84be2ca9 PS1, Line 482: if (!ec_wait_for_obuf(ctx_data->control_port, : EC_MAX_STATUS_CHECKS * 3)) : return 1; : : if (INB(ctx_data->data_port) == 0xf8) : return 0; Looks like this is open-coding `tuxedo_ec_read_byte` because the `max_checks` parameter isn't exposed.
https://review.coreboot.org/c/flashrom/+/55715/comment/4943ce49_24efca15 PS1, Line 504: ++to_chunk This would make flashrom erase too much?
https://review.coreboot.org/c/flashrom/+/55715/comment/05c505d6_d9d25bad PS1, Line 534: legnth typo: length
https://review.coreboot.org/c/flashrom/+/55715/comment/74409476_f24632ee PS1, Line 551: ctx_data->rom_size_in_kbytes This is the only place where `rom_size_in_kbytes` is used. Given that it provides redundant information, why not drop the member and use `ctx_data->rom_size_in_blocks * KBYTES_PER_BLOCK` here?
Oh wait, this is what the block eraser size already does. If you don't want to repeat the calculation twice (it currently appears three times), make `rom_size_in_kbytes` a local constant inside this function.
https://review.coreboot.org/c/flashrom/+/55715/comment/a60e1d93_eee2cd83 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.
https://review.coreboot.org/c/flashrom/+/55715/comment/bf51e078_dac2d240 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. I don't expect anything else to be supported in the foreseeable future, and this could be added later anyway.
https://review.coreboot.org/c/flashrom/+/55715/comment/b093cce1_f1150854 PS1, Line 666: static void get_flash_part_from_id(tuxedo_ec_data_t *ctx_data, uint32_t manuf_id, Please ignore. I left a comment here before, but Gerrit doesn't let me delete the draft...
https://review.coreboot.org/c/flashrom/+/55715/comment/c744ec55_8dde3553 PS1, Line 713: if (id_length == 4) If you zero-initialise the array, this "if" can be removed.
https://review.coreboot.org/c/flashrom/+/55715/comment/2c5bd501_d88ca095 PS1, Line 722: = NULL Dead assignment
https://review.coreboot.org/c/flashrom/+/55715/comment/bdadb228_0989ce42 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?
https://review.coreboot.org/c/flashrom/+/55715/comment/9b2a2cd9_1609d200 PS1, Line 750: extra extra in which sense?
https://review.coreboot.org/c/flashrom/+/55715/comment/7f79f33c_1a350a6f 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?
https://review.coreboot.org/c/flashrom/+/55715/comment/c8481420_c7fb292c 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?
https://review.coreboot.org/c/flashrom/+/55715/comment/c7c4c308_a88d4f21 PS1, Line 825: nto typo: not
https://review.coreboot.org/c/flashrom/+/55715/comment/0ed47353_dcc878cf PS1, Line 826: Ifthe missing a space
https://review.coreboot.org/c/flashrom/+/55715/comment/5c0ba19e_d008b047 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 current contents already match, and skipping flashing when only the version matches could prevent recovering from a botched flash.
https://review.coreboot.org/c/flashrom/+/55715/comment/f93ebc00_f2ab9df9 PS1, Line 832: Contects typo: Contents
https://review.coreboot.org/c/flashrom/+/55715/comment/50da9b94_3d0fe6cc 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?