linux-omap.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [pm-wip/voltdm_nm][PATCH 10/10] OMAP2+: PM: init_voltages: handle non compliant bootloaders
@ 2011-06-08  1:12 Colin Cross
  2011-06-08  1:41 ` Menon, Nishanth
  0 siblings, 1 reply; 4+ messages in thread
From: Colin Cross @ 2011-06-08  1:12 UTC (permalink / raw)
  To: Nishanth Menon; +Cc: l-o, Kevin Hilman

> Bootloaders should in theory setup a frequency which is enabled in
> OPP table. However, there can be mismatches, and we should try
> both going lower in addition to the going higher to find
> a match if bootloader boots up at a OPP than the kernel thinks it
> should be allowed. We also sequence the frequency and voltage settings
> properly.
>
> Reported-by: Colin Cross <ccross@google.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
> PS: Apologies on the spam.. for some reason 10/10 never appeared in
> http://marc.info/?l=linux-omap&r=2&b=201106&w=2 - grumble grumble :(
>
>  arch/arm/mach-omap2/pm.c |   55 ++++++++++++++++++++++++++++++++++++---------
>  1 files changed, 44 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
> index 7355347..ce7554b 100644
> --- a/arch/arm/mach-omap2/pm.c
> +++ b/arch/arm/mach-omap2/pm.c
> @@ -174,7 +174,9 @@ static int __init omap2_set_init_voltage(char *vdd_name, ch ar *clk_name,
>  	struct voltagedomain *voltdm;
>  	struct clk *clk;
>  	struct opp *opp;
> -	unsigned long freq, bootup_volt;
> +	unsigned long freq_cur, freq_valid, bootup_volt;
> +	int raise_freq_idx, i;
> +	int ret = -EINVAL;
>
>  	if (!vdd_name || !clk_name || !dev) {
>  		printk(KERN_ERR "%s: Invalid parameters!\n", __func__);
> @@ -195,16 +197,20 @@ static int __init omap2_set_init_voltage(char *vdd_name, char *clk_name,
>  		goto exit;
>  	}
>
> -	freq = clk->rate;
> -	clk_put(clk);
> +	freq_cur = clk->rate;
> +	freq_valid = freq_cur;
>
>  	rcu_read_lock();
> -	opp = opp_find_freq_ceil(dev, &freq);
> +	opp = opp_find_freq_ceil(dev, &freq_valid);
>  	if (IS_ERR(opp)) {
> -		rcu_read_unlock();
> -		printk(KERN_ERR "%s: unable to find boot up OPP for vdd_%s\n",
> -			__func__, vdd_name);
> -		goto exit;
> +		opp = opp_find_freq_floor(dev, &freq_valid);
> +		if (IS_ERR(opp)) {
> +			rcu_read_unlock();
> +			pr_err("%s: no boot OPP match for %ld on vdd_%s\n",
> +				__func__, freq_cur, vdd_name);
> +			ret = -ENOENT;
> +			goto exit_ck;
> +		}
>  	}
>
>  	bootup_volt = opp_get_voltage(opp);
> @@ -212,11 +218,38 @@ static int __init omap2_set_init_voltage(char *vdd_name, char *clk_name,
>  	if (!bootup_volt) {
>  		printk(KERN_ERR "%s: unable to find voltage corresponding"
>  			"to the bootup OPP for vdd_%s\n", __func__, vdd_name);
> -		goto exit;
> +		ret = -ENOENT;
> +		goto exit_ck;
>  	}
>
> -	voltdm_scale(voltdm, bootup_volt);
> -	return 0;
> +	/*
> +	 * Frequency and Voltage have to be sequenced: if we move from
> +	 * a lower frequency to higher frequency, raise voltage, followed by
> +	 * frequency, and vice versa. we assume that the voltage at boot
> +	 * is the required voltage for the frequency it was set for.
> +	 */
> +	raise_freq_idx = freq_cur < freq_valid;
> +	for (i = 0; i < 2; i++) {
> +		if (i == raise_freq_idx)
> +			ret = clk_set_rate(clk, freq_valid);
> +		else
> +			ret = voltdm_scale(voltdm, bootup_volt);
> +		if (ret < 0) {
> +			pr_err("%s: unable to set %s-%s(f=%ld v=%ld)on vdd%s\n",
> +				__func__,
> +				(i == raise_freq_idx) ? "clk" : "voltage",
> +				(i == raise_freq_idx) ? clk_name : vdd_name,
> +				freq_valid, bootup_volt, vdd_name);
> +			goto exit_ck;
> +		}
> +	}

This is way too complicated.  Writing out what you mean is much more
readable, and not many more LOC:

if (freq_cur < freq_valid) {
	ret = voltdm_scale(voltdm, bootup_volt);
	if (ret) {
		pr_err("%s: unable to set voltage-%s(f=%ld v=%ld)on vdd%s\n",
			vdd_name, freq_valid, bootup_volt, vdd_name);
		goto exit_ck;
	}
}

ret = clk_set_rate(clk, freq_valid);
if (ret) {
	pr_err("%s: unable to set clk-%s(f=%ld v=%ld)on vdd%s\n",
		clk_name, freq_valid, bootup_volt, vdd_name);
	goto exit_ck;
}

if (freq_cur > freq_valid) {
	ret = voltdm_scale(voltdm, bootup_volt);
	if (ret) {
		pr_err("%s: unable to set voltage-%s(f=%ld v=%ld)on vdd%s\n",
			vdd_name, freq_valid, bootup_volt, vdd_name);
		goto exit_ck;
	}
}

> +
> +	ret = 0;
> +exit_ck:
> +	clk_put(clk);
> +
> +	if (!ret)
> +		return 0;
>
>  exit:
>  	printk(KERN_ERR "%s: Unable to put vdd_%s to its init voltage\n\n",
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 4+ messages in thread
[parent not found: <[pm-wip/voltdm_nm][PATCH 00/10] OMAP2+: voltage: fixes>]

end of thread, other threads:[~2011-06-08  2:09 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-06-08  1:12 [pm-wip/voltdm_nm][PATCH 10/10] OMAP2+: PM: init_voltages: handle non compliant bootloaders Colin Cross
2011-06-08  1:41 ` Menon, Nishanth
2011-06-08  2:09   ` Menon, Nishanth
     [not found] <[pm-wip/voltdm_nm][PATCH 00/10] OMAP2+: voltage: fixes>
2011-06-08  0:39 ` Nishanth Menon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).