Bluemax has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32235 )
Change subject: mainboard: Add MSI MS-7707 ......................................................................
Patch Set 2:
(17 comments)
Thx so far. Will flash the image later the day. Fortunately, the board has a JSPI port and flashing can be done in no time.
https://review.coreboot.org/#/c/32235/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32235/1//COMMIT_MSG@7 PS1, Line 7: Mainboard
mainboard
Done
https://review.coreboot.org/#/c/32235/1//COMMIT_MSG@8 PS1, Line 8:
I was confused by the lid code. It is *not* a laptop.
Done
https://review.coreboot.org/#/c/32235/1//COMMIT_MSG@9 PS1, Line 9: MSI MS-7707 V1.1 (Medion OEM Akoya P4385D MSN10014555) : SandyBridge Intel P67 (BD82x6x) : Winbond 25Q32BV (4MB) : Fintek F71808A : Intel 82579V Gigabit : NEC uPD720200 USB 3.0 Host Controller : IME 7.0.4.1197
Please format this as a list.
Done
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
Done
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 5: register "gfx.use_spread_spectrum_clock" = "0" : register "gpu_cpu_backlight" = "0x00000000" : register "gpu_dp_b_hotplug" = "0" : register "gpu_dp_c_hotplug" = "0" : register "gpu_dp_d_hotplug" = "0" : 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" = "0" : register "gpu_panel_power_down_delay" = "0" : register "gpu_panel_power_up_delay" = "0" : register "gpu_pch_backlight" = "0x00000000"
these can be removed
Done
https://review.coreboot.org/#/c/32235/1/src/mainboard/medion/ms_7707/devicet... PS1, Line 29: :
Add a line here to inherit the subsystemid for all devices: […]
Done
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
Done
https://review.coreboot.org/#/c/32235/1/src/mainboard/medion/ms_7707/devicet... PS1, Line 45: subsystemid 0x1462 0x7707
If the subsystemid is inherited (see comment on line 30) this line becomes redundant
Done
https://review.coreboot.org/#/c/32235/1/src/mainboard/medion/ms_7707/devicet... PS1, Line 47: device pci 16.1 off # Management Engine Interface 2 : end
maybe put the "end" in the same line to ease reading
Done
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.
superiotool.log and datasheet say so.
https://review.coreboot.org/#/c/32235/1/src/mainboard/medion/ms_7707/devicet... PS1, Line 176: Host bridge
Redundant comment (needs fix on autoport side)
Done
https://review.coreboot.org/#/c/32235/1/src/mainboard/medion/ms_7707/devicet... PS1, Line 182: device pci 02.0 off # Internal graphics : end
Right, these are disabled because the P67 chipset has no video outputs
Done
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
Doesn't compile if removed.
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
Done
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 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
Done
https://review.coreboot.org/#/c/32235/1/src/mainboard/medion/ms_7707/romstag... PS1, Line 62: {
Oh, right. […]
Done
https://review.coreboot.org/#/c/32235/2/src/mainboard/medion/ms_7707/romstag... File src/mainboard/medion/ms_7707/romstage.c:
https://review.coreboot.org/#/c/32235/2/src/mainboard/medion/ms_7707/romstag... PS2, Line 61: /* GPIO */ Not sure if this will work. Will test later the day.