Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35523 )
Change subject: mb/acer: Add Acer Aspire VN7-572G ......................................................................
Patch Set 62: Code-Review+1
(13 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
Why? I didn't want to enable too much at once, but isn't it good to inform the OS of the hardware, s […]
This symbol isn't used anywhere: https://github.com/coreboot/coreboot/search?q=MAINBOARD_PCI_SUBSYSTEM_VENDOR...
I know, it was used before, but it's now gone. Refer to the seven commits on GitHub search results
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... File src/mainboard/acer/aspire_vn7_572g/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... PS62, Line 155: Method (_Q1C, 0, NotSerialized) // _Qxx: EC Query, xx=0x00-0xFF
At the moment, I can hazard guesses based on which device is notified and by corroborating the value […]
Well, you can apply ACPI overrides even when using the vendor BIOS: https://wiki.archlinux.org/index.php/DSDT
I used that to track down how special keys work on a Sony laptop.
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.
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?
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... PS62, Line 107: #| AC LoadLine | 10.3 mOhm | 2.4 mOhm | 3.1 mOhm | 3.1 mOhm |
6th Generation Intel Processor for U/Y-Platforms Datasheet, Volume 1 of 2 (#332990)
Ack, thanks
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)
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"
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
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. Maybe "SkipInit" ?
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. This is the PEG port on the SA (System Agent), and it is not present on U nor Y series chips.
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. Would be nice to add a comment
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)
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
It is fairly generic code and I have modified it, mostly according to the schematics and what I've s […]
Ack. I would update the prints however