Re: [pm-wip/voltdm_nm][PATCH 10/10] OMAP2+: PM: init_voltages: handle non compliant bootloaders
On Tue, Jun 7, 2011 at 20:41, Menon, Nishanth wrote:
> On Tue, Jun 7, 2011 at 20:12, Colin Cross wrote:
>>> 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
>>> Signed-off-by: Nishanth Menon
>>> ---
>>> 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:
> hmm.. fine with me..
>
>>
>> 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;
>> }
>> }
>>
> optionally -
> if (freq_cur == freq_valid) {
oops.. typo - intended:
if (freq_cur != freq_valid) {
Since the get_rate already says that we are in proper freq, why do
set_rate again?
> > 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 > f
Re: [pm-wip/voltdm_nm][PATCH 10/10] OMAP2+: PM: init_voltages: handle non compliant bootloaders
On Tue, Jun 7, 2011 at 20:12, Colin Cross wrote:
>> 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
>> Signed-off-by: Nishanth Menon
>> ---
>> 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:
hmm.. fine with me..
>
> 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;
> }
> }
>
optionally -
if (freq_cur == freq_valid) {
> 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) {
since we dont really know how the bootloader might have set the
voltage, (twl4030 allows i2c1 programming, others plugged on I2C_SR
can either use forcedupdate/vc bypass, we should do a force setting of
voltage..
if (freq_cur >= freq_valid) ?
> ret = voltdm_scale(voltdm, bootup_volt);
> if
Re: [pm-wip/voltdm_nm][PATCH 10/10] OMAP2+: PM: init_voltages: handle non compliant bootloaders
> 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
> Signed-off-by: Nishanth Menon
> ---
> 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 ma
[pm-wip/voltdm_nm][PATCH 10/10] OMAP2+: PM: init_voltages: handle non compliant bootloaders
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
Signed-off-by: Nishanth Menon
---
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,
char *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;
+ }
+ }
+
+ 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
