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).
> ------------------------------------------------------------------------
> -
> diff -r b6d0b03690de usr/src/uts/i86pc/io/speedstep.c
> --- a/usr/src/uts/i86pc/io/speedstep.c Mon Aug 20 17:45:19 2007 -0700
> +++ b/usr/src/uts/i86pc/io/speedstep.c Fri Sep 07 05:48:48 2007 +0800
> @@ -52,6 +52,14 @@
>
> #define ESS_PDC_REVISION 0x1
> #define ESS_PDC_PS_MSR (1<<0)
> +#define ESS_PDC_C1_FFH (1<<1)
> +#define ESS_PDC_T_MSR (1<<2)
> +#define ESS_PDC_C1PT (1<<3)
> +#define ESS_PDC_C2C3 (1<<4)
> +#define ESS_PDC_SMP_P (1<<5)
> +#define ESS_PDC_SMP_C (1<<6)
> +#define ESS_PDC_SMP_T (1<<7)
> +#define ESS_PDC_SMP_C_C1_FFH (1<<8)
>
> /*
> * MSR registers for changing and reading processor power state.
> @@ -195,7 +203,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));
> @@ -290,7 +297,7 @@ speedstep_init(cpudrv_devstate_t *cpudsp
> }
>
> if (ess_pdccap) {
> - uint32_t pdccap = ESS_PDC_PS_MSR;
> + uint32_t pdccap = (ESS_PDC_PS_MSR | ESS_PDC_C1_FFH |
> ESS_PDC_C1PT);
>
> /*
> * _PDC support is optional and the driver should
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.
> ------------------------------------------------------------------------
> ----------
> 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),
> ------------------------------------------------------------------------
> ----------
> diff -r b6d0b03690de usr/src/uts/intel/io/acpica/osl.c
> --- a/usr/src/uts/intel/io/acpica/osl.c Mon Aug 20 17:45:19 2007 -0700
> +++ b/usr/src/uts/intel/io/acpica/osl.c Fri Sep 07 05:54:09 2007 +0800
> @@ -1337,7 +1337,8 @@ acpica_add_processor_to_map(UINT32 acpi_
> continue;
>
> if (cpu_map[cpu_id]->mpa->ProcessorId == acpi_id) {
> - cpu_map[cpu_id]->obj = obj;
> + if (cpu_map[cpu_id]->obj == NULL)
> + cpu_map[cpu_id]->obj = obj;
> break;
> }
> }
> ------------------------------------------------------------------------
> ----------
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;
>
> 3) Processor supports SpeedStep feature, but the ACPI table offered by
> BIOS does not
> included SSDT table, and thus _PSS can not be found. For this case, I
> think the only
> thing we can do is logging the warning, but please correct me if I'm
> wrong, ;-)
Right. That's what we do.
Thanks,
Mark
