Attention is currently required from: Nico Huber, Eugene Myers, Paul Menzel, Angel Pons. 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 182:
(2 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/35523/comment/f109dcb7_f4e4f6a9 PS178, Line 37:
The only thing that I can think of is ether something is not expecting the STM to be occupying the M […]
The SMI handler is now in this commit, but I've modified it a lot since I tried using the STM. Also, I can't make any claim about STM breaking its accesses until I make the ACPI table actually start issuing the SMI.
Patchset:
PS174:
- The commit message says vboot is untested but there are a lot of vboot references in the code. If some advanced feature is untested, please (re)move it.
I'll have to look at the vboot documentation for details on signing keys. Also, it appears SPI PRRs are configured separately in the security menu (i.e., "WP_RO" is only a recommendation), which is convenient.
However, while I've tried to hook up vboot correctly, I can't test it with the fallback mechanism. So, testing it is slightly riskier. If anything goes wrong, I'll have to flash externally.
vboot is broken on this board at present. I suspect that PTT isn't available in verstage until FSP configures the UMA for the CSME in romstage, but I didn't think to get a SPI flash log (so it could be anything). If you have any thoughts on this, I'd appreciate the pointers, but for now I've moved the vboot bits to a follow-up commit.
- There's explicit dead code, e.g. `#if 0`. Anything like that is a burden for the reviewer and future maintenance, it's unlikely to be acked.
Yes, I left the EC's second memory map in the ACPI files. This can be used when LGMR is configured: I now understand that LGMR can be faster than issuing I/O (in hindsight, this seems obvious: MMIO was always meant to be faster than IO).
I also commented out functions in the smihandler as a reminder that I may need to implement these.
This should be better now.
- There are comments that disagree with the code. For instance in `gpio.c` a lot of `// NC` but the code configures a native function. The commit doesn't have to be perfect, but it needs to be consistent with itself.
The comments are copied from the schematics, but the code is based on an inteltool dump. I've commented on the contradiction at the top of the file. I've also slowly been fixing the code.
My impression is that the OEM was lazy and configured a good-enough table that enabled pins even when the relevant device/feature was disabled. I'll continue trying to improve the table, but I hope this doesn't hold back merging the board. Any ideas? I can move the comments to a follow-up commit, but I think that would merely hide the fact that I am aware of some inconsistency with the OEM.