Attention is currently required from: Felix Singer, Nico Huber, Paul Menzel, Sergii Dmytruk. Michał Żygowski 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:
(12 comments)
Patchset:
PS29:
Now I probe only for ITE Super I/Os. […]
You mean compile the files when CONFIG_INTERNAL is selected? Michael also wanted to add more interfaces to program ITE ECs (using different methods). I assume the "-p " would not have to change?
PS29:
Splitting an old comment thread, might make things easier to follow. […]
The internal DMI decoder checks the legacy region for SMBIOS entry. This will simply fail on UEFI firmware which do not populate SMBIOS entry in the legacy range, thus external decoder is the only viable option. We could also rely on the OS to expose the SMBIOS tables as a file in sysfs, but that would not apply to all build targets. There is also the raw memory access problem, not all memory can be mapped to the application.
File flashrom.8.tmpl:
https://review.coreboot.org/c/flashrom/+/55715/comment/33da8698_0df2d28f PS29, Line 1423: .B " flashrom -p ite_ecfw:portpair=pairnum"
Isn't this board specific? If so the information should come from […]
We can add a field that would specify the default port pair for given board. Could be board specific or OEM specific, depending on UEFI/EC configuration
https://review.coreboot.org/c/flashrom/+/55715/comment/fe968320_006f643a PS29, Line 1471: .B " flashrom -p ite_ecfw:boardmismatch=force"
I don't think flashrom supports such a force yet nor that it should. […]
There is a System76 EC firmware which serves as a replacement for the original firmware being used here. One could use the force switch to flash the replacement anyway. S76 EC embeds different strings into EC firmware which would result in a mismatch. Secondly it can override board PCI matching, e.g. we have a board that we know is supported (but not yet added to the board support table) and would like to flash the EC anyways. There are two usecases for this flag so I have to split it somehow, e.g. projectmismatch should enable forcing the use of a file despite the strings in the file don't match. I understand that forcing a flash operation is questionable and generally shouldn't be allowed, but we still permit it for internal programmer with boardmismatch. The results also may be catastrophic.
File it87spi.c:
https://review.coreboot.org/c/flashrom/+/55715/comment/3dd6d7e2_c4b24a36 PS29, Line 105: case 0x89:
How does this affect the internal programmer? Would be nice to split this […]
We can split it, however I have no means to answer your question (no hardware with ITE of ID starting with 0x89)
File ite_ecfw.c:
https://review.coreboot.org/c/flashrom/+/55715/comment/276535b8_8f155fff PS29, Line 411: }
Doesn't this mean we always have to flash the whole chip? and all blocks in […]
We replay the behaviour of the original EFI file doing the update. We only have to erase the whole chip in one sway. Write operations may be of one block size (64K) and flashrom would write and verify per block. Writing order is not relevant, just the first kbyte has to be written at the end (if the first block has been modified).
https://review.coreboot.org/c/flashrom/+/55715/comment/6c0d12ab_6f23f3c0 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() […]
The code should skip the autoload patching if offset is 0, I have to fix it. The offset is obtained in ite_ecfw_get_ite_string_offset and is supposed to be in the first 256 bytes of the firmware file (with 16 bytes interval). Offset is stored in the ctx_data
https://review.coreboot.org/c/flashrom/+/55715/comment/cc3b3f9c_7b63b4b6 PS29, Line 495: (uint8_t *)
Ah, I was too tired yesterday when I read this and didn't […]
The autoload option breaks the verification indeed. Such cast allows modifying the initial content of the firmware file to be flashed for the autoload purpose. How would you see such modification to be done on the fly in flashrom?
https://review.coreboot.org/c/flashrom/+/55715/comment/073a87e3_1180a413 PS29, Line 603:
What about ite_ecfw_chunkwise_erase() for ITE5570?
Probably we can set the erase size to 256 bytes or 1024 bytes, this needs testing. EC_CMD_WRITE_BLOCK may also have some parameter to specify which chunk to write (it can select the block nimber with second parameter and starting chunk with third parameter, but it always consumes the number of chunks needed to flash full block). Maybe Michael could tell more if this is possible. Then we wouldn't need any 64k granularity.
https://review.coreboot.org/c/flashrom/+/55715/comment/1511842e_eb8542ad PS29, Line 696: }
Are the IDs from RDID? I guess we should also check that the database entry […]
It looks like RDID, but the EC doesn't return reliable data. It starts with 0xef (Winbond, which is true for the usd chip) but the rest of ID is either 0xef or 0x00. So we use this function for informative purposes only (until we find out what is going wrong here).
https://review.coreboot.org/c/flashrom/+/55715/comment/82626999_7b0e5b5f PS29, Line 812: Probing
Probing might be the wrong word. AFAICS, the code just starts writing to EC RAM.
No longer true. If the board is not on the list of supported boards (matched by PIC and PCI subsystem of the host bridge), the code won't get to writing to EC RAM, unless we force it.
We probe the Super I/O later but yeah it starts with writing to EC RAM (after board matching). We could start with probing for Super I/O first and see if anything is detected.
https://review.coreboot.org/c/flashrom/+/55715/comment/3f3db44d_23a77c62 PS29, Line 905: int ite_ecfw_verify_file_project(uint8_t *const newcontents,
Is it still call somewhere?
Currently not, but I wish it would be at some point. We haven't decided yet where a programmer hook could be placed before the flash process begins (we have only the bad model of internal programmer and cb_check_image function). Could struct programmer_entry be extended with some pre flash access hook?