Hi George,
Few comments on your patch:
On Fri, Jun 15, 2018 at 03:03:15AM -0700, George Cherian wrote:
> Per Section 8.4.7.1.3 of ACPI 6.2, The platform provides performance
> feedback via set of performance counters. To determine the actual
> performance level delivered over time, OSPM may read a set of
> performance counters from the Reference Performance Counter Register
> and the Delivered Performance Counter Register.
>
> OSPM calculates the delivered performance over a given time period by
> taking a beginning and ending snapshot of both the reference and
> delivered performance counters, and calculating:
>
> delivered_perf = reference_perf X (delta of delivered_perf counter / delta of
> reference_perf counter).
>
> Implement the above and hook this to the cpufreq->get method.
>
> Signed-off-by: George Cherian <george.cher...@cavium.com>
> Acked-by: Viresh Kumar <viresh.ku...@linaro.org>
> ---
> drivers/cpufreq/cppc_cpufreq.c | 71
> ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 71 insertions(+)
>
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index 3464580..3fe7625 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -296,10 +296,81 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy
> *policy)
> return ret;
> }
>
> +static int cppc_get_rate_from_fbctrs(struct cppc_cpudata *cpu,
> + struct cppc_perf_fb_ctrs fb_ctrs_t0,
> + struct cppc_perf_fb_ctrs fb_ctrs_t1)
> +{
> + u64 delta_reference, delta_delivered;
> + u64 reference_perf, delivered_perf;
> +
> + reference_perf = fb_ctrs_t0.reference_perf;
> + if (fb_ctrs_t1.reference > fb_ctrs_t0.reference) {
> + delta_reference = fb_ctrs_t1.reference - fb_ctrs_t0.reference;
> + } else {
> + /*
> + * Counters would have wrapped-around
> + * We also need to find whether the low level fw
> + * maintains 32 bit or 64 bit counters, to calculate
> + * the correct delta.
> + */
> + if (fb_ctrs_t0.reference > (~(u32)0))
> + delta_reference = (~((u64)0) - fb_ctrs_t0.reference) +
> + fb_ctrs_t1.reference;
> + else
> + delta_reference = (~((u32)0) - fb_ctrs_t0.reference) +
> + fb_ctrs_t1.reference;
> + }
> +
> + if (fb_ctrs_t1.delivered > fb_ctrs_t0.delivered) {
> + delta_delivered = fb_ctrs_t1.delivered - fb_ctrs_t0.delivered;
> + } else {
> + /*
> + * Counters would have wrapped-around
> + * We also need to find whether the low level fw
> + * maintains 32 bit or 64 bit counters, to calculate
> + * the correct delta.
> + */
> + if (fb_ctrs_t0.delivered > (~(u32)0))
> + delta_delivered = (~((u64)0) - fb_ctrs_t0.delivered) +
> + fb_ctrs_t1.delivered;
> + else
> + delta_delivered = (~((u32)0) - fb_ctrs_t0.delivered) +
> + fb_ctrs_t1.delivered;
> + }
Having this code repeated twice does not look great. Also the math here
is not correct, since (~0 - val2 + val1) is off by one. Because of
binary representation, unsigned subtraction will work even if
val2 < val1. So cleaner way would be to do:
static inline u64 ts_sub(u64 t1, u64 t0)
{
if (t1 > t0 || t0 > ~(u32)0)
return t1 - t0;
return (u32)t1 - (u32)t0;
}
And then use ts_sub in both places above.
JC.
