ia64 acpi-cpufreq driver

ia64 acpi-cpufreq driver

[Bjorn Helgaas]

arch/ia64/kernel/cpufreq/acpi-cpufreq.c currently makes direct
calls to PAL_SET_PSTATE.

Section 8.4.4.1 (_PCT) of the 3.0 ACPI spec says:

  OSPM performs processor performance transitions by writing
  the performance state-specific control value to a Performance
  Control Register (PERF_CTRL).

According to one of our architecture guys, this means we really
ought to have an OpRegion driver that encapsulates the PAL_SET_PSTATE
call.

I guess _PCT would return an acpi_generic_address in the FFixedHW
address space, and we would use acpi_hw_low_level_read() and
acpi_hw_low_level_write() to do the accesses (those functions
would need to be extended to support additional OpRegions, or
maybe I'm looking in the wrong place).

This sort of makes sense to me, because it would mean this part
of acpi-cpufreq wouldn't need to be ia64-specific.  On x86, I
suppose _PCT would return a ACPI_ADR_SPACE_SYSTEM_IO address,
and acpi_processor_set_performance() could use acpi_hw_low_level_write()
instead of its own acpi_processor_write_port().

But I haven't seen anything resembling an ACPI FFH OpRegion
driver for Linux, or even a spec (e.g., a definition of what
the FFH address space contains) that would allow such a driver
to be written.

Any hints on what the future might hold in this area?

Bjorn

RE: ia64 acpi-cpufreq driver

[Pallipadi, Venkatesh]

 
>-----Original Message-----
>From: Bjorn Helgaas [mailto:bjorn.helgaas@hp.com] 
>Sent: Monday, October 23, 2006 2:31 PM
>To: Pallipadi, Venkatesh
>Cc: linux-acpi@vger.kernel.org; linux-ia64@vger.kernel.org
>Subject: ia64 acpi-cpufreq driver
>
>arch/ia64/kernel/cpufreq/acpi-cpufreq.c currently makes direct
>calls to PAL_SET_PSTATE.
>
>Section 8.4.4.1 (_PCT) of the 3.0 ACPI spec says:
>
>  OSPM performs processor performance transitions by writing
>  the performance state-specific control value to a Performance
>  Control Register (PERF_CTRL).
>
>According to one of our architecture guys, this means we really
>ought to have an OpRegion driver that encapsulates the PAL_SET_PSTATE
>call.
>

Actually it is slightly different from low_level_read and write. 
Generic ACPI definition of ACPI PERF_CTRL and PERF_STATUS define 
them as if they are registers. But, with FfixedHW, ACPI allows 
architectures to implement this functionality in a native way.
Just like x86 implements FfixedHW based P-state support in 16 bits 
of some known MSR (Note the register field itself in _PCT is not used)
or FFH C-states are supported by native instructions like "hlt",
"monitor-mwait".

So, when firmware tells P-state are FFH, OS will look at the hardware 
and processor information and use appropriate native interfaces. 
In this case, appropriate native interface is PAL_GET_PSTATE 
and PAL_SET_PSTATE.

>This sort of makes sense to me, because it would mean this part
>of acpi-cpufreq wouldn't need to be ia64-specific.  On x86, I
>suppose _PCT would return a ACPI_ADR_SPACE_SYSTEM_IO address,
>and acpi_processor_set_performance() could use 
>acpi_hw_low_level_write()
>instead of its own acpi_processor_write_port().
>
>But I haven't seen anything resembling an ACPI FFH OpRegion
>driver for Linux, or even a spec (e.g., a definition of what
>the FFH address space contains) that would allow such a driver
>to be written.
>
>Any hints on what the future might hold in this area?

FFH by nature is specific to hardware and we will have to 
have arch specific driver to handle things. However, 
At present there is a missing documentation to link this 
ACPI FFH to Itanium Software Developers Manual's PAL_GET_PSTATE
and PAL_SET_PSTATE. There is a document that is in the works
and should go online real soon.

Thanks,
Venki

Re: ia64 acpi-cpufreq driver

[Bjorn Helgaas]

On Monday 23 October 2006 22:46, Pallipadi, Venkatesh wrote:
> Actually it is slightly different from low_level_read and write. 
> Generic ACPI definition of ACPI PERF_CTRL and PERF_STATUS define 
> them as if they are registers. But, with FfixedHW, ACPI allows 
> architectures to implement this functionality in a native way.
> Just like x86 implements FfixedHW based P-state support in 16 bits 
> of some known MSR (Note the register field itself in _PCT is not used)
> or FFH C-states are supported by native instructions like "hlt",
> "monitor-mwait".
> 
> So, when firmware tells P-state are FFH, OS will look at the hardware 
> and processor information and use appropriate native interfaces. 
> In this case, appropriate native interface is PAL_GET_PSTATE 
> and PAL_SET_PSTATE.

Clearly ia64 ultimately has to use PAL_SET_PSTATE.  My question
is, does the PAL_SET_PSTATE call belong in acpi-cpufreq, or does
it belong in the FFH driver?

I think it belongs in the latter, because the OSPM can be more
generic if the architecture-specific stuff is in the FFH driver.

Another way to ask this is, if ia64 had an FFH driver, who would
use it?  I assume acpi-cpufreq would be one user.  If so, what
interface would acpi-cpufreq use to access FFH?

Bjorn

RE: ia64 acpi-cpufreq driver

[Pallipadi, Venkatesh]

 

>-----Original Message-----
>From: Bjorn Helgaas [mailto:bjorn.helgaas@hp.com] 
>Sent: Tuesday, October 24, 2006 10:51 AM
>To: Pallipadi, Venkatesh
>Cc: linux-acpi@vger.kernel.org; linux-ia64@vger.kernel.org
>Subject: Re: ia64 acpi-cpufreq driver
>
>On Monday 23 October 2006 22:46, Pallipadi, Venkatesh wrote:
>> Actually it is slightly different from low_level_read and write. 
>> Generic ACPI definition of ACPI PERF_CTRL and PERF_STATUS define 
>> them as if they are registers. But, with FfixedHW, ACPI allows 
>> architectures to implement this functionality in a native way.
>> Just like x86 implements FfixedHW based P-state support in 16 bits 
>> of some known MSR (Note the register field itself in _PCT is 
>not used)
>> or FFH C-states are supported by native instructions like "hlt",
>> "monitor-mwait".
>> 
>> So, when firmware tells P-state are FFH, OS will look at the 
>hardware 
>> and processor information and use appropriate native interfaces. 
>> In this case, appropriate native interface is PAL_GET_PSTATE 
>> and PAL_SET_PSTATE.
>
>Clearly ia64 ultimately has to use PAL_SET_PSTATE.  My question
>is, does the PAL_SET_PSTATE call belong in acpi-cpufreq, or does
>it belong in the FFH driver?
>
>I think it belongs in the latter, because the OSPM can be more
>generic if the architecture-specific stuff is in the FFH driver.
>
>Another way to ask this is, if ia64 had an FFH driver, who would
>use it?  I assume acpi-cpufreq would be one user.  If so, what
>interface would acpi-cpufreq use to access FFH?
>

Yes. As it is today, acpi-cpufreq contains FFH driver functionality 
in itself. It calls acpi and cpufreq arch-independent interfaces but 
implements FFH internally. 

By, FFH driver do you mean to handle FFH related functions only for 
P-states or something that can handle all FFH functions (like C-states,
etc). 
I don't see any other kernel part/or driver will use this particular 
PAL_GET_PSTATE and PAL_SET_PSTATE intefaces. So, my feeling is we don't
need a separate FFH driver.

Thanks,
Venki

RE: ia64 acpi-cpufreq driver

[Bjorn Helgaas]

> Yes. As it is today, acpi-cpufreq contains FFH driver functionality
> in itself. It calls acpi and cpufreq arch-independent interfaces but
> implements FFH internally.
>
> By, FFH driver do you mean to handle FFH related functions only for
> P-states or something that can handle all FFH functions (like C-states,
> etc).
> I don't see any other kernel part/or driver will use this particular
> PAL_GET_PSTATE and PAL_SET_PSTATE intefaces. So, my feeling is we don't
> need a separate FFH driver.

The only reason I can think of to have a separate FFH driver
would be that we could potentially combine parts of the x86
acpi-cpufreq and the ia64 acpi-cpufreq.

I didn't look closely, but the x86 and ia64 acpi-cpufreq drivers
looked pretty different, even apart from the FFH stuff, so maybe
that wouldn't be feasible.

Bjorn

Re: ia64 acpi-cpufreq driver

[Bjorn Helgaas]

On Monday 23 October 2006 22:46, Pallipadi, Venkatesh wrote:
> >Section 8.4.4.1 (_PCT) of the 3.0 ACPI spec says:
> >
> >  OSPM performs processor performance transitions by writing
> >  the performance state-specific control value to a Performance
> >  Control Register (PERF_CTRL).
> >
> >According to one of our architecture guys, this means we really
> >ought to have an OpRegion driver that encapsulates the PAL_SET_PSTATE
> >call.
> 
> Actually it is slightly different from low_level_read and write. 
> Generic ACPI definition of ACPI PERF_CTRL and PERF_STATUS define 
> them as if they are registers. But, with FfixedHW, ACPI allows 
> architectures to implement this functionality in a native way.

I'd like to understand this better.  Something like the following
patch (not for inclusion, may not even compile) is what I'm
thinking.  This seems more in line with the spec intent.

Apart from details like "bit_width <= 8" vs. "bit_width = 8",
this should be functionally the same.  Are you saying it's different
because of those details, or is there a larger difference I don't
understand?

Nacked-by: Bjorn Helgaas

Index: work-1/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c
=================================--- work-1.orig/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c	2006-10-25 17:04:46.000000000 -0600
+++ work-1/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c	2006-10-25 17:07:34.000000000 -0600
@@ -61,55 +61,16 @@
 static unsigned int acpi_pstate_strict;
 
 static int
-acpi_processor_write_port(
-	u16	port,
-	u8	bit_width,
-	u32	value)
-{
-	if (bit_width <= 8) {
-		outb(value, port);
-	} else if (bit_width <= 16) {
-		outw(value, port);
-	} else if (bit_width <= 32) {
-		outl(value, port);
-	} else {
-		return -ENODEV;
-	}
-	return 0;
-}
-
-static int
-acpi_processor_read_port(
-	u16	port,
-	u8	bit_width,
-	u32	*ret)
-{
-	*ret = 0;
-	if (bit_width <= 8) {
-		*ret = inb(port);
-	} else if (bit_width <= 16) {
-		*ret = inw(port);
-	} else if (bit_width <= 32) {
-		*ret = inl(port);
-	} else {
-		return -ENODEV;
-	}
-	return 0;
-}
-
-static int
 acpi_processor_set_performance (
 	struct cpufreq_acpi_io	*data,
 	unsigned int		cpu,
 	int			state)
 {
-	u16			port = 0;
-	u8			bit_width = 0;
 	int			i = 0;
-	int			ret = 0;
 	u32			value = 0;
 	int			retval;
 	struct acpi_processor_performance	*perf;
+	acpi_status		status;
 
 	dprintk("acpi_processor_set_performance\n");
 
@@ -132,16 +93,16 @@
 	 * control_register.
 	 */
 
-	port = perf->control_register.address;
-	bit_width = perf->control_register.bit_width;
 	value = (u32) perf->states[state].control;
 
-	dprintk("Writing 0x%08x to port 0x%04x\n", value, port);
+	dprintk("Writing 0x%08x to PERF_CTRL\n", value);
 
-	ret = acpi_processor_write_port(port, bit_width, value);
-	if (ret) {
-		dprintk("Invalid port width 0x%04x\n", bit_width);
-		return (ret);
+	status = acpi_hw_low_level_write(perf->control_register.bit_width,
+		value, perf->control_register);
+
+	if (ACPI_FAILURE(status)) {
+		dprintk("Failure writing PERF_CTRL\n");
+		return -ENODEV;
 	}
 
 	/*
@@ -157,17 +118,15 @@
 		 * before giving up.
 		 */
 
-		port = perf->status_register.address;
-		bit_width = perf->status_register.bit_width;
-
-		dprintk("Looking for 0x%08x from port 0x%04x\n",
-			(u32) perf->states[state].status, port);
+		dprintk("Looking for 0x%08x from PERF_CTRL\n",
+			(u32) perf->states[state].status);
 
 		for (i = 0; i < 100; i++) {
-			ret = acpi_processor_read_port(port, bit_width, &value);
-			if (ret) {	
-				dprintk("Invalid port width 0x%04x\n", bit_width);
-				return (ret);
+			status = acpi_hw_low_level_read(perf->control_register.bit_width,
+				&value, perf->control_register);
+			if (ACPI_FAILURE(status)) {
+				dprintk("Failure reading PERF_CTRL\n");
+				return -ENODEV;
 			}
 			if (value = (u32) perf->states[state].status)
 				break;
@@ -473,15 +432,6 @@
 		goto err_unreg;
 	}
 
-	if ((perf->control_register.space_id != ACPI_ADR_SPACE_SYSTEM_IO) ||
-	    (perf->status_register.space_id != ACPI_ADR_SPACE_SYSTEM_IO)) {
-		dprintk("Unsupported address space [%d, %d]\n",
-			(u32) (perf->control_register.space_id),
-			(u32) (perf->status_register.space_id));
-		result = -ENODEV;
-		goto err_unreg;
-	}
-
 	/* alloc freq_table */
 	data->freq_table = kmalloc(sizeof(struct cpufreq_frequency_table) * (perf->state_count + 1), GFP_KERNEL);
 	if (!data->freq_table) {

RE: ia64 acpi-cpufreq driver

[Pallipadi, Venkatesh]

 

>-----Original Message-----
>From: Bjorn Helgaas [mailto:bjorn.helgaas@hp.com] 
>Sent: Wednesday, October 25, 2006 4:21 PM
>To: Pallipadi, Venkatesh
>Cc: linux-acpi@vger.kernel.org; linux-ia64@vger.kernel.org
>Subject: Re: ia64 acpi-cpufreq driver
>
>On Monday 23 October 2006 22:46, Pallipadi, Venkatesh wrote:
>> >Section 8.4.4.1 (_PCT) of the 3.0 ACPI spec says:
>> >
>> >  OSPM performs processor performance transitions by writing
>> >  the performance state-specific control value to a Performance
>> >  Control Register (PERF_CTRL).
>> >
>> >According to one of our architecture guys, this means we really
>> >ought to have an OpRegion driver that encapsulates the 
>PAL_SET_PSTATE
>> >call.
>> 
>> Actually it is slightly different from low_level_read and write. 
>> Generic ACPI definition of ACPI PERF_CTRL and PERF_STATUS define 
>> them as if they are registers. But, with FfixedHW, ACPI allows 
>> architectures to implement this functionality in a native way.
>
>I'd like to understand this better.  Something like the following
>patch (not for inclusion, may not even compile) is what I'm
>thinking.  This seems more in line with the spec intent.
>
>Apart from details like "bit_width <= 8" vs. "bit_width = 8",
>this should be functionally the same.  Are you saying it's different
>because of those details, or is there a larger difference I don't
>understand?
>

Yes. Something like this will work. I don't think it is the intent of
the SPEC. As I understand, FFH in SPEC is intended to be used whenever
we have hardware/processor specific interfaces (As described in ACPI 3.0
spec - page 46). Also, it will complicate the code. I think having
separate code, even if there is some amount of duplication is there, is
better in this case. The reasons:
- i386 acpi-cpufreq code has support for both IO port and FFH. Please
refer to arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c in 2.6.19-rc2-mm2,
which is a lot different from current git.
 
- FFH in i386 works in a different way from IPF. Specifically,
PERF_STATUS is just a register read, and hardware performance feedback
is by using different APERF/MPERF MSR. But, in IPF it is done through
passing a different type parameter to same PAL_GET_PSTATE calls (will be
adding this to IPF driver soon).

Thanks,
Venki

Re: ia64 acpi-cpufreq driver

[Bjorn Helgaas]

On Wednesday 25 October 2006 22:36, Pallipadi, Venkatesh wrote:
> Yes. Something like this will work. I don't think it is the intent of
> the SPEC. As I understand, FFH in SPEC is intended to be used whenever
> we have hardware/processor specific interfaces (As described in ACPI 3.0
> spec - page 46). Also, it will complicate the code.

It sure looks like the intent to me.  Why would you bother with
the concept of FFH at all unless it provided some abstraction?
Abstraction can help simplify the code: you can use the same
code for multiple architectures, with differences encapsulated
in the per-arch FFH driver.

> I think having 
> separate code, even if there is some amount of duplication is there, is
> better in this case. The reasons:
> - i386 acpi-cpufreq code has support for both IO port and FFH. Please
> refer to arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c in 2.6.19-rc2-mm2,
> which is a lot different from current git.
> - FFH in i386 works in a different way from IPF. Specifically,
> PERF_STATUS is just a register read, and hardware performance feedback
> is by using different APERF/MPERF MSR. But, in IPF it is done through
> passing a different type parameter to same PAL_GET_PSTATE calls (will be
> adding this to IPF driver soon).

Well, of course FFH would be *implemented* differently on x86
than ia64!  But the layout of the FFH address space, which
essentially determines the semantics of FFH, should be the
same for all architectures (though somebody seems to have
forgotten to specify that layout).

There's nothing really x86-specific about i386/.../acpi-cpufreq,
except for the whole mess about supporting I/O ports, MSRs, etc.
If we made an x86 FFH OpRegion driver that knew how to do the MSR
stuff, we could probably use the same acpi-cpufreq driver across
x86 and ia64.

RE: ia64 acpi-cpufreq driver

[Pallipadi, Venkatesh]

 

>-----Original Message-----
>From: Bjorn Helgaas [mailto:bjorn.helgaas@hp.com] 
>Sent: Thursday, October 26, 2006 8:30 AM
>To: Pallipadi, Venkatesh
>Cc: linux-acpi@vger.kernel.org; linux-ia64@vger.kernel.org
>Subject: Re: ia64 acpi-cpufreq driver
>> I think having 
>> separate code, even if there is some amount of duplication 
>is there, is
>> better in this case. The reasons:
>> - i386 acpi-cpufreq code has support for both IO port and FFH. Please
>> refer to arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c in 
>2.6.19-rc2-mm2,
>> which is a lot different from current git.
>> - FFH in i386 works in a different way from IPF. Specifically,
>> PERF_STATUS is just a register read, and hardware 
>performance feedback
>> is by using different APERF/MPERF MSR. But, in IPF it is done through
>> passing a different type parameter to same PAL_GET_PSTATE 
>calls (will be
>> adding this to IPF driver soon).
>
>Well, of course FFH would be *implemented* differently on x86
>than ia64!  But the layout of the FFH address space, which
>essentially determines the semantics of FFH, should be the
>same for all architectures (though somebody seems to have
>forgotten to specify that layout).
>

What I meant is, way FFH address space is defined and used is not
totally 
same across i386 and ia64. Specifically, FFH is only used in definition
of 
_PCT status and control objects and a FFH driver can be useful there.
But,
Hardware feedback of current/average frequency over a period of time 
(which does not have FFH object in ACPI) can also use some interface 
which overlap with interface that FFH driver is using. In the sense that
There is no clear distinction on the scope of this FFH driver.
Also i386 uses FFH for both C-states and P-states where as IA64 uses it
only for P-states today.

All this issues can be resolved once we have a definition of this FFH
scope.
I guess we need to revisit once we have the FFH related documentation
for IA64
(which should happen soon).

But, having seen the undue complications we are having in i386 world
with 
different drivers (SYSTEM_IO and FFH drivers) and stuff my feeling at
this
point is, it will just add more overhead due to abstraction than the
commonality in code that it is going to bring. I mean, all the cpufreq
and
ACPI code are already getting shared across architectures today. So,
only 
thing this new abstraction is going to make common across archs is
registering and deregistering with ACPI and cpufreq.


Thanks,
Venki