Attention is currently required from: Felix Singer, Michał Żygowski, Michał Kopeć, Michael Niewöhner. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62498 )
Change subject: mb/clevo/tgl-u: Add Clevo NV41 Tiger Lake laptop support ......................................................................
Patch Set 3:
(38 comments)
File src/mainboard/clevo/tgl-u/Kconfig:
https://review.coreboot.org/c/coreboot/+/62498/comment/027038c3_3a170c7b PS3, Line 13: MAINBOARD_HAS_LPC_TPM MEMORY_MAPPED_TPM
https://review.coreboot.org/c/coreboot/+/62498/comment/8895ab1c_7486d33e PS3, Line 14: select NO_UART_ON_SUPERIO Shouldn't be needed, selecting `INTEL_LPSS_UART_FOR_CONSOLE` should already provide the necessary implications.
https://review.coreboot.org/c/coreboot/+/62498/comment/f3880664_238a95ff PS3, Line 20: select HAVE_SMI_HANDLER SoC already selects this
https://review.coreboot.org/c/coreboot/+/62498/comment/49137e6e_8f55dd3c PS3, Line 22: select TPM_MEASURED_BOOT Why is it necessary to force people to use measured boot? This option is user-configurable, selecting it prevents user configuration.
https://review.coreboot.org/c/coreboot/+/62498/comment/450defdb_0fa82471 PS3, Line 25: bool "NV4x" Why does this have a prompt? You can simply omit the line, the type only needs to be specified once (see my other comments).
https://review.coreboot.org/c/coreboot/+/62498/comment/ddb3e919_622880ee PS3, Line 32: string Type not needed
https://review.coreboot.org/c/coreboot/+/62498/comment/8730522f_a7c23733 PS3, Line 36: string Type not needed
https://review.coreboot.org/c/coreboot/+/62498/comment/b3de50f7_bfccae5d PS3, Line 40: string Type not needed
https://review.coreboot.org/c/coreboot/+/62498/comment/234343e0_0f8d744e PS3, Line 43: config MAX_CPUS : int : default 8 Is this actually needed?
https://review.coreboot.org/c/coreboot/+/62498/comment/41b1cb98_a19573f6 PS3, Line 47: config PCIEXP_HOTPLUG : default y Redundant, given the select statement at the start of the file.
https://review.coreboot.org/c/coreboot/+/62498/comment/ea91a212_626f90cf PS3, Line 50: config PCIEXP_HOTPLUG_BUSES : int : default 42 : : config PCIEXP_HOTPLUG_MEM : hex : default 0xc200000 # 194 MiB : : config PCIEXP_HOTPLUG_PREFETCH_MEM : hex : default 0x1c00000 # 448 MiB Where do these numbers come from?
https://review.coreboot.org/c/coreboot/+/62498/comment/efbded88_a5933753 PS3, Line 63: string Type not needed
https://review.coreboot.org/c/coreboot/+/62498/comment/87b99ce5_104ed262 PS3, Line 67: string Type not needed
https://review.coreboot.org/c/coreboot/+/62498/comment/44731b42_573a3d7b PS3, Line 70: config DIMM_MAX : int : default 4 # Hack to make soc code work Huh?
https://review.coreboot.org/c/coreboot/+/62498/comment/8d831baa_8c6a61b5 PS3, Line 75: int Type probably not needed
https://review.coreboot.org/c/coreboot/+/62498/comment/7774625d_ddf4a74e PS3, Line 90: string Type not needed
File src/mainboard/clevo/tgl-u/acpi/mainboard.asl:
https://review.coreboot.org/c/coreboot/+/62498/comment/77f2ca49_28b9161c PS3, Line 8: // echo '_SB.SLPB._PRW' | sudo tee /proc/acpi/call; sudo cat /proc/acpi/call : // echo '_SB.PC00.LPCB.EC._GPE' | sudo tee /proc/acpi/call; sudo cat /proc/acpi/call What is this for?
File src/mainboard/clevo/tgl-u/acpi/sleep.asl:
https://review.coreboot.org/c/coreboot/+/62498/comment/ed1bdc28_fa2687ec PS3, Line 20: PGPM (MISCCFG_GPIO_PM_CONFIG_BITS) Why doesn't SoC code handle this?
File src/mainboard/clevo/tgl-u/bootblock.c:
https://review.coreboot.org/c/coreboot/+/62498/comment/9f38896f_3b36bc76 PS1, Line 16: if (onoff) { : gpio_set(DGPU_RST_N, 0); : mdelay(4); : gpio_set(DGPU_PWR_EN, 1); : mdelay(4); : gpio_set(DGPU_RST_N, 1); : } else { : gpio_set(DGPU_RST_N, 0); : mdelay(4); : gpio_set(DGPU_PWR_EN, 0);
Aren't the power sequence GPIOs mainboard-specific? The sequence may also depend on other stuff. […]
Also, no polling for PWROK/PG to assert before releasing dGPU reset?
File src/mainboard/clevo/tgl-u/bootblock.c:
https://review.coreboot.org/c/coreboot/+/62498/comment/2da07e5f_15b8a618 PS3, Line 14: int Why not `bool`?
https://review.coreboot.org/c/coreboot/+/62498/comment/c92eb27d_a14e9d07 PS3, Line 14: static void dgpu_power_enable(int onoff) {
open brace '{' following function definitions go on the next line
Please fix.
https://review.coreboot.org/c/coreboot/+/62498/comment/fa065b62_3d259c40 PS3, Line 15: %d I'd use `%s` and print `onoff ? "on" : "off"`
File src/mainboard/clevo/tgl-u/cmos.layout:
https://review.coreboot.org/c/coreboot/+/62498/comment/4b42e582_599dca13 PS3, Line 11: #395 4 e 3 debug_level Why is this commented out?
https://review.coreboot.org/c/coreboot/+/62498/comment/84e32ba1_36277f49 PS3, Line 12: 408 1 h 1 preserve_smmstore What does this do?
File src/mainboard/clevo/tgl-u/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/62498/comment/cc871af9_526d2631 PS3, Line 4: #include <baseboard/gpio.h> Is it possible to include this from the file that needs it?
https://review.coreboot.org/c/coreboot/+/62498/comment/6508aeca_0c6a98cb PS3, Line 12: /* OEM revision */ Is it, though?
https://review.coreboot.org/c/coreboot/+/62498/comment/ad9db19d_97084e2e PS3, Line 36: // Mainboard specific Redundant comment, please kill with fire
File src/mainboard/clevo/tgl-u/romstage.c:
https://review.coreboot.org/c/coreboot/+/62498/comment/71683ab9_dccfd703 PS3, Line 33: const uint8_t vtd = get_uint_option("vtd", 1); : memupd->FspmConfig.VtdDisable = !vtd; : : const uint8_t ht = get_uint_option("hyper_threading", : memupd->FspmConfig.HyperThreading); : memupd->FspmConfig.HyperThreading = ht; I don't see these in cmos.layout
File src/mainboard/clevo/tgl-u/variants/baseboard/devicetree.cb:
PS1:
We don't use a baseboard directory in the clevo mainboards. […]
+1, the concept of `variants/baseboard` gives me aneurysms.
File src/mainboard/clevo/tgl-u/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/62498/comment/0671bdb6_ad22956f PS3, Line 3: .chipset_lockdown = CHIPSET_LOCKDOWN_COREBOOT, Should be the default already?
https://review.coreboot.org/c/coreboot/+/62498/comment/7ee5e115_4bc3fe91 PS3, Line 13: # Disable DPTF : register "dptf_enable" = "0" Just omit it then?
https://review.coreboot.org/c/coreboot/+/62498/comment/b2b5ba0e_0ec3dffc PS3, Line 22: required Really?
https://review.coreboot.org/c/coreboot/+/62498/comment/f1749506_4f52313e PS3, Line 26: # Enable C6 DRAM Please don't restate the obvious
https://review.coreboot.org/c/coreboot/+/62498/comment/8507d58d_08ee5607 PS3, Line 33: # Acoustic settings This is not useful, please drop.
File src/mainboard/clevo/tgl-u/variants/nv4x/gpio_early.c:
https://review.coreboot.org/c/coreboot/+/62498/comment/9f71917f_889c5239 PS3, Line 11: PAD_CFG_TERM_GPO(GPP_U4, 0, NONE, DEEP), /* DGPU_RST#_PCH */ : PAD_CFG_TERM_GPO(GPP_U5, 0, NONE, DEEP), /* DGPU_PWR_EN */ Why not use `PAD_CFG_GPO` instead?
Also, I guess unconditionally configuring these pads is not a problem because HAVE_ACPI_RESUME is not selected.
File src/mainboard/clevo/tgl-u/variants/nv4x/overridetree.cb:
PS3: Are any PCIe RPs wired to a slot?
https://review.coreboot.org/c/coreboot/+/62498/comment/5d515dbf_4478ee0e PS3, Line 3: # Power limits : # TODO: Check if this is correct : register "power_limits_config[POWER_LIMITS_U_4_CORE]" = "{ : .tdp_pl1_override = 10, : .tdp_pl2_override = 20, : }" : register "power_limits_config[POWER_LIMITS_U_2_CORE]" = "{ : .tdp_pl1_override = 10, : .tdp_pl2_override = 20, : }" Why not just drop it?
https://review.coreboot.org/c/coreboot/+/62498/comment/2c3a3abf_3f68537a PS3, Line 163: # Pantone ROM W-what is this for?