Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31603 )
Change subject: mainboard: Add ASRock H110M-DVS ......................................................................
Patch Set 2:
(10 comments)
Looks interesting. My main concern is why the variant mechanism is used even when no variant is present.
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@5... PS2, Line 54: config MAINBOARD_FAMILY : string : default "ASROCK" : Is this used anywhere?
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.
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?
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/Makefile.... File src/mainboard/asrock/h110m/Makefile.inc:
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/Makefile.... PS2, Line 25: subdirs-y += variants/baseboard : CPPFLAGS_common += -I$(src)/mainboard/$(MAINBOARDDIR)/variants/baseboard/include : : subdirs-y += variants/$(VARIANT_DIR) : CPPFLAGS_common += -I$(src)/mainboard/$(MAINBOARDDIR)/variants/$(VARIANT_DIR)/include This can be removed if not using variants
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"
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/dsdt.asl File src/mainboard/asrock/h110m/dsdt.asl:
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/dsdt.asl@... PS2, Line 29: // Some generic macros This comment can be removed
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/spd/spd_u... File src/mainboard/asrock/h110m/spd/spd_util.c:
PS2: Is this needed?
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 54: register "SkipExtGfxScan" = "1" Um?
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?
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/variants/... File src/mainboard/asrock/h110m/variants/dvs/overridetree.cb:
https://review.coreboot.org/#/c/31603/2/src/mainboard/asrock/h110m/variants/... PS2, Line 131: Why are these spaces?