[PATCH/RFT 0/1] MPU DVFS using SMPS regulator driver

[PATCH/RFT 0/1] MPU DVFS using SMPS regulator driver

[Kevin Hilman]

This patch modifies the OMAP CPUfreq driver to use the regulator framework
to scale voltage when scaling frequency.  It uses the new SMPS regulator
driver from Tero.

This patch applies on top of the recently posted SMPS regulator series
from Tero:

   Subject: [PATCHv9 0/5] arm: omap: smps regulator support
   Date:	Thu, 16 Feb 2012 12:27:48 +0200

I'd especially appreciate testing from those of you who are setup to
measure voltage at the voltage rails and can confirm that voltage is
actually being scaled.

Kevin Hilman (1):
  OMAP2+: cpufreq: scale voltage along with frequency

 arch/arm/mach-omap2/voltage.c  |    2 ++
 drivers/cpufreq/omap-cpufreq.c |   29 ++++++++++++++++++++++++++++-
 2 files changed, 30 insertions(+), 1 deletions(-)

-- 
1.7.9

[PATCH/RFT 1/1] OMAP2+: cpufreq: scale voltage along with frequency

[Kevin Hilman]

Use the regulator framework to get the voltage regulator associated
with the MPU voltage domain and use it to scale voltage along with
frequency.

Signed-off-by: Kevin Hilman <khilman@ti.com>
---
 arch/arm/mach-omap2/voltage.c  |    2 ++
 drivers/cpufreq/omap-cpufreq.c |   29 ++++++++++++++++++++++++++++-
 2 files changed, 30 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
index 8a36342..140c032 100644
--- a/arch/arm/mach-omap2/voltage.c
+++ b/arch/arm/mach-omap2/voltage.c
@@ -89,6 +89,8 @@ int voltdm_scale(struct voltagedomain *voltdm,
 	ret = voltdm->scale(voltdm, target_volt);
 	if (!ret)
 		voltdm->nominal_volt = target_volt;
+	printk("KJH: %s: %d\n", __func__, target_volt);
+	dump_stack();
 
 	return ret;
 }
diff --git a/drivers/cpufreq/omap-cpufreq.c b/drivers/cpufreq/omap-cpufreq.c
index 5d04c57..e4f4841 100644
--- a/drivers/cpufreq/omap-cpufreq.c
+++ b/drivers/cpufreq/omap-cpufreq.c
@@ -25,6 +25,7 @@
 #include <linux/opp.h>
 #include <linux/cpu.h>
 #include <linux/module.h>
+#include <linux/regulator/consumer.h>
 
 #include <asm/system.h>
 #include <asm/smp_plat.h>
@@ -52,6 +53,7 @@ static atomic_t freq_table_users = ATOMIC_INIT(0);
 static struct clk *mpu_clk;
 static char *mpu_clk_name;
 static struct device *mpu_dev;
+static struct regulator *mpu_reg;
 
 static int omap_verify_speed(struct cpufreq_policy *policy)
 {
@@ -78,6 +80,8 @@ static int omap_target(struct cpufreq_policy *policy,
 	unsigned int i;
 	int ret = 0;
 	struct cpufreq_freqs freqs;
+	struct opp *opp;
+	unsigned long freq, volt;
 
 	if (!freq_table) {
 		dev_err(mpu_dev, "%s: cpu%d: no freq table!\n", __func__,
@@ -115,9 +119,26 @@ static int omap_target(struct cpufreq_policy *policy,
 	pr_info("cpufreq-omap: transition: %u --> %u\n", freqs.old, freqs.new);
 #endif
 
+	freq = freqs.new * 1000;
+	opp = opp_find_freq_ceil(mpu_dev, &freq);
+	if (IS_ERR(opp)) {
+		printk(KERN_ERR "%s: unable to find MPU OPP for %d\n",
+		       __func__, freqs.new);
+		return -EINVAL;
+	}
+	volt = opp_get_voltage(opp);
+
+	/* scaling up?  scale voltage before frequency */
+	if (mpu_reg && (freqs.new > freqs.old))
+		regulator_set_voltage(mpu_reg, volt, volt);
+
 	ret = clk_set_rate(mpu_clk, freqs.new * 1000);
-	freqs.new = omap_getspeed(policy->cpu);
 
+	/* scaling down?  scale voltage after frequency */
+	if (mpu_reg && (freqs.new < freqs.old))
+		regulator_set_voltage(mpu_reg, volt, volt);
+
+	freqs.new = omap_getspeed(policy->cpu);
 #ifdef CONFIG_SMP
 	/*
 	 * Note that loops_per_jiffy is not updated on SMP systems in
@@ -260,6 +281,12 @@ static int __init omap_cpufreq_init(void)
 		return -EINVAL;
 	}
 
+	mpu_reg = regulator_get(mpu_dev, "vcc");
+	if (IS_ERR(mpu_reg)) {
+		pr_warning("%s: unable to get MPU regulator\n", __func__);
+		mpu_reg = NULL;
+	}
+
 	return cpufreq_register_driver(&omap_driver);
 }
 
-- 
1.7.9

Re: [PATCH/RFT 1/1] OMAP2+: cpufreq: scale voltage along with frequency

[Jean Pihet]

Hi Kevin,

On Thu, Feb 16, 2012 at 11:20 AM, Kevin Hilman <khilman@ti.com> wrote:
> diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
> index 8a36342..140c032 100644
> --- a/arch/arm/mach-omap2/voltage.c
> +++ b/arch/arm/mach-omap2/voltage.c
> @@ -89,6 +89,8 @@ int voltdm_scale(struct voltagedomain *voltdm,
>        ret = voltdm->scale(voltdm, target_volt);
>        if (!ret)
>                voltdm->nominal_volt = target_volt;
> +       printk("KJH: %s: %d\n", __func__, target_volt);
> +       dump_stack();
The debugging letfovers need to be removed.

>
>        return ret;
>  }

Jean
--
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

Re: [PATCH/RFT 1/1] OMAP2+: cpufreq: scale voltage along with frequency

[Kevin Hilman]

Jean Pihet <jean.pihet@newoldbits.com> writes:

> Hi Kevin,
>
> On Thu, Feb 16, 2012 at 11:20 AM, Kevin Hilman <khilman@ti.com> wrote:
>> diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
>> index 8a36342..140c032 100644
>> --- a/arch/arm/mach-omap2/voltage.c
>> +++ b/arch/arm/mach-omap2/voltage.c
>> @@ -89,6 +89,8 @@ int voltdm_scale(struct voltagedomain *voltdm,
>>        ret = voltdm->scale(voltdm, target_volt);
>>        if (!ret)
>>                voltdm->nominal_volt = target_volt;
>> +       printk("KJH: %s: %d\n", __func__, target_volt);
>> +       dump_stack();
> The debugging letfovers need to be removed.

heh, that's why it's RFT.  :)

Here's a version without the stack dumping.


From 62541aac986ee8ba3b67f40de4610068b2d7fbd7 Mon Sep 17 00:00:00 2001
From: Kevin Hilman <khilman@ti.com>
Date: Fri, 15 Jul 2011 15:05:04 -0700
Subject: [PATCH/RFT 1/1] OMAP2+: cpufreq: scale voltage along with frequency

Use the regulator framework to get the voltage regulator associated
with the MPU voltage domain and use it to scale voltage along with
frequency.

Signed-off-by: Kevin Hilman <khilman@ti.com>
---
 drivers/cpufreq/omap-cpufreq.c |   29 ++++++++++++++++++++++++++++-
 1 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/drivers/cpufreq/omap-cpufreq.c b/drivers/cpufreq/omap-cpufreq.c
index 5d04c57..e4f4841 100644
--- a/drivers/cpufreq/omap-cpufreq.c
+++ b/drivers/cpufreq/omap-cpufreq.c
@@ -25,6 +25,7 @@
 #include <linux/opp.h>
 #include <linux/cpu.h>
 #include <linux/module.h>
+#include <linux/regulator/consumer.h>
 
 #include <asm/system.h>
 #include <asm/smp_plat.h>
@@ -52,6 +53,7 @@ static atomic_t freq_table_users = ATOMIC_INIT(0);
 static struct clk *mpu_clk;
 static char *mpu_clk_name;
 static struct device *mpu_dev;
+static struct regulator *mpu_reg;
 
 static int omap_verify_speed(struct cpufreq_policy *policy)
 {
@@ -78,6 +80,8 @@ static int omap_target(struct cpufreq_policy *policy,
 	unsigned int i;
 	int ret = 0;
 	struct cpufreq_freqs freqs;
+	struct opp *opp;
+	unsigned long freq, volt;
 
 	if (!freq_table) {
 		dev_err(mpu_dev, "%s: cpu%d: no freq table!\n", __func__,
@@ -115,9 +119,26 @@ static int omap_target(struct cpufreq_policy *policy,
 	pr_info("cpufreq-omap: transition: %u --> %u\n", freqs.old, freqs.new);
 #endif
 
+	freq = freqs.new * 1000;
+	opp = opp_find_freq_ceil(mpu_dev, &freq);
+	if (IS_ERR(opp)) {
+		printk(KERN_ERR "%s: unable to find MPU OPP for %d\n",
+		       __func__, freqs.new);
+		return -EINVAL;
+	}
+	volt = opp_get_voltage(opp);
+
+	/* scaling up?  scale voltage before frequency */
+	if (mpu_reg && (freqs.new > freqs.old))
+		regulator_set_voltage(mpu_reg, volt, volt);
+
 	ret = clk_set_rate(mpu_clk, freqs.new * 1000);
-	freqs.new = omap_getspeed(policy->cpu);
 
+	/* scaling down?  scale voltage after frequency */
+	if (mpu_reg && (freqs.new < freqs.old))
+		regulator_set_voltage(mpu_reg, volt, volt);
+
+	freqs.new = omap_getspeed(policy->cpu);
 #ifdef CONFIG_SMP
 	/*
 	 * Note that loops_per_jiffy is not updated on SMP systems in
@@ -260,6 +281,12 @@ static int __init omap_cpufreq_init(void)
 		return -EINVAL;
 	}
 
+	mpu_reg = regulator_get(mpu_dev, "vcc");
+	if (IS_ERR(mpu_reg)) {
+		pr_warning("%s: unable to get MPU regulator\n", __func__);
+		mpu_reg = NULL;
+	}
+
 	return cpufreq_register_driver(&omap_driver);
 }
 
-- 
1.7.9

--
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

Re: [PATCH/RFT 1/1] OMAP2+: cpufreq: scale voltage along with frequency

[Tero Kristo]

On Thu, 2012-02-16 at 11:36 -0800, Kevin Hilman wrote:
> Jean Pihet <jean.pihet@newoldbits.com> writes:
> 
> > Hi Kevin,
> >
> > On Thu, Feb 16, 2012 at 11:20 AM, Kevin Hilman <khilman@ti.com> wrote:
> >> diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
> >> index 8a36342..140c032 100644
> >> --- a/arch/arm/mach-omap2/voltage.c
> >> +++ b/arch/arm/mach-omap2/voltage.c
> >> @@ -89,6 +89,8 @@ int voltdm_scale(struct voltagedomain *voltdm,
> >>        ret = voltdm->scale(voltdm, target_volt);
> >>        if (!ret)
> >>                voltdm->nominal_volt = target_volt;
> >> +       printk("KJH: %s: %d\n", __func__, target_volt);
> >> +       dump_stack();
> > The debugging letfovers need to be removed.
> 
> heh, that's why it's RFT.  :)

<snip>

I tested this out with performance governor and limiting the
max_scaling_freq to different numbers. Clock frequency seems to change
based on arm_fck/rate debugfs entry, and the voltage is changing based
on both measurement and microvolts entry for the regulator.

However, twd_update_frequency crashes on omap3, as it doesn't have local
timer support but smp_twd registers a cpufreq notifier anyways.
Following patch fixes this issue (I'll send this out to l-o and l-arm
lists separately in a bit):

---
 arch/arm/kernel/smp_twd.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index 4285daa..dae8902 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -129,7 +129,7 @@ static struct notifier_block twd_cpufreq_nb = {
 
 static int twd_cpufreq_init(void)
 {
-       if (!IS_ERR(twd_clk))
+       if (twd_clk && !IS_ERR(twd_clk))
                return cpufreq_register_notifier(&twd_cpufreq_nb,
                        CPUFREQ_TRANSITION_NOTIFIER);
 
-- 
1.7.4.1

RE: [PATCH/RFT 1/1] OMAP2+: cpufreq: scale voltage along with frequency

[Mohammed, Afzal]

Hi Kevin,

On Fri, Feb 17, 2012 at 00:50:43, Hilman, Kevin wrote:
> +	/* scaling up?  scale voltage before frequency */
> +	if (mpu_reg && (freqs.new > freqs.old))
> +		regulator_set_voltage(mpu_reg, volt, volt);

Probably voltage ranges has to be specified, otherwise
if I understand correctly,  if exact voltage 'volt'
is a value that cannot be set by voltage regulator,
it may not work properly.

>  	ret = clk_set_rate(mpu_clk, freqs.new * 1000);
> -	freqs.new = omap_getspeed(policy->cpu);
>  
> +	/* scaling down?  scale voltage after frequency */
> +	if (mpu_reg && (freqs.new < freqs.old))
> +		regulator_set_voltage(mpu_reg, volt, volt);
> +
> +	freqs.new = omap_getspeed(policy->cpu);

It would be better to handle error cases too,
we have a patch for doing DVFS for AM335X as follows

Regards
Afzal


8<------------------------------------------------------

---
 drivers/cpufreq/omap-cpufreq.c |   97 +++++++++++++++++++++++++++++++++++++---
 1 files changed, 91 insertions(+), 6 deletions(-)

diff --git a/drivers/cpufreq/omap-cpufreq.c b/drivers/cpufreq/omap-cpufreq.c
index 5d04c57..a897a9e 100644
--- a/drivers/cpufreq/omap-cpufreq.c
+++ b/drivers/cpufreq/omap-cpufreq.c
@@ -25,6 +25,7 @@
 #include <linux/opp.h>
 #include <linux/cpu.h>
 #include <linux/module.h>
+#include <linux/regulator/consumer.h>

 #include <asm/system.h>
 #include <asm/smp_plat.h>
@@ -37,6 +38,8 @@

 #include <mach/hardware.h>

+#define DEFAULT_RESOLUTION 12500
+
 #ifdef CONFIG_SMP
 struct lpj_info {
        unsigned long   ref;
@@ -52,6 +55,7 @@ static atomic_t freq_table_users = ATOMIC_INIT(0);
 static struct clk *mpu_clk;
 static char *mpu_clk_name;
 static struct device *mpu_dev;
+static struct regulator *mpu_reg;

 static int omap_verify_speed(struct cpufreq_policy *policy)
 {
@@ -78,6 +82,8 @@ static int omap_target(struct cpufreq_policy *policy,
        unsigned int i;
        int ret = 0;
        struct cpufreq_freqs freqs;
+       struct opp *opp;
+       int volt_old = 0, volt_new = 0;

        if (!freq_table) {
                dev_err(mpu_dev, "%s: cpu%d: no freq table!\n", __func__,
@@ -105,16 +111,45 @@ static int omap_target(struct cpufreq_policy *policy,
        if (freqs.old == freqs.new && policy->cur == freqs.new)
                return ret;

+       opp = opp_find_freq_exact(mpu_dev, freqs.new * 1000, true);
+       if (IS_ERR(opp)) {
+               dev_err(mpu_dev, "%s: cpu%d: no opp match for freq %d\n",
+                       __func__, policy->cpu, target_freq);
+               return -EINVAL;
+       }
+
+       volt_new = opp_get_voltage(opp);
+       if (!volt_new) {
+               dev_err(mpu_dev, "%s: cpu%d: no opp voltage for freq %d\n",
+                       __func__, policy->cpu, target_freq);
+               return -EINVAL;
+       }
+
+       volt_old = regulator_get_voltage(mpu_reg);
+
+#ifdef CONFIG_CPU_FREQ_DEBUG
+       pr_info("cpufreq-omap: frequency transition: %u --> %u\n",
+                       freqs.old, freqs.new);
+       pr_info("cpufreq-omap: voltage transition: %d --> %d\n",
+                       volt_old, volt_new);
+#endif
+
+       if (freqs.new > freqs.old) {
+               ret = regulator_set_voltage(mpu_reg, volt_new,
+                       volt_new + DEFAULT_RESOLUTION - 1);
+               if (ret) {
+                       dev_err(mpu_dev, "%s: unable to set voltage to %d uV (for %u MHz)\n",
+                               __func__, volt_new, freqs.new/1000);
+                       return ret;
+               }
+       }
+
        /* notifiers */
        for_each_cpu(i, policy->cpus) {
                freqs.cpu = i;
                cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
        }

-#ifdef CONFIG_CPU_FREQ_DEBUG
-       pr_info("cpufreq-omap: transition: %u --> %u\n", freqs.old, freqs.new);
-#endif
-
        ret = clk_set_rate(mpu_clk, freqs.new * 1000);
        freqs.new = omap_getspeed(policy->cpu);

@@ -150,6 +185,38 @@ static int omap_target(struct cpufreq_policy *policy,
                cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
        }

+       if (freqs.new < freqs.old) {
+               ret = regulator_set_voltage(mpu_reg, volt_new,
+                       volt_new + DEFAULT_RESOLUTION - 1);
+               if (ret) {
+                       unsigned int temp;
+
+                       dev_err(mpu_dev, "%s: unable to set voltage to %d uV (for %u MHz)\n",
+                               __func__, volt_new, freqs.new/1000);
+
+                       if (clk_set_rate(mpu_clk, freqs.old * 1000)) {
+                               dev_err(mpu_dev,
+                                       "%s: failed restoring clock rate to %u MHz, clock rate is %u MHz",
+                                       __func__,
+                                       freqs.old/1000, freqs.new/1000);
+                               return ret;
+                       }
+
+                       temp = freqs.new;
+                       freqs.new = freqs.old;
+                       freqs.old = temp;
+
+                       for_each_cpu(i, policy->cpus) {
+                               freqs.cpu = i;
+                               cpufreq_notify_transition(&freqs,
+                                       CPUFREQ_PRECHANGE);
+                               cpufreq_notify_transition(&freqs,
+                                       CPUFREQ_POSTCHANGE);
+                       }
+                       return ret;
+               }
+       }
+
        return ret;
 }

@@ -167,11 +234,27 @@ static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
        if (IS_ERR(mpu_clk))
                return PTR_ERR(mpu_clk);

-       if (policy->cpu >= NR_CPUS) {
+       mpu_reg = regulator_get(NULL, "vdd_mpu");
+       if (IS_ERR(mpu_reg)) {
                result = -EINVAL;
                goto fail_ck;
        }

+       /* success of regulator_get doesn't gurantee presence of driver for
+          physical regulator and presence of physical regulator (this
+          situation arises if dummy regulator is enabled),so check voltage
+          to verify that physical regulator and it's driver is present
+        */
+       if (regulator_get_voltage(mpu_reg) < 0) {
+               result = -EINVAL;
+               goto fail_reg;
+       }
+
+       if (policy->cpu >= NR_CPUS) {
+               result = -EINVAL;
+               goto fail_reg;
+       }
+
        policy->cur = policy->min = policy->max = omap_getspeed(policy->cpu);

        if (atomic_inc_return(&freq_table_users) == 1)
@@ -180,7 +263,7 @@ static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
        if (result) {
                dev_err(mpu_dev, "%s: cpu%d: failed creating freq table[%d]\n",
                                __func__, policy->cpu, result);
-               goto fail_ck;
+               goto fail_reg;
        }

        result = cpufreq_frequency_table_cpuinfo(policy, freq_table);
@@ -212,6 +295,8 @@ static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)

 fail_table:
        freq_table_free();
+fail_reg:
+       regulator_put(mpu_reg);
 fail_ck:
        clk_put(mpu_clk);
        return result;
--
1.7.1

Re: [PATCH/RFT 1/1] OMAP2+: cpufreq: scale voltage along with frequency

[Kevin Hilman]

"Mohammed, Afzal" <afzal@ti.com> writes:

> Hi Kevin,
>
> On Fri, Feb 17, 2012 at 00:50:43, Hilman, Kevin wrote:
>> +	/* scaling up?  scale voltage before frequency */
>> +	if (mpu_reg && (freqs.new > freqs.old))
>> +		regulator_set_voltage(mpu_reg, volt, volt);
>
> Probably voltage ranges has to be specified, otherwise
> if I understand correctly,  if exact voltage 'volt'
> is a value that cannot be set by voltage regulator,
> it may not work properly.

In this case, volt comes from the OPP table, and was requested using a
rounding call into the OPP table, so the resolution problem is handled
there.  If 'volt' cannot be set by the regulator, then the OPP tables
are also broken.

Also, in your patch, you only add some offset.  If you want to be
approximate, shouldn't you have plus and minus?

IMO, we should let the OPP table handle that, and not the CPUfreq driver.

>>  	ret = clk_set_rate(mpu_clk, freqs.new * 1000);
>> -	freqs.new = omap_getspeed(policy->cpu);
>>  
>> +	/* scaling down?  scale voltage after frequency */
>> +	if (mpu_reg && (freqs.new < freqs.old))
>> +		regulator_set_voltage(mpu_reg, volt, volt);
>> +
>> +	freqs.new = omap_getspeed(policy->cpu);
>
> It would be better to handle error cases too,
> we have a patch for doing DVFS for AM335X as follows

I agree, my version is not very robust in the face of errors from the
regulator framework.

Hoever, I'm not crazy about the extra notifications in your proposed
patch.  I think it's cleaner to always pre and post notify.  If there's
a failure, the post notify will have the same freq as the pre-notify,
but that's not a big problem.

I'll send an updated patch that follows this approach instead:

- pre notify

- scale volage up
  - if fail, goto done

- scale freq

- scale voltage down
  - if fail
    - scale freq back
    - goto done

- SMP jiffies magic

done:
- post notify


Kevin

RE: [PATCH/RFT 1/1] OMAP2+: cpufreq: scale voltage along with frequency

[Mohammed, Afzal]

Hi Kevin,

On Wed, Feb 22, 2012 at 05:36:12, Hilman, Kevin wrote:
> In this case, volt comes from the OPP table, and was requested using a
> rounding call into the OPP table, so the resolution problem is handled
> there.  If 'volt' cannot be set by the regulator, then the OPP tables
> are also broken.
> 
> Also, in your patch, you only add some offset.  If you want to be
> approximate, shouldn't you have plus and minus?
> 
> IMO, we should let the OPP table handle that, and not the CPUfreq driver.

I have a different opinion.

Consider following case,

Voltage to set for OPPX is 1262.5mV and regulator has steps of 10mV,
and suppose regulator steps are .., 1260mV, 1270mV etc. Regulator
framework will not be able to set voltage for OPPX (certainly if
set_voltage_sel is used) if no range specified.

But instead if range of 1262.5 - (1262.5 + 10 - 1) is provided,
regulator can certainly set voltage for 1270mV (a nearest value)

Resolution in my patch was not meant to be resolution per se, it was meant
so that a voltage step can always be found for the regulator (assuming
worst resolution is 12.5mV), and for regulator to get a step, resolution
had to be used.

Ideal solution may be to use a variable to have a default resolution
of worst regulator and update it to that of regulator used (perhaps
making use of something like module parameters)

If regulator for a particular SoC is changed, exact OPP voltage may
not be settable, but a near value should be sufficient, this can't be
achieved if range is not specified. That happens exactly for AM335X,
voltage for one of OPP is 1.26V, but as regulator cannot set the
value, it is being set at 1.2625V and if no range is specified,
it will not work.

And in my patch plus - minus was not used as regulator framework will
try to set voltage for the least voltage which sometimes corresponds
to exact OPP required value.

For cpufreq omap driver to work with various regulators, it would be
better to specify ranges.

> I agree, my version is not very robust in the face of errors from the
> regulator framework.
> 
> Hoever, I'm not crazy about the extra notifications in your proposed
> patch.  I think it's cleaner to always pre and post notify.  If there's
> a failure, the post notify will have the same freq as the pre-notify,
> but that's not a big problem.


Yes, agree that error handling path is bulky. 

A problem that can happen is that drivers who has registered cpufreq
notifier will think that frequency has changed, that may cause problem
in addition to delay time getting altered.

But whether these are worth handling with a penalty of bulky error
handling, probably you know better.

Regards
Afzal

Re: [PATCH/RFT 1/1] OMAP2+: cpufreq: scale voltage along with frequency

[Kevin Hilman]

"Mohammed, Afzal" <afzal@ti.com> writes:

> Hi Kevin,
>
> On Wed, Feb 22, 2012 at 05:36:12, Hilman, Kevin wrote:
>> In this case, volt comes from the OPP table, and was requested using a
>> rounding call into the OPP table, so the resolution problem is handled
>> there.  If 'volt' cannot be set by the regulator, then the OPP tables
>> are also broken.
>> 
>> Also, in your patch, you only add some offset.  If you want to be
>> approximate, shouldn't you have plus and minus?
>> 
>> IMO, we should let the OPP table handle that, and not the CPUfreq driver.
>
> I have a different opinion.
>
> Consider following case,
>
> Voltage to set for OPPX is 1262.5mV and regulator has steps of 10mV,
> and suppose regulator steps are .., 1260mV, 1270mV etc. Regulator
> framework will not be able to set voltage for OPPX (certainly if
> set_voltage_sel is used) if no range specified.
>
> But instead if range of 1262.5 - (1262.5 + 10 - 1) is provided,
> regulator can certainly set voltage for 1270mV (a nearest value)
>
> Resolution in my patch was not meant to be resolution per se, it was meant
> so that a voltage step can always be found for the regulator (assuming
> worst resolution is 12.5mV), and for regulator to get a step, resolution
> had to be used.

I agree, if the OPP table doesn't represent actual voltages, then some
sort of range needs to be used.

> Ideal solution may be to use a variable to have a default resolution
> of worst regulator and update it to that of regulator used (perhaps
> making use of something like module parameters)
>
> If regulator for a particular SoC is changed, exact OPP voltage may
> not be settable, but a near value should be sufficient, this can't be
> achieved if range is not specified. That happens exactly for AM335X,
> voltage for one of OPP is 1.26V, but as regulator cannot set the
> value, it is being set at 1.2625V and if no range is specified,
> it will not work.
>
> And in my patch plus - minus was not used as regulator framework will
> try to set voltage for the least voltage which sometimes corresponds
> to exact OPP required value.

sometimes?

Using your example above, what if the closest value was 1.259V?
Wouldn't you then need +/- range?

> For cpufreq omap driver to work with various regulators, it would be
> better to specify ranges.

I am fine with that, and am open for proposals.  Feel free to send a
patch on top of my v2.

>> I agree, my version is not very robust in the face of errors from the
>> regulator framework.
>> 
>> Hoever, I'm not crazy about the extra notifications in your proposed
>> patch.  I think it's cleaner to always pre and post notify.  If there's
>> a failure, the post notify will have the same freq as the pre-notify,
>> but that's not a big problem.
>
>
> Yes, agree that error handling path is bulky. 
>
> A problem that can happen is that drivers who has registered cpufreq
> notifier will think that frequency has changed, that may cause problem
> in addition to delay time getting altered.
>
> But whether these are worth handling with a penalty of bulky error
> handling, probably you know better.

Please have a look at my v2.

The drivers will get pre and post notifiers, with the caveat that upon
DVFS failure, the freq in the post notifier might be the same as the one
in the pre notifier, indicating that no change was made.

Kevin

RE: [PATCH/RFT 1/1] OMAP2+: cpufreq: scale voltage along with frequency

[Mohammed, Afzal]

Hi Kevin,

On Thu, Feb 23, 2012 at 00:12:06, Hilman, Kevin wrote:
> > And in my patch plus - minus was not used as regulator framework will
> > try to set voltage for the least voltage which sometimes corresponds
> > to exact OPP required value.
> 
> sometimes?

I was not clear in my previous statement, let me explain it differently.

Regulator framework will always try to set lowest voltage in the range.
Upon, regulator_set_voltage(reg, OPPVOLT, (OPPVOLT+RESOLUTION-1)), if
OPPVOLT is a value that can be set by the regulator, that will be set by
the regulator, this is what I meant by 'sometimes'. Otherwise it will set
the next possible step.

> Using your example above, what if the closest value was 1.259V?
> Wouldn't you then need +/- range?

In that case, it will set to next step after 1.259V.

If +/- is used, it may happen that SoC will work for a particular frequency
at a voltage lower than it has been characterized (if you ask me to define
characterized, I don't know, but what I know is that SoC can work at a
lower frequency for the voltage of an OPP, but not vice versa). Still as
voltage will be only very less than that specified by OPP, it may not be
a problem to use +/-

> Please have a look at my v2.
> 
> The drivers will get pre and post notifiers, with the caveat that upon
> DVFS failure, the freq in the post notifier might be the same as the one
> in the pre notifier, indicating that no change was made.

Yes, I overlooked your freq.new update for error condition.

Earlier while adding support for DVFS on AM335X, it was noticed that, lpj
was going wrong after dvfs failure, then those error notifiers were added
to recover properly. Later it was realized that it was due to a bug in
cpufreq framework,

"d08de0c1  [CPUFREQ] update lpj only if frequency has changed"

fixed it, probably those extra handling in  my patch is not required now,
if drivers can understand your pre & post notifiers properly (if drivers
can't, then maybe it is driver problem)

Regards
Afzal

RE: [PATCH/RFT 1/1] OMAP2+: cpufreq: scale voltage along with frequency

[Mohammed, Afzal]

Hi Kevin,

On Thu, Feb 23, 2012 at 10:58:57, Mohammed, Afzal wrote:
> > Using your example above, what if the closest value was 1.259V?
> > Wouldn't you then need +/- range?
> 
> In that case, it will set to next step after 1.259V.
> 
> If +/- is used, it may happen that SoC will work for a particular frequency
> at a voltage lower than it has been characterized (if you ask me to define
> characterized, I don't know, but what I know is that SoC can work at a
> lower frequency for the voltage of an OPP, but not vice versa). Still as
> voltage will be only very less than that specified by OPP, it may not be
> a problem to use +/-

Upon discussing with Sekhar, realized that I was wrong in the above
statements, +/- OPP tolerance range should be used.

Regards
Afzal