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 10: Code-Review+1
(10 comments)
https://review.coreboot.org/c/coreboot/+/38988/10/src/mainboard/asus/p8z77-m... File src/mainboard/asus/p8z77-m/Kconfig:
https://review.coreboot.org/c/coreboot/+/38988/10/src/mainboard/asus/p8z77-m... PS10, Line 7: select BOARD_ROMSIZE_KB_8192 nit: sort these alphabetically?
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
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
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
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
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.0` and is electrically x16, and the other slot is `device pci 1c.0`, but it's electrically x4 (note how PCIe ports 2, 3 and 4 are disabled)
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 108: for (i = 0; i < max; i += 2) Why not do it like the Asus P8Z77-V LX2 does it?
https://review.coreboot.org/c/coreboot/+/38988/10/src/mainboard/asus/p8z77-m... PS10, Line 158: .pei_version = PEI_VERSION, : .mchbar = (uintptr_t)DEFAULT_MCHBAR, : .dmibar = (uintptr_t)DEFAULT_DMIBAR, : .epbar = DEFAULT_EPBAR, : .pciexbar = CONFIG_MMCONF_BASE_ADDRESS, : .smbusbar = SMBUS_IO_BASE, : .wdbbar = 0x4000000, : .wdbsize = 0x1000, : .hpet_address = CONFIG_HPET_ADDRESS, : .rcba = (uintptr_t)DEFAULT_RCBABASE, : .pmbase = DEFAULT_PMBASE, : .gpiobase = DEFAULT_GPIOBASE, : .thermalbase = 0xfed08000, Half of these are already set on the `pei_data` struct pointer parameter. If you change this code to update the members of `pei_data` directly, it will be much more concise.
https://review.coreboot.org/c/coreboot/+/38988/10/src/mainboard/asus/p8z77-m... PS10, Line 217: P8Z77-M Pro Copypasta! 😄
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