On Friday, September 07, 2007 8:54 PM, Mark.Haywood at Sun.COM wrote:
> Hi Aubrey,
>
> See comments inline ...
>
> Li, Aubrey wrote:
>> Hi,
>>
>> You may noticed this issue was reported many times on the mailing
>> list.
>>
>> I recently work on the speedstep driver and found the root cause.
>>
>> 1) Processor supports SpeedStep feature, and the ACPI table offered
>> by BIOS has already included SSDT table, which includes processor
>> configuration objects like _PSS as well. The currect driver works
>> properly on this kind of machine.
>>
>> 2) Processor supports SpeedStep feature, and the ACPI table offered
>> by BIOS has included SSDT table, but this table doesn't include
>> processor configuration objects. This causes _PSS not found warning.
>> Actually, _PSS object exists in the processor configuration table,
>> which will be loaded by evaluating _PDC methods. The current driver
>> DOES call _PDC Methods, but the passed parameter is wrong. The
>> following patch fixed this issue. ( the patch is attached in case of
>> whitespace coding style issue).
>>
>
> You aren't looking at the latest source. I've recently
> implemented this
> fix (about 2 weeks ago), sans the OR'ing of the ESS_PDC_C1_FFH
> bit which
> I don't think makes any sense for P-states since it's described in the
> Intel Processor Vendor-Specific ACPI Interface Specification this way:
>
> "If set, OSPM supports the C1 "I/O then Halt" FFH sequence for
> multi-processor configurations"
>
> Can you explain why that bit should be enabled? If it really does need
> to be enabled, then I think we should request that Intel should
> update the Interface Specification.
Yes, I got the latest source now. I should update my working copy from
time to time.
As for this fix, I thnk the following patch is better to explain my
thoughts.
In case we support C state driver in future, It's better we call _PDC
method in cpu_acpi driver.
And IMHO, we should try to enable the all available capability when
evaluating _PDC.
Rename "cpu_acpi_write_pdc" to "cpu_acpi_init_pdc" is another change.
According to the spec, initialize PDC may be better, ;-)
At least, we should put the PDC bits definitions into cpu_acpi.h. It
shouldn't belong to speedstep only.
What's your thoughts?
------------------------------------------------------------------------
--
diff -r 304eb1332eef usr/src/uts/i86pc/io/cpu_acpi.c
--- a/usr/src/uts/i86pc/io/cpu_acpi.c Thu Sep 06 19:30:39 2007 -0700
+++ b/usr/src/uts/i86pc/io/cpu_acpi.c Mon Sep 10 03:35:54 2007 +0800
@@ -37,6 +37,7 @@ cpu_acpi_init(dev_info_t *dip)
cpu_acpi_init(dev_info_t *dip)
{
cpu_acpi_handle_t handle;
+ uint32_t pdc_cap;
handle = kmem_zalloc(sizeof (cpu_acpi_state_t), KM_SLEEP);
@@ -45,6 +46,13 @@ cpu_acpi_init(dev_info_t *dip)
return (NULL);
}
handle->cs_dip = dip;
+
+ pdc_cap = CPU_ACPI_PDC_MSR | CPU_ACPI_PDC_MP_C1_FFH |
+ CPU_ACPI_PDC_MP_C1PT | CPU_ACPI_PDC_MP_PSD |
+ CPU_ACPI_PDC_MP_CSD | CPU_ACPI_PDC_MP_C1;
+
+ if (cpu_acpi_init_pdc(handle, CPU_ACPI_PDC_REVISION, 1,
&pdc_cap) != 0)
+ cmn_err(CE_NOTE,"Failed to write PDC");
return (handle);
}
@@ -416,7 +424,7 @@ cpu_acpi_install_ppc_handler(cpu_acpi_ha
* Write _PDC.
*/
int
-cpu_acpi_write_pdc(cpu_acpi_handle_t handle, uint32_t revision,
uint32_t count,
+cpu_acpi_init_pdc(cpu_acpi_handle_t handle, uint32_t revision, uint32_t
count,
uint32_t *capabilities)
{
ACPI_OBJECT obj;
diff -r 304eb1332eef usr/src/uts/i86pc/io/speedstep.c
--- a/usr/src/uts/i86pc/io/speedstep.c Thu Sep 06 19:30:39 2007 -0700
+++ b/usr/src/uts/i86pc/io/speedstep.c Mon Sep 10 03:29:30 2007 +0800
@@ -51,16 +51,6 @@
#define ESS_LATENCY_WAIT 100
/*
- * The SpeedStep related Processor Driver Capabilities (_PDC).
- * See Intel Processor Vendor-Specific ACPI Interface Specification
- * for details.
- */
-#define ESS_PDC_REVISION 0x1
-#define ESS_PDC_PS_MSR (1<<0)
-#define ESS_PDC_MP (1<<3)
-#define ESS_PDC_PSD (1<<5)
-
-/*
* MSR registers for changing and reading processor power state.
*/
#define IA32_PERF_STATCPUDRV_MSR 0x198
@@ -83,8 +73,6 @@ typedef struct speedstep_state {
typedef struct speedstep_state {
uint32_t ss_state;
} speedstep_state_t;
-
-uint32_t ess_pdccap = ESS_PDC_PS_MSR | ESS_PDC_MP | ESS_PDC_PSD;
/*
* Read the status register. How it is read, depends upon the _PCT
@@ -202,7 +190,6 @@ speedstep_pstate_transition(int *ret, cp
if (i >= ESS_MAX_LATENCY_MICROSECS) {
DTRACE_PROBE(ess_transition_incomplete);
}
-
speedstep_state->ss_state = req_state;
CPU->cpu_curr_clock =
(((uint64_t)CPU_ACPI_FREQ(req_pstate) * 1000000));
@@ -296,14 +283,6 @@ speedstep_init(cpudrv_devstate_t *cpudsp
return (ESS_RET_NO_PM);
}
- /*
- * _PDC support is optional and the driver should
- * function even if the _PDC write fails.
- */
- if (cpu_acpi_write_pdc(handle, ESS_PDC_REVISION, 1,
- &ess_pdccap) != 0)
- ESSDEBUG(("Failed to write PDC\n"));
-
if (cpu_acpi_cache_data(handle) != 0) {
ESSDEBUG(("Failed to cache ACPI data\n"));
cpu_acpi_fini(handle);
diff -r 304eb1332eef usr/src/uts/i86pc/sys/cpu_acpi.h
--- a/usr/src/uts/i86pc/sys/cpu_acpi.h Thu Sep 06 19:30:39 2007 -0700
+++ b/usr/src/uts/i86pc/sys/cpu_acpi.h Mon Sep 10 03:36:29 2007 +0800
@@ -59,6 +59,21 @@ extern "C" {
#define CPU_ACPI_IS_OBJ_CACHED(sp, obj) (sp->cpu_acpi_cached &
obj)
#define CPU_ACPI_OBJ_IS_CACHED(sp, obj) (sp->cpu_acpi_cached |=
obj)
#define CPU_ACPI_OBJ_IS_NOT_CACHED(sp, obj) (sp->cpu_acpi_cached
&= ~obj)
+
+/*
+ * Bit and revision definitions for _PDC
+ */
+#define CPU_ACPI_PDC_REVISION 0x1
+
+#define CPU_ACPI_PDC_MSR (1 << 0)
+#define CPU_ACPI_PDC_MP_C1_FFH (1 << 1)
+#define CPU_ACPI_PDC_T (1 << 2)
+#define CPU_ACPI_PDC_MP_C1PT (1 << 3)
+#define CPU_ACPI_PDC_MP_C2C3 (1 << 4)
+#define CPU_ACPI_PDC_MP_PSD (1 << 5)
+#define CPU_ACPI_PDC_MP_CSD (1 << 6)
+#define CPU_ACPI_PDC_MP_TSD (1 << 7)
+#define CPU_ACPI_PDC_MP_C1 (1 << 8)
/*
* Container for _PSD information
@@ -128,7 +143,7 @@ extern int cpu_acpi_cache_data(cpu_acpi_
extern int cpu_acpi_cache_data(cpu_acpi_handle_t);
extern void cpu_acpi_install_ppc_handler(cpu_acpi_handle_t,
ACPI_NOTIFY_HANDLER, dev_info_t *);
-extern int cpu_acpi_write_pdc(cpu_acpi_handle_t, uint32_t, uint32_t,
+extern int cpu_acpi_init_pdc(cpu_acpi_handle_t, uint32_t, uint32_t,
uint32_t *);
extern int cpu_acpi_write_port(ACPI_IO_ADDRESS, uint32_t, uint32_t);
extern int cpu_acpi_read_port(ACPI_IO_ADDRESS, uint32_t *, uint32_t);
------------------------------------------------------------------------
--------------------------
>
>>
> ---------------------------------------------------------------
> ---------
>> ----------
>> In addition, acpica building cpu map function assumes there is only
>> one processor type object under _PR_ object. That causes the
>> processor handler is always mapped to the last Processor Type
>> object. This is not correct on case 2) machine, it will cause to
>> give a wrong handler to evaluate _PDC object. That's another reason
>> that _PSS not found. Here is the patch(it's attached as well in case
>> of whitespace coding style issue),
>>
>
> I'll have to look at this one some more. I don't understand you
> explanation and I don't understand the proposed fix. Are you sure
> that you didn't mean
>
> if (cpu_map[cpu_id]->obj != NULL)
> cpu_map[cpu_id]->obj = obj;
>
Two kinds of processor configuration machines in my hand.
1) _PR_ has only one child CPU0, which is ok.
CPU0 is _PDC's parent, and it will be mapped to the cpu acpi handler.
So when passed the handler to evaluate _PDC, it will be sucessful.
Level = 1, parent = \___ child = _PR_
Level = 2, parent = _PR_ child = CPU0
Level = 3, parent = CPU0 child = _PPC
Level = 3, parent = CPU0 child = _PCT
Level = 3, parent = CPU0 child = _PSS
Level = 3, parent = CPU0 child = SPSS
Level = 3, parent = CPU0 child = NPSS
Level = 3, parent = CPU0 child = _PDC
2) _PR_ has two child: P001 and CPU1. and the last object CPU1 is always
mapped to be the cpu acpi handler.
However, P001 which is the parent of _PDC, should be the cpu acpi
handler.
We should always use the first one. So when **cpu_map[cpu_id]->obj ==
NULL **, we can assign **obj** to it.
Otherwise the first matched handler will be overwrite by the last one.
And the wrong parent parameter will be passed to evaluate _PDC.
So we run into failure.
Level = 1, parent = \___ child = _PR_
Level = 2, parent = _PR_ child = P001
Level = 3, parent = P001 child = HI0_
Level = 3, parent = P001 child = _PDC
Level = 3, parent = P001 child = _OSC
Level = 3, parent = P001 child = _CST
Level = 3, parent = P001 child = _PPC
Level = 3, parent = P001 child = _PCT
Level = 3, parent = P001 child = _PSD
Level = 3, parent = P001 child = _PSS
Level = 3, parent = P001 child = SPSS
Level = 3, parent = P001 child = NPSS
Level = 2, parent = _PR_ child = CPU1
------------------------------------------------------------------
Did you see the same problem on Toshiba Tecra M5?
Thanks,
-Aubrey
