Attention is currently required from: Patrick Georgi, Martin Roth, Paul Menzel, Arthur Heymans, Keith Hui. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38988 )
Change subject: mainboard/asus/p8x7x-series: Add new variant P8Z77-M ......................................................................
Patch Set 19: Code-Review+1
(18 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/38988/comment/274c965a_6efa34e2 PS19, Line 9: dumps. nit: Since CB:60006 the commit message line length limit is 72. This part goes past the limit.
https://review.coreboot.org/c/coreboot/+/38988/comment/b67b3273_a49d313c PS19, Line 14: tests nit: Since CB:60006 the commit message line length limit is 72. This part goes past the limit.
https://review.coreboot.org/c/coreboot/+/38988/comment/35eb7c7c_9fcad759 PS19, Line 27: Anything not working or untested?
Patchset:
PS19: Looks pretty good!
File src/mainboard/asus/p8x7x-series/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/38988/comment/04f6b0db_5e06699a PS19, Line 46: select SUPERIO_NUVOTON_NCT6779D
'SUPERIO' may be misspelled - perhaps ''?
*slaps checkpatch*
https://review.coreboot.org/c/coreboot/+/38988/comment/a23d838a_55060575 PS19, Line 47: select USE_NATIVE_RAMINIT If you select this, it's impossible to use MRC.bin with this board, so all the MRC.bin-specific code is useless. It's still possible to use native raminit without this select (`USE_NATIVE_RAMINIT` is user-configurable, it has a Kconfig prompt).
In the commit message you said you've tested MRC.bin, but I imagine this was before converting this board into a variant.
File src/mainboard/asus/p8x7x-series/variants/p8z77-m/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/38988/comment/a5d06ec4_41416426 PS19, Line 4: #include <southbridge/intel/bd82x6x/nvs.h> Hmmm, this doesn't exist. This file isn't being compiled in, and can be dropped.
https://review.coreboot.org/c/coreboot/+/38988/comment/c10bdf55_019a57ed PS19, Line 8: /* Critical temp: Shut down the PC at 95 degrees C */ : gnvs->tcrt = 95; : : /* Start throttling the CPU at 85 degrees C */ : gnvs->tpsv = 85; These values aren't used anywhere. The entire file can be dropped.
File src/mainboard/asus/p8x7x-series/variants/p8z77-m/cmos.layout:
https://review.coreboot.org/c/coreboot/+/38988/comment/a0295d40_181a9395 PS19, Line 26: 1 After CB:39828 this should be 2 bits, there's three possible settings for `sata_mode`
https://review.coreboot.org/c/coreboot/+/38988/comment/e5b0368b_ffc9f835 PS19, Line 95: 6 0 AHCI : 6 1 Compatible After CB:39828 this should be expanded accordingly:
6 0 AHCI 6 1 Compatible 6 2 Legacy
File src/mainboard/asus/p8x7x-series/variants/p8z77-m/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/38988/comment/a0d2ab64_02f895cc PS19, Line 10: /* OEM revision */ autoport adds this comment, but it's not true. I'd remove it.
File src/mainboard/asus/p8x7x-series/variants/p8z77-m/early_init.c:
https://review.coreboot.org/c/coreboot/+/38988/comment/c4504717_3d8e7468 PS19, Line 1: /* SPDX-License-Identifier: GPL-2.0-or-later */ nit: add a blank line before the #includes
File src/mainboard/asus/p8x7x-series/variants/p8z77-m/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/38988/comment/3e29735e_225d720f PS19, Line 6: device pci 00.0 on end # Host bridge : device pci 01.0 on end # PCIe Bridge for discrete graphics : device pci 02.0 on end # Internal graphics VGA controller Already specified in the devicetree, can be dropped.
https://review.coreboot.org/c/coreboot/+/38988/comment/51889aec_eee386d2 PS19, Line 11: register "pcie_port_coalesce" = "1" Shouldn't be needed, `device pci 1c.0` is enabled.
https://review.coreboot.org/c/coreboot/+/38988/comment/f42b4e3a_ebe2d15a PS19, Line 12: register "sata_interface_speed_support" = "0x3" # 0x3=SATAIII : register "sata_port_map" = "0x3f" : register "spi_lvscc" = "0x2005" : register "spi_uvscc" = "0x2005" : register "superspeed_capable_ports" = "0x0000000f" : register "xhci_overcurrent_mapping" = "0x00000c03" : register "xhci_switchable_ports" = "0x0000000f" # the 4 ports Already specified in the devicetree, can be dropped.
https://review.coreboot.org/c/coreboot/+/38988/comment/764c3238_efcedef9 PS19, Line 20: device pci 14.0 on end # USB 3.0 Controller : device pci 16.0 on end # Management Engine Interface 1 : device pci 16.1 off end # Management Engine Interface 2 : device pci 16.2 off end # Management Engine IDE Redirect : device pci 16.3 off end # Management Engine Keyboard/Text : device pci 1a.0 on end # USB2 EHCI #2 : device pci 1b.0 on end # High Definition Audio controller Already specified in the devicetree, can be dropped.
https://review.coreboot.org/c/coreboot/+/38988/comment/66e2cb8b_b0b34a2f PS19, Line 35: device pci 1d.0 on end # USB2 EHCI #1 : device pci 1e.0 off end # PCI bridge Already specified in the devicetree, can be dropped.
https://review.coreboot.org/c/coreboot/+/38988/comment/3650e02a_702addc8 PS19, Line 99: device pci 1f.2 on end # SATA Controller (AHCI) : device pci 1f.3 on end # SMBus : device pci 1f.5 off end # SATA Controller (Legacy ports 4-5) : device pci 1f.6 off end # Thermal Already specified in the devicetree, can be dropped.