Attention is currently required from: Sean Rhodes, Andy Pont, Paul Menzel, Felix Held. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58343 )
Change subject: ec/starlabs: Add standardised ITE EC support ......................................................................
Patch Set 43: Code-Review+1
(22 comments)
File src/ec/starlabs/merlin/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/58343/comment/74ee936d_5e6b7e11 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:
EC_VARIANT_DIR := $(call strip_quotes, $(CONFIG_EC_VARIANT_DIR)) CPPFLAGS_common += -I$(src)/ec/starlabs/merlin/variants/$(EC_VARIANT_DIR)
https://review.coreboot.org/c/coreboot/+/58343/comment/8a77e12d_7fe632ed PS42, Line 9: ifeq ($(CONFIG_EC_STARLABS_ITE),y) This should also guard the lines above, this file is unconditionally included.
https://review.coreboot.org/c/coreboot/+/58343/comment/e01709ac_def8d011 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. I'd remove it.
https://review.coreboot.org/c/coreboot/+/58343/comment/6cd6ddb3_223ab2c6 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 warning is only printed when `EC_STARLABS_IT_BIN` is set but `EC_STARLABS_IT_BIN_PATH` is completely empty (which I think never happens).
File src/ec/starlabs/merlin/ec.c:
https://review.coreboot.org/c/coreboot/+/58343/comment/71991a51_d9cd8918 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.
Alternatively, why not combine the two bytes as in `it_get_version()`?
static u16 ite_get_chip_id(unsigned int port) { return (pnp_read_index(port, ITE_CHIPID1) << 8) | pnp_read_index(port, ITE_CHIPID2); }
I've used the `unsigned int` type for the `port` parameter because it's the type of `dev->path.pnp.port` (c.f. `struct pnp_path` in `src/include/device/path.h`), even though `pnp_read_index()` expects an `u16`.
In any case, this would be used as follows:
const u16 chip_id = ite_get_chip_id(dev->path.pnp.port); if (chip_id != ITE_CHIPID_VAL) { printk(BIOS_ERR, "ITE: Device not found.\n"); return; }
The chip ID defines would need to be adapted as follows:
#define ITE_CHIPID_VAL 0x8987
I'd also remove the preceding comments (several of which are wrong, I commented about it). I'm pretty sure you can see that this ID corresponds to the ITE IT8987 😜
https://review.coreboot.org/c/coreboot/+/58343/comment/f513a716_d0f90b95 PS42, Line 50: printk(BIOS_ERR, "ITE: Device not found.\n"); Idea: print the expected and actual IDs?
printk(BIOS_ERR, "ITE: Expected chip ID 0x%04x, but got 0x%04x instead.\n", ITE_CHIPID_VAL, chip_id);
https://review.coreboot.org/c/coreboot/+/58343/comment/6c4fcf7e_6ff57bc4 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 needed when the payload doesn't do keyboard init.
To avoid potential confusion, I'd remove this log message. `pc_keyboard_init()` already prints stuff when doing keyboard init.
File src/ec/starlabs/merlin/variants/apl/ecdefs.h:
https://review.coreboot.org/c/coreboot/+/58343/comment/7594e63c_4cea90b3 PS42, Line 13: IT5570 Looks like IT8987
File src/ec/starlabs/merlin/variants/apl/emem.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/c862f081_badbc145 PS42, Line 3: 0xFF This makes the OperationRegion 0xff = 255 bytes long. Shouldn't it be 256?
https://review.coreboot.org/c/coreboot/+/58343/comment/573f137a_26869e45 PS42, Line 172: // TODO: : // Causes build failure, but 0xff is in range : // EJ8E, 8, // EJ898A Error See comment on OperationRegion declaration at the start of the file
File src/ec/starlabs/merlin/variants/cml/ecdefs.h:
https://review.coreboot.org/c/coreboot/+/58343/comment/315c3ef0_1dede964 PS42, Line 10: IT5570 Looks like IT8987
File src/ec/starlabs/merlin/variants/cml/emem.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/581b1222_6cdb17e3 PS42, Line 3: 0xFF This makes the OperationRegion 0xff = 255 bytes long. Shouldn't it be 256?
File src/ec/starlabs/merlin/variants/glk/ecdefs.h:
https://review.coreboot.org/c/coreboot/+/58343/comment/3c835239_9e5091c5 PS42, Line 13: IT5570 Looks like IT8987
File src/ec/starlabs/merlin/variants/glk/emem.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/0439802b_62b30ba2 PS42, Line 3: 0xFF This makes the OperationRegion 0xff = 255 bytes long. Shouldn't it be 256?
https://review.coreboot.org/c/coreboot/+/58343/comment/cc2d2872_5550c621 PS42, Line 172: // TODO: : // Causes build failure, but 0xff is in range : // EJ8E, 8, // EJ898A Error See comment on OperationRegion declaration at the start of the file
File src/ec/starlabs/merlin/variants/kbl/ecdefs.h:
https://review.coreboot.org/c/coreboot/+/58343/comment/c79075a3_ed2034f7 PS42, Line 10: IT5570 Looks like IT8987
File src/ec/starlabs/merlin/variants/kbl/emem.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/cfd954c5_76bd6fba PS42, Line 3: 0xFF This makes the OperationRegion 0xff = 255 bytes long. Shouldn't it be 256?
https://review.coreboot.org/c/coreboot/+/58343/comment/c2ee76ee_7a4ac92a PS42, Line 159: // TODO: : // Causes build failure, but 0xff is in range : // EJ8E, 8, // EJ898A Error See comment on OperationRegion declaration at the start of the file
File src/ec/starlabs/merlin/variants/merlin/emem.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/ada869ae_4f3230c2 PS42, Line 3: 0xFF This makes the OperationRegion 0xff = 255 bytes long. Shouldn't it be 256?
https://review.coreboot.org/c/coreboot/+/58343/comment/3d1566e1_0a42dd0c PS42, Line 160: H nit: lowercase `h`
File src/ec/starlabs/merlin/variants/tgl/emem.asl:
https://review.coreboot.org/c/coreboot/+/58343/comment/2e3a8ffe_a6d47f64 PS42, Line 3: 0xFF This makes the OperationRegion 0xff = 255 bytes long. Shouldn't it be 256?
https://review.coreboot.org/c/coreboot/+/58343/comment/6d63438f_0a5e06ff PS42, Line 167: H nit: lowercase `h`
-
Angel Pons (Code Review)