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

["zhenglifeng (A)" <zhenglifeng1@huawei.com>]

On 2025/1/15 2:24, Rafael J. Wysocki wrote:

> 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.

Will mention it. Thanks.

> 
>> 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.

Yes, good point. Thanks.

> 
>> 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.

Will add. Thanks.

> 
>> + */
>> +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?

Actually, after the while loop above, auto_act_window is not larger than
CPPC_AUTO_ACT_WINDOW_SIG_CARRY_THRESH, which is 129. So this if condition
is valid only when auto_act_window is 128 or 129. Since we only have 7 bits
to store this value, 128 and 129 can only be cut to 127.

> 
>> +
>> +       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
>>
>>
>