Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 2:
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.
I'd also say that setting this up in ramstage is early enough.
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).
It would make reviewing the patch much easier for me when I can see the part that calls the API.
When are you planning to push the patch? For things that aren't basically adapted copies from some existing code, I usually wait with the review until the code that uses the new functionality is published.
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.
Sure within one manufacturer the blocks are usually much more similar than between vendors.
I don't expect the API to be exactly the same on all SIOs; the general idea should be applicable for all typical SIOs.
Having some functionality for raw HWM register access that has a slightly different parameter format for different vendors is better than having a super complicated API that can do everything for every device, but requires reading the whole implementation to see what need to be done.
I wanted to do something that was easy to use (or at least copy and adapt) by someone with no experience.
Sounds good.
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.
Ok. Apart from reviewing some patches I haven't looked too closely at the Fintek SIOs yet.
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.
Yeah, having an API for the most commonly used functionality that can be extended if needed is a good thing; for some more uncommon stuff the corresponding functionality can be added later or in the worst case things can be configured by some raw register access functions that are specific to a vendor.
I believe that an API with self explanatory names and easy to understand parameters makes it much easier for anyone to use it
I agree.