Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31323 )
Change subject: mb/asus/p5ql-em: Add mainboard ......................................................................
Patch Set 11:
(17 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
I was confused about ahci sata mode. I guess no-one will mind AHCI only.
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
You mean the subsystem DID? Why do you think so? It's often used by the OS to deal with quirks iirc.
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
Done
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
Done
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
From the DSDT it looks like 1c.1 is the other PCIe port.
https://review.coreboot.org/c/coreboot/+/31323/11/src/mainboard/asus/p5ql-em... PS11, Line 145: pannel
panel
Done
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
Done
https://review.coreboot.org/c/coreboot/+/31323/11/src/mainboard/asus/p5ql-em... PS11, Line 159: IDE
SATA, but in legacy mode
Done
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) {
That is not the same afaict and also not the same in the binaries. No other target is doing that btw.
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. […]
Done
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. […]
Done
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?
Done
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
Correct
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
It's not specific to LGA775. When moving things to a common place, I'll think about dealing with BSEL straps via SIO.
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
I intend to put this in a common place to reduce the boilerplate romstage code before moving to C_ENVIRONMENT_BOOTBLOCK
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?
Done
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
now it does ^^