Attention is currently required from: Paul Menzel. Sean Rhodes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58343 )
Change subject: ec/starlabs: Add standardised ITE EC support ......................................................................
Patch Set 8:
(9 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/58343/comment/3c434143_7b4c2220 PS5, Line 14:
Wow, thank you very much. Could you please elaborate a little? […]
Done
File src/ec/starlabs/merlin/Kconfig:
https://review.coreboot.org/c/coreboot/+/58343/comment/bb696321_bf526435 PS5, Line 7: ITE embedded controller
… with XXX firmware …
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/d579c068_4cf5e9b3 PS5, Line 28: Use open source embedded controller firmware
- Please add an URL to the repository or Web site. […]
It's best to add that later; just need to get the coreboot side stuff upstream so rebasing stops breaking it every week!.
The plan is to add it as a submodule and have it built alongside coreboot, plus we need to make it work with fwupd. They're pretty much the same, hence sharing all this stuff.
File src/ec/starlabs/merlin/ec.c:
https://review.coreboot.org/c/coreboot/+/58343/comment/5b0216e9_a3e91f51 PS5, Line 39: printk(BIOS_DEBUG, "ITE: Device not found.\n");
Should that be an error? […]
Yes!
coreboot can't talk to the EC - so not much works!
https://review.coreboot.org/c/coreboot/+/58343/comment/ebfd89fe_0da445ee PS5, Line 57: *
Remove the blank line?
Done
File src/ec/starlabs/merlin/variants/cml/ecdefs.h:
https://review.coreboot.org/c/coreboot/+/58343/comment/0e478006_8b30f724 PS5, Line 4: * EC communication interface for ITE ITE Embedded Controller.
Please remove the dot/period at the end, as it’s not a sentence.
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/d08e9062_6155833e PS5, Line 4: ITE ITE Embedded Controller
Is exactly one ITE enough?
Always good to have a spare :)
https://review.coreboot.org/c/coreboot/+/58343/comment/a799ca93_bb237e91 PS5, Line 10: /* IT5570 chip ID byte values. */
Ditto.
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/ef5e62c5_b54fe61b PS5, Line 14: /* EC RAM offsets. */
Ditto. (End probably other files. […]
Done
-
Sean Rhodes (Code Review)