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 85:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... File src/mainboard/acer/aspire_vn7_572g/acpi/ec.asl:
PS62:
This is a lot of code given the "unknown" state in the commit message. Is […]
All functionality seems to work now. I can work on documenting it now. By and large, parts look very similar to other code I've seen, including Lenovo's H8.
https://review.coreboot.org/c/coreboot/+/35523/65/src/mainboard/acer/aspire_... File src/mainboard/acer/aspire_vn7_572g/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/35523/65/src/mainboard/acer/aspire_... PS65, Line 674: /* WBEC: Calls SMI function 0x11 */
No. […]
These functions don't seem to be necessary for correct functioning. As noted, the EC memory was mirrored into system memory and switching the operations to the EC's copy was enough.
However, these may serve a purpose that I am not yet aware of.
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
Definitely the vendor firmware, or is this value universal? In any case, I'll have to work on this.
As I understand, the vendor firmware mirrored the EC memory map into its own memory space. I don't really understand why.
I've noticed the same offsets in fields that the Lenovo H8 had with battery registers that caused them to implement paging, but my vendor's tables don't seem to do that.
I've switched the table to use the EC's copy and that seems to be working.
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... PS62, Line 34: 0x7CE7D018
I've fixed this by factoring it out and including some functionality from EC_ACPI. […]
Done
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... File src/mainboard/acer/aspire_vn7_572g/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/35523/62/src/mainboard/acer/aspire_... PS62, Line 46: // Mainboard specific
Right. The idea is to avoid having both "mbdefines" and "mainboard", as they sound a bit redundant.
Marking this as done. Other comments more directly address the code.