Re: [PATCH v5] clocksource:arm_global_timer: Add ARM global timer support.
Thankyou for the comments.
On 24/06/13 23:08, Stephen Boyd wrote:
On 06/24/13 08:53, Srinivas KANDAGATLA wrote:
+#include linux/clkdev.h
Why do you need this include?
+#include asm/mach/irq.h
And this one?
Removed them.
+static u64 gt_counter_read(void)
+{
+u64 counter;
+u32 lower;
+u32 upper, old_upper;
+
+upper = readl_relaxed(gt_base + GT_COUNTER1);
+do {
+old_upper = upper;
+lower = readl_relaxed(gt_base + GT_COUNTER0);
+upper = readl_relaxed(gt_base + GT_COUNTER1);
[snip]
+static void gt_compare_set(unsigned long delta, int periodic)
+{
+u64 counter = gt_counter_read();
+unsigned long ctrl = readl(gt_base + GT_CONTROL);
+
+counter += delta;
+ctrl = ~(GT_CONTROL_COMP_ENABLE | GT_CONTROL_IRQ_ENABLE);
+
+writel(ctrl, gt_base + GT_CONTROL);
+writel(lower_32_bits(counter), gt_base + GT_COMP0);
+writel(upper_32_bits(counter), gt_base + GT_COMP1);
+
+if (periodic) {
+writel(delta, gt_base + GT_AUTO_INC);
+ctrl |= GT_CONTROL_AUTO_INC;
+}
+
+ctrl |= GT_CONTROL_COMP_ENABLE | GT_CONTROL_IRQ_ENABLE;
+writel(ctrl, gt_base + GT_CONTROL);
+}
Why is there a mix of the relaxed and non-relaxed io accessors?
gt_counter_read will be used very frequently, so using relaxed can
reduce latencies involved in memory barriers.
+#ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
+static u32 gt_sched_clock_read(void)
notrace
Ok, fixed it.
+{
+if (!gt_base)
+return 0;
Seems impossible? Remove this check?
Yep, I agree this case is impossible in the code flow.
Thanks,
srini
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v5] clocksource:arm_global_timer: Add ARM global timer support.
Thankyou for the comments, On 24/06/13 23:00, Stephen Boyd wrote: On 06/24/13 14:08, Srinivas KANDAGATLA wrote: On 24/06/13 21:06, Stephen Boyd wrote: On 06/24/13 08:53, Srinivas KANDAGATLA wrote: I think the problem is your clockevent has no rating. Please give it a rating (300?) so that it prevents the dummy from taking over. You don't need to worry about disabling the local timer API, it will register a harmless clockevent with a low rating (100) that should be ignored. I think I forgot to add rating in my last attempt to use cpu notifier. With rating set to 300, the driver works fine with cpu notifiers. Will send a new patch. Thanks, srini ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
[PATCH v5] clocksource:arm_global_timer: Add ARM global timer support.
From: Stuart Menefy stuart.men...@st.com
This is a simple driver for the global timer module found in the Cortex
A9-MP cores from revision r1p0 onwards. This should be able to perform
the functions of the system timer and the local timer in an SMP system.
The global timer has the following features:
The global timer is a 64-bit incrementing counter with an
auto-incrementing feature. It continues incrementing after sending
interrupts. The global timer is memory mapped in the private memory
region.
The global timer is accessible to all Cortex-A9 processors in the
cluster. Each Cortex-A9 processor has a private 64-bit comparator that
is used to assert a private interrupt when the global timer has reached
the comparator value. All the Cortex-A9 processors in a design use the
banked ID, ID27, for this interrupt. ID27 is sent to the Interrupt
Controller as a Private Peripheral Interrupt. The global timer is
clocked by PERIPHCLK.
Signed-off-by: Stuart Menefy stuart.men...@st.com
Signed-off-by: Srinivas Kandagatla srinivas.kandaga...@st.com
CC: Arnd Bergmann a...@arndb.de
CC: Rob Herring robherri...@gmail.com
CC: Linus Walleij linus.wall...@linaro.org
CC: Will Deacon will.dea...@arm.com
CC: Thomas Gleixner t...@linutronix.de
---
Thankyou for reveiwing the v4 patch.
This patch is split out of the orignal 10 patches submitted for Sti SOC
support to arm-kernel mailing list. This patch has undergone 3 cycles of
reviews in arm-kernel mailing list.
Arnd already picked up SOC support patches [4-10] and merged them via arm-soc
tree for 3.11.
If its not too late can this patch be considered for 3.11 via clocksource tree?
This patch has no build dependencies.
Thanks,
srini
Changes since v4:
All the comments are from Thomas G.
- disabled irq and comp enable bits while setting one-shot mode.
- remove double pointer usage of clock_event_device structure.
- remove spaces from device name.
- remove few un-necessary comments.
- Fix error checks and error case handling in global_timer_of_register
Changes since v3:
- Arnd suggested to replaced all __raw_readl/__raw_writel with
readl/writel or readl_relaxed/writel_relaxed(for optimized path)
as __raw* apis are not Endian safe.
Changes since v2:
- cleaned up arm-global-timer code for non-dt as suggested by Linus W
and
- fixed minmum clock tick setting pointed by Linus W.
Changes since RFC:
Most of the comments are suggested by Linus W.
- moved to drivers/clocksource.
- added revision check in driver.
- removed unused header file.
- moved to u64 from union gt_counter
- comments added in get_counter
- removed leftover debug code.
- moved code to use __raw_readl/writel.
- used DIV_ROUND_CLOSEST
- added check in interrupt handler.
- expanded CE and CS acronyms usage.
- Fixed minimum clock ticks value.
- move to use clocksource_register_hz
- added arch sched_clock support.
- added ERRATA 740657 workaround.
.../devicetree/bindings/arm/global_timer.txt | 21 ++
drivers/clocksource/Kconfig| 13 +
drivers/clocksource/Makefile |1 +
drivers/clocksource/arm_global_timer.c | 320
4 files changed, 355 insertions(+), 0 deletions(-)
create mode 100644 Documentation/devicetree/bindings/arm/global_timer.txt
create mode 100644 drivers/clocksource/arm_global_timer.c
diff --git a/Documentation/devicetree/bindings/arm/global_timer.txt
b/Documentation/devicetree/bindings/arm/global_timer.txt
new file mode 100644
index 000..b64abac
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/global_timer.txt
@@ -0,0 +1,21 @@
+
+* ARM Global Timer
+ Cortex-A9 are often associated with a per-core Global timer.
+
+** Timer node required properties:
+
+- compatible : Should be arm,cortex-a9-global-timer
+ Driver supports versions r2p0 and above.
+
+- interrupts : One interrupt to each core
+
+- reg : Specify the base address and the size of the GT timer
+ register window.
+
+Example:
+
+ timer@2c000600 {
+ compatible = arm,cortex-a9-global-timer;
+ reg = 0x2c000600 0x20;
+ interrupts = 1 13 0xf01;
+ };
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 5871933..33e4453 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -69,6 +69,19 @@ config ARM_ARCH_TIMER
bool
select CLKSRC_OF if OF
+config ARM_GLOBAL_TIMER
+ bool
+ select CLKSRC_OF if OF
+ help
+ This options enables support for the ARM global timer unit
+
+config CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
+ bool
+ depends on ARM_GLOBAL_TIMER
+ default y
+ help
+Use ARM global timer clock source as sched_clock
+
config
Re: [PATCH v5] clocksource:arm_global_timer: Add ARM global timer support.
On Mon, 24 Jun 2013, Srinivas KANDAGATLA wrote: From: Stuart Menefy stuart.men...@st.com This is a simple driver for the global timer module found in the Cortex A9-MP cores from revision r1p0 onwards. This should be able to perform the functions of the system timer and the local timer in an SMP system. The global timer has the following features: The global timer is a 64-bit incrementing counter with an auto-incrementing feature. It continues incrementing after sending interrupts. The global timer is memory mapped in the private memory region. The global timer is accessible to all Cortex-A9 processors in the cluster. Each Cortex-A9 processor has a private 64-bit comparator that is used to assert a private interrupt when the global timer has reached the comparator value. All the Cortex-A9 processors in a design use the banked ID, ID27, for this interrupt. ID27 is sent to the Interrupt Controller as a Private Peripheral Interrupt. The global timer is clocked by PERIPHCLK. Signed-off-by: Stuart Menefy stuart.men...@st.com Signed-off-by: Srinivas Kandagatla srinivas.kandaga...@st.com CC: Arnd Bergmann a...@arndb.de CC: Rob Herring robherri...@gmail.com CC: Linus Walleij linus.wall...@linaro.org CC: Will Deacon will.dea...@arm.com CC: Thomas Gleixner t...@linutronix.de Reviewed-by: Thomas Gleixner t...@linutronix.de ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v5] clocksource:arm_global_timer: Add ARM global timer support.
On 06/24/13 08:53, Srinivas KANDAGATLA wrote:
+
+static void gt_clockevents_stop(struct clock_event_device *clk)
+{
+ gt_clockevent_set_mode(CLOCK_EVT_MODE_UNUSED, clk);
+ disable_percpu_irq(clk-irq);
+}
+
+static int __cpuinit gt_clockevents_setup(struct clock_event_device *clk)
+{
+ struct clock_event_device *evt = this_cpu_ptr(gt_evt);
+ return evt-name ? 0 : gt_clockevents_init(evt);
+}
How does this work? gt_clockevents_stop() is using the
clock_event_device struct from the ARM local timer layer whereas
gt_clockevents_setup() is using a driver private allocation. Please just
don't use the local timer API at all and use cpu notifiers instead.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v5] clocksource:arm_global_timer: Add ARM global timer support.
On 24/06/13 21:06, Stephen Boyd wrote:
On 06/24/13 08:53, Srinivas KANDAGATLA wrote:
+
+static void gt_clockevents_stop(struct clock_event_device *clk)
+{
+gt_clockevent_set_mode(CLOCK_EVT_MODE_UNUSED, clk);
+disable_percpu_irq(clk-irq);
+}
+
+static int __cpuinit gt_clockevents_setup(struct clock_event_device *clk)
+{
+struct clock_event_device *evt = this_cpu_ptr(gt_evt);
+return evt-name ? 0 : gt_clockevents_init(evt);
+}
How does this work? gt_clockevents_stop() is using the
clock_event_device struct from the ARM local timer layer whereas
gt_clockevents_setup() is using a driver private allocation.
Thanks for pointing this..
This should fix it.
static void gt_clockevents_stop(struct clock_event_device *clk)
{
struct clock_event_device *evt = this_cpu_ptr(gt_evt);
gt_clockevent_set_mode(CLOCK_EVT_MODE_UNUSED, evt);
disable_percpu_irq(evt-irq);
}
Please just
don't use the local timer API at all and use cpu notifiers instead.
Last time when I did try using cpu notifiers like arm_arch_timer, the
broadcast dummy timer did kick off and took over the local timer on the
secondary cpus. Resulting in lot of broadcast IPI's.
If I use cpu notifiers I will end up two clk events on a each core (one
dummy from arm/kernel/smp.c and other gt clk_evt). I think I can only
use cpu notifiers in my case once your patches are in.
Also I cant disable LOCAL_TIMERS as it y by default.
Am I missing something?
Am happy to move to cpu notifiers if it works, else the driver will be
broken.
Thanks,
srini
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v5] clocksource:arm_global_timer: Add ARM global timer support.
On 24/06/13 21:01, Thomas Gleixner wrote: On Mon, 24 Jun 2013, Srinivas KANDAGATLA wrote: From: Stuart Menefy stuart.men...@st.com This is a simple driver for the global timer module found in the Cortex A9-MP cores from revision r1p0 onwards. This should be able to perform the functions of the system timer and the local timer in an SMP system. The global timer has the following features: The global timer is a 64-bit incrementing counter with an auto-incrementing feature. It continues incrementing after sending interrupts. The global timer is memory mapped in the private memory region. The global timer is accessible to all Cortex-A9 processors in the cluster. Each Cortex-A9 processor has a private 64-bit comparator that is used to assert a private interrupt when the global timer has reached the comparator value. All the Cortex-A9 processors in a design use the banked ID, ID27, for this interrupt. ID27 is sent to the Interrupt Controller as a Private Peripheral Interrupt. The global timer is clocked by PERIPHCLK. Signed-off-by: Stuart Menefy stuart.men...@st.com Signed-off-by: Srinivas Kandagatla srinivas.kandaga...@st.com CC: Arnd Bergmann a...@arndb.de CC: Rob Herring robherri...@gmail.com CC: Linus Walleij linus.wall...@linaro.org CC: Will Deacon will.dea...@arm.com CC: Thomas Gleixner t...@linutronix.de Reviewed-by: Thomas Gleixner t...@linutronix.de Thanks Thomas, I will fix Stephen's comment in next spin. Thanks, srini ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v5] clocksource:arm_global_timer: Add ARM global timer support.
On 06/24/13 14:08, Srinivas KANDAGATLA wrote:
On 24/06/13 21:06, Stephen Boyd wrote:
On 06/24/13 08:53, Srinivas KANDAGATLA wrote:
+
+static void gt_clockevents_stop(struct clock_event_device *clk)
+{
+ gt_clockevent_set_mode(CLOCK_EVT_MODE_UNUSED, clk);
+ disable_percpu_irq(clk-irq);
+}
+
+static int __cpuinit gt_clockevents_setup(struct clock_event_device *clk)
+{
+ struct clock_event_device *evt = this_cpu_ptr(gt_evt);
+ return evt-name ? 0 : gt_clockevents_init(evt);
+}
How does this work? gt_clockevents_stop() is using the
clock_event_device struct from the ARM local timer layer whereas
gt_clockevents_setup() is using a driver private allocation.
Thanks for pointing this..
This should fix it.
static void gt_clockevents_stop(struct clock_event_device *clk)
{
struct clock_event_device *evt = this_cpu_ptr(gt_evt);
gt_clockevent_set_mode(CLOCK_EVT_MODE_UNUSED, evt);
disable_percpu_irq(evt-irq);
}
Looks good, but even better would be to not use the local timer API.
Please just
don't use the local timer API at all and use cpu notifiers instead.
Last time when I did try using cpu notifiers like arm_arch_timer, the
broadcast dummy timer did kick off and took over the local timer on the
secondary cpus. Resulting in lot of broadcast IPI's.
If I use cpu notifiers I will end up two clk events on a each core (one
dummy from arm/kernel/smp.c and other gt clk_evt). I think I can only
use cpu notifiers in my case once your patches are in.
Also I cant disable LOCAL_TIMERS as it y by default.
Am I missing something?
Am happy to move to cpu notifiers if it works, else the driver will be
broken.
I think the problem is your clockevent has no rating. Please give it a
rating (300?) so that it prevents the dummy from taking over. You don't
need to worry about disabling the local timer API, it will register a
harmless clockevent with a low rating (100) that should be ignored.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v5] clocksource:arm_global_timer: Add ARM global timer support.
On 06/24/13 08:53, Srinivas KANDAGATLA wrote:
+#include linux/clkdev.h
Why do you need this include?
+#include asm/mach/irq.h
And this one?
+static u64 gt_counter_read(void)
+{
+ u64 counter;
+ u32 lower;
+ u32 upper, old_upper;
+
+ upper = readl_relaxed(gt_base + GT_COUNTER1);
+ do {
+ old_upper = upper;
+ lower = readl_relaxed(gt_base + GT_COUNTER0);
+ upper = readl_relaxed(gt_base + GT_COUNTER1);
[snip]
+static void gt_compare_set(unsigned long delta, int periodic)
+{
+ u64 counter = gt_counter_read();
+ unsigned long ctrl = readl(gt_base + GT_CONTROL);
+
+ counter += delta;
+ ctrl = ~(GT_CONTROL_COMP_ENABLE | GT_CONTROL_IRQ_ENABLE);
+
+ writel(ctrl, gt_base + GT_CONTROL);
+ writel(lower_32_bits(counter), gt_base + GT_COMP0);
+ writel(upper_32_bits(counter), gt_base + GT_COMP1);
+
+ if (periodic) {
+ writel(delta, gt_base + GT_AUTO_INC);
+ ctrl |= GT_CONTROL_AUTO_INC;
+ }
+
+ ctrl |= GT_CONTROL_COMP_ENABLE | GT_CONTROL_IRQ_ENABLE;
+ writel(ctrl, gt_base + GT_CONTROL);
+}
Why is there a mix of the relaxed and non-relaxed io accessors?
+#ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
+static u32 gt_sched_clock_read(void)
notrace
+{
+ if (!gt_base)
+ return 0;
Seems impossible? Remove this check?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
