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 14: Code-Review+1
(4 comments)
Looks pretty good
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? […]
Mainboard-specific quirks are usually related to firmware. I don't really mind, though.
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) { […]
autoport uses this: https://github.com/coreboot/coreboot/blob/master/util/autoport/main.go#L879
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 […]
Ack
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 […]
Ack