Change in coreboot[master]: superio/ite/common: Add support for SuperIOs with 6 temperature readi...
Attention is currently required from: Damien Zammit, Michał Żygowski, Paul Menzel, Michał Kopeć, Michael Niewöhner, Piotr Król, Felix Held. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63672 )
Change subject: superio/ite/common: Add support for SuperIOs with 6 temperature reading registers ......................................................................
Patch Set 1:
(4 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/63672/comment/3903dcee_0472bada PS1, Line 14: Additionally, decouple temperature offset/min/max configuration from TMPIN : mode configuration, since in these SIOs, TMPINs do not have to map directly : to temperature reading registers (e.g. TMPIN1 can read to temperature reading : register 3).
Can you put that into a separate commit?
+1, this can and should be done in two commits:
1. Decouple temperature offset stuff from TMPIN 2. Add support for Super I/Os with 6 temp reading regs
https://review.coreboot.org/c/coreboot/+/63672/comment/7a04be3a_acf4fb68 PS1, Line 19: Add a Kconfig option SUPERIO_ITE_ENV_CTRL_6_TEMPS for these ECs.
Is there no way to determin the number of registers at run-time?
I don't think so. But even if there was some way to detect this at runtime, it wouldn't be useful: the Super I/O on a given mainboard is most often known at build-time [1], so runtime detection code would be pointless.
[1] An exception would be HP 280 G2 (see CB:48386 for details), which can have an ITE IT8625E or an ITE IT8656E depending on the BOM (Bill Of Materials) configuration. I can think of two ways to identify which Super I/O chip is on the board: either checking the chip ID bytes (global config registers 0x20 and 0x21, what util/superiotool uses to identify chips) or by reading a PCH GPIO that indicates the BOM configuration (see bootblock.c function `bootblock_mainboard_init()`, the line initialising the `brd_str` variable). Both of these methods depend on some mainboard-specific information; at the very least, that the board can only have an IT8625E or an IT8656E. I haven't implemented Super I/O support on the 280 G2 yet, and I only have the board version with IT8656E.
TL;DR: Most likely not, and it wouldn't be worth it in most cases.
File src/superio/ite/common/env_ctrl.c:
https://review.coreboot.org/c/coreboot/+/63672/comment/df8f01a7_d86e2902 PS1, Line 138: nit: drop blank line
https://review.coreboot.org/c/coreboot/+/63672/comment/84b9515b_7cebc22b PS1, Line 145: temp - 1 Looks like you could refactor this to avoid the `- 1` everywhere.
-
Angel Pons (Code Review)