Attention is currently required from: Jan Philipp Groß, Máté Kukri, Nicholas Chin.
Angel Pons has posted comments on this change by Jan Philipp Groß. ( https://review.coreboot.org/c/coreboot/+/82760?usp=email )
Change subject: mb/asrock: Add Z97E-ITX/ac (Haswell/Broadwell) ......................................................................
Patch Set 2: Code-Review+1
(28 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/82760/comment/fffc03dc_160989e0?usp... : PS2, Line 9: This is a rudimentary port of this board. It was done with Haswell Autoport, wherein some adjustments for Broadwell were made (Thanks to Angel Pons!). nit: Commit message lines should be at most 72 characters long, wrap long lines to fit.
https://review.coreboot.org/c/coreboot/+/82760/comment/36a3c5e9_2f07fc5c?usp... : PS2, Line 34: - Broadwell CPUs (refer to Angel Pons Z97 Extreme 6 port for further info) You could reference the commit in question:
``` - Broadwell CPUs, see commit f5105313cf69 (mb/asrock/z97_extreme6: Add new mainboard) ```
https://review.coreboot.org/c/coreboot/+/82760/comment/6455d868_a7fc7047?usp... : PS2, Line 36: nit: One empty line is enough to separate paragraphs
https://review.coreboot.org/c/coreboot/+/82760/comment/6a8495d9_ee804673?usp... : PS2, Line 39: nit: One empty line is enough to separate paragraphs
File src/mainboard/asrock/z97e-itx_ac/Kconfig:
https://review.coreboot.org/c/coreboot/+/82760/comment/7f34137f_ec3b5d81?usp... : PS2, Line 5: select BOARD_ROMSIZE_KB_8192 Add this and check if DVI-to-VGA works
```suggestion select BOARD_ROMSIZE_KB_8192 select GFX_GMA_ANALOG_I2C_HDMI_B ```
File src/mainboard/asrock/z97e-itx_ac/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/82760/comment/5a582ac7_1349cff8?usp... : PS2, Line 1: chip northbridge/intel/haswell # FIXME: check ec_present, usb_xhci_on_resume, gfx : register "ec_present" = "false" Remove this line:
```suggestion chip northbridge/intel/haswell # FIXME: check ec_present, usb_xhci_on_resume, gfx ```
https://review.coreboot.org/c/coreboot/+/82760/comment/f7f32dfb_bfea0c96?usp... : PS2, Line 7: : register "usb_xhci_on_resume" = "false" Remove this line:
```suggestion register "gpu_dp_d_hotplug" = "4" ```
https://review.coreboot.org/c/coreboot/+/82760/comment/e7a972f0_9974957d?usp... : PS2, Line 16: device domain 0x0 on ```suggestion device domain 0 on ```
https://review.coreboot.org/c/coreboot/+/82760/comment/3050e861_4f258228?usp... : PS2, Line 19: : register "docking_supported" = "0" 1. Fix the comment 2. Remove the `docking_supported` line
```suggestion chip southbridge/intel/lynxpoint # Intel 9 Series Lynx Point PCH ```
https://review.coreboot.org/c/coreboot/+/82760/comment/66999d34_859c6694?usp... : PS2, Line 23: : register "gen4_dec" = "0x00000000" ```suggestion register "gen3_dec" = "0x000c0251" ```
https://review.coreboot.org/c/coreboot/+/82760/comment/04a0cb9e_be42c17f?usp... : PS2, Line 25: : register "gpe0_en_2" = "0x0" ```suggestion register "gpe0_en_1" = "0x40002046" ```
https://review.coreboot.org/c/coreboot/+/82760/comment/44a7d1f8_91cab1c9?usp... : PS2, Line 33: device pci 16.0 off # Management Engine Interface 1 : end I presume the ME wasn't working properly when you ran autoport? This device should be enabled (coreboot will disable it if the ME isn't working).
Also, you can shorten the comment. And if there's nothing inside a device, you can write it on a single line (use tabs so that comments align):
```suggestion device pci 16.0 on end # MEI #1 ```
https://review.coreboot.org/c/coreboot/+/82760/comment/58c18193_e5d67a91?usp... : PS2, Line 35: device pci 16.1 off # Management Engine Interface 2 : end This one should remain disabled, but you can still do the cosmetic clean-up:
```suggestion device pci 16.1 off end # MEI #2 ```
https://review.coreboot.org/c/coreboot/+/82760/comment/4b274294_d33db5c1?usp... : PS2, Line 37: device pci 16.2 off # Management Engine IDE-R : end : device pci 16.3 off # Management Engine KT : end IIRC, coreboot complains about these devices not existing. I believe they only exist on Corporate (big) ME firmware SKUs (used by B-prefix and Q-prefix PCHs), whereas Z97 uses Consumer (small) ME firmware without AMT.
TL;DR you can remove these. If unsure, send me a coreboot log *before* removing the devices.
https://review.coreboot.org/c/coreboot/+/82760/comment/b4c97a39_c2d9b8b8?usp... : PS2, Line 41: device pci 19.0 on # Intel Gigabit Ethernet Unsupported PCI device 8086:15a1 ```suggestion device pci 19.0 on # Intel Gigabit Ethernet ```
https://review.coreboot.org/c/coreboot/+/82760/comment/2066e128_a57bf308?usp... : PS2, Line 50: device pci 1c.0 on # PCIe Port #1 : subsystemid 0x1849 0x8c90 : end : device pci 1c.1 on # PCIe Port #2 : end : device pci 1c.2 on # PCIe Port #3 : end : device pci 1c.3 on # PCIe Port #4 : end : device pci 1c.4 on # PCIe Port #5 : end : device pci 1c.5 on # PCIe Port #6 : end : device pci 1c.6 on # PCIe Port #7 : end : device pci 1c.7 on # PCIe Port #8 : end I find it really odd that only the first root port has a subsystem ID. The two hex numbers are "subsystem vendor ID" and "subsystem device ID", respectively.
Looks like Asrock programs subsystem IDs so that subsystem vendor ID is "Asrock" and subsystem device ID is the same as the PCI device ID of the device. For the root ports, the IDs are sequential, incrementing by 2 each time. So:
```suggestion device pci 1c.0 on # PCIe Port #1 subsystemid 0x1849 0x8c90 end device pci 1c.1 on # PCIe Port #2 subsystemid 0x1849 0x8c92 end device pci 1c.2 on # PCIe Port #3 subsystemid 0x1849 0x8c94 end device pci 1c.3 on # PCIe Port #4 subsystemid 0x1849 0x8c96 end device pci 1c.4 on # PCIe Port #5 subsystemid 0x1849 0x8c98 end device pci 1c.5 on # PCIe Port #6 subsystemid 0x1849 0x8c9a end device pci 1c.6 on # PCIe Port #7 subsystemid 0x1849 0x8c9c end device pci 1c.7 on # PCIe Port #8 subsystemid 0x1849 0x8c9e end ```
But I know at least one of these root ports has to be off because `device pci 19.0` is enabled, which takes away one of the chipset's eight lanes. More investigation needed
Also, if you have the time, figuring out which root ports correspond to what connectors would be nice. `lspci -nntv` is quite useful for this. Of course, you need to plug devices in.
https://review.coreboot.org/c/coreboot/+/82760/comment/954f3118_b12dd373?usp... : PS2, Line 86: device pci 01.0 on # PCIe Bridge for discrete graphics Unsupported PCI device 8086:0c01 ```suggestion device pci 01.0 on # PEG ```
https://review.coreboot.org/c/coreboot/+/82760/comment/c79e1455_0ffd8bf5?usp... : PS2, Line 89: device pci 02.0 on # Internal graphics VGA controller ```suggestion device pci 02.0 on # iGPU ```
https://review.coreboot.org/c/coreboot/+/82760/comment/eff3ab69_9e17a5ba?usp... : PS2, Line 82: : device pci 00.0 on # Desktop Host bridge : subsystemid 0x1849 0x0c00 : end : device pci 01.0 on # PCIe Bridge for discrete graphics Unsupported PCI device 8086:0c01 : subsystemid 0x1849 0x0c01 : end : device pci 02.0 on # Internal graphics VGA controller : subsystemid 0x1849 0x0412 : end : device pci 03.0 on # Mini-HD audio : subsystemid 0x1849 0x0c0c : end Move these devices above `chip southbridge/intel/lynxpoint` They're *north*bridge devices, so they should be "north of" (above) *south*bridge devices 😄
File src/mainboard/asrock/z97e-itx_ac/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/82760/comment/6010fa6a_62d53ae1?usp... : PS2, Line 3: nit: Remove one blank line
https://review.coreboot.org/c/coreboot/+/82760/comment/5d3b995c_a5a661bd?usp... : PS2, Line 12: 0x20141018 /* OEM revision */ Please get rid of this autoport nonsense:
```suggestion 0x20141018 ```
https://review.coreboot.org/c/coreboot/+/82760/comment/bfd35b3c_ce8e8081?usp... : PS2, Line 24: : #include <northbridge/intel/haswell/acpi/hostbridge.asl> : /* FIXME: remove this if the board doesn't have backlight. */ : #include <drivers/intel/gma/acpi/default_brightness_levels.asl> Fix the FIXME:
```suggestion { #include <northbridge/intel/haswell/acpi/hostbridge.asl> ```
File src/mainboard/asrock/z97e-itx_ac/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/82760/comment/b8f52803_7c5843c5?usp... : PS2, Line 11: -- FIXME: check this : ports : constant Port_List := : (DP1, : DP2, : DP3, : HDMI1, : HDMI2, : HDMI3, : Analog, : LVDS, : eDP); Try this:
```suggestion ports : constant Port_List := (DP2, -- DP HDMI1, -- DVI-I HDMI2, -- DP HDMI3, -- HDMI Analog, -- DVI-I others => Disabled); ```
It's the same port mapping as Z97 Extreme6, we know Asrock used the same port mapping for another board already.
File src/mainboard/asrock/z97e-itx_ac/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/82760/comment/4086b4d0_9ee8d480?usp... : PS2, Line 20: nit: Drop blank line
File src/mainboard/asrock/z97e-itx_ac/romstage.c:
https://review.coreboot.org/c/coreboot/+/82760/comment/0e616ae8_02f2f6bb?usp... : PS2, Line 3: #include <stdint.h> Elyes said in another change that this doesn't seem to be used, so remove the line.
https://review.coreboot.org/c/coreboot/+/82760/comment/f5cb685f_5a139b3c?usp... : PS2, Line 10: : : /* FIXME: called after romstage_common, remove it if not used */ : void mb_late_romstage_setup(void) : { : } : We won't need this:
```suggestion }
```
https://review.coreboot.org/c/coreboot/+/82760/comment/48064939_dd228e02?usp... : PS2, Line 18: : /* FIXME: check this */ : spdi->addresses[0] = 0x50; : spdi->addresses[1] = 0x51; : spdi->addresses[2] = 0x52; : spdi->addresses[3] = 0x53; Since there's two slots, the mapping should probably be this:
```suggestion { spdi->addresses[0] = 0x50; spdi->addresses[2] = 0x52; ```
You can check using something like `decode-dimms`
https://review.coreboot.org/c/coreboot/+/82760/comment/12e27f67_0066c27a?usp... : PS2, Line 27: /* FIXME: Length and Location are computed from IOBP values, may be inaccurate */ If USB ports work, I would remove the FIXME. The values seem reasonable.