Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31323 )
Change subject: mb/asus/p5ql-em: Add mainboard ......................................................................
Patch Set 11:
(18 comments)
https://review.coreboot.org/c/coreboot/+/31323/11/src/mainboard/asus/p5ql-em... File src/mainboard/asus/p5ql-em/cmos.layout:
https://review.coreboot.org/c/coreboot/+/31323/11/src/mainboard/asus/p5ql-em... PS11, Line 51: #408 1 e 10 sata_mode Can remove I guess
https://review.coreboot.org/c/coreboot/+/31323/11/src/mainboard/asus/p5ql-em... PS11, Line 87: : #10 0 AHCI : #10 1 Compatible Can remove I guess
https://review.coreboot.org/c/coreboot/+/31323/11/src/mainboard/asus/p5ql-em... File src/mainboard/asus/p5ql-em/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/31323/11/src/mainboard/asus/p5ql-em... PS11, Line 26: subsystemid 0x1043 0x8336 Factor out and inherit?
I'm pretty sure the second number of the subsystemid isn't really meaningful
https://review.coreboot.org/c/coreboot/+/31323/11/src/mainboard/asus/p5ql-em... PS11, Line 41: device pci 6.0 off end # PEG 2 Only on P45
https://review.coreboot.org/c/coreboot/+/31323/11/src/mainboard/asus/p5ql-em... PS11, Line 50: 0,2,3 I would not be so sure
https://review.coreboot.org/c/coreboot/+/31323/11/src/mainboard/asus/p5ql-em... PS11, Line 69: # There are 2 PCIe slots on the board. : # Only one is enabled here because lack of testing I would rather enable both ports, so that they have a chance to work
https://review.coreboot.org/c/coreboot/+/31323/11/src/mainboard/asus/p5ql-em... PS11, Line 145: pannel panel
https://review.coreboot.org/c/coreboot/+/31323/11/src/mainboard/asus/p5ql-em... PS11, Line 152: device pci 1f.1 off end # PATA/IDE This does not exist, AFAIK
https://review.coreboot.org/c/coreboot/+/31323/11/src/mainboard/asus/p5ql-em... PS11, Line 159: IDE SATA, but in legacy mode
https://review.coreboot.org/c/coreboot/+/31323/11/src/mainboard/asus/p5ql-em... File src/mainboard/asus/p5ql-em/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/31323/11/src/mainboard/asus/p5ql-em... PS11, Line 30: Scope (_SB) { : Device (PCI0) : { Device (_SB.PCI0) {
https://review.coreboot.org/c/coreboot/+/31323/11/src/mainboard/asus/p5ql-em... PS11, Line 40: #include <southbridge/intel/i82801jx/acpi/sleepstates.asl> master now has a common sleepstates.asl file
https://review.coreboot.org/c/coreboot/+/31323/11/src/mainboard/asus/p5ql-em... PS11, Line 40: #include <southbridge/intel/i82801jx/acpi/sleepstates.asl> master now should use a common sleepstates.asl file
https://review.coreboot.org/c/coreboot/+/31323/11/src/mainboard/asus/p5ql-em... File src/mainboard/asus/p5ql-em/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/31323/11/src/mainboard/asus/p5ql-em... PS11, Line 24: Drop the empty line after the comment?
https://review.coreboot.org/c/coreboot/+/31323/11/src/mainboard/asus/p5ql-em... PS11, Line 39: 0x80862803, : 0x80860101, : 1, : : AZALIA_PIN_CFG(1, 0x03, 0x18560010), This seems to be HDMI audio
https://review.coreboot.org/c/coreboot/+/31323/11/src/mainboard/asus/p5ql-em... File src/mainboard/asus/p5ql-em/romstage.c:
https://review.coreboot.org/c/coreboot/+/31323/11/src/mainboard/asus/p5ql-em... PS11, Line 32: : static u8 msr_get_fsb(void) : { : u8 fsbcfg; : msr_t msr; : const u32 eax = cpuid_eax(1); : : /* Netburst */ : if (((eax >> 8) & 0xf) == 0xf) { : msr = rdmsr(MSR_EBC_FREQUENCY_ID); : fsbcfg = (msr.lo >> 16) & 0x7; : } else { /* Intel Core 2 */ : msr = rdmsr(MSR_FSB_FREQ); : fsbcfg = msr.lo & 0x7; : } : : return fsbcfg; : } Would it be worth factoring out this function into a more generic place? It seems to fit better as part of the "LGA775 socket" concept
https://review.coreboot.org/c/coreboot/+/31323/11/src/mainboard/asus/p5ql-em... PS11, Line 123: COMB_LPC_EN : | COMA_LPC_EN One of the serial ports of the SuperIO is disabled, so this seems unnecessary
https://review.coreboot.org/c/coreboot/+/31323/11/src/mainboard/asus/p5ql-em... PS11, Line 125: HW EC This comment is rather uninformative, isn't this LPC decode range used for the SuperIO HWM?
https://review.coreboot.org/c/coreboot/+/31323/11/src/mainboard/asus/p5ql-em... PS11, Line 157: printk(BIOS_DEBUG, : "Needs reset to configure CPU BSEL straps\n"); I would say this fits in one line