Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35523 )
Change subject: mb/acer: Add Acer Aspire VN7-572G ......................................................................
Patch Set 63:
(14 comments)
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... File src/mainboard/acer/aspire_vn7_572g/Kconfig:
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... PS62, Line 55: #config MAINBOARD_PCI_SUBSYSTEM_VENDOR_ID
This symbol isn't used anywhere: https://github. […]
Ah, okay.
So we don't do this anymore?
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... File src/mainboard/acer/aspire_vn7_572g/acpi/mbdefines.asl:
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... PS62, Line 29: 0xFE800000 FIXME: This defines a hardcoded memory region.
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... PS62, Line 34: 0x7CE7D018
This looks like a very bad idea. AIUI, it's a physical DRAM address and […]
That does sound very bad. How should this be handled instead?
The named variable "EMBA" does the same for other opregions. Marking as such.
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... File src/mainboard/acer/aspire_vn7_572g/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... PS62, Line 53: 0
This one should be enabled in order to use a SATA SSD.
Done
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... PS62, Line 58: 2
Shouldn't this be 3 for SATA3 6Gbps speeds?
I found an ATA error in dmesg once, even with the vendor BIOS. I thought I'd try this, in case limiting the speed fixes it, as it did for Purism's board. There isn't an error there now, though.
Removing this.
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... PS62, Line 170: x4 In the schematics, ports 1-4 are connected to PEG. Should these ports be enabled?
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... PS62, Line 202: register "usb2_ports[4]" = "USB2_PORT_FLEX(OC_SKIP)" # Bluetooth
Missing `usb2_ports[5]` for the touch screen? (likely unused)
Device does not have a touchscreen.
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... PS62, Line 204: register "usb2_ports[7]" = "USB2_PORT_FLEX(OC_SKIP)" # SD
Also missing `usb2_ports[8]` for the "finger printer"
Not present.
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... PS62, Line 208: # register "usb3_ports[2]" = "USB3_PORT_DEFAULT(OC_SKIP)" # Type-C Port
This and `usb3_ports[3]` seem to be used for USB Type-C
There's only one Type-C port?
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... PS62, Line 232: [PchSerialIoIndexUart2] = PchSerialIoDisabled,
This could be enabled as a debug port. […]
Done
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... PS62, Line 240: device pci 01.0 on end # Dedicated Graphics Device
I don't think SKL-U has such device. […]
You're right. Volume 2 of the datasheet for 6th gen platforms concurs. Is it sufficient to enable the root port? While coreboot doesn't remove Port 1 (dGPU), it doesn't locate anything behind it either[1], so something seems to be missing.
1. It may not be useful, but the log is here: https://gist.github.com/benjamindoron/37204d78c915d3fb7801462416d65c2d
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... PS62, Line 253: device pci 1c.0 on end # PCI Express Port 1
This is actually the dGPU root port. […]
Very well.
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... File src/mainboard/acer/aspire_vn7_572g/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... PS62, Line 26: DP1,
Not used, the DP AUX channel is not available over HDMI (for obvious reasons)
Done
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... File src/mainboard/acer/aspire_vn7_572g/ramstage.c:
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... PS62, Line 34: system76
Ack. […]
Done