RE: [ACPI] _PDC method in DSDT

RE: [ACPI] _PDC method in DSDT

[Grover, Andrew]

> From: David Moore [mailto:dcm-HInyCGIudOg@public.gmane.org] 
> My intention was not for processor.c and cpufreq to cooperate, but
> rather for processor.c to be able to control Enhanced Speedstep solely
> with P-states -- no cpufreq necessary.  Cpufreq is certainly 
> useful for
> those systems not running the ACPI driver, but in the long 
> run, ACPI and
> processor.c seem to be the right way to get maximum compatibility with
> many platforms.

> I only have two concerns:
> 1) If the processor is an AMD or a Transmeta, it looks like _PCT may
> also return FixedHardware registers for their respective power
> management systems.  Do we need to read cpuid to treat these cases
> differently in processor.c, or will the blind wrmsr() do the trick?

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.

> 2) If this capability is open-source, do you personally have 
> a conflict
> of interest between protecting the binary-only module vs. 
> accepting the
> patch?

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

Re: [ACPI] _PDC method in DSDT

[Dominik Brodowski]

On Mon, Jun 23, 2003 at 02:19:27AM -0700, Grover, Andrew wrote:
> 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.

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

Re: [ACPI] _PDC method in DSDT

[Ducrot Bruno]

On Mon, Jun 23, 2003 at 03:38:34PM +0200, Dominik Brodowski wrote:
> On Mon, Jun 23, 2003 at 02:19:27AM -0700, Grover, Andrew wrote:
> > 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.
> 
> 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.
> 

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.

Re: [ACPI] _PDC method in DSDT

[Jeremy Fitzhardinge]

On Mon, 2003-06-23 at 09:11, Ducrot Bruno wrote:
> Yes.  It can be added.  But do we want to add it?  That's the real
> question.

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

Re: [ACPI] _PDC method in DSDT

[David Moore]

On Mon, 2003-06-23 at 10:12, Jeremy Fitzhardinge wrote:
> On Mon, 2003-06-23 at 09:11, Ducrot Bruno wrote:
> > Yes.  It can be added.  But do we want to add it?  That's the real
> > question.
> 
> 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.

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

Re: [ACPI] _PDC method in DSDT

[Dominik Brodowski]

On Mon, Jun 23, 2003 at 01:02:25PM -0700, David Moore wrote:
> 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.

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

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

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.

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

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

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.

Re: [ACPI] _PDC method in DSDT

[David Moore]

On Mon, 2003-06-23 at 14:37, Dominik Brodowski wrote:
> 
> Firstly, please let me explain the goal of the cpufreq project: it is a
> cross-architecture, _generic framework_ which provides

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.

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

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

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

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

Re: [ACPI] _PDC method in DSDT

[Jeremy Fitzhardinge]

On Mon, 2003-06-23 at 18:14, David Moore wrote:
> 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.

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.

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

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

Re: [ACPI] _PDC method in DSDT

[David Moore]

On Mon, 2003-06-23 at 18:37, Jeremy Fitzhardinge wrote:
> 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.
> 

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?

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

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

Re: [ACPI] _PDC method in DSDT

[Jeremy Fitzhardinge]

On Mon, 2003-06-23 at 19:02, David Moore wrote:
> On Mon, 2003-06-23 at 18:37, Jeremy Fitzhardinge wrote:
> > 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.
> > 
> 
> 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?

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

Re: [ACPI] _PDC method in DSDT

[Ducrot Bruno]

On Mon, Jun 23, 2003 at 07:02:29PM -0700, David Moore wrote:
> On Mon, 2003-06-23 at 18:37, Jeremy Fitzhardinge wrote:
> > 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.
> > 
> 
> 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?
>
> > 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.
> > 
> 
> 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.
> 

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.

Re: [ACPI] _PDC method in DSDT

[David Moore]

On Tue, 2003-06-24 at 02:18, Ducrot Bruno wrote:
> 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.
> 

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

Re: [ACPI] _PDC method in DSDT

[Ducrot Bruno]

On Tue, Jun 24, 2003 at 02:47:52PM -0700, David Moore wrote:
> On Tue, 2003-06-24 at 02:18, Ducrot Bruno wrote:
> > 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.
> > 
> 
> 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.

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

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

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.

Re: [ACPI] _PDC method in DSDT

[Ducrot Bruno]

> > 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.
> 
> Sound like ok, but not for Dword 0.  Will look more at it then.

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

-- 
Ducrot Bruno

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

[RFC] cpufreq: use new acpi processor perflib in speedstep-centrino [Was: Re: [ACPI] _PDC method in DSDT]

[Dominik Brodowski]

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
--- linux-original/arch/i386/kernel/cpu/cpufreq/Kconfig	2003-09-04 21:36:58.000000000 +0200
+++ linux/arch/i386/kernel/cpu/cpufreq/Kconfig	2003-09-05 01:13:44.374639488 +0200
@@ -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
diff -ruN linux-original/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c linux/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c
--- linux-original/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c	2003-09-04 20:52:48.000000000 +0200
+++ linux/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c	2003-09-05 01:13:55.006023272 +0200
@@ -37,6 +37,7 @@
 #define dprintk(msg...) do { } while(0)
 #endif
 
+
 struct cpu_model
 {
 	const char	*model_name;
@@ -46,7 +47,7 @@
 };
 
 /* Operating points for current CPU */
-static const struct cpu_model *centrino_model;
+static struct cpufreq_frequency_table *centrino_table;
 
 /* Computes the correct form for IA32_PERF_CTL MSR for a particular
    frequency/voltage operating point; frequency in MHz, volts in mV.
@@ -196,7 +197,7 @@
 {
 	unsigned freq;
 
-	if (policy->cpu != 0 || centrino_model == NULL)
+	if (policy->cpu != 0 || centrino_table == NULL)
 		return -ENODEV;
 
 	freq = get_cur_freq();
@@ -207,8 +208,8 @@
 
 	dprintk(KERN_INFO PFX "centrino_cpu_init: policy=%d cur=%dkHz\n",
 		policy->policy, policy->cur);
-	
-	return cpufreq_frequency_table_cpuinfo(policy, centrino_model->op_points);
+
+	return cpufreq_frequency_table_cpuinfo(policy, centrino_table);
 }
 
 /**
@@ -220,7 +221,7 @@
  */
 static int centrino_verify (struct cpufreq_policy *policy)
 {
-	return cpufreq_frequency_table_verify(policy, centrino_model->op_points);
+	return cpufreq_frequency_table_verify(policy, centrino_table);
 }
 
 /**
@@ -237,14 +238,14 @@
 	unsigned int	msr, oldmsr, h;
 	struct cpufreq_freqs	freqs;
 
-	if (centrino_model == NULL)
+	if (centrino_table == NULL)
 		return -ENODEV;
 
-	if (cpufreq_frequency_table_target(policy, centrino_model->op_points, target_freq,
+	if (cpufreq_frequency_table_target(policy, centrino_table, target_freq,
 					   relation, &newstate))
 		return -EINVAL;
 
-	msr = centrino_model->op_points[newstate].index;
+	msr = centrino_table[newstate].index;
 	rdmsr(MSR_IA32_PERF_CTL, oldmsr, h);
 
 	if (msr == (oldmsr & 0xffff))
@@ -293,6 +294,63 @@
 	.owner		= THIS_MODULE,
 };
 
+#ifdef CONFIG_X86_SPEEDSTEP_CENTRINO_ACPI
+#include <linux/acpi.h>
+#include <acpi/processor.h>
+
+#define ACPI_PDC_REVISION_ID			0x1
+#define ACPI_PDC_CAPABILITY_ENHANCED_SPEEDSTEP	0x1
+
+static struct cpufreq_frequency_table * get_frequency_table_from_acpi(void) 
+{
+	union acpi_object		arg0 = {ACPI_TYPE_BUFFER};
+	u32				arg0_buf[3];
+	struct acpi_object_list		arg_list = {1, &arg0};
+	struct acpi_perflib_driver	perflib_driver;
+	struct acpi_perflib_data 	*data;
+	struct cpufreq_frequency_table	*table = NULL;
+	int				i;
+
+	/* First call _PDC, if available, to indicate to ACPI that this
+	 * driver supports Enhanced Speedstep.
+	 * Documentation for the _PDC method is at:
+	 * http://www.microsoft.com/whdc/hwdev/tech/onnow/procperfctrl.mspx */
+	arg0.buffer.length = 12;
+	arg0.buffer.pointer = (u8 *) arg0_buf;
+	arg0_buf[0] = ACPI_PDC_REVISION_ID;
+	arg0_buf[1] = 1;
+	arg0_buf[2] = ACPI_PDC_CAPABILITY_ENHANCED_SPEEDSTEP;
+
+	perflib_driver.pdc = &arg_list;
+
+	data = acpi_processor_perflib_register(&perflib_driver, 0);
+	if (!data)
+		return NULL;
+
+	if ((data->state_count < 2) ||
+	    (data->pct_status.space_id != ACPI_ADR_SPACE_FIXED_HARDWARE) ||
+	    (data->pct_control.space_id != ACPI_ADR_SPACE_FIXED_HARDWARE) ||
+	    (data->pct_status.address != MSR_IA32_PERF_STATUS) ||
+	    (data->pct_control.address != MSR_IA32_PERF_CTL))
+		goto out;
+
+	table = kmalloc(sizeof(struct cpufreq_frequency_table) * (data->state_count + 1), GFP_KERNEL);
+	if (!table)
+		goto out;
+
+	for (i=0; i < data->state_count; i++) {
+		table[i].index = data->states[i].control;
+		table[i].frequency = data->states[i].core_frequency * 1000;
+	}
+        table[data->state_count].frequency = CPUFREQ_TABLE_END;
+
+ out:
+	acpi_processor_perflib_unregister(&perflib_driver, 0);
+	return table;
+
+}
+#endif
+
 
 /**
  * centrino_init - initializes the Enhanced SpeedStep CPUFreq driver
@@ -348,15 +406,19 @@
 	for(model = models; model->model_name != NULL; model++)
 		if (strcmp(cpu->x86_model_id, model->model_name) == 0)
 			break;
-	if (model->model_name == NULL) {
-		printk(KERN_INFO PFX "no support for CPU model \"%s\": "
+	if (model->model_name) {
+		centrino_table = model->op_points;
+	} else if (model->model_name == NULL) {
+		printk(KERN_INFO PFX "no hardcoded support for CPU model \"%s\": "
 		       "send /proc/cpuinfo to " MAINTAINER "\n",
 		       cpu->x86_model_id);
-		return -ENOENT;
+#ifdef CONFIG_X86_SPEEDSTEP_CENTRINO_ACPI
+		centrino_table = get_frequency_table_from_acpi();
+		if (!centrino_table)
+#endif
+			return -ENOENT;
 	}
 
-	centrino_model = model;
-		
 	printk(KERN_INFO PFX "found \"%s\": max frequency: %dkHz\n",
 	       model->model_name, model->max_freq);

Re: [RFC] cpufreq: use new acpi processor perflib in speedstep-centrino [Was: Re: [ACPI] _PDC method in DSDT]

[Jeremy Fitzhardinge]

On Thu, 2003-09-04 at 16:19, Dominik Brodowski wrote:
> 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].

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