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 49: Code-Review+1
(3 comments)
File src/ec/starlabs/merlin/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/58343/comment/af2d23ef_6ea1f381 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"
That's what it was meant to do - only CML and TGL need the EC bin, and if they are built without it, […]
Ok, then we need another Kconfig option: `EC_STARLABS_NEED_ITE_BIN` or similar name:
config EC_STARLABS_NEED_ITE_BIN bool depends on EC_STARLABS_ITE help Select if the mainboard requires EC firmware in the main flash chip.
config EC_STARLABS_ADD_ITE_BIN bool "Add Star Labs EC binary file" default n depends on EC_STARLABS_NEED_ITE_BIN help Select to add an EC firmware binary into the coreboot image. EC firmware is necessary, flashing a coreboot image without EC firmware will render your laptop unusable.
config EC_STARLABS_ITE_BIN_PATH string "Star Labs EC binary file path" depends on EC_STARLABS_ADD_ITE_BIN default ""
The idea of `EC_STARLABS_NEED_ITE_BIN` is to only show the options to add the EC firmware for boards that actually need it. Mainboards which need EC firmware and whose EC firmware is available in the blobs repo would do the following:
config BOARD_STARLABS_STARBOOK_TGL select EC_STARLABS_NEED_ITE_BIN
config EC_STARLABS_ADD_ITE_BIN default y
config EC_STARLABS_ITE_BIN_PATH default "3rdparty/blobs/mainboard/starlabs/..."
This works because mainboard scope defaults take precedence over EC scope defaults (Mainboard Kconfig is sourced before EC Kconfig). Note that the user-visible `EC_STARLABS_ADD_ITE_BIN` option isn't selected: selects force-enable an option, and someone might want to build a coreboot image without the EC firmware for some reason (unusual, but no need to restrict the decision).
File src/ec/starlabs/merlin/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/58343/comment/63840d1b_d9eb643a PS49, Line 18: CONFIG_EC_STARLABS_IT_BIN_PATH Hmmm, just realized this Kconfig option doesn't exist. I assumed this is a string option with a prompt (in case the user wants to use a different EC firmware). Even with the prompt, boards can still provide default paths to the files in the blobs repo.
Anyway, see complete suggestion in the comment below.
File src/ec/starlabs/merlin/ec.c:
https://review.coreboot.org/c/coreboot/+/58343/comment/8f304397_6329b672 PS44, Line 47: static u16 ite_get_chip_id(unsigned int port) : { : return (pnp_read_index(port, ITE_CHIPID1) << 8) | : pnp_read_index(port, ITE_CHIPID2); : } :
Done
Indeed, sorry if I wasn't clear about it