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

[Mario Limonciello <mario.limonciello@xxxxxxx>]

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

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