Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40813 )
Change subject: mb/ongy/h61m-s1: Add new mainboard ported with autoport script ......................................................................
Patch Set 14:
(19 comments)
Hi Robin, welcome to coreboot!
I left a bunch of comments around the files. Feel free to ask if you have any doubts 😄
Patch Set 10:
(11 comments)
Patch Set 9:
Patch Set 6:
(11 comments)
Many vendors are just reselling mainboards from other companies. Is this yours? https://www.gigabyte.com/de/Motherboard/GA-H61M-S1-rev-21#ov
If yes, then the vendor is Gigabyte.
No, it is not GA-H61M-S1 but onlu H61m-S1. It's seems to be a chinese version of this board
only*
Felix, the board is completely different. Here's a picture I found:
https://i0.wp.com/ae01.alicdn.com/kf/H2ef1edff41744124861983142fb86314f/H61M...
https://review.coreboot.org/c/coreboot/+/40813/14//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40813/14//COMMIT_MSG@7 PS14, Line 7: mb/ongy/h61m-s1: Add new mainboard Add an empty line between the commit summary and the commit message:
mb/ongy/h61m-s1: Add new mainboard
Port done using the autoport utility.
https://review.coreboot.org/c/coreboot/+/40813/14//COMMIT_MSG@15 PS14, Line 15: Integrated VGA and HDMI works Was this with libgfxinit? (the default graphics init for most Intel iGPUs)
https://review.coreboot.org/c/coreboot/+/40813/14//COMMIT_MSG@17 PS14, Line 17: Seabios SeaBIOS
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... File src/mainboard/ongy/h61m-s1/Kconfig:
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 26: config VGA_BIOS_FILE : string : default "pci8086,0112.rom" : : config VGA_BIOS_ID : string : default "8086,0112" Remove this, it depends on the installed CPU
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 34: onfig DRAM_RESET_GATE_GPIO : int : default 60 This is not correct. GPIO60 is configured as native mode on your board. If ACPI S3 suspend/resume work correctly, you can remove it. Otherwise, we will have to guess the correct GPIO.
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... File src/mainboard/ongy/h61m-s1/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 8: /* Disable USB ports in S3 by default */ : gnvs->s3u0 = 0; : gnvs->s3u1 = 0; : /* Disable USB ports in S5 by default */ : gnvs->s5u0 = 0; : gnvs->s5u1 = 0; This does nothing. The gnvs struct is cleared before calling this function
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... File src/mainboard/ongy/h61m-s1/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 5: register "gfx" = "GMA_STATIC_DISPLAYS(0)" : register "gpu_dp_b_hotplug" = "4" : register "gpu_dp_c_hotplug" = "4" : register "gpu_dp_d_hotplug" = "4" : register "gpu_panel_power_cycle_delay" = "4" These settings aren't needed for a desktop board
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 10: 0x0 0
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 18: 0x0 0
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 22: 0x0 0
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 25: register "docking_supported" = "0" This defaults to zero already. You can remove it
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 27: register "pcie_hotplug_map" = "{ 0, 0, 0, 0, 0, 0, 0, 0 }" This defaults to zero already. You can remove it
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 64: : device pci 00.0 on # Host bridge Host bridge : subsystemid 0x8086 0x0100 : end : device pci 01.0 on # PEG : subsystemid 0x8086 0x0101 : end : device pci 02.0 on # iGPU : subsystemid 0x8086 0x2010 : end Please move these entries above the southbridge block. That is, before this line:
chip southbridge/intel/bd82x6x # Intel Series 6 Cougar Point PCH
https://review.coreboot.org/c/coreboot/+/40813/10/src/mainboard/ongy/h61m-s1... File src/mainboard/ongy/h61m-s1/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/40813/10/src/mainboard/ongy/h61m-s1... PS10, Line 4: #define ACPI_VIDEO_DEVICE _SB.PCI0.GFX0
yes, it seems to be used for integrated GPU
It's only used on Lenovo mainboards. Please remove
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... File src/mainboard/ongy/h61m-s1/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 4: #define ACPI_VIDEO_DEVICE _SB.PCI0.GFX0 Unused, can remove
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 22: g Capitalize: "Global"
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 29: #include <drivers/intel/gma/acpi/default_brightness_levels.asl> Not needed for a desktop board without an integrated panel
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... File src/mainboard/ongy/h61m-s1/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... PS14, Line 10: private package GMA.Mainboard is Almost all the gma_mainboard.ads files in coreboot are indented with three spaces, instead of tabs. Please do the same here
https://review.coreboot.org/c/coreboot/+/40813/14/src/mainboard/ongy/h61m-s1... File src/mainboard/ongy/h61m-s1/mainboard.c:
PS14: If you don't plan on using the VGA BIOS, I would remove this and the `select INTEL_INT15` line from Kconfig