[v3,3/4] ACPI: CPPC: Add autonomous selection ABIs

cppc_set_epp - write energy performance preference register

cppc_get_auto_act_window - read autonomous activity window register

cppc_set_auto_act_window - write autonomous activity window register

cppc_get_auto_sel - read autonomous selection enable register

Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
---
 drivers/acpi/cppc_acpi.c | 44 ++++++++++++++++++++++++++++++++++++++++
 include/acpi/cppc_acpi.h | 20 ++++++++++++++++++
 2 files changed, 64 insertions(+)

Hello Lifeng, Huang, Gautham, Mario,

On 12/16/24 10:16, Lifeng Zheng wrote:
> cppc_set_epp - write energy performance preference register
> 
> cppc_get_auto_act_window - read autonomous activity window register
> 
> cppc_set_auto_act_window - write autonomous activity window register
> 
> cppc_get_auto_sel - read autonomous selection enable register
> 
> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> ---
>   drivers/acpi/cppc_acpi.c | 44 ++++++++++++++++++++++++++++++++++++++++
>   include/acpi/cppc_acpi.h | 20 ++++++++++++++++++
>   2 files changed, 64 insertions(+)
> 
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 83c7fcad74ad..645f2366c888 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -1595,6 +1595,50 @@ int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable)
>   }
>   EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
>   
> +/**
> + * 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_set_epp(int cpu, u64 epp_val)
> +{
> +	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.
> + */
> +int cppc_get_auto_act_window(int cpu, u64 *auto_act_window)

As there is only one way to interpret the value of the
'Autonomous Activity Window Register', maybe the logic to convert
from/to the register value to a value in us should be placed here
rather than in the cppc_cpufreq driver.
Meaning, maybe the prototype should be:

int cppc_get_auto_act_window(int cpu, unsigned int *auto_act_window);

Similar remark for cppc_set_epp() and other functions.

> +{
> +	return cppc_get_reg_val(cpu, AUTO_ACT_WINDOW, auto_act_window);
> +}
> +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: Value to write to the autonomous activity window register.
> + */
> +int cppc_set_auto_act_window(int cpu, u64 auto_act_window)
> +{
> +	return cppc_set_reg_val(cpu, AUTO_ACT_WINDOW, auto_act_window);
> +}
> +EXPORT_SYMBOL_GPL(cppc_set_auto_act_window);
> +
> +/**
> + * cppc_get_auto_sel() - Read autonomous selection register.
> + * @cpu: CPU from which to read register.
> + * @auto_sel: Return address.
> + */
> +int cppc_get_auto_sel(int cpu, u64 *auto_sel)

Similarly, maybe it would be better to use:
int cppc_get_auto_sel(int cpu, bool *auto_sel);

> +{
> +	return cppc_get_reg_val(cpu, AUTO_SEL_ENABLE, auto_sel);
> +}
> +EXPORT_SYMBOL_GPL(cppc_get_auto_sel);
> +
>   /**
>    * cppc_get_auto_sel_caps - Read autonomous selection register.
>    * @cpunum : CPU from which to read register.
> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
> index 62d368bcd9ec..134931b081a0 100644
> --- a/include/acpi/cppc_acpi.h
> +++ b/include/acpi/cppc_acpi.h
> @@ -159,6 +159,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_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, u64 *auto_sel);
>   extern int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps);

This is a bit annoying, but maybe only one function between:
- cppc_get_auto_sel_caps()
- cppc_get_auto_sel()
is necessary.

I added the owners of the amd-pstate driver to ask if this would
be ok to replace cppc_get_auto_sel_caps() by cppc_get_auto_sel().

>   extern int cppc_set_auto_sel(int cpu, bool enable);
>   extern int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf);
> @@ -225,6 +229,22 @@ static inline int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls,
>   {
>   	return -EOPNOTSUPP;
>   }
> +static inline int cppc_set_epp(int cpu, u64 epp_val)
> +{
> +	return -EOPNOTSUPP;
> +}
> +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, u64 *auto_sel)
> +{
> +	return -EOPNOTSUPP;
> +}
>   static inline int cppc_get_epp_perf(int cpunum, u64 *epp_perf)
>   {
>   	return -EOPNOTSUPP;

Hello Pierre,

On 2024/12/17 21:48, Pierre Gondois wrote:
> Hello Lifeng, Huang, Gautham, Mario,
> 
> On 12/16/24 10:16, Lifeng Zheng wrote:
>> cppc_set_epp - write energy performance preference register
>>
>> cppc_get_auto_act_window - read autonomous activity window register
>>
>> cppc_set_auto_act_window - write autonomous activity window register
>>
>> cppc_get_auto_sel - read autonomous selection enable register
>>
>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
>> ---
>>   drivers/acpi/cppc_acpi.c | 44 ++++++++++++++++++++++++++++++++++++++++
>>   include/acpi/cppc_acpi.h | 20 ++++++++++++++++++
>>   2 files changed, 64 insertions(+)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index 83c7fcad74ad..645f2366c888 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -1595,6 +1595,50 @@ int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable)
>>   }
>>   EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
>>   +/**
>> + * 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_set_epp(int cpu, u64 epp_val)
>> +{
>> +    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.
>> + */
>> +int cppc_get_auto_act_window(int cpu, u64 *auto_act_window)
> 
> As there is only one way to interpret the value of the
> 'Autonomous Activity Window Register', maybe the logic to convert
> from/to the register value to a value in us should be placed here
> rather than in the cppc_cpufreq driver.
> Meaning, maybe the prototype should be:
> 
> int cppc_get_auto_act_window(int cpu, unsigned int *auto_act_window);
> 
> Similar remark for cppc_set_epp() and other functions.

Good point. Will improve it. Thanks.

> 
>> +{
>> +    return cppc_get_reg_val(cpu, AUTO_ACT_WINDOW, auto_act_window);
>> +}
>> +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: Value to write to the autonomous activity window register.
>> + */
>> +int cppc_set_auto_act_window(int cpu, u64 auto_act_window)
>> +{
>> +    return cppc_set_reg_val(cpu, AUTO_ACT_WINDOW, auto_act_window);
>> +}
>> +EXPORT_SYMBOL_GPL(cppc_set_auto_act_window);
>> +
>> +/**
>> + * cppc_get_auto_sel() - Read autonomous selection register.
>> + * @cpu: CPU from which to read register.
>> + * @auto_sel: Return address.
>> + */
>> +int cppc_get_auto_sel(int cpu, u64 *auto_sel)
> 
> Similarly, maybe it would be better to use:
> int cppc_get_auto_sel(int cpu, bool *auto_sel);

Good point the same. Thanks.

> 
>> +{
>> +    return cppc_get_reg_val(cpu, AUTO_SEL_ENABLE, auto_sel);
>> +}
>> +EXPORT_SYMBOL_GPL(cppc_get_auto_sel);
>> +
>>   /**
>>    * cppc_get_auto_sel_caps - Read autonomous selection register.
>>    * @cpunum : CPU from which to read register.
>> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
>> index 62d368bcd9ec..134931b081a0 100644
>> --- a/include/acpi/cppc_acpi.h
>> +++ b/include/acpi/cppc_acpi.h
>> @@ -159,6 +159,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_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, u64 *auto_sel);
>>   extern int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps);
> 
> This is a bit annoying, but maybe only one function between:
> - cppc_get_auto_sel_caps()
> - cppc_get_auto_sel()
> is necessary.
> 
> I added the owners of the amd-pstate driver to ask if this would
> be ok to replace cppc_get_auto_sel_caps() by cppc_get_auto_sel().

Really nice. Thanks.

> 
>>   extern int cppc_set_auto_sel(int cpu, bool enable);
>>   extern int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf);
>> @@ -225,6 +229,22 @@ static inline int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls,
>>   {
>>       return -EOPNOTSUPP;
>>   }
>> +static inline int cppc_set_epp(int cpu, u64 epp_val)
>> +{
>> +    return -EOPNOTSUPP;
>> +}
>> +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, u64 *auto_sel)
>> +{
>> +    return -EOPNOTSUPP;
>> +}
>>   static inline int cppc_get_epp_perf(int cpunum, u64 *epp_perf)
>>   {
>>       return -EOPNOTSUPP;
>

Hello Pierre, Mario, Gautham, Huang

On 2024/12/18 4:38, Mario Limonciello wrote:
> On 12/17/2024 07:48, Pierre Gondois wrote:
>> Hello Lifeng, Huang, Gautham, Mario,
>>
>> On 12/16/24 10:16, Lifeng Zheng wrote:
>>> cppc_set_epp - write energy performance preference register
>>>
>>> cppc_get_auto_act_window - read autonomous activity window register
>>>
>>> cppc_set_auto_act_window - write autonomous activity window register
>>>
>>> cppc_get_auto_sel - read autonomous selection enable register
>>>
>>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
>>> ---
>>>   drivers/acpi/cppc_acpi.c | 44 ++++++++++++++++++++++++++++++++++++++++
>>>   include/acpi/cppc_acpi.h | 20 ++++++++++++++++++
>>>   2 files changed, 64 insertions(+)
>>>
>>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>>> index 83c7fcad74ad..645f2366c888 100644
>>> --- a/drivers/acpi/cppc_acpi.c
>>> +++ b/drivers/acpi/cppc_acpi.c
>>> @@ -1595,6 +1595,50 @@ int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable)
>>>   }
>>>   EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
>>> +/**
>>> + * 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_set_epp(int cpu, u64 epp_val)
>>> +{
>>> +    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.
>>> + */
>>> +int cppc_get_auto_act_window(int cpu, u64 *auto_act_window)
>>
>> As there is only one way to interpret the value of the
>> 'Autonomous Activity Window Register', maybe the logic to convert
>> from/to the register value to a value in us should be placed here
>> rather than in the cppc_cpufreq driver.
>> Meaning, maybe the prototype should be:
>>
>> int cppc_get_auto_act_window(int cpu, unsigned int *auto_act_window);
>>
>> Similar remark for cppc_set_epp() and other functions.
>>
>>> +{
>>> +    return cppc_get_reg_val(cpu, AUTO_ACT_WINDOW, auto_act_window);
>>> +}
>>> +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: Value to write to the autonomous activity window register.
>>> + */
>>> +int cppc_set_auto_act_window(int cpu, u64 auto_act_window)
>>> +{
>>> +    return cppc_set_reg_val(cpu, AUTO_ACT_WINDOW, auto_act_window);
>>> +}
>>> +EXPORT_SYMBOL_GPL(cppc_set_auto_act_window);
>>> +
>>> +/**
>>> + * cppc_get_auto_sel() - Read autonomous selection register.
>>> + * @cpu: CPU from which to read register.
>>> + * @auto_sel: Return address.
>>> + */
>>> +int cppc_get_auto_sel(int cpu, u64 *auto_sel)
>>
>> Similarly, maybe it would be better to use:
>> int cppc_get_auto_sel(int cpu, bool *auto_sel);
>>
>>> +{
>>> +    return cppc_get_reg_val(cpu, AUTO_SEL_ENABLE, auto_sel);
>>> +}
>>> +EXPORT_SYMBOL_GPL(cppc_get_auto_sel);
>>> +
>>>   /**
>>>    * cppc_get_auto_sel_caps - Read autonomous selection register.
>>>    * @cpunum : CPU from which to read register.
>>> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
>>> index 62d368bcd9ec..134931b081a0 100644
>>> --- a/include/acpi/cppc_acpi.h
>>> +++ b/include/acpi/cppc_acpi.h
>>> @@ -159,6 +159,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_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, u64 *auto_sel);
>>>   extern int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps);
>>
>> This is a bit annoying, but maybe only one function between:
>> - cppc_get_auto_sel_caps()
>> - cppc_get_auto_sel()
>> is necessary.
>>
>> I added the owners of the amd-pstate driver to ask if this would
>> be ok to replace cppc_get_auto_sel_caps() by cppc_get_auto_sel().
> 
> Yeah I have no concerns with this if that's the direction this patch series goes.  Feel free to change amd-pstate in the patch that introduces cppc_get_auto_sel().
> 
> I'll be out around the US holiday, so I might not be able to review it for a while, but CC Gautham on the series and he may be able to.

After checking, it turns out that the only place uses
cppc_get_auto_sel_caps() only check the ret but never uses the value of
perf_caps. I believe cppc_get_auto_sel() will meet the requirements.

> 
>>
>>>   extern int cppc_set_auto_sel(int cpu, bool enable);
>>>   extern int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf);
>>> @@ -225,6 +229,22 @@ static inline int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls,
>>>   {
>>>       return -EOPNOTSUPP;
>>>   }
>>> +static inline int cppc_set_epp(int cpu, u64 epp_val)
>>> +{
>>> +    return -EOPNOTSUPP;
>>> +}
>>> +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, u64 *auto_sel)
>>> +{
>>> +    return -EOPNOTSUPP;
>>> +}
>>>   static inline int cppc_get_epp_perf(int cpunum, u64 *epp_perf)
>>>   {
>>>       return -EOPNOTSUPP;
>