[XL] Change in coreboot[master]: mb/protectli/vault_cml: Add Comet Lake 6 port board support
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 15: Code-Review+1
(12 comments)
File Documentation/mainboard/protectli/vp46xx.md:
https://review.coreboot.org/c/coreboot/+/67940/comment/b14f17f5_e82acfe5 PS7, Line 68: - Super I/O serial port 0 via front microUSB connector
Added info about the adapter in this bulletpoint
Thanks!
https://review.coreboot.org/c/coreboot/+/67940/comment/0ee4a1dd_c1943f29 PS7, Line 90: ITE IT8786E/IT8784E
Added a note on the beginning of this section.
Interesting. The HP 280 G2 mainboard has either an IT8625E or an IT8656E depending on BOM (the former for the full featured config, the latter for the legacy-free config).
File src/mainboard/protectli/vault_cml/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/67940/comment/644a6f6f_c1f46826 PS7, Line 287: device pci 1f.1 on end # P2SB : device pci 1f.2 on end # Power Management Controller
Marked them both as hidden. Is that fine for you?
If it works properly, sure. This may be related to the issues with PMC and global reset request.
File src/mainboard/protectli/vault_cml/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/67940/comment/f68e5dd3_ccb1d1f4 PS13, Line 55: register "SataMode" = "0" This should be dropped too. SATA mode should be a Kconfig option instead, anyway.
https://review.coreboot.org/c/coreboot/+/67940/comment/9e8a4b91_7876615a PS13, Line 67: register "PcieRpEnable[13]" = "1" : register "PcieRpEnable[14]" = "1" : register "PcieRpEnable[15]" = "1" As root port 13 (array index 12) is x4, these three root ports do not have any lanes associated to them, so their PCI devices should not be enabled.
TL;DR drop these settings
https://review.coreboot.org/c/coreboot/+/67940/comment/f02d59ad_85d8c3af PS13, Line 80: register "PcieRpAdvancedErrorReporting[13]" = "1" : register "PcieRpAdvancedErrorReporting[14]" = "1" : register "PcieRpAdvancedErrorReporting[15]" = "1" Same as above
https://review.coreboot.org/c/coreboot/+/67940/comment/05e59a32_3d5f7047 PS13, Line 93: register "PcieRpLtrEnable[13]" = "1" : register "PcieRpLtrEnable[14]" = "1" : register "PcieRpLtrEnable[15]" = "1" Same as above
https://review.coreboot.org/c/coreboot/+/67940/comment/8aa34552_e065ed80 PS13, Line 97: register "PcieClkSrcUsage[0]" = "PCIE_CLK_FREE" : register "PcieClkSrcUsage[1]" = "PCIE_CLK_FREE" : register "PcieClkSrcUsage[2]" = "PCIE_CLK_FREE" : register "PcieClkSrcUsage[3]" = "PCIE_CLK_FREE" : register "PcieClkSrcUsage[4]" = "PCIE_CLK_FREE" : register "PcieClkSrcUsage[5]" = "PCIE_CLK_FREE" You could map the clocks to the corresponding root ports (using the array index values), if you have schematics. Clock request should be mapped as well if you want to support ASPM L1
https://review.coreboot.org/c/coreboot/+/67940/comment/0a500957_1e73c5fc PS13, Line 202: device pci 1d.5 on end # PCI Express Port 14 : device pci 1d.6 on end # PCI Express Port 15 : device pci 1d.7 on end # PCI Express Port 16 Same as the FSP settings. These root ports have no lanes associated to them (`device pci 1d.4` is configured to use 4 lanes), so these should be off.
File src/mainboard/protectli/vault_cml/die.c:
https://review.coreboot.org/c/coreboot/+/67940/comment/c3f173ac_84d53dd8 PS13, Line 12: static uint8_t beep_count = 0;;
Statements terminations use 1 semicolon
Please fix.
File src/mainboard/protectli/vault_cml/mainboard.c:
https://review.coreboot.org/c/coreboot/+/67940/comment/03686991_4b5d1e69 PS7, Line 72: beep(1500);
Made it optional with Kconfig. […]
Nice, thank you!
FWIW, we added the "blink SATA LED on death" to the Librem Mini because it would be hard to tell whether coreboot died or FSP was still doing the thing. It proved to be very useful if RAM isn't installed properly, as coreboot will die pretty quickly.
https://review.coreboot.org/c/coreboot/+/67940/comment/42fd45c4_b35f0e51 PS7, Line 78: * when the screen goes blank/inactive/idle in the OS */
Apparently the issue is not present anymore, so I removed the workaround.
Ah, good to know.
-
Angel Pons (Code Review)