_PDC method in DSDT

The spec intended FixedFunctionalHW registers to be an "escape hatch". FFH means that while ACPI is used for perf state enumeration, the transitions will be handled in a CPU-specific driver, not via methods described in the ACPI spec. This division (use ACPI for enum, x for control) isn't currently supported by cpufreq; ACPI either does both enum and control, or neither.

It is not safe to assume the CPU-specific mechanism will be the same across vendors, or even across models from the same vendor. Indeed, the whole point of all this was so these mechanisms could be different, and remain proprietary.

Response #1 (practical):
What patch? The patch you haven't written yet? That may or may not be against files I maintain (as opposed to arch/i386/kernel/cpu/cpufreq/acpi.c)?

Response #2 (philosophical):
Do you really think a maintainer really has any power? Or just responsibility?

Regards -- Andy

But it could easily be added to the cpufreq drivers. For example, the speedstep-centrino driver could call a

int acpi_get_frequency_table(struct cpufreq_frequency_table **freq_table)

which returns an array with the frequency in freq_table[i].frequency, and some sort of "index" in freq_table[i].index, e.g. what needs to be written into the IA32_PERF_CTL MSR.

Dominik

Yes. It can be added. But do we want to add it? That's the real question.

Ducrot Bruno

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.

Well, the current scheme only works because there's a small number of easily identifiable Enhanced SpeedStep-capable processors which happen to use the same encoding for the PERF_CTL register. It could get more complicated in future, and falling back to a table from ACPI may well be the appropriate solution for many people - though I would like to maintain the tables in speedstep-centrino so that it isn't dependent on ACPI.

J

Thanks for all the responses on this issue. Forgive my ignorance in many cases, I've been involved in this code much less time than most people here.

What about this approach as a possible solution to everything that's been discussed:

Patch ACPI as follows: In the case where _PCT returns FFH as the register type in processor.c, we would simply call a processor-specific function in /arch/i386/ instead of using outb() for the state changes. The function would be called with arguments that provide all the information that ACPI gives us such as the control register, control value, status value, etc.

This code in /arch/i386/ would be fairly trivial for Enhanced Speedstep:
just a call to wrmsr(). For AMD and Transmeta processors, it would be a little more complicated since ACPI does not provide all the necessary information to do the state changes. Furthermore, the processor-specific code would not need to be part of the normal ACPI distribution if necessary -- it has minimal dependencies and could just reside normally in the kernel.

For those who want to use cpufreq and ACPI simultaneously, they could use the cpufreq-ACPI driver, which would now be capable of using the appropriate processor-specific functions for state changes with little ACPI overhead.

The only big drawback of this approach is the duplication of code between the existing cpufreq drivers and these new processor-specific state changing drivers. Perhaps they could be folded into one by some changes to the cpufreq API?

Does this seem like a sensible approach?

Regards,

David

"Fresh", new ideas are always most welcome :-)

Firstly, please let me explain the goal of the cpufreq project: it is a cross-architecture, _generic framework_ which provides

a) a standard userspace interface to select certain processor speeds (sysfs based in 2.5)
b) another interface to "governors" which decide what speed to set (e.g. based on userspace input), and
c) common core functions (so-called notifiers) which update essential timing code in the kernel, such as the Time Stamp Counter (TSC) timer.

So, to make life simple for the user (a), to offer him some cool features (b), and to assure the kernel won't break switching the core frequency
(c) all "processor drivers" which change the processor core frequency should register themselves as cpufreq drivers.[*]

Because of this, the ACPI P-States driver became a "cpufreq driver" in the _development_ series -- see arch/i386/kernel/cpu/cpufreq/acpi.c in latest 2.5. kernels.

Also, you should try to think of ACPI as a "library" which provides useful helpers for other kernel services -- CPU enumeration, information on how IRQs can be routed, information on how a specific device can be put to sleep. It shouldn't be the ACPI code which is in "control of things", as there are too many broken BIOSes around, and too many "legacy" systems can't even spell ACPI at all. Instead, it should but the specific __device drivers__ which are in "control of things". Nonetheless, these _may_ use ACPI to obtain information/control of the hardware.

IMO the ACPI P-States driver should only include the ACPI 2.0 P-States driver, and not try to support all other sorts of legacy support and become one "mega-driver". Instead, the other information stored in _PCT you talk about should be available to other processor-specific cpufreq drivers which see a need to obtain this information -- ACPI as a library, available to those who want it.

Using some sort of method to switch the core frequency of the processor without informing the cpufreq core is dangerous. The cpufreq core makes sure the timing code in other areas of the kernel is updated.

Indeed, and it is unnecessary. What's worth having, though, is a way to pass the information ACPI possesses to the cpufreq drivers whenever these think it's necessary.

Dominik

[*] I know the newly-added drivers/acpi/processor.c in 2.4.22-pre contains the ACPI P-States 2.0 driver which can't register itself to any cpufreq core as cpufreq is only part of 2.5. kernels. But this is a different issue.

Thanks for your explanation. My fault was that I was too busy looking at 2.4.21-ac1 and I didn't realize cpufreq had matured a lot in the development series and resolved the duplication between it and the ACPI drivers.

Yes, I see that now. That looks like exactly the right way to do it.

I see your rationale for wanting the P-state driver to be ACPI 2.0-only, but I think there is some advantage to adding Enhanced Speedstep support to i386/kernel/cpu/cpufreq/acpi.c rather than just added ACPI query capabilities to speedstep-centrino.c:

speedstep-centrino.c is very picky about the precise CPU because its tables are hard-coded with no margin for error. With ACPI, the test for Enhanced Speedstep would be more general:
if cpu_has(cpu, X86_FEATURE_EST) is true, and after calling _PDC, if _PTC returns FFH registers, then treat those registers as MSRs instead of IO ports. Thus, it supports any Enhanced Speedstep hardware with a correct BIOS, not just the ones enumerated by speedstep-centrino.c.

I guess my main point is that in order for speedstep-centrino.c to call ACPI as a library, it would basically need to use every function already written in acpi.c. Why not just dispatch the wrmsr() from acpi.c rather than the other way around? It would take probably a 15-line patch to add Enhanced Speedstep support to acpi.c, but probably would be much more complicated to query ACPI from speedstep-centrino.c. For AMD and Transmeta processors, there would be no point in specific support for them in acpi.c since ACPI doesn't really provide useful information for their performance control anyway.

-David

Hardly any of the code in speedstep-centrino has to do with actually setting the speed; nor is much of it to do with identifying the CPU and selecting the right table. Most of it is to do with being a well-behaved cpufreq driver. You would need to move more code than just the "wrmsr" into acpi.c.

The driver as it stands still has to exist for the case where you're not using ACPI, but if ACPI is available, using its table is certainly a useful thing to have. If the ACPI code for getting that table information is so complicated, then how about a nice simple entrypoint which provides the appropiate frequency points with the corresponding PERF_CTL values?

The core of the tension is whether to put ACPI code under arch/i386/kernel/cpu/cpufreq or put cpufreq code into drivers/acpi. There doesn't seem to be much to choose between the two, but if ACPI is a mechanism which is supposed to supply services to other drivers, it doesn't make much sense to migrate all those drivers into drivers/acpi.

J

My understanding was that i386/kernel/cpu/cpufreq/acpi.c in 2.5.x was already a well-behaved cpufreq driver. Am I wrong about that?

Right, that's why the ACPI performance control code got moved out of drivers/acpi and into arch/i386/kernel/cpu/cpufreq in the 2.5 series. The problem is that, if I understand it correctly, the current code for ACPI performance control in 2.5 does not really work as a library for other drivers -- it's basically just a standalone cpufreq driver. It would be much easier to add a few lines for Enhanced Speedstep than to restructure the code to work well as a library.

-David

I had misunderstood - I had thought you were talking about acpi core code rather than cpufreq/acpi.c. Hm, I see what you mean.

J

Then break cpufreq/acpi.c in 2 files. One common acting like a library. And the other will be the real cpufreq driver.

Still, I don't see why you want the Enhanced SpeedStep in the cpufreq driver called acpi. Better to export the table via a function if that is really needed, and to support it in an external driver like speedstep-centrino. This was the intention of ACPI to do things like this anyway, or you miss-interpret the current ACPI specification perhaps.

Then only concern I may have for is still the _PDC method. It is currently only documented in a MS document. It is defined as an optional method used by the OS in order to be somehow compatible with older releases of Windows. But it has the folowing:

Arguments:
Arg0 (buffer):

DWORD 0:         Revision Id
DWORD 1:    Number of capabilities DWORDs in buffer
DWORD 2-n:   Capabilities DWORDs, where each bit

defines capabilities and features supported by the OSPM for processor power management.
The definitions and meaning of each capabilities bit is vendor specific, and shared with OSPM by the vendor.

Result Code:
None

Then give us now the correct values to be passed for Pentium-M found in Centrino platform, and I will be glad to do the break of cpufreq/acpi.c for you.

Cheers,

Ducrot Bruno

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.

Dword 0 should be 0x1
Dword 1 should be 0x1
Dword 2 should be 0x1

The 0x1 in Dword 2 indicates that the driver supports Enhanced Speedstep. In my DSDT, Dwords 0 and 1 are ignored anyway, but the values above should be correct.

Feel free to do the split into two files if you think that's the way to go. I'm going to submit my patch to the cpufreq list anyway under a separate email, just so it exists for the record. You might change your mind after seeing it. I also have a patch that fixes a few bugs in the current cpufreq/acpi.c.

Regards,

David

Sound like ok, but not for Dword 0. Will look more at it then.

Yes, I saw some of those bugs too. Will look at it a little bit more.

Note that I am not really against at, but I prefer to see at first all implications of supporting this method.

Ducrot Bruno

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.

Ok. Actually Dword 0 seems to be 0x0001. Sorry.

Ducrot Bruno

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.

This patch modifies the speedstep-centrino driver to use the ACPI processor perflib if no hardcoded value could be found. Centrino-specific stuff is taken from a patch to arch/i386/kernel/cpu/cpufreq/acpi.c by David Moore [in this thread].

 arch/i386/kernel/cpu/cpufreq/Kconfig              |   14 +++
 arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c |   88 ++++++++++++++++++----
2 files changed, 89 insertions(+), 13 deletions(-)

diff -ruN linux-original/arch/i386/kernel/cpu/cpufreq/Kconfig linux/arch/i386/kernel/cpu/cpufreq/Kconfig
@@ -113,6 +113,20 @@
 	  
 	  If in doubt, say N.
 
+config X86_SPEEDSTEP_CENTRINO_ACPI
+	bool "Fall back to ACPI tables info"
+	depends on X86_SPEEDSTEP_CENTRINO && ACPI_PROCESSOR_PERFLIB
+	default n
+	help
+	  If your Intel Pentium M (Centrino) CPU is so new that
+	  it cannot be found yet by the Intel Enhanced SpeedStep
+	  CPUfreq driver, you might want to try enabling this option.
+
+	  It reads out the ACPI tables trying to determine the correct
+	  settings.
+	  
+	  If in doubt, say N.
+
 config X86_SPEEDSTEP_LIB
        tristate
        depends on X86_SPEEDSTEP_ICH

Sorry, I haven't really been following this thread. What's the complete minimal set of other patches I need to make this one work? Are they against 2.5.0-test4?

Thanks,
J