Li, Aubrey wrote: > 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? > No. Why would you think that they were? For example, AMD expects folks to use these objects for PowerNow!.
>> 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? > > I understand what the _PDC is used for and I realize that we only want to evaluate it once. What I believe I said was that doing so in the cpu_acpi.c module probably wasn't the place to do it *and* that we don't currently support any of the functionality besides P-state support so I'm satisfied for the moment leaving the PDC definitions in the speedstep.c module. Yes, things will change when we support C-states. >> 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 I meant was that so many platforms were returning "_PSS not found" due to the _PDC bug that we just lumped it all together and didn't recognize that there might have been more than one cause. > >> 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 understand that. I provided that table only as an example as to why I felt your proposed fix wouldn't work with that table. > 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. > This could be true. What I'm asking is whether or not there was a bug in the table that you encountered. If not, then I'd like to know how you determine which of the processor objects you are supposed to use. > I'll continue to investigate and let you know when I get something. > Thanks, Mark > Thanks, > -Aubrey >
