Could you rename the AMD_CPPC_ prefix to AMD_CPPC_CAPS_1_, just to make it apparent that these are applicable (only) to the CAPS 1 MSR?

Same here please, AMD_CPPC_ => AMD_CPPC_REQUEST_.

You're basically open-coding some AMD_CPPC_ macros (those to prefix with AMD_CPPC_CAPS_1_) here. Please use the macros instead.

That second read actually reads the same MSR. I think you should completely remove data2 and use data instead. This distinction made sense in the Intel case as there is a specific MSR targeted at the package (MSR_IA32_HWP_REQUEST_PKG) but there's no such thing in AMD processors.

Even if there is no specific hardware support for package control on AMD processors, I think that providing this functionality to users is beneficial, in that it simplifies a lot the configuration of the "baseline" case where all cores are set to the same value.

You probably meant PSTATE_CPPC here, as bailing out in the case of PSTATE_CPPC_PKG_CTRL does not seem to make sense.

This is in the wrong place. It must be moved between the lines val = ((r >> 24) & 0xFFULL) * 100 / 0xff; and if (val > 100) {.

It would be better to use AMD_CPPC_REQUEST_ENERGY_PERF_BITS here, even if that's more verbose. With it, we immediately know that the code is correct without having to check if the shift and mask are correct.

If you find that too verbose, factor out the formula into a macro.

Missing input/output at this point (see previous comment).

Generally, support for PSTATE_CPPC_PKG_CTRL is missing (as said above, I think this functionality is

We may want to mimick what happens in the Intel counterpart, which means that setting the knob for one core should affect all the other cores. If so, basically, when PSTATE_CPPC_PKG_CTRL is set, we have to loop over all CPUs for the setting part.

Same as above, it would be preferable to use a macro based only on AMD_CPPC_REQUEST_ENERGY_PERF_BITS and avoid the hardcoded 24.

Aren't we supposed to use MSR C001_0280 ("Performance Time Stamp Counter (CU_PTSC)") here? Or are we sure the regular TSC is just invariant if PSTATE_CPPC_PWR_CALC is set?

But maybe that's not worth bothering, as it appears most AMD processors do not support PSTATE_CPPC_PWR_CALC (i.e., only some of family 16h do, skimming various PPR manuals).

Similarly as above, I'd prefer macros here on the right-hand side of |=, and no hardcoded shifts (and proper mask applied to the new value, just in case, even though you aptly chose uint8_t as the type for the cppc_settings fields).

Since you (rightly) added an occurrence of a call to device_set_desc() earlier, you can remove this one (redundant).

Mark tell me that this can be moved to a place before we lock a thread. Should we move it back?

The sysctl_handle_int() call doesn't need to be inside the sched_bind() section, it should be moved earlier.

I think we should remove the check as the value is always between 0~255. The value won't never exceed 100 when we scale down the value. 255 * 100 / 255 = 100

I will check it but needs some time because I see they have different capabilities register value for each CPU. Thus I think it is package level in default.

I think I should delete this function totally as you know that this function is accessible in very few models. However, I will leave the macro in specialreg.h alone to prevent being reused by AMD.

So we have AMD_CPPC_CAPS_1_*_PERF that are function-like macros and AMD_CPPC_REQUEST_*_BITS that are plain ones. I think both should be consistent.

I have a slight preference for the AMD_CPPC_REQUEST_*_BITS as it allows to spot problems more immediately.

Proposing a revamp using only *_BITS with an additional set of function-like macros (BITS_VALUE(), BITS_FROM_VALUE(), SET_BITS_VALUE()). SET_BITS_VALUE() is similar to SET_VALUES(), and replaces it.

See the attached patch.

There's a space between #define and the macro name on these lines, contrary to other existing lines and new MSR_AMD* ones where there's a TAB, so use a TAB here preferably (or switch all lines to a space).

I was actually wondering if we should keep it. Currently, this structure is only written to, and only some of its values are immediately used in amd_set_autonomous_hwp() and nowhere else after that. In particular, amdhwp_dump_sysctl_handler() never uses sc->req.

We could perhaps keep this structure, and use it in amdhwp_dump_sysctl_handler(), for the following reasons. For Intel processors, the capabilities can change at any moment due to changing load/thermal package conditions (in fact, not more than once per second, and only the guaranteed and most efficient ones), according to the Architecture Software Developers reference manual. For AMD, the Architecture's Programmer Manual says that the "guaranteed" performance corresponds to ideal conditions, which can be interpreted as "will not change", contrary to what Intel does. Assuming it's the case in practice, then saving the capabilities into the structure here makes sense and then saves doing rdmsr() in amdhwp_dump_sysctl_handler().

Preferring a more paranoid approach is also fine, but in this case this structure is in fact not needed and should be removed.

If keeping it, should be renamed to hwpstate_cppc_state to better reflect purpose, and req should be moved to it.

See inline comment above about struct hwpstate_cppc_setting (=> hwpstate_cppc_state) about adding req to this structure.

If you keep it (see previous comment).

Please see discussion above on struct hwpstate_cppc_setting (=> struct hwpstate_cppc_state) to see if this function needs changing to stop reading anything from the hardware and just reporting values stored in sc->cppc_state.

Style (I know hwpstate_intel does the same, and should gradually be fixed, but this one is new, so...).

There are also several occurences of if (ret) => if (ret != 0) below, not creating an inline comment for each.

If the wrmsr_safe() before the loop for the current CPU worked, we should assume that it is going to work on all others. So:

• If an error nonetheless occurs here inside the loop, I would just ignore it (print some diagnostic, probably only once), and reset ret to 0.
• That allows to finish the loop and set sc->req to what the current CPU was set.

Doing so would allow us to be informed that the seemingly impossible case actually occurred, and at least try to have most CPUs in line with the request, and also have sc->req set to the requested value, even if setting fails on some other CPU.
That said, I'd also move the sc->req after the loop earlier, just after the test that the initial wrmsr() succeeded.
I don't think that adding code to revert previous modifications on an error in the loop is necessary at this point.

You must set the local CPU's sc->req as well in the loop, else the other (per-CPU) "epp" knobs will report a wrong value.

See inline comment at top about whether to keep struct hwpstate_cppc_setting, and thus sc->cppc_settings.

I'd keep the original comment more or less. It's a bit obscure, and I haven't been able so far to verify what it says.

Would rather set it to something like "AMD CPPC" on PSTATE_CPPC. "Cool`n'Quiet 2.0" is... cool but quite confusing.

Either 0 has to be returned instead, or hwpstate_attach() must be modified to read the flags again into sc->flags (see DEVICE_PROBE(9) manual page).

First option seems fine.

I found a problem that we cannot actually set sc->req to all cpu since each cpu core can have different low and high capabilities. Only the EPP is in range 0~255 for each core. So we have to rdmsr each req bits for all cpu and set bits then finally wrmsr it back.

It's in fact apparently possible to get to the desired devices directly, through something like:

const devclass_t dc = devclass_find(HWP_AMD_CLASSNAME);

and in the loop:

const device_t dev = devclass_get_device(dc, i);

which is simpler and independent of the precise hierarchy of devices (acpi -> acpi_cpu -> hwpstate).

Also, I suggested HWP_AMD_CLASSNAME to factor out this device's class name, which should be changed to hwpstate_amd.

Re-reading the last line of my comment: "You must set the local CPU's sc->req as well in the loop, else the other (per-CPU) "epp" knobs will report a wrong value.", the use of "set the local CPU's sc->req" could be confusing. I meant the softc associated to the core whose MSR you're writing into, i.e., while looping over each CPU, we should update the corresponding softc.

Once using the right softc, as you already do in sysctl_epp_select_foreach_core(), you don't really need to read the MSR at all.

Consistently with another inline comment, please use HWPSTATE_AMD_CLASSNAME and define this macro to "hwpstate_amd" near the top of the file.

Also, please replace existing verbatim "hwpstate" with HWPSTATE_AMD_CLASSNAME (it seems that makes sense for all occurrences).

We must not short-circuit sysctl_epp_select_foreach_core() in this case (in case of changes to hwpstate_pkg_ctrl_enable).

Label hwpstate_pkg_ctrl (or whichever name you choose) should be to statement if (hwpstate_pkg_ctrl_enable).

I currently assumes that CPPC is only controlled by hwpstate_amd module and thus all epp in any condition should be same for all cores. That is why I short-circuit in here.

If we trusted the value of sc->req, we should be able to short-circuit in if r == sc->req in package control mode as the msr should not be modified by other modules.

If we don't trust it, we should re-read the sc->req everytime and apply epp vlaue only.

I think I will just remove this code segment. Thanks!

I see. But hwpstate_pkg_ctrl_enable is RDTUN currently. Which means we are unable to change it until next reboot. This is same as the intel's code.

After this commit I see a warning during boot:

sysctl_register_oid: can't re-use a leaf (machdep.hwpstate_pkg_ctrl)!

I guess because hwpstate_intel uses the same sysctl name, and both drivers are compiled into the kernel. We could rename this one, or maybe change both drivers to dynamically register the sysctl during attach.

I will think about this tomorrow. My final go is to allow powerd to set different performance based on the power status (AC or Battery). So it would be better for this sysctl to be same under two driver. Maybe this should be exposed in cpufreq level instead of the CPPC driver.

If you're planning to MFC this to stable/14, Mark's suggestion of dynamically registering the sysctl seems in order. Else, I'm planning to actually remove machdep.hwpstate_pkg_ctrl completely (including from the Intel driver), so we could as well live with the warning for a while.

Oh, I see you've posted D54528 in the meantime, that's fine.