Re: [RFC][PATCH v2] OMAP3:PM: Fix OPP scale logic

[Roger Quadros <ext-roger.quadros@nokia.com>]

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 <nm@ti.com>
> ---
>  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)