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:
(12 comments)
Patchset:
PS29:
The internal DMI decoder checks the legacy region for SMBIOS entry. […]
I was wondering for some time why the external DMI option is not the default. Maybe it's time to change that. However, this seems like an independent problem. Thanks for the clarification.
PS29:
You mean compile the files when CONFIG_INTERNAL is selected? Michael also wanted to add more interfa […]
I mean it should be fully included in `-p internal` eventually. The `internal` programmer is meant for flash chips on the system mainboard. All the infra- structure is there, and I hope it wouldn't be hard to hook an EC driver properly up. Basically a board-enable entry would call the init function of this driver, and that would register another master. Later logic could decide if this master is to be used, or we could filter the call beforehand, e.g. something like `-p internal:master=ec`.
To make use of the board-enable infrastructure for other than the `internal` programmers would need a lot of refactoring I guess. For now, I would rather keep the current design. Unless there is something I miss, i.e. any reason not to make EC drivers part of `-p internal`?
As I said, I'd be ok with merging it as a separate programmer. But we'd have to expect that it will change in the future (i.e. I would welcome patches that make it part of `-p internal`).
File flashrom.8.tmpl:
https://review.coreboot.org/c/flashrom/+/55715/comment/6520b66f_2c6cd84d PS29, Line 1423: .B " flashrom -p ite_ecfw:portpair=pairnum"
We can add a field that would specify the default port pair for given board. […]
Yes, I would much prefer default values in the board list.
https://review.coreboot.org/c/flashrom/+/55715/comment/724d6f9e_cc711baa PS29, Line 1471: .B " flashrom -p ite_ecfw:boardmismatch=force"
There is a System76 EC firmware which serves as a replacement for the original firmware being used h […]
AIUI (hopefully I'm not wrong) the original boardmismatch option is only useful in combination with coreboot and the image checks.
To be able to force a board-enable entry there is this syntax: ``` flashrom -p internal:mainboard=<vendor>:<board> ``` This has the advantage that one can force board-specific code or default, i.e. could be useful in combination with defaults for port pairs like mentioned above. IOW, if we add per-board defaults, we'd want something like this too.
File it87spi.c:
https://review.coreboot.org/c/flashrom/+/55715/comment/7ffe7fd8_8114618f PS29, Line 105: case 0x89:
We can split it, however I have no means to answer your question (no hardware with ITE of ID startin […]
Yes, please split. I'd want to carefully read the related code. It's a bit annoying (definitely not your fault) that the internal programmer always calls this code, no matter what board was detected.
Your call is actually safer ;)
File ite_ecfw.c:
https://review.coreboot.org/c/flashrom/+/55715/comment/b38e8e56_a4e053de PS29, Line 411: }
We replay the behaviour of the original EFI file doing the update. […]
AFAICS, if we'd allow a partial erase, flashrom could decide to only write the first block. Then this function would never be called for the last block and the first kilobyte wouldn't get written?
I know it's only a theoretical problem as long as we erase the whole chip.
Also, when the end of the image is empty, i.e. 0xff, same problem could occure, hmmm. This wouldn't be fun to test /o\ That's also rather theoretical, right? I guess these chips are always fully stuffed with data?
https://review.coreboot.org/c/flashrom/+/55715/comment/817078ef_65df9ffe PS29, Line 493: ctx_data->ite_string_offset >= start && : ctx_data->ite_string_offset <= (start + len - sizeof(struct ite_string))
The code should skip the autoload patching if offset is 0, I have to fix it. […]
I rather meant what if the string offset is within start+len but start is not 0. I would expect ite_ecfw_patch_autoload() to patch the wrong location (`ite_string_offset` instead of `ite_string_offset - start`?).
But let's discuss the autoload feature further first.
https://review.coreboot.org/c/flashrom/+/55715/comment/83e544be_51866ca9 PS29, Line 495: (uint8_t *)
The autoload option breaks the verification indeed. […]
To be honest I don't see such modifications in the flashrom tool. Wouldn't mind it in another tool in the project, of course.
It's kind of an unwritten rule (well, unwritten beside the `const`). Flashrom should just sync file contents with flash contents (maybe I'm too much a fan of the Unix phylosophy). I would feel very uncomfortable to change that rule on my own, without a prior discussion on the mailing list where everybody would have a chance to raise their concerns. Also, removing the `const` from the API would make it less flexible. Currently the patching works by coincidence but a refactoring of the generic code could break that (unless we make it officially non-const constrain refactorings).
I would like to propose an alternative: Leave it to the usual content sync'ing in `flashrom`. But provide another tool that uses libflashrom and takes care of everything, including the project check and the autoload patching. I would be happy to help with the new CLI code if that is an issue.
https://review.coreboot.org/c/flashrom/+/55715/comment/df328cf8_c7780104 PS29, Line 603:
Probably we can set the erase size to 256 bytes or 1024 bytes, this needs testing. […]
I was more wondering about erasing 4 * 64KiB individually instead of 256KiB at once (what the code tells flashrom to do). Multiples of 64KiB is what the first if in ite_ecfw_erase() enforces. But we send individual commands per chunk indeed. Somewhat odd. Let's leave it for now.
https://review.coreboot.org/c/flashrom/+/55715/comment/2e5af0f4_54a31a8a PS29, Line 696: }
It looks like RDID, but the EC doesn't return reliable data. […]
Does it show something useful yet?
https://review.coreboot.org/c/flashrom/+/55715/comment/7999a2b2_f02f5cf7 PS29, Line 812: Probing
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.
What I mean is that the message says we would force "probing", but it's more a force using this driver. Not important atm.
https://review.coreboot.org/c/flashrom/+/55715/comment/24b63c24_d4d557e5 PS29, Line 905: int ite_ecfw_verify_file_project(uint8_t *const newcontents,
Currently not, but I wish it would be at some point. […]
We could probably find a place for a hook. Could that also be used for the autoload patching? But again, it might fit a separate tool much better.
-
Nico Huber (Code Review)