Nishant,
Have you tested your opp patches on 3630? Apparently opp changes in wip branch does not even boot on ZOOM3 board.
Vishwa
> -----Original Message-----
> From: Menon, Nishanth
> Sent: Thursday, January 07, 2010 5:16 AM
> To: Kevin Hilman
> Cc: linux-omap; Cousson, Benoit; Chikkature Rajashekar, Madhusudhan; Paul
> Walmsley; Dasgupta, Romit; Premi, Sanjeev; Shilimkar, Santosh; Aguirre, Sergio;
> Gopinath, Thara; Sripathy, Vishwanath; K, Ambresh
> Subject: Re: [PATCH 5/9] omap3: pm: sr: replace get_opp with freq_to_opp
>
> 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??
>
> --
> Regards,
> Nishanth Menon
--
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
