Attention is currently required from: Felix Singer, Michał Żygowski, Angel Pons, Michael Niewöhner. Michał Kopeć has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62498 )
Change subject: mb/clevo/tgl-u: Add Clevo NV4x Tiger Lake laptop support ......................................................................
Patch Set 6:
(31 comments)
Patchset:
PS6: Rebased on master, where the correct `clevo/tgl-u` structure is already present.
File src/mainboard/clevo/tgl-u/Kconfig:
https://review.coreboot.org/c/coreboot/+/62498/comment/efae5f71_a46f450e PS3, Line 14: select NO_UART_ON_SUPERIO
Shouldn't be needed, selecting `INTEL_LPSS_UART_FOR_CONSOLE` should already provide the necessary im […]
File removed
https://review.coreboot.org/c/coreboot/+/62498/comment/46e7143f_7c412a56 PS3, Line 20: select HAVE_SMI_HANDLER
SoC already selects this
File removed
https://review.coreboot.org/c/coreboot/+/62498/comment/960893b9_f82e7519 PS3, Line 22: select TPM_MEASURED_BOOT
Why is it necessary to force people to use measured boot? This option is user-configurable, selectin […]
File removed
https://review.coreboot.org/c/coreboot/+/62498/comment/03986fb8_fbf7bc2a 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 […]
Prompt removed
https://review.coreboot.org/c/coreboot/+/62498/comment/e19327b6_aa632108 PS3, Line 32: string
Type not needed
Removed
https://review.coreboot.org/c/coreboot/+/62498/comment/5e02c307_8967a233 PS3, Line 36: string
Type not needed
Removed
https://review.coreboot.org/c/coreboot/+/62498/comment/71c36132_fed960fe PS3, Line 40: string
Type not needed
Option removed
https://review.coreboot.org/c/coreboot/+/62498/comment/8a239bb1_f3b6985e PS3, Line 43: config MAX_CPUS : int : default 8
Is this actually needed?
Removed
https://review.coreboot.org/c/coreboot/+/62498/comment/d0f7c048_c3740be9 PS3, Line 47: config PCIEXP_HOTPLUG : default y
Redundant, given the select statement at the start of the file.
Removed
https://review.coreboot.org/c/coreboot/+/62498/comment/98fc6ccf_9f0de31c 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?
Removed
https://review.coreboot.org/c/coreboot/+/62498/comment/65b6a543_52918f75 PS3, Line 63: string
Type not needed
Removed
https://review.coreboot.org/c/coreboot/+/62498/comment/857dd132_3d0b6b9c PS3, Line 67: string
Type not needed
Removed
https://review.coreboot.org/c/coreboot/+/62498/comment/cb4cef49_631d83f6 PS3, Line 70: config DIMM_MAX : int : default 4 # Hack to make soc code work
Huh?
Removed
https://review.coreboot.org/c/coreboot/+/62498/comment/98be4572_ac928063 PS3, Line 75: int
Type probably not needed
Removed
https://review.coreboot.org/c/coreboot/+/62498/comment/6cdf3e8b_f3dc5af6 PS3, Line 90: string
Type not needed
Removed
File src/mainboard/clevo/tgl-u/board.fmd:
PS1:
Nothing special here. All handled automatically by build system. Michał you can remove the file.
Removed
File src/mainboard/clevo/tgl-u/bootblock.c:
https://review.coreboot.org/c/coreboot/+/62498/comment/ed911ba3_5394eb6a 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);
the gpios might be mb specific; we still have APIs to handle that in a generic way
Removed
File src/mainboard/clevo/tgl-u/bootblock.c:
https://review.coreboot.org/c/coreboot/+/62498/comment/b6798d58_c71feff2 PS3, Line 14: int
Why not `bool`?
Removed - for now, until the nvidia driver is completed
https://review.coreboot.org/c/coreboot/+/62498/comment/0c22a704_975e8a37 PS3, Line 14: static void dgpu_power_enable(int onoff) {
open brace '{' following function definitions go on the next line […]
Fixed
https://review.coreboot.org/c/coreboot/+/62498/comment/59c60cd2_75468187 PS3, Line 15: %d
I'd use `%s` and print `onoff ? "on" : "off"`
Removed
File src/mainboard/clevo/tgl-u/cmos.layout:
https://review.coreboot.org/c/coreboot/+/62498/comment/dc69a9a1_11d686bf PS3, Line 11: #395 4 e 3 debug_level
Why is this commented out?
Removed
https://review.coreboot.org/c/coreboot/+/62498/comment/6ec7d02c_fedfc614 PS3, Line 12: 408 1 h 1 preserve_smmstore
What does this do?
Removed
File src/mainboard/clevo/tgl-u/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/62498/comment/90e4167a_fbb4aee0 PS3, Line 4: #include <baseboard/gpio.h>
Is it possible to include this from the file that needs it?
File removed
https://review.coreboot.org/c/coreboot/+/62498/comment/7c5193eb_478356d9 PS3, Line 12: /* OEM revision */
Is it, though?
Removed
https://review.coreboot.org/c/coreboot/+/62498/comment/d32de6df_c37bdbd7 PS3, Line 36: // Mainboard specific
Redundant comment, please kill with fire
Killed
File src/mainboard/clevo/tgl-u/memory.c:
PS1:
Rename this file to romstage. […]
Done
File src/mainboard/clevo/tgl-u/romstage.c:
PS1:
Move this file to variant/nv4x/
Moved
File src/mainboard/clevo/tgl-u/romstage.c:
https://review.coreboot.org/c/coreboot/+/62498/comment/ad05e6ef_6b1c3b5a 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. […]
Copy-paste error. Removed
File src/mainboard/clevo/tgl-u/variants/baseboard/devicetree.cb:
PS1:
+1, the concept of `variants/baseboard` gives me aneurysms.
Removed `baseboard` dir
File src/mainboard/clevo/tgl-u/variants/baseboard/include/baseboard/variants.h:
PS1:
Same here. Move the include folder to mainboard/clevo/tgl-u. […]
Done