[PATCH v2 0/9] arm: zynq: Add support for cpufreq

[PATCH v2 0/9] arm: zynq: Add support for cpufreq

[Soren Brinkmann]

I revised this series and rebased on top of Linus' tree. Patch specific
changes are tracked in the respective email with their patch. On the
level of the whole series, this changed:

v2:
 - added 'clocksource/cadence_ttc: Use enable/disable_irq'. I hope that
   is more appropriate to avoid races of the timer with its interrupt
   handler.
 - added 'arm: zynq: Don't use arm_global_timer with cpufreq'. In the
   3.13 merge window we enabled the arm_global_timer for Zynq. That
   timer is clocked by the CPU clock and hence does not provide a stable
   clocksource when cpufreq is enabled. That patch makes the selection of
   the GT driver depend on !CPU_FREQ for Zynq.

	Sören

Soren Brinkmann (9):
  arm: dt: zynq: Remove 'clock-ranges' from TTC nodes
  arm: dt: zynq: Add 'cpus' node
  clocksource/cadence_ttc: Store timer frequency in driver data
  clocksource/cadence_ttc: Use enable/disable_irq
  clocksource/cadence_ttc: Adjust interval in clock notifier
  clocksource/cadence_ttc: Overhaul clocksource frequency adjustment
  clocksource/cadence_ttc: Use only one counter
  arm: zynq: Don't use arm_global_timer with cpufreq
  arm: zynq: Add support for cpufreq

 arch/arm/boot/dts/zynq-7000.dtsi        |  27 +++-
 arch/arm/mach-zynq/Kconfig              |   4 +-
 arch/arm/mach-zynq/common.c             |   3 +
 drivers/clocksource/cadence_ttc_timer.c | 235 +++++++++++++++++++-------------
 4 files changed, 174 insertions(+), 95 deletions(-)

-- 
1.8.4.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/9] arm: dt: zynq: Remove 'clock-ranges' from TTC nodes

[Soren Brinkmann]

The bindings for the TTC changed in commit 'arm: zynq: Use standard
timer binding' (e932900a3279b5dbb6d8f43c7b369003620e137c). That change
removed possible subnodes from this driver rendering the 'clock-ranges'
property invalid for this node.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
Acked-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/arm/boot/dts/zynq-7000.dtsi | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
index e7f73b2e4550..605a2d0a39fc 100644
--- a/arch/arm/boot/dts/zynq-7000.dtsi
+++ b/arch/arm/boot/dts/zynq-7000.dtsi
@@ -106,7 +106,6 @@
 			compatible = "cdns,ttc";
 			clocks = <&clkc 6>;
 			reg = <0xF8001000 0x1000>;
-			clock-ranges;
 		};
 
 		ttc1: ttc1@f8002000 {
@@ -115,7 +114,6 @@
 			compatible = "cdns,ttc";
 			clocks = <&clkc 6>;
 			reg = <0xF8002000 0x1000>;
-			clock-ranges;
 		};
 		scutimer: scutimer@f8f00600 {
 			interrupt-parent = <&intc>;
-- 
1.8.4.4

Re: [PATCH v2 1/9] arm: dt: zynq: Remove 'clock-ranges' from TTC nodes

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 1052 bytes --]

On 11/27/2013 02:04 AM, Soren Brinkmann wrote:
> The bindings for the TTC changed in commit 'arm: zynq: Use standard
> timer binding' (e932900a3279b5dbb6d8f43c7b369003620e137c). That change
> removed possible subnodes from this driver rendering the 'clock-ranges'
> property invalid for this node.
> 
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> Acked-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  arch/arm/boot/dts/zynq-7000.dtsi | 2 --
>  1 file changed, 2 deletions(-)

I think this is completely unrelated to cpufreq and should just go
directly via arm-soc tree.
If yes, I will add 1/9 and 2/9 to zynq/dt branch.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v2 1/9] arm: dt: zynq: Remove 'clock-ranges' from TTC nodes

[Sören Brinkmann]

Hi Michal,

On Thu, Dec 12, 2013 at 09:53:13AM +0100, Michal Simek wrote:
> On 11/27/2013 02:04 AM, Soren Brinkmann wrote:
> > The bindings for the TTC changed in commit 'arm: zynq: Use standard
> > timer binding' (e932900a3279b5dbb6d8f43c7b369003620e137c). That change
> > removed possible subnodes from this driver rendering the 'clock-ranges'
> > property invalid for this node.
> > 
> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> > Acked-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> > Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > ---
> >  arch/arm/boot/dts/zynq-7000.dtsi | 2 --
> >  1 file changed, 2 deletions(-)
> 
> I think this is completely unrelated to cpufreq and should just go
> directly via arm-soc tree.
> If yes, I will add 1/9 and 2/9 to zynq/dt branch.

I agree. This is just clean up that I found during this work. The second
patch could go in as well I think. cpufreq will need the CPU nodes
later, but I think there is no real reason to not add them now.

	Sören

Re: [PATCH v2 1/9] arm: dt: zynq: Remove 'clock-ranges' from TTC nodes

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 1489 bytes --]

On 12/12/2013 06:01 PM, Sören Brinkmann wrote:
> Hi Michal,
> 
> On Thu, Dec 12, 2013 at 09:53:13AM +0100, Michal Simek wrote:
>> On 11/27/2013 02:04 AM, Soren Brinkmann wrote:
>>> The bindings for the TTC changed in commit 'arm: zynq: Use standard
>>> timer binding' (e932900a3279b5dbb6d8f43c7b369003620e137c). That change
>>> removed possible subnodes from this driver rendering the 'clock-ranges'
>>> property invalid for this node.
>>>
>>> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
>>> Acked-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> ---
>>>  arch/arm/boot/dts/zynq-7000.dtsi | 2 --
>>>  1 file changed, 2 deletions(-)
>>
>> I think this is completely unrelated to cpufreq and should just go
>> directly via arm-soc tree.
>> If yes, I will add 1/9 and 2/9 to zynq/dt branch.
> 
> I agree. This is just clean up that I found during this work. The second
> patch could go in as well I think. cpufreq will need the CPU nodes
> later, but I think there is no real reason to not add them now.

ok then. 1/9 and 2/9 applied to zynq/dt branch.

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

[PATCH v2 2/9] arm: dt: zynq: Add 'cpus' node

[Soren Brinkmann]

Add a 'cpus' node to describe the CPU cores of Zynq.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
Acked-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
v2:
 - remove cache size related properties
---
 arch/arm/boot/dts/zynq-7000.dtsi | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
index 605a2d0a39fc..23ba82c205a2 100644
--- a/arch/arm/boot/dts/zynq-7000.dtsi
+++ b/arch/arm/boot/dts/zynq-7000.dtsi
@@ -15,6 +15,25 @@
 / {
 	compatible = "xlnx,zynq-7000";
 
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		cpu@0 {
+			compatible = "arm,cortex-a9";
+			device_type = "cpu";
+			reg = <0>;
+			clocks = <&clkc 3>;
+		};
+
+		cpu@1 {
+			compatible = "arm,cortex-a9";
+			device_type = "cpu";
+			reg = <1>;
+			clocks = <&clkc 3>;
+		};
+	};
+
 	pmu {
 		compatible = "arm,cortex-a9-pmu";
 		interrupts = <0 5 4>, <0 6 4>;
-- 
1.8.4.4

[PATCH v2 3/9] clocksource/cadence_ttc: Store timer frequency in driver data

[Soren Brinkmann]

It is not allowed to call clk_get_rate() from interrupt context. To
avoid such calls the timer input frequency is stored in the driver's
data struct which makes it accessible to the driver in any context.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/clocksource/cadence_ttc_timer.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c
index b2bb3a4bc205..a92350b55d32 100644
--- a/drivers/clocksource/cadence_ttc_timer.c
+++ b/drivers/clocksource/cadence_ttc_timer.c
@@ -67,11 +67,13 @@
  * struct ttc_timer - This definition defines local timer structure
  *
  * @base_addr:	Base address of timer
+ * @freq:	Timer input clock frequency
  * @clk:	Associated clock source
  * @clk_rate_change_nb	Notifier block for clock rate changes
  */
 struct ttc_timer {
 	void __iomem *base_addr;
+	unsigned long freq;
 	struct clk *clk;
 	struct notifier_block clk_rate_change_nb;
 };
@@ -196,9 +198,8 @@ static void ttc_set_mode(enum clock_event_mode mode,
 
 	switch (mode) {
 	case CLOCK_EVT_MODE_PERIODIC:
-		ttc_set_interval(timer,
-				DIV_ROUND_CLOSEST(clk_get_rate(ttce->ttc.clk),
-					PRESCALE * HZ));
+		ttc_set_interval(timer, DIV_ROUND_CLOSEST(ttce->ttc.freq,
+						PRESCALE * HZ));
 		break;
 	case CLOCK_EVT_MODE_ONESHOT:
 	case CLOCK_EVT_MODE_UNUSED:
@@ -273,6 +274,8 @@ static void __init ttc_setup_clocksource(struct clk *clk, void __iomem *base)
 		return;
 	}
 
+	ttccs->ttc.freq = clk_get_rate(ttccs->ttc.clk);
+
 	ttccs->ttc.clk_rate_change_nb.notifier_call =
 		ttc_rate_change_clocksource_cb;
 	ttccs->ttc.clk_rate_change_nb.next = NULL;
@@ -298,16 +301,14 @@ static void __init ttc_setup_clocksource(struct clk *clk, void __iomem *base)
 	__raw_writel(CNT_CNTRL_RESET,
 		     ttccs->ttc.base_addr + TTC_CNT_CNTRL_OFFSET);
 
-	err = clocksource_register_hz(&ttccs->cs,
-			clk_get_rate(ttccs->ttc.clk) / PRESCALE);
+	err = clocksource_register_hz(&ttccs->cs, ttccs->ttc.freq / PRESCALE);
 	if (WARN_ON(err)) {
 		kfree(ttccs);
 		return;
 	}
 
 	ttc_sched_clock_val_reg = base + TTC_COUNT_VAL_OFFSET;
-	setup_sched_clock(ttc_sched_clock_read, 16,
-			clk_get_rate(ttccs->ttc.clk) / PRESCALE);
+	setup_sched_clock(ttc_sched_clock_read, 16, ttccs->ttc.freq / PRESCALE);
 }
 
 static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
@@ -334,6 +335,9 @@ static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
 				ndata->new_rate / PRESCALE);
 		local_irq_restore(flags);
 
+		/* update cached frequency */
+		ttc->freq = ndata->new_rate;
+
 		/* fall through */
 	}
 	case PRE_RATE_CHANGE:
@@ -367,6 +371,7 @@ static void __init ttc_setup_clockevent(struct clk *clk,
 	if (clk_notifier_register(ttcce->ttc.clk,
 				&ttcce->ttc.clk_rate_change_nb))
 		pr_warn("Unable to register clock notifier.\n");
+	ttcce->ttc.freq = clk_get_rate(ttcce->ttc.clk);
 
 	ttcce->ttc.base_addr = base;
 	ttcce->ce.name = "ttc_clockevent";
@@ -396,7 +401,7 @@ static void __init ttc_setup_clockevent(struct clk *clk,
 	}
 
 	clockevents_config_and_register(&ttcce->ce,
-			clk_get_rate(ttcce->ttc.clk) / PRESCALE, 1, 0xfffe);
+			ttcce->ttc.freq / PRESCALE, 1, 0xfffe);
 }
 
 /**
-- 
1.8.4.4

Re: [PATCH v2 3/9] clocksource/cadence_ttc: Store timer frequency in driver data

[Sören Brinkmann]

Hi Daniel,

On Tue, Nov 26, 2013 at 05:04:50PM -0800, Soren Brinkmann wrote:
> It is not allowed to call clk_get_rate() from interrupt context. To
> avoid such calls the timer input frequency is stored in the driver's
> data struct which makes it accessible to the driver in any context.
> 
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

I doubt that we'll resolve all issues with this series before the
holidays or even the next merge window. Could you take this patch into
your tree for 3.14? It is not directly related to the cpufreq work and
fixes an actual issue that triggers a kernel WARN under some condition
(I missed preserving the details and the trace). That would take the
easy stuff out of the way and we can focus on the more controversial
changes.

	Thanks,
	Sören

Re: [PATCH v2 3/9] clocksource/cadence_ttc: Store timer frequency in driver data

[Daniel Lezcano]

On 12/17/2013 08:21 PM, Sören Brinkmann wrote:
> Hi Daniel,
>
> On Tue, Nov 26, 2013 at 05:04:50PM -0800, Soren Brinkmann wrote:
>> It is not allowed to call clk_get_rate() from interrupt context. To
>> avoid such calls the timer input frequency is stored in the driver's
>> data struct which makes it accessible to the driver in any context.
>>
>> Signed-off-by: Soren Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
>> Acked-by: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
> I doubt that we'll resolve all issues with this series before the
> holidays or even the next merge window. Could you take this patch into
> your tree for 3.14? It is not directly related to the cpufreq work and
> fixes an actual issue that triggers a kernel WARN under some condition
> (I missed preserving the details and the trace). That would take the
> easy stuff out of the way and we can focus on the more controversial
> changes.

You are asking to take it for 3.14 but shouldn't it go as a 3.13 fix ?

   -- Daniel

-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/9] clocksource/cadence_ttc: Store timer frequency in driver data

[Sören Brinkmann]

Hi Daniel,

On Wed, Dec 18, 2013 at 03:53:51PM +0100, Daniel Lezcano wrote:
> On 12/17/2013 08:21 PM, Sören Brinkmann wrote:
> >Hi Daniel,
> >
> >On Tue, Nov 26, 2013 at 05:04:50PM -0800, Soren Brinkmann wrote:
> >>It is not allowed to call clk_get_rate() from interrupt context. To
> >>avoid such calls the timer input frequency is stored in the driver's
> >>data struct which makes it accessible to the driver in any context.
> >>
> >>Signed-off-by: Soren Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> >>Acked-by: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >
> >I doubt that we'll resolve all issues with this series before the
> >holidays or even the next merge window. Could you take this patch into
> >your tree for 3.14? It is not directly related to the cpufreq work and
> >fixes an actual issue that triggers a kernel WARN under some condition
> >(I missed preserving the details and the trace). That would take the
> >easy stuff out of the way and we can focus on the more controversial
> >changes.
> 
> You are asking to take it for 3.14 but shouldn't it go as a 3.13 fix ?

That's also an option. As I remember, the patch fixes a kernel WARN. The
system still seemed operational though. Up to you whether this is
considered severe enough for the 3.13 series. I'm happy either way.

	Sören


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/9] clocksource/cadence_ttc: Store timer frequency in driver data

[Daniel Lezcano]

On 12/18/2013 05:47 PM, Sören Brinkmann wrote:
> Hi Daniel,
>
> On Wed, Dec 18, 2013 at 03:53:51PM +0100, Daniel Lezcano wrote:
>> On 12/17/2013 08:21 PM, Sören Brinkmann wrote:
>>> Hi Daniel,
>>>
>>> On Tue, Nov 26, 2013 at 05:04:50PM -0800, Soren Brinkmann wrote:
>>>> It is not allowed to call clk_get_rate() from interrupt context. To
>>>> avoid such calls the timer input frequency is stored in the driver's
>>>> data struct which makes it accessible to the driver in any context.
>>>>
>>>> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
>>>> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>
>>> I doubt that we'll resolve all issues with this series before the
>>> holidays or even the next merge window. Could you take this patch into
>>> your tree for 3.14? It is not directly related to the cpufreq work and
>>> fixes an actual issue that triggers a kernel WARN under some condition
>>> (I missed preserving the details and the trace). That would take the
>>> easy stuff out of the way and we can focus on the more controversial
>>> changes.
>>
>> You are asking to take it for 3.14 but shouldn't it go as a 3.13 fix ?
>
> That's also an option. As I remember, the patch fixes a kernel WARN. The
> system still seemed operational though. Up to you whether this is
> considered severe enough for the 3.13 series. I'm happy either way.

I was not able to reproduce the WARN with my board.

Please, could you give the WARN or give the procedure to reproduce it ?

Thanks
   -- Daniel


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v2 3/9] clocksource/cadence_ttc: Store timer frequency in driver data

[Sören Brinkmann]

Hi Daniel,

On Wed, Dec 18, 2013 at 10:58:26PM +0100, Daniel Lezcano wrote:
> On 12/18/2013 05:47 PM, Sören Brinkmann wrote:
> >Hi Daniel,
> >
> >On Wed, Dec 18, 2013 at 03:53:51PM +0100, Daniel Lezcano wrote:
> >>On 12/17/2013 08:21 PM, Sören Brinkmann wrote:
> >>>Hi Daniel,
> >>>
> >>>On Tue, Nov 26, 2013 at 05:04:50PM -0800, Soren Brinkmann wrote:
> >>>>It is not allowed to call clk_get_rate() from interrupt context. To
> >>>>avoid such calls the timer input frequency is stored in the driver's
> >>>>data struct which makes it accessible to the driver in any context.
> >>>>
> >>>>Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> >>>>Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >>>
> >>>I doubt that we'll resolve all issues with this series before the
> >>>holidays or even the next merge window. Could you take this patch into
> >>>your tree for 3.14? It is not directly related to the cpufreq work and
> >>>fixes an actual issue that triggers a kernel WARN under some condition
> >>>(I missed preserving the details and the trace). That would take the
> >>>easy stuff out of the way and we can focus on the more controversial
> >>>changes.
> >>
> >>You are asking to take it for 3.14 but shouldn't it go as a 3.13 fix ?
> >
> >That's also an option. As I remember, the patch fixes a kernel WARN. The
> >system still seemed operational though. Up to you whether this is
> >considered severe enough for the 3.13 series. I'm happy either way.
> 
> I was not able to reproduce the WARN with my board.
> 
> Please, could you give the WARN or give the procedure to reproduce it ?

I can't either... I thought I saw the WARN on a vanilla kernel during
boot (IIRC, when cpuidle started). Is there any chance the timer core
calls the timer's set_mode() from interrupt context?
Anyway, let's drop it for now. I'll make sure to record more information
in case it reappears.

	Sören

Re: [PATCH v2 3/9] clocksource/cadence_ttc: Store timer frequency in driver data

[Daniel Lezcano]

On 12/19/2013 07:32 PM, Sören Brinkmann wrote:
> Hi Daniel,
>
> On Wed, Dec 18, 2013 at 10:58:26PM +0100, Daniel Lezcano wrote:
>> On 12/18/2013 05:47 PM, Sören Brinkmann wrote:
>>> Hi Daniel,
>>>
>>> On Wed, Dec 18, 2013 at 03:53:51PM +0100, Daniel Lezcano wrote:
>>>> On 12/17/2013 08:21 PM, Sören Brinkmann wrote:
>>>>> Hi Daniel,
>>>>>
>>>>> On Tue, Nov 26, 2013 at 05:04:50PM -0800, Soren Brinkmann wrote:
>>>>>> It is not allowed to call clk_get_rate() from interrupt context. To
>>>>>> avoid such calls the timer input frequency is stored in the driver's
>>>>>> data struct which makes it accessible to the driver in any context.
>>>>>>
>>>>>> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
>>>>>> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>>>
>>>>> I doubt that we'll resolve all issues with this series before the
>>>>> holidays or even the next merge window. Could you take this patch into
>>>>> your tree for 3.14? It is not directly related to the cpufreq work and
>>>>> fixes an actual issue that triggers a kernel WARN under some condition
>>>>> (I missed preserving the details and the trace). That would take the
>>>>> easy stuff out of the way and we can focus on the more controversial
>>>>> changes.
>>>>
>>>> You are asking to take it for 3.14 but shouldn't it go as a 3.13 fix ?
>>>
>>> That's also an option. As I remember, the patch fixes a kernel WARN. The
>>> system still seemed operational though. Up to you whether this is
>>> considered severe enough for the 3.13 series. I'm happy either way.
>>
>> I was not able to reproduce the WARN with my board.
>>
>> Please, could you give the WARN or give the procedure to reproduce it ?
>
> I can't either... I thought I saw the WARN on a vanilla kernel during
> boot (IIRC, when cpuidle started). Is there any chance the timer core
> calls the timer's set_mode() from interrupt context?
> Anyway, let's drop it for now. I'll make sure to record more information
> in case it reappears.

Finally I was able to reproduce it with the highres timers disabled, the 
periodic tick system and the locks debug.

Indeed, we are in an interrupt context (IPI) and we are calling 
clk_get_rate in the the set_mode function which in turn ends up by 
getting a mutex... Even if that does not hang, it is a potential kernel 
crash so I will apply the patch with an updated changelog.

------------[ cut here ]------------
WARNING: CPU: 1 PID: 0 at 
/home/dlezcano/Work/src/cpuidle-next/kernel/mutex.c:856 
mutex_trylock+0x70/0x1fc()
DEBUG_LOCKS_WARN_ON(in_interrupt())
Modules linked in:
CPU: 1 PID: 0 Comm: swapper/1 Not tainted 3.12.0-xilinx-dirty #93
[<c0014a78>] (unwind_backtrace+0x0/0x11c) from [<c0011b6c>] 
(show_stack+0x10/0x14)
[<c0011b6c>] (show_stack+0x10/0x14) from [<c039120c>] (dump_stack+0x7c/0xc0)
[<c039120c>] (dump_stack+0x7c/0xc0) from [<c001fda4>] 
(warn_slowpath_common+0x60/0x84)
[<c001fda4>] (warn_slowpath_common+0x60/0x84) from [<c001fe48>] 
(warn_slowpath_fmt+0x2c/0x3c)
[<c001fe48>] (warn_slowpath_fmt+0x2c/0x3c) from [<c0392658>] 
(mutex_trylock+0x70/0x1fc)
[<c0392658>] (mutex_trylock+0x70/0x1fc) from [<c02dfc08>] 
(clk_prepare_lock+0xc/0xe4)
[<c02dfc08>] (clk_prepare_lock+0xc/0xe4) from [<c02e099c>] 
(clk_get_rate+0xc/0x44)
[<c02e099c>] (clk_get_rate+0xc/0x44) from [<c02d0394>] 
(ttc_set_mode+0x34/0x78)
[<c02d0394>] (ttc_set_mode+0x34/0x78) from [<c005f794>] 
(clockevents_set_mode+0x28/0x5c)
[<c005f794>] (clockevents_set_mode+0x28/0x5c) from [<c00607fc>] 
(tick_broadcast_on_off+0x190/0x1c0)
[<c00607fc>] (tick_broadcast_on_off+0x190/0x1c0) from [<c005f168>] 
(clockevents_notify+0x58/0x1ac)
[<c005f168>] (clockevents_notify+0x58/0x1ac) from [<c02b99dc>] 
(cpuidle_setup_broadcast_timer+0x20/0x24)
[<c02b99dc>] (cpuidle_setup_broadcast_timer+0x20/0x24) from [<c006cd04>] 
(generic_smp_call_function_single_interrupt+0)
[<c006cd04>] (generic_smp_call_function_single_interrupt+0xe0/0x130) 
from [<c00138c8>] (handle_IPI+0x88/0x118)
[<c00138c8>] (handle_IPI+0x88/0x118) from [<c0008504>] 
(gic_handle_irq+0x58/0x60)
[<c0008504>] (gic_handle_irq+0x58/0x60) from [<c0012644>] 
(__irq_svc+0x44/0x78)
Exception stack(0xef099fa0 to 0xef099fe8)
9fa0: 00000001 ef092100 00000000 ef092100 ef098000 00000015 c0399f2c 
c0579d74
9fc0: 0000406a 413fc090 00000000 00000000 00000000 ef099fe8 c00666ec 
c000f46c
9fe0: 20000113 ffffffff
[<c0012644>] (__irq_svc+0x44/0x78) from [<c000f46c>] 
(arch_cpu_idle+0x34/0x3c)
[<c000f46c>] (arch_cpu_idle+0x34/0x3c) from [<c0053980>] 
(cpu_startup_entry+0xa8/0x10c)
[<c0053980>] (cpu_startup_entry+0xa8/0x10c) from [<000085a4>] (0x85a4)
---[ end trace 8185ad1c7a65f6e7 ]---

-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v2 3/9] clocksource/cadence_ttc: Store timer frequency in driver data

[Sören Brinkmann]

Hi Daniel,

On Thu, Dec 19, 2013 at 09:53:14PM +0100, Daniel Lezcano wrote:
> On 12/19/2013 07:32 PM, Sören Brinkmann wrote:
> >Hi Daniel,
> >
> >On Wed, Dec 18, 2013 at 10:58:26PM +0100, Daniel Lezcano wrote:
> >>On 12/18/2013 05:47 PM, Sören Brinkmann wrote:
> >>>Hi Daniel,
> >>>
> >>>On Wed, Dec 18, 2013 at 03:53:51PM +0100, Daniel Lezcano wrote:
> >>>>On 12/17/2013 08:21 PM, Sören Brinkmann wrote:
> >>>>>Hi Daniel,
> >>>>>
> >>>>>On Tue, Nov 26, 2013 at 05:04:50PM -0800, Soren Brinkmann wrote:
> >>>>>>It is not allowed to call clk_get_rate() from interrupt context. To
> >>>>>>avoid such calls the timer input frequency is stored in the driver's
> >>>>>>data struct which makes it accessible to the driver in any context.
> >>>>>>
> >>>>>>Signed-off-by: Soren Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> >>>>>>Acked-by: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>>>>
> >>>>>I doubt that we'll resolve all issues with this series before the
> >>>>>holidays or even the next merge window. Could you take this patch into
> >>>>>your tree for 3.14? It is not directly related to the cpufreq work and
> >>>>>fixes an actual issue that triggers a kernel WARN under some condition
> >>>>>(I missed preserving the details and the trace). That would take the
> >>>>>easy stuff out of the way and we can focus on the more controversial
> >>>>>changes.
> >>>>
> >>>>You are asking to take it for 3.14 but shouldn't it go as a 3.13 fix ?
> >>>
> >>>That's also an option. As I remember, the patch fixes a kernel WARN. The
> >>>system still seemed operational though. Up to you whether this is
> >>>considered severe enough for the 3.13 series. I'm happy either way.
> >>
> >>I was not able to reproduce the WARN with my board.
> >>
> >>Please, could you give the WARN or give the procedure to reproduce it ?
> >
> >I can't either... I thought I saw the WARN on a vanilla kernel during
> >boot (IIRC, when cpuidle started). Is there any chance the timer core
> >calls the timer's set_mode() from interrupt context?
> >Anyway, let's drop it for now. I'll make sure to record more information
> >in case it reappears.
> 
> Finally I was able to reproduce it with the highres timers disabled,
> the periodic tick system and the locks debug.
> 
> Indeed, we are in an interrupt context (IPI) and we are calling
> clk_get_rate in the the set_mode function which in turn ends up by
> getting a mutex... Even if that does not hang, it is a potential
> kernel crash so I will apply the patch with an updated changelog.

Thanks! Kind of comforting to know that the issue I tried to fix actually exists.

	Sören


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/9] clocksource/cadence_ttc: Store timer frequency in driver data

[Daniel Lezcano]

On 12/19/2013 10:23 PM, Sören Brinkmann wrote:
> Hi Daniel,
>
> On Thu, Dec 19, 2013 at 09:53:14PM +0100, Daniel Lezcano wrote:
>> On 12/19/2013 07:32 PM, Sören Brinkmann wrote:
>>> Hi Daniel,
>>>
>>> On Wed, Dec 18, 2013 at 10:58:26PM +0100, Daniel Lezcano wrote:
>>>> On 12/18/2013 05:47 PM, Sören Brinkmann wrote:
>>>>> Hi Daniel,
>>>>>
>>>>> On Wed, Dec 18, 2013 at 03:53:51PM +0100, Daniel Lezcano wrote:
>>>>>> On 12/17/2013 08:21 PM, Sören Brinkmann wrote:
>>>>>>> Hi Daniel,
>>>>>>>
>>>>>>> On Tue, Nov 26, 2013 at 05:04:50PM -0800, Soren Brinkmann wrote:
>>>>>>>> It is not allowed to call clk_get_rate() from interrupt context. To
>>>>>>>> avoid such calls the timer input frequency is stored in the driver's
>>>>>>>> data struct which makes it accessible to the driver in any context.
>>>>>>>>
>>>>>>>> Signed-off-by: Soren Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
>>>>>>>> Acked-by: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>>>>>
>>>>>>> I doubt that we'll resolve all issues with this series before the
>>>>>>> holidays or even the next merge window. Could you take this patch into
>>>>>>> your tree for 3.14? It is not directly related to the cpufreq work and
>>>>>>> fixes an actual issue that triggers a kernel WARN under some condition
>>>>>>> (I missed preserving the details and the trace). That would take the
>>>>>>> easy stuff out of the way and we can focus on the more controversial
>>>>>>> changes.
>>>>>>
>>>>>> You are asking to take it for 3.14 but shouldn't it go as a 3.13 fix ?
>>>>>
>>>>> That's also an option. As I remember, the patch fixes a kernel WARN. The
>>>>> system still seemed operational though. Up to you whether this is
>>>>> considered severe enough for the 3.13 series. I'm happy either way.
>>>>
>>>> I was not able to reproduce the WARN with my board.
>>>>
>>>> Please, could you give the WARN or give the procedure to reproduce it ?
>>>
>>> I can't either... I thought I saw the WARN on a vanilla kernel during
>>> boot (IIRC, when cpuidle started). Is there any chance the timer core
>>> calls the timer's set_mode() from interrupt context?
>>> Anyway, let's drop it for now. I'll make sure to record more information
>>> in case it reappears.
>>
>> Finally I was able to reproduce it with the highres timers disabled,
>> the periodic tick system and the locks debug.
>>
>> Indeed, we are in an interrupt context (IPI) and we are calling
>> clk_get_rate in the the set_mode function which in turn ends up by
>> getting a mutex... Even if that does not hang, it is a potential
>> kernel crash so I will apply the patch with an updated changelog.
>
> Thanks! Kind of comforting to know that the issue I tried to fix actually exists.

Applied to my tree as a 3.13 fix.

Thanks !
   -- Daniel



-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 4/9] clocksource/cadence_ttc: Use enable/disable_irq

[Soren Brinkmann]

To ensure that the timer interrupt is properly enabled/disabled across
the whole CPU cluster use enable/disable_irq() instead of
local_irq_disable().

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/clocksource/cadence_ttc_timer.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c
index a92350b55d32..246d018d1e63 100644
--- a/drivers/clocksource/cadence_ttc_timer.c
+++ b/drivers/clocksource/cadence_ttc_timer.c
@@ -322,18 +322,16 @@ static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
 	switch (event) {
 	case POST_RATE_CHANGE:
 	{
-		unsigned long flags;
-
 		/*
 		 * clockevents_update_freq should be called with IRQ disabled on
 		 * the CPU the timer provides events for. The timer we use is
 		 * common to both CPUs, not sure if we need to run on both
 		 * cores.
 		 */
-		local_irq_save(flags);
+		disable_irq(ttcce->ce.irq);
 		clockevents_update_freq(&ttcce->ce,
 				ndata->new_rate / PRESCALE);
-		local_irq_restore(flags);
+		enable_irq(ttcce->ce.irq);
 
 		/* update cached frequency */
 		ttc->freq = ndata->new_rate;
-- 
1.8.4.4

Re: [PATCH v2 4/9] clocksource/cadence_ttc: Use enable/disable_irq

[Daniel Lezcano]

On 11/27/2013 02:04 AM, Soren Brinkmann wrote:
> To ensure that the timer interrupt is properly enabled/disabled across
> the whole CPU cluster use enable/disable_irq() instead of
> local_irq_disable().
>
> Signed-off-by: Soren Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> ---
>   drivers/clocksource/cadence_ttc_timer.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c
> index a92350b55d32..246d018d1e63 100644
> --- a/drivers/clocksource/cadence_ttc_timer.c
> +++ b/drivers/clocksource/cadence_ttc_timer.c
> @@ -322,18 +322,16 @@ static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
>   	switch (event) {
>   	case POST_RATE_CHANGE:
>   	{
> -		unsigned long flags;
> -
>   		/*
>   		 * clockevents_update_freq should be called with IRQ disabled on
>   		 * the CPU the timer provides events for. The timer we use is
>   		 * common to both CPUs, not sure if we need to run on both
>   		 * cores.
>   		 */
> -		local_irq_save(flags);
> +		disable_irq(ttcce->ce.irq);
>   		clockevents_update_freq(&ttcce->ce,
>   				ndata->new_rate / PRESCALE);
> -		local_irq_restore(flags);
> +		enable_irq(ttcce->ce.irq);
>
>   		/* update cached frequency */
>   		ttc->freq = ndata->new_rate;
>

I am worried about the 'disable_irq' function calling 'synchronize_irq'. 
Isn't possible to deadlock with the ondemand cpufreq governor ? I added 
Viresh in Cc, he knows better than me the code path.

-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/9] clocksource/cadence_ttc: Use enable/disable_irq

[Thomas Gleixner]

On Thu, 28 Nov 2013, Daniel Lezcano wrote:

> On 11/27/2013 02:04 AM, Soren Brinkmann wrote:
> > To ensure that the timer interrupt is properly enabled/disabled across
> > the whole CPU cluster use enable/disable_irq() instead of
> > local_irq_disable().
> > 
> > Signed-off-by: Soren Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> > ---
> >   drivers/clocksource/cadence_ttc_timer.c | 6 ++----
> >   1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/clocksource/cadence_ttc_timer.c
> > b/drivers/clocksource/cadence_ttc_timer.c
> > index a92350b55d32..246d018d1e63 100644
> > --- a/drivers/clocksource/cadence_ttc_timer.c
> > +++ b/drivers/clocksource/cadence_ttc_timer.c
> > @@ -322,18 +322,16 @@ static int ttc_rate_change_clockevent_cb(struct
> > notifier_block *nb,
> >   	switch (event) {
> >   	case POST_RATE_CHANGE:
> >   	{
> > -		unsigned long flags;
> > -
> >   		/*
> >   		 * clockevents_update_freq should be called with IRQ disabled
> > on
> >   		 * the CPU the timer provides events for. The timer we use is
> >   		 * common to both CPUs, not sure if we need to run on both
> >   		 * cores.

Can we adjust that bogus comment as well, please?

> >   		 */
> > -		local_irq_save(flags);
> > +		disable_irq(ttcce->ce.irq);
> >   		clockevents_update_freq(&ttcce->ce,
> >   				ndata->new_rate / PRESCALE);
> > -		local_irq_restore(flags);
> > +		enable_irq(ttcce->ce.irq);
> > 
> >   		/* update cached frequency */
> >   		ttc->freq = ndata->new_rate;
> > 
> 
> I am worried about the 'disable_irq' function calling 'synchronize_irq'. Isn't
> possible to deadlock with the ondemand cpufreq governor ? I added Viresh in
> Cc, he knows better than me the code path.

disable_irq() will only deadlock if called from the interrupt handler
of the device interrupt itself or when the calling code is holding
locks which prevent the function to proceed.

But what's more important is, that the patch violates the calling
convention of clockevents_update_freq(). It must be called with
interrupts disabled. 

Now the problem with this device is that it is not a per cpu
device. It's a global device, so this update can conflict with a
parallel access on the other CPU. Now the disable_irq() only prevents
that the other CPU can handle a device interrupt from that timer. But
it does not prevent any parallel access from e.g. the idle code path
which will try to reprogram it.

Soren, is that timer used as the broadcast device ?

Thanks,

	tglx



--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/9] clocksource/cadence_ttc: Use enable/disable_irq

[Sören Brinkmann]

Hi Thomas,

On Thu, Nov 28, 2013 at 03:18:50PM +0100, Thomas Gleixner wrote:
> On Thu, 28 Nov 2013, Daniel Lezcano wrote:
> 
> > On 11/27/2013 02:04 AM, Soren Brinkmann wrote:
> > > To ensure that the timer interrupt is properly enabled/disabled across
> > > the whole CPU cluster use enable/disable_irq() instead of
> > > local_irq_disable().
> > > 
> > > Signed-off-by: Soren Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> > > ---
> > >   drivers/clocksource/cadence_ttc_timer.c | 6 ++----
> > >   1 file changed, 2 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/clocksource/cadence_ttc_timer.c
> > > b/drivers/clocksource/cadence_ttc_timer.c
> > > index a92350b55d32..246d018d1e63 100644
> > > --- a/drivers/clocksource/cadence_ttc_timer.c
> > > +++ b/drivers/clocksource/cadence_ttc_timer.c
> > > @@ -322,18 +322,16 @@ static int ttc_rate_change_clockevent_cb(struct
> > > notifier_block *nb,
> > >   	switch (event) {
> > >   	case POST_RATE_CHANGE:
> > >   	{
> > > -		unsigned long flags;
> > > -
> > >   		/*
> > >   		 * clockevents_update_freq should be called with IRQ disabled
> > > on
> > >   		 * the CPU the timer provides events for. The timer we use is
> > >   		 * common to both CPUs, not sure if we need to run on both
> > >   		 * cores.
> 
> Can we adjust that bogus comment as well, please?
I can remove that comment. I'll see if including it in one of the
existing patches or adding another patch for it makes sense.

> 
> > >   		 */
> > > -		local_irq_save(flags);
> > > +		disable_irq(ttcce->ce.irq);
> > >   		clockevents_update_freq(&ttcce->ce,
> > >   				ndata->new_rate / PRESCALE);
> > > -		local_irq_restore(flags);
> > > +		enable_irq(ttcce->ce.irq);
> > > 
> > >   		/* update cached frequency */
> > >   		ttc->freq = ndata->new_rate;
> > > 
> > 
> > I am worried about the 'disable_irq' function calling 'synchronize_irq'. Isn't
> > possible to deadlock with the ondemand cpufreq governor ? I added Viresh in
> > Cc, he knows better than me the code path.
> 
> disable_irq() will only deadlock if called from the interrupt handler
> of the device interrupt itself or when the calling code is holding
> locks which prevent the function to proceed.
> 
> But what's more important is, that the patch violates the calling
> convention of clockevents_update_freq(). It must be called with
> interrupts disabled. 
> 
> Now the problem with this device is that it is not a per cpu
> device. It's a global device, so this update can conflict with a
> parallel access on the other CPU. Now the disable_irq() only prevents
> that the other CPU can handle a device interrupt from that timer. But
> it does not prevent any parallel access from e.g. the idle code path
> which will try to reprogram it.
Does that mean interrupts need to be disabled globally? Also, does the
cpuidle path depend on interrupts or can it interfere no matter what?

> 
> Soren, is that timer used as the broadcast device ?
Yes, this is the only broadcast capable timer on Zynq, AFAIK. Other than
the TTC we only have the arm_global_timer and smp_twd timers, which both
are per_cpu devices and thus not broadcast capable.

	Sören


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/9] clocksource/cadence_ttc: Use enable/disable_irq

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5893 bytes --]

On Thu, 28 Nov 2013, Sören Brinkmann wrote:
> On Thu, Nov 28, 2013 at 03:18:50PM +0100, Thomas Gleixner wrote:
> > Now the problem with this device is that it is not a per cpu
> > device. It's a global device, so this update can conflict with a
> > parallel access on the other CPU. Now the disable_irq() only prevents
> > that the other CPU can handle a device interrupt from that timer. But
> > it does not prevent any parallel access from e.g. the idle code path
> > which will try to reprogram it.
>
> Does that mean interrupts need to be disabled globally? Also, does the

Globally disabling interrupts is not going to work, except you want to
use stomp_machine(). But that would be overkill.

> cpuidle path depend on interrupts or can it interfere no matter what?

It can interfere no matter what. The broadcast is modified for the cpu
which loses its per cpu timer due to the idle state via

      clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, ...);
 
> > Soren, is that timer used as the broadcast device ?
>
> Yes, this is the only broadcast capable timer on Zynq, AFAIK. Other than
> the TTC we only have the arm_global_timer and smp_twd timers, which both
> are per_cpu devices and thus not broadcast capable.

There is a solution to this. We can identify the broadcast device in
the core and serialize all callers including interrupts on a different
cpu against the update. So no need for the disable/enable_irq() dance.

See patch below.

Thanks,

	tglx
---

Index: linux-2.6/kernel/time/clockevents.c
===================================================================
--- linux-2.6.orig/kernel/time/clockevents.c
+++ linux-2.6/kernel/time/clockevents.c
@@ -439,6 +439,16 @@ void clockevents_config_and_register(str
 }
 EXPORT_SYMBOL_GPL(clockevents_config_and_register);
 
+int __clockevents_update_freq(struct clock_event_device *dev, u32 freq)
+{
+	clockevents_config(dev, freq);
+
+	if (dev->mode != CLOCK_EVT_MODE_ONESHOT)
+		return 0;
+
+	return clockevents_program_event(dev, dev->next_event, false);
+}
+
 /**
  * clockevents_update_freq - Update frequency and reprogram a clock event device.
  * @dev:	device to modify
@@ -446,17 +456,22 @@ EXPORT_SYMBOL_GPL(clockevents_config_and
  *
  * Reconfigure and reprogram a clock event device in oneshot
  * mode. Must be called on the cpu for which the device delivers per
- * cpu timer events with interrupts disabled!  Returns 0 on success,
- * -ETIME when the event is in the past.
+ * cpu timer events. If called for the broadcast device the core takes
+ * care of serialization.
+ *
+ * Returns 0 on success, -ETIME when the event is in the past.
  */
 int clockevents_update_freq(struct clock_event_device *dev, u32 freq)
 {
-	clockevents_config(dev, freq);
+	unsigned long flags;
+	int ret;
 
-	if (dev->mode != CLOCK_EVT_MODE_ONESHOT)
-		return 0;
-
-	return clockevents_program_event(dev, dev->next_event, false);
+	local_irq_save(flags);
+	ret = tick_broadcast_update_freq(dev, freq);
+	if (ret == -ENODEV)
+		ret = __clockevents_update_freq(dev, freq);
+	local_irq_restore(flags);
+	return ret;
 }
 
 /*
Index: linux-2.6/kernel/time/tick-broadcast.c
===================================================================
--- linux-2.6.orig/kernel/time/tick-broadcast.c
+++ linux-2.6/kernel/time/tick-broadcast.c
@@ -120,6 +120,19 @@ int tick_is_broadcast_device(struct cloc
 	return (dev && tick_broadcast_device.evtdev == dev);
 }
 
+int tick_broadcast_update_freq(struct clock_event_device *dev, u32 freq)
+{
+	int ret = -ENODEV;
+
+	if (tick_is_broadcast_device(dev)) {
+		raw_spin_lock(&tick_broadcast_lock);
+		ret = __clockevents_update_freq(dev, freq);
+		raw_spin_unlock(&tick_broadcast_lock);
+	}
+	return ret;
+}
+
+
 static void err_broadcast(const struct cpumask *mask)
 {
 	pr_crit_once("Failed to broadcast timer tick. Some CPUs may be unresponsive.\n");
@@ -272,12 +285,8 @@ static void tick_do_broadcast(struct cpu
  */
 static void tick_do_periodic_broadcast(void)
 {
-	raw_spin_lock(&tick_broadcast_lock);
-
 	cpumask_and(tmpmask, cpu_online_mask, tick_broadcast_mask);
 	tick_do_broadcast(tmpmask);
-
-	raw_spin_unlock(&tick_broadcast_lock);
 }
 
 /*
@@ -287,13 +296,15 @@ static void tick_handle_periodic_broadca
 {
 	ktime_t next;
 
+	raw_spin_lock(&tick_broadcast_lock);
+
 	tick_do_periodic_broadcast();
 
 	/*
 	 * The device is in periodic mode. No reprogramming necessary:
 	 */
 	if (dev->mode == CLOCK_EVT_MODE_PERIODIC)
-		return;
+		goto unlock;
 
 	/*
 	 * Setup the next period for devices, which do not have
@@ -306,9 +317,11 @@ static void tick_handle_periodic_broadca
 		next = ktime_add(next, tick_period);
 
 		if (!clockevents_program_event(dev, next, false))
-			return;
+			goto unlock;
 		tick_do_periodic_broadcast();
 	}
+unlock:
+	raw_spin_unlock(&tick_broadcast_lock);
 }
 
 /*
Index: linux-2.6/kernel/time/tick-internal.h
===================================================================
--- linux-2.6.orig/kernel/time/tick-internal.h
+++ linux-2.6/kernel/time/tick-internal.h
@@ -111,6 +111,7 @@ extern int tick_resume_broadcast(void);
 extern void tick_broadcast_init(void);
 extern void
 tick_set_periodic_handler(struct clock_event_device *dev, int broadcast);
+int tick_broadcast_update_freq(struct clock_event_device *dev, u32 freq);
 
 #else /* !BROADCAST */
 
@@ -133,6 +134,8 @@ static inline void tick_shutdown_broadca
 static inline void tick_suspend_broadcast(void) { }
 static inline int tick_resume_broadcast(void) { return 0; }
 static inline void tick_broadcast_init(void) { }
+static inline int tick_broadcast_update_freq(struct clock_event_device *dev,
+					     u32 freq) { return -ENODEV; }
 
 /*
  * Set the periodic handler in non broadcast mode
@@ -154,4 +157,5 @@ static inline int tick_device_is_functio
 
 #endif
 
+int __clockevents_update_freq(struct clock_event_device *dev, u32 freq);
 extern void do_timer(unsigned long ticks);

Re: [PATCH v2 4/9] clocksource/cadence_ttc: Use enable/disable_irq

[Sören Brinkmann]

Hi Thomas,

sorry for the delay, but I couldn't find time any earlier.

On Thu, Nov 28, 2013 at 08:07:10PM +0100, Thomas Gleixner wrote:
> On Thu, 28 Nov 2013, Sören Brinkmann wrote:
> > On Thu, Nov 28, 2013 at 03:18:50PM +0100, Thomas Gleixner wrote:
> > > Now the problem with this device is that it is not a per cpu
> > > device. It's a global device, so this update can conflict with a
> > > parallel access on the other CPU. Now the disable_irq() only prevents
> > > that the other CPU can handle a device interrupt from that timer. But
> > > it does not prevent any parallel access from e.g. the idle code path
> > > which will try to reprogram it.
> >
> > Does that mean interrupts need to be disabled globally? Also, does the
> 
> Globally disabling interrupts is not going to work, except you want to
> use stomp_machine(). But that would be overkill.
> 
> > cpuidle path depend on interrupts or can it interfere no matter what?
> 
> It can interfere no matter what. The broadcast is modified for the cpu
> which loses its per cpu timer due to the idle state via
> 
>       clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, ...);
>  
> > > Soren, is that timer used as the broadcast device ?
> >
> > Yes, this is the only broadcast capable timer on Zynq, AFAIK. Other than
> > the TTC we only have the arm_global_timer and smp_twd timers, which both
> > are per_cpu devices and thus not broadcast capable.
> 
> There is a solution to this. We can identify the broadcast device in
> the core and serialize all callers including interrupts on a different
> cpu against the update. So no need for the disable/enable_irq() dance.

IIUC, and please correct me if I'm wrong, with the patch I'd simply call
'clockevents_update_freq() without having to disable IRQs. But I'm not
sure whether periodic mode is covered. I found, that I had to reprogram
the timer interval in my clock notifier callback when the timer
frequency changes. I think 'clockevents_update_freq()' only handles
oneshot mode. For that reason I call 'ttc_set_interval()' in the clock
notifier in case the timer is in periodic mode. For that call we'd still
have possible races. I guess the best solution would be to move that
functionality into 'clockevents_update_freq()'?

	Sören

Re: [PATCH v2 4/9] clocksource/cadence_ttc: Use enable/disable_irq

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 973 bytes --]

On Fri, 6 Dec 2013, Sören Brinkmann wrote:
> On Thu, Nov 28, 2013 at 08:07:10PM +0100, Thomas Gleixner wrote:
> > There is a solution to this. We can identify the broadcast device in
> > the core and serialize all callers including interrupts on a different
> > cpu against the update. So no need for the disable/enable_irq() dance.
> 
> IIUC, and please correct me if I'm wrong, with the patch I'd simply call
> 'clockevents_update_freq() without having to disable IRQs. But I'm not
> sure whether periodic mode is covered. I found, that I had to reprogram
> the timer interval in my clock notifier callback when the timer
> frequency changes. I think 'clockevents_update_freq()' only handles
> oneshot mode. For that reason I call 'ttc_set_interval()' in the clock
> notifier in case the timer is in periodic mode. For that call we'd still
> have possible races. I guess the best solution would be to move that
> functionality into 'clockevents_update_freq()'?

Indeed.

[PATCH 0/2] clockevents

[Soren Brinkmann]

Hi Thomas,

before respinning the complete series, I thought we could take a look at
the changes to the timer core, since I expect them to need to be refined
for an actual submission.

The first patch is the patch you proposed to serialize callers of
clockevents_update_freq(). I guess you may find some better words to
describe your changes. I found it not that easy to find a nice
description for somebody else's patch.

The second patch is my shot at extending clockevents_update_freq()
to also handle timers in periodic mode. In my approach I assume that
simply calling the timer's 'set_mode()' API takes care of programming an
appropriate interval based on the current timer frequency. If that
doesn't work for all timers, I guess it would require a new API call. Or
I just missed some other way.

	Thanks,
	Sören

Soren Brinkmann (2):
  time: Serialize calls to 'clockevents_update_freq' in the timing core
  time: clockevents: Adjust timer interval when frequency changes

 kernel/time/clockevents.c    | 32 +++++++++++++++++++++++++-------
 kernel/time/tick-broadcast.c | 25 +++++++++++++++++++------
 kernel/time/tick-internal.h  |  4 ++++
 3 files changed, 48 insertions(+), 13 deletions(-)

-- 
1.8.5.1

[PATCH 1/2] time: Serialize calls to 'clockevents_update_freq' in the timing core

[Soren Brinkmann]

From: Thomas Gleixner <tglx@linutronix.de>

We can identify the broadcast device in the core and serialize all
callers including interrupts on a different CPU against the update.
Also, disabling interrupts is moved into the core allowing callers to
leave interrutps enabled when calling clockevents_update_freq().

Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 kernel/time/clockevents.c    | 29 ++++++++++++++++++++++-------
 kernel/time/tick-broadcast.c | 25 +++++++++++++++++++------
 kernel/time/tick-internal.h  |  4 ++++
 3 files changed, 45 insertions(+), 13 deletions(-)

diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index 086ad6043bcb..641d91003a45 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -439,6 +439,16 @@ void clockevents_config_and_register(struct clock_event_device *dev,
 }
 EXPORT_SYMBOL_GPL(clockevents_config_and_register);
 
+int __clockevents_update_freq(struct clock_event_device *dev, u32 freq)
+{
+	clockevents_config(dev, freq);
+
+	if (dev->mode != CLOCK_EVT_MODE_ONESHOT)
+		return 0;
+
+	return clockevents_program_event(dev, dev->next_event, false);
+}
+
 /**
  * clockevents_update_freq - Update frequency and reprogram a clock event device.
  * @dev:	device to modify
@@ -446,17 +456,22 @@ EXPORT_SYMBOL_GPL(clockevents_config_and_register);
  *
  * Reconfigure and reprogram a clock event device in oneshot
  * mode. Must be called on the cpu for which the device delivers per
- * cpu timer events with interrupts disabled!  Returns 0 on success,
- * -ETIME when the event is in the past.
+ * cpu timer events. If called for the broadcast device the core takes
+ * care of serialization.
+ *
+ * Returns 0 on success, -ETIME when the event is in the past.
  */
 int clockevents_update_freq(struct clock_event_device *dev, u32 freq)
 {
-	clockevents_config(dev, freq);
-
-	if (dev->mode != CLOCK_EVT_MODE_ONESHOT)
-		return 0;
+	unsigned long flags;
+	int ret;
 
-	return clockevents_program_event(dev, dev->next_event, false);
+	local_irq_save(flags);
+	ret = tick_broadcast_update_freq(dev, freq);
+	if (ret == -ENODEV)
+		ret = __clockevents_update_freq(dev, freq);
+	local_irq_restore(flags);
+	return ret;
 }
 
 /*
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index 9532690daaa9..63a33b33fc4e 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -120,6 +120,19 @@ int tick_is_broadcast_device(struct clock_event_device *dev)
 	return (dev && tick_broadcast_device.evtdev == dev);
 }
 
+int tick_broadcast_update_freq(struct clock_event_device *dev, u32 freq)
+{
+	int ret = -ENODEV;
+
+	if (tick_is_broadcast_device(dev)) {
+		raw_spin_lock(&tick_broadcast_lock);
+		ret = __clockevents_update_freq(dev, freq);
+		raw_spin_unlock(&tick_broadcast_lock);
+	}
+	return ret;
+}
+
+
 static void err_broadcast(const struct cpumask *mask)
 {
 	pr_crit_once("Failed to broadcast timer tick. Some CPUs may be unresponsive.\n");
@@ -272,12 +285,8 @@ static void tick_do_broadcast(struct cpumask *mask)
  */
 static void tick_do_periodic_broadcast(void)
 {
-	raw_spin_lock(&tick_broadcast_lock);
-
 	cpumask_and(tmpmask, cpu_online_mask, tick_broadcast_mask);
 	tick_do_broadcast(tmpmask);
-
-	raw_spin_unlock(&tick_broadcast_lock);
 }
 
 /*
@@ -287,13 +296,15 @@ static void tick_handle_periodic_broadcast(struct clock_event_device *dev)
 {
 	ktime_t next;
 
+	raw_spin_lock(&tick_broadcast_lock);
+
 	tick_do_periodic_broadcast();
 
 	/*
 	 * The device is in periodic mode. No reprogramming necessary:
 	 */
 	if (dev->mode == CLOCK_EVT_MODE_PERIODIC)
-		return;
+		goto unlock;
 
 	/*
 	 * Setup the next period for devices, which do not have
@@ -306,9 +317,11 @@ static void tick_handle_periodic_broadcast(struct clock_event_device *dev)
 		next = ktime_add(next, tick_period);
 
 		if (!clockevents_program_event(dev, next, false))
-			return;
+			goto unlock;
 		tick_do_periodic_broadcast();
 	}
+unlock:
+	raw_spin_unlock(&tick_broadcast_lock);
 }
 
 /*
diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h
index 18e71f7fbc2a..22fd431a02eb 100644
--- a/kernel/time/tick-internal.h
+++ b/kernel/time/tick-internal.h
@@ -111,6 +111,7 @@ extern int tick_resume_broadcast(void);
 extern void tick_broadcast_init(void);
 extern void
 tick_set_periodic_handler(struct clock_event_device *dev, int broadcast);
+int tick_broadcast_update_freq(struct clock_event_device *dev, u32 freq);
 
 #else /* !BROADCAST */
 
@@ -133,6 +134,8 @@ static inline void tick_shutdown_broadcast(unsigned int *cpup) { }
 static inline void tick_suspend_broadcast(void) { }
 static inline int tick_resume_broadcast(void) { return 0; }
 static inline void tick_broadcast_init(void) { }
+static inline int tick_broadcast_update_freq(struct clock_event_device *dev,
+					     u32 freq) { return -ENODEV; }
 
 /*
  * Set the periodic handler in non broadcast mode
@@ -154,4 +157,5 @@ static inline int tick_device_is_functional(struct clock_event_device *dev)
 
 #endif
 
+int __clockevents_update_freq(struct clock_event_device *dev, u32 freq);
 extern void do_timer(unsigned long ticks);
-- 
1.8.5.1

Re: [PATCH 1/2] time: Serialize calls to 'clockevents_update_freq' in the timing core

[Daniel Lezcano]

On 12/10/2013 01:34 AM, Soren Brinkmann wrote:
> From: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
>
> We can identify the broadcast device in the core and serialize all
> callers including interrupts on a different CPU against the update.
> Also, disabling interrupts is moved into the core allowing callers to
> leave interrutps enabled when calling clockevents_update_freq().
>
> Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> Signed-off-by: Soren Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> ---
>   kernel/time/clockevents.c    | 29 ++++++++++++++++++++++-------
>   kernel/time/tick-broadcast.c | 25 +++++++++++++++++++------
>   kernel/time/tick-internal.h  |  4 ++++
>   3 files changed, 45 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
> index 086ad6043bcb..641d91003a45 100644
> --- a/kernel/time/clockevents.c
> +++ b/kernel/time/clockevents.c
> @@ -439,6 +439,16 @@ void clockevents_config_and_register(struct clock_event_device *dev,
>   }
>   EXPORT_SYMBOL_GPL(clockevents_config_and_register);
>
> +int __clockevents_update_freq(struct clock_event_device *dev, u32 freq)
> +{
> +	clockevents_config(dev, freq);
> +
> +	if (dev->mode != CLOCK_EVT_MODE_ONESHOT)
> +		return 0;
> +
> +	return clockevents_program_event(dev, dev->next_event, false);
> +}
> +

./arch/arm/kernel/smp_twd.c should be modified to call 
__clockevents_update_freq instead of clockevents_update_freq, no ?

>   /**
>    * clockevents_update_freq - Update frequency and reprogram a clock event device.
>    * @dev:	device to modify
> @@ -446,17 +456,22 @@ EXPORT_SYMBOL_GPL(clockevents_config_and_register);
>    *
>    * Reconfigure and reprogram a clock event device in oneshot
>    * mode. Must be called on the cpu for which the device delivers per
> - * cpu timer events with interrupts disabled!  Returns 0 on success,
> - * -ETIME when the event is in the past.
> + * cpu timer events. If called for the broadcast device the core takes
> + * care of serialization.
> + *
> + * Returns 0 on success, -ETIME when the event is in the past.
>    */
>   int clockevents_update_freq(struct clock_event_device *dev, u32 freq)
>   {
> -	clockevents_config(dev, freq);
> -
> -	if (dev->mode != CLOCK_EVT_MODE_ONESHOT)
> -		return 0;
> +	unsigned long flags;
> +	int ret;
>
> -	return clockevents_program_event(dev, dev->next_event, false);
> +	local_irq_save(flags);
> +	ret = tick_broadcast_update_freq(dev, freq);
> +	if (ret == -ENODEV)
> +		ret = __clockevents_update_freq(dev, freq);
> +	local_irq_restore(flags);
> +	return ret;
>   }
>
>   /*
> diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
> index 9532690daaa9..63a33b33fc4e 100644
> --- a/kernel/time/tick-broadcast.c
> +++ b/kernel/time/tick-broadcast.c
> @@ -120,6 +120,19 @@ int tick_is_broadcast_device(struct clock_event_device *dev)
>   	return (dev && tick_broadcast_device.evtdev == dev);
>   }
>
> +int tick_broadcast_update_freq(struct clock_event_device *dev, u32 freq)
> +{
> +	int ret = -ENODEV;
> +
> +	if (tick_is_broadcast_device(dev)) {
> +		raw_spin_lock(&tick_broadcast_lock);
> +		ret = __clockevents_update_freq(dev, freq);
> +		raw_spin_unlock(&tick_broadcast_lock);
> +	}
> +	return ret;
> +}
> +
> +
>   static void err_broadcast(const struct cpumask *mask)
>   {
>   	pr_crit_once("Failed to broadcast timer tick. Some CPUs may be unresponsive.\n");
> @@ -272,12 +285,8 @@ static void tick_do_broadcast(struct cpumask *mask)
>    */
>   static void tick_do_periodic_broadcast(void)
>   {
> -	raw_spin_lock(&tick_broadcast_lock);
> -
>   	cpumask_and(tmpmask, cpu_online_mask, tick_broadcast_mask);
>   	tick_do_broadcast(tmpmask);
> -
> -	raw_spin_unlock(&tick_broadcast_lock);
>   }
>
>   /*
> @@ -287,13 +296,15 @@ static void tick_handle_periodic_broadcast(struct clock_event_device *dev)
>   {
>   	ktime_t next;
>
> +	raw_spin_lock(&tick_broadcast_lock);
> +
>   	tick_do_periodic_broadcast();
>
>   	/*
>   	 * The device is in periodic mode. No reprogramming necessary:
>   	 */
>   	if (dev->mode == CLOCK_EVT_MODE_PERIODIC)
> -		return;
> +		goto unlock;
>
>   	/*
>   	 * Setup the next period for devices, which do not have
> @@ -306,9 +317,11 @@ static void tick_handle_periodic_broadcast(struct clock_event_device *dev)
>   		next = ktime_add(next, tick_period);
>
>   		if (!clockevents_program_event(dev, next, false))
> -			return;
> +			goto unlock;
>   		tick_do_periodic_broadcast();
>   	}
> +unlock:
> +	raw_spin_unlock(&tick_broadcast_lock);
>   }
>
>   /*
> diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h
> index 18e71f7fbc2a..22fd431a02eb 100644
> --- a/kernel/time/tick-internal.h
> +++ b/kernel/time/tick-internal.h
> @@ -111,6 +111,7 @@ extern int tick_resume_broadcast(void);
>   extern void tick_broadcast_init(void);
>   extern void
>   tick_set_periodic_handler(struct clock_event_device *dev, int broadcast);
> +int tick_broadcast_update_freq(struct clock_event_device *dev, u32 freq);
>
>   #else /* !BROADCAST */
>
> @@ -133,6 +134,8 @@ static inline void tick_shutdown_broadcast(unsigned int *cpup) { }
>   static inline void tick_suspend_broadcast(void) { }
>   static inline int tick_resume_broadcast(void) { return 0; }
>   static inline void tick_broadcast_init(void) { }
> +static inline int tick_broadcast_update_freq(struct clock_event_device *dev,
> +					     u32 freq) { return -ENODEV; }
>
>   /*
>    * Set the periodic handler in non broadcast mode
> @@ -154,4 +157,5 @@ static inline int tick_device_is_functional(struct clock_event_device *dev)
>
>   #endif
>
> +int __clockevents_update_freq(struct clock_event_device *dev, u32 freq);
>   extern void do_timer(unsigned long ticks);
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] time: Serialize calls to 'clockevents_update_freq' in the timing core

[Sören Brinkmann]

Hi Daniel,

On Wed, Dec 11, 2013 at 03:32:57PM +0100, Daniel Lezcano wrote:
> On 12/10/2013 01:34 AM, Soren Brinkmann wrote:
> >From: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> >
> >We can identify the broadcast device in the core and serialize all
> >callers including interrupts on a different CPU against the update.
> >Also, disabling interrupts is moved into the core allowing callers to
> >leave interrutps enabled when calling clockevents_update_freq().
> >
> >Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> >Signed-off-by: Soren Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> >---
> >  kernel/time/clockevents.c    | 29 ++++++++++++++++++++++-------
> >  kernel/time/tick-broadcast.c | 25 +++++++++++++++++++------
> >  kernel/time/tick-internal.h  |  4 ++++
> >  3 files changed, 45 insertions(+), 13 deletions(-)
> >
> >diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
> >index 086ad6043bcb..641d91003a45 100644
> >--- a/kernel/time/clockevents.c
> >+++ b/kernel/time/clockevents.c
> >@@ -439,6 +439,16 @@ void clockevents_config_and_register(struct clock_event_device *dev,
> >  }
> >  EXPORT_SYMBOL_GPL(clockevents_config_and_register);
> >
> >+int __clockevents_update_freq(struct clock_event_device *dev, u32 freq)
> >+{
> >+	clockevents_config(dev, freq);
> >+
> >+	if (dev->mode != CLOCK_EVT_MODE_ONESHOT)
> >+		return 0;
> >+
> >+	return clockevents_program_event(dev, dev->next_event, false);
> >+}
> >+
> 
> ./arch/arm/kernel/smp_twd.c should be modified to call
> __clockevents_update_freq instead of clockevents_update_freq, no ?

IIUC, the __-version is only for timer core internal usage and not an
exported interface. Doesn't the non-__ version work for the twd?
I don't see issues on my Zynq platform with these patches.

Another side note regarding the twd: Shouldn't we set the
CLOCK_EVT_FEAT_PERCPU flag we introduced recently for that timer?

	Sören


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] time: Serialize calls to 'clockevents_update_freq' in the timing core

[Daniel Lezcano]

On 12/11/2013 09:09 PM, Sören Brinkmann wrote:
> Hi Daniel,
>
> On Wed, Dec 11, 2013 at 03:32:57PM +0100, Daniel Lezcano wrote:
>> On 12/10/2013 01:34 AM, Soren Brinkmann wrote:
>>> From: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
>>>
>>> We can identify the broadcast device in the core and serialize all
>>> callers including interrupts on a different CPU against the update.
>>> Also, disabling interrupts is moved into the core allowing callers to
>>> leave interrutps enabled when calling clockevents_update_freq().
>>>
>>> Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
>>> Signed-off-by: Soren Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>   kernel/time/clockevents.c    | 29 ++++++++++++++++++++++-------
>>>   kernel/time/tick-broadcast.c | 25 +++++++++++++++++++------
>>>   kernel/time/tick-internal.h  |  4 ++++
>>>   3 files changed, 45 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
>>> index 086ad6043bcb..641d91003a45 100644
>>> --- a/kernel/time/clockevents.c
>>> +++ b/kernel/time/clockevents.c
>>> @@ -439,6 +439,16 @@ void clockevents_config_and_register(struct clock_event_device *dev,
>>>   }
>>>   EXPORT_SYMBOL_GPL(clockevents_config_and_register);
>>>
>>> +int __clockevents_update_freq(struct clock_event_device *dev, u32 freq)
>>> +{
>>> +	clockevents_config(dev, freq);
>>> +
>>> +	if (dev->mode != CLOCK_EVT_MODE_ONESHOT)
>>> +		return 0;
>>> +
>>> +	return clockevents_program_event(dev, dev->next_event, false);
>>> +}
>>> +
>>
>> ./arch/arm/kernel/smp_twd.c should be modified to call
>> __clockevents_update_freq instead of clockevents_update_freq, no ?
>
> IIUC, the __-version is only for timer core internal usage and not an
> exported interface. Doesn't the non-__ version work for the twd?
> I don't see issues on my Zynq platform with these patches.

I don't think there is an issue but if we can rid of the extra code 
added in clockevents_update_freq with this patch for twd, it would make 
sense. The __-version means the function is lockless and there is a 
function without '__' in the name with the lock.

> Another side note regarding the twd: Shouldn't we set the
> CLOCK_EVT_FEAT_PERCPU flag we introduced recently for that timer?

Yes.

There will be some fun with the patchset http://lwn.net/Articles/566270/ 
where the local timer could be used as a broadcast timer.


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] time: clockevents: Adjust timer interval when frequency changes

[Soren Brinkmann]

To adjust the timer's interval in periodic mode, the clockevent device
is put into periodic mode during clockevents_update_freq() in case the
timer is in periodic mode.

Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 kernel/time/clockevents.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index 641d91003a45..f85e5fda9c66 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -443,10 +443,13 @@ int __clockevents_update_freq(struct clock_event_device *dev, u32 freq)
 {
 	clockevents_config(dev, freq);
 
-	if (dev->mode != CLOCK_EVT_MODE_ONESHOT)
-		return 0;
+	if (dev->mode == CLOCK_EVT_MODE_ONESHOT)
+		return clockevents_program_event(dev, dev->next_event, false);
+
+	if (dev->mode == CLOCK_EVT_MODE_PERIODIC)
+		dev->set_mode(CLOCK_EVT_MODE_PERIODIC, dev);
 
-	return clockevents_program_event(dev, dev->next_event, false);
+	return 0;
 }
 
 /**
-- 
1.8.5.1

[PATCH v2 5/9] clocksource/cadence_ttc: Adjust interval in clock notifier

[Soren Brinkmann]

The clockevent has to be reprogrammed if the timer's input
clock frequency changes and the timer is in periodic mode, in order to
maintain the correct timer interval.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
v2:
 - adjust the timer interval while the timer interrupt is disabled
---
 drivers/clocksource/cadence_ttc_timer.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c
index 246d018d1e63..77517675653e 100644
--- a/drivers/clocksource/cadence_ttc_timer.c
+++ b/drivers/clocksource/cadence_ttc_timer.c
@@ -322,6 +322,9 @@ static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
 	switch (event) {
 	case POST_RATE_CHANGE:
 	{
+		/* update cached frequency */
+		ttc->freq = ndata->new_rate;
+
 		/*
 		 * clockevents_update_freq should be called with IRQ disabled on
 		 * the CPU the timer provides events for. The timer we use is
@@ -329,12 +332,15 @@ static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
 		 * cores.
 		 */
 		disable_irq(ttcce->ce.irq);
+
 		clockevents_update_freq(&ttcce->ce,
 				ndata->new_rate / PRESCALE);
-		enable_irq(ttcce->ce.irq);
 
-		/* update cached frequency */
-		ttc->freq = ndata->new_rate;
+		if (ttcce->ce.mode == CLOCK_EVT_MODE_PERIODIC)
+			ttc_set_interval(ttc, DIV_ROUND_CLOSEST(ttc->freq,
+						PRESCALE * HZ));
+
+		enable_irq(ttcce->ce.irq);
 
 		/* fall through */
 	}
-- 
1.8.4.4

Re: [PATCH v2 5/9] clocksource/cadence_ttc: Adjust interval in clock notifier

[Daniel Lezcano]

On 11/27/2013 02:04 AM, Soren Brinkmann wrote:
> The clockevent has to be reprogrammed if the timer's input
> clock frequency changes and the timer is in periodic mode, in order to
> maintain the correct timer interval.
>
> Signed-off-by: Soren Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> ---
> v2:
>   - adjust the timer interval while the timer interrupt is disabled
> ---
>   drivers/clocksource/cadence_ttc_timer.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c
> index 246d018d1e63..77517675653e 100644
> --- a/drivers/clocksource/cadence_ttc_timer.c
> +++ b/drivers/clocksource/cadence_ttc_timer.c
> @@ -322,6 +322,9 @@ static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
>   	switch (event) {
>   	case POST_RATE_CHANGE:
>   	{
> +		/* update cached frequency */
> +		ttc->freq = ndata->new_rate;
> +
>   		/*
>   		 * clockevents_update_freq should be called with IRQ disabled on
>   		 * the CPU the timer provides events for. The timer we use is
> @@ -329,12 +332,15 @@ static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
>   		 * cores.
>   		 */
>   		disable_irq(ttcce->ce.irq);
> +
>   		clockevents_update_freq(&ttcce->ce,
>   				ndata->new_rate / PRESCALE);
> -		enable_irq(ttcce->ce.irq);
>
> -		/* update cached frequency */
> -		ttc->freq = ndata->new_rate;
> +		if (ttcce->ce.mode == CLOCK_EVT_MODE_PERIODIC)
> +			ttc_set_interval(ttc, DIV_ROUND_CLOSEST(ttc->freq,
> +						PRESCALE * HZ));
> +
> +		enable_irq(ttcce->ce.irq);
>
>   		/* fall through */
>   	}

When looking at the smp_twd.c, there is a similar need.

There are a couple of notifier used on for cpufreq and one for clock 
rate change.

Wouldn't make sense to add the notifier calls and registration in the 
time framework and add the callbacks in the dev ops ? The switch would 
be in the timer framework and perhaps the irq disabling may be easier to 
deal with ?

-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/9] clocksource/cadence_ttc: Adjust interval in clock notifier

[Sören Brinkmann]

Hi Daniel,

On Thu, Dec 12, 2013 at 01:15:38PM +0100, Daniel Lezcano wrote:
> On 11/27/2013 02:04 AM, Soren Brinkmann wrote:
> >The clockevent has to be reprogrammed if the timer's input
> >clock frequency changes and the timer is in periodic mode, in order to
> >maintain the correct timer interval.
> >
> >Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> >---
> >v2:
> >  - adjust the timer interval while the timer interrupt is disabled
> >---
> >  drivers/clocksource/cadence_ttc_timer.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> >
> >diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c
> >index 246d018d1e63..77517675653e 100644
> >--- a/drivers/clocksource/cadence_ttc_timer.c
> >+++ b/drivers/clocksource/cadence_ttc_timer.c
> >@@ -322,6 +322,9 @@ static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
> >  	switch (event) {
> >  	case POST_RATE_CHANGE:
> >  	{
> >+		/* update cached frequency */
> >+		ttc->freq = ndata->new_rate;
> >+
> >  		/*
> >  		 * clockevents_update_freq should be called with IRQ disabled on
> >  		 * the CPU the timer provides events for. The timer we use is
> >@@ -329,12 +332,15 @@ static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
> >  		 * cores.
> >  		 */
> >  		disable_irq(ttcce->ce.irq);
> >+
> >  		clockevents_update_freq(&ttcce->ce,
> >  				ndata->new_rate / PRESCALE);
> >-		enable_irq(ttcce->ce.irq);
> >
> >-		/* update cached frequency */
> >-		ttc->freq = ndata->new_rate;
> >+		if (ttcce->ce.mode == CLOCK_EVT_MODE_PERIODIC)
> >+			ttc_set_interval(ttc, DIV_ROUND_CLOSEST(ttc->freq,
> >+						PRESCALE * HZ));
> >+
> >+		enable_irq(ttcce->ce.irq);
> >
> >  		/* fall through */
> >  	}
> 
> When looking at the smp_twd.c, there is a similar need.
Right, but with my proposed patches a call to clockevents_update_freq()
should be all that is needed.

> 
> There are a couple of notifier used on for cpufreq and one for clock
> rate change.
Only one is actually used I think, depending on what kernel
infrastructure is available.

> 
> Wouldn't make sense to add the notifier calls and registration in
> the time framework and add the callbacks in the dev ops ? The switch
> would be in the timer framework and perhaps the irq disabling may be
> easier to deal with ?
I don't think that works - at least not that easy -  because timers could
have all different input clocks. The twd is kind of special since it always
uses the CPU clock.  And - unfortunately - our TTC implementation on Zynq
does it similar. But that is not the generic case and I don't know if it
makes sense/is possible to handle this in the timer core. It would mean the
timer core would need to keep track of the timers + their input clocks.
I was hoping that Zynq, twd and arm_global_timer are rather the
exception and that the majority of timer HW is provided a fixed and stable
input frequency - as the arm generic timer spec demands.

	Sören

[PATCH v2 6/9] clocksource/cadence_ttc: Overhaul clocksource frequency adjustment

[Soren Brinkmann]

The currently used method adjusting the clocksource to a changing input
frequency does not work on kernels from 3.11 on.
The new approach is to keep the timer frequency as constant as possible.
I.e.
 - due to the TTC's prescaler limitations, allow frequency changes
   only if the frequency scales by a power of 2
 - adjust the counter's divider on the fly when a frequency change
   occurs

This limits cpufreq to scale by certain factors only.
But we may keep the time base somewhat constant, so that sleep() & co
keep working as expected, while supporting cpufreq.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
v2:
 - remove PM notifier and associated code
---
 drivers/clocksource/cadence_ttc_timer.c | 112 ++++++++++++++++++++++++++------
 1 file changed, 92 insertions(+), 20 deletions(-)

diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c
index 77517675653e..b0cd7368b466 100644
--- a/drivers/clocksource/cadence_ttc_timer.c
+++ b/drivers/clocksource/cadence_ttc_timer.c
@@ -16,6 +16,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/interrupt.h>
 #include <linux/clockchips.h>
 #include <linux/of_address.h>
@@ -52,6 +53,8 @@
 #define TTC_CNT_CNTRL_DISABLE_MASK	0x1
 
 #define TTC_CLK_CNTRL_CSRC_MASK		(1 << 5)	/* clock source */
+#define TTC_CLK_CNTRL_PSV_MASK		0x1e
+#define TTC_CLK_CNTRL_PSV_SHIFT		1
 
 /*
  * Setup the timers to use pre-scaling, using a fixed value for now that will
@@ -63,6 +66,8 @@
 #define CLK_CNTRL_PRESCALE_EN	1
 #define CNT_CNTRL_RESET		(1 << 4)
 
+#define MAX_F_ERR 50
+
 /**
  * struct ttc_timer - This definition defines local timer structure
  *
@@ -82,6 +87,9 @@ struct ttc_timer {
 		container_of(x, struct ttc_timer, clk_rate_change_nb)
 
 struct ttc_timer_clocksource {
+	int			scale_dir;
+	u32			scale_clk_ctrl_reg_old;
+	u32			scale_clk_ctrl_reg_new;
 	struct ttc_timer	ttc;
 	struct clocksource	cs;
 };
@@ -229,32 +237,96 @@ static int ttc_rate_change_clocksource_cb(struct notifier_block *nb,
 			struct ttc_timer_clocksource, ttc);
 
 	switch (event) {
-	case POST_RATE_CHANGE:
+	case PRE_RATE_CHANGE:
+	{
+		u32 psv;
+		unsigned long factor, rate_low, rate_high;
+
+		if (ndata->new_rate == ndata->old_rate) {
+			ttccs->scale_dir = 0;
+			return NOTIFY_OK;
+		}
+
+		if (ndata->new_rate > ndata->old_rate) {
+			factor = DIV_ROUND_CLOSEST(ndata->new_rate,
+					ndata->old_rate);
+			ttccs->scale_dir = 1;
+			rate_low = ndata->old_rate;
+			rate_high = ndata->new_rate;
+		} else {
+			factor = DIV_ROUND_CLOSEST(ndata->old_rate,
+					ndata->new_rate);
+			ttccs->scale_dir = -1;
+			rate_low = ndata->new_rate;
+			rate_high = ndata->old_rate;
+		}
+
+		if (!is_power_of_2(factor))
+				return NOTIFY_BAD;
+
+		if (abs(rate_high - (factor * rate_low)) > MAX_F_ERR)
+			return NOTIFY_BAD;
+
+		factor = __ilog2_u32(factor);
+
 		/*
-		 * Do whatever is necessary to maintain a proper time base
-		 *
-		 * I cannot find a way to adjust the currently used clocksource
-		 * to the new frequency. __clocksource_updatefreq_hz() sounds
-		 * good, but does not work. Not sure what's that missing.
-		 *
-		 * This approach works, but triggers two clocksource switches.
-		 * The first after unregister to clocksource jiffies. And
-		 * another one after the register to the newly registered timer.
-		 *
-		 * Alternatively we could 'waste' another HW timer to ping pong
-		 * between clock sources. That would also use one register and
-		 * one unregister call, but only trigger one clocksource switch
-		 * for the cost of another HW timer used by the OS.
+		 * store timer clock ctrl register so we can restore it in case
+		 * of an abort.
 		 */
-		clocksource_unregister(&ttccs->cs);
-		clocksource_register_hz(&ttccs->cs,
-				ndata->new_rate / PRESCALE);
-		/* fall through */
-	case PRE_RATE_CHANGE:
+		ttccs->scale_clk_ctrl_reg_old =
+			__raw_readl(ttccs->ttc.base_addr +
+					TTC_CLK_CNTRL_OFFSET);
+
+		psv = (ttccs->scale_clk_ctrl_reg_old &
+				TTC_CLK_CNTRL_PSV_MASK) >>
+				TTC_CLK_CNTRL_PSV_SHIFT;
+		if (ttccs->scale_dir < 0)
+			psv -= factor;
+		else
+			psv += factor;
+
+		/* prescaler within legal range? */
+		if (psv & ~(TTC_CLK_CNTRL_PSV_MASK >> TTC_CLK_CNTRL_PSV_SHIFT))
+			return NOTIFY_BAD;
+
+		ttccs->scale_clk_ctrl_reg_new = ttccs->scale_clk_ctrl_reg_old &
+			~TTC_CLK_CNTRL_PSV_MASK;
+		ttccs->scale_clk_ctrl_reg_new |= psv << TTC_CLK_CNTRL_PSV_SHIFT;
+
+
+		/* scale down: adjust divider in post-change notification */
+		if (ttccs->scale_dir < 0)
+			return NOTIFY_DONE;
+
+		/* scale up: adjust divider now - before frequency change */
+		__raw_writel(ttccs->scale_clk_ctrl_reg_new,
+				ttccs->ttc.base_addr + TTC_CLK_CNTRL_OFFSET);
+		break;
+	}
+	case POST_RATE_CHANGE:
+		/* scale up: pre-change notification did the adjustment */
+		if (ttccs->scale_dir >= 0)
+			return NOTIFY_OK;
+
+		/* scale down: adjust divider now - after frequency change */
+		__raw_writel(ttccs->scale_clk_ctrl_reg_new,
+				ttccs->ttc.base_addr + TTC_CLK_CNTRL_OFFSET);
+		break;
+
 	case ABORT_RATE_CHANGE:
+		/* we have to undo the adjustment in case we scale up */
+		if (ttccs->scale_dir <= 0)
+			return NOTIFY_OK;
+
+		/* restore original register value */
+		__raw_writel(ttccs->scale_clk_ctrl_reg_old,
+				ttccs->ttc.base_addr + TTC_CLK_CNTRL_OFFSET);
+		/* fall through */
 	default:
 		return NOTIFY_DONE;
 	}
+
+	return NOTIFY_DONE;
 }
 
 static void __init ttc_setup_clocksource(struct clk *clk, void __iomem *base)
-- 
1.8.4.4

[PATCH v2 7/9] clocksource/cadence_ttc: Use only one counter

[Soren Brinkmann]

Currently the driver uses two of the three counters the TTC provides to
implement a clocksource and a clockevent device. By using the TTC's
match feature we can implement both use cases using a single counter
only.
The old approach is to use timer over-/underflow to generate an
interrupt. Using the match register allows to generate an interrupt on
arbitrary counter values. This way a dedicated clockevent counter can be
avoided.

Signed-off-by: Soren Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
---
 drivers/clocksource/cadence_ttc_timer.c | 92 +++++++++++----------------------
 1 file changed, 31 insertions(+), 61 deletions(-)

diff --git a/drivers/clocksource/cadence_ttc_timer.c b/drivers/clocksource/cadence_ttc_timer.c
index b0cd7368b466..b00656371350 100644
--- a/drivers/clocksource/cadence_ttc_timer.c
+++ b/drivers/clocksource/cadence_ttc_timer.c
@@ -47,6 +47,7 @@
 #define TTC_CNT_CNTRL_OFFSET		0x0C /* Counter Control Reg, RW */
 #define TTC_COUNT_VAL_OFFSET		0x18 /* Counter Value Reg, RO */
 #define TTC_INTR_VAL_OFFSET		0x24 /* Interval Count Reg, RW */
+#define TTC_MATCH1_OFFSET	0x30 /* Match reg, RW */
 #define TTC_ISR_OFFSET		0x54 /* Interrupt Status Reg, RO */
 #define TTC_IER_OFFSET		0x60 /* Interrupt Enable Reg, RW */
 
@@ -64,7 +65,10 @@
 #define PRESCALE		2048	/* The exponent must match this */
 #define CLK_CNTRL_PRESCALE	((PRESCALE_EXPONENT - 1) << 1)
 #define CLK_CNTRL_PRESCALE_EN	1
-#define CNT_CNTRL_RESET		(1 << 4)
+#define CNT_CNTRL_RESET		BIT(4)
+#define CNT_CNTRL_MATCH		BIT(3)
+
+#define TTC_INTERRUPT_MATCH1	BIT(1)
 
 #define MAX_F_ERR 50
 
@@ -100,6 +104,7 @@ struct ttc_timer_clocksource {
 struct ttc_timer_clockevent {
 	struct ttc_timer		ttc;
 	struct clock_event_device	ce;
+	unsigned long			interval;
 };
 
 #define to_ttc_timer_clkevent(x) \
@@ -113,25 +118,20 @@ static void __iomem *ttc_sched_clock_val_reg;
  * @timer:	Pointer to the timer instance
  * @cycles:	Timer interval ticks
  **/
-static void ttc_set_interval(struct ttc_timer *timer,
-					unsigned long cycles)
+static void ttc_set_interval(struct ttc_timer *timer, unsigned long cycles)
 {
-	u32 ctrl_reg;
+	struct ttc_timer_clockevent *ttcce = container_of(timer,
+			struct ttc_timer_clockevent, ttc);
 
-	/* Disable the counter, set the counter value  and re-enable counter */
-	ctrl_reg = __raw_readl(timer->base_addr + TTC_CNT_CNTRL_OFFSET);
-	ctrl_reg |= TTC_CNT_CNTRL_DISABLE_MASK;
-	__raw_writel(ctrl_reg, timer->base_addr + TTC_CNT_CNTRL_OFFSET);
+	/* set interval */
+	u32 reg = __raw_readl(timer->base_addr + TTC_COUNT_VAL_OFFSET);
+	reg += cycles;
+	__raw_writel(reg, timer->base_addr + TTC_MATCH1_OFFSET);
 
-	__raw_writel(cycles, timer->base_addr + TTC_INTR_VAL_OFFSET);
+	/* enable match interrupt */
+	__raw_writel(TTC_INTERRUPT_MATCH1, timer->base_addr + TTC_IER_OFFSET);
 
-	/*
-	 * Reset the counter (0x10) so that it starts from 0, one-shot
-	 * mode makes this needed for timing to be right.
-	 */
-	ctrl_reg |= CNT_CNTRL_RESET;
-	ctrl_reg &= ~TTC_CNT_CNTRL_DISABLE_MASK;
-	__raw_writel(ctrl_reg, timer->base_addr + TTC_CNT_CNTRL_OFFSET);
+	ttcce->interval = cycles;
 }
 
 /**
@@ -149,6 +149,8 @@ static irqreturn_t ttc_clock_event_interrupt(int irq, void *dev_id)
 
 	/* Acknowledge the interrupt and call event handler */
 	__raw_readl(timer->base_addr + TTC_ISR_OFFSET);
+	if (ttce->ce.mode == CLOCK_EVT_MODE_PERIODIC)
+		ttc_set_interval(timer, ttce->interval);
 
 	ttce->ce.event_handler(&ttce->ce);
 
@@ -202,7 +204,6 @@ static void ttc_set_mode(enum clock_event_mode mode,
 {
 	struct ttc_timer_clockevent *ttce = to_ttc_timer_clkevent(evt);
 	struct ttc_timer *timer = &ttce->ttc;
-	u32 ctrl_reg;
 
 	switch (mode) {
 	case CLOCK_EVT_MODE_PERIODIC:
@@ -212,18 +213,9 @@ static void ttc_set_mode(enum clock_event_mode mode,
 	case CLOCK_EVT_MODE_ONESHOT:
 	case CLOCK_EVT_MODE_UNUSED:
 	case CLOCK_EVT_MODE_SHUTDOWN:
-		ctrl_reg = __raw_readl(timer->base_addr +
-					TTC_CNT_CNTRL_OFFSET);
-		ctrl_reg |= TTC_CNT_CNTRL_DISABLE_MASK;
-		__raw_writel(ctrl_reg,
-				timer->base_addr + TTC_CNT_CNTRL_OFFSET);
+		__raw_writel(0, timer->base_addr + TTC_IER_OFFSET);
 		break;
 	case CLOCK_EVT_MODE_RESUME:
-		ctrl_reg = __raw_readl(timer->base_addr +
-					TTC_CNT_CNTRL_OFFSET);
-		ctrl_reg &= ~TTC_CNT_CNTRL_DISABLE_MASK;
-		__raw_writel(ctrl_reg,
-				timer->base_addr + TTC_CNT_CNTRL_OFFSET);
 		break;
 	}
 }
@@ -362,17 +354,6 @@ static void __init ttc_setup_clocksource(struct clk *clk, void __iomem *base)
 	ttccs->cs.mask = CLOCKSOURCE_MASK(16);
 	ttccs->cs.flags = CLOCK_SOURCE_IS_CONTINUOUS;
 
-	/*
-	 * Setup the clock source counter to be an incrementing counter
-	 * with no interrupt and it rolls over at 0xFFFF. Pre-scale
-	 * it by 32 also. Let it start running now.
-	 */
-	__raw_writel(0x0,  ttccs->ttc.base_addr + TTC_IER_OFFSET);
-	__raw_writel(CLK_CNTRL_PRESCALE | CLK_CNTRL_PRESCALE_EN,
-		     ttccs->ttc.base_addr + TTC_CLK_CNTRL_OFFSET);
-	__raw_writel(CNT_CNTRL_RESET,
-		     ttccs->ttc.base_addr + TTC_CNT_CNTRL_OFFSET);
-
 	err = clocksource_register_hz(&ttccs->cs, ttccs->ttc.freq / PRESCALE);
 	if (WARN_ON(err)) {
 		kfree(ttccs);
@@ -458,16 +439,6 @@ static void __init ttc_setup_clockevent(struct clk *clk,
 	ttcce->ce.irq = irq;
 	ttcce->ce.cpumask = cpu_possible_mask;
 
-	/*
-	 * Setup the clock event timer to be an interval timer which
-	 * is prescaled by 32 using the interval interrupt. Leave it
-	 * disabled for now.
-	 */
-	__raw_writel(0x23, ttcce->ttc.base_addr + TTC_CNT_CNTRL_OFFSET);
-	__raw_writel(CLK_CNTRL_PRESCALE | CLK_CNTRL_PRESCALE_EN,
-		     ttcce->ttc.base_addr + TTC_CLK_CNTRL_OFFSET);
-	__raw_writel(0x1,  ttcce->ttc.base_addr + TTC_IER_OFFSET);
-
 	err = request_irq(irq, ttc_clock_event_interrupt,
 			  IRQF_DISABLED | IRQF_TIMER,
 			  ttcce->ce.name, ttcce);
@@ -490,7 +461,7 @@ static void __init ttc_timer_init(struct device_node *timer)
 {
 	unsigned int irq;
 	void __iomem *timer_baseaddr;
-	struct clk *clk_cs, *clk_ce;
+	struct clk *clk;
 	static int initialized;
 	int clksel;
 
@@ -510,7 +481,7 @@ static void __init ttc_timer_init(struct device_node *timer)
 		BUG();
 	}
 
-	irq = irq_of_parse_and_map(timer, 1);
+	irq = irq_of_parse_and_map(timer, 0);
 	if (irq <= 0) {
 		pr_err("ERROR: invalid interrupt number\n");
 		BUG();
@@ -518,22 +489,21 @@ static void __init ttc_timer_init(struct device_node *timer)
 
 	clksel = __raw_readl(timer_baseaddr + TTC_CLK_CNTRL_OFFSET);
 	clksel = !!(clksel & TTC_CLK_CNTRL_CSRC_MASK);
-	clk_cs = of_clk_get(timer, clksel);
-	if (IS_ERR(clk_cs)) {
+	clk = of_clk_get(timer, clksel);
+	if (IS_ERR(clk)) {
 		pr_err("ERROR: timer input clock not found\n");
 		BUG();
 	}
 
-	clksel = __raw_readl(timer_baseaddr + 4 + TTC_CLK_CNTRL_OFFSET);
-	clksel = !!(clksel & TTC_CLK_CNTRL_CSRC_MASK);
-	clk_ce = of_clk_get(timer, clksel);
-	if (IS_ERR(clk_ce)) {
-		pr_err("ERROR: timer input clock not found\n");
-		BUG();
-	}
+	__raw_writel(CLK_CNTRL_PRESCALE | CLK_CNTRL_PRESCALE_EN,
+			timer_baseaddr + TTC_CLK_CNTRL_OFFSET);
+
+	/* start timer in overflow and match mode */
+	__raw_writel(CNT_CNTRL_RESET | CNT_CNTRL_MATCH,
+			timer_baseaddr + TTC_CNT_CNTRL_OFFSET);
 
-	ttc_setup_clocksource(clk_cs, timer_baseaddr);
-	ttc_setup_clockevent(clk_ce, timer_baseaddr + 4, irq);
+	ttc_setup_clocksource(clk, timer_baseaddr);
+	ttc_setup_clockevent(clk, timer_baseaddr, irq);
 
 	pr_info("%s #0 at %p, irq=%d\n", timer->name, timer_baseaddr, irq);
 }
-- 
1.8.4.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 8/9] arm: zynq: Don't use arm_global_timer with cpufreq

[Soren Brinkmann]

The timer frequency of the arm_global_timer depends on the CPU
frequency. With cpufreq altering that frequency the arm_global_timer
does not maintain a stable time base. Therefore don't enable that timer
in case cpufreq is enabled.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 arch/arm/mach-zynq/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-zynq/Kconfig b/arch/arm/mach-zynq/Kconfig
index 6b04260aa142..f84fab14f0b7 100644
--- a/arch/arm/mach-zynq/Kconfig
+++ b/arch/arm/mach-zynq/Kconfig
@@ -13,6 +13,6 @@ config ARCH_ZYNQ
 	select HAVE_SMP
 	select SPARSE_IRQ
 	select CADENCE_TTC_TIMER
-	select ARM_GLOBAL_TIMER
+	select ARM_GLOBAL_TIMER if !CPU_FREQ
 	help
 	  Support for Xilinx Zynq ARM Cortex A9 Platform
-- 
1.8.4.4

[PATCH v2 9/9] arm: zynq: Add support for cpufreq

[Soren Brinkmann]

The generic cpufreq-cpu0 driver can scale the CPU frequency on Zynq
SOCs. Add the required platform device to the BSP and appropriate
OPPs to the dts.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 arch/arm/boot/dts/zynq-7000.dtsi | 6 ++++++
 arch/arm/mach-zynq/Kconfig       | 2 ++
 arch/arm/mach-zynq/common.c      | 3 +++
 3 files changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
index 23ba82c205a2..dc030481ee64 100644
--- a/arch/arm/boot/dts/zynq-7000.dtsi
+++ b/arch/arm/boot/dts/zynq-7000.dtsi
@@ -24,6 +24,12 @@
 			device_type = "cpu";
 			reg = <0>;
 			clocks = <&clkc 3>;
+			operating-points = <
+				/* kHz    uV */
+				666667  1000000
+				333334  1000000
+				111112  1000000
+			>;
 		};
 
 		cpu@1 {
diff --git a/arch/arm/mach-zynq/Kconfig b/arch/arm/mach-zynq/Kconfig
index f84fab14f0b7..f03e75bd0b2b 100644
--- a/arch/arm/mach-zynq/Kconfig
+++ b/arch/arm/mach-zynq/Kconfig
@@ -2,6 +2,8 @@ config ARCH_ZYNQ
 	bool "Xilinx Zynq ARM Cortex A9 Platform" if ARCH_MULTI_V7
 	select ARM_AMBA
 	select ARM_GIC
+	select ARCH_HAS_CPUFREQ
+	select ARCH_HAS_OPP
 	select COMMON_CLK
 	select CPU_V7
 	select GENERIC_CLOCKEVENTS
diff --git a/arch/arm/mach-zynq/common.c b/arch/arm/mach-zynq/common.c
index 9a7bd137c8fd..eeff33a1d14a 100644
--- a/arch/arm/mach-zynq/common.c
+++ b/arch/arm/mach-zynq/common.c
@@ -54,6 +54,8 @@ static struct platform_device zynq_cpuidle_device = {
  */
 static void __init zynq_init_machine(void)
 {
+	struct platform_device_info devinfo = { .name = "cpufreq-cpu0", };
+
 	/*
 	 * 64KB way size, 8-way associativity, parity disabled
 	 */
@@ -62,6 +64,7 @@ static void __init zynq_init_machine(void)
 	of_platform_bus_probe(NULL, zynq_of_bus_ids, NULL);
 
 	platform_device_register(&zynq_cpuidle_device);
+	platform_device_register_full(&devinfo);
 }
 
 static void __init zynq_timer_init(void)
-- 
1.8.4.4