Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40581 )
Change subject: mb/clevo/n141cu: Add new Comet Lake mainboard ......................................................................
Patch Set 35:
(6 comments)
https://review.coreboot.org/c/coreboot/+/40581/34/src/mainboard/clevo/cml-u/... File src/mainboard/clevo/cml-u/Kconfig:
https://review.coreboot.org/c/coreboot/+/40581/34/src/mainboard/clevo/cml-u/... PS34, Line 14: # select MAINBOARD_HAS_LIBGFXINIT if libgfxinit doesn't support CML, then this should be dropped
https://review.coreboot.org/c/coreboot/+/40581/34/src/mainboard/clevo/cml-u/... PS34, Line 18: ONBOARD_VGA_IS_PRIMARY is this needed?
https://review.coreboot.org/c/coreboot/+/40581/34/src/mainboard/clevo/cml-u/... File src/mainboard/clevo/cml-u/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/40581/34/src/mainboard/clevo/cml-u/... PS34, Line 8: ramstage-$(CONFIG_MAINBOARD_USE_LIBGFXINIT) += variants/$(VARIANT_DIR)/gma-mainboard.ads if libgfxinit doesn't support CML, then this should be dropped
https://review.coreboot.org/c/coreboot/+/40581/34/src/mainboard/clevo/cml-u/... File src/mainboard/clevo/cml-u/ramstage.c:
https://review.coreboot.org/c/coreboot/+/40581/34/src/mainboard/clevo/cml-u/... PS34, Line 7: /* : * TODO: : * - Add kill switches for WLAN, BT, LTE, CCD : * - Add support for WoL (LAN, WLAN) : * - Make M.2 port configurable (SATA <> PCIe) : * - Make SATA DevSlp configurable : * - Make TBT port configurable (TBT <> DisplayPort) : */ generally this is less than ideal unless there's a specific reason these haven't been implemented
https://review.coreboot.org/c/coreboot/+/40581/34/src/mainboard/clevo/cml-u/... File src/mainboard/clevo/cml-u/variants/n141cu/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/40581/34/src/mainboard/clevo/cml-u/... PS34, Line 1: -- SPDX-License-Identifier: GPL-2.0-or-later if libgfxinit doesn't support CML, then this should be dropped.
also, doubtful the board actually has all these ports present
https://review.coreboot.org/c/coreboot/+/40581/34/src/mainboard/clevo/cml-u/... File src/mainboard/clevo/cml-u/variants/n141cu/include/gpio_table.h:
https://review.coreboot.org/c/coreboot/+/40581/34/src/mainboard/clevo/cml-u/... PS34, Line 1: /* SPDX-License-Identifier: GPL-2.0-only */ assume this is just taken from an inteltool dump? the 2nd data word should be & 0xff00 and can be truncated to 4 bytes for readability. And the RSVD GPIOs don't need to be programmed AIUI.
-
Matt DeVillier (Code Review)