[PATCH] ARM: smp_twd: reprogram twd based on clk notifier

[PATCH] ARM: smp_twd: reprogram twd based on clk notifier

[Shawn Guo]

From: Mike Turquette <mturquette@linaro.org>

Running cpufreq driver on imx6q, the following warning is seen.

$ BUG: sleeping function called from invalid context at kernel/mutex.c:269

<snip>

stack backtrace:
Backtrace:
[<80011d64>] (dump_backtrace+0x0/0x10c) from [<803fc164>] (dump_stack+0x18/0x1c)
 r6:bf8142e0 r5:bf814000 r4:806ac794 r3:bf814000
[<803fc14c>] (dump_stack+0x0/0x1c) from [<803fd444>] (print_usage_bug+0x250/0x2b
8)
[<803fd1f4>] (print_usage_bug+0x0/0x2b8) from [<80060f90>] (mark_lock+0x56c/0x67
0)
[<80060a24>] (mark_lock+0x0/0x670) from [<80061a20>] (__lock_acquire+0x98c/0x19b
4)
[<80061094>] (__lock_acquire+0x0/0x19b4) from [<80062f14>] (lock_acquire+0x68/0x
7c)
[<80062eac>] (lock_acquire+0x0/0x7c) from [<80400f28>] (mutex_lock_nested+0x78/0
x344)
 r7:00000000 r6:bf872000 r5:805cc858 r4:805c2a04
[<80400eb0>] (mutex_lock_nested+0x0/0x344) from [<803089ac>] (clk_get_rate+0x1c/
0x58)
[<80308990>] (clk_get_rate+0x0/0x58) from [<80013c48>] (twd_update_frequency+0x1
8/0x50)
 r5:bf253d04 r4:805cadf4
[<80013c30>] (twd_update_frequency+0x0/0x50) from [<80068e20>] (generic_smp_call
_function_single_interrupt+0xd4/0x13c)
 r4:bf873ee0 r3:80013c30
[<80068d4c>] (generic_smp_call_function_single_interrupt+0x0/0x13c) from [<80013
34c>] (handle_IPI+0xc0/0x194)
 r8:00000001 r7:00000000 r6:80574e48 r5:bf872000 r4:80593958
[<8001328c>] (handle_IPI+0x0/0x194) from [<800084e8>] (gic_handle_irq+0x58/0x60)
 r8:00000000 r7:bf873f8c r6:bf873f58 r5:80593070 r4:f4000100
r3:00000005
[<80008490>] (gic_handle_irq+0x0/0x60) from [<8000e124>] (__irq_svc+0x44/0x60)
Exception stack(0xbf873f58 to 0xbf873fa0)
3f40:                                                       00000001 00000001
3f60: 00000000 bf814000 bf872000 805cab48 80405aa4 80597648 00000000 412fc09a
3f80: bf872000 bf873fac bf873f70 bf873fa0 80063844 8000f1f8 20000013 ffffffff
 r6:ffffffff r5:20000013 r4:8000f1f8 r3:bf814000
[<8000f1b8>] (default_idle+0x0/0x4c) from [<8000f428>] (cpu_idle+0x98/0x114)
[<8000f390>] (cpu_idle+0x0/0x114) from [<803f9834>] (secondary_start_kernel+0x11
c/0x140)
[<803f9718>] (secondary_start_kernel+0x0/0x140) from [<103f9234>] (0x103f9234)
 r6:10c03c7d r5:0000001f r4:4f86806a r3:803f921c

It looks that the warning is caused by that twd_update_frequency() gets
called from an atomic context while it calls clk_get_rate() where
a mutex gets held.

To fix the warning, let's convert the existing code to use clk notifiers
in place of CPUfreq notifiers.  This works out nicely for Cortex-A9
MPcore designs that scale all CPUs at the same frequency.

Signed-off-by: Mike Turquette <mturquette@linaro.org>
Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 arch/arm/kernel/smp_twd.c |   32 +++++++++++++-------------------
 1 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index fef42b2..635e3ea 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -11,7 +11,6 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/clk.h>
-#include <linux/cpufreq.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/err.h>
@@ -96,51 +95,46 @@ static void twd_timer_stop(struct clock_event_device *clk)
 	disable_percpu_irq(clk->irq);
 }
 
-#ifdef CONFIG_CPU_FREQ
-
 /*
  * Updates clockevent frequency when the cpu frequency changes.
  * Called on the cpu that is changing frequency with interrupts disabled.
  */
-static void twd_update_frequency(void *data)
+static void twd_update_frequency(void *new_rate)
 {
-	twd_timer_rate = clk_get_rate(twd_clk);
+	twd_timer_rate = *((unsigned long *) new_rate);
 
 	clockevents_update_freq(*__this_cpu_ptr(twd_evt), twd_timer_rate);
 }
 
-static int twd_cpufreq_transition(struct notifier_block *nb,
-	unsigned long state, void *data)
+static int twd_rate_change(struct notifier_block *nb,
+	unsigned long flags, void *data)
 {
-	struct cpufreq_freqs *freqs = data;
+	struct clk_notifier_data *cnd = data;
 
 	/*
 	 * The twd clock events must be reprogrammed to account for the new
 	 * frequency.  The timer is local to a cpu, so cross-call to the
 	 * changing cpu.
 	 */
-	if (state == CPUFREQ_POSTCHANGE || state == CPUFREQ_RESUMECHANGE)
-		smp_call_function_single(freqs->cpu, twd_update_frequency,
-			NULL, 1);
+	if (flags == POST_RATE_CHANGE)
+		smp_call_function(twd_update_frequency,
+				  (void *)&cnd->new_rate, 1);
 
 	return NOTIFY_OK;
 }
 
-static struct notifier_block twd_cpufreq_nb = {
-	.notifier_call = twd_cpufreq_transition,
+static struct notifier_block twd_clk_nb = {
+	.notifier_call = twd_rate_change,
 };
 
-static int twd_cpufreq_init(void)
+static int twd_clk_init(void)
 {
 	if (twd_evt && *__this_cpu_ptr(twd_evt) && !IS_ERR(twd_clk))
-		return cpufreq_register_notifier(&twd_cpufreq_nb,
-			CPUFREQ_TRANSITION_NOTIFIER);
+		return clk_notifier_register(twd_clk, &twd_clk_nb);
 
 	return 0;
 }
-core_initcall(twd_cpufreq_init);
-
-#endif
+core_initcall(twd_clk_init);
 
 static void __cpuinit twd_calibrate_rate(void)
 {
-- 
1.7.5.4

[PATCH] ARM: smp_twd: reprogram twd based on clk notifier

[Ulf Hansson]

Hi Shawn and Mike,

In general I am in favor of this patch, but I also see some
corresponding problems to resolve. Please correct me if I am wrong.

I suppose most machines is relying on that it enough to only take care
of the notification by using "cpufreq_notify_transition" from their
cpufreq drivers. Thus those have to make sure the updates for smp_twd
clock is supported from within the cpufreq driver I suppose.

In other words the cpufreq drivers needs to do clk_get of "smp_twd"
clock and then do a set_rate on it when it's has changed, to trigger a
rate change notification. Is this what you also have in mind or do you
have any other idea of how this should work?

Kind regards
Ulf Hansson

On 7 September 2012 10:19, Shawn Guo <shawn.guo@linaro.org> wrote:
> From: Mike Turquette <mturquette@linaro.org>
>
> Running cpufreq driver on imx6q, the following warning is seen.
>
> $ BUG: sleeping function called from invalid context at kernel/mutex.c:269
>
> <snip>
>
> stack backtrace:
> Backtrace:
> [<80011d64>] (dump_backtrace+0x0/0x10c) from [<803fc164>] (dump_stack+0x18/0x1c)
>  r6:bf8142e0 r5:bf814000 r4:806ac794 r3:bf814000
> [<803fc14c>] (dump_stack+0x0/0x1c) from [<803fd444>] (print_usage_bug+0x250/0x2b
> 8)
> [<803fd1f4>] (print_usage_bug+0x0/0x2b8) from [<80060f90>] (mark_lock+0x56c/0x67
> 0)
> [<80060a24>] (mark_lock+0x0/0x670) from [<80061a20>] (__lock_acquire+0x98c/0x19b
> 4)
> [<80061094>] (__lock_acquire+0x0/0x19b4) from [<80062f14>] (lock_acquire+0x68/0x
> 7c)
> [<80062eac>] (lock_acquire+0x0/0x7c) from [<80400f28>] (mutex_lock_nested+0x78/0
> x344)
>  r7:00000000 r6:bf872000 r5:805cc858 r4:805c2a04
> [<80400eb0>] (mutex_lock_nested+0x0/0x344) from [<803089ac>] (clk_get_rate+0x1c/
> 0x58)
> [<80308990>] (clk_get_rate+0x0/0x58) from [<80013c48>] (twd_update_frequency+0x1
> 8/0x50)
>  r5:bf253d04 r4:805cadf4
> [<80013c30>] (twd_update_frequency+0x0/0x50) from [<80068e20>] (generic_smp_call
> _function_single_interrupt+0xd4/0x13c)
>  r4:bf873ee0 r3:80013c30
> [<80068d4c>] (generic_smp_call_function_single_interrupt+0x0/0x13c) from [<80013
> 34c>] (handle_IPI+0xc0/0x194)
>  r8:00000001 r7:00000000 r6:80574e48 r5:bf872000 r4:80593958
> [<8001328c>] (handle_IPI+0x0/0x194) from [<800084e8>] (gic_handle_irq+0x58/0x60)
>  r8:00000000 r7:bf873f8c r6:bf873f58 r5:80593070 r4:f4000100
> r3:00000005
> [<80008490>] (gic_handle_irq+0x0/0x60) from [<8000e124>] (__irq_svc+0x44/0x60)
> Exception stack(0xbf873f58 to 0xbf873fa0)
> 3f40:                                                       00000001 00000001
> 3f60: 00000000 bf814000 bf872000 805cab48 80405aa4 80597648 00000000 412fc09a
> 3f80: bf872000 bf873fac bf873f70 bf873fa0 80063844 8000f1f8 20000013 ffffffff
>  r6:ffffffff r5:20000013 r4:8000f1f8 r3:bf814000
> [<8000f1b8>] (default_idle+0x0/0x4c) from [<8000f428>] (cpu_idle+0x98/0x114)
> [<8000f390>] (cpu_idle+0x0/0x114) from [<803f9834>] (secondary_start_kernel+0x11
> c/0x140)
> [<803f9718>] (secondary_start_kernel+0x0/0x140) from [<103f9234>] (0x103f9234)
>  r6:10c03c7d r5:0000001f r4:4f86806a r3:803f921c
>
> It looks that the warning is caused by that twd_update_frequency() gets
> called from an atomic context while it calls clk_get_rate() where
> a mutex gets held.
>
> To fix the warning, let's convert the existing code to use clk notifiers
> in place of CPUfreq notifiers.  This works out nicely for Cortex-A9
> MPcore designs that scale all CPUs at the same frequency.
>
> Signed-off-by: Mike Turquette <mturquette@linaro.org>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  arch/arm/kernel/smp_twd.c |   32 +++++++++++++-------------------
>  1 files changed, 13 insertions(+), 19 deletions(-)
>
> diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
> index fef42b2..635e3ea 100644
> --- a/arch/arm/kernel/smp_twd.c
> +++ b/arch/arm/kernel/smp_twd.c
> @@ -11,7 +11,6 @@
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/clk.h>
> -#include <linux/cpufreq.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
> @@ -96,51 +95,46 @@ static void twd_timer_stop(struct clock_event_device *clk)
>         disable_percpu_irq(clk->irq);
>  }
>
> -#ifdef CONFIG_CPU_FREQ
> -
>  /*
>   * Updates clockevent frequency when the cpu frequency changes.
>   * Called on the cpu that is changing frequency with interrupts disabled.
>   */
> -static void twd_update_frequency(void *data)
> +static void twd_update_frequency(void *new_rate)
>  {
> -       twd_timer_rate = clk_get_rate(twd_clk);
> +       twd_timer_rate = *((unsigned long *) new_rate);
>
>         clockevents_update_freq(*__this_cpu_ptr(twd_evt), twd_timer_rate);
>  }
>
> -static int twd_cpufreq_transition(struct notifier_block *nb,
> -       unsigned long state, void *data)
> +static int twd_rate_change(struct notifier_block *nb,
> +       unsigned long flags, void *data)
>  {
> -       struct cpufreq_freqs *freqs = data;
> +       struct clk_notifier_data *cnd = data;
>
>         /*
>          * The twd clock events must be reprogrammed to account for the new
>          * frequency.  The timer is local to a cpu, so cross-call to the
>          * changing cpu.
>          */
> -       if (state == CPUFREQ_POSTCHANGE || state == CPUFREQ_RESUMECHANGE)
> -               smp_call_function_single(freqs->cpu, twd_update_frequency,
> -                       NULL, 1);
> +       if (flags == POST_RATE_CHANGE)
> +               smp_call_function(twd_update_frequency,
> +                                 (void *)&cnd->new_rate, 1);
>
>         return NOTIFY_OK;
>  }
>
> -static struct notifier_block twd_cpufreq_nb = {
> -       .notifier_call = twd_cpufreq_transition,
> +static struct notifier_block twd_clk_nb = {
> +       .notifier_call = twd_rate_change,
>  };
>
> -static int twd_cpufreq_init(void)
> +static int twd_clk_init(void)
>  {
>         if (twd_evt && *__this_cpu_ptr(twd_evt) && !IS_ERR(twd_clk))
> -               return cpufreq_register_notifier(&twd_cpufreq_nb,
> -                       CPUFREQ_TRANSITION_NOTIFIER);
> +               return clk_notifier_register(twd_clk, &twd_clk_nb);
>
>         return 0;
>  }
> -core_initcall(twd_cpufreq_init);
> -
> -#endif
> +core_initcall(twd_clk_init);
>
>  static void __cpuinit twd_calibrate_rate(void)
>  {
> --
> 1.7.5.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] ARM: smp_twd: reprogram twd based on clk notifier

[Shawn Guo]

On Fri, Sep 07, 2012 at 01:59:40PM +0200, Ulf Hansson wrote:
> Hi Shawn and Mike,
> 
> In general I am in favor of this patch, but I also see some
> corresponding problems to resolve. Please correct me if I am wrong.
> 
> I suppose most machines is relying on that it enough to only take care
> of the notification by using "cpufreq_notify_transition" from their
> cpufreq drivers. Thus those have to make sure the updates for smp_twd
> clock is supported from within the cpufreq driver I suppose.
> 
> In other words the cpufreq drivers needs to do clk_get of "smp_twd"
> clock and then do a set_rate on it when it's has changed, to trigger a
> rate change notification. Is this what you also have in mind or do you
> have any other idea of how this should work?
> 
Here is how it works on imx6q.

1) There are clks arm and twd defined in imx6q clock tree.  And twd
   is registered to clock framework as one child of arm clk with a fixed
   divider 2.

	/*                                    name         parent_name     mult div */
	clk[twd]       = imx_clk_fixed_factor("twd",       "arm",            1, 2);

2) Register lookup "cpu0" and "smp_twd" for cpufreq and smp_twd drivers
   respectively.

	clk_register_clkdev(clk[twd], NULL, "smp_twd");
	clk_register_clkdev(clk[arm], NULL, "cpu0");

That's it.  The clock framework will just handle all the work nicely.
When cpufreq driver scales arm clk, the framework will propagate the
rate change to twd clk automatically.  And once twd clk rate changes,
smp_twd will get the notification.

-- 
Regards,
Shawn

[PATCH] ARM: smp_twd: reprogram twd based on clk notifier

[Ulf Hansson]

On 7 September 2012 14:40, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Fri, Sep 07, 2012 at 01:59:40PM +0200, Ulf Hansson wrote:
>> Hi Shawn and Mike,
>>
>> In general I am in favor of this patch, but I also see some
>> corresponding problems to resolve. Please correct me if I am wrong.
>>
>> I suppose most machines is relying on that it enough to only take care
>> of the notification by using "cpufreq_notify_transition" from their
>> cpufreq drivers. Thus those have to make sure the updates for smp_twd
>> clock is supported from within the cpufreq driver I suppose.
>>
>> In other words the cpufreq drivers needs to do clk_get of "smp_twd"
>> clock and then do a set_rate on it when it's has changed, to trigger a
>> rate change notification. Is this what you also have in mind or do you
>> have any other idea of how this should work?
>>
> Here is how it works on imx6q.
>
> 1) There are clks arm and twd defined in imx6q clock tree.  And twd
>    is registered to clock framework as one child of arm clk with a fixed
>    divider 2.
>
>         /*                                    name         parent_name     mult div */
>         clk[twd]       = imx_clk_fixed_factor("twd",       "arm",            1, 2);
>
> 2) Register lookup "cpu0" and "smp_twd" for cpufreq and smp_twd drivers
>    respectively.
>
>         clk_register_clkdev(clk[twd], NULL, "smp_twd");
>         clk_register_clkdev(clk[arm], NULL, "cpu0");
>
> That's it.  The clock framework will just handle all the work nicely.
> When cpufreq driver scales arm clk, the framework will propagate the
> rate change to twd clk automatically.  And once twd clk rate changes,
> smp_twd will get the notification.
>

That's a nice solution!
This won't work for ux500 that simple though. Maybe I might be able to
use some kind of clock "parent" thing though, let's see. Thanks for
the information!

Kind regards
Ulf Hansson

[PATCH] ARM: smp_twd: reprogram twd based on clk notifier

[Mike Turquette]

Quoting Ulf Hansson (2012-09-07 06:26:21)
> On 7 September 2012 14:40, Shawn Guo <shawn.guo@linaro.org> wrote:
> > On Fri, Sep 07, 2012 at 01:59:40PM +0200, Ulf Hansson wrote:
> >> Hi Shawn and Mike,
> >>
> >> In general I am in favor of this patch, but I also see some
> >> corresponding problems to resolve. Please correct me if I am wrong.
> >>
> >> I suppose most machines is relying on that it enough to only take care
> >> of the notification by using "cpufreq_notify_transition" from their
> >> cpufreq drivers. Thus those have to make sure the updates for smp_twd
> >> clock is supported from within the cpufreq driver I suppose.
> >>
> >> In other words the cpufreq drivers needs to do clk_get of "smp_twd"
> >> clock and then do a set_rate on it when it's has changed, to trigger a
> >> rate change notification. Is this what you also have in mind or do you
> >> have any other idea of how this should work?
> >>
> > Here is how it works on imx6q.
> >
> > 1) There are clks arm and twd defined in imx6q clock tree.  And twd
> >    is registered to clock framework as one child of arm clk with a fixed
> >    divider 2.
> >
> >         /*                                    name         parent_name     mult div */
> >         clk[twd]       = imx_clk_fixed_factor("twd",       "arm",            1, 2);
> >
> > 2) Register lookup "cpu0" and "smp_twd" for cpufreq and smp_twd drivers
> >    respectively.
> >
> >         clk_register_clkdev(clk[twd], NULL, "smp_twd");
> >         clk_register_clkdev(clk[arm], NULL, "cpu0");
> >
> > That's it.  The clock framework will just handle all the work nicely.
> > When cpufreq driver scales arm clk, the framework will propagate the
> > rate change to twd clk automatically.  And once twd clk rate changes,
> > smp_twd will get the notification.
> >
> 
> That's a nice solution!
> This won't work for ux500 that simple though. Maybe I might be able to
> use some kind of clock "parent" thing though, let's see. Thanks for
> the information!

Hi Ulf,

Why won't this work for ux500?

Thanks,
Mike

> 
> Kind regards
> Ulf Hansson
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] ARM: smp_twd: reprogram twd based on clk notifier

[Ulf Hansson]

On 7 September 2012 23:59, Mike Turquette <mturquette@linaro.org> wrote:
> Quoting Ulf Hansson (2012-09-07 06:26:21)
>> On 7 September 2012 14:40, Shawn Guo <shawn.guo@linaro.org> wrote:
>> > On Fri, Sep 07, 2012 at 01:59:40PM +0200, Ulf Hansson wrote:
>> >> Hi Shawn and Mike,
>> >>
>> >> In general I am in favor of this patch, but I also see some
>> >> corresponding problems to resolve. Please correct me if I am wrong.
>> >>
>> >> I suppose most machines is relying on that it enough to only take care
>> >> of the notification by using "cpufreq_notify_transition" from their
>> >> cpufreq drivers. Thus those have to make sure the updates for smp_twd
>> >> clock is supported from within the cpufreq driver I suppose.
>> >>
>> >> In other words the cpufreq drivers needs to do clk_get of "smp_twd"
>> >> clock and then do a set_rate on it when it's has changed, to trigger a
>> >> rate change notification. Is this what you also have in mind or do you
>> >> have any other idea of how this should work?
>> >>
>> > Here is how it works on imx6q.
>> >
>> > 1) There are clks arm and twd defined in imx6q clock tree.  And twd
>> >    is registered to clock framework as one child of arm clk with a fixed
>> >    divider 2.
>> >
>> >         /*                                    name         parent_name     mult div */
>> >         clk[twd]       = imx_clk_fixed_factor("twd",       "arm",            1, 2);
>> >
>> > 2) Register lookup "cpu0" and "smp_twd" for cpufreq and smp_twd drivers
>> >    respectively.
>> >
>> >         clk_register_clkdev(clk[twd], NULL, "smp_twd");
>> >         clk_register_clkdev(clk[arm], NULL, "cpu0");
>> >
>> > That's it.  The clock framework will just handle all the work nicely.
>> > When cpufreq driver scales arm clk, the framework will propagate the
>> > rate change to twd clk automatically.  And once twd clk rate changes,
>> > smp_twd will get the notification.
>> >
>>
>> That's a nice solution!
>> This won't work for ux500 that simple though. Maybe I might be able to
>> use some kind of clock "parent" thing though, let's see. Thanks for
>> the information!
>
> Hi Ulf,
>
> Why won't this work for ux500?
>

The value for smp_twd clock freq is not directly connected to the CPU
freq clock, I believe. I will dig into the details to see what we can
do to make the most easiest solution.
Using a "parent" solution is definitely to prefer, so let's see.

Kind regards
Ulf Hansson

[PATCH] ARM: smp_twd: reprogram twd based on clk notifier

[Shilimkar, Santosh]

On Mon, Sep 10, 2012 at 4:52 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 7 September 2012 23:59, Mike Turquette <mturquette@linaro.org> wrote:
>> Quoting Ulf Hansson (2012-09-07 06:26:21)
>>> On 7 September 2012 14:40, Shawn Guo <shawn.guo@linaro.org> wrote:
>>> > On Fri, Sep 07, 2012 at 01:59:40PM +0200, Ulf Hansson wrote:
>>> >> Hi Shawn and Mike,
>>> >>
>>> >> In general I am in favor of this patch, but I also see some
>>> >> corresponding problems to resolve. Please correct me if I am wrong.
>>> >>
>>> >> I suppose most machines is relying on that it enough to only take care
>>> >> of the notification by using "cpufreq_notify_transition" from their
>>> >> cpufreq drivers. Thus those have to make sure the updates for smp_twd
>>> >> clock is supported from within the cpufreq driver I suppose.
>>> >>
>>> >> In other words the cpufreq drivers needs to do clk_get of "smp_twd"
>>> >> clock and then do a set_rate on it when it's has changed, to trigger a
>>> >> rate change notification. Is this what you also have in mind or do you
>>> >> have any other idea of how this should work?
>>> >>
>>> > Here is how it works on imx6q.
>>> >
>>> > 1) There are clks arm and twd defined in imx6q clock tree.  And twd
>>> >    is registered to clock framework as one child of arm clk with a fixed
>>> >    divider 2.
>>> >
>>> >         /*                                    name         parent_name     mult div */
>>> >         clk[twd]       = imx_clk_fixed_factor("twd",       "arm",            1, 2);
>>> >
>>> > 2) Register lookup "cpu0" and "smp_twd" for cpufreq and smp_twd drivers
>>> >    respectively.
>>> >
>>> >         clk_register_clkdev(clk[twd], NULL, "smp_twd");
>>> >         clk_register_clkdev(clk[arm], NULL, "cpu0");
>>> >
>>> > That's it.  The clock framework will just handle all the work nicely.
>>> > When cpufreq driver scales arm clk, the framework will propagate the
>>> > rate change to twd clk automatically.  And once twd clk rate changes,
>>> > smp_twd will get the notification.
>>> >
>>>
>>> That's a nice solution!
>>> This won't work for ux500 that simple though. Maybe I might be able to
>>> use some kind of clock "parent" thing though, let's see. Thanks for
>>> the information!
>>
>> Hi Ulf,
>>
>> Why won't this work for ux500?
>>
>
> The value for smp_twd clock freq is not directly connected to the CPU
> freq clock, I believe. I will dig into the details to see what we can
> do to make the most easiest solution.
> Using a "parent" solution is definitely to prefer, so let's see.
>
It is connected to the CPU clock wih a fixed divider in between on
A9 based SoCs. This divider is different across SoC's as seen from
various patches done for the CPUFREQ TWD update.

if you look at OMAP TWD clock node, it's parent is CPU
clock node with fixed divider of 2.

Regards
Santosh

[PATCH] ARM: smp_twd: reprogram twd based on clk notifier

[Shawn Guo]

Any comment?  Otherwise, I will submit it to patch tracker system
tomorrow.

Regards,
Shawn

On Fri, Sep 07, 2012 at 04:19:27PM +0800, Shawn Guo wrote:
> From: Mike Turquette <mturquette@linaro.org>
> 
> Running cpufreq driver on imx6q, the following warning is seen.
> 
> $ BUG: sleeping function called from invalid context at kernel/mutex.c:269
> 
> <snip>
> 
> stack backtrace:
> Backtrace:
> [<80011d64>] (dump_backtrace+0x0/0x10c) from [<803fc164>] (dump_stack+0x18/0x1c)
>  r6:bf8142e0 r5:bf814000 r4:806ac794 r3:bf814000
> [<803fc14c>] (dump_stack+0x0/0x1c) from [<803fd444>] (print_usage_bug+0x250/0x2b
> 8)
> [<803fd1f4>] (print_usage_bug+0x0/0x2b8) from [<80060f90>] (mark_lock+0x56c/0x67
> 0)
> [<80060a24>] (mark_lock+0x0/0x670) from [<80061a20>] (__lock_acquire+0x98c/0x19b
> 4)
> [<80061094>] (__lock_acquire+0x0/0x19b4) from [<80062f14>] (lock_acquire+0x68/0x
> 7c)
> [<80062eac>] (lock_acquire+0x0/0x7c) from [<80400f28>] (mutex_lock_nested+0x78/0
> x344)
>  r7:00000000 r6:bf872000 r5:805cc858 r4:805c2a04
> [<80400eb0>] (mutex_lock_nested+0x0/0x344) from [<803089ac>] (clk_get_rate+0x1c/
> 0x58)
> [<80308990>] (clk_get_rate+0x0/0x58) from [<80013c48>] (twd_update_frequency+0x1
> 8/0x50)
>  r5:bf253d04 r4:805cadf4
> [<80013c30>] (twd_update_frequency+0x0/0x50) from [<80068e20>] (generic_smp_call
> _function_single_interrupt+0xd4/0x13c)
>  r4:bf873ee0 r3:80013c30
> [<80068d4c>] (generic_smp_call_function_single_interrupt+0x0/0x13c) from [<80013
> 34c>] (handle_IPI+0xc0/0x194)
>  r8:00000001 r7:00000000 r6:80574e48 r5:bf872000 r4:80593958
> [<8001328c>] (handle_IPI+0x0/0x194) from [<800084e8>] (gic_handle_irq+0x58/0x60)
>  r8:00000000 r7:bf873f8c r6:bf873f58 r5:80593070 r4:f4000100
> r3:00000005
> [<80008490>] (gic_handle_irq+0x0/0x60) from [<8000e124>] (__irq_svc+0x44/0x60)
> Exception stack(0xbf873f58 to 0xbf873fa0)
> 3f40:                                                       00000001 00000001
> 3f60: 00000000 bf814000 bf872000 805cab48 80405aa4 80597648 00000000 412fc09a
> 3f80: bf872000 bf873fac bf873f70 bf873fa0 80063844 8000f1f8 20000013 ffffffff
>  r6:ffffffff r5:20000013 r4:8000f1f8 r3:bf814000
> [<8000f1b8>] (default_idle+0x0/0x4c) from [<8000f428>] (cpu_idle+0x98/0x114)
> [<8000f390>] (cpu_idle+0x0/0x114) from [<803f9834>] (secondary_start_kernel+0x11
> c/0x140)
> [<803f9718>] (secondary_start_kernel+0x0/0x140) from [<103f9234>] (0x103f9234)
>  r6:10c03c7d r5:0000001f r4:4f86806a r3:803f921c
> 
> It looks that the warning is caused by that twd_update_frequency() gets
> called from an atomic context while it calls clk_get_rate() where
> a mutex gets held.
> 
> To fix the warning, let's convert the existing code to use clk notifiers
> in place of CPUfreq notifiers.  This works out nicely for Cortex-A9
> MPcore designs that scale all CPUs at the same frequency.
> 
> Signed-off-by: Mike Turquette <mturquette@linaro.org>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  arch/arm/kernel/smp_twd.c |   32 +++++++++++++-------------------
>  1 files changed, 13 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
> index fef42b2..635e3ea 100644
> --- a/arch/arm/kernel/smp_twd.c
> +++ b/arch/arm/kernel/smp_twd.c
> @@ -11,7 +11,6 @@
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/clk.h>
> -#include <linux/cpufreq.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
> @@ -96,51 +95,46 @@ static void twd_timer_stop(struct clock_event_device *clk)
>  	disable_percpu_irq(clk->irq);
>  }
>  
> -#ifdef CONFIG_CPU_FREQ
> -
>  /*
>   * Updates clockevent frequency when the cpu frequency changes.
>   * Called on the cpu that is changing frequency with interrupts disabled.
>   */
> -static void twd_update_frequency(void *data)
> +static void twd_update_frequency(void *new_rate)
>  {
> -	twd_timer_rate = clk_get_rate(twd_clk);
> +	twd_timer_rate = *((unsigned long *) new_rate);
>  
>  	clockevents_update_freq(*__this_cpu_ptr(twd_evt), twd_timer_rate);
>  }
>  
> -static int twd_cpufreq_transition(struct notifier_block *nb,
> -	unsigned long state, void *data)
> +static int twd_rate_change(struct notifier_block *nb,
> +	unsigned long flags, void *data)
>  {
> -	struct cpufreq_freqs *freqs = data;
> +	struct clk_notifier_data *cnd = data;
>  
>  	/*
>  	 * The twd clock events must be reprogrammed to account for the new
>  	 * frequency.  The timer is local to a cpu, so cross-call to the
>  	 * changing cpu.
>  	 */
> -	if (state == CPUFREQ_POSTCHANGE || state == CPUFREQ_RESUMECHANGE)
> -		smp_call_function_single(freqs->cpu, twd_update_frequency,
> -			NULL, 1);
> +	if (flags == POST_RATE_CHANGE)
> +		smp_call_function(twd_update_frequency,
> +				  (void *)&cnd->new_rate, 1);
>  
>  	return NOTIFY_OK;
>  }
>  
> -static struct notifier_block twd_cpufreq_nb = {
> -	.notifier_call = twd_cpufreq_transition,
> +static struct notifier_block twd_clk_nb = {
> +	.notifier_call = twd_rate_change,
>  };
>  
> -static int twd_cpufreq_init(void)
> +static int twd_clk_init(void)
>  {
>  	if (twd_evt && *__this_cpu_ptr(twd_evt) && !IS_ERR(twd_clk))
> -		return cpufreq_register_notifier(&twd_cpufreq_nb,
> -			CPUFREQ_TRANSITION_NOTIFIER);
> +		return clk_notifier_register(twd_clk, &twd_clk_nb);
>  
>  	return 0;
>  }
> -core_initcall(twd_cpufreq_init);
> -
> -#endif
> +core_initcall(twd_clk_init);
>  
>  static void __cpuinit twd_calibrate_rate(void)
>  {
> -- 
> 1.7.5.4
> 

[PATCH] ARM: smp_twd: reprogram twd based on clk notifier

[Linus Walleij]

On Fri, Sep 7, 2012 at 10:19 AM, Shawn Guo <shawn.guo@linaro.org> wrote:

> -static struct notifier_block twd_cpufreq_nb = {
> -       .notifier_call = twd_cpufreq_transition,
> +static struct notifier_block twd_clk_nb = {
> +       .notifier_call = twd_rate_change,

But what happens if there is a platform which is now using this through
cpufreq and has not yet switched to the common clock? It regresses
right, because no notifications arrive anymore? Does it even compile?

These in linux-next look like they could be in trouble:

cd arch/arm
grep -r smp_twd .
./arch/arm/mach-imx/clk-imx6q.c:	clk_register_clkdev(clk[twd], NULL, "smp_twd");
./arch/arm/mach-tegra/tegra30_clocks_data.c:	CLK_DUPLICATE("twd",
"smp_twd", NULL),

Tegra seems to be a common clk implementation, but imx6 looks like
it's in trouble right?

Yours,
Linus Walleij

[PATCH] ARM: smp_twd: reprogram twd based on clk notifier

[Linus Walleij]

On Wed, Sep 12, 2012 at 6:05 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Sep 7, 2012 at 10:19 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
>
>> -static struct notifier_block twd_cpufreq_nb = {
>> -       .notifier_call = twd_cpufreq_transition,
>> +static struct notifier_block twd_clk_nb = {
>> +       .notifier_call = twd_rate_change,
>
> But what happens if there is a platform which is now using this through
> cpufreq and has not yet switched to the common clock? It regresses
> right, because no notifications arrive anymore? Does it even compile?

Bah too late in the afternoon :-(

So the real question is - are we sure that imx6 is the last platform
migrated to common clock of all those using the CPUfreq scaling
(this seems to be the last piece) such that
we don't break anything....

Yours,
Linus Walleij

[PATCH] ARM: smp_twd: reprogram twd based on clk notifier

[Mike Turquette]

Quoting Linus Walleij (2012-09-12 09:07:51)
> On Wed, Sep 12, 2012 at 6:05 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Fri, Sep 7, 2012 at 10:19 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> >
> >> -static struct notifier_block twd_cpufreq_nb = {
> >> -       .notifier_call = twd_cpufreq_transition,
> >> +static struct notifier_block twd_clk_nb = {
> >> +       .notifier_call = twd_rate_change,
> >
> > But what happens if there is a platform which is now using this through
> > cpufreq and has not yet switched to the common clock? It regresses
> > right, because no notifications arrive anymore? Does it even compile?
> 
> Bah too late in the afternoon :-(
> 
> So the real question is - are we sure that imx6 is the last platform
> migrated to common clock of all those using the CPUfreq scaling
> (this seems to be the last piece) such that
> we don't break anything....
> 

OMAP will break.  My original version of this patch had "HACK:" in the
$subject since it was just a demo of the notifiers.

I'll submit a new version with some #ifdef CONFIG_COMMON_CLK bits ASAP.

Regards,
Mike

> Yours,
> Linus Walleij
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] ARM: smp_twd: reprogram twd based on clk notifier

[Stephen Warren]

On 09/07/2012 02:19 AM, Shawn Guo wrote:
> From: Mike Turquette <mturquette@linaro.org>
> 
> Running cpufreq driver on imx6q, the following warning is seen.
> 
> $ BUG: sleeping function called from invalid context at kernel/mutex.c:269
...
> It looks that the warning is caused by that twd_update_frequency() gets
> called from an atomic context while it calls clk_get_rate() where
> a mutex gets held.
> 
> To fix the warning, let's convert the existing code to use clk notifiers
> in place of CPUfreq notifiers.  This works out nicely for Cortex-A9
> MPcore designs that scale all CPUs at the same frequency.
> 
> Signed-off-by: Mike Turquette <mturquette@linaro.org>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>

At least in next-20120912, this doesn't appear to cause any issue on
either Tegra20 or Tegra30. I'm copying Prashant to verify this is OK for
Tegra.

For reference, the situation for Tegra is:

Pre-3.7:

Tegra20: Does not use common clock, and there is NO "smp_twd" clock defined.

Tegra30: Does not use common clock, and there IS a "smp_twd" clock defined.

In 3.7 will be:

Tegra20: Supports common clock, but there is NO "smp_twd" clock defined.

Tegra30: Supports common clock, and there IS a "smp_twd" clock defined.

I believe Prashant will be sending a patch very soon to define an
"smp_twd" clock for Tegra20.

The Tegra cpufreq driver currently only initializes on Tegra20 and not
Tegra30 right now due to some clock-naming differences. I don't expect
this to change in 3.7, but who knows, someone might send a patch in the
next 2 days or so...

> ---
>  arch/arm/kernel/smp_twd.c |   32 +++++++++++++-------------------
>  1 files changed, 13 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
> index fef42b2..635e3ea 100644
> --- a/arch/arm/kernel/smp_twd.c
> +++ b/arch/arm/kernel/smp_twd.c
> @@ -11,7 +11,6 @@
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/clk.h>
> -#include <linux/cpufreq.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
> @@ -96,51 +95,46 @@ static void twd_timer_stop(struct clock_event_device *clk)
>  	disable_percpu_irq(clk->irq);
>  }
>  
> -#ifdef CONFIG_CPU_FREQ
> -
>  /*
>   * Updates clockevent frequency when the cpu frequency changes.
>   * Called on the cpu that is changing frequency with interrupts disabled.
>   */
> -static void twd_update_frequency(void *data)
> +static void twd_update_frequency(void *new_rate)
>  {
> -	twd_timer_rate = clk_get_rate(twd_clk);
> +	twd_timer_rate = *((unsigned long *) new_rate);
>  
>  	clockevents_update_freq(*__this_cpu_ptr(twd_evt), twd_timer_rate);
>  }
>  
> -static int twd_cpufreq_transition(struct notifier_block *nb,
> -	unsigned long state, void *data)
> +static int twd_rate_change(struct notifier_block *nb,
> +	unsigned long flags, void *data)
>  {
> -	struct cpufreq_freqs *freqs = data;
> +	struct clk_notifier_data *cnd = data;
>  
>  	/*
>  	 * The twd clock events must be reprogrammed to account for the new
>  	 * frequency.  The timer is local to a cpu, so cross-call to the
>  	 * changing cpu.
>  	 */
> -	if (state == CPUFREQ_POSTCHANGE || state == CPUFREQ_RESUMECHANGE)
> -		smp_call_function_single(freqs->cpu, twd_update_frequency,
> -			NULL, 1);
> +	if (flags == POST_RATE_CHANGE)
> +		smp_call_function(twd_update_frequency,
> +				  (void *)&cnd->new_rate, 1);
>  
>  	return NOTIFY_OK;
>  }
>  
> -static struct notifier_block twd_cpufreq_nb = {
> -	.notifier_call = twd_cpufreq_transition,
> +static struct notifier_block twd_clk_nb = {
> +	.notifier_call = twd_rate_change,
>  };
>  
> -static int twd_cpufreq_init(void)
> +static int twd_clk_init(void)
>  {
>  	if (twd_evt && *__this_cpu_ptr(twd_evt) && !IS_ERR(twd_clk))
> -		return cpufreq_register_notifier(&twd_cpufreq_nb,
> -			CPUFREQ_TRANSITION_NOTIFIER);
> +		return clk_notifier_register(twd_clk, &twd_clk_nb);
>  
>  	return 0;
>  }
> -core_initcall(twd_cpufreq_init);
> -
> -#endif
> +core_initcall(twd_clk_init);
>  
>  static void __cpuinit twd_calibrate_rate(void)
>  {
> 

[PATCH v2] ARM: smp_twd: reprogram twd based on clk notifier

[Mike Turquette]

Running cpufreq driver on imx6q, the following warning is seen.

$ BUG: sleeping function called from invalid context at kernel/mutex.c:269

<snip>

stack backtrace:
Backtrace:
[<80011d64>] (dump_backtrace+0x0/0x10c) from [<803fc164>] (dump_stack+0x18/0x1c)
 r6:bf8142e0 r5:bf814000 r4:806ac794 r3:bf814000
[<803fc14c>] (dump_stack+0x0/0x1c) from [<803fd444>] (print_usage_bug+0x250/0x2b
8)
[<803fd1f4>] (print_usage_bug+0x0/0x2b8) from [<80060f90>] (mark_lock+0x56c/0x67
0)
[<80060a24>] (mark_lock+0x0/0x670) from [<80061a20>] (__lock_acquire+0x98c/0x19b
4)
[<80061094>] (__lock_acquire+0x0/0x19b4) from [<80062f14>] (lock_acquire+0x68/0x
7c)
[<80062eac>] (lock_acquire+0x0/0x7c) from [<80400f28>] (mutex_lock_nested+0x78/0
x344)
 r7:00000000 r6:bf872000 r5:805cc858 r4:805c2a04
[<80400eb0>] (mutex_lock_nested+0x0/0x344) from [<803089ac>] (clk_get_rate+0x1c/
0x58)
[<80308990>] (clk_get_rate+0x0/0x58) from [<80013c48>] (twd_update_frequency+0x1
8/0x50)
 r5:bf253d04 r4:805cadf4
[<80013c30>] (twd_update_frequency+0x0/0x50) from [<80068e20>] (generic_smp_call
_function_single_interrupt+0xd4/0x13c)
 r4:bf873ee0 r3:80013c30
[<80068d4c>] (generic_smp_call_function_single_interrupt+0x0/0x13c) from [<80013
34c>] (handle_IPI+0xc0/0x194)
 r8:00000001 r7:00000000 r6:80574e48 r5:bf872000 r4:80593958
[<8001328c>] (handle_IPI+0x0/0x194) from [<800084e8>] (gic_handle_irq+0x58/0x60)
 r8:00000000 r7:bf873f8c r6:bf873f58 r5:80593070 r4:f4000100
r3:00000005
[<80008490>] (gic_handle_irq+0x0/0x60) from [<8000e124>] (__irq_svc+0x44/0x60)
Exception stack(0xbf873f58 to 0xbf873fa0)
3f40:                                                       00000001 00000001
3f60: 00000000 bf814000 bf872000 805cab48 80405aa4 80597648 00000000 412fc09a
3f80: bf872000 bf873fac bf873f70 bf873fa0 80063844 8000f1f8 20000013 ffffffff
 r6:ffffffff r5:20000013 r4:8000f1f8 r3:bf814000
[<8000f1b8>] (default_idle+0x0/0x4c) from [<8000f428>] (cpu_idle+0x98/0x114)
[<8000f390>] (cpu_idle+0x0/0x114) from [<803f9834>] (secondary_start_kernel+0x11
c/0x140)
[<803f9718>] (secondary_start_kernel+0x0/0x140) from [<103f9234>] (0x103f9234)
 r6:10c03c7d r5:0000001f r4:4f86806a r3:803f921c

It looks that the warning is caused by that twd_update_frequency() gets
called from an atomic context while it calls clk_get_rate() where a
mutex gets held.

To fix the warning, let's convert common clk users over to clk notifiers
in place of CPUfreq notifiers.  This works out nicely for Cortex-A9
MPcore designs that scale all CPUs at the same frequency.

Platforms that have not been converted to the common clk framework and
support CPUfreq will rely on the old mechanism.  Once these platforms
are converted over fully then we can remove the CPUfreq-specific bits
for good.

Signed-off-by: Mike Turquette <mturquette@linaro.org>
Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
Changes in v2:
 * Kept the original CPUfreq notifier scheme in place for platforms that
   have not adapted to the common clk framework yet

Testing notes:
 * Tested on OMAP4430 Panda against v3.6-rc5 with legacy clk framework
   and with local common clk patches, thus both code paths are validated
 * CPUfreq userspace governor @ 300MHz, 600MHz and 1008MHz
 * Ran "date; sleep 30; date" and compared with a stopwatch to verify
   correct timekeeping

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

diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index fef42b2..e1f9069 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -11,7 +11,6 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/clk.h>
-#include <linux/cpufreq.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/err.h>
@@ -96,7 +95,52 @@ static void twd_timer_stop(struct clock_event_device *clk)
 	disable_percpu_irq(clk->irq);
 }
 
-#ifdef CONFIG_CPU_FREQ
+#ifdef CONFIG_COMMON_CLK
+
+/*
+ * Updates clockevent frequency when the cpu frequency changes.
+ * Called on the cpu that is changing frequency with interrupts disabled.
+ */
+static void twd_update_frequency(void *new_rate)
+{
+	twd_timer_rate = *((unsigned long *) new_rate);
+
+	clockevents_update_freq(*__this_cpu_ptr(twd_evt), twd_timer_rate);
+}
+
+static int twd_rate_change(struct notifier_block *nb,
+	unsigned long flags, void *data)
+{
+	struct clk_notifier_data *cnd = data;
+
+	/*
+	 * The twd clock events must be reprogrammed to account for the new
+	 * frequency.  The timer is local to a cpu, so cross-call to the
+	 * changing cpu.
+	 */
+	if (flags == POST_RATE_CHANGE)
+		smp_call_function(twd_update_frequency,
+				  (void *)&cnd->new_rate, 1);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block twd_clk_nb = {
+	.notifier_call = twd_rate_change,
+};
+
+static int twd_clk_init(void)
+{
+	if (twd_evt && *__this_cpu_ptr(twd_evt) && !IS_ERR(twd_clk))
+		return clk_notifier_register(twd_clk, &twd_clk_nb);
+
+	return 0;
+}
+core_initcall(twd_clk_init);
+
+#elif defined (CONFIG_CPU_FREQ)
+
+#include <linux/cpufreq.h>
 
 /*
  * Updates clockevent frequency when the cpu frequency changes.
-- 
1.7.9.5

[PATCH v2] ARM: smp_twd: reprogram twd based on clk notifier

[Linus Walleij]

On Thu, Sep 13, 2012 at 12:44 AM, Mike Turquette <mturquette@linaro.org> wrote:

> Signed-off-by: Mike Turquette <mturquette@linaro.org>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
> Changes in v2:
>  * Kept the original CPUfreq notifier scheme in place for platforms that
>    have not adapted to the common clk framework yet

This looks good. We can always delete the cpufreq part
(which is a kinda hack anyway) once all OMAPs have
notifiers.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v2] ARM: smp_twd: reprogram twd based on clk notifier

[Ulf Hansson]

On 13 September 2012 00:44, Mike Turquette <mturquette@linaro.org> wrote:
> Running cpufreq driver on imx6q, the following warning is seen.
>
> $ BUG: sleeping function called from invalid context at kernel/mutex.c:269
>
> <snip>
>
> stack backtrace:
> Backtrace:
> [<80011d64>] (dump_backtrace+0x0/0x10c) from [<803fc164>] (dump_stack+0x18/0x1c)
>  r6:bf8142e0 r5:bf814000 r4:806ac794 r3:bf814000
> [<803fc14c>] (dump_stack+0x0/0x1c) from [<803fd444>] (print_usage_bug+0x250/0x2b
> 8)
> [<803fd1f4>] (print_usage_bug+0x0/0x2b8) from [<80060f90>] (mark_lock+0x56c/0x67
> 0)
> [<80060a24>] (mark_lock+0x0/0x670) from [<80061a20>] (__lock_acquire+0x98c/0x19b
> 4)
> [<80061094>] (__lock_acquire+0x0/0x19b4) from [<80062f14>] (lock_acquire+0x68/0x
> 7c)
> [<80062eac>] (lock_acquire+0x0/0x7c) from [<80400f28>] (mutex_lock_nested+0x78/0
> x344)
>  r7:00000000 r6:bf872000 r5:805cc858 r4:805c2a04
> [<80400eb0>] (mutex_lock_nested+0x0/0x344) from [<803089ac>] (clk_get_rate+0x1c/
> 0x58)
> [<80308990>] (clk_get_rate+0x0/0x58) from [<80013c48>] (twd_update_frequency+0x1
> 8/0x50)
>  r5:bf253d04 r4:805cadf4
> [<80013c30>] (twd_update_frequency+0x0/0x50) from [<80068e20>] (generic_smp_call
> _function_single_interrupt+0xd4/0x13c)
>  r4:bf873ee0 r3:80013c30
> [<80068d4c>] (generic_smp_call_function_single_interrupt+0x0/0x13c) from [<80013
> 34c>] (handle_IPI+0xc0/0x194)
>  r8:00000001 r7:00000000 r6:80574e48 r5:bf872000 r4:80593958
> [<8001328c>] (handle_IPI+0x0/0x194) from [<800084e8>] (gic_handle_irq+0x58/0x60)
>  r8:00000000 r7:bf873f8c r6:bf873f58 r5:80593070 r4:f4000100
> r3:00000005
> [<80008490>] (gic_handle_irq+0x0/0x60) from [<8000e124>] (__irq_svc+0x44/0x60)
> Exception stack(0xbf873f58 to 0xbf873fa0)
> 3f40:                                                       00000001 00000001
> 3f60: 00000000 bf814000 bf872000 805cab48 80405aa4 80597648 00000000 412fc09a
> 3f80: bf872000 bf873fac bf873f70 bf873fa0 80063844 8000f1f8 20000013 ffffffff
>  r6:ffffffff r5:20000013 r4:8000f1f8 r3:bf814000
> [<8000f1b8>] (default_idle+0x0/0x4c) from [<8000f428>] (cpu_idle+0x98/0x114)
> [<8000f390>] (cpu_idle+0x0/0x114) from [<803f9834>] (secondary_start_kernel+0x11
> c/0x140)
> [<803f9718>] (secondary_start_kernel+0x0/0x140) from [<103f9234>] (0x103f9234)
>  r6:10c03c7d r5:0000001f r4:4f86806a r3:803f921c
>
> It looks that the warning is caused by that twd_update_frequency() gets
> called from an atomic context while it calls clk_get_rate() where a
> mutex gets held.
>
> To fix the warning, let's convert common clk users over to clk notifiers
> in place of CPUfreq notifiers.  This works out nicely for Cortex-A9
> MPcore designs that scale all CPUs at the same frequency.
>
> Platforms that have not been converted to the common clk framework and
> support CPUfreq will rely on the old mechanism.  Once these platforms
> are converted over fully then we can remove the CPUfreq-specific bits
> for good.
>
> Signed-off-by: Mike Turquette <mturquette@linaro.org>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
> Changes in v2:
>  * Kept the original CPUfreq notifier scheme in place for platforms that
>    have not adapted to the common clk framework yet
>
> Testing notes:
>  * Tested on OMAP4430 Panda against v3.6-rc5 with legacy clk framework
>    and with local common clk patches, thus both code paths are validated
>  * CPUfreq userspace governor @ 300MHz, 600MHz and 1008MHz
>  * Ran "date; sleep 30; date" and compared with a stopwatch to verify
>    correct timekeeping
>
>  arch/arm/kernel/smp_twd.c |   48 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 46 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
> index fef42b2..e1f9069 100644
> --- a/arch/arm/kernel/smp_twd.c
> +++ b/arch/arm/kernel/smp_twd.c
> @@ -11,7 +11,6 @@
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/clk.h>
> -#include <linux/cpufreq.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
> @@ -96,7 +95,52 @@ static void twd_timer_stop(struct clock_event_device *clk)
>         disable_percpu_irq(clk->irq);
>  }
>
> -#ifdef CONFIG_CPU_FREQ
> +#ifdef CONFIG_COMMON_CLK
> +
> +/*
> + * Updates clockevent frequency when the cpu frequency changes.
> + * Called on the cpu that is changing frequency with interrupts disabled.
> + */
> +static void twd_update_frequency(void *new_rate)
> +{
> +       twd_timer_rate = *((unsigned long *) new_rate);
> +
> +       clockevents_update_freq(*__this_cpu_ptr(twd_evt), twd_timer_rate);
> +}
> +
> +static int twd_rate_change(struct notifier_block *nb,
> +       unsigned long flags, void *data)
> +{
> +       struct clk_notifier_data *cnd = data;
> +
> +       /*
> +        * The twd clock events must be reprogrammed to account for the new
> +        * frequency.  The timer is local to a cpu, so cross-call to the
> +        * changing cpu.
> +        */
> +       if (flags == POST_RATE_CHANGE)
> +               smp_call_function(twd_update_frequency,
> +                                 (void *)&cnd->new_rate, 1);
> +
> +       return NOTIFY_OK;
> +}
> +
> +static struct notifier_block twd_clk_nb = {
> +       .notifier_call = twd_rate_change,
> +};
> +
> +static int twd_clk_init(void)
> +{
> +       if (twd_evt && *__this_cpu_ptr(twd_evt) && !IS_ERR(twd_clk))
> +               return clk_notifier_register(twd_clk, &twd_clk_nb);
> +
> +       return 0;
> +}
> +core_initcall(twd_clk_init);
> +
> +#elif defined (CONFIG_CPU_FREQ)
> +
> +#include <linux/cpufreq.h>
>
>  /*
>   * Updates clockevent frequency when the cpu frequency changes.
> --
> 1.7.9.5
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

I think this makes good sense! I will adapt ux500 smp_twd clock
handling accordingly.
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Ulf Hansson

[PATCH v2] ARM: smp_twd: reprogram twd based on clk notifier

[Mike Turquette]

Quoting Linus Walleij (2012-09-13 05:51:52)
> On Thu, Sep 13, 2012 at 12:44 AM, Mike Turquette <mturquette@linaro.org> wrote:
> 
> > Signed-off-by: Mike Turquette <mturquette@linaro.org>
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> > Changes in v2:
> >  * Kept the original CPUfreq notifier scheme in place for platforms that
> >    have not adapted to the common clk framework yet
> 
> This looks good. We can always delete the cpufreq part
> (which is a kinda hack anyway) once all OMAPs have
> notifiers.
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 

Thanks Linus.  I've added v2 of this patch to Russell's patch tracker
with your and Ulf's Reviewed-by's.

Regards,
Mike

> Yours,
> Linus Walleij