Attention is currently required from: Alexander Couzens, Felix Singer, Jan Philipp Groß, Nicholas Chin, Paul Menzel.
Angel Pons has posted comments on this change by Nicholas Chin. ( https://review.coreboot.org/c/coreboot/+/74187?usp=email )
Change subject: mb/lenovo: Add ThinkCentre M900 (Skylake/LGA 1151) ......................................................................
Patch Set 4:
(10 comments)
File src/mainboard/lenovo/m900/Kconfig:
https://review.coreboot.org/c/coreboot/+/74187/comment/fe3851c4_23376c1b?usp... : PS4, Line 36: config PRERAM_CBMEM_CONSOLE_SIZE : default 0xd00 Why is the default value not suitable?
File src/mainboard/lenovo/m900/acpi/dptf.asl:
PS4: Was this tested?
File src/mainboard/lenovo/m900/bootblock.c:
https://review.coreboot.org/c/coreboot/+/74187/comment/cf7ac9e2_f5b643d4?usp... : PS4, Line 9: /* Change to NCT6687D_SP1 to use COM2 header */ Should probably be a Kconfig
File src/mainboard/lenovo/m900/cmos.layout:
https://review.coreboot.org/c/coreboot/+/74187/comment/672321c4_890e29fb?usp... : PS4, Line 24: 408 1 e 1 nmi Skylake doesn't implement this option, IIRC.
https://review.coreboot.org/c/coreboot/+/74187/comment/af644bc0_1476c065?usp... : PS4, Line 28: ChromeOS Hmmmmm... I think vboot doesn't use this CMOS field anymore?
File src/mainboard/lenovo/m900/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/74187/comment/6573803a_c3329e7f?usp... : PS4, Line 20: # SLP_S3 Minimum Assertion Width. Values 0: 60us, 1: 1ms, 2: 50ms, 3: 2s For another patch: provide an enum in SoC code
https://review.coreboot.org/c/coreboot/+/74187/comment/720f0217_331ac8e0?usp... : PS4, Line 38: register "SendVrMbxCmd" = "2" I'm pretty sure this is copy-pasta from a Chromebook that no one knows what it does. I've had no problems removing it on the HP 280 G2.
File src/mainboard/lenovo/m900/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/74187/comment/f2ab0b6b_a669c7cc?usp... : PS4, Line 25: /* Image processing unit */ : #include <soc/intel/skylake/acpi/ipu.asl> I am pretty sure IPU is not used on a desktop. I'm not even sure if it exists on PCH-H platforms.
File src/mainboard/lenovo/m900/ramstage.c:
https://review.coreboot.org/c/coreboot/+/74187/comment/8dbecf32_2c75c1e2?usp... : PS4, Line 12: params->CdClock = 3; What does this do? Is this needed?
File src/mainboard/lenovo/m900/romstage.c:
https://review.coreboot.org/c/coreboot/+/74187/comment/eaa2cfe4_dac75737?usp... : PS4, Line 21: assert(sizeof(mem_cfg->RcompResistor) == sizeof(rcomp_resistors)); : assert(sizeof(mem_cfg->RcompTarget) == sizeof(rcomp_targets)); nit: move just above corresponding memcpy()?