Attention is currently required from: Angel Pons, Piotr Król.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/67940 )
Change subject: mb/protectli/vault_cml: Add Comet Lake 6 port board support ......................................................................
Patch Set 9:
(21 comments)
File Documentation/mainboard/protectli/vp46xx.md:
https://review.coreboot.org/c/coreboot/+/67940/comment/d119b501_51461386 PS7, Line 68: - Super I/O serial port 0 via front microUSB connector
Does the board have an onboard USB-to-UART adapter? Wow.
Yes, it does. Fintek F81232.
https://review.coreboot.org/c/coreboot/+/67940/comment/27b517cc_27fb9eeb PS7, Line 90: ITE IT8786E/IT8784E
Are the two chips equivalent, or is there a reason for a board to have one or the other?
The configuration of the registers is the same when compared with the IT8786E datasheet. Although I don't know the differences, since there is no IT8784E public datasheet. not the first time we see such a trick, that suddenly some units have different Super I/O chip, fortunately, this time the change is compatible.
File src/mainboard/protectli/vault_cml/Kconfig:
https://review.coreboot.org/c/coreboot/+/67940/comment/03d42414_cf9051c2 PS7, Line 13: select SOC_INTEL_COMMON_BLOCK_HDA
Already selected by SoC Kconfig
Done
https://review.coreboot.org/c/coreboot/+/67940/comment/dd54b01b_03eec644 PS7, Line 22: config MAINBOARD_DIR : string : default "protectli/vault_cml" : : config MAINBOARD_PART_NUMBER : string : default "VP46XX" : : config MAINBOARD_FAMILY : string : default "Vault Pro"
Specifying the `string` type shouldn't be needed, please drop
Done
https://review.coreboot.org/c/coreboot/+/67940/comment/1c6a30ea_6400a5fe PS7, Line 35: int
Please drop `int` type as per CB:57258
Done
https://review.coreboot.org/c/coreboot/+/67940/comment/b3fcd48a_756bb4e7 PS7, Line 38: config MAX_CPUS : int : default 12
Should already be the default for CML-U
Done
https://review.coreboot.org/c/coreboot/+/67940/comment/57d0888a_0df0a1db PS7, Line 42: config DEVICETREE : string : default "devicetree.cb"
Leftover from variants? Please drop, it's redundant.
Done
File src/mainboard/protectli/vault_cml/cmos.layout:
https://review.coreboot.org/c/coreboot/+/67940/comment/66324866_34861c9a PS7, Line 13: 416 128 r 0 vbnv
There are plans to deprecate support for VBNV in CMOS in favor of VBNV in flash, see mailing list.
Removed.
File src/mainboard/protectli/vault_cml/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/67940/comment/c95e2ffc_5e04310d PS7, Line 21: register "ScsSdCardWpPinEnabled" = "0"
All "register" settings with a value of 0 can be omitted.
Done
https://review.coreboot.org/c/coreboot/+/67940/comment/64da9aab_9c996ec0 PS7, Line 58: # Enable SERIRQ continous
'continous' may be misspelled - perhaps 'continuous'? […]
Done
https://review.coreboot.org/c/coreboot/+/67940/comment/3dfba3d0_f7746c1c PS7, Line 165:
nit: remove one blank line
Done
https://review.coreboot.org/c/coreboot/+/67940/comment/e16b5117_a2203313 PS7, Line 181: # Lock Down : register "common_soc_config" = "{ : .chipset_lockdown = CHIPSET_LOCKDOWN_COREBOOT, : }"
Please drop, `CHIPSET_LOCKDOWN_COREBOOT` is zero, and lockdown by FSP cannot be supported.
Done
https://review.coreboot.org/c/coreboot/+/67940/comment/1b994e80_353819d4 PS7, Line 269: device pnp 2e.4 on # Enviroment Controller
'Enviroment' may be misspelled - perhaps 'Environment'? […]
Done
https://review.coreboot.org/c/coreboot/+/67940/comment/c09135c1_b309bf22 PS7, Line 287: device pci 1f.1 on end # P2SB : device pci 1f.2 on end # Power Management Controller
AFAIK the P2SB should be disabled (it's not a "discoverable PCI device") and the PMC should be hidde […]
Many boards keep it enabled. Skylake even unhides the device on purpose to let coreboot PCI enumerator read its resources. Is there a strong reason to not have it set to ON? Maybe hidden makes sense here?
https://review.coreboot.org/c/coreboot/+/67940/comment/cb00d4c6_3dbb1e3c PS7, Line 291: off
Please enable this so that the allocator can properly allocate resources for the "fast" SPI controll […]
Done
File src/mainboard/protectli/vault_cml/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/67940/comment/d5959881_53fb3d65 PS7, Line 10: // OEM revision
Is this comment true, or is it copy-pasta?
copy-pasta, removed
File src/mainboard/protectli/vault_cml/mainboard.c:
https://review.coreboot.org/c/coreboot/+/67940/comment/3bcba4e9_fddbbf63 PS7, Line 53: |
Please add spaces around `|`
Done
https://review.coreboot.org/c/coreboot/+/67940/comment/a533ff51_b8e9395f PS7, Line 62: milioseconds
nit: *milli*seconds […]
Done
https://review.coreboot.org/c/coreboot/+/67940/comment/8ca273aa_765ec8ac PS7, Line 64: for (i = 0; i < 10000; i++) inb(0x61);
trailing statements should be on next line […]
Done
https://review.coreboot.org/c/coreboot/+/67940/comment/1ae07786_7f9ef13f PS7, Line 72: beep(1500);
It would be nice to make this optional as it slows down the boot process a bit. […]
Cool, thanks!
https://review.coreboot.org/c/coreboot/+/67940/comment/08ffe201_f0ba38c0 PS7, Line 78: * when the screen goes blank/inactive/idle in the OS */
Interesting issue. […]
I am still checking if the newer revisions of the boards still reproduce it. This code is already pretty old TBH, but the final hardware revision was delayed very much. The first revisions could have problems with the graphics or due to operating systems not properly supporting the IGD back then in i915 driver.