Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32235 )
Change subject: Mainboard: Add MSI MS-7707 ......................................................................
Patch Set 1:
(8 comments)
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
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:
subsystemid 0x1462 0x7707 inherit
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
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
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)
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
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.
At some point you would want to use proper macros here (LPC_EN, GENx_DEC...)
https://review.coreboot.org/#/c/32235/1/src/mainboard/medion/ms_7707/romstag... PS1, Line 62: {
configure superio for serial output here
there is no serial port on the board, from what I can tell from pictures. You would want to write the SuperIO registers that do not get set on devicetree here, though.
How? superio-specific code should have a register write function somewhere, or it may be missing. Check src/superio/fintek/f71808a