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 45:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35523/45//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35523/45//COMMIT_MSG@23 PS45, Line 23: ACPI
You can dump the DSDT of the vendor firmware. […]
I've redone the ACPI table since the first, broken one, but haven't tested it yet. I had planned to test all this at once, but the display (mainly dGPU at this time) is still giving me trouble.
https://review.coreboot.org/c/coreboot/+/35523/45//COMMIT_MSG@28 PS45, Line 28: - Remaining RAM slots (need other SPD addresses)
How many RAM slots does this thing have?
The vendor firmware and dmidecode list 4, but the internet says that there are only 2.
I checked the datasheet just now, which also only lists 2. I'll remove this, but will fix it if I find evidence to the contrary.
(I've also realised now that the vendor firmware would have been written for many devices, so hidden information in the advanced settings might even be a lie, after a fashion. dmidecode cannot help with confirmation on this, because it receives information from the vendor firmware)
https://review.coreboot.org/c/coreboot/+/35523/13/src/mainboard/acer/aspire_... File src/mainboard/acer/aspire_vn7_572g/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35523/13/src/mainboard/acer/aspire_... PS13, Line 150: # register "PcieRpEnable[6]" = "1"
NVMe should have x4 lanes. […]
I enabled it in the list of PCI devices and it probably doesn't hurt to enable it in general. I'm just concerned that these ports need special clock values, etc, that I don't know yet.
https://review.coreboot.org/c/coreboot/+/35523/40/src/mainboard/acer/aspire_... File src/mainboard/acer/aspire_vn7_572g/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35523/40/src/mainboard/acer/aspire_... PS40, Line 71: register "SaGv" = "3"
It does, so you should use `SaGv_Enabled`: […]
Done
https://review.coreboot.org/c/coreboot/+/35523/45/src/mainboard/acer/aspire_... File src/mainboard/acer/aspire_vn7_572g/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/35523/45/src/mainboard/acer/aspire_... PS45, Line 29: 0x10251037
No need to repeat the literal hex value in the comment
Done
https://review.coreboot.org/c/coreboot/+/35523/45/src/mainboard/acer/aspire_... PS45, Line 38: 0x1A
nit: use lowercase for these hex constants
Done