RE: [ACPI] _PDC method in DSDT

RE: [ACPI] _PDC method in DSDT

[Grover, Andrew]

> From: David Moore [mailto:dcm-HInyCGIudOg@public.gmane.org] 
> Given this document, I think I have enough information now to patch
> processor.c to support Enhanced Speedstep on Pentium M for 
> those BIOSes
> that have a correctly written _PDC method in the DSDT.  Would the
> maintainers of the ACPI driver be willing to accept such a patch?

I don't think so, at least not right now.

_PDC is used for the OS to indicate to the firmware (the AML) if it
supports native CPU perf transitions or not. If it does, then _PCT/_PSS
returns one thing, if it doesn't, _PCT/_PSS returns something else.
Typically, if _PDC is not called, the platform will indicate ACPI 2.0
CPU perf state support instead of native support.

However, since the recently added non-vendor-supported Pentium M cpufreq
driver uses hardcoded tables and cpuid instead of relying on ACPI for
this information anyways, adding _PDC support to processor.c would not
affect its behavior. If that driver were changed to use ACPI for
determining platform capabilities, like Windows and our released binary
driver do, then yes, adding _PDC support would be important.

Regards -- Andy

btw my understanding is that _PDC will be actually in the ACPI spec in
the near future.

RE: [ACPI] _PDC method in DSDT

[David Moore]

On Mon, 2003-06-23 at 00:11, Grover, Andrew wrote:
> > From: David Moore [mailto:dcm-HInyCGIudOg@public.gmane.org] 
> > Given this document, I think I have enough information now to patch
> > processor.c to support Enhanced Speedstep on Pentium M for 
> > those BIOSes
> > that have a correctly written _PDC method in the DSDT.  Would the
> > maintainers of the ACPI driver be willing to accept such a patch?
> 
> I don't think so, at least not right now.
> 
> However, since the recently added non-vendor-supported Pentium M cpufreq
> driver uses hardcoded tables and cpuid instead of relying on ACPI for
> this information anyways, adding _PDC support to processor.c would not
> affect its behavior. If that driver were changed to use ACPI for
> determining platform capabilities, like Windows and our released binary
> driver do, then yes, adding _PDC support would be important.
> 

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.

If my understanding is correct, once _PDC is called, the only change
necessary to drive Enhanced Speedstep with P-states is simply to
interpret the registers returned by _PCT and if they are of type
FixedHardware, to use wrmsr() instead of outb().  I'm slightly
oversimplifying, but such an approach works on my system.

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

Thanks for your input,

David

Re: [ACPI] _PDC method in DSDT

[Ducrot Bruno]

On Mon, Jun 23, 2003 at 12:53:27AM -0700, David Moore wrote:
> On Mon, 2003-06-23 at 00:11, Grover, Andrew wrote:
> > > From: David Moore [mailto:dcm-HInyCGIudOg@public.gmane.org] 
> > > Given this document, I think I have enough information now to patch
> > > processor.c to support Enhanced Speedstep on Pentium M for 
> > > those BIOSes
> > > that have a correctly written _PDC method in the DSDT.  Would the
> > > maintainers of the ACPI driver be willing to accept such a patch?
> > 
> > I don't think so, at least not right now.
> > 
> > However, since the recently added non-vendor-supported Pentium M cpufreq
> > driver uses hardcoded tables and cpuid instead of relying on ACPI for
> > this information anyways, adding _PDC support to processor.c would not
> > affect its behavior. If that driver were changed to use ACPI for
> > determining platform capabilities, like Windows and our released binary
> > driver do, then yes, adding _PDC support would be important.
> > 
> 
> 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.
> 
> If my understanding is correct, once _PDC is called, the only change
> necessary to drive Enhanced Speedstep with P-states is simply to
> interpret the registers returned by _PCT and if they are of type
> FixedHardware, to use wrmsr() instead of outb().  I'm slightly
> oversimplifying, but such an approach works on my system.
> 
> 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?

For AMD athlons, most of the time, the returned values for _PCT
are FixedHardware address 0.  Therefore, this will not work (writing to
msr 0 will be probably 'interessting', but I doubt that will
change the freq/voltage for this cpu).

More, actually, Athlons with PowerNow have to write to 2 MSRs, not
only one.

You have to consider that all of this returned values after calling
the _PCT() and _PSS() methods, *when FixedHardware is returned by
_PCT*, have to be passed to an *external, per cpu, driver*.
No way to make the acpi processor generic, or you have to consider
all cases (PowerNow, LongRun, Enhanced SpeedStep, what ever) after
retrieving the correct cpu model.

And even in that situation, you are not assured that better
tables may be retrieven by other means (only 5 states by ACPI in
one of my laptops, a PB laptop, but 9 states by parsing a BIOS
table for an AMD mobile Athlon 2200+, which seems to be used
even by Windows OS, not ACPI).

-- 
Ducrot Bruno

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

Re: [ACPI] _PDC method in DSDT

[Dominik Brodowski]

On Mon, Jun 23, 2003 at 12:53:27AM -0700, David Moore wrote:
> 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.

Actually, in 2.5. ACPI provides one of several cpufreq drivers. The user can
choose which one best suits his needs [e.g. speedstep-centrino for the
faster wrmsr method instead of the slow io-based ACPI-2.0 module - 
arch/i386/cpu/cpufreq]. Also I doubt that ACPI will allow maximum
compatibility: this new _PCT method again tries to implement things
in a "proprietary", "legacy", "secret" way, supported only by
reverse-engineered or binary-only drivers.

	Dominik

RE: [ACPI] _PDC method in DSDT

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 559 bytes --]

On Mon, 2003-06-23 at 09:11, Grover, Andrew wrote:

> However, since the recently added non-vendor-supported Pentium M cpufreq
> driver uses hardcoded tables and cpuid instead of relying on ACPI for
> this information anyways, adding _PDC support to processor.c would not
> affect its behavior. If that driver were changed to use ACPI for
> determining platform capabilities, like Windows and our released binary
> driver do, then yes, adding _PDC support would be important.

Intel is more than welcome to release the linux driver under the GPL :)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [ACPI] _PDC method in DSDT

[Dominik Brodowski]

On Mon, Jun 23, 2003 at 12:11:49AM -0700, Grover, Andrew wrote:
> btw my understanding is that _PDC will be actually in the ACPI spec in
> the near future.

Oh no... a victory for legacy methods... I thought ACPI's philosophy was to
remove legacy code...

	Dominik

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.