Attention is currently required from: Alexander Couzens, Angel Pons, Felix Singer, Jan Philipp Groß, Paul Menzel.
Nicholas Chin 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 5:
(8 comments)
File src/mainboard/lenovo/m900/Kconfig:
https://review.coreboot.org/c/coreboot/+/74187/comment/e112db4a_2106208b?usp... : PS4, Line 36: config PRERAM_CBMEM_CONSOLE_SIZE : default 0xd00
Why is the default value not suitable?
Copypasta from the H110. The console seems to be fine with the default value, so I'll remove the override here.
File src/mainboard/lenovo/m900/acpi/dptf.asl:
PS4:
Was this tested?
No. Should I remove it?
File src/mainboard/lenovo/m900/cmos.layout:
https://review.coreboot.org/c/coreboot/+/74187/comment/a618cc07_a463d427?usp... : PS4, Line 24: 408 1 e 1 nmi
Skylake doesn't implement this option, IIRC.
Done
https://review.coreboot.org/c/coreboot/+/74187/comment/4774e0e8_03839fc3?usp... : PS4, Line 28: ChromeOS
Hmmmmm... […]
It seems like there is an intention to deprecate VBOOT_VBNV_CMOS (https://issuetracker.google.com/issues/235293589; you posted this link on my E6400 patch before). Seems like the code for it is still present though (`security/vboot/vbnv_cmos.c`), and Skylake also seems to select `VBOOT_VBNV_CMOS` and `VBOOT_VBNV_CMOS_BACKUP_TO_FLASH` in `soc/intel/skylake/Kconfig`.
File src/mainboard/lenovo/m900/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/74187/comment/bb0388c9_52797b3e?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. […]
Looks like it was added in commit 9a8b67d0af6a ("soc/intel/skylake: Enable another VR mailbox command for certain boards"). Seems like it has something to do with hangs during S0ix and the IVMP8 PS4 power state (can't find much information about that). I've removed it.
File src/mainboard/lenovo/m900/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/74187/comment/46a958a8_d3d218cb?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.
Done. The IPU seems to be pci 14.3, which only seems to be on the mobile U/Y single chip platforms based on the datasheets.
File src/mainboard/lenovo/m900/ramstage.c:
https://review.coreboot.org/c/coreboot/+/74187/comment/33a97e66_a38e980a?usp... : PS4, Line 12: params->CdClock = 3;
What does this do? Is this needed?
Seems like it corresponds to the Core Display Clock Frequency selection, based on a comment in `soc/intel/jasperlake/chip.h`. Nowhere else seems to expand the abbreviation. Anyway, according to the Kabylake FSP Integration guide it defaults to 3 so its probably safe to just remove.
File src/mainboard/lenovo/m900/romstage.c:
https://review.coreboot.org/c/coreboot/+/74187/comment/8cc4d842_7b3c6c56?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()?
Done