Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38988 )
Change subject: mainboard: Add new Ivy Bridge board ASUS P8Z77-M ......................................................................
Patch Set 11: Code-Review+1
(18 comments)
a
https://review.coreboot.org/c/coreboot/+/38988/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38988/1//COMMIT_MSG@7 PS1, Line 7: New board
Add Sandybridge board Asus P8Z77-M
Done
https://review.coreboot.org/c/coreboot/+/38988/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38988/7//COMMIT_MSG@7 PS7, Line 7: mainboard: New ivybridge board ASUS P8Z77-M
Please make it a statement by adding a verb (in imperative mood): […]
Done
https://review.coreboot.org/c/coreboot/+/38988/7//COMMIT_MSG@13 PS7, Line 13: - SeaBIOS boot
What version, and what OS?
Done. I expect SeaBIOS version to be the currently-default version.
https://review.coreboot.org/c/coreboot/+/38988/7//COMMIT_MSG@18 PS7, Line 18: - Hardware monitoring under Linux
What version?
Done (stated above)
https://review.coreboot.org/c/coreboot/+/38988/7//COMMIT_MSG@24 PS7, Line 24: - 2ch sound playback, Linux and Windows
What versions?
Done (stated above)
https://review.coreboot.org/c/coreboot/+/38988/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38988/11//COMMIT_MSG@7 PS11, Line 7: Ivy nit: this board also supports Sandy Bridge CPUs
https://review.coreboot.org/c/coreboot/+/38988/11//COMMIT_MSG@12 PS11, Line 12: With which CPU did you test this port?
https://review.coreboot.org/c/coreboot/+/38988/7/src/mainboard/asus/p8z77-m/... File src/mainboard/asus/p8z77-m/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/38988/7/src/mainboard/asus/p8z77-m/... PS7, Line 6: Return(Package(){0,0})
Add spaces (`util/autoport/ec_none.go`).
Ack (IMHO, we should auto-format ASL)
https://review.coreboot.org/c/coreboot/+/38988/10/src/mainboard/asus/p8z77-m... File src/mainboard/asus/p8z77-m/cmos.layout:
https://review.coreboot.org/c/coreboot/+/38988/10/src/mainboard/asus/p8z77-m... PS10, Line 6: # ----------------------------------------------------------------- : # Status Register A : # ----------------------------------------------------------------- : # Status Register B : # ----------------------------------------------------------------- : # Status Register C : #96 4 r 0 status_c_rsvd : #100 1 r 0 uf_flag : #101 1 r 0 af_flag : #102 1 r 0 pf_flag : #103 1 r 0 irqf_flag : # ----------------------------------------------------------------- : # Status Register D : #104 7 r 0 status_d_rsvd : #111 1 r 0 valid_cmos_ram : # ----------------------------------------------------------------- : # Diagnostic Status Register : #112 8 r 0 diag_rsvd1
We can get rid of this I think
Done
https://review.coreboot.org/c/coreboot/+/38988/10/src/mainboard/asus/p8z77-m... File src/mainboard/asus/p8z77-m/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/38988/10/src/mainboard/asus/p8z77-m... PS10, Line 21: Series 6 Cougar Point PCH
Series 7 Panther Point PCH
Done
https://review.coreboot.org/c/coreboot/+/38988/10/src/mainboard/asus/p8z77-m... PS10, Line 33: on
Please add a 2nd space after the "on" words, so that the "end" words are all aligned
Done
https://review.coreboot.org/c/coreboot/+/38988/10/src/mainboard/asus/p8z77-m... PS10, Line 38:
I think there's mixed tabs/spaces right before the comments
Done
https://review.coreboot.org/c/coreboot/+/38988/10/src/mainboard/asus/p8z77-m... PS10, Line 40: slots
Slots, in plural? There are two PCIe slots, the one closest to the CPU is on `device pci 01. […]
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
*poke*
https://review.coreboot.org/c/coreboot/+/38988/10/src/mainboard/asus/p8z77-m... File src/mainboard/asus/p8z77-m/early_init.c:
https://review.coreboot.org/c/coreboot/+/38988/10/src/mainboard/asus/p8z77-m... PS10, Line 100: int
unsigned int
*poke*
https://review.coreboot.org/c/coreboot/+/38988/10/src/mainboard/asus/p8z77-m... PS10, Line 108: for (i = 0; i < max; i += 2)
Difficult question how to optimize between code size and readability. […]
IMHO, it's well-commented code, so I don't mind keeping it like this. I'd add the rationale as a comment though. (See current patchset)
https://review.coreboot.org/c/coreboot/+/38988/11/src/mainboard/asus/p8z77-m... File src/mainboard/asus/p8z77-m/early_init.c:
https://review.coreboot.org/c/coreboot/+/38988/11/src/mainboard/asus/p8z77-m... PS11, Line 48: static const u8 register_values[] = { I'd strongly recommend adding some comment here explaining this is done to save some bytes. Otherwise, people may be tempted to change this to align it with other boards.
https://review.coreboot.org/c/coreboot/+/38988/10/src/mainboard/asus/p8z77-m... File src/mainboard/asus/p8z77-m/gpio.c:
https://review.coreboot.org/c/coreboot/+/38988/10/src/mainboard/asus/p8z77-m... PS10, Line 2: /* This file is part of the coreboot project. */
We got rid of these comments
Done