[L] Change in coreboot[main]: mb/asrock: Add Fatal1ty Z87 Professional (Haswell)

[Angel Pons (Code Review)]

Attention is currently required from: Felix Singer, Jan Philipp Groß, Máté Kukri, Nicholas Chin, Paul Menzel.

Angel Pons has posted comments on this change by Jan Philipp Groß. ( https://review.coreboot.org/c/coreboot/+/82913?usp=email )

Change subject: mb/asrock: Add Fatal1ty Z87 Professional (Haswell)
......................................................................

Patch Set 4:

(34 comments)

Patchset:

PS4: 
Habemus review comments

Commit Message:

https://review.coreboot.org/c/coreboot/+/82913/comment/e603bd0f_4cbd6119?usp... :
PS4, Line 10: ASMP 
`ASM1061 ASPM`

ASM1061 is the model of the ASMedia SATA controllers with misbehaving ASPM (Active State Power Management)

https://review.coreboot.org/c/coreboot/+/82913/comment/8f657375_f5eb28f5?usp... :
PS4, Line 13: jumper 
Huh, interesting. The Z97 Extreme6 has an actual switch.

https://review.coreboot.org/c/coreboot/+/82913/comment/e0c86e21_1c2da0a7?usp... :
PS4, Line 21: - both RJ-45 Gigabit LAN Ports
For the 2nd one, does the MAC address stay the same between vendor firmware and coreboot?

https://review.coreboot.org/c/coreboot/+/82913/comment/b60cd1f1_00b6d72e?usp... :
PS4, Line 39: - COM Port header
You haven't tested the best kind of coreboot console there is?

https://review.coreboot.org/c/coreboot/+/82913/comment/d1abf6bc_89693d21?usp... :
PS4, Line 44: - HDMI-In Port
This port should only work while the board is off. I think there may be a way to control it via software, but I haven't looked.

https://review.coreboot.org/c/coreboot/+/82913/comment/37b27f8d_a614c45e?usp... :
PS4, Line 52: - Dr. Debug: on vendor firmware, the LEDs turn off after successful
This is because missing Super I/O config. If done too early, you'll switch off the POST code display before coreboot is done. I think I chose to keep it on for the Z97 Extreme6.

https://review.coreboot.org/c/coreboot/+/82913/comment/70ce2d76_e0018864?usp... :
PS4, Line 53: boot. On coreboot, the LED shows two bright zeros after boot.
It seems that Linux writes zeroes to I/O port 0x80 (what is shown in the POST code display). On Windows, the display shows whatever value the firmware last wrote to I/O 0x80.

File src/mainboard/asrock/fatal1ty_z87_professional/Kconfig:

https://review.coreboot.org/c/coreboot/+/82913/comment/f709967a_b78ac6a5?usp... :
PS4, Line 18: 	string
Drop `string`

https://review.coreboot.org/c/coreboot/+/82913/comment/ac88b6fe_01c30b47?usp... :
PS4, Line 22: 	string
Drop `string`

https://review.coreboot.org/c/coreboot/+/82913/comment/d2331bb7_02456bd9?usp... :
PS4, Line 25: config VGA_BIOS_ID
            : 	string
            : 	default "8086,0412"
Drop the whole thing

File src/mainboard/asrock/fatal1ty_z87_professional/devicetree.cb:

https://review.coreboot.org/c/coreboot/+/82913/comment/b33f0a53_a5231d08?usp... :
PS4, Line 1: eck ec_present, usb_xhci_on_resume, gfx
           : 	register "gfx" = "GMA_STATIC_DISPLAYS(0)"
           : 	register "gpu_ddi_e_connected" = "1"
           : 	register "gpu_dp_b_hotplug" = "0"
           : 	register "gpu_dp_c_hotplug" = "0"
           : 	register "gpu_dp_d_hotplug" = "0"
           : 
           : 	register "usb_xhci_on_resume" = "false"
```suggestion
chip northbridge/intel/haswell
```

https://review.coreboot.org/c/coreboot/+/82913/comment/4ae9fd4f_687fa4a4?usp... :
PS4, Line 17: Series 8
```suggestion
    	chip southbridge/intel/lynxpoint # Intel 8 Series Lynx Point PCH
```

https://review.coreboot.org/c/coreboot/+/82913/comment/46886ef2_a433d525?usp... :
PS4, Line 20: 
            : 			register "gpe0_en_1" = "0x0"
            : 			register "gpe0_en_2" = "0x0"
            : 			register "sata_port0_gen3_dtle" = "0x0"
            : 			register "sata_port1_gen3_dtle" = "0x0"
Drop these 4 lines, values are 0

https://review.coreboot.org/c/coreboot/+/82913/comment/c869a239_1ce17121?usp... :
PS4, Line 29: 
            : 				subsystemid 0x1849 0x8c3a
            : 			end
            : 			device pci 16.1 off # Management Engine Interface 2
            : 			end
            : 			device pci 16.2 off # Management Engine IDE-R
            : 			end
            : 			device pci 16.3 off # Management Engine KT
            : 			end
```suggestion
    		device pci 16.0 on	# MEI #1
    			subsystemid 0x1849 0x8c3a
    		end
    		device pci 16.1 off end	# MEI #2
```

https://review.coreboot.org/c/coreboot/+/82913/comment/558a1089_8cc7cf19?usp... :
PS4, Line 38: 			device pci 19.0 on # Intel Gigabit Ethernet Unsupported PCI device 8086:153b
```suggestion
    		device pci 19.0 on # Intel Gigabit Ethernet
```

https://review.coreboot.org/c/coreboot/+/82913/comment/af8a244e_52be747f?usp... :
PS4, Line 47: device pci 1c.0 on # PCIe Port #1 mPCIe WiFi
            : 				subsystemid 0x1849 0x8c10
            : 			end
```suggestion
    		device pci 1c.0 off end	# RP #1
```

https://review.coreboot.org/c/coreboot/+/82913/comment/a532baed_9e3d7dc7?usp... :
PS4, Line 50: 			device pci 1c.1 on # PCIe Port #2 ASM1061 SATA Controller
```suggestion
    		device pci 1c.1 on	# RP #2: mPCIe slot
```

https://review.coreboot.org/c/coreboot/+/82913/comment/24c38f25_6eb0e44f?usp... :
PS4, Line 53: 			device pci 1c.2 on # PCIe Port #3 Intel I211 Gigabit Network
```suggestion
    		device pci 1c.2 on	# RP #3: ASM1061 SATA controller
```

https://review.coreboot.org/c/coreboot/+/82913/comment/fd334ecf_3e8b61ce?usp... :
PS4, Line 56: 
            : 				subsystemid 0x1849 0x8c16
            : 			end
```suggestion
    		device pci 1c.3 on	# RP #4: Intel I211 GbE
    			subsystemid 0x1849 0x8c16
    			device pci 00.0 on end
    		end
```

https://review.coreboot.org/c/coreboot/+/82913/comment/ecd57394_8c64418d?usp... :
PS4, Line 59: 			device pci 1c.4 on # PCIe Port #5
```suggestion
    		device pci 1c.4 on	# RP #5: ASM1061 SATA controller
```

https://review.coreboot.org/c/coreboot/+/82913/comment/d7698f63_abddd6f9?usp... :
PS4, Line 62: 
            : 			end
```suggestion
    		device pci 1c.5 on	# RP #6: PCIe x1 slot
    			subsystemid 0x1849 0x8c1a
    		end
```

https://review.coreboot.org/c/coreboot/+/82913/comment/2e551c34_6d8141c1?usp... :
PS4, Line 64: 
            : 			end
```suggestion
    		device pci 1c.6 on	# RP #7: ASM1083 PCIe-to-PCI bridge
    			subsystemid 0x1849 0x8c1c
    		end
```

https://review.coreboot.org/c/coreboot/+/82913/comment/9ad6385c_64b48c98?usp... :
PS4, Line 66: device pci 1c.7 on # PCIe Port #8
            : 			end
```suggestion
    		device pci 1c.7 off end	# RP #8
```

https://review.coreboot.org/c/coreboot/+/82913/comment/832a1e43_3bc6646d?usp... :
PS4, Line 126: 
             : 			end
             : 			device pci 1f.3 on # SMBus
             : 				subsystemid 0x1849 0x8c22
             : 			end
             : 			device pci 1f.5 off # SATA Controller (Legacy)
             : 			end
             : 			device pci 1f.6 off # Thermal
             : 			end
```suggestion
    		device pci 1f.2 on  end	# SATA Controller (AHCI)
    		device pci 1f.3 on	# SMBus
    			subsystemid 0x1849 0x8c22
    		end
    		device pci 1f.5 off end	# SATA Controller (Legacy)
    		device pci 1f.6 off end	# Thermal
```

https://review.coreboot.org/c/coreboot/+/82913/comment/03b73fe4_49087260?usp... :
PS4, Line 139: PCIe Bridge for discrete graphics Unsupported PCI device 8086:0c01
```suggestion
    	device pci 01.0 on # PEG
```

https://review.coreboot.org/c/coreboot/+/82913/comment/539f08b9_2de343c1?usp... :
PS4, Line 142: 		device pci 02.0 on # Internal graphics VGA controller
```suggestion
    	device pci 02.0 on # iGPU
```

https://review.coreboot.org/c/coreboot/+/82913/comment/af2ec25c_4eb7b59f?usp... :
PS4, Line 136: 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 above the southbridge

File src/mainboard/asrock/fatal1ty_z87_professional/dsdt.asl:

https://review.coreboot.org/c/coreboot/+/82913/comment/faaeb1f3_4b081a24?usp... :
PS4, Line 25: 
            : 	/* FIXME: remove this if the board doesn't have backlight. */
            : 		#include <drivers/intel/gma/acpi/default_brightness_levels.asl>
You know what to do 😄

File src/mainboard/asrock/fatal1ty_z87_professional/gma-mainboard.ads:

https://review.coreboot.org/c/coreboot/+/82913/comment/d1197a05_17649898?usp... :
PS4, Line 11:    -- FIXME: check this
One DP output, one HDMI output. coreboot logs with libgfxinit debug enabled will show the correct ports. For DP, keep the corresponding HDMI port enabled. In the end, there should be 3 ports left.

File src/mainboard/asrock/fatal1ty_z87_professional/romstage.c:

https://review.coreboot.org/c/coreboot/+/82913/comment/3e07bfa0_b28eeb36?usp... :
PS4, Line 3: #include <stdint.h>
Remove this

https://review.coreboot.org/c/coreboot/+/82913/comment/a0eeb243_8caa903c?usp... :
PS4, Line 12: /* FIXME: called after romstage_common, remove it if not used */
            : void mb_late_romstage_setup(void)
            : {
            : }
Remove this

https://review.coreboot.org/c/coreboot/+/82913/comment/ec8feabc_cfd3d829?usp... :
PS4, Line 19: 	/* FIXME: check this */
If all DIMM slots work, remove this

https://review.coreboot.org/c/coreboot/+/82913/comment/886027f6_da957351?usp... :
PS4, Line 27: 	/* FIXME: Length and Location are computed from IOBP values, may be inaccurate */
If all USB ports work, remove this