Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33328 )
Change subject: mainboard: Add support for ASUS P8Z77-M PRO desktop mainboard ......................................................................
Patch Set 40: Code-Review+1
(9 comments)
Looks very nice!
https://review.coreboot.org/#/c/33328/32//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33328/32//COMMIT_MSG@10 PS32, Line 10:
Filled the […]
Even MRC with 4 DIMMs is working?
https://review.coreboot.org/#/c/33328/38/Documentation/mainboard/asus/p8z77-... File Documentation/mainboard/asus/p8z77-m_pro.md:
https://review.coreboot.org/#/c/33328/38/Documentation/mainboard/asus/p8z77-... PS38, Line 110: Gb Gigabit or GigaByte?
https://review.coreboot.org/#/c/33328/38/Documentation/mainboard/asus/p8z77-... PS38, Line 111: I'd avoid spaces inside parentheses
https://review.coreboot.org/#/c/33328/23/src/mainboard/asus/p8z77-m_pro/cmos... File src/mainboard/asus/p8z77-m_pro/cmos.default:
https://review.coreboot.org/#/c/33328/23/src/mainboard/asus/p8z77-m_pro/cmos... PS23, Line 23: usb3_mode=Enable : usb3_preOS_drv=Enable : usb3_streams=Enable
I think these options should be present, because: […]
I see, then it's a good idea to keep them.
https://review.coreboot.org/#/c/33328/38/src/mainboard/asus/p8z77-m_pro/cmos... File src/mainboard/asus/p8z77-m_pro/cmos.layout:
https://review.coreboot.org/#/c/33328/38/src/mainboard/asus/p8z77-m_pro/cmos... PS38, Line 165: I'd suggest using spaces on this file, since it's what it uses
https://review.coreboot.org/#/c/33328/38/src/mainboard/asus/p8z77-m_pro/devi... File src/mainboard/asus/p8z77-m_pro/devicetree.cb:
https://review.coreboot.org/#/c/33328/38/src/mainboard/asus/p8z77-m_pro/devi... PS38, Line 18: : 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" not sure if those are needed, the board doesn't seem to use displayport
https://review.coreboot.org/#/c/33328/38/src/mainboard/asus/p8z77-m_pro/hda_... File src/mainboard/asus/p8z77-m_pro/hda_verb.c:
https://review.coreboot.org/#/c/33328/38/src/mainboard/asus/p8z77-m_pro/hda_... PS38, Line 19: /* generated by Autoport */ Please remove this
https://review.coreboot.org/#/c/33328/23/src/mainboard/asus/p8z77-m_pro/roms... File src/mainboard/asus/p8z77-m_pro/romstage.c:
https://review.coreboot.org/#/c/33328/23/src/mainboard/asus/p8z77-m_pro/roms... PS23, Line 72: /* : * Config several SuperIO ACPI things. : * See Nuvuton 6779D datasheet, section : * "21.10 Configuration Register->Logical Device A (ACPI)", : * page 340-341 : */
Tested a couple times. The 3VSBSW is **definitively** needed. […]
AFAIK, most nuvoton/winbond SuperIOs have this option that needs to be enabled.
https://review.coreboot.org/#/c/33328/38/src/mainboard/asus/p8z77-m_pro/roms... File src/mainboard/asus/p8z77-m_pro/romstage.c:
https://review.coreboot.org/#/c/33328/38/src/mainboard/asus/p8z77-m_pro/roms... PS38, Line 106: sure is "ensure it is", or "make sure it is"