Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45317 )
Change subject: mb/asrock/h77pro4-m: add new mainboard ......................................................................
Patch Set 1:
(27 comments)
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`
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 12: # FIXME: check this This is correct
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?
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.
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
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.
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.
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 think the best approach is to switch to a dynamically-generated SSDT, but it's not done yet.
If you want to try Windows on this board, check Asus P8Z77-V LX2 or Asrock B85M Pro4 (I ported both boards and both boot to Windows 10 with TianoCore)
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
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
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
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)
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 32: Host bridge Host bridge Once is enough.
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 35: off on
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
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
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 105: # TODO verify superiotool is your friend 😊
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.c)
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 147: SATA Controller 1 SATA (AHCI)
https://review.coreboot.org/c/coreboot/+/45317/1/src/mainboard/asrock/h77pro... PS1, Line 153: SATA Controller 2 SATA (Legacy)
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.
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.
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 37: SERIAL_DEV I'd do what mb/asus/p8z77-v_lx2 does, which should be clearer. The registers you're writing to aren't specific to any LDN, so `GLOBAL_DEV` is more reasonable
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.
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. Repeat for all four slots. If all slots boot correctly, then it's correct.
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:
others => Disabled
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. If you don't want to use a VBIOS, drop the entire file and `select INTEL_INT15` from Kconfig.