Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 2:
(1 comment)
Patch Set 2:
Patch Set 2:
My real concern was having fan_control.h accepted, because it introduces a new API.
Only had a brief look at the patch, but wanted to get some feedback soonish, so here we go:
For the SIO chips of other vendors (mostly some ITE ones and I think one Nuvoton one) the configuration of the fan control/HWM part is done via device tree settings (look for the chip.h file in the SIO folders). The problem with the configuration via device tree settings is that the code can't tell apart if a setting isn't configured or if it is set to zero. I'm not opposed to do the configuration of the HWM by function calls in ramstage instead of device tree settings, since this would work around that device tree setting issue. For assigning IO ports and IRQ and that sort of stuff, the device tree works rather well, but at least some parts of the configuration of the HWM does indeed feel a bit shoehorned in to the device tree.
The discussion device tree setting vs. doing things in code recently also started in the context of the FSP-S settings, although the problem there is a bit different; the bigger issue there is that some FSP settings simply aren't hooked up to the device tree setting infrastructure and to a lesser extend that this is mostly unnecessary code (well, the FSP contains much more boilerplate compared to the device tree settings stuff).
For the SIO HWM part, I'd like to make sure that we won't end up with multiple non-devicetree API concepts; this getting too inconsistent for different SIOs HWMs would be my only serious concern here. Since the SIO configuration is rather board-specific and a board doesn't have a huge variety of SIOs (sure, some variants of one boards have 2 different SIO chips, but this is no blocker for me), it doesn't need to be one exact API that is used in the first and final form for all SIOs, but the general concept of how things are done should be similar and clear.
Is there a patch that shows how this new API is used?
Might be a good idea to have a discussion about the device tree setting vs. function calls in ramstage on the mailing list, but I don't expect too much opposition against this.
TL;DR since I'm not sure if this sounded a bit too negative: I do like the idea, but want to make sure that the change will improve the architecture and won't be a pain to maintain in the future.
I could have implemented it in bootblock, but I considered that code run faster once in ramstage, and fan control is only truly needed after at least 30 seconds after power up. This is specially true with fintek, as at reset it programs a 40% of max speed for all fans until fan control is started.
The patch that shows how this API is used cant't be pushed right now (jenkins would fail) because https://review.coreboot.org/c/blobs/+/33615 is not yet merged. It's the mainboard/amd/padmelon code, which will then make use of all the patches I have implemented so far. If you really want it, I could upload it and mark as work in progress (I'm not sure if jenkins would still examine it if declared work in progress).
I have done fan control on a nuvoton SIO before (UEFI, at Intel), so I do know that it's complicated (was easier this time, because I had done it before). My impression is that within devices of the same vendor, it's very similar (some times identical), but between different vendors it varies a lot, thus I made this API fintek specific. I wanted to do something that was easy to use (or at least copy and adapt) by someone with no experience.
Nuvoton SIO had a paging scheme using 2 IO addresses programmed within the main SIO page (as if a separate SIO) within the HWM while fintek uses a single memory based page (256 bytes range) from an address programmed within the main SIO page.
However, complications apart, both follow a general idea for an SIO only fan control (though nuvoton had a lot more options, including manual control that could be implemented via ACPI).
- The temperature source has to be defined.
- The fan type has to be defined (PWM or voltage controlled).
- 5 regions are defined through 4 inflection points.
- The fan speed in these 5 regions must be defined (Nuvoton you also needed to define if it was a single speed or linear change between inflections point)
- Define if fan is voltage controlled or PWM. If PWM, what frequency the PWM is to be set.
- (Nuvoton only). Define the feedback (tachometer).
- (Fintek only). Define the rate (RPM per time slice) at which fan speed will change.
Creating an API for each of the 5 common functionality looked the easiest way, with less parameters to be passed on each function. With nuvoton due to time consideration caused by multiple pages in a SIO like IO access, I decided to do a table of triplets: page, register value, and run the table as many times as there were pages and program a single page at a time... but the end result was that it still was complicated and whoever programmed it for a particular board had to have a good knowledge of the SIO itself.
I believe that an API with self explanatory names and easy to understand parameters makes it much easier for anyone to use it, though I'm not saying that the API I defined is the best possible. I'm only saying it's much more simple than what I previously implemented for nuvoton at Intel.
If anyone ever wants to program it for nuvoton, I suggest maintaining the table of triplets scheme (time considerations), but create macros that act in a similar mode as my defined API, thus hiding the details from the regular user. These macros would then actually fill the table of triplets.
My bad... fintek also uses 2 IO addresses (index, data), but index is odd (bit 0 set) and data is the next address.
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... File src/superio/fintek/f81803a/f81803a_hwm.h:
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... PS2, Line 28: #define TP_EXTERNAL_SENSOR2_OPEN 0x04 : #define TP_EXTERNAL_SENSOR1_OPEN 0x02
same here
Will not change this one, those are bit test positions.