diff mbox

[2/2,RFC] cpufreq: omap: scale regulator from clk notifier

Message ID 1342225001-22962-3-git-send-email-mturquette@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Mike Turquette July 14, 2012, 12:16 a.m. UTC 
This patch moves direct control of the MPU voltage regulator out of the
cpufreq driver .target callback and instead puts that logic into a clock
rate change notifier callback.

The same frequency/voltage lookup via the OPP library is present, except
that the calls to regulator_set_voltage are done from the clock
framework instead of cpufreq.

Ideally it would be nice to reduce the .target callback for OMAP's
cpufreq driver to a simple call to clk_set_rate.  For now there is still
some other stuff needed there (jiffies per loop, rounding the rate, etc
etc).

Not-signed-off-by: Mike Turquette <mturquette@linaro.org>
---
 drivers/cpufreq/omap-cpufreq.c |  154 +++++++++++++++++++++++++---------------
 1 file changed, 96 insertions(+), 58 deletions(-)

Comments

Linus Walleij July 16, 2012, 10:28 p.m. UTC | #1 
On Sat, Jul 14, 2012 at 2:16 AM, Mike Turquette <mturquette@linaro.org> wrote:
'
> This patch moves direct control of the MPU voltage regulator out of the
> cpufreq driver .target callback and instead puts that logic into a clock
> rate change notifier callback.

That's heavy stuff.

I was hoping that the first example of using clk notifiers would be
something like mach-davinci replacing it's legacy clock framework
with drivers/clk and then go in and change the horrid cpufreq hack
that is currently in drivers/mmc/host/davinci_mmc.c to use a
clock notifier instead of cpufreq.

Sekhar/Kevin, any chance to have DaVinci converted to Mikes new
clock framework? It doesn't look super-complex and would be a
good example for others I think.

Yours,
Linus Walleij
Mike Turquette July 16, 2012, 11:22 p.m. UTC | #2 
On Mon, Jul 16, 2012 at 3:28 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Sat, Jul 14, 2012 at 2:16 AM, Mike Turquette <mturquette@linaro.org> wrote:
> '
>> This patch moves direct control of the MPU voltage regulator out of the
>> cpufreq driver .target callback and instead puts that logic into a clock
>> rate change notifier callback.
>
> That's heavy stuff.
>

Gravity is stronger here.

CPU dvfs is the least interesting dvfs in my opinion, since it has
been solved by CPUfreq for eons.  However it was the lowest hanging
fruit for demoing the idea behind dvfs-via-clk-notifer.

This is a chicken before the egg problem: drivers are not adapted for
dvfs since we have no dvfs api.  In trying to demonstrate
dvfs-via-clk-notifier it was much easier for me to hijack one of the
few existing sources of dvfs in the kernel: cpufreq.  If my platform
of choice (pandaboard) had any device which supported devfreq (e.g.
gpu, ddr or mmc controller) then I might have chosen that instead, but
the change would have been essentially the same as the one above.

> I was hoping that the first example of using clk notifiers would be
> something like mach-davinci replacing it's legacy clock framework
> with drivers/clk and then go in and change the horrid cpufreq hack
> that is currently in drivers/mmc/host/davinci_mmc.c to use a
> clock notifier instead of cpufreq.
>

I am not familiar with the issue you're talking about here, but it
sounds more like some clk functional dependencies have been stuffed
into the davinci cpufreq driver?  clk notifiers can handle that as
well, and the reentrancy issues would remain (calling clk_set_rate
from within clk_set_rate).  The point of this series is that the state
of reentrancy in the clk framework sucks and I could use some
collective brainpower to improve it :-)

> Sekhar/Kevin, any chance to have DaVinci converted to Mikes new
> clock framework? It doesn't look super-complex and would be a
> good example for others I think.
>

Again, I'd love more platform ports to the common clock framework, but
I hope that the purpose of this RFC is not lost in the noise.  Devices
like off-soc audio chips and pmics are screwed without some change to
the core code and we are not advancing the state of dvfs in an
upstream kernel until some of these issues are sorted.

Thanks much,
Mike

> Yours,
> Linus Walleij
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Prashant Gaikwad July 17, 2012, 5:20 a.m. UTC | #3 
On Saturday 14 July 2012 05:46 AM, Mike Turquette wrote:
> This patch moves direct control of the MPU voltage regulator out of the
> cpufreq driver .target callback and instead puts that logic into a clock
> rate change notifier callback.
>
> The same frequency/voltage lookup via the OPP library is present, except
> that the calls to regulator_set_voltage are done from the clock
> framework instead of cpufreq.
>
> Ideally it would be nice to reduce the .target callback for OMAP's
> cpufreq driver to a simple call to clk_set_rate.  For now there is still
> some other stuff needed there (jiffies per loop, rounding the rate, etc
> etc).
>
> Not-signed-off-by: Mike Turquette<mturquette@linaro.org>
> ---
>   drivers/cpufreq/omap-cpufreq.c |  154 +++++++++++++++++++++++++---------------
>   1 file changed, 96 insertions(+), 58 deletions(-)
>

<snip>

>
> -static int __init omap_cpufreq_init(void)
> +static int mpu_clk_volt_scale_handler(struct notifier_block *nb,
> +	unsigned long flags, void *data)
>   {
> -	if (cpu_is_omap24xx())
> -		mpu_clk_name = "virt_prcm_set";
> -	else if (cpu_is_omap34xx())
> -		mpu_clk_name = "dpll1_ck";
> -	else if (cpu_is_omap44xx())
> -		mpu_clk_name = "dpll_mpu_ck";
> +	struct clk_notifier_data *cnd = data;
> +	unsigned long tol;
> +	int ret, volt_new, volt_old;
> +	struct opp *opp;
>
> -	if (!mpu_clk_name) {
> -		pr_err("%s: unsupported Silicon?\n", __func__);
> -		return -EINVAL;
> +	volt_old = regulator_get_voltage(mpu_reg);
> +	opp = opp_find_freq_exact(mpu_dev, cnd->new_rate, true);
> +	volt_new = opp_get_voltage(opp);
> +
> +	tol = volt_new * OPP_TOLERANCE / 100;
> +
> +	/* scaling up?  scale voltage before frequency */
> +	if (cnd->new_rate>  cnd->old_rate) {
> +		dev_dbg(mpu_dev, "cpufreq-omap: %d mV -->  %d mV\n",
> +				volt_old, volt_new);
> +
> +		ret = regulator_set_voltage(mpu_reg, volt_new - tol, volt_new + tol);
> +
> +		if (ret<  0) {
> +			dev_warn(mpu_dev, "%s: unable to scale voltage up.\n",
> +				 __func__);
> +			return NOTIFY_BAD;
> +		}
> +	}
> +
> +	/* scaling down?  scale voltage after frequency */
> +	if (cnd->new_rate<  cnd->old_rate) {
> +		dev_dbg(mpu_dev, "cpufreq-omap: %d mV -->  %d mV\n",
> +				volt_old, volt_new);
> +
> +		ret = regulator_set_voltage(mpu_reg, volt_new - tol, volt_new + tol);
> +
> +		if (ret<  0) {
> +			dev_warn(mpu_dev, "%s: unable to scale voltage down.\n",
> +				 __func__);
> +			return NOTIFY_BAD;
> +		}
>   	}

How are you checking pre and post rate change condition here? Need 
switch case for event?

>
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block mpu_clk_volt_scale_nb = {
> +	.notifier_call = mpu_clk_volt_scale_handler,
> +};
> +
> +
Mike Turquette July 17, 2012, 3:42 p.m. UTC | #4 
On 20120717-10:50, Prashant Gaikwad wrote:
> On Saturday 14 July 2012 05:46 AM, Mike Turquette wrote:
> >+	/* scaling up?  scale voltage before frequency */
> >+	if (cnd->new_rate>  cnd->old_rate) {
> >+		dev_dbg(mpu_dev, "cpufreq-omap: %d mV -->  %d mV\n",
> >+				volt_old, volt_new);
> >+
> >+		ret = regulator_set_voltage(mpu_reg, volt_new - tol, volt_new + tol);
> >+
> >+		if (ret<  0) {
> >+			dev_warn(mpu_dev, "%s: unable to scale voltage up.\n",
> >+				 __func__);
> >+			return NOTIFY_BAD;
> >+		}
> >+	}
> >+
> >+	/* scaling down?  scale voltage after frequency */
> >+	if (cnd->new_rate<  cnd->old_rate) {
> >+		dev_dbg(mpu_dev, "cpufreq-omap: %d mV -->  %d mV\n",
> >+				volt_old, volt_new);
> >+
> >+		ret = regulator_set_voltage(mpu_reg, volt_new - tol, volt_new + tol);
> >+
> >+		if (ret<  0) {
> >+			dev_warn(mpu_dev, "%s: unable to scale voltage down.\n",
> >+				 __func__);
> >+			return NOTIFY_BAD;
> >+		}
> >  	}
> 
> How are you checking pre and post rate change condition here? Need
> switch case for event?
>

Oops.  This version of the patch is missing that.  I previously had a
version of the patch which did pre/post checking but all of this code
was in arch/arm/mach-omap2/.  It looks like when I moved things around I
accidentally dropped that bit.

Anyways that is trivial to do and simply changes like the following:

if (cnd->new_rate > cnd->old_rate && flags & CLK_PRE_RATE_CHANGE)
	...

if (cnd->new_rate < cnd->old_rate && flags & CLK_POST_RATE_CHANGE)
	...

Regards,
Mike
Sekhar Nori July 20, 2012, 1:26 p.m. UTC | #5 
Hi Linus,

On 7/17/2012 3:58 AM, Linus Walleij wrote:
> On Sat, Jul 14, 2012 at 2:16 AM, Mike Turquette <mturquette@linaro.org> wrote:
> '
>> This patch moves direct control of the MPU voltage regulator out of the
>> cpufreq driver .target callback and instead puts that logic into a clock
>> rate change notifier callback.
> 
> That's heavy stuff.
> 
> I was hoping that the first example of using clk notifiers would be
> something like mach-davinci replacing it's legacy clock framework
> with drivers/clk and then go in and change the horrid cpufreq hack
> that is currently in drivers/mmc/host/davinci_mmc.c to use a
> clock notifier instead of cpufreq.
> 
> Sekhar/Kevin, any chance to have DaVinci converted to Mikes new
> clock framework? It doesn't look super-complex and would be a
> good example for others I think.

Murali (CCed) from TI is planning to work on it and should be posting
patches in 3rd/4th week of August. Once he is done, will work on
migrating the drivers to clock notifiers.

Thanks,
Sekhar
diff mbox

Patch

diff --git a/drivers/cpufreq/omap-cpufreq.c b/drivers/cpufreq/omap-cpufreq.c
index 17fa04d..bf15c49 100644
--- a/drivers/cpufreq/omap-cpufreq.c
+++ b/drivers/cpufreq/omap-cpufreq.c
@@ -80,10 +80,9 @@  static int omap_target(struct cpufreq_policy *policy,
 		       unsigned int relation)
 {
 	unsigned int i;
-	int r, ret = 0;
+	int ret = 0;
 	struct cpufreq_freqs freqs;
-	struct opp *opp;
-	unsigned long freq, volt = 0, volt_old = 0, tol = 0;
+	unsigned long freq;
 
 	if (!freq_table) {
 		dev_err(mpu_dev, "%s: cpu%d: no freq table!\n", __func__,
@@ -119,47 +118,11 @@  static int omap_target(struct cpufreq_policy *policy,
 
 	freq = freqs.new * 1000;
 
-	if (mpu_reg) {
-		opp = opp_find_freq_ceil(mpu_dev, &freq);
-		if (IS_ERR(opp)) {
-			dev_err(mpu_dev, "%s: unable to find MPU OPP for %d\n",
-				__func__, freqs.new);
-			return -EINVAL;
-		}
-		volt = opp_get_voltage(opp);
-		tol = volt * OPP_TOLERANCE / 100;
-		volt_old = regulator_get_voltage(mpu_reg);
-	}
-
-	dev_dbg(mpu_dev, "cpufreq-omap: %u MHz, %ld mV --> %u MHz, %ld mV\n", 
-		freqs.old / 1000, volt_old ? volt_old / 1000 : -1,
-		freqs.new / 1000, volt ? volt / 1000 : -1);
-
-	/* scaling up?  scale voltage before frequency */
-	if (mpu_reg && (freqs.new > freqs.old)) {
-		r = regulator_set_voltage(mpu_reg, volt - tol, volt + tol);
-		if (r < 0) {
-			dev_warn(mpu_dev, "%s: unable to scale voltage up.\n",
-				 __func__);
-			freqs.new = freqs.old;
-			goto done;
-		}
-	}
+	dev_dbg(mpu_dev, "cpufreq-omap: %u MHz --> %u MHz\n",
+			freqs.old / 1000, freqs.new / 1000); 
 
 	ret = clk_set_rate(mpu_clk, freqs.new * 1000);
 
-	/* scaling down?  scale voltage after frequency */
-	if (mpu_reg && (freqs.new < freqs.old)) {
-		r = regulator_set_voltage(mpu_reg, volt - tol, volt + tol);
-		if (r < 0) {
-			dev_warn(mpu_dev, "%s: unable to scale voltage down.\n",
-				 __func__);
-			ret = clk_set_rate(mpu_clk, freqs.old * 1000);
-			freqs.new = freqs.old;
-			goto done;
-		}
-	}
-
 	freqs.new = omap_getspeed(policy->cpu);
 #ifdef CONFIG_SMP
 	/*
@@ -187,7 +150,6 @@  static int omap_target(struct cpufreq_policy *policy,
 					freqs.new);
 #endif
 
-done:
 	/* notifiers */
 	for_each_cpu(i, policy->cpus) {
 		freqs.cpu = i;
@@ -207,10 +169,6 @@  static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
 {
 	int result = 0;
 
-	mpu_clk = clk_get(NULL, mpu_clk_name);
-	if (IS_ERR(mpu_clk))
-		return PTR_ERR(mpu_clk);
-
 	if (policy->cpu >= NR_CPUS) {
 		result = -EINVAL;
 		goto fail_ck;
@@ -284,32 +242,74 @@  static struct cpufreq_driver omap_driver = {
 	.attr		= omap_cpufreq_attr,
 };
 
-static int __init omap_cpufreq_init(void)
+static int mpu_clk_volt_scale_handler(struct notifier_block *nb,
+	unsigned long flags, void *data)
 {
-	if (cpu_is_omap24xx())
-		mpu_clk_name = "virt_prcm_set";
-	else if (cpu_is_omap34xx())
-		mpu_clk_name = "dpll1_ck";
-	else if (cpu_is_omap44xx())
-		mpu_clk_name = "dpll_mpu_ck";
+	struct clk_notifier_data *cnd = data;
+	unsigned long tol;
+	int ret, volt_new, volt_old;
+	struct opp *opp;
 
-	if (!mpu_clk_name) {
-		pr_err("%s: unsupported Silicon?\n", __func__);
-		return -EINVAL;
+	volt_old = regulator_get_voltage(mpu_reg);
+	opp = opp_find_freq_exact(mpu_dev, cnd->new_rate, true);
+	volt_new = opp_get_voltage(opp);
+
+	tol = volt_new * OPP_TOLERANCE / 100;
+
+	/* scaling up?  scale voltage before frequency */
+	if (cnd->new_rate > cnd->old_rate) {
+		dev_dbg(mpu_dev, "cpufreq-omap: %d mV --> %d mV\n",
+				volt_old, volt_new);
+
+		ret = regulator_set_voltage(mpu_reg, volt_new - tol, volt_new + tol);
+
+		if (ret < 0) {
+			dev_warn(mpu_dev, "%s: unable to scale voltage up.\n",
+				 __func__);
+			return NOTIFY_BAD;
+		}
+	}
+
+	/* scaling down?  scale voltage after frequency */
+	if (cnd->new_rate < cnd->old_rate) {
+		dev_dbg(mpu_dev, "cpufreq-omap: %d mV --> %d mV\n",
+				volt_old, volt_new);
+
+		ret = regulator_set_voltage(mpu_reg, volt_new - tol, volt_new + tol);
+
+		if (ret< 0) {
+			dev_warn(mpu_dev, "%s: unable to scale voltage down.\n",
+				 __func__);
+			return NOTIFY_BAD;
+		}
 	}
 
+	return NOTIFY_OK;
+}
+
+static struct notifier_block mpu_clk_volt_scale_nb = {
+	.notifier_call = mpu_clk_volt_scale_handler,
+};
+
+static int __init omap_regulator_init(void)
+{
+	int ret = 0;
+
 	mpu_dev = omap_device_get_by_hwmod_name("mpu");
 	if (!mpu_dev) {
 		pr_warning("%s: unable to get the mpu device\n", __func__);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out;
 	}
 
 	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;
+		ret = -ENODEV;
+		goto out;
 	} else {
-		/* 
+		/*
 		 * Ensure physical regulator is present.
 		 * (e.g. could be dummy regulator.)
 		 */
@@ -318,9 +318,47 @@  static int __init omap_cpufreq_init(void)
 				__func__);
 			regulator_put(mpu_reg);
 			mpu_reg = NULL;
+			ret = -ENODEV;
+			goto out;
 		}
 	}
 
+out:
+	return ret;
+}
+
+static int __init omap_cpufreq_init(void)
+{
+	int ret;
+
+	if (cpu_is_omap24xx())
+		mpu_clk_name = "virt_prcm_set";
+	else if (cpu_is_omap34xx())
+		mpu_clk_name = "dpll1_ck";
+	else if (cpu_is_omap44xx())
+		mpu_clk_name = "dpll_mpu_ck";
+
+	if (!mpu_clk_name) {
+		pr_err("%s: unsupported Silicon?\n", __func__);
+		return -EINVAL;
+	}
+
+	mpu_clk = clk_get(NULL, mpu_clk_name);
+	if (IS_ERR(mpu_clk))
+		return PTR_ERR(mpu_clk);
+
+	ret = omap_regulator_init();
+	if (ret) {
+		pr_err("%s: failed to get mpu regulator\n", __func__);
+		pr_err("%s: skipping clk notifier registration\n", __func__);
+		goto out;
+	}
+
+	ret = clk_notifier_register(mpu_clk, &mpu_clk_volt_scale_nb);
+	if (ret)
+		pr_err("%s: failed to register clk notifier\n", __func__);
+
+out:
 	return cpufreq_register_driver(&omap_driver);
 }