Attention is currently required from: Michał Żygowski, Piotr Król.
Angel Pons 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 7: Code-Review+1
(22 comments)
File Documentation/mainboard/protectli/vp46xx.md:
https://review.coreboot.org/c/coreboot/+/67940/comment/b800d3bf_92a5c8c4 PS7, Line 68: - Super I/O serial port 0 via front microUSB connector Does the board have an onboard USB-to-UART adapter? Wow.
https://review.coreboot.org/c/coreboot/+/67940/comment/12e68ec1_d9c7e01a 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?
File src/mainboard/protectli/vault_cml/Kconfig:
https://review.coreboot.org/c/coreboot/+/67940/comment/e3fa202f_54ded488 PS7, Line 13: select SOC_INTEL_COMMON_BLOCK_HDA Already selected by SoC Kconfig
https://review.coreboot.org/c/coreboot/+/67940/comment/f5cc87f1_a25580f7 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
https://review.coreboot.org/c/coreboot/+/67940/comment/7f86bf99_7b37099b PS7, Line 35: int Please drop `int` type as per CB:57258
https://review.coreboot.org/c/coreboot/+/67940/comment/02a27695_bfd40780 PS7, Line 38: config MAX_CPUS : int : default 12 Should already be the default for CML-U
https://review.coreboot.org/c/coreboot/+/67940/comment/7fefb8d5_caaafac4 PS7, Line 42: config DEVICETREE : string : default "devicetree.cb" Leftover from variants? Please drop, it's redundant.
File src/mainboard/protectli/vault_cml/cmos.layout:
https://review.coreboot.org/c/coreboot/+/67940/comment/d0e1d591_269cc386 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.
File src/mainboard/protectli/vault_cml/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/67940/comment/f9d1c417_7ad59070 PS7, Line 21: register "ScsSdCardWpPinEnabled" = "0" All "register" settings with a value of 0 can be omitted.
https://review.coreboot.org/c/coreboot/+/67940/comment/228cb2b3_3c931fd9 PS7, Line 58: # Enable SERIRQ continous
'continous' may be misspelled - perhaps 'continuous'?
Please fix.
https://review.coreboot.org/c/coreboot/+/67940/comment/b45059eb_d74b8913 PS7, Line 165: nit: remove one blank line
https://review.coreboot.org/c/coreboot/+/67940/comment/ef14de98_8f1afe46 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.
https://review.coreboot.org/c/coreboot/+/67940/comment/6bcbe799_88e76388 PS7, Line 228: "M.2/E 2230 (M2_WIFI2)" "SlotDataBusWidth1X" nit: add an extra tab for visual alignment, `smbios_slot_desc ` is exactly 16 characters wide.
https://review.coreboot.org/c/coreboot/+/67940/comment/63852962_cf9a15a1 PS7, Line 269: device pnp 2e.4 on # Enviroment Controller
'Enviroment' may be misspelled - perhaps 'Environment'?
Please fix. While at it, use tabs so that the LDN comments are aligned?
https://review.coreboot.org/c/coreboot/+/67940/comment/3574c025_ffddf9c6 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 hidden.
https://review.coreboot.org/c/coreboot/+/67940/comment/10bc0292_490ef2bc PS7, Line 291: off Please enable this so that the allocator can properly allocate resources for the "fast" SPI controller.
File src/mainboard/protectli/vault_cml/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/67940/comment/65e8347a_6e9a63d3 PS7, Line 10: // OEM revision Is this comment true, or is it copy-pasta?
File src/mainboard/protectli/vault_cml/mainboard.c:
https://review.coreboot.org/c/coreboot/+/67940/comment/774dabd1_09c67c6b PS7, Line 53: | Please add spaces around `|`
https://review.coreboot.org/c/coreboot/+/67940/comment/684fd96d_717b3642 PS7, Line 62: milioseconds nit: *milli*seconds
Also, why not use a delay function?
https://review.coreboot.org/c/coreboot/+/67940/comment/3fb1b68f_1ee71df7 PS7, Line 64: for (i = 0; i < 10000; i++) inb(0x61);
trailing statements should be on next line
Please fix.
https://review.coreboot.org/c/coreboot/+/67940/comment/4bb10fdc_67575d64 PS7, Line 72: beep(1500); It would be nice to make this optional as it slows down the boot process a bit. It's just a matter of adding a Kconfig option.
While at it, it would be interesting to use this to beep on death, by implementing `die_notify()`. Feel free to use CB:50013 as inspiration.
https://review.coreboot.org/c/coreboot/+/67940/comment/d7f8ffca_56836ae9 PS7, Line 78: * when the screen goes blank/inactive/idle in the OS */ Interesting issue. Do you have plans to address this now, or can this board port be submitted with this workaround?