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 7:
(14 comments)
File src/mainboard/clevo/tgl-u/Kconfig:
https://review.coreboot.org/c/coreboot/+/62498/comment/0ebb6fcf_903ccb4e PS6, Line 60: GPP_C14
GPP_C14_IRQ
Done
File src/mainboard/clevo/tgl-u/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/62498/comment/3993ad45_a8ced969 PS3, Line 13: # Disable DPTF : register "dptf_enable" = "0"
Just omit it then?
Removed
https://review.coreboot.org/c/coreboot/+/62498/comment/5b4648cf_19e4a6d8 PS3, Line 26: # Enable C6 DRAM
Please don't restate the obvious
Comment removed
https://review.coreboot.org/c/coreboot/+/62498/comment/c9c4a342_03eec4a2 PS3, Line 33: # Acoustic settings
This is not useful, please drop.
Comment removed
File src/mainboard/clevo/tgl-u/variants/nv40mz/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/62498/comment/1b354a20_1db45f3a PS6, Line 3: bootblock-y += gpio.c : : romstage-y += romstage.c : : ramstage-y += gpio.c : ramstage-y += hda_verb.c :
not needed. covered by tgl-u/Makefile. […]
File removed
File src/mainboard/clevo/tgl-u/variants/nv40mz/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/62498/comment/b135ea1a_fb8a4416 PS6, Line 13: # Disable DPTF : register "dptf_enable" = "0"
This can be dropped, default is 0
Removed
https://review.coreboot.org/c/coreboot/+/62498/comment/cda55770_08a15890 PS6, Line 83: register "CnviBtCore" = "true"
move down to cnvi dev
Moved
https://review.coreboot.org/c/coreboot/+/62498/comment/8af45b5d_c1b344f5 PS6, Line 86: GPE configuration : register "pmc_gpe0_dw0" = "PMC_GPP_C" : register "pmc_gpe0_dw1" = "PMC_GPP_E" : register "pmc_gpe0_dw2" = "PMC_GPD" :
move down to pmc dev?
Moved
File src/mainboard/clevo/tgl-u/variants/nv40mz/gpio.c:
https://review.coreboot.org/c/coreboot/+/62498/comment/89a9ac3c_73a6d19b PS6, Line 210: PAD_CFG_GPI_SMI(GPP_E7, NONE, PLTRST, EDGE_SINGLE, INVERT),
board uses espi vwire -> don't configure the gpio
Changed to `PAD_NC`
File src/mainboard/clevo/tgl-u/variants/nv40mz/ramstage.c:
https://review.coreboot.org/c/coreboot/+/62498/comment/23dc3f95_cf1e8005 PS6, Line 12: params->CpuPcieRpAdvancedErrorReporting[0] = 0;
also params->CpuPcieRpLtrEnable[0] = 1;
Added both params
File src/mainboard/clevo/tgl-u/variants/nv4x/gpio_early.c:
https://review.coreboot.org/c/coreboot/+/62498/comment/9f5a70f4_3d63770b 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? […]
Switched to `PAD_CFG_GPO`
File src/mainboard/clevo/tgl-u/variants/nv4x/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/62498/comment/217bb035_a6605c76 PS3, Line 4: # TODO: Check if this is correct
it's not wrong, but the fans should be powerful enough to handle way more power
Right, removed the entire power limit config
https://review.coreboot.org/c/coreboot/+/62498/comment/ab01096d_f31977a1 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?
Dropped
https://review.coreboot.org/c/coreboot/+/62498/comment/87241d61_03f54579 PS3, Line 42: register "HybridStorageMode" = "0"
not needed, 0 is the default
Removed