Re: [PATCH 5/9] omap3: pm: sr: replace get_opp with freq_to_opp

[Kevin Hilman <khilman@deeprootsystems.com>]

"Menon, Nishanth" <nm@ti.com> writes:

> Kevin Hilman said the following on 12/19/2009 04:42 AM:
>> Nishanth Menon <nm@ti.com> writes:
>>
>>   
>>> SmartReflex implements a get_opp to search through the opp table,
>>> replace it with the accessor function as it is a duplicate of
>>> freq_to_opp
>>>     
>>
>> SmartReflex is not quite working with this version which is in
>> pm-wip-opp.  My (untested) theory below...
>>
>> [...]
>>   
> Ambresh and I  just tested the very latest of the pm-wip-opp branch
> and checked. Voltage transitions and SR adjustments are happily
> happening on SDP3430 ES3.1 at the very least (verified with a scope on
> vdd1).
>
> and if you look closely in the code, sr2.vdd_opp_clk->rate and
> sr1.vdd_opp_clk->rate are based on
> sr1.vdd_opp_clk = clk_get(NULL, "dpll1_ck")
> sr2.vdd_opp_clk = clk_get(NULL, "l3_ick");
>
> now, if the dpll1_ck ->rate and l3_ick->rate are not exact frequencies
> as the opp tables, I think we have a clockframework bug and the code
> here is correct. we should fix the clockframework/find the rootcause
> elsewhere.
>
> Now is the clockframework wrong? we added a patch to print the
> frequencies and checked if IS_ERR(opp) is true -> not a single call
> while using cpu_freq transitions resulted in an error value and all
> the frequencies we printed were from the OPP table
>
> Might be good to hear your rationale for saying this result..

Part of my rationale was that the PM branch version get_vdd1_opp()
does an approximate match (actually the equivalent of
opp_find_freq_ceil() via get_opp.)  So you replaced an approximate
match with an exact match.  That may be the right solution and point
to a bug elsewhere, but it was not an equivalent replacement, and
probably should've been done in a separate patch with
justification/rationale.  I didn't catch it during initial review.

But the primary reason I noticed it in the first place was that I was
seeing DVFS errors on n900, which is the only platform I have that
actually can do SmartReflex.  Specifically, sr_reset_voltage() is
failing for VDD1.  Dumping out the clock rates used in get_vdd1_opp()
show that the clocks rates are close, but not the exact value used in
the OPP table.

Here is the patch I used to add some debugging, and I see that for the
250MHz OPP, the clock framework rate is 249600000 and for the 125MHz
OPP, the clock rate is 124800000.  As I said, close, but clearly an
find with exact match is going to fail.

Below the patch, is the console dump and backtrace of how get_vdd1_opp()
was called so you can see the call chain.

Kevin

diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c
index d341857..01a3dbd 100644
--- a/arch/arm/mach-omap2/smartreflex.c
+++ b/arch/arm/mach-omap2/smartreflex.c
@@ -155,6 +155,8 @@ static u8 get_vdd1_opp(void)
 							mpu_opps == NULL)
 		return 0;
 
+	printk("%s: sr1.vdd_opp_clk->rate = %d\n", __func__,
+	       sr1.vdd_opp_clk->rate);
 	opp = opp_find_freq_exact(mpu_opps, sr1.vdd_opp_clk->rate, true);
 	if (IS_ERR(opp))
 		return 0;
@@ -451,6 +453,7 @@ static int sr_reset_voltage(int srid)
 		target_opp_no = get_vdd1_opp();
 		if (!target_opp_no) {
 			pr_info("Current OPP unknown: Cannot reset voltage\n");
+			__backtrace();
 			return 1;
 		}
 


/sys/devices/system/cpu/cpu0/cpufreq # echo 250000 > scaling_setspeed 
get_vdd1_opp: sr1.vdd_opp_clk->rate = 249600000
Current OPP unknown: Cannot reset voltage
[<c0042c98>] (sr_reset_voltage+0x0/0x190) from [<c0043094>] (sr_stop_vddautocomap+0x12c/0x148)
 r7:00000001 r6:00000001 r5:c03ac878 r4:00000000
[<c0042f68>] (sr_stop_vddautocomap+0x0/0x148) from [<c0043388>] (sr_voltagescale_vcbypass+0x2c/0x170)
 r7:00000001 r6:00000001 r5:00000030 r4:00000026
[<c004335c>] (sr_voltagescale_vcbypass+0x0/0x170) from [<c0043e00>] (program_opp+0x1e4/0x210)
[<c0043c1c>] (program_opp+0x0/0x210) from [<c0043f64>] (resource_set_opp_level+0x138/0x1b0)
[<c0043e2c>] (resource_set_opp_level+0x0/0x1b0) from [<c0044024>] (set_opp+0x48/0x120)
[<c0043fdc>] (set_opp+0x0/0x120) from [<c004bb48>] (update_resource_level+0xb0/0xd4)
 r5:c03b365c r4:00000002
[<c004ba98>] (update_resource_level+0x0/0xd4) from [<c004be34>] (resource_request+0x154/0x184)
 r7:00000002 r6:c03db5a0 r5:c03b365c r4:00000000
[<c004bce0>] (resource_request+0x0/0x184) from [<c0043a50>] (set_freq+0xb4/0xe8)
 r7:0ee6b280 r6:cf801080 r5:cf801000 r4:c0359347
[<c004399c>] (set_freq+0x0/0xe8) from [<c004bb48>] (update_resource_level+0xb0/0xd4)
 r8:c03d52f8 r7:0ee6b280 r6:c03dbf70 r5:c03b36a4 r4:0ee6b280
[<c004ba98>] (update_resource_level+0x0/0xd4) from [<c004be34>] (resource_request+0x154/0x184)
 r7:0ee6b280 r6:c03dbf70 r5:c03b36a4 r4:00000000
[<c004bce0>] (resource_request+0x0/0x184) from [<c004b7a4>] (omap_pm_cpu_set_freq+0x34/0x44)
 r7:00000007 r6:cfae7000 r5:cf96c100 r4:0003d090
[<c004b770>] (omap_pm_cpu_set_freq+0x0/0x44) from [<c004ac44>] (omap_target+0x64/0x74)
[<c004abe0>] (omap_target+0x0/0x74) from [<c02318c4>] (__cpufreq_driver_target+0x30/0x40)
[<c0231894>] (__cpufreq_driver_target+0x0/0x40) from [<c02339e8>] (cpufreq_set+0x58/0x74)
[<c0233990>] (cpufreq_set+0x0/0x74) from [<c0231b9c>] (store_scaling_setspeed+0x64/0x7c)
 r5:00000007 r4:cf96c100
[<c0231b38>] (store_scaling_setspeed+0x0/0x7c) from [<c0232ae0>] (store+0x60/0x7c)
 r5:cf96c100 r4:c03d53f8
[<c0232a80>] (store+0x0/0x7c) from [<c010c684>] (sysfs_write_file+0x110/0x144)
 r7:cf96c150 r6:cfb10bc0 r5:cf8fc000 r4:00000007
[<c010c574>] (sysfs_write_file+0x0/0x144) from [<c00bbdcc>] (vfs_write+0xb8/0x164)
[<c00bbd14>] (vfs_write+0x0/0x164) from [<c00bbf3c>] (sys_write+0x44/0x70)
 r8:40001000 r7:00000007 r6:cfafa280 r5:00000000 r4:00000000
[<c00bbef8>] (sys_write+0x0/0x70) from [<c0030fc0>] (ret_fast_syscall+0x0/0x2c)
 r8:c0031144 r7:00000004 r6:41149600 r5:40001000 r4:00000007



/sys/devices/system/cpu/cpu0/cpufreq # echo 125000 > scaling_setspeed 
get_vdd1_opp: sr1.vdd_opp_clk->rate = 124800000
Current OPP unknown: Cannot reset voltage
[<c0042c98>] (sr_reset_voltage+0x0/0x190) from [<c0043094>] (sr_stop_vddautocomap+0x12c/0x148)
 r7:00000001 r6:00000001 r5:c03ac878 r4:00000000
[<c0042f68>] (sr_stop_vddautocomap+0x0/0x148) from [<c0043388>] (sr_voltagescale_vcbypass+0x2c/0x170)
 r7:00000001 r6:00000001 r5:00000026 r4:0000001e
[<c004335c>] (sr_voltagescale_vcbypass+0x0/0x170) from [<c0043e00>] (program_opp+0x1e4/0x210)
[<c0043c1c>] (program_opp+0x0/0x210) from [<c0043f64>] (resource_set_opp_level+0x138/0x1b0)
[<c0043e2c>] (resource_set_opp_level+0x0/0x1b0) from [<c0044024>] (set_opp+0x48/0x120)
[<c0043fdc>] (set_opp+0x0/0x120) from [<c004bb48>] (update_resource_level+0xb0/0xd4)
 r5:c03b365c r4:00000001
[<c004ba98>] (update_resource_level+0x0/0xd4) from [<c004be34>] (resource_request+0x154/0x184)
 r7:00000001 r6:c03db5a0 r5:c03b365c r4:00000000
[<c004bce0>] (resource_request+0x0/0x184) from [<c0043a50>] (set_freq+0xb4/0xe8)
 r7:07735940 r6:cf801080 r5:cf801000 r4:c0359347
[<c004399c>] (set_freq+0x0/0xe8) from [<c004bb48>] (update_resource_level+0xb0/0xd4)
 r8:c03d52f8 r7:07735940 r6:c03dbf70 r5:c03b36a4 r4:07735940
[<c004ba98>] (update_resource_level+0x0/0xd4) from [<c004be34>] (resource_request+0x154/0x184)
 r7:07735940 r6:c03dbf70 r5:c03b36a4 r4:00000000
[<c004bce0>] (resource_request+0x0/0x184) from [<c004b7a4>] (omap_pm_cpu_set_freq+0x34/0x44)
 r7:00000007 r6:cfae7000 r5:cf96c100 r4:0001e848
[<c004b770>] (omap_pm_cpu_set_freq+0x0/0x44) from [<c004ac44>] (omap_target+0x64/0x74)
[<c004abe0>] (omap_target+0x0/0x74) from [<c02318c4>] (__cpufreq_driver_target+0x30/0x40)
[<c0231894>] (__cpufreq_driver_target+0x0/0x40) from [<c02339e8>] (cpufreq_set+0x58/0x74)
[<c0233990>] (cpufreq_set+0x0/0x74) from [<c0231b9c>] (store_scaling_setspeed+0x64/0x7c)
 r5:00000007 r4:cf96c100
[<c0231b38>] (store_scaling_setspeed+0x0/0x7c) from [<c0232ae0>] (store+0x60/0x7c)
 r5:cf96c100 r4:c03d53f8
[<c0232a80>] (store+0x0/0x7c) from [<c010c684>] (sysfs_write_file+0x110/0x144)
 r7:cf96c150 r6:cfb10bc0 r5:cf8fc000 r4:00000007
[<c010c574>] (sysfs_write_file+0x0/0x144) from [<c00bbdcc>] (vfs_write+0xb8/0x164)
[<c00bbd14>] (vfs_write+0x0/0x164) from [<c00bbf3c>] (sys_write+0x44/0x70)
 r8:40001000 r7:00000007 r6:cfafa700 r5:00000000 r4:00000000
[<c00bbef8>] (sys_write+0x0/0x70) from [<c0030fc0>] (ret_fast_syscall+0x0/0x2c)
 r8:c0031144 r7:00000004 r6:41149600 r5:40001000 r4:00000007