Two more things to consider. First, you finally reduce items expected to be read-only to an integer (field value in struct acpi_cppc_rdonly_item), but some of these are dynamic and may change during runtime, in which case we have to keep the full register specification. Second, in the _CPC example I sent you offline for an AMD machine where processors do not report the CPPC extended feature flag, most present items have the PCC access method, so won't be covered by the code here. In the same example, there are only two FFH, for the ReferencePerformanceCounterRegister and DeliveredPerformanceCounterRegister, and the addresses seem to correspond to the MPERF and APERF MSR numbers respectively.

In D55474#1333742, @olce wrote:

As noted on previous revisions, SYS_RES_FFH should not exist, nor should acpi_ffh_write() and acpi_ffh_read(). So, basically, this needs a full rework.

acpi_ffh_read and write should exist. Different platform implement different ffh. But SYS_RES_FFH can be rmoved.

First, you'll have to discriminate, for each item, whether it can be represented as a struct resource or as a FFH. Thus, you need to extend the union inside struct acpi_cppc_item with a new field of type ACPI_GENERIC_ADDRESS, and extend the type member carefully to indicate that possibility.

Second, to handle read/write of FFH resource descriptors, assuming all these are to be interpreted in the same way, you'll need to add two field to struct acpi_cppc_ctx, one for the read function and another for the write functions. These two functions will differ depending on the architecture (amd64, arm64) but also possibly the processor manufacturer. More precisely, consulting the ARM FFH Specification document, FFH descriptors in the _CPC table are only possible for the DeliveredPerformanceCounterRegister and ReferencePerformanceCounterRegister fields. Basically, this means that you'll have to have read and write functions for all members of the _CPC table to support Arm correctly (and those for all items except the previous two will just return an error). These two fields could be compacted by using a single slot describing the actual FFH format for the field, which is then enough to apply the right read/write functions.

This is why we have acpi_ffh_read and acpi_ffh_write. Currently, there is no vendor-defined ffh now. It might be good to have read and write callback in this library. But we don't have to do in here. DeliveredPerformanceCounterRegister and ReferencePerformanceCounterRegister are for counting real frequecy.

For AMD processors, I don't know which are the FFH specifics as there is no documentation, which basically means we'll probably have to use Linux as a documentation source. Intel processors probably never use FFH in the _CPC table since Intel does not define the FFH format in their specification; this implies that there should never be a _CPC table on an Intel platform, but we'd better be safe than sorry and ensure read/write from FFH from such a hypothetical table will fail (until we are able to figure out the format). Depending on AMD's FFH specifics, although I quite doubt it, it might be beneficial to introduce a SYS_RES_MSR resource type (but no related changes to resource managers and the like) but, as you must have understood from the above, you can't get around having a FFH representation for items anyway (as some will have FFH values that are not MSRs).

We don't have "CPC table doesn't exist so hwpstate cppc mode breaks" problem on Intel. So yes, we don't have to read CPC table on Intel. I think SYS_RES_MSR would be bad as we don't manage MSR in SYS_RES format.

Finally, as long as AMD CPUs have the CPPC flag in their Extended Features, we should probably disable a generic ACPI CPPC driver, or at least the parts that are redundant (or we will have to evolve both, at least because currently hwpstate_amd(4) maintains a cache of the CPPC_REQUEST MSR).

Should be a future work I think as we don't have generic CPPC driver in tree now.

---

General style:

1. Please use error, not err.
2. Always test for an error with if (error != 0) (the != 0 part is currently missing).
3. Don't assign error in the if guard (except perhaps for very short assignment).

In D55474#1333882, @olce wrote:

Two more things to consider. First, you finally reduce items expected to be read-only to an integer (field value in struct acpi_cppc_rdonly_item), but some of these are dynamic and may change during runtime, in which case we have to keep the full register specification. Second, in the _CPC example I sent you offline for an AMD machine where processors do not report the CPPC extended feature flag, most present items have the PCC access method, so won't be covered by the code here. In the same example, there are only two FFH, for the ReferencePerformanceCounterRegister and DeliveredPerformanceCounterRegister, and the addresses seem to correspond to the MPERF and APERF MSR numbers respectively.

For first thing. Yes, I have changed this. Thanks!

Second, it should be a different story, right? In your machine, generic driver + PCC support can allow your device use CPPC.