Keith Hui has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38988 )
Change subject: mainboard: New ivybridge board ASUS P8Z77-M ......................................................................
Patch Set 7:
(13 comments)
https://review.coreboot.org/c/coreboot/+/38988/6/src/mainboard/asus/p8z77-m/... File src/mainboard/asus/p8z77-m/devicetree.cb:
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
Done
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.: […]
Ack
https://review.coreboot.org/c/coreboot/+/38988/6/src/mainboard/asus/p8z77-m/... PS6, Line 98: SATA Controller 1
SATA Controller (AHCI)
Ack
https://review.coreboot.org/c/coreboot/+/38988/6/src/mainboard/asus/p8z77-m/... PS6, Line 100: SATA Controller 2
SATA Controller (Legacy)
Ack
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
Done
Ack
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
Not quite in this case. The header is actually labeled USB2_910 on the PCB. I plan on updating to that in the next patch set.
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
Ack
https://review.coreboot.org/c/coreboot/+/38988/6/src/mainboard/asus/p8z77-m/... PS6, Line 60: int
strictly speaking, size_t
Ack
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
If the compiled binary code is shorter; since all pnp_xxx() functions are inlined except {enter,exit}_conf_state, if there is a way to enable serial reliably with this construct, I'd gladly do it.
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: […]
Ack
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)
Ack
https://review.coreboot.org/c/coreboot/+/38988/6/src/mainboard/asus/p8z77-m/... PS6, Line 154: /*
These comments should start like this: […]
Ack
https://review.coreboot.org/c/coreboot/+/38988/7/src/mainboard/asus/p8z77-m/... File src/mainboard/asus/p8z77-m/early_init.c:
https://review.coreboot.org/c/coreboot/+/38988/7/src/mainboard/asus/p8z77-m/... PS7, Line 108: for (i = 0; i < max; i += 2) {
braces {} are not necessary for single statement blocks
Ack