Maybe a top-post will get attention....
Yet another ping; this was first submitted on 20 July, and has received
no comments. It has now been a month and other architectures are starting
to use CPPC so they will run into the same errors that this fixes. Can
I get an ACK, NAK, or further instructions, please?
Also adding Rafael on the ACPI side, just in case, since he's also reviewing
the Intel patches on the linux-acpi mailing list that are adding CPPC usage.
On 08/11/2016 12:15 PM, Al Stone wrote:
> On 08/01/2016 02:31 PM, Viresh Kumar wrote:
>> [+ Ashwin's new email id..]
>>
>> On 20-07-16, 15:10, Al Stone wrote:
>>> When CPPC is being used by ACPI on arm64, user space tools such as
>>> cpupower report CPU frequency values from sysfs that are incorrect.
>>>
>>> What the driver was doing was reporting the values given by ACPI tables
>>> in whatever scale was used to provide them. However, the ACPI spec
>>> defines the CPPC values as unitless abstract numbers. Internal kernel
>>> structures such as struct perf_cap, in contrast, expect these values
>>> to be in KHz. When these struct values get reported via sysfs, the
>>> user space tools also assume they are in KHz, causing them to report
>>> incorrect values (for example, reporting a CPU frequency of 1MHz when
>>> it should be 1.8GHz).
>>>
>>> The downside is that this approach has some assumptions:
>>>
>>> (1) It relies on SMBIOS3 being used, *and* that the Max Frequency
>>> value for a processor is set to a non-zero value.
>>>
>>> (2) It assumes that all processors run at the same speed, or that
>>> the CPPC values have all been scaled to reflect relative speed.
>>> This patch retrieves the largest CPU Max Frequency from a type 4 DMI
>>> record that it can find. This may not be an issue, however, as a
>>> sampling of DMI data on x86 and arm64 indicates there is often only
>>> one such record regardless. Since CPPC is relatively new, it is
>>> unclear if the ACPI ASL will always be written to reflect any sort
>>> of relative performance of processors of differing speeds.
>>>
>>> (3) It assumes that performance and frequency both scale linearly.
>>>
>>> For arm64 servers, this may be sufficient, but it does rely on
>>> firmware values being set correctly. Hence, other approaches will
>>> be considered in the future.
>>>
>>> This has been tested on three arm64 servers, with and without DMI, with
>>> and without CPPC support.
>>>
>>> Changes for v5:
>>> -- Move code to cpufreq/cppc_cpufreq.c from acpi/cppc_acpi.c to keep
>>> frequency-related code together, and keep the CPPC abstract scale
>>> in ACPI (Prashanth Prakash)
>>> -- Fix the scaling to remove the incorrect assumption that frequency
>>> was always a range from zero to max; as a practical matter, it is
>>> not (Prasanth Prakash); this also allowed us to remove an over-
>>> engineered function to do this math.
>>>
>>> Changes for v4:
>>> -- Replaced magic constants with #defines (Rafael Wysocki)
>>> -- Renamed cppc_unitless_to_khz() to cppc_to_khz() (Rafael Wysocki)
>>> -- Replaced hidden initialization with a clearer form (Rafael Wysocki)
>>> -- Instead of picking up the first Max Speed value from DMI, we will
>>> now get the largest Max Speed; still an approximation, but slightly
>>> less subject to error (Rafael Wysocki)
>>> -- Kconfig for cppc_cpufreq now depends on DMI, instead of selecting
>>> it, in order to make sure DMI is set up properly (Rafael Wysocki)
>>>
>>> Changes for v3:
>>> -- Added clarifying commentary re short-term vs long-term fix (Alexey
>>> Klimov)
>>> -- Added range checking code to ensure proper arithmetic occurs,
>>> especially no division by zero (Alexey Klimov)
>>>
>>> Changes for v2:
>>> -- Corrected thinko: needed to have DEPENDS on DMI in Kconfig.arm,
>>> not SELECT DMI (found by build daemon)
>>>
>>> Signed-off-by: Al Stone <a...@redhat.com>
>>> Signed-off-by: Prashanth Prakash <pprak...@codeaurora.org>
>>> ---
>>> drivers/cpufreq/cppc_cpufreq.c | 53
>>> ++++++++++++++++++++++++++++++++++++++----
>>> 1 file changed, 49 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>>> index 8882b8e..6debc18 100644
>>> --- a/drivers/cpufreq/cppc_cpufreq.c
>>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>>> @@ -19,10 +19,19 @@
>>> #include <linux/delay.h>
>>> #include <linux/cpu.h>
>>> #include <linux/cpufreq.h>
>>> +#include <linux/dmi.h>
>>> #include <linux/vmalloc.h>
>>>
>>> +#include <asm/unaligned.h>
>>> +
>>> #include <acpi/cppc_acpi.h>
>>>
>>> +/* Minimum struct length needed for the DMI processor entry we want */
>>> +#define DMI_ENTRY_PROCESSOR_MIN_LENGTH 48
>>> +
>>> +/* Offest in the DMI processor structure for the max frequency */
>>> +#define DMI_PROCESSOR_MAX_SPEED 0x14
>>> +
>>> /*
>>> * These structs contain information parsed from per CPU
>>> * ACPI _CPC structures.
>>> @@ -32,6 +41,39 @@
>>> */
>>> static struct cpudata **all_cpu_data;
>>>
>>> +/* Capture the max KHz from DMI */
>>> +static u64 cppc_dmi_max_khz;
>>> +
>>> +/* Callback function used to retrieve the max frequency from DMI */
>>> +static void cppc_find_dmi_mhz(const struct dmi_header *dm, void *private)
>>> +{
>>> + const u8 *dmi_data = (const u8 *)dm;
>>> + u16 *mhz = (u16 *)private;
>>> +
>>> + if (dm->type == DMI_ENTRY_PROCESSOR &&
>>> + dm->length >= DMI_ENTRY_PROCESSOR_MIN_LENGTH) {
>>> + u16 val = (u16)get_unaligned((const u16 *)
>>> + (dmi_data + DMI_PROCESSOR_MAX_SPEED));
>>> + *mhz = val > *mhz ? val : *mhz;
>>> + }
>>> +}
>>> +
>>> +/* Look up the max frequency in DMI */
>>> +static u64 cppc_get_dmi_max_khz(void)
>>> +{
>>> + u16 mhz = 0;
>>> +
>>> + dmi_walk(cppc_find_dmi_mhz, &mhz);
>>> +
>>> + /*
>>> + * Real stupid fallback value, just in case there is no
>>> + * actual value set.
>>> + */
>>> + mhz = mhz ? mhz : 1;
>>> +
>>> + return (1000 * mhz);
>>> +}
>>> +
>>> static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
>>> unsigned int target_freq,
>>> unsigned int relation)
>>> @@ -42,7 +84,7 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy
>>> *policy,
>>>
>>> cpu = all_cpu_data[policy->cpu];
>>>
>>> - cpu->perf_ctrls.desired_perf = target_freq;
>>> + cpu->perf_ctrls.desired_perf = target_freq * policy->max /
>>> cppc_dmi_max_khz;
>>> freqs.old = policy->cur;
>>> freqs.new = target_freq;
>>>
>>> @@ -94,8 +136,10 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy
>>> *policy)
>>> return ret;
>>> }
>>>
>>> - policy->min = cpu->perf_caps.lowest_perf;
>>> - policy->max = cpu->perf_caps.highest_perf;
>>> + cppc_dmi_max_khz = cppc_get_dmi_max_khz();
>>> +
>>> + policy->min = cpu->perf_caps.lowest_perf * cppc_dmi_max_khz /
>>> cpu->perf_caps.highest_perf;
>>> + policy->max = cppc_dmi_max_khz;
>>> policy->cpuinfo.min_freq = policy->min;
>>> policy->cpuinfo.max_freq = policy->max;
>>> policy->shared_type = cpu->shared_type;
>>> @@ -112,7 +156,8 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy
>>> *policy)
>>> cpu->cur_policy = policy;
>>>
>>> /* Set policy->cur to max now. The governors will adjust later. */
>>> - policy->cur = cpu->perf_ctrls.desired_perf =
>>> cpu->perf_caps.highest_perf;
>>> + policy->cur = cppc_dmi_max_khz;
>>> + cpu->perf_ctrls.desired_perf = cpu->perf_caps.highest_perf;
>>>
>>> ret = cppc_set_perf(cpu_num, &cpu->perf_ctrls);
>>> if (ret)
>>> --
>>> 2.7.4
>>
>
> Another gentle ping -- any comments? Can this get pulled in now?
>
> Thanks.
>
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
-----------------------------------
