Attention is currently required from: Felix Singer, Nico Huber, 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 175:
(3 comments)
Patchset:
PS174:
This commit is already pretty big as is. And it's not 100% tidy. For instance, what I spotted skimming through random files:
I can respond to these points, but I realise there would be more issues. In general, I've tried to document some issues/observations/etc. as comments in the code. Perhaps I forgot some.
Overall, moving EC stuff (particularly the smihandler) to a follow-up commit is probably a good idea.
- 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.
- 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.
- 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.
File src/mainboard/acer/aspire_vn7_572g/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/35523/comment/ed7278ec_2cd85902 PS174, Line 36: /* Undocumented settings, see data blob in
Not easy to decode but this is usually documented in the codec's datasheet. Note, there are two types of verbs: 4 bit and 12 bit. The former are stored in bits 19..16 and 15..0 are used for data. The latter use 19..8 with only 8 bits of data.
I don't have a datasheet for ALC255, but these were actually standardised verbs. They're in the HDA spec, so I'm not concerned about using older Realtek datasheets (which were more helpful than I originally thought). I've now documented the verbs.
Most of it is coefficient programming, which is not very useful to know on its own. I still don't know why we're programming each coeff. with its specific value.
For instance, the first `0x02050038` is
- codec 0x0
- node 0x20
- verb 0x5
- data 0x0038
Right, Intel documents in the HDA spec that 0x7 and 0xF mark an extended verb (it would have been useful to read that part of the spec before looking at verb IDs in Realtek datasheets...)
https://review.coreboot.org/c/coreboot/+/35523/comment/08457cc5_6eb85c6d PS174, Line 90: 0
This should be `2`, shouldn't it? AFAIK, the code processing this table […]
I agree, this looks odd. While I copied these from a hexdump of a vendor UEFI module (InstallPchHdaVerbTablePei), it clearly modifies some of these before submitting it to the silicon init modules (in the data, the pin config NIDs also have codec address 0). I've corrected this now.
For what it's worth, the Kabylake RVP3 (the board I copied the comment from) programs both tables on codec 0.