Bug 7859

Summary: Frequency can't switch on a dual Turion 64
Product: Power Management Reporter: Romain Li (roms)
Component: cpufreqAssignee: Thomas Renninger (trenn)
Status: CLOSED PATCH_ALREADY_AVAILABLE    
Severity: normal CC: trenn
Priority: P2    
Hardware: i386   
OS: Linux   
Kernel Version: 2.6.19.2 Subsystem:
Regression: --- Bisected commit-id:
Attachments: dmesg output
Values reported by sysfs (and can't be changed!)
Output reported by cpufreq-info
ACPI dump (BIOS v1.0a)
That one should fix it: Set cpufreq limit for all affected/managed cpus
dmesg output (PPC - ppc: 0, cpuid 0, policy_cpu: 0 line)
dmesg log after marc's patch
dmesg output with patch #6 and AC insertion/removal
ACPI dump (bios v1.1d)

Description Romain Li 2007-01-21 07:53:32 UTC
Most recent kernel where this bug did *NOT* occur:
Distribution: Debian testing 64-bits
Hardware Environment: Laptop Fujistu-Siemens Amilo Pa 1538
Software Environment: vanilla kernel 2.6.19.2 SMP w/ ACPI
Problem Description: while the driver correctly detects the two available
frequencies (800 & 1600 MHz), any governor will always use the lowest one.
Sometimes, it enables the highest frequency. In this case, my laptop switches
well between lowest and highest frequencies on power insertion/removal.
But, most of the time, I can switch between 800 and 800MHz, sic!

It looks like the powernow-k8 driver correctly constructs a two entry frequency
table and verifies it, and then reverifies a second frequency table with only 1
entry in it. But, I feel very strange that frequency range can vary ("request
for verification of policy") between 2 calls of cpufreq_driver->verify() on
the same CPU (cpufreq.c: 1386 & 1400). First call returns 800000-1600000MHz
while the second one returns 800000-800000. Moreover, both calls are done before
calling set_policy() at line 1413.

I have attached:
- dmesg output
- sysfs files
- cpufreq-info output
- ACPI dump

Romain.
Comment 1 Romain Li 2007-01-21 07:54:34 UTC
Created attachment 10143 [details]
dmesg output
Comment 2 Romain Li 2007-01-21 07:55:08 UTC
Created attachment 10144 [details]
Values reported by sysfs (and can't be changed!)
Comment 3 Romain Li 2007-01-21 07:55:30 UTC
Created attachment 10145 [details]
Output reported by cpufreq-info
Comment 4 Romain Li 2007-01-21 07:55:57 UTC
Created attachment 10146 [details]
ACPI dump (BIOS v1.0a)
Comment 5 Thomas Renninger 2007-01-21 13:18:56 UTC
Created attachment 10149 [details]
That one should fix it: Set cpufreq limit for all affected/managed cpus
Comment 6 Thomas Renninger 2007-01-22 00:51:56 UTC
What I found:
your BIOS only uses Notify (\_PR.CPU0, 0x80) calls, but does not inform the 
second CPU about changes (something like Notify (\_PR.CPU1, 0x80)).

In drivers/cpufreq/cpufreq.c:cpufreq_add_dev() a new policy object is 
allocated, also for a mangaged CPU. Therefore I think the attached patch is 
needed.

The _PPC function is used to tell the kernel which frequencies are allowed. A 
value of zero means all freqs are allowed, a value of one means the highest 
freq is not allowed (only 800 in your case).

Your BIOS initialses the _PPC value (XPPC variable) with 1 and it could be that 
_PPC must not be invoked at cpufreq initialisation time. To figure that out you 
should add a line like that at the end of drivers/cpufreq/processor_perflib.c: 
acpi_processor_ppc_notifier() (Best before the verify call, if you use my patch 
also place this line into the for_each_cpu_mask loop):
        printk ("PPC - ppc: %d, cpuid: %d, policy_cpu: %d\n", ppc, pr->id, 
policy->cpu);

If _PPC must not get invoked at initialisation time (if you have only the ppc 
call once at module load time and it's always 1), this patch is also needed:
http://marc.theaimsgroup.com/?l=linux-acpi&m=116900368102272&w=2
Comment 7 Romain Li 2007-01-22 08:39:19 UTC
I have added your printk line at the end of acpi_processor_ppc_notifier() but
before the verify call. On the other hand, I forgot to add it in the
for_each_cpu_mask loop. I have the call once at module load time (see
cpufreq2.dmesg).

As stated in your mail, I have added the marc's patch and I'm happy to tell you
that's working. I have changed governor twice as you may see in the
cpufreq3.dmesg log.

As a final check, I will try and build a UP kernel. More report later.
Comment 8 Romain Li 2007-01-22 08:41:18 UTC
Created attachment 10153 [details]
dmesg output (PPC - ppc: 0, cpuid 0, policy_cpu: 0 line)
Comment 9 Romain Li 2007-01-22 08:41:46 UTC
Created attachment 10154 [details]
dmesg log after marc's patch
Comment 10 Romain Li 2007-01-22 13:34:29 UTC
I build a UniProcessor kernel: frequency can't switch. I need both of your patches.

You are telling that my ACPI tables may be broken... Should I fix them?

Do you want I merge the two patches into a single one? Should I submit them on lkml?
Do you think that your patches can be accepted for merge in the current Linux
kernel tree?

Romain.
Comment 11 Thomas Renninger 2007-01-23 02:45:52 UTC
dmesg output (PPC - ppc: 0, cpuid 0, policy_cpu: 0 line)
ppc: 0
means you have to reach all freqs. Something is going wrong (or is this with 
the patch that makes all freqs reachable applied?).

The patch I pointed to was for a machine that wrongly returns ppc: 2 (in your 
case it should be 1) on startup.

> You are telling that my ACPI tables may be broken... Should I fix them?
Not sure, I am a bit confused now...
I expected a line like:
dmesg output (PPC - ppc: 1, cpuid 0, policy_cpu: 0 line)
In this case it is a BIOS problem (depends on how you read the specs, see the 
patch description from comment #6). The patch from comment #6 is the right fix 
and should find it's way mainline through Len, maybe it's worth also adding it 
to 2.6.19.3.
I assume that above line comes from the patched kernel, the patch from comment 
#6 gets mainline soon, everything is fine and I am closing this one now.

After digging a bit more... my patch from comment #5 should not be needed as 
CPU1's policy never gets used.

To verify you can add the printk that produced this line
dmesg output (PPC - ppc: 0, cpuid 0, policy_cpu: 0 line)
and make sure cpufreq is broken, there should be ppc: 1.

And/Or what would be really great: you can search BIOS configs for a 
Powermanagement/PowerNow! config to let the machine run on lower freq when 
working on battery if your BIOS provides that. Be sure also AC/battery module 
is loaded and unplug/replug AC adapter and verify that max freq gets lowered on 
battery and is increased again when working on AC. Reopen if you think 
something is still broken.
Comment 12 Romain Li 2007-01-23 03:47:38 UTC
I have taken a look at the Chapter 9 (Power and Thermal Management) of the "BIOS
and Kernel Developer's Guide for the AMD Athlon 64 and AMD Opteron Processors"
and they are telling this:
<<
9.5.6.3 Dual Core P-state Transition Requirements
For dual core processors, it is the processor driver
Comment 13 Romain Li 2007-01-23 13:10:12 UTC
1) I can confirm that patch from comment #5 is not needed.

2) I can't config CPU from BIOS because my BIOS is very ... light!

3) When I remove AC power, max freq is lowered. But, when I set AC power again, 
freq is still low :-(

I have attached dmesg output with your 'printk' lines.
Comment 14 Romain Li 2007-01-23 13:11:33 UTC
Created attachment 10165 [details]
dmesg output with patch #6 and AC insertion/removal
Comment 15 Romain Li 2007-01-23 13:29:52 UTC
Add-on: I can manually change governors and frequencies without any problems 
(ppc = 0). But switching AC power supply raises ppc to 1 and it is never reset 
to 0.
Comment 16 Thomas Renninger 2007-01-25 02:45:44 UTC
Can you do:
cat /proc/acpi/event
(you probably need to stop acpid, or whatever process access this file, you can 
chech with fuser /proc/acpi/event).

The cat should block.
If you (un)plug the AC adapter you should see ACPI events popping up.
Is there also a processor (0x80 should be the last of the values)?
If yes, there might still be a bug in the kernel (related to linked Dual Core 
cpus?)
Also adding a printk add at end of:
acpi_processor_get_platform_limit() in drivers/acpi/processor_perflib.c showing 
the ppc value and the CPU that gets read when a CPU event happens, could be 
helpful.

If not, modifing/debugging the DSDT is the next stepp I'd take.
The question then is why "Notify (\_PR.CPU0, 0x80)" is not invoked in DSDT on a 
AC change. This should happen in "Method (_L10, 0, NotSerialized)", the only 
place where ACON variable is modified at runtime and where "Notifies" are 
thrown (conditionally, depending on some variables...).

To test whether CPU notification and kernel cpufreq works in general you could 
modify your DSDT like that:
in method _PSR of device ACDC add two lines:

--- kernel-7859_Turion_PPC_limited.orig/DSDT.modif.dsl
+++ kernel-7859_Turion_PPC_limited/DSDT.modif.dsl
@@ -4062,6 +4062,8 @@ DefinitionBlock ("DSDT.aml", "DSDT", 1, 
                 Acquire (PSMX, 0xFFFF)
                 And (\_SB.PCI0.LPC0.PMRD (0xA0), 0x01, Local0)
                 Store (Local0, ACFG)
+                Store (Local0, ACON)
+                Notify (\_PR.CPU0, 0x80)
                 Release (PSMX)
                 Return (ACFG)

To be able to do that follow up following steps:
  - get the newest iasl tools (not sure which ubuntu package this is, you may 
    want to ask Matthew Garrett <mjg59@srcf.ucam.org> (AFAIK he packaged it).
  - acpidump > acpidump
  - acpixtract acpidump   (on old versions cat acpidump |acpixtract)
  - iasl -d DSDT.dat      (disassemble the DSDT table)
  - Modify the outcomig DSDT.dsl as shown above
  - iasl -ta DSDT.dsl     (recompile the DSDT table including your changes, 
    IIRC it's the -ta option and you must use the .hex file popping out)
  - modify your kernel's compile option  (make config, make menuconfig or 
    whatever, whatch out for: CONFIG_ACPI_CUSTOM_DSDT=y must be set, 
    ACPI_CUSTOM_DSDT_FILE must point to your newly created DSDT.hex file
  - recompile and boot the kernel
  - when booted, dmesg should show something that DSDT got overridden

Are all freqs available if you unplug AC?
If you don't even get an AC ACPI event, do a cat /proc/acpi/ac_adapter/state, 
then above added code (update ACON and throw CPU event) should get processed.

It's also very useful (you should always compile with ACPI_DEBUG=y) to 
additional debug statements inside the DSDT. You might want to add lines like 
that:
   Store (ACON, debug)         (replace ACON with whatever variable's value you 
                                like to debug)

Clustering the _L10 method (the conditions where ACON and Notify (..CPU0, 0x80) 
are involved) would make sense and find out whether ACON did not get set 
correctly or why the Notify (..CPU0, 0x80) did not get invoked.

You should only modify the DSDT for debug purposes. You might want to goolge a 
bit, there should be more info around.
Comment 17 Romain Li 2007-01-25 13:09:45 UTC
About first step, I get some events (BIOS v1.0a):
AC off:
processor CPU0 00000080 00000001
ac_adapter ACAD 00000080 00000000
processor CPU0 00000081 00000000
ac_adapter ACAD 00000080 00000000
battery BAT0 00000080 00000001

AC on:
processor CPU0 00000080 00000001
ac_adapter ACAD 00000080 00000001
processor CPU0 00000081 00000000
ac_adapter ACAD 00000080 00000001
battery BAT0 00000080 00000001

I have been on my vendor site to find a BIOS update. They have just released (a 
big) new one: v1.1d. This seems to be a major release but the changelog is 
almost empty. Anyways, I tried it: cpufreq seems to work fine and fan, too. 
ACPI events are not the same for now:
AC off:
processor CPU0 00000080 00000001
ac_adapter ACAD 00000080 00000000
processor CPU0 00000081 00000000

AC on:
processor CPU0 00000080 00000000
ac_adapter ACAD 00000080 00000001
processor CPU0 00000081 00000000

Maybe I shouldn't have updated BIOS for testing consistency: I have downgraded 
my BIOS to original version (1.0a) => cpufreq is now fixed to 800MHz; patch 
does not work any more (ppc=1, always)?!

I don't know whether I should do other tests you proposed. Please tell me...
Otherwise, I have attached DSDT tables of new BIOS for comparison (if any 
interest).
Comment 18 Romain Li 2007-01-25 13:15:27 UTC
Created attachment 10191 [details]
ACPI dump (bios v1.1d)
Comment 19 Romain Li 2007-01-25 13:20:27 UTC
I have noticed a weird thing: ACPI dumps are the same (diff returns no ouput) 
between 2 BIOS. Thus, tables did not change?!
Comment 20 Romain Li 2007-01-30 04:03:36 UTC
Patch from comment #6 works for me. Frequency gets limited on battery and
unlimited on AC as probably intended by BIOS.