Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38143 )
Change subject: mb/asus/p5qc: Add ASUS P5Q as a variant (with documentation) ......................................................................
Patch Set 12: Code-Review+1
(15 comments)
It's pretty good, some minor things and it should be good to go
https://review.coreboot.org/c/coreboot/+/38143/1//COMMIT_MSG Commit Message:
PS1:
Just tested sleep mode (not hibernate, i don't have swap to do it) and it works, so ACPI S3 -- check […]
Right, so the tested and working things can go into the documentation. Things like FireWire should be marked as "Untested", but specifying that it is likely to work:
- FireWire: PCI device shows up and driver loads, no further test.
https://review.coreboot.org/c/coreboot/+/38143/12/Documentation/mainboard/as... File Documentation/mainboard/asus/p5q.md:
https://review.coreboot.org/c/coreboot/+/38143/12/Documentation/mainboard/as... PS12, Line 3: desktop desktop *board*
https://review.coreboot.org/c/coreboot/+/38143/12/Documentation/mainboard/as... PS12, Line 9: The following things are untested on this coreboot port: Add an extra blank line before untested things
https://review.coreboot.org/c/coreboot/+/38143/12/Documentation/mainboard/as... PS12, Line 27: No? Untested, likely does not work.
https://review.coreboot.org/c/coreboot/+/38143/12/Documentation/mainboard/as... PS12, Line 29: SOIC-8 Socketed DIP-8
https://review.coreboot.org/c/coreboot/+/38143/12/Documentation/mainboard/as... PS12, Line 53: Intel ME? It is disabled (does not have any firmware) on your board
https://review.coreboot.org/c/coreboot/+/38143/12/Documentation/mainboard/as... PS12, Line 55: ??? See mainboard: http://diy.yesky.com/imagelist/2008/167/1b529813etl1.jpg
Notice the square chip left of the blue PCIe x16 slot, south of the first PCIe x1 slot (PCIEX1_1). That is the clock generator.
And it is a *squints eyes* `ICS 9LPRS918JKLF` ?
https://review.coreboot.org/c/coreboot/+/38143/1/src/mainboard/asus/p5qc/Kco... File src/mainboard/asus/p5qc/Kconfig:
https://review.coreboot.org/c/coreboot/+/38143/1/src/mainboard/asus/p5qc/Kco... PS1, Line 55: config GPIO_C
I did it on stock BIOS as you wrote […]
Then it's a perfect match, so all good :D
https://review.coreboot.org/c/coreboot/+/38143/1/src/mainboard/asus/p5qc/Kco... PS1, Line 64: # The MARVELL IDE controller delays SeaBIOS a lot and results in an unbootable : # bogus disk. Compiling SeaBIOS without ATA support is a workaround. : : # The Asus P5QL PRO's Marvell controller (88SE6102-NNC2) does not need this, apparently.
My lspci output is: […]
Right, that seems to be the buggy controller type, so this section doesn't need changes.
https://review.coreboot.org/c/coreboot/+/38143/1/src/mainboard/asus/p5qc/var... File src/mainboard/asus/p5qc/variants/p5q/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/38143/1/src/mainboard/asus/p5qc/var... PS1, Line 58: device pci 1c.0 on end # PCIe 1 slot 1
Just reflashed w/ Silicon Image SIL3114 support and enabled 1c.3 port […]
That doesn't show what is connected to the 1c.3 port. Do you have the full lspci log?
https://review.coreboot.org/c/coreboot/+/38143/1/src/mainboard/asus/p5qc/var... PS1, Line 70: chip superio/winbond/w83667hg-a
I understood how should I dump registers so here it is: https://pastebin. […]
Thanks, only saw one thing (floppy should be enabled, see below)
https://review.coreboot.org/c/coreboot/+/38143/1/src/mainboard/asus/p5qc/var... PS1, Line 71: device pnp 2e.0 off end # FDC : device pnp 2e.1 off end # LPT1 : device pnp 2e.2 on # COM1 : # Global registers : irq 0x2a = 0x00 : irq 0x2c = 0x22 : irq 0x2d = 0x00 : io 0x60 = 0x3f8 : irq 0x70 = 4 : end Floppy is enabled on your board:
device pnp 2e.0 on # FDC # Global registers irq 0x2a = 0x00 irq 0x2c = 0x22 irq 0x2d = 0x00 # Floppy io 0x60 = 0x3f0 irq 0xf0 = 0x0e end device pnp 2e.1 off end # LPT1 device pnp 2e.2 on # COM1 io 0x60 = 0x3f8 irq 0x70 = 4 end
https://review.coreboot.org/c/coreboot/+/38143/1/src/mainboard/asus/p5qc/var... PS1, Line 93: 0x02 Your board has 0x82, but it's probably because some GPIOs are used as inputs. If things work fine, I'd leave it as-is.
https://review.coreboot.org/c/coreboot/+/38143/1/src/mainboard/asus/p5qc/var... PS1, Line 106: irq 0xe4 = 0x10 # 3VSBSW# enable
Should I change somrthing here?
Nope, all good.
https://review.coreboot.org/c/coreboot/+/38143/1/src/mainboard/asus/p5qc/var... PS1, Line 124: IDE
So what should I do with it?
If you want, you can update the comment. It's not critical, though