RE: [PATCH v5 10/18] xen/cpufreq: introduce a new amd cppc driver for cpufreq scaling

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

> -----Original Message-----
> From: Penny, Zheng
> Sent: Wednesday, July 2, 2025 5:49 PM
> To: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> Subject: RE: [PATCH v5 10/18] xen/cpufreq: introduce a new amd cppc driver for
> cpufreq scaling
>
>
>
> > -----Original Message-----
> > From: Jan Beulich <jbeulich@xxxxxxxx>
> > Sent: Tuesday, June 17, 2025 12:00 AM
> > To: Penny, Zheng <penny.zheng@xxxxxxx>
> > Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andrew Cooper
> > <andrew.cooper3@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>;
> > xen- devel@xxxxxxxxxxxxxxxxxxxx
> > Subject: Re: [PATCH v5 10/18] xen/cpufreq: introduce a new amd cppc
> > driver for cpufreq scaling
> >
> > On 27.05.2025 10:48, Penny Zheng wrote:
> > > --- a/xen/arch/x86/acpi/cpufreq/amd-cppc.c
> > > +++ b/xen/arch/x86/acpi/cpufreq/amd-cppc.c
> > > +        /*
> > > +         * We don't need to convert to kHz for computing offset and can
> > > +         * directly use nominal_mhz and lowest_mhz as the division
> > > +         * will remove the frequency unit.
> > > +         */
> > > +        offset = data->caps.nominal_perf -
> > > +                 (mul * cppc_data->cpc.nominal_mhz) / div;
> > > +    }
> > > +    else
> > > +    {
> > > +        /* Read Processor Max Speed(MHz) as anchor point */
> > > +        mul = data->caps.highest_perf;
> > > +        div = this_cpu(pxfreq_mhz);
> > > +        if ( !div )
> > > +            return -EINVAL;
> >
> > What's wrong about the function arguments in this case? (Same question
> > again on further uses of EINVAL below.)
> >
>
> If we could not get processor max frequency, the whole function is useless...
> Maybe -EOPNOTSUPP is more suitable than -EINVAL;
>
> > > +static int cf_check amd_cppc_cpufreq_target(struct cpufreq_policy 
> > > *policy,
> > > +                                            unsigned int target_freq,
> > > +                                            unsigned int relation) {
> > > +    unsigned int cpu = policy->cpu;
> > > +    const struct amd_cppc_drv_data *data = per_cpu(amd_cppc_drv_data,
> cpu);
> > > +    uint8_t des_perf;
> > > +    int res;
> > > +
> > > +    if ( unlikely(!target_freq) )
> > > +        return 0;
> > > +
> > > +    res = amd_cppc_khz_to_perf(data, target_freq, &des_perf);
> > > +    if ( res )
> > > +        return res;
> > > +
> > > +    /*
> > > +     * Setting with "lowest_nonlinear_perf" to ensure governoring
> > > +     * performance in P-state range.
> > > +     */
> > > +    amd_cppc_write_request(policy->cpu, data->caps.lowest_nonlinear_perf,
> > > +                           des_perf, data->caps.highest_perf);
> >
> > I fear I don't understand the comment, and hence it remains unclear to
> > me why lowest_nonlinear_perf is being used here.
> >
>
> How about
> ```
> Choose lowest nonlinear performance as the minimum performance level at which
> the platform may run.
> Lowest nonlinear performance is the lowest performance level at which 
> nonlinear
> power savings are achieved, Above this threshold, lower performance levels 
> should
> be generally more energy efficient than higher performance levels.
> ```

I'm adding a few more explanation on highest performance too
```
Choose lowest nonlinear performance as the minimum performance level, and 
highest performance as the maximum performance level.
Lowest nonlinear performance is the lowest performance level at which nonlinear 
power savings are achieved, Above this threshold, lower
performance levels should be generally more energy efficient than higher 
performance levels.
And highest performance is the absolute maximum performance an individual 
processor may reach, assuming ideal conditions.
```
> >
> > Jan