Arthur Heymans 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 22:
(12 comments)
https://review.coreboot.org/#/c/33328/14/Documentation/mainboard/asus/p8z77-... File Documentation/mainboard/asus/p8z77-m_pro.md:
https://review.coreboot.org/#/c/33328/14/Documentation/mainboard/asus/p8z77-... PS14, Line 47: Using MRC.bin, : seems the maximum allocable real size is 32Mb mrc.bin indeed reconfigures this regardless.
https://review.coreboot.org/#/c/33328/22/src/mainboard/asus/p8z77-m_pro/cmos... File src/mainboard/asus/p8z77-m_pro/cmos.layout:
https://review.coreboot.org/#/c/33328/22/src/mainboard/asus/p8z77-m_pro/cmos... PS22, Line 118: doen down
https://review.coreboot.org/#/c/33328/22/src/mainboard/asus/p8z77-m_pro/cmos... PS22, Line 162: 7 16 1024M Every setting larger than 512M is reserved according to the datasheet, so please remove. It also just seems wrong with it linearly increasing with 32M per bit.
https://review.coreboot.org/#/c/33328/22/src/mainboard/asus/p8z77-m_pro/devi... File src/mainboard/asus/p8z77-m_pro/devicetree.cb:
https://review.coreboot.org/#/c/33328/22/src/mainboard/asus/p8z77-m_pro/devi... PS22, Line 96: device pnp 2e.2 off end # UART A there is a COM port on the board?
https://review.coreboot.org/#/c/33328/22/src/mainboard/asus/p8z77-m_pro/devi... PS22, Line 100: drq 0x30 = 1 # Enable device When a LDN is set to 'on' it will set the right bit in 0x030.
https://review.coreboot.org/#/c/33328/22/src/mainboard/asus/p8z77-m_pro/devi... PS22, Line 103: drq 0x70 = 1 # Keyboard IRQ : drq 0x72 = 12 # Mouse IRQ Those are irq's
https://review.coreboot.org/#/c/33328/22/src/mainboard/asus/p8z77-m_pro/devi... PS22, Line 127: 4e.0 most often 0c31 is set here as a dummy because PNP0C31 ACPI ID.
https://review.coreboot.org/#/c/33328/22/src/mainboard/asus/p8z77-m_pro/hda_... File src/mainboard/asus/p8z77-m_pro/hda_verb.c:
https://review.coreboot.org/#/c/33328/22/src/mainboard/asus/p8z77-m_pro/hda_... PS22, Line 30: : /* NID 0x11. */ those comments are pretty redundant. Please remove
https://review.coreboot.org/#/c/33328/22/src/mainboard/asus/p8z77-m_pro/roms... File src/mainboard/asus/p8z77-m_pro/romstage.c:
https://review.coreboot.org/#/c/33328/22/src/mainboard/asus/p8z77-m_pro/roms... PS22, Line 65: mainboard_config_superio You probably want to enable serial on the COM port here.
https://review.coreboot.org/#/c/33328/22/src/mainboard/asus/p8z77-m_pro/roms... PS22, Line 81: /* Setup "power on after fail" via NVRAM option */ : uint8_t acp; : int powerOnAfterFail = 0; : get_option(&powerOnAfterFail, "power_on_after_fail"); : : switch (powerOnAfterFail) { : case 2: : /* keep state before power off */ : acp = 0x40; /* 01000000b=0x40 */ : break; : case 1: : /* enable: turn computer on after computer fail */ : acp = 0x20; /* 00100000b=0x20 */ : break; : case 0: : default: : /* disable: turn off computer */ : acp = 0; /* bits 6-5 to zero */ : break; : } This is not mainboard specific. Move it to the superio code.
https://review.coreboot.org/#/c/33328/22/src/mainboard/asus/p8z77-m_pro/roms... PS22, Line 101: : /* Set "3VSBSW# enable bit" to keep RAM powered on S3 suspend */ : acp |= 0x10; : pnp_write_config(ACPI_DEV, 0xe4, acp); Setting this in the devicetree (irq 0xe4 = ...) should work too.
https://review.coreboot.org/#/c/33328/22/src/mainboard/asus/p8z77-m_pro/roms... PS22, Line 110: #if CONFIG(USE_NATIVE_RAMINIT) Remove