Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38988 )
Change subject: mainboard: New ivybridge board ASUS P8Z77-M ......................................................................
Patch Set 6: Code-Review+1
(27 comments)
The Asus P8Z77-V LX2 is also in the tree, if you want to use it as a reference
https://review.coreboot.org/c/coreboot/+/38988/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38988/1//COMMIT_MSG@12 PS1, Line 12: Does not yet boot. Submitting for eyeballs and help.
Any logs?
Ack
https://review.coreboot.org/c/coreboot/+/38988/4/src/mainboard/asus/p8z77-m/... File src/mainboard/asus/p8z77-m/Kconfig:
https://review.coreboot.org/c/coreboot/+/38988/4/src/mainboard/asus/p8z77-m/... PS4, Line 21: select INTEL_INT15
This option is for the code in mainboard.c, which got removed. […]
Done
https://review.coreboot.org/c/coreboot/+/38988/6/src/mainboard/asus/p8z77-m/... File src/mainboard/asus/p8z77-m/Kconfig:
PS6: Use SPDX here as well
https://review.coreboot.org/c/coreboot/+/38988/6/src/mainboard/asus/p8z77-m/... File src/mainboard/asus/p8z77-m/Kconfig.name:
PS6: SPDX here as well (or drop the copyright notices, we don't usually have them on these files)
https://review.coreboot.org/c/coreboot/+/38988/6/src/mainboard/asus/p8z77-m/... File src/mainboard/asus/p8z77-m/Makefile.inc:
PS6: Also SPDX
https://review.coreboot.org/c/coreboot/+/38988/5/src/mainboard/asus/p8z77-m/... File src/mainboard/asus/p8z77-m/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/38988/5/src/mainboard/asus/p8z77-m/... PS5, Line 8: shutdown
shut down
Done
https://review.coreboot.org/c/coreboot/+/38988/5/src/mainboard/asus/p8z77-m/... PS5, Line 8: pc
PC
Done
https://review.coreboot.org/c/coreboot/+/38988/5/src/mainboard/asus/p8z77-m/... PS5, Line 11: cpu
CPU
Done
https://review.coreboot.org/c/coreboot/+/38988/4/src/mainboard/asus/p8z77-m/... File src/mainboard/asus/p8z77-m/board_info.txt:
https://review.coreboot.org/c/coreboot/+/38988/4/src/mainboard/asus/p8z77-m/... PS4, Line 6: Flashrom support: ?
Yes, bitbanging SPI is a bit slow. […]
Done
https://review.coreboot.org/c/coreboot/+/38988/6/src/mainboard/asus/p8z77-m/... File src/mainboard/asus/p8z77-m/cmos.default:
PS6: This should be SPDX
https://review.coreboot.org/c/coreboot/+/38988/6/src/mainboard/asus/p8z77-m/... File src/mainboard/asus/p8z77-m/cmos.layout:
PS6: SPDX here as well
https://review.coreboot.org/c/coreboot/+/38988/6/src/mainboard/asus/p8z77-m/... File src/mainboard/asus/p8z77-m/devicetree.cb:
PS6: SPDX here as well
https://review.coreboot.org/c/coreboot/+/38988/6/src/mainboard/asus/p8z77-m/... PS6, Line 44: device pci 14.0 on end # USB 3.0 Controller I like aligning the "end" words with an extra space on the "on" lines
https://review.coreboot.org/c/coreboot/+/38988/6/src/mainboard/asus/p8z77-m/... PS6, Line 51: device pci 1c.0 on end # PCIe Port #1 What's on each PCIe root port? e.g.:
device pci 1c.5 on end # PCIe Port #6: Realtek RTL8111 GbE NIC
https://review.coreboot.org/c/coreboot/+/38988/6/src/mainboard/asus/p8z77-m/... PS6, Line 98: SATA Controller 1 SATA Controller (AHCI)
https://review.coreboot.org/c/coreboot/+/38988/6/src/mainboard/asus/p8z77-m/... PS6, Line 100: SATA Controller 2 SATA Controller (Legacy)
https://review.coreboot.org/c/coreboot/+/38988/5/src/mainboard/asus/p8z77-m/... File src/mainboard/asus/p8z77-m/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/38988/5/src/mainboard/asus/p8z77-m/... PS5, Line 4: // define ACPI_VIDEO_DEVICE _SB.PCI0.GFX0
Still there!
Done
https://review.coreboot.org/c/coreboot/+/38988/6/src/mainboard/asus/p8z77-m/... File src/mainboard/asus/p8z77-m/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/38988/6/src/mainboard/asus/p8z77-m/... PS6, Line 25: #include <drivers/intel/gma/acpi/default_brightness_levels.asl> Not needed anymore
https://review.coreboot.org/c/coreboot/+/38988/6/src/mainboard/asus/p8z77-m/... File src/mainboard/asus/p8z77-m/early_init.c:
https://review.coreboot.org/c/coreboot/+/38988/6/src/mainboard/asus/p8z77-m/... PS6, Line 34: { 1, 2, 4 }, /* Port 9: USB2 internal header USB910, bottom */ My OCD would appreciate an extra space before the single-digit numbers for alignment purposes
https://review.coreboot.org/c/coreboot/+/38988/6/src/mainboard/asus/p8z77-m/... PS6, Line 59: ARRAY_SIZE(register_values) You can put this inside the loop
https://review.coreboot.org/c/coreboot/+/38988/6/src/mainboard/asus/p8z77-m/... PS6, Line 60: int strictly speaking, size_t
https://review.coreboot.org/c/coreboot/+/38988/6/src/mainboard/asus/p8z77-m/... PS6, Line 68: Doing it this way saves a config state re-entry. Not sure if it's worth the hassle, though
https://review.coreboot.org/c/coreboot/+/38988/6/src/mainboard/asus/p8z77-m/... PS6, Line 113: struct pei_data pd = { I think it's better to set the non-default fields directly:
pei_data->spd_addresses = { 0xa0, 0xa2, 0xa4, 0xa6 };
https://review.coreboot.org/c/coreboot/+/38988/6/src/mainboard/asus/p8z77-m/... PS6, Line 131: Asus 2203 bios shows XUECA016, but no EC It's a desktop board, these don't use an EC (Embedded Controller)
https://review.coreboot.org/c/coreboot/+/38988/6/src/mainboard/asus/p8z77-m/... PS6, Line 154: /* These comments should start like this:
/* * foo bar ...
https://review.coreboot.org/c/coreboot/+/38988/5/src/mainboard/asus/p8z77-m/... File src/mainboard/asus/p8z77-m/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/38988/5/src/mainboard/asus/p8z77-m/... PS5, Line 12: FIXME
Fixed already, I guess?
Done
https://review.coreboot.org/c/coreboot/+/38988/5/src/mainboard/asus/p8z77-m/... PS5, Line 14: rear
nit: it's a desktop mainboard, it is assumed that all video ports will be on its rear side
Done