Li, Aubrey wrote:
> 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?
>
Well, I agree with you that when we support something other than
P-states, that we will probably want to move some of the _PDC stuff out
of the speedstep.c module. I'm not sure that moving the _PDC stuff into
cpu_acpi.[ch] is a good idea though:
1 - The cpu_acpi.[ch] code is supposed to be vendor agnostic. We have to
support processors other than Intel. So coding cpu_acpi_init() to be
specific to Intel (setting the pdc_cap as you have in your code below)
wouldn't be a good idea.
2 - I don't believe that we should enable any of the _PDC bits until we
actually support the functionality that
they represent (maybe you weren't suggesting that and you were just
providing an example) and I don't believe that we support all of the
bits that were enabled in the code below.
3 - I'm not sure whether things will be as straightforward as this code
makes it appear. That is, I can imagine us
wanting to set some of these bits on some Intel processors and not on
some others (I could be mistaken).
I'm not resistant to making some changes to this code, but I'm not sure
that I see a good reason to make these changes at the moment? We'll
probably want to readdress this when C-state support is supported, at
which time speedstep.c might become intel.c or something like that (who
knows).
> ------------------------------------------------------------------------
> --
> 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?
>
This is interesting and is not something that I've seen before. But then
again, given the previously existing bug with _PDC negotiation, we might
not have noticed this problem. As far as I know, folks with Toshiba
Tecra M5s are not seeing this problem.
What machine had a BIOS that gave you the (_PR) Processor Tree object in
case 2 above? That seems wrong to me. Or at least it seems to me that we
(I) need to have a better understanding of it. I'd not have thought that
it was possible for the ACPI Processor NameSpace to define two separate
Processor objects for the same processor. For example, is it possible to
have the following tree structure?
Level = 1, parent = \___ child = _PR_
Level = 2, parent = _PR_ child = CPU1
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
If so, then the proposed fix above would not work.
Thanks,
Mark
> Thanks,
> -Aubrey
>
