Re: [PATCH v4 5/6] ACPI: CPPC: Add autonomous selection ABIs

Rafael J. Wysocki
2025-01-14T18:25:11Z

On Mon, Jan 13, 2025 at 1:21 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:

This should mention the specification revision and section number(s)
for the specification material the code is based on.

> cppc_set_epp - write energy performance preference register value
>
> cppc_get_auto_act_window - read autonomous activity window register value
>
> cppc_set_auto_act_window - write autonomous activity window register value
>
> cppc_get_auto_sel - read autonomous selection enable register value,
> modified from cppc_get_auto_sel_caps()

It would be better to move the modification part into a separate patch.

> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> ---
> drivers/acpi/cppc_acpi.c | 82 ++++++++++++++++++++++++++++++++----
> drivers/cpufreq/amd-pstate.c | 3 +-
> include/acpi/cppc_acpi.h | 30 +++++++++++--
> 3 files changed, 103 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 03134613311d..7bfe30f7b40f 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -1568,23 +1568,89 @@ int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable)
> EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
>
> /**
> - * cppc_get_auto_sel_caps - Read autonomous selection register.
> - * @cpunum : CPU from which to read register.
> - * @perf_caps : struct where autonomous selection register value is updated.
> + * cppc_set_epp() - Write the EPP register.
> + * @cpu: CPU on which to write register.
> + * @epp_val: Value to write to the EPP register.
> */
> -int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps)
> +int cppc_set_epp(int cpu, u64 epp_val)
> {
> - u64 auto_sel;
> + if (epp_val > CPPC_ENERGY_PERF_MAX)
> + return -EINVAL;
> +
> + return cppc_set_reg_val(cpu, ENERGY_PERF, epp_val);
> +}
> +EXPORT_SYMBOL_GPL(cppc_set_epp);
> +
> +/**
> + * cppc_get_auto_act_window() - Read autonomous activity window register.
> + * @cpu: CPU from which to read register.
> + * @auto_act_window: Return address.

It would be good to describe the autonomous activity window encoding.

> + */
> +int cppc_get_auto_act_window(int cpu, u64 *auto_act_window)
> +{
> + unsigned int exp;
> + u64 val, sig;
> + int ret;
> +
> + ret = cppc_get_reg_val(cpu, AUTO_ACT_WINDOW, &val);
> + if (ret)
> + return ret;
> +
> + sig = val & CPPC_AUTO_ACT_WINDOW_MAX_SIG;
> + exp = (val >> CPPC_AUTO_ACT_WINDOW_SIG_BIT_SIZE) & CPPC_AUTO_ACT_WINDOW_MAX_EXP;
> + *auto_act_window = sig * int_pow(10, exp);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(cppc_get_auto_act_window);
> +
> +/**
> + * cppc_set_auto_act_window() - Write autonomous activity window register.
> + * @cpu: CPU on which to write register.
> + * @auto_act_window: usec value to write to the autonomous activity window register.
> + */
> +int cppc_set_auto_act_window(int cpu, u64 auto_act_window)
> +{
> + u64 max_val = CPPC_AUTO_ACT_WINDOW_MAX_SIG * int_pow(10, CPPC_AUTO_ACT_WINDOW_MAX_EXP);
> + int digits = 0;
> + u64 val;
> +
> + if (auto_act_window > max_val)
> + return -EINVAL;
> +
> + while (auto_act_window > CPPC_AUTO_ACT_WINDOW_SIG_CARRY_THRESH) {
> + auto_act_window /= 10;
> + digits += 1;
> + }
> +
> + if (auto_act_window > CPPC_AUTO_ACT_WINDOW_MAX_SIG)
> + auto_act_window = CPPC_AUTO_ACT_WINDOW_MAX_SIG;

It looks like this may clobber the most significant bit, or am I mistaken?

> +
> + val = (digits << CPPC_AUTO_ACT_WINDOW_SIG_BIT_SIZE) + auto_act_window;
> +
> + return cppc_set_reg_val(cpu, AUTO_ACT_WINDOW, val);
> +}
> +EXPORT_SYMBOL_GPL(cppc_set_auto_act_window);
> +
> +/**
> + * cppc_get_auto_sel() - Read autonomous selection register.
> + * @cpu: CPU from which to read register.
> + * @enable: Return address.
> + */
> +int cppc_get_auto_sel(int cpu, bool *enable)
> +{
> + u64 val;
> int ret;
>
> - ret = cppc_get_reg_val(cpunum, AUTO_SEL_ENABLE, &auto_sel);
> + ret = cppc_get_reg_val(cpu, AUTO_SEL_ENABLE, &val);
> if (ret)
> return ret;
>
> - perf_caps->auto_sel = (bool)auto_sel;
> + *enable = (bool)val;
> +
> return 0;
> }
> -EXPORT_SYMBOL_GPL(cppc_get_auto_sel_caps);
> +EXPORT_SYMBOL_GPL(cppc_get_auto_sel);
>
> /**
> * cppc_set_auto_sel - Write autonomous selection register.
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 66e5dfc711c0..8bc11d0618f8 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -399,6 +399,7 @@ static int shmem_init_perf(struct amd_cpudata *cpudata)
> {
> struct cppc_perf_caps cppc_perf;
> u64 numerator;
> + bool auto_sel;
>
> int ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
> if (ret)
> @@ -420,7 +421,7 @@ static int shmem_init_perf(struct amd_cpudata *cpudata)
> if (cppc_state == AMD_PSTATE_ACTIVE)
> return 0;
>
> - ret = cppc_get_auto_sel_caps(cpudata->cpu, &cppc_perf);
> + ret = cppc_get_auto_sel(cpudata->cpu, &auto_sel);
> if (ret) {
> pr_warn("failed to get auto_sel, ret: %d\n", ret);
> return 0;
> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
> index 62d368bcd9ec..325e9543e08f 100644
> --- a/include/acpi/cppc_acpi.h
> +++ b/include/acpi/cppc_acpi.h
> @@ -32,6 +32,15 @@
> #define CMD_READ 0
> #define CMD_WRITE 1
>
> +#define CPPC_AUTO_ACT_WINDOW_SIG_BIT_SIZE (7)
> +#define CPPC_AUTO_ACT_WINDOW_EXP_BIT_SIZE (3)
> +#define CPPC_AUTO_ACT_WINDOW_MAX_SIG ((1 << CPPC_AUTO_ACT_WINDOW_SIG_BIT_SIZE) - 1)
> +#define CPPC_AUTO_ACT_WINDOW_MAX_EXP ((1 << CPPC_AUTO_ACT_WINDOW_EXP_BIT_SIZE) - 1)
> +/* CPPC_AUTO_ACT_WINDOW_MAX_SIG is 127, so 128 and 129 will decay to 127 when writing */
> +#define CPPC_AUTO_ACT_WINDOW_SIG_CARRY_THRESH 129
> +
> +#define CPPC_ENERGY_PERF_MAX (0xFF)
> +
> /* Each register has the folowing format. */
> struct cpc_reg {
> u8 descriptor;
> @@ -159,7 +168,10 @@ extern int cpc_read_ffh(int cpunum, struct cpc_reg *reg, u64 *val);
> extern int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val);
> extern int cppc_get_epp_perf(int cpunum, u64 *epp_perf);
> extern int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable);
> -extern int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps);
> +extern int cppc_set_epp(int cpu, u64 epp_val);
> +extern int cppc_get_auto_act_window(int cpu, u64 *auto_act_window);
> +extern int cppc_set_auto_act_window(int cpu, u64 auto_act_window);
> +extern int cppc_get_auto_sel(int cpu, bool *enable);
> extern int cppc_set_auto_sel(int cpu, bool enable);
> extern int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf);
> extern int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator);
> @@ -229,11 +241,23 @@ static inline int cppc_get_epp_perf(int cpunum, u64 *epp_perf)
> {
> return -EOPNOTSUPP;
> }
> -static inline int cppc_set_auto_sel(int cpu, bool enable)
> +static inline int cppc_set_epp(int cpu, u64 epp_val)
> {
> return -EOPNOTSUPP;
> }
> -static inline int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps)
> +static inline int cppc_get_auto_act_window(int cpu, u64 *auto_act_window)
> +{
> + return -EOPNOTSUPP;
> +}
> +static inline int cppc_set_auto_act_window(int cpu, u64 auto_act_window)
> +{
> + return -EOPNOTSUPP;
> +}
> +static inline int cppc_get_auto_sel(int cpu, bool *enable)
> +{
> + return -EOPNOTSUPP;
> +}
> +static inline int cppc_set_auto_sel(int cpu, bool enable)
> {
> return -EOPNOTSUPP;
> }
> --
> 2.33.0
>
>