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 62:
(4 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 can go away, I guess?
Why? I didn't want to enable too much at once, but isn't it good to inform the OS of the hardware, so that it can apply quirks if it needs?
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
Have you figured out what these EC queries do? It would be nice to comment what they are about. […]
At the moment, I can hazard guesses based on which device is notified and by corroborating the values with other ACPI code. I can work on this, but it'll be easier once an OS will boot.
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 107: #| AC LoadLine | 10.3 mOhm | 2.4 mOhm | 3.1 mOhm | 3.1 mOhm |
Out of curiosity, where do you get the loadline values from?
6th Generation Intel Processor for U/Y-Platforms Datasheet, Volume 1 of 2 (#332990)
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
hmmmmm
It is fairly generic code and I have modified it, mostly according to the schematics and what I've seen of Intel's SwitchableGraphics.