From mboxrd@z Thu Jan 1 00:00:00 1970 From: Roger Quadros Subject: Re: [RFC][PATCH v2] OMAP3:PM: Fix OPP scale logic Date: Tue, 04 Aug 2009 18:10:37 +0300 Message-ID: <4A784F6D.1080404@nokia.com> References: <[RFC PATCH] OMAP3:PM: Fix OPP scale logic> <1249392595-18758-1-git-send-email-nm@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from smtp.nokia.com ([192.100.122.233]:19158 "EHLO mgw-mx06.nokia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932241AbZHDPLL (ORCPT ); Tue, 4 Aug 2009 11:11:11 -0400 In-Reply-To: <1249392595-18758-1-git-send-email-nm@ti.com> Sender: linux-omap-owner@vger.kernel.org List-Id: linux-omap@vger.kernel.org To: ext Nishanth Menon Cc: Kevin , Paul , linux-omap ext Nishanth Menon wrote: > Rev 2 of the patch: > Ref: http://marc.info/?t=124930700200005&r=1&w=2 > > While switching from higher OPP to lower OPP, > current scale logic can fail by switching to lower > voltage while frequency remains at old value. > > This patch adds a cleaner recovery logic and > additional freq dpll checks. This changes > program_freq_opp return type in the process > for program_opp to handle error in a consistent > manner. > > NOTE: I moved the *cur_opp setting to under the > scratchpad locked region to allow for code > simplicity - I wonder if anyone sees an issue with it > > Tested on:rx-51, ported to pm branch - untested linux-omap > Patch generated on linux-omap pm branch, commit: > 7e7377395d6b4576341a6939bf2179f3946f2ea0 > > Signed-off-by: Nishanth Menon > --- > arch/arm/mach-omap2/resource34xx.c | 61 +++++++++++++++++++++++++++--------- > 1 files changed, 46 insertions(+), 15 deletions(-) > > diff --git a/arch/arm/mach-omap2/resource34xx.c b/arch/arm/mach-omap2/resource34xx.c > index 25535a3..83241c4 100644 > --- a/arch/arm/mach-omap2/resource34xx.c > +++ b/arch/arm/mach-omap2/resource34xx.c > @@ -240,13 +240,23 @@ static int program_opp_freq(int res, int target_level, int current_level) > lock_scratchpad_sem(); > if (res == VDD1_OPP) { > curr_opp = &curr_vdd1_opp; > - clk_set_rate(dpll1_clk, mpu_opps[target_level].rate); > - clk_set_rate(dpll2_clk, dsp_opps[target_level].rate); > + ret = clk_set_rate(dpll1_clk, mpu_opps[target_level].rate); > + if (unlikely(ret)) > + goto out; > + > + ret = clk_set_rate(dpll2_clk, dsp_opps[target_level].rate); > + if (unlikely(ret)) > + /* reset the dpll1 if failed */ > + clk_set_rate(dpll1_clk, mpu_opps[current_level].rate); > #ifndef CONFIG_CPU_FREQ > - /*Update loops_per_jiffy if processor speed is being changed*/ > - loops_per_jiffy = compute_lpj(loops_per_jiffy, > - mpu_opps[current_level].rate/1000, > - mpu_opps[target_level].rate/1000); > + else > + /* > + * Update loops_per_jiffy if processor speed > + * is being changed > + */ > + loops_per_jiffy = compute_lpj(loops_per_jiffy, > + mpu_opps[current_level].rate/1000, > + mpu_opps[target_level].rate/1000); > #endif > } else { > curr_opp = &curr_vdd2_opp; > @@ -255,17 +265,16 @@ static int program_opp_freq(int res, int target_level, int current_level) > ret = clk_set_rate(dpll3_clk, > l3_opps[target_level].rate * l3_div); > } > - if (ret) { > - unlock_scratchpad_sem(); > - return current_level; > - } > +out: > + if (!ret) { > #ifdef CONFIG_PM > - omap3_save_scratchpad_contents(); > + omap3_save_scratchpad_contents(); > + *curr_opp = target_level; > #endif shouldn't &curr_opp = target_level be done outside of #ifdef CONFIG_PM? > + } > unlock_scratchpad_sem(); > > - *curr_opp = target_level; > - return target_level; > + return ret; > } > > static int program_opp(int res, struct omap_opp *opp, int target_level, > @@ -289,13 +298,35 @@ static int program_opp(int res, struct omap_opp *opp, int target_level, > current_level); > #ifdef CONFIG_OMAP_SMARTREFLEX > else > - sr_voltagescale_vcbypass(t_opp, c_opp, > + ret = sr_voltagescale_vcbypass(t_opp, c_opp, > opp[target_level].vsel, > opp[current_level].vsel); > + if (ret) { > + int ret1 = 0; > + /* > + * If something did not work, put me back to old state. > + * Recover the other guy if at least 1 prev iteration > + * had run > + */ > + if (i && raise) > + ret1 = sr_voltagescale_vcbypass(c_opp, t_opp, > + opp[current_level].vsel, > + opp[target_level].vsel); > + else if (i) > + ret1 = program_opp_freq(res, current_level, > + target_level); > + /* > + * If I could not reset my old state back.. system > + * is no longer in a controlled state.. bug me > + */ > + if (unlikely(ret1)) > + BUG(); > + break; > + } > #endif > } > > - return ret; > + return (res == PRCM_VDD1) ? curr_vdd1_opp : curr_vdd2_opp; > } > > int resource_set_opp_level(int res, u32 target_level, int flags)