On Tuesday, September 11, 2007 12:39 AM, Mark.Haywood at Sun.COM wrote: > 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.
errr, we have already cache pct/pss/psd and ppc in cpu_acpi.c. Aren't these function specific to Intel? > > 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. Enabling _PDC bits is in order to load all available ACPI feature. So when we call P-state driver, and we enable C-state driver in future, We don't have to evaluate _PDC twice or more. What's wrong if we enable all bits at the initial time? > > 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 agree. So it's better if we set the bits by checking the feature from cpuid. > > 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). >> That's fine. We can discuss this later. > >>>> ---------- >>>> 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. > This problem is not depend on the previously existing bug. The current driver doesn't log _PDC failure, so we didn't notice this problem. I can get _PSS object on that machine by iasl and acpidump, but the driver still warns me it's not found. So I dig into it and found _PDC evaluating failure. So I suggest we put _PDC evaluating failure into log, on the boot screen is not necessary. > What machine had a BIOS that gave you the (_PR) Processor Tree > object in > case 2 above? I'll check if the platform is release, ;-) >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. > Your change of that table is so good that we don't have to change the current code behavior. But that's the table the BIOS give out. And the driver have to be based on it. I didn't found the specification for it so far. At least, the current method of building cpu acpi map doesn't cover all kinds of machine, which need to be fixed. I'll continue to investigate and let you know when I get something. Thanks, -Aubrey
