James has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31363 )
Change subject: mb/gigabyte: add GA-P67A-UD3R ......................................................................
Patch Set 3:
(13 comments)
https://review.coreboot.org/c/coreboot/+/31363/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/31363/3//COMMIT_MSG@10 PS3, Line 10: The P67 chipset has no graphics support.
Please add a blank line below.
Done
https://review.coreboot.org/c/coreboot/+/31363/3//COMMIT_MSG@14 PS3, Line 14: This is an original P67 chipset, and is affected by a SATA 2 hardware bug.
Any reference/URL to the bug?
http://web.archive.org/web/20110623010109/http://www.intel.com/support/chips... https://www.anandtech.com/show/4142/intel-discovers-bug-in-6series-chipset-b...
Are you asking for reading material, or to include a reference on the commit message?
https://review.coreboot.org/c/coreboot/+/31363/3/src/mainboard/gigabyte/ga-p... File src/mainboard/gigabyte/ga-p67a-ud3r/acpi/mainboard.asl:
PS3:
Is this file needed?
I guess not? Linux picks up a PWRF power button, the ACPI spec says PNP0C0C is "only needed if the power button is not supported using the fixed register space" so I'm guessing that's what PWRF is?
https://review.coreboot.org/c/coreboot/+/31363/3/src/mainboard/gigabyte/ga-p... File src/mainboard/gigabyte/ga-p67a-ud3r/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/31363/3/src/mainboard/gigabyte/ga-p... PS3, Line 21: : /* Disable USB ports in S3 by default */ : gnvs->s3u0 = 0; : gnvs->s3u1 = 0; : : /* Disable USB ports in S5 by default */ : gnvs->s5u0 = 0; : gnvs->s5u1 = 0;
Please remove, their value is already zero.
Done
https://review.coreboot.org/c/coreboot/+/31363/3/src/mainboard/gigabyte/ga-p... File src/mainboard/gigabyte/ga-p67a-ud3r/cmos.layout:
https://review.coreboot.org/c/coreboot/+/31363/3/src/mainboard/gigabyte/ga-p... PS3, Line 81: 2 0 Enable : 2 1 Disable
This isn't used
Is there a standardised enum layout? Otherwise I should just renumber the following enums?
https://review.coreboot.org/c/coreboot/+/31363/3/src/mainboard/gigabyte/ga-p... File src/mainboard/gigabyte/ga-p67a-ud3r/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/31363/3/src/mainboard/gigabyte/ga-p... PS3, Line 17: 0x0
just 0
Done
https://review.coreboot.org/c/coreboot/+/31363/3/src/mainboard/gigabyte/ga-p... PS3, Line 19: 0x0
just 0
Done
https://review.coreboot.org/c/coreboot/+/31363/3/src/mainboard/gigabyte/ga-p... PS3, Line 34: 0x0
just 0
Done
https://review.coreboot.org/c/coreboot/+/31363/3/src/mainboard/gigabyte/ga-p... PS3, Line 50: register "pcie_port_coalesce" = "0"
This is zero by default already, so it can be removed.
Done
https://review.coreboot.org/c/coreboot/+/31363/3/src/mainboard/gigabyte/ga-p... PS3, Line 56: off
Shouldn't this be on?
Possibly? It looks like coreboot hides the ME device anyway, what difference does it make?
Having it enabled here results in error messages from mei_me if using me_cleaner, though I suppose coreboot should hide the device if it detects me_cleaner being used.
https://review.coreboot.org/c/coreboot/+/31363/3/src/mainboard/gigabyte/ga-p... PS3, Line 84: (PCIEX1_1)
huh? How are the PCIe ports distributed?
There are 4 lanes, allocated to either the x4 slot, or 4 x1 slots (one of which is the x4 slot). This is configured in PCHSTRAP9. Vendor BIOS is able to configure this and hide/show the relevant root ports, I don't know if that's doable in coreboot as it stands, or if that's even considered in scope.
I'll write this up in the documentation for this port I've yet to write.
https://review.coreboot.org/c/coreboot/+/31363/3/src/mainboard/gigabyte/ga-p... PS3, Line 99: rounted
routed
Done
https://review.coreboot.org/c/coreboot/+/31363/2/src/mainboard/gigabyte/ga-p... File src/mainboard/gigabyte/ga-p67a-ud3r/romstage.c:
https://review.coreboot.org/c/coreboot/+/31363/2/src/mainboard/gigabyte/ga-p... PS2, Line 33: CNF2_LPC_EN
Ack
This code is gone.