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: 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.
```
>
> Jan