Nishanth Menon <nm@xxxxxx> writes:
> Kevin Hilman had written, on 12/22/2009 10:45 AM, the following:
>> "Menon, Nishanth" <nm@xxxxxx> writes:
>>
>>> Kevin Hilman said the following on 12/19/2009 04:42 AM:
>>>> Nishanth Menon <nm@xxxxxx> 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)
> [...]
>
> Throws up my hands! 3430 on N900/SDP3430 should be essentially the same!
>
> Configuration:
> ==============
> kernel:
> git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git
> branch:
> pm-wip-opp 6e1276f omap3:pm: remove map3_[mpu|dsp|l3]_rate_tables
> Applied my bugfix: http://patchwork.kernel.org/patch/71457/
>
> I have a 3430 ES3.1 SDP3430 where I did run the test:
>
> Log Patch: http://pastebin.mozilla.org/695225
> Test script: http://pastebin.mozilla.org/695226 (I call this test-me.sh)
> Result log: http://pastebin.mozilla.org/695224
>
> huh? no crash, all the frequencies and transitions are good, the rates
> of dpll1,2,3 look good! there is something definitely fishy about N900
> configuration??
OK, so for now we need to at least replace the original approximate
match with an equivalent approximate match. Then, use an additional
patch to change it to an exact match when we find out what's happening
on N900.
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
