Change in flashrom[master]: ite_ecfw: Implement support for flashing ITE ECs found on TUXEDO laptops
Attention is currently required from: Felix Singer, Michał Żygowski, Paul Menzel, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55715 )
Change subject: ite_ecfw: Implement support for flashing ITE ECs found on TUXEDO laptops ......................................................................
Patch Set 29:
(30 comments)
Patchset:
PS29: Splitting an old comment thread, might make things easier to follow.
Now I probe only for ITE Super I/Os. Just added more model IDs in the switch to catch the supported chips. Redesigning Super I/O code needs more thought and I won't pursue it right now. DMI checks have been replaced with PCI ID and SSID checks to not depend on any dmidecoder (which may fail depending on UEFI vs legacy).
I don't follow, can you elaborate when DMI checks would fail?
File flashrom.8.tmpl:
https://review.coreboot.org/c/flashrom/+/55715/comment/3b09e897_7b745a2a PS29, Line 1423: .B " flashrom -p ite_ecfw:portpair=pairnum" Isn't this board specific? If so the information should come from board_enable or similar mechanism.
https://review.coreboot.org/c/flashrom/+/55715/comment/afe36d67_5d593028 PS29, Line 1449: The default value is queried from EC. When is it needed then?
https://review.coreboot.org/c/flashrom/+/55715/comment/95fb25c5_4479ffa7 PS29, Line 1471: .B " flashrom -p ite_ecfw:boardmismatch=force" I don't think flashrom supports such a force yet nor that it should. However, for the internal programmer we allow to override the matched board. (Things would be much easier if you'd use the existing infrastructure.)
File it87spi.c:
https://review.coreboot.org/c/flashrom/+/55715/comment/d9b17aff_a3f5ae53 PS29, Line 105: case 0x89: How does this affect the internal programmer? Would be nice to split this out. It probably will take me hours to review.
File ite_ecfw.c:
https://review.coreboot.org/c/flashrom/+/55715/comment/38f9abcc_bb571d29 PS29, Line 22: #if defined(__i386__) || defined(__x86_64__) Please drop, the Makefile should decide when to compile things.
https://review.coreboot.org/c/flashrom/+/55715/comment/0dc0d2c1_d161ceb4 PS29, Line 94: } __attribute__((packed)); It's one byte AFAICS, so nothing to pack.
https://review.coreboot.org/c/flashrom/+/55715/comment/27c6ffd5_96ec34c6 PS29, Line 131: static int force = 0; No globals, please.
https://review.coreboot.org/c/flashrom/+/55715/comment/90087253_9e3d9495 PS29, Line 133: uint8_t *buf We usually put output parameters first, like in standard C APIs.
https://review.coreboot.org/c/flashrom/+/55715/comment/62d9f147_9d209ae8 PS29, Line 173: uint8_t i; You can just use an `int` or `unsigned int`. Using `uint8_t` probably just makes the compiler's job a little harder.
https://review.coreboot.org/c/flashrom/+/55715/comment/607a62c3_357ef139 PS29, Line 189: } Please make sure `ec_project` gets 0 terminated, no matter what the EC replies.
https://review.coreboot.org/c/flashrom/+/55715/comment/ffa1fb3e_344b434e PS29, Line 213: } Same as above.
https://review.coreboot.org/c/flashrom/+/55715/comment/91c27f0e_d097cf18 PS29, Line 216: } Aside from the command these two functions look the same, any reason to repeat the code?
https://review.coreboot.org/c/flashrom/+/55715/comment/c9a0a6a5_d260c45f PS29, Line 231: "You may need to pass the flash size via the programmer parameters or simply try again in a while.\n"); Please break this line around 70 chars (of text).
https://review.coreboot.org/c/flashrom/+/55715/comment/2acdece3_0a341068 PS29, Line 247: /* flush the EC registers */ Is that even possible for the status register?
https://review.coreboot.org/c/flashrom/+/55715/comment/a0c97201_0699b6e2 PS29, Line 268: unsigned int start, unsigned int len) Please align (I guess the intention was to, as there's already a mix of tabs and spaces).
https://review.coreboot.org/c/flashrom/+/55715/comment/ba79ca23_a3417bbc PS29, Line 368: unsigned int block) Please align.
https://review.coreboot.org/c/flashrom/+/55715/comment/388bde86_c750f7cf PS29, Line 392: CHUNKS_PER_KBYTE * BYTES_PER_CHUNK Isn't this just `1*KiB`?
No parentheses needed btw.
https://review.coreboot.org/c/flashrom/+/55715/comment/87282e08_69db12f3 PS29, Line 394: ){ Missing space.
https://review.coreboot.org/c/flashrom/+/55715/comment/46be4f43_6d6d1743 PS29, Line 413: memset(ctx_data->first_kbyte, 0, sizeof(ctx_data->first_kbyte)); What for?
https://review.coreboot.org/c/flashrom/+/55715/comment/b56fceef_04e37bd1 PS29, Line 470: Spurious space.
https://review.coreboot.org/c/flashrom/+/55715/comment/f73115bd_536e3e93 PS29, Line 493: ctx_data->ite_string_offset >= start && : ctx_data->ite_string_offset <= (start + len - sizeof(struct ite_string)) Shouldn't this check `start == 0` instead? How would ite_ecfw_patch_autoload() know if there is an offset?
https://review.coreboot.org/c/flashrom/+/55715/comment/8aaeca21_7ff97bb3 PS29, Line 603: What about ite_ecfw_chunkwise_erase() for ITE5570?
https://review.coreboot.org/c/flashrom/+/55715/comment/11b3bf6f_7bebdcb4 PS29, Line 696: } Are the IDs from RDID? I guess we should also check that the database entry assumes that.
https://review.coreboot.org/c/flashrom/+/55715/comment/2ab009fe_2ea6a7ab PS29, Line 717: id_length = 4; Hmmm, if smaller chips have a smaller id, maybe it's not RDID after all.
https://review.coreboot.org/c/flashrom/+/55715/comment/e884880d_506465ab PS29, Line 758: msg_perr Not an error?
https://review.coreboot.org/c/flashrom/+/55715/comment/e47f5466_54b410ea PS29, Line 785: if (superio_count == 0) : return; Really not necessary, the next line checks about the same.
https://review.coreboot.org/c/flashrom/+/55715/comment/fa679e10_228e3ab1 PS29, Line 812: Probing Probing might be the wrong word. AFAICS, the code just starts writing to EC RAM.
https://review.coreboot.org/c/flashrom/+/55715/comment/cc2e7be5_77c4f984 PS29, Line 813: "backlight failure and sudden poweroff.\n" I assume this is copied from the text for the flash probing on legacy buses? I think it's much different as that would not write to EC registers. In this case, I guess, even severe things can happen, e.g. need to reflash the EC firmware, or even permanent damage.
I don't think we want a force option for this.
https://review.coreboot.org/c/flashrom/+/55715/comment/767d18c3_2a2a6a27 PS29, Line 905: int ite_ecfw_verify_file_project(uint8_t *const newcontents, Is it still call somewhere?
-
Nico Huber (Code Review)