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 123:
(18 comments)
@Nico, were you considering disassembled ASL as "copied code?"
Also, this is presently untested. I'm having some issues with compiling crossgcc on Fedora 32.
https://review.coreboot.org/c/coreboot/+/35523/119/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/Kconfig:
https://review.coreboot.org/c/coreboot/+/35523/119/src/mainboard/acer/aspire... PS119, Line 13: # TODO: select DRIVERS_NVIDIA_OPTIMUS
then rebase on CB:28380 or remove and add as a follow-up patch on top of CB:28380
Done
https://review.coreboot.org/c/coreboot/+/35523/119/src/mainboard/acer/aspire... PS119, Line 75: #config TPM_PIRQ : # hex : # default 0x1e # GPP_A6_IRQ
what's the consequence of enabling this on your board which lacks the TPM?
I've double-checked the schematics and found that the interrupt is SERIRQ, so I'm removing this.
Note: I tested it too and there was no effect. I also found that this config option is only used by the LPC_TPM driver (in drivers/pc80/tpm/tis.c).
There is still the question of which driver should be selected. As I understand it, only one driver can be used, so should this be handled with variants? Would this be something to do now or something possibly in the future (if this scenario comes up)?
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/acpi/ac.asl:
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 11: Name (ACST, One)
Where is this consumed? Why is it needed to cache this?
battery.asl, but I'll switch it to use the variable directly from the EC instead.
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 14: // _HID: Hardware ID
Automated comments make it look like you borrowed the code from somebody else.
They were added by iASL's disassembler. I left them in because I had been unfamiliar with ACPI and still sometimes find it useful to be reminded what I'm working with.
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 19: If (EACS) : { : ACST = One : } : Else : { : ACST = Zero : }
Isn't it easier to just write […]
Ack
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 31: Method (_STA, 0, NotSerialized) // _STA: Status : { : Return (0x0F) : }
This is the default, are you sure it's needed?
I don't understand. Is the status method always implicitly declared?
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/acpi/battery.asl:
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 11: EB0A, 1, : , 2, : EB0R, 1, : EB0L, 1, : EB0F, 1, : EB0N, 1
These seem to describe the bits in `EB0S` below. Why not use them instead […]
That does seem correct. I'll attempt to identify some places this could be changed and comment below.
I agree with your assumption that this could be used to reduce the number of reads from the EC. For the same reason, the vendor firmware mirrored the EC RAM into its own memory space.
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 41: Offset (0xE0),
Same offset as above. So the EC provides different data on the same addresses […]
Right, I had noticed that the offsets were the same, but that full 'paging' wasn't implemented the way it is for Lenovo's H8.
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 69: Arg1 [One] = 0xFFFFFFFF : Arg1 [0x02] = 0xFFFFFFFF : Arg1 [0x04] = 0xFFFFFFFF : Arg1 [0x05] = Zero : Arg1 [0x06] = Zero
Again, looks like borrowed code. Wouldn't […]
Ack
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 78: Zero
0?
Done
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 78: (Local0 ^ One)
isn't this just `!Local0`? […]
EBCM is 1 bit. Good catch, thanks.
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 79: If (Local0) : { : Local1 = (EBDC * 0x0A) : } : Else : { : Local1 = EBDC : } : : Arg1 [One] = Local1 // Design capacity : If (Local0) : { : Local2 = (EBFC * 0x0A) : } : Else : { : Local2 = EBFC : }
Can this be written with one `if` block?
EBDC and EBFC are in different memory fields and this method is serialized. I'll give it a try.
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 100: 0x64
% calculations are easier to follow if you use decimal numbers.
Done
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 100: Divide (Local2, 0x64, Local7, Local6) // Warning capacity
Why Divide() instead of `Local6 = Local2 / 100`? Would it hurt to write […]
Done
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 102: Arg1 [0x05] = Local3
/* 7% */
Capacity of warning, I assume, but how do you know that? We don't know in which state bits may be, there's even a 'multiply by 10' case.
https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire... PS122, Line 283: (Local1 & 0x08) `EB0L`?
https://review.coreboot.org/c/coreboot/+/35523/119/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35523/119/src/mainboard/acer/aspire... PS119, Line 68: # TODO: register "DspEndpointDmic" = "3" The vendor is using this UPD to enable the microphone as none of the HDA pin_cfgs address a microphone and I'm including all of their undocumented verbs (extracted from the InstallPchHdaVerbTablePei module).
However, the microphone isn't working even with the vendor firmware (may be an OS issue).
https://review.coreboot.org/c/coreboot/+/35523/119/src/mainboard/acer/aspire... PS119, Line 140: # TODO: register "PcieRpAspm[9]" = "ASPM_L1" : # TODO: register "PcieRpL1Substates[9]" = "L1_SS_DISABLED"
either rebase on CB:39538 and remove the TODOs, or drop them and re-add in a follow-on patch that's […]
Done. Moving microphone statements to a separate comment.