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 ......................................................................
Patch Set 4:
(19 comments)
Patch Set 1:
(4 comments)
Patch Set 1:
(5 comments)
Hi, welcome to coreboot!
I've got a few suggestions for you. If you have any questions, feel free to ask me anything :-)
TL:DR; Looks like I need to fix these errors & commit it again ¯_(ツ)_/¯ (excuse me if any kind of emoji is prohibited here)
Oh, using emojis is perfectly fine. What's more, have you seen our 404 error page? https://review.coreboot.org/c/nothing
Patch Set 3:
I really can't compare it for now, so here is dump from inteltool: https://pastebin.com/dB1RQa2B Also marked "done" all solved (IMHO) problems
Perfect, thank you. Did you make this log while running coreboot, or with the stock BIOS? I would need a log with the stock BIOS.
Patch Set 3:
How can i send board_status about P5Q, not P5QC. Is it because of P5Q is a variant of P5QC in repo?
Yes, board_status doesn't know about variants yet. Even then, it has been very useful to double-check some things.
https://review.coreboot.org/c/coreboot/+/38143/1//COMMIT_MSG Commit Message:
PS1:
About working state on P5Q: yes, it works, I also sent board_status but it marked as P5QC. […]
Congrats on your first working coreboot port :)
Right, board_status doesn't know about variants yet. Still, thanks for uploading a report, the coreboot log is quite useful :)
For documentation, I think you can use this as a reference:
https://doc.coreboot.org/mainboard/intel/dg43gt.html
You can find the .md source file in the `Documentation` subfolder in the coreboot repo.
That Intel board does not work with in-circuit flashing, which consists of using an external programmer while the flash chip is still connected to the mainboard. It likely does not work on the Asus P5Q either, but since it has a socketed flash chip, you don't need to flash in-circuit: just remove the flash chip carefully. So, that part should be changed for the Asus P5Q documentation.
About untested things, here's some suggestions for testing various things. I've also included basic checks for things that need extra hardware: that way, even if not tested fully, they are likely to work well :D
- Does the RS232 port work? If it does, coreboot can output its logs through it, which is very useful when debugging things.
- If you have some conventional PCI cards, it would be nice to test the PCI slots. Pro-tip: use an Ethernet card with a boot ROM on it. Since it appears as a boot option on SeaBIOS, you don't need to boot to the OS, so it is faster :^) To do this, I have some 3Com cards which look like this one, but have a chip in the PLCC socket (top right corner):
https://i.ebayimg.com/images/g/JyQAAOSwxa9avWvz/s-l1600.jpg
- Please confirm that ACPI S3 (suspend to RAM) works. That is, that the mainboard can go to sleep and resume successfully. If it works fine, I would also run a longer test: for example, put it to sleep before going to bed, and check if it resumes fine the next morning.
- LAN probably works, looks like kinux can load the driver correctly, so if a MAC address is present it should work fine. Test if the MAC address stays after unplugging the mainboard, just in case the NIC keeps the MAC address set by the vendor firmware.
- Floppy might not work, but who needs it anyway? In any case, I could check if it works on the P5QL PRO, as these mainboards are quite similar.
- The TPM slot needs additional code. If you don't have a TPM installed, I would not bother. If somebody wants to use it, support can be added rather easily.
- The FireWire controller shows up in lspci and its driver loads fine, so it should work.
https://review.coreboot.org/c/coreboot/+/38143/1//COMMIT_MSG@7 PS1, Line 7: ASUS P5Q support and menu entries added
Done
Yes, you can use `git commit --amend`. As long as the Change-Id line remains unchanged, Gerrit will treat the new commit as a new patchset of this change.
https://review.coreboot.org/c/coreboot/+/38143/1//COMMIT_MSG@10 PS1, Line 10:
Done
Everybody forgets to sign-off from time to time :P
If you have configured git properly, you can use `git commit -s` for new commits, which adds the sign-off automatically.
https://review.coreboot.org/c/coreboot/+/38143/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38143/4//COMMIT_MSG@10 PS4, Line 10: Signed-off by Missing a `-`: Signed-off-by
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
Here is GPIO registers: https://pastebin. […]
I've checked and it matches the current gpio.c file. Did you make this log with the stock BIOS or with coreboot?
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. Does the Asus P5Q have a Marvell 88SE6102? lspci seems to differ, but the best way to know is to look at the physical chip on the board.
https://review.coreboot.org/c/coreboot/+/38143/4/src/mainboard/asus/p5qc/Kco... File src/mainboard/asus/p5qc/Kconfig:
https://review.coreboot.org/c/coreboot/+/38143/4/src/mainboard/asus/p5qc/Kco... PS4, Line 39: default "p5qc" if BOARD_ASUS_P5QC || BOARD_ASUS_P5Q I've added some suggestions to the devicetree on Patchset 1, so you might have to undo this.
I can see why you would want to do this, though: the boards are very similar, so having a full devicetree is inefficient. The best solution would be another commit to make these boards use overridetrees, but this can be done later.
https://review.coreboot.org/c/coreboot/+/38143/1/src/mainboard/asus/p5qc/var... File src/mainboard/asus/p5qc/variants/p5q/devicetree.cb:
PS1:
Done
Right. Some things are different on the Asus P5Q, so I will open comments on them.
https://review.coreboot.org/c/coreboot/+/38143/1/src/mainboard/asus/p5qc/var... PS1, Line 29: end Personally, I like to align these `end` keywords. If you want, you can do it as well, but it's not a requirement. I think I might have OCD :P
https://review.coreboot.org/c/coreboot/+/38143/1/src/mainboard/asus/p5qc/var... PS1, Line 30: device pci 1.0 on end # PEG Note: This port on the northbridge is where the GPU is connected.
https://review.coreboot.org/c/coreboot/+/38143/1/src/mainboard/asus/p5qc/var... PS1, Line 31: device pci 2.0 off end # Integrated graphics controller : device pci 2.1 off end # Integrated graphics controller 2 These devices don't exist on the P45 northbridge, so you can remove these lines
https://review.coreboot.org/c/coreboot/+/38143/1/src/mainboard/asus/p5qc/var... PS1, Line 33: device pci 3.0 off end # ME : device pci 3.1 off end # ME : device pci 3.2 off end # ME : device pci 3.3 off end # ME Since these Asus boards don't have any ME firmware, the ME devices should be disabled by default, so these lines can be removed.
https://review.coreboot.org/c/coreboot/+/38143/1/src/mainboard/asus/p5qc/var... PS1, Line 41: # Set AHCI mode. This comment is outdated (AHCI mode was made the default option, because it's better).
You can instead describe what the value in the `sata_port_map` register does. In short, it's a bitfield that indicates which of the six SATA ports on the ICH10 southbridge should be enabled. On this board, all of them should be enabled. So the comment can be changed to:
# Enable all six SATA ports.
https://review.coreboot.org/c/coreboot/+/38143/1/src/mainboard/asus/p5qc/var... PS1, Line 43: register "sata_clock_request" = "0" : register "sata_traffic_monitor" = "0" You can remove these entries. The default value is zero already.
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 The used PCIe ports may be different on the P5Q. You can check `lspci` while running the stock BIOS. Specifically, `lspci -nntv` shows which device is connected to which PCIe root port (the `1c.x` devices). With these logs, we can compare and see if anything is wrong.
I have used this picture as a reference. Am I looking at the correct board?
http://diy.yesky.com/imagelist/2008/167/1b529813etl1.jpg
From the kernel log of your board_status:
- 1c.0 has a [1106:3483] VIA Labs VL805 USB 3.0 Host Controller. - 1c.1 is empty. - 1c.2 has a [11ab:6121] Marvell 88SE6121 SATA II Controller. - 1c.3 is disabled. - 1c.4 has a [1969:1026] Atheros AR8121/AR8113/AR8114 PCI-E Ethernet Controller. - 1c.5 has a [11c1:5811] LSI FW322/323 [TrueFire] 1394a Controller.
Let me guess... the USB 3.0 controller is on a PCIe expansion card. The other devices would be part of the mainboard.
You can test the other PCIe slot with the USB 3.0 card. If the GPU is so bulky it blocks other slots, you can still grab a coreboot log through the RS232 port without a graphics card. Of course, you would need another computer with a serial port (or an USB-to-RS232 adapter).
What seems to be missing is the Silicon Image SATA II controller. Instead, there's a Marvell SATA II controller. Maybe I am not looking at the correct mainboard? It could also be that the Silicon Image controller is connected to the disabled `1c.3` PCIe port. Logs will help.
https://review.coreboot.org/c/coreboot/+/38143/1/src/mainboard/asus/p5qc/var... PS1, Line 70: chip superio/winbond/w83667hg-a You can use `util/superiotool` while running the stock BIOS to double-check these registers. Note that offset 0x30 indicates whether a certain device is enabled or not.
The SuperIO datasheet isn't avaliable on the internet, but you can request it from Nuvoton. (that's how I obtained it)
https://review.coreboot.org/c/coreboot/+/38143/1/src/mainboard/asus/p5qc/var... PS1, Line 106: irq 0xe4 = 0x10 # 3VSBSW# enable Even if superiotool does not set bit 4 of this register, it should be enabled. Otherwise, the RAM loses power when the board goes to sleep (suspend to RAM), and then resume does not work.
https://review.coreboot.org/c/coreboot/+/38143/1/src/mainboard/asus/p5qc/var... PS1, Line 120: device pci 1f.1 off end # PATA/IDE This device does not exist on ICH10, so you can remove this line
https://review.coreboot.org/c/coreboot/+/38143/1/src/mainboard/asus/p5qc/var... PS1, Line 124: IDE Strictly speaking, this is still SATA, but when it's configured in Legacy mode.