Re: [PATCH v2] Force cppc_cpufreq to report values in KHz to fix user space reporting

[Al Stone]

On 04/21/2016 11:30 PM, Viresh Kumar wrote:
> On 19-04-16, 16:12, Ashwin Chaugule wrote:
>> + Ryan
>>
>> Hi Al,
>>
>> On 18 April 2016 at 20:11, 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).
>>>
>>> While the investigation for a long term fix proceeds (several options
>>> are being explored, some of which may require spec changes or other
>>> much more invasive fixes), this patch forces the values read by CPPC
>>> to be read in KHz, regardless of what they actually represent.
>>>
>>> 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.  This
>>>patch retrieves the first 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.
> 
> Don't we have any big LITTLE ARM servers yet ? Or we will not have them at 
> all ?

My apologies, but I missed this question earlier and just now noticed
it.  AFAIK, there are no big.LITTLE ARM servers yet.  That doesn't mean
there aren't any, or that no one is planning one; I just don't know of
any.  I have been in discussions about doing that, but in the past those
have ended up concluding that there is probably no need for that level
of power management in a server.

-- 
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---

Re: [PATCH v2] Force cppc_cpufreq to report values in KHz to fix user space reporting

[Al Stone]

On 04/21/2016 11:30 PM, Viresh Kumar wrote:
> On 19-04-16, 16:12, Ashwin Chaugule wrote:
>> + Ryan
>>
>> Hi Al,
>>
>> On 18 April 2016 at 20:11, 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).
>>>
>>> While the investigation for a long term fix proceeds (several options
>>> are being explored, some of which may require spec changes or other
>>> much more invasive fixes), this patch forces the values read by CPPC
>>> to be read in KHz, regardless of what they actually represent.
>>>
>>> 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.  This
>>>patch retrieves the first 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.
> 
> Don't we have any big LITTLE ARM servers yet ? Or we will not have them at 
> all ?

My apologies, but I missed this question earlier and just now noticed
it.  AFAIK, there are no big.LITTLE ARM servers yet.  That doesn't mean
there aren't any, or that no one is planning one; I just don't know of
any.  I have been in discussions about doing that, but in the past those
have ended up concluding that there is probably no need for that level
of power management in a server.

-- 
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---

Re: [PATCH v2] Force cppc_cpufreq to report values in KHz to fix user space reporting

[rwells]

On 2016-04-19 18:53, Al Stone wrote:

On 04/19/2016 02:12 PM, Ashwin Chaugule wrote:

+ Ryan

Hi Al,

On 18 April 2016 at 20:11, 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).

While the investigation for a long term fix proceeds (several options
are being explored, some of which may require spec changes or other
much more invasive fixes), this patch forces the values read by CPPC
to be read in KHz, regardless of what they actually represent.

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.  This
   patch retrieves the first 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.

For arm64 servers, this may be sufficient, but it does rely on
firmware values being set correctly.  Hence, other approaches are
also being considered.

This has been tested on three arm64 servers, with and without DMI, 
with

and without CPPC support.

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 


This looks like a good short term solution. Does it make more sense to
move this to the cppc_cpufreq driver though? Since that ties more
closely into the cpufreq framework which requires the kHz values in
sysfs. That way we can keep the cppc_acpi.c shim compliant with the
ACPI spec. (i.e. values read in cppc structures remain abstract and
unitless).


Perhaps.  Doing it that way made the patch a bit messier since
cppc_acpi.c would set values that then had to be replaced in
cppc_cpufreq.c, so initialization looked odd to me; that's how
I ended up here.  You do raise a good point, however; I'll look
at that approach again since I could have missed an easier way
to do it.


Rafael, Viresh, others,

Any other ideas how to handle this better in the long term?

 - Decouple the cpufreq sysfs from the cppc driver and introduce its
own entries. Is it possibly to do this cleanly while still allowing
usage of cpufreq registration with existing governors?

 - Come up with a scaling factor using the PMU cycle counter at boot
before the CPPC drivers are initialized. This would use the current
freq set by some UEFI var. This would possibly require some messy
perfevents plumbing and added bootup time though.

- .. ?


Cheers,
Ashwin.



The other thought that occurs to me is to go back through the
perf_cap and cpufreq structs and make them more general -- perhaps
store the units being used and pointers to functions to convert them
to KHz.  This may require separating sysfs data for perf_cap from the
cpufreq sysfs data from the cppc sysfs data.  But, if units are then
reported out to sysfs, user space tools can do whatever conversions
they want, or at least know what they're reporting instead of there
being an implicit ABI between the kernel and the tools.  This would
be a far more invasive patch set, I think, but it still may be the
right thing to do for the long term.


The issue is a little more fundamental than that even.  We are 
retrofitting a performance management interface (CPPC) into a frequency 
management framework (cpufreq) and accompanying tools.  Regardless of 
what scheme we come up with for deriving/exposing frequency, we still 
haven’t completely solved the problem as that assumes a linear 
relationship between freq and performance.  This will work for many but 
not necessarily all CPPC systems.  In fact, making that assumption is 
explicitly forbidden in the ACPI spec: "OSPM must make no assumption 
about the exact meaning of the performance values presented by the 
platform, or how they may correlate to specific hardware metrics like 
processor frequency."


So to be completely consistent with the current spec, we would need to 
ween the tools off of frequency altogether and move to abstract 
performance - either specifically when CPPC driver is loaded or more 
generally.  If we think reporting frequency is required that might still 
be doable, but would need to be separate interface from the CPPC perf 
scale.  But I agree with Al 

Re: [PATCH v2] Force cppc_cpufreq to report values in KHz to fix user space reporting

[rwells]

On 2016-04-19 18:53, Al Stone wrote:

On 04/19/2016 02:12 PM, Ashwin Chaugule wrote:

+ Ryan

Hi Al,

On 18 April 2016 at 20:11, 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).

While the investigation for a long term fix proceeds (several options
are being explored, some of which may require spec changes or other
much more invasive fixes), this patch forces the values read by CPPC
to be read in KHz, regardless of what they actually represent.

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.  This
   patch retrieves the first 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.

For arm64 servers, this may be sufficient, but it does rely on
firmware values being set correctly.  Hence, other approaches are
also being considered.

This has been tested on three arm64 servers, with and without DMI, 
with

and without CPPC support.

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 


This looks like a good short term solution. Does it make more sense to
move this to the cppc_cpufreq driver though? Since that ties more
closely into the cpufreq framework which requires the kHz values in
sysfs. That way we can keep the cppc_acpi.c shim compliant with the
ACPI spec. (i.e. values read in cppc structures remain abstract and
unitless).


Perhaps.  Doing it that way made the patch a bit messier since
cppc_acpi.c would set values that then had to be replaced in
cppc_cpufreq.c, so initialization looked odd to me; that's how
I ended up here.  You do raise a good point, however; I'll look
at that approach again since I could have missed an easier way
to do it.


Rafael, Viresh, others,

Any other ideas how to handle this better in the long term?

 - Decouple the cpufreq sysfs from the cppc driver and introduce its
own entries. Is it possibly to do this cleanly while still allowing
usage of cpufreq registration with existing governors?

 - Come up with a scaling factor using the PMU cycle counter at boot
before the CPPC drivers are initialized. This would use the current
freq set by some UEFI var. This would possibly require some messy
perfevents plumbing and added bootup time though.

- .. ?


Cheers,
Ashwin.



The other thought that occurs to me is to go back through the
perf_cap and cpufreq structs and make them more general -- perhaps
store the units being used and pointers to functions to convert them
to KHz.  This may require separating sysfs data for perf_cap from the
cpufreq sysfs data from the cppc sysfs data.  But, if units are then
reported out to sysfs, user space tools can do whatever conversions
they want, or at least know what they're reporting instead of there
being an implicit ABI between the kernel and the tools.  This would
be a far more invasive patch set, I think, but it still may be the
right thing to do for the long term.


The issue is a little more fundamental than that even.  We are 
retrofitting a performance management interface (CPPC) into a frequency 
management framework (cpufreq) and accompanying tools.  Regardless of 
what scheme we come up with for deriving/exposing frequency, we still 
haven’t completely solved the problem as that assumes a linear 
relationship between freq and performance.  This will work for many but 
not necessarily all CPPC systems.  In fact, making that assumption is 
explicitly forbidden in the ACPI spec: "OSPM must make no assumption 
about the exact meaning of the performance values presented by the 
platform, or how they may correlate to specific hardware metrics like 
processor frequency."


So to be completely consistent with the current spec, we would need to 
ween the tools off of frequency altogether and move to abstract 
performance - either specifically when CPPC driver is loaded or more 
generally.  If we think reporting frequency is required that might still 
be doable, but would need to be separate interface from the CPPC perf 
scale.  But I agree with Al that is a more invasive change.


For 

Re: [PATCH v2] Force cppc_cpufreq to report values in KHz to fix user space reporting

[Rafael J. Wysocki]

On Tuesday, April 19, 2016 04:12:41 PM Ashwin Chaugule wrote:
> + Ryan
> 
> Hi Al,
> 
> On 18 April 2016 at 20:11, 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).
> >
> > While the investigation for a long term fix proceeds (several options
> > are being explored, some of which may require spec changes or other
> > much more invasive fixes), this patch forces the values read by CPPC
> > to be read in KHz, regardless of what they actually represent.
> >
> > 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.  This
> >patch retrieves the first 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.
> >
> > For arm64 servers, this may be sufficient, but it does rely on
> > firmware values being set correctly.  Hence, other approaches are
> > also being considered.
> >
> > This has been tested on three arm64 servers, with and without DMI, with
> > and without CPPC support.
> >
> > 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 
> 
> This looks like a good short term solution. Does it make more sense to
> move this to the cppc_cpufreq driver though? Since that ties more
> closely into the cpufreq framework which requires the kHz values in
> sysfs. That way we can keep the cppc_acpi.c shim compliant with the
> ACPI spec. (i.e. values read in cppc structures remain abstract and
> unitless).
> 
> Rafael, Viresh, others,
> 
> Any other ideas how to handle this better in the long term?
> 
>  - Decouple the cpufreq sysfs from the cppc driver and introduce its
> own entries. Is it possibly to do this cleanly while still allowing
> usage of cpufreq registration with existing governors?
> 
>  - Come up with a scaling factor using the PMU cycle counter at boot
> before the CPPC drivers are initialized. This would use the current
> freq set by some UEFI var. This would possibly require some messy
> perfevents plumbing and added bootup time though.
> 
> - .. ?

Not sure at the moment.

Plus, there's one more thing to consider.  We may end up having to use CPPC on
x86 after all (for reasons that are not relevant here), in which case it
probably would make sense to merge the acpi-cpufreq and cppc-cpufreq drivers 
IMO.

Thanks,
Rafael

Re: [PATCH v2] Force cppc_cpufreq to report values in KHz to fix user space reporting

[Rafael J. Wysocki]

On Tuesday, April 19, 2016 04:12:41 PM Ashwin Chaugule wrote:
> + Ryan
> 
> Hi Al,
> 
> On 18 April 2016 at 20:11, 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).
> >
> > While the investigation for a long term fix proceeds (several options
> > are being explored, some of which may require spec changes or other
> > much more invasive fixes), this patch forces the values read by CPPC
> > to be read in KHz, regardless of what they actually represent.
> >
> > 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.  This
> >patch retrieves the first 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.
> >
> > For arm64 servers, this may be sufficient, but it does rely on
> > firmware values being set correctly.  Hence, other approaches are
> > also being considered.
> >
> > This has been tested on three arm64 servers, with and without DMI, with
> > and without CPPC support.
> >
> > 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 
> 
> This looks like a good short term solution. Does it make more sense to
> move this to the cppc_cpufreq driver though? Since that ties more
> closely into the cpufreq framework which requires the kHz values in
> sysfs. That way we can keep the cppc_acpi.c shim compliant with the
> ACPI spec. (i.e. values read in cppc structures remain abstract and
> unitless).
> 
> Rafael, Viresh, others,
> 
> Any other ideas how to handle this better in the long term?
> 
>  - Decouple the cpufreq sysfs from the cppc driver and introduce its
> own entries. Is it possibly to do this cleanly while still allowing
> usage of cpufreq registration with existing governors?
> 
>  - Come up with a scaling factor using the PMU cycle counter at boot
> before the CPPC drivers are initialized. This would use the current
> freq set by some UEFI var. This would possibly require some messy
> perfevents plumbing and added bootup time though.
> 
> - .. ?

Not sure at the moment.

Plus, there's one more thing to consider.  We may end up having to use CPPC on
x86 after all (for reasons that are not relevant here), in which case it
probably would make sense to merge the acpi-cpufreq and cppc-cpufreq drivers 
IMO.

Thanks,
Rafael

Re: [PATCH v2] Force cppc_cpufreq to report values in KHz to fix user space reporting

[Rafael J. Wysocki]

On Friday, April 22, 2016 11:00:20 AM Viresh Kumar wrote:
> On 19-04-16, 16:12, Ashwin Chaugule wrote:
> > + Ryan
> > 
> > Hi Al,
> > 
> > On 18 April 2016 at 20:11, 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).
> > >
> > > While the investigation for a long term fix proceeds (several options
> > > are being explored, some of which may require spec changes or other
> > > much more invasive fixes), this patch forces the values read by CPPC
> > > to be read in KHz, regardless of what they actually represent.
> > >
> > > 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.  This
> > >patch retrieves the first 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.
> 
> Don't we have any big LITTLE ARM servers yet ? Or we will not have them at 
> all ?
> 
> > > For arm64 servers, this may be sufficient, but it does rely on
> > > firmware values being set correctly.  Hence, other approaches are
> > > also being considered.
> > >
> > > This has been tested on three arm64 servers, with and without DMI, with
> > > and without CPPC support.
> > >
> > > 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 
> > 
> > This looks like a good short term solution. Does it make more sense to
> > move this to the cppc_cpufreq driver though? Since that ties more
> > closely into the cpufreq framework which requires the kHz values in
> > sysfs. That way we can keep the cppc_acpi.c shim compliant with the
> > ACPI spec. (i.e. values read in cppc structures remain abstract and
> > unitless).
> > 
> > Rafael, Viresh, others,
> > 
> > Any other ideas how to handle this better in the long term?
> > 
> >  - Decouple the cpufreq sysfs from the cppc driver and introduce its
> > own entries. Is it possibly to do this cleanly while still allowing
> > usage of cpufreq registration with existing governors?
> > 
> >  - Come up with a scaling factor using the PMU cycle counter at boot
> > before the CPPC drivers are initialized. This would use the current
> > freq set by some UEFI var. This would possibly require some messy
> > perfevents plumbing and added bootup time though.
> 
> I may be missing the obvious, but can't we just create the cpufreq-table from
> this table in khz? We wouldn't require any further change then.

I wouldn't really like to do that, because the freq table would be totally
artificial then.

Thanks,
Rafael

Re: [PATCH v2] Force cppc_cpufreq to report values in KHz to fix user space reporting

[Rafael J. Wysocki]

On Friday, April 22, 2016 11:00:20 AM Viresh Kumar wrote:
> On 19-04-16, 16:12, Ashwin Chaugule wrote:
> > + Ryan
> > 
> > Hi Al,
> > 
> > On 18 April 2016 at 20:11, 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).
> > >
> > > While the investigation for a long term fix proceeds (several options
> > > are being explored, some of which may require spec changes or other
> > > much more invasive fixes), this patch forces the values read by CPPC
> > > to be read in KHz, regardless of what they actually represent.
> > >
> > > 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.  This
> > >patch retrieves the first 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.
> 
> Don't we have any big LITTLE ARM servers yet ? Or we will not have them at 
> all ?
> 
> > > For arm64 servers, this may be sufficient, but it does rely on
> > > firmware values being set correctly.  Hence, other approaches are
> > > also being considered.
> > >
> > > This has been tested on three arm64 servers, with and without DMI, with
> > > and without CPPC support.
> > >
> > > 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 
> > 
> > This looks like a good short term solution. Does it make more sense to
> > move this to the cppc_cpufreq driver though? Since that ties more
> > closely into the cpufreq framework which requires the kHz values in
> > sysfs. That way we can keep the cppc_acpi.c shim compliant with the
> > ACPI spec. (i.e. values read in cppc structures remain abstract and
> > unitless).
> > 
> > Rafael, Viresh, others,
> > 
> > Any other ideas how to handle this better in the long term?
> > 
> >  - Decouple the cpufreq sysfs from the cppc driver and introduce its
> > own entries. Is it possibly to do this cleanly while still allowing
> > usage of cpufreq registration with existing governors?
> > 
> >  - Come up with a scaling factor using the PMU cycle counter at boot
> > before the CPPC drivers are initialized. This would use the current
> > freq set by some UEFI var. This would possibly require some messy
> > perfevents plumbing and added bootup time though.
> 
> I may be missing the obvious, but can't we just create the cpufreq-table from
> this table in khz? We wouldn't require any further change then.

I wouldn't really like to do that, because the freq table would be totally
artificial then.

Thanks,
Rafael

Re: [PATCH v2] Force cppc_cpufreq to report values in KHz to fix user space reporting

[Viresh Kumar]

On 19-04-16, 16:12, Ashwin Chaugule wrote:
> + Ryan
> 
> Hi Al,
> 
> On 18 April 2016 at 20:11, 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).
> >
> > While the investigation for a long term fix proceeds (several options
> > are being explored, some of which may require spec changes or other
> > much more invasive fixes), this patch forces the values read by CPPC
> > to be read in KHz, regardless of what they actually represent.
> >
> > 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.  This
> >patch retrieves the first 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.

Don't we have any big LITTLE ARM servers yet ? Or we will not have them at all ?

> > For arm64 servers, this may be sufficient, but it does rely on
> > firmware values being set correctly.  Hence, other approaches are
> > also being considered.
> >
> > This has been tested on three arm64 servers, with and without DMI, with
> > and without CPPC support.
> >
> > 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 
> 
> This looks like a good short term solution. Does it make more sense to
> move this to the cppc_cpufreq driver though? Since that ties more
> closely into the cpufreq framework which requires the kHz values in
> sysfs. That way we can keep the cppc_acpi.c shim compliant with the
> ACPI spec. (i.e. values read in cppc structures remain abstract and
> unitless).
> 
> Rafael, Viresh, others,
> 
> Any other ideas how to handle this better in the long term?
> 
>  - Decouple the cpufreq sysfs from the cppc driver and introduce its
> own entries. Is it possibly to do this cleanly while still allowing
> usage of cpufreq registration with existing governors?
> 
>  - Come up with a scaling factor using the PMU cycle counter at boot
> before the CPPC drivers are initialized. This would use the current
> freq set by some UEFI var. This would possibly require some messy
> perfevents plumbing and added bootup time though.

I may be missing the obvious, but can't we just create the cpufreq-table from
this table in khz? We wouldn't require any further change then.

-- 
viresh

Re: [PATCH v2] Force cppc_cpufreq to report values in KHz to fix user space reporting

[Viresh Kumar]

On 19-04-16, 16:12, Ashwin Chaugule wrote:
> + Ryan
> 
> Hi Al,
> 
> On 18 April 2016 at 20:11, 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).
> >
> > While the investigation for a long term fix proceeds (several options
> > are being explored, some of which may require spec changes or other
> > much more invasive fixes), this patch forces the values read by CPPC
> > to be read in KHz, regardless of what they actually represent.
> >
> > 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.  This
> >patch retrieves the first 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.

Don't we have any big LITTLE ARM servers yet ? Or we will not have them at all ?

> > For arm64 servers, this may be sufficient, but it does rely on
> > firmware values being set correctly.  Hence, other approaches are
> > also being considered.
> >
> > This has been tested on three arm64 servers, with and without DMI, with
> > and without CPPC support.
> >
> > 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 
> 
> This looks like a good short term solution. Does it make more sense to
> move this to the cppc_cpufreq driver though? Since that ties more
> closely into the cpufreq framework which requires the kHz values in
> sysfs. That way we can keep the cppc_acpi.c shim compliant with the
> ACPI spec. (i.e. values read in cppc structures remain abstract and
> unitless).
> 
> Rafael, Viresh, others,
> 
> Any other ideas how to handle this better in the long term?
> 
>  - Decouple the cpufreq sysfs from the cppc driver and introduce its
> own entries. Is it possibly to do this cleanly while still allowing
> usage of cpufreq registration with existing governors?
> 
>  - Come up with a scaling factor using the PMU cycle counter at boot
> before the CPPC drivers are initialized. This would use the current
> freq set by some UEFI var. This would possibly require some messy
> perfevents plumbing and added bootup time though.

I may be missing the obvious, but can't we just create the cpufreq-table from
this table in khz? We wouldn't require any further change then.

-- 
viresh

Re: [PATCH v2] Force cppc_cpufreq to report values in KHz to fix user space reporting

[Al Stone]

On 04/21/2016 08:53 AM, Alexey Klimov wrote:
> 
> On Tue, Apr 19, 2016 at 1:11 AM, 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).
>>
>> While the investigation for a long term fix proceeds (several options
>> are being explored, some of which may require spec changes or other
>> much more invasive fixes), this patch forces the values read by CPPC
>> to be read in KHz, regardless of what they actually represent.
>>
>> 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.  This
> 
> Sometimes short-term solution becomes long-term. It's worth to place
> comment in code about this assumption.

True.  I'll add a comment.  Thanks.

>>patch retrieves the first 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.
>>
>> For arm64 servers, this may be sufficient, but it does rely on
>> firmware values being set correctly.  Hence, other approaches are
>> also being considered.
>>
>> This has been tested on three arm64 servers, with and without DMI, with
>> and without CPPC support.
>>
>> 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 
>> ---
>>  drivers/acpi/cppc_acpi.c| 61 
>> +
>>  drivers/cpufreq/Kconfig.arm |  1 +
>>  2 files changed, 57 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index 8adac69..d61ced6 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -40,6 +40,9 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>> +
>> +#include 
>>
>>  #include 
>>  /*
>> @@ -709,6 +712,47 @@ static int cpc_write(struct cpc_reg *reg, u64 val)
>> return ret_val;
>>  }
>>
>> +static u64 cppc_dmi_khz;
>> +
>> +static void cppc_find_dmi_mhz(const struct dmi_header *dm, void *private)
>> +{
>> +   u16 *mhz = (u16 *)private;
>> +   const u8 *dmi_data = (const u8 *)dm;
>> +
>> +   if (dm->type == DMI_ENTRY_PROCESSOR && dm->length >= 48)
>> +   *mhz = (u16)get_unaligned((const u16 *)(dmi_data + 0x14));
>> +}
>> +
>> +
>> +static u64 cppc_get_dmi_khz(void)
>> +{
>> +   u16 mhz;
>> +
>> +   dmi_walk(cppc_find_dmi_mhz, );
>> +
>> +   /*
>> +* Real stupid fallback value, just in case there is no
>> +* actual value set.
>> +*/
>> +   mhz = mhz ? mhz : 1;
>> +
>> +   return (1000 * mhz);
>> +}
>> +
>> +static u64 cppc_unitless_to_khz(u64 min, u64 max, u64 val)
>> +{
>> +   /*
>> +* The incoming val should be min <= val <= max.  Our
>> +* job is to convert that to KHz so it can be properly
>> +* reported to user space via cpufreq_policy.
>> +*/
>> +
>> +   if (!cppc_dmi_khz)
>> +   cppc_dmi_khz = cppc_get_dmi_khz();
>> +
>> +   return ((val - min) * cppc_dmi_khz) / (max - min);
> 
> How pedantic should the kernel be while dealing with this values?

I'm not sure it can be.  By definition, the CPPC values define an abstract
range.  We are only associating it with a frequency here because those are
the units assumed elsewhere in the kernel, and that user space tools make the
same assumptions.  What I'm looking at for the longer term is possibly breaking
those assumptions so that maybe we can be pedantic.

> This 1) can potentially divide by zero (extra care is required to
> perform this in Solar System) and 2) can return 0.

Hrm.  I'll double check the path for divide by zero; I thought that was covered
elsewhere along the path but I might have missed it.

A zero in this case would mean the processor is running at its lowest possible
level of performance, and is an artifact of mapping the CPPC abstract value
onto a linear scale from 0 to max KHz.  Granted, that may not be exactly the
same as 0 KHz; I'm open to suggestions here.  If there's a relatively
straightforward way to get a processor's minimum operating 

Re: [PATCH v2] Force cppc_cpufreq to report values in KHz to fix user space reporting

[Al Stone]

On 04/21/2016 08:53 AM, Alexey Klimov wrote:
> 
> On Tue, Apr 19, 2016 at 1:11 AM, 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).
>>
>> While the investigation for a long term fix proceeds (several options
>> are being explored, some of which may require spec changes or other
>> much more invasive fixes), this patch forces the values read by CPPC
>> to be read in KHz, regardless of what they actually represent.
>>
>> 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.  This
> 
> Sometimes short-term solution becomes long-term. It's worth to place
> comment in code about this assumption.

True.  I'll add a comment.  Thanks.

>>patch retrieves the first 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.
>>
>> For arm64 servers, this may be sufficient, but it does rely on
>> firmware values being set correctly.  Hence, other approaches are
>> also being considered.
>>
>> This has been tested on three arm64 servers, with and without DMI, with
>> and without CPPC support.
>>
>> 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 
>> ---
>>  drivers/acpi/cppc_acpi.c| 61 
>> +
>>  drivers/cpufreq/Kconfig.arm |  1 +
>>  2 files changed, 57 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index 8adac69..d61ced6 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -40,6 +40,9 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>> +
>> +#include 
>>
>>  #include 
>>  /*
>> @@ -709,6 +712,47 @@ static int cpc_write(struct cpc_reg *reg, u64 val)
>> return ret_val;
>>  }
>>
>> +static u64 cppc_dmi_khz;
>> +
>> +static void cppc_find_dmi_mhz(const struct dmi_header *dm, void *private)
>> +{
>> +   u16 *mhz = (u16 *)private;
>> +   const u8 *dmi_data = (const u8 *)dm;
>> +
>> +   if (dm->type == DMI_ENTRY_PROCESSOR && dm->length >= 48)
>> +   *mhz = (u16)get_unaligned((const u16 *)(dmi_data + 0x14));
>> +}
>> +
>> +
>> +static u64 cppc_get_dmi_khz(void)
>> +{
>> +   u16 mhz;
>> +
>> +   dmi_walk(cppc_find_dmi_mhz, );
>> +
>> +   /*
>> +* Real stupid fallback value, just in case there is no
>> +* actual value set.
>> +*/
>> +   mhz = mhz ? mhz : 1;
>> +
>> +   return (1000 * mhz);
>> +}
>> +
>> +static u64 cppc_unitless_to_khz(u64 min, u64 max, u64 val)
>> +{
>> +   /*
>> +* The incoming val should be min <= val <= max.  Our
>> +* job is to convert that to KHz so it can be properly
>> +* reported to user space via cpufreq_policy.
>> +*/
>> +
>> +   if (!cppc_dmi_khz)
>> +   cppc_dmi_khz = cppc_get_dmi_khz();
>> +
>> +   return ((val - min) * cppc_dmi_khz) / (max - min);
> 
> How pedantic should the kernel be while dealing with this values?

I'm not sure it can be.  By definition, the CPPC values define an abstract
range.  We are only associating it with a frequency here because those are
the units assumed elsewhere in the kernel, and that user space tools make the
same assumptions.  What I'm looking at for the longer term is possibly breaking
those assumptions so that maybe we can be pedantic.

> This 1) can potentially divide by zero (extra care is required to
> perform this in Solar System) and 2) can return 0.

Hrm.  I'll double check the path for divide by zero; I thought that was covered
elsewhere along the path but I might have missed it.

A zero in this case would mean the processor is running at its lowest possible
level of performance, and is an artifact of mapping the CPPC abstract value
onto a linear scale from 0 to max KHz.  Granted, that may not be exactly the
same as 0 KHz; I'm open to suggestions here.  If there's a relatively
straightforward way to get a processor's minimum operating frequency (apart
from completely off), we 

Re: [PATCH v2] Force cppc_cpufreq to report values in KHz to fix user space reporting

[Alexey Klimov]

On Tue, Apr 19, 2016 at 1:11 AM, 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).
> 
> While the investigation for a long term fix proceeds (several options
> are being explored, some of which may require spec changes or other
> much more invasive fixes), this patch forces the values read by CPPC
> to be read in KHz, regardless of what they actually represent.
> 
> 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.  This

Sometimes short-term solution becomes long-term. It's worth to place
comment in code about this assumption.

>patch retrieves the first 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.
> 
> For arm64 servers, this may be sufficient, but it does rely on
> firmware values being set correctly.  Hence, other approaches are
> also being considered.
> 
> This has been tested on three arm64 servers, with and without DMI, with
> and without CPPC support.
> 
> 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 
> ---
>  drivers/acpi/cppc_acpi.c| 61 
> +
>  drivers/cpufreq/Kconfig.arm |  1 +
>  2 files changed, 57 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 8adac69..d61ced6 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -40,6 +40,9 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +
> +#include 
> 
>  #include 
>  /*
> @@ -709,6 +712,47 @@ static int cpc_write(struct cpc_reg *reg, u64 val)
> return ret_val;
>  }
> 
> +static u64 cppc_dmi_khz;
> +
> +static void cppc_find_dmi_mhz(const struct dmi_header *dm, void *private)
> +{
> +   u16 *mhz = (u16 *)private;
> +   const u8 *dmi_data = (const u8 *)dm;
> +
> +   if (dm->type == DMI_ENTRY_PROCESSOR && dm->length >= 48)
> +   *mhz = (u16)get_unaligned((const u16 *)(dmi_data + 0x14));
> +}
> +
> +
> +static u64 cppc_get_dmi_khz(void)
> +{
> +   u16 mhz;
> +
> +   dmi_walk(cppc_find_dmi_mhz, );
> +
> +   /*
> +* Real stupid fallback value, just in case there is no
> +* actual value set.
> +*/
> +   mhz = mhz ? mhz : 1;
> +
> +   return (1000 * mhz);
> +}
> +
> +static u64 cppc_unitless_to_khz(u64 min, u64 max, u64 val)
> +{
> +   /*
> +* The incoming val should be min <= val <= max.  Our
> +* job is to convert that to KHz so it can be properly
> +* reported to user space via cpufreq_policy.
> +*/
> +
> +   if (!cppc_dmi_khz)
> +   cppc_dmi_khz = cppc_get_dmi_khz();
> +
> +   return ((val - min) * cppc_dmi_khz) / (max - min);

How pedantic should the kernel be while dealing with this values?

This 1) can potentially divide by zero (extra care is required to
perform this in Solar System) and 2) can return 0.

Not sure if there is some benefit for firmware to export such
values.

[..]

Best regards,
Alexey

Re: [PATCH v2] Force cppc_cpufreq to report values in KHz to fix user space reporting

[Alexey Klimov]

On Tue, Apr 19, 2016 at 1:11 AM, 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).
> 
> While the investigation for a long term fix proceeds (several options
> are being explored, some of which may require spec changes or other
> much more invasive fixes), this patch forces the values read by CPPC
> to be read in KHz, regardless of what they actually represent.
> 
> 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.  This

Sometimes short-term solution becomes long-term. It's worth to place
comment in code about this assumption.

>patch retrieves the first 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.
> 
> For arm64 servers, this may be sufficient, but it does rely on
> firmware values being set correctly.  Hence, other approaches are
> also being considered.
> 
> This has been tested on three arm64 servers, with and without DMI, with
> and without CPPC support.
> 
> 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 
> ---
>  drivers/acpi/cppc_acpi.c| 61 
> +
>  drivers/cpufreq/Kconfig.arm |  1 +
>  2 files changed, 57 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 8adac69..d61ced6 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -40,6 +40,9 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +
> +#include 
> 
>  #include 
>  /*
> @@ -709,6 +712,47 @@ static int cpc_write(struct cpc_reg *reg, u64 val)
> return ret_val;
>  }
> 
> +static u64 cppc_dmi_khz;
> +
> +static void cppc_find_dmi_mhz(const struct dmi_header *dm, void *private)
> +{
> +   u16 *mhz = (u16 *)private;
> +   const u8 *dmi_data = (const u8 *)dm;
> +
> +   if (dm->type == DMI_ENTRY_PROCESSOR && dm->length >= 48)
> +   *mhz = (u16)get_unaligned((const u16 *)(dmi_data + 0x14));
> +}
> +
> +
> +static u64 cppc_get_dmi_khz(void)
> +{
> +   u16 mhz;
> +
> +   dmi_walk(cppc_find_dmi_mhz, );
> +
> +   /*
> +* Real stupid fallback value, just in case there is no
> +* actual value set.
> +*/
> +   mhz = mhz ? mhz : 1;
> +
> +   return (1000 * mhz);
> +}
> +
> +static u64 cppc_unitless_to_khz(u64 min, u64 max, u64 val)
> +{
> +   /*
> +* The incoming val should be min <= val <= max.  Our
> +* job is to convert that to KHz so it can be properly
> +* reported to user space via cpufreq_policy.
> +*/
> +
> +   if (!cppc_dmi_khz)
> +   cppc_dmi_khz = cppc_get_dmi_khz();
> +
> +   return ((val - min) * cppc_dmi_khz) / (max - min);

How pedantic should the kernel be while dealing with this values?

This 1) can potentially divide by zero (extra care is required to
perform this in Solar System) and 2) can return 0.

Not sure if there is some benefit for firmware to export such
values.

[..]

Best regards,
Alexey

Re: [PATCH v2] Force cppc_cpufreq to report values in KHz to fix user space reporting

[Al Stone]

On 04/19/2016 02:12 PM, Ashwin Chaugule wrote:
> + Ryan
> 
> Hi Al,
> 
> On 18 April 2016 at 20:11, 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).
>>
>> While the investigation for a long term fix proceeds (several options
>> are being explored, some of which may require spec changes or other
>> much more invasive fixes), this patch forces the values read by CPPC
>> to be read in KHz, regardless of what they actually represent.
>>
>> 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.  This
>>patch retrieves the first 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.
>>
>> For arm64 servers, this may be sufficient, but it does rely on
>> firmware values being set correctly.  Hence, other approaches are
>> also being considered.
>>
>> This has been tested on three arm64 servers, with and without DMI, with
>> and without CPPC support.
>>
>> 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 
> 
> This looks like a good short term solution. Does it make more sense to
> move this to the cppc_cpufreq driver though? Since that ties more
> closely into the cpufreq framework which requires the kHz values in
> sysfs. That way we can keep the cppc_acpi.c shim compliant with the
> ACPI spec. (i.e. values read in cppc structures remain abstract and
> unitless).

Perhaps.  Doing it that way made the patch a bit messier since
cppc_acpi.c would set values that then had to be replaced in
cppc_cpufreq.c, so initialization looked odd to me; that's how
I ended up here.  You do raise a good point, however; I'll look
at that approach again since I could have missed an easier way
to do it.

> Rafael, Viresh, others,
> 
> Any other ideas how to handle this better in the long term?
> 
>  - Decouple the cpufreq sysfs from the cppc driver and introduce its
> own entries. Is it possibly to do this cleanly while still allowing
> usage of cpufreq registration with existing governors?
> 
>  - Come up with a scaling factor using the PMU cycle counter at boot
> before the CPPC drivers are initialized. This would use the current
> freq set by some UEFI var. This would possibly require some messy
> perfevents plumbing and added bootup time though.
> 
> - .. ?
> 
> 
> Cheers,
> Ashwin.
> 

The other thought that occurs to me is to go back through the
perf_cap and cpufreq structs and make them more general -- perhaps
store the units being used and pointers to functions to convert them
to KHz.  This may require separating sysfs data for perf_cap from the
cpufreq sysfs data from the cppc sysfs data.  But, if units are then
reported out to sysfs, user space tools can do whatever conversions
they want, or at least know what they're reporting instead of there
being an implicit ABI between the kernel and the tools.  This would
be a far more invasive patch set, I think, but it still may be the
right thing to do for the long term.

-- 
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---

Re: [PATCH v2] Force cppc_cpufreq to report values in KHz to fix user space reporting

[Al Stone]

On 04/19/2016 02:12 PM, Ashwin Chaugule wrote:
> + Ryan
> 
> Hi Al,
> 
> On 18 April 2016 at 20:11, 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).
>>
>> While the investigation for a long term fix proceeds (several options
>> are being explored, some of which may require spec changes or other
>> much more invasive fixes), this patch forces the values read by CPPC
>> to be read in KHz, regardless of what they actually represent.
>>
>> 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.  This
>>patch retrieves the first 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.
>>
>> For arm64 servers, this may be sufficient, but it does rely on
>> firmware values being set correctly.  Hence, other approaches are
>> also being considered.
>>
>> This has been tested on three arm64 servers, with and without DMI, with
>> and without CPPC support.
>>
>> 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 
> 
> This looks like a good short term solution. Does it make more sense to
> move this to the cppc_cpufreq driver though? Since that ties more
> closely into the cpufreq framework which requires the kHz values in
> sysfs. That way we can keep the cppc_acpi.c shim compliant with the
> ACPI spec. (i.e. values read in cppc structures remain abstract and
> unitless).

Perhaps.  Doing it that way made the patch a bit messier since
cppc_acpi.c would set values that then had to be replaced in
cppc_cpufreq.c, so initialization looked odd to me; that's how
I ended up here.  You do raise a good point, however; I'll look
at that approach again since I could have missed an easier way
to do it.

> Rafael, Viresh, others,
> 
> Any other ideas how to handle this better in the long term?
> 
>  - Decouple the cpufreq sysfs from the cppc driver and introduce its
> own entries. Is it possibly to do this cleanly while still allowing
> usage of cpufreq registration with existing governors?
> 
>  - Come up with a scaling factor using the PMU cycle counter at boot
> before the CPPC drivers are initialized. This would use the current
> freq set by some UEFI var. This would possibly require some messy
> perfevents plumbing and added bootup time though.
> 
> - .. ?
> 
> 
> Cheers,
> Ashwin.
> 

The other thought that occurs to me is to go back through the
perf_cap and cpufreq structs and make them more general -- perhaps
store the units being used and pointers to functions to convert them
to KHz.  This may require separating sysfs data for perf_cap from the
cpufreq sysfs data from the cppc sysfs data.  But, if units are then
reported out to sysfs, user space tools can do whatever conversions
they want, or at least know what they're reporting instead of there
being an implicit ABI between the kernel and the tools.  This would
be a far more invasive patch set, I think, but it still may be the
right thing to do for the long term.

-- 
ciao,
al
---
Al Stone
Software Engineer
Red Hat, Inc.
a...@redhat.com
---

Re: [PATCH v2] Force cppc_cpufreq to report values in KHz to fix user space reporting

[Ashwin Chaugule]

+ Ryan

Hi Al,

On 18 April 2016 at 20:11, 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).
>
> While the investigation for a long term fix proceeds (several options
> are being explored, some of which may require spec changes or other
> much more invasive fixes), this patch forces the values read by CPPC
> to be read in KHz, regardless of what they actually represent.
>
> 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.  This
>patch retrieves the first 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.
>
> For arm64 servers, this may be sufficient, but it does rely on
> firmware values being set correctly.  Hence, other approaches are
> also being considered.
>
> This has been tested on three arm64 servers, with and without DMI, with
> and without CPPC support.
>
> 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 

This looks like a good short term solution. Does it make more sense to
move this to the cppc_cpufreq driver though? Since that ties more
closely into the cpufreq framework which requires the kHz values in
sysfs. That way we can keep the cppc_acpi.c shim compliant with the
ACPI spec. (i.e. values read in cppc structures remain abstract and
unitless).

Rafael, Viresh, others,

Any other ideas how to handle this better in the long term?

 - Decouple the cpufreq sysfs from the cppc driver and introduce its
own entries. Is it possibly to do this cleanly while still allowing
usage of cpufreq registration with existing governors?

 - Come up with a scaling factor using the PMU cycle counter at boot
before the CPPC drivers are initialized. This would use the current
freq set by some UEFI var. This would possibly require some messy
perfevents plumbing and added bootup time though.

- .. ?


Cheers,
Ashwin.

Re: [PATCH v2] Force cppc_cpufreq to report values in KHz to fix user space reporting

[Ashwin Chaugule]

+ Ryan

Hi Al,

On 18 April 2016 at 20:11, 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).
>
> While the investigation for a long term fix proceeds (several options
> are being explored, some of which may require spec changes or other
> much more invasive fixes), this patch forces the values read by CPPC
> to be read in KHz, regardless of what they actually represent.
>
> 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.  This
>patch retrieves the first 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.
>
> For arm64 servers, this may be sufficient, but it does rely on
> firmware values being set correctly.  Hence, other approaches are
> also being considered.
>
> This has been tested on three arm64 servers, with and without DMI, with
> and without CPPC support.
>
> 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 

This looks like a good short term solution. Does it make more sense to
move this to the cppc_cpufreq driver though? Since that ties more
closely into the cpufreq framework which requires the kHz values in
sysfs. That way we can keep the cppc_acpi.c shim compliant with the
ACPI spec. (i.e. values read in cppc structures remain abstract and
unitless).

Rafael, Viresh, others,

Any other ideas how to handle this better in the long term?

 - Decouple the cpufreq sysfs from the cppc driver and introduce its
own entries. Is it possibly to do this cleanly while still allowing
usage of cpufreq registration with existing governors?

 - Come up with a scaling factor using the PMU cycle counter at boot
before the CPPC drivers are initialized. This would use the current
freq set by some UEFI var. This would possibly require some messy
perfevents plumbing and added bootup time though.

- .. ?


Cheers,
Ashwin.

[PATCH v2] Force cppc_cpufreq to report values in KHz to fix user space reporting

[Al Stone]

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).

While the investigation for a long term fix proceeds (several options
are being explored, some of which may require spec changes or other
much more invasive fixes), this patch forces the values read by CPPC
to be read in KHz, regardless of what they actually represent.

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.  This
   patch retrieves the first 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.

For arm64 servers, this may be sufficient, but it does rely on
firmware values being set correctly.  Hence, other approaches are
also being considered.

This has been tested on three arm64 servers, with and without DMI, with
and without CPPC support.

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 
---
 drivers/acpi/cppc_acpi.c| 61 +
 drivers/cpufreq/Kconfig.arm |  1 +
 2 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 8adac69..d61ced6 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -40,6 +40,9 @@
 #include 
 #include 
 #include 
+#include 
+
+#include 
 
 #include 
 /*
@@ -709,6 +712,47 @@ static int cpc_write(struct cpc_reg *reg, u64 val)
return ret_val;
 }
 
+static u64 cppc_dmi_khz;
+
+static void cppc_find_dmi_mhz(const struct dmi_header *dm, void *private)
+{
+   u16 *mhz = (u16 *)private;
+   const u8 *dmi_data = (const u8 *)dm;
+
+   if (dm->type == DMI_ENTRY_PROCESSOR && dm->length >= 48)
+   *mhz = (u16)get_unaligned((const u16 *)(dmi_data + 0x14));
+}
+
+
+static u64 cppc_get_dmi_khz(void)
+{
+   u16 mhz;
+
+   dmi_walk(cppc_find_dmi_mhz, );
+
+   /*
+* Real stupid fallback value, just in case there is no
+* actual value set.
+*/
+   mhz = mhz ? mhz : 1;
+
+   return (1000 * mhz);
+}
+
+static u64 cppc_unitless_to_khz(u64 min, u64 max, u64 val)
+{
+   /*
+* The incoming val should be min <= val <= max.  Our
+* job is to convert that to KHz so it can be properly
+* reported to user space via cpufreq_policy.
+*/
+
+   if (!cppc_dmi_khz)
+   cppc_dmi_khz = cppc_get_dmi_khz();
+
+   return ((val - min) * cppc_dmi_khz) / (max - min);
+}
+
 /**
  * cppc_get_perf_caps - Get a CPUs performance capabilities.
  * @cpunum: CPU from which to get capabilities info.
@@ -748,17 +792,24 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps 
*perf_caps)
}
}
 
+   /*
+* Since these values in perf_caps will be used in setting
+* up the cpufreq policy, they must always be stored in units
+* of KHz.  If they are not, user space tools will become very
+* confused since they assume these are in KHz when reading
+* sysfs.
+*/
cpc_read(_reg->cpc_entry.reg, );
-   perf_caps->highest_perf = high;
-
cpc_read(_reg->cpc_entry.reg, );
-   perf_caps->lowest_perf = low;
+
+   perf_caps->highest_perf = cppc_unitless_to_khz(low, high, high);
+   perf_caps->lowest_perf = cppc_unitless_to_khz(low, high, low);
 
cpc_read(_perf->cpc_entry.reg, );
-   perf_caps->reference_perf = ref;
+   perf_caps->reference_perf = cppc_unitless_to_khz(low, high, ref);
 
cpc_read(_perf->cpc_entry.reg, );
-   perf_caps->nominal_perf = nom;
+   perf_caps->nominal_perf = cppc_unitless_to_khz(low, high, nom);
 
if (!ref)
perf_caps->reference_perf = perf_caps->nominal_perf;
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 14b1f93..d79 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -254,6 +254,7 @@ config ARM_PXA2xx_CPUFREQ
 config ACPI_CPPC_CPUFREQ
tristate "CPUFreq driver based on the ACPI CPPC spec"
depends on ACPI
+   depends on DMI
 

[PATCH v2] Force cppc_cpufreq to report values in KHz to fix user space reporting

[Al Stone]

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).

While the investigation for a long term fix proceeds (several options
are being explored, some of which may require spec changes or other
much more invasive fixes), this patch forces the values read by CPPC
to be read in KHz, regardless of what they actually represent.

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.  This
   patch retrieves the first 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.

For arm64 servers, this may be sufficient, but it does rely on
firmware values being set correctly.  Hence, other approaches are
also being considered.

This has been tested on three arm64 servers, with and without DMI, with
and without CPPC support.

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 
---
 drivers/acpi/cppc_acpi.c| 61 +
 drivers/cpufreq/Kconfig.arm |  1 +
 2 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 8adac69..d61ced6 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -40,6 +40,9 @@
 #include 
 #include 
 #include 
+#include 
+
+#include 
 
 #include 
 /*
@@ -709,6 +712,47 @@ static int cpc_write(struct cpc_reg *reg, u64 val)
return ret_val;
 }
 
+static u64 cppc_dmi_khz;
+
+static void cppc_find_dmi_mhz(const struct dmi_header *dm, void *private)
+{
+   u16 *mhz = (u16 *)private;
+   const u8 *dmi_data = (const u8 *)dm;
+
+   if (dm->type == DMI_ENTRY_PROCESSOR && dm->length >= 48)
+   *mhz = (u16)get_unaligned((const u16 *)(dmi_data + 0x14));
+}
+
+
+static u64 cppc_get_dmi_khz(void)
+{
+   u16 mhz;
+
+   dmi_walk(cppc_find_dmi_mhz, );
+
+   /*
+* Real stupid fallback value, just in case there is no
+* actual value set.
+*/
+   mhz = mhz ? mhz : 1;
+
+   return (1000 * mhz);
+}
+
+static u64 cppc_unitless_to_khz(u64 min, u64 max, u64 val)
+{
+   /*
+* The incoming val should be min <= val <= max.  Our
+* job is to convert that to KHz so it can be properly
+* reported to user space via cpufreq_policy.
+*/
+
+   if (!cppc_dmi_khz)
+   cppc_dmi_khz = cppc_get_dmi_khz();
+
+   return ((val - min) * cppc_dmi_khz) / (max - min);
+}
+
 /**
  * cppc_get_perf_caps - Get a CPUs performance capabilities.
  * @cpunum: CPU from which to get capabilities info.
@@ -748,17 +792,24 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps 
*perf_caps)
}
}
 
+   /*
+* Since these values in perf_caps will be used in setting
+* up the cpufreq policy, they must always be stored in units
+* of KHz.  If they are not, user space tools will become very
+* confused since they assume these are in KHz when reading
+* sysfs.
+*/
cpc_read(_reg->cpc_entry.reg, );
-   perf_caps->highest_perf = high;
-
cpc_read(_reg->cpc_entry.reg, );
-   perf_caps->lowest_perf = low;
+
+   perf_caps->highest_perf = cppc_unitless_to_khz(low, high, high);
+   perf_caps->lowest_perf = cppc_unitless_to_khz(low, high, low);
 
cpc_read(_perf->cpc_entry.reg, );
-   perf_caps->reference_perf = ref;
+   perf_caps->reference_perf = cppc_unitless_to_khz(low, high, ref);
 
cpc_read(_perf->cpc_entry.reg, );
-   perf_caps->nominal_perf = nom;
+   perf_caps->nominal_perf = cppc_unitless_to_khz(low, high, nom);
 
if (!ref)
perf_caps->reference_perf = perf_caps->nominal_perf;
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 14b1f93..d79 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -254,6 +254,7 @@ config ARM_PXA2xx_CPUFREQ
 config ACPI_CPPC_CPUFREQ
tristate "CPUFreq driver based on the ACPI CPPC spec"
depends on ACPI
+   depends on DMI
select