Attention is currently required from: Andy Pont, Paul Menzel, Angel Pons, Felix Held. 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 44:
(19 comments)
File src/ec/starlabs/merlin/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/58343/comment/0a4b68d6_729a346e PS42, Line 7: $(CONFIG_EC_VARIANT_DIR)
I'm pretty sure you need to use `strip_quotes` here, in CB:29447 I ran into the same issue: […]
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/4f81e55e_fcd2b253 PS42, Line 9: ifeq ($(CONFIG_EC_STARLABS_ITE),y)
This should also guard the lines above, this file is unconditionally included.
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/e5552865_156aa5fb PS42, Line 11: smm-$(CONFIG_DEBUG_SMI) += ec.c
I'm pretty sure this isn't needed, and causes build errors when `DEBUG_SMI` is enabled. […]
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/89065efc_e78968b8 PS42, Line 18: files_added:: warn_no_ite_fw : : PHONY+=warn_no_ite_fw : warn_no_ite_fw: : printf "\n\t** WARNING **\n" : printf "coreboot has been built without the ITE EC Firmware.\n" : printf "Do not flash this image. Your laptop's power button\n" : printf "may not respond when you press it.\n\n"
Shouldn't this be in the `else` branch of `ifeq ($(CONFIG_EC_STARLABS_IT_BIN),y)`? Currently, the wa […]
That's what it was meant to do - only CML and TGL need the EC bin, and if they are built without it, the world will end.
File src/ec/starlabs/merlin/ec.c:
https://review.coreboot.org/c/coreboot/+/58343/comment/07256f39_66d8c597 PS42, Line 47: u8 chipid1 = pnp_read_index(dev->path.pnp.port, ITE_CHIPID1); : u8 chipid2 = pnp_read_index(dev->path.pnp.port, ITE_CHIPID2);
nit: `chipid1` and `chipid2` can be const. […]
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/44a7e3ee_2a2a2b86 PS42, Line 50: printk(BIOS_ERR, "ITE: Device not found.\n");
Idea: print the expected and actual IDs? […]
Nice!
https://review.coreboot.org/c/coreboot/+/58343/comment/0efa8893_2f092212 PS42, Line 54: printk(BIOS_DEBUG, "ITE: Initializing keyboard.\n");
Strictly speaking, coreboot only does keyboard init if `DRIVERS_PS2_KEYBOARD` is set, which is only […]
Done
File src/ec/starlabs/merlin/variants/apl/ecdefs.h:
https://review.coreboot.org/c/coreboot/+/58343/comment/96aac706_c3597fe4 PS42, Line 13: IT5570
Looks like IT8987
Done
File src/ec/starlabs/merlin/variants/apl/emem.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/72668a37_4ff7bd9b PS42, Line 3: 0xFF
This makes the OperationRegion 0xff = 255 bytes long. […]
Done
File src/ec/starlabs/merlin/variants/cml/ecdefs.h:
https://review.coreboot.org/c/coreboot/+/58343/comment/85bfd845_f062406c PS42, Line 10: IT5570
Looks like IT8987
Done
File src/ec/starlabs/merlin/variants/cml/emem.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/b8fd8596_5613c796 PS42, Line 3: 0xFF
This makes the OperationRegion 0xff = 255 bytes long. […]
Done
File src/ec/starlabs/merlin/variants/glk/ecdefs.h:
https://review.coreboot.org/c/coreboot/+/58343/comment/bc2d60ac_49a8c1e8 PS42, Line 13: IT5570
Looks like IT8987
Done
File src/ec/starlabs/merlin/variants/glk/emem.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/56eab6f0_8046d0e0 PS42, Line 3: 0xFF
This makes the OperationRegion 0xff = 255 bytes long. […]
Done
File src/ec/starlabs/merlin/variants/kbl/ecdefs.h:
https://review.coreboot.org/c/coreboot/+/58343/comment/2c82dd27_890c9ee0 PS42, Line 10: IT5570
Looks like IT8987
Done
File src/ec/starlabs/merlin/variants/kbl/emem.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/bc7268ff_884eeb36 PS42, Line 3: 0xFF
This makes the OperationRegion 0xff = 255 bytes long. […]
Done
File src/ec/starlabs/merlin/variants/merlin/emem.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/b462f4e3_5115951e PS42, Line 3: 0xFF
This makes the OperationRegion 0xff = 255 bytes long. […]
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/6773b8f3_3e89e30e PS42, Line 160: H
nit: lowercase `h`
Done
File src/ec/starlabs/merlin/variants/tgl/emem.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/e8b4961d_b9c98cdd PS42, Line 3: 0xFF
This makes the OperationRegion 0xff = 255 bytes long. […]
Done
https://review.coreboot.org/c/coreboot/+/58343/comment/8dbe210d_5108dc3b PS42, Line 167: H
nit: lowercase `h`
Done
-
Sean Rhodes (Code Review)