Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31603 )
Change subject: mainboard: Add ASRock H110M-DVS ......................................................................
Patch Set 4:
(21 comments)
Patch Set 2:
(10 comments)
Looks interesting. My main concern is why the variant mechanism is used even when no variant is present.
I reworked the patch to remove the "variants" directory. I`ll put it back in one of the following patches to add another motherboard from the "H110M" series.
https://review.coreboot.org/#/c/31603/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31603/2//COMMIT_MSG@9 PS2, Line 9: Kabylake
Kaby Lake
Done
https://review.coreboot.org/#/c/31603/2//COMMIT_MSG@11 PS2, Line 11: OS
Maybe add *(Ubuntu 16.04.2 with Linux 4.15)* already here.
Done
https://review.coreboot.org/#/c/31603/2//COMMIT_MSG@16 PS2, Line 16: Work
Works
Done
https://review.coreboot.org/#/c/31603/2//COMMIT_MSG@17 PS2, Line 17: - Integrated graphics (only DVI port);
What resolution did you use?
1920x1080 This is the maximum resolution of my display.
https://review.coreboot.org/#/c/31603/2//COMMIT_MSG@28 PS2, Line 28: Tianocore
TianoCore (what version)?
Done
https://review.coreboot.org/#/c/31603/2//COMMIT_MSG@28 PS2, Line 28: Seabios
SeaBIOS (what version)
Done
https://review.coreboot.org/#/c/31603/2//COMMIT_MSG@30 PS2, Line 30:
What about suspend and resume?
Sorry, I forgot to mention that this isn`t working. Board uses _PTS and _WAK ACPI-methods from https://github.com/coreboot/coreboot/blob/master/src/soc/intel/skylake/acpi/... I added it to dsdt.asl. When I do suspend in the OS, the system falls to sleep state, but after that it does not wake up.
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/Kconfig File src/mainboard/asrock/h110m/Kconfig:
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/Kconfig@1... PS2, Line 16: TSC_MONOTONIC_TIMER
already selected in the skylake soc Kconfig
Done
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/Kconfig@2... PS2, Line 20: HAVE_SMI_HANDLER
already selected in the skylake soc Kconfig
Done
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/Kconfig@2... PS2, Line 24: choice : prompt "Processor family" : default USE_SOC_INTEL_SKYLAKE : : config USE_SOC_INTEL_SKYLAKE : bool "Skylake" : select SOC_INTEL_SKYLAKE : : config USE_SOC_INTEL_KABYLAKE : bool "Kabylake" : select SOC_INTEL_KABYLAKE : : endchoice
is making this distinction really needed? you end up using the kabylake FSP anyway iirc.
Done
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/Kconfig@5... PS2, Line 54: config MAINBOARD_FAMILY : string : default "ASROCK" :
Is this used anywhere?
Done
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/Kconfig@6... PS2, Line 62: config DEVICETREE : string : default "variants/baseboard/devicetree.cb" : : config OVERRIDE_DEVICETREE : string : default "variants/$(CONFIG_VARIANT_DIR)/overridetree.cb"
This isn't needed if no variants are used.
I reworked the patch to remove the "variants" directory. I`ll put it back in one of the following patches to add another motherboard from the "H110M" series.
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/Kconfig@7... PS2, Line 73: BOARD_ASROCK_H110M_DVS
not needed.
Done
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/Kconfig@7... PS2, Line 73: "3rdparty/blobs/mainboard/$(CONFIG_MAINBOARD_DIR)/descriptor.dvs.bin"
Does this exist?
I removed it
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/Kconfig@7... PS2, Line 78: BOARD_ASROCK_H110M_DVS
not needed.
Done
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/cmos.defa... File src/mainboard/asrock/h110m/cmos.default:
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/cmos.defa... PS2, Line 2: Spew
Please default to "Debug"
Done
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/variants/... File src/mainboard/asrock/h110m/variants/baseboard/devicetree.cb:
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/variants/... PS2, Line 38: # EC host command ranges are in 0x800-0x8ff & 0x200-0x20f : register "gen1_dec" = "0x00fc0801
I don't see a device decoding this range.
I removed it
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/variants/... PS2, Line 54: register "SkipExtGfxScan" = "1"
Um?
Yes, I think it will be true if "SkipExtGfxScan" is set to 0 so that the system would scan external graphics. However, the external GPU works without it.
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/variants/... PS2, Line 156: register "pirqa_routing" = "0x0b" : register "pirqb_routing" = "0x0a" : register "pirqc_routing" = "0x0b" : register "pirqd_routing" = "0x0b" : register "pirqe_routing" = "0x0b" : register "pirqf_routing" = "0x0b" : register "pirqg_routing" = "0x0b" : register "pirqh_routing" = "0x0b"
already done at line 60.
I removed it
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/variants/... PS2, Line 224: device pci 1c.0 off end # PCI Express Port 1 : device pci 1c.1 off end # PCI Express Port 2 : device pci 1c.2 off end # PCI Express Port 3 : device pci 1c.3 off end # PCI Express Port 4 : device pci 1c.4 off end # PCI Express Port 5 : device pci 1c.5 off end # PCI Express Port 6 : device pci 1c.6 off end # PCI Express Port 7 : device pci 1c.7 off end # PCI Express Port 8 : device pci 1d.0 off end # PCI Express Port 9 : device pci 1d.1 off end # PCI Express Port 10 : device pci 1d.2 off end # PCI Express Port 11 : device pci 1d.3 off end # PCI Express Port 12
How are PCIe ports working?
I used to enable ports in overridetree.cb, but then I decided to move it to devicetree until new version of this board is added to project.
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/variants/... PS2, Line 246: off
Set to on to enable onboard audio. […]
I enabled hda need to add "EnableAzalia" = "1"