Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32235 )
Change subject: Mainboard: Add MSI MS-7707 ......................................................................
Patch Set 1:
(9 comments)
https://review.coreboot.org/#/c/32235/1/src/mainboard/medion/ms_7707/acpi_ta... File src/mainboard/medion/ms_7707/acpi_tables.c:
https://review.coreboot.org/#/c/32235/1/src/mainboard/medion/ms_7707/acpi_ta... PS1, Line 31: gnvs->lids = 1; there's no lid
https://review.coreboot.org/#/c/32235/1/src/mainboard/medion/ms_7707/devicet... File src/mainboard/medion/ms_7707/devicetree.cb:
https://review.coreboot.org/#/c/32235/1/src/mainboard/medion/ms_7707/devicet... PS1, Line 38: register "pcie_hotplug_map" = "{ 0, 0, 0, 0, 0, 0, 0, 0 }" can be removed as the default is all zero
https://review.coreboot.org/#/c/32235/1/src/mainboard/medion/ms_7707/devicet... PS1, Line 41: register "sata_port_map" = "0xc" two sata ports are missing
https://review.coreboot.org/#/c/32235/1/src/mainboard/medion/ms_7707/devicet... PS1, Line 109: io 0x60 = 0x295 are you sure? usually they are aligned to 2.
https://review.coreboot.org/#/c/32235/1/src/mainboard/medion/ms_7707/dsdt.as... File src/mainboard/medion/ms_7707/dsdt.asl:
https://review.coreboot.org/#/c/32235/1/src/mainboard/medion/ms_7707/dsdt.as... PS1, Line 38: #include <drivers/intel/gma/acpi/default_brightness_levels.asl> no need for brightness as there's no GMA
https://review.coreboot.org/#/c/32235/1/src/mainboard/medion/ms_7707/mainboa... File src/mainboard/medion/ms_7707/mainboard.c:
https://review.coreboot.org/#/c/32235/1/src/mainboard/medion/ms_7707/mainboa... PS1, Line 23: /* install_intel_vga_int15_handler(GMA_INT15_ACTIVE_LFP_INT_LVDS, no dead code please
https://review.coreboot.org/#/c/32235/1/src/mainboard/medion/ms_7707/romstag... File src/mainboard/medion/ms_7707/romstage.c:
https://review.coreboot.org/#/c/32235/1/src/mainboard/medion/ms_7707/romstag... PS1, Line 29: pci_write_config32(PCI_DEV(0, 0x1f, 0), 0x84, 0x00fc0295); do you need to access that IO range in romstage? if not it can be moved to devicetree.
https://review.coreboot.org/#/c/32235/1/src/mainboard/medion/ms_7707/romstag... PS1, Line 30: pci_write_config32(PCI_DEV(0, 0x1f, 0), 0x88, 0x00000000); Ranges set to zero can be removed, as it's the power on reset
https://review.coreboot.org/#/c/32235/1/src/mainboard/medion/ms_7707/romstag... PS1, Line 62: { configure superio for serial output here