Michael Büchler has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45317 )
Change subject: mb/asrock: Add ASRock H77 Pro4-M mainboard ......................................................................
Patch Set 2:
(29 comments)
Your feedback was a great help for getting things in a proper way. Then I messed up my Super I/O or PCH when measuring at some Super I/O pins and things got weird. Next step might be soldering a new NCT6776D once it arrives.. and: fixing S3; making this a variant of mb/asrock/b75pro3-m
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... File src/mainboard/asrock/h77pro4-m/Kconfig:
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 3: H77PRO4_M
I'd use `H77_PRO4_M`
Yes. On the other hand the B75 Pro3-M uses B75PRO3_M and as per your suggestion I'm considering to make this board a variant of it. Then I think it should be consistent.
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 12: # FIXME: check this
This is correct
Done
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 17: select DRIVERS_ASMEDIA_ASPM_BLACKLIST # FIXME copied from B75 Pro3M
Sort this alphabetically?
Done (if I still know the ABC correctly)
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 21: default asrock/h77pro4-m
default "asrock/h77pro4-m" […]
Done
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 27: config VGA_BIOS_FILE : string : default "pci8086,0152.rom" : : config VGA_BIOS_ID : string : default "8086,0152"
Depends on the installed CPU. I'd drop it.
Done
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 35: #config DRAM_RESET_GATE_GPIO # FIXME: check this
If suspend/resume works, drop this
It does! \o/
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 39: #config USBDEBUG_HCD_INDEX # FIXME: check this
If you can test usbdebug, then adjust it. Otherwise, drop it.
Done. Can't test it.
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... File src/mainboard/asrock/h77pro4-m/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 4: bootblock-y += gpio.c
I think GPIO isn't needed in bootblock.
Build fails when I remove it. It seems to be called in src/southbridge/intel/bd82x6x/early_pch.c:early_pch_init(). Hence I'm leaving it in there.
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... File src/mainboard/asrock/h77pro4-m/acpi/superio.asl:
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 11: #include "superio/nuvoton/nct6776/acpi/superio.asl"
This ACPI code is rather broken, and at least Windows 10 will complain about it. […]
I was able to install and use Windows 10 without a change here, but later I will try what you suggested.
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... File src/mainboard/asrock/h77pro4-m/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 9: /* The lid is open by default. */ : gnvs->lids = 1;
No lid on a desktop
Done
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... File src/mainboard/asrock/h77pro4-m/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 4: #register "gfx.use_spread_spectrum_clock" = "0" # from B75 Pro3-M : #register "gfx" = "GMA_STATIC_DISPLAYS(0)" # from autoport
Drop both
Done
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 6: register "gpu_cpu_backlight" = "0x00000000" : register "gpu_dp_b_hotplug" = "4" : register "gpu_dp_c_hotplug" = "4" : register "gpu_dp_d_hotplug" = "4" : register "gpu_panel_port_select" = "0" : register "gpu_panel_power_backlight_off_delay" = "0" : register "gpu_panel_power_backlight_on_delay" = "0" : register "gpu_panel_power_cycle_delay" = "4" : register "gpu_panel_power_down_delay" = "0" : register "gpu_panel_power_up_delay" = "0" : register "gpu_pch_backlight" = "0x00000000"
None of these apply
Done
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 26: end
Please "pick up" the `end` for empty blocks (put them on the previous line)
Done
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 32: Host bridge Host bridge
Once is enough.
Done
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 35: off
on
Done
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 42: register "docking_supported" = "0"
The devicetree becomes a struct, so all fields that are set to zero can be dropped
Done. Thanks for explaining why.
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 46: register "gen4_dec" = "0x00000000" : register "pcie_hotplug_map" = "{ 0, 0, 0, 0, 0, 0, 0, 0 }" : register "pcie_port_coalesce" = "0"
The devicetree becomes a struct, so all fields that are set to zero can be dropped
Done
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 105: # TODO verify
superiotool is your friend 😊
Indeed!
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 106: # global : irq 0x1c = 0x80 : irq 0x27 = 0xc0 : irq 0x2a = 0x62
I prefer to set these in `mainboard_config_superio` (early_init. […]
Put into early_init.c:bootblock_mainboard_early_init(). Was already in there actually.
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 147: SATA Controller 1
SATA (AHCI)
Done
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 153: SATA Controller 2
SATA (Legacy)
Done
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... File src/mainboard/asrock/h77pro4-m/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 3:
One blank line is enough.
Done
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 24: #include <drivers/intel/gma/acpi/default_brightness_levels.asl>
Nope, no brightness for you. This used to be necessary, but it isn't anymore.
Done
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... File src/mainboard/asrock/h77pro4-m/early_init.c:
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 5: #include <device/pci_def.h>
maybe not needed
Indeed, thanks. Done.
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 37: SERIAL_DEV
I'd do what mb/asus/p8z77-v_lx2 does, which should be clearer. […]
Done
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 67: pci_write_config16(PCI_DEV(0, 0x1f, 0), 0x80, 0x0000);
Probably not required.
Nope, doesn't seem to be.
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 70: /* FIXME: Put proper SPD map here. */
This is probably correct. One simple way to check is to try booting with only one DIMM. […]
Thanks for explaining how to check. Boots with a single DIMM in any slot.
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... File src/mainboard/asrock/h77pro4-m/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 20: LVDS, : eDP
I don't think so. Replace with: […]
Done. I looked at `xrandr` output and saw which are needed.
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... File src/mainboard/asrock/h77pro4-m/mainboard.c:
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 10: GMA_INT15_ACTIVE_LFP_INT_LVDS
That's wrong. […]
Uhm I think it was never actually compiled or linked. Dropped the file. VBIOS works with and without it.