Re: [PATCH v4] clocksource:arm_global_timer: Add ARM global timer support.

2013-06-24 Thread Srinivas KANDAGATLA 
Thankyou for the comments.

On 21/06/13 16:56, Thomas Gleixner wrote:
 On Fri, 21 Jun 2013, Srinivas KANDAGATLA wrote:
 +static void gt_clockevent_set_mode(enum clock_event_mode mode,
 +   struct clock_event_device *clk)
 +{
 +unsigned long ctrl;
 +
 +ctrl = readl(gt_base + GT_CONTROL);
 +switch (mode) {
 +case CLOCK_EVT_MODE_PERIODIC:
 +gt_compare_set(DIV_ROUND_CLOSEST(gt_clk_rate, HZ), 1);
 +break;
 +case CLOCK_EVT_MODE_ONESHOT:
 +ctrl = ~(GT_CONTROL_AUTO_INC);
 
 You should probably clear the irq enable bit as well. The core will
 program the next event right away.
Yep, it makes sense to clear the irq enable and comp enable in this case.
 
 +static irqreturn_t gt_clockevent_interrupt(int irq, void *dev_id)
 +{
 +struct clock_event_device *evt = *(struct clock_event_device **)dev_id;
 
 What kind of construct is this?
 
 You are using request_percpu_irq() and the device id is pointing to
 per cpu memory. Why do you need this horrible pointer indirection?
 
 Because a lot of other ARM code uses the same broken construct?

As Stephen already pointed out,

The reason for such a construct is ARM LOCAL TIMER apis, as ARM local
timer apis allocate struct clock_event_device. If the driver want to
reuse this clock event stucture it needs this double pointer. Which is
why we end up with this code.

On the other hand, The driver can ignore the struct clock_event_device
allocated by the local_timer code, and just use its own per cpu
clock_event which will remove this type of constructs. We do this for
boot cpu. I will go ahead doing this way because local_timer apis are
anyway going to be removed in near future (by Stephen's patch) and its
neat and obvious to manage allocations of clock_event structure with in
the driver.

 
 +static struct clock_event_device __percpu **gt_evt;
 +static DEFINE_PER_CPU(struct clock_event_device, gt_clockevent);
 
 So you have compile time allocated clock event device structures and
 then you allocate a percpu pointer area which holds pointers to the
 static area. Whatfor?
 
 Why not doing the obvious?
 
 static struct clock_event_device __percpu *gt_evt;
 
   gt_evt = alloc_percpu(struct clock_event_device):
 
   request_percpu_irq(.., gt_evt);
  
 And in the interrupt handler
 
   struct clock_event_device *evt = dev_id;
 
 +static int __cpuinit gt_clockevents_init(struct clock_event_device *clk)
 +{
 +struct clock_event_device **this_cpu_clk;
 +int cpu = smp_processor_id();
 +
 +clk-name = ARM global timer clock event;
 
 No spaces in the names please
Yep, replaced by arm_global_timer
 

 +static int __cpuinit gt_clockevents_setup(struct clock_event_device *clk)
 +{
 +/* Use existing clock_event for boot cpu */
 
 That comment is telling me what?
 
Will re-comment it in more detail.

 +if (per_cpu(percpu_init_called, smp_processor_id()))
 +return 0;
 
 And why do you need another percpu variable, if you can deduce the
 same information from clk as well.
 
   return clk-name ? 0 : gt_clockevents_init(clk);

As it get rid of a percpu variable I will change it.

 
 Hmm?
 
 +/* already enabled in gt_clocksource_init. */
 
 Huch?
 
There is only one enable bit for all the cores in the global_timer IP.
I will add more comments here for clarity.

 +return gt_clockevents_init(clk);
 +}
 
 +static void __init global_timer_of_register(struct device_node *np)
 +{
 +gt_clk = of_clk_get(np, 0);
 +if (!IS_ERR(gt_clk)) {
 +err = clk_prepare_enable(gt_clk);
 
 If that fails, you happily proceed, right?
I think there is a check missing here.

 
 +} else {
 +pr_warn(global-timer: clk not found\n);
 +err = -EINVAL;
 +goto out_unmap;
 +}
 +
 +gt_evt = alloc_percpu(struct clock_event_device *);
 +if (!gt_evt) {
 +pr_warn(global-timer: can't allocate memory\n);
 +err = -ENOMEM;
 +goto out_unmap;
 
 Leaves the clock enabled and prepared.

Yes I will fix it by adding new label

out_clk:
clk_disable_unprepare(clk);


 +
 +gt_clk_rate = clk_get_rate(gt_clk);
 +gt_clocksource_init();
 +gt_clockevents_init(evt);
 +#ifdef CONFIG_LOCAL_TIMERS
 +err =  local_timer_register(gt_lt_ops);
 +if (err) {
 +pr_warn(global-timer: unable to register local timer.\n);
 +free_percpu_irq(gt_ppi, gt_evt);
 +goto out_free;
 
 So that frees your magic pointers, but you still have the clockevent
 registered for the boot cpu. The interrupt handler of that device is
 happily dereferencing the freed percpu memory.

Yes I agree, there is a error handling issue.

I think, not doing anything in error-case seems to be best option and
most of the drivers do this way. This will at-least leave the clockevent
on boot cpu unaffected and let the system boot. I will with this
approach as it will let the system boot with some

[PATCH v4] clocksource:arm_global_timer: Add ARM global timer support.

2013-06-21 Thread Srinivas KANDAGATLA 
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
---
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 perfered to pick up SOC support patches [4-10] and merge them via arm-soc
tree for 3.11.

If its not too late can this patch be considered for 3.11 via clocksource tree?

Thanks,
srini

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 |  334 
 4 files changed, 369 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 f151c6c..b0c4c42 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -67,6 +67,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 CLKSRC_METAG_GENERIC
def_bool y if METAG
help
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 8d979c7..b2363cb 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -28,4 +28,5 @@ obj-$(CONFIG_CLKSRC_EXYNOS_MCT)   += exynos_mct.o
 obj-$(CONFIG_CLKSRC_SAMSUNG_PWM)   += samsung_pwm_timer.o
 
 obj-$(CONFIG_ARM_ARCH_TIMER)   += arm_arch_timer.o

Re: [PATCH v4] clocksource:arm_global_timer: Add ARM global timer support.

2013-06-21 Thread Thomas Gleixner 
On Fri, 21 Jun 2013, Srinivas KANDAGATLA wrote:
 +static void gt_clockevent_set_mode(enum clock_event_mode mode,
 +struct clock_event_device *clk)
 +{
 + unsigned long ctrl;
 +
 + ctrl = readl(gt_base + GT_CONTROL);
 + switch (mode) {
 + case CLOCK_EVT_MODE_PERIODIC:
 + gt_compare_set(DIV_ROUND_CLOSEST(gt_clk_rate, HZ), 1);
 + break;
 + case CLOCK_EVT_MODE_ONESHOT:
 + ctrl = ~(GT_CONTROL_AUTO_INC);

You should probably clear the irq enable bit as well. The core will
program the next event right away.

 +static irqreturn_t gt_clockevent_interrupt(int irq, void *dev_id)
 +{
 + struct clock_event_device *evt = *(struct clock_event_device **)dev_id;

What kind of construct is this?

You are using request_percpu_irq() and the device id is pointing to
per cpu memory. Why do you need this horrible pointer indirection?

Because a lot of other ARM code uses the same broken construct?

 +static struct clock_event_device __percpu **gt_evt;
 +static DEFINE_PER_CPU(struct clock_event_device, gt_clockevent);

So you have compile time allocated clock event device structures and
then you allocate a percpu pointer area which holds pointers to the
static area. Whatfor?

Why not doing the obvious?

static struct clock_event_device __percpu *gt_evt;

gt_evt = alloc_percpu(struct clock_event_device):

request_percpu_irq(.., gt_evt);
 
And in the interrupt handler

struct clock_event_device *evt = dev_id;

 +static int __cpuinit gt_clockevents_init(struct clock_event_device *clk)
 +{
 + struct clock_event_device **this_cpu_clk;
 + int cpu = smp_processor_id();
 +
 + clk-name = ARM global timer clock event;

No spaces in the names please

 + clk-features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
 + clk-set_mode = gt_clockevent_set_mode;
 + clk-set_next_event = gt_clockevent_set_next_event;
 + this_cpu_clk = __this_cpu_ptr(gt_evt);
 + *this_cpu_clk = clk;
 + clk-cpumask = cpumask_of(cpu);
 + clk-irq = gt_ppi;
 + clockevents_config_and_register(clk, gt_clk_rate,
 + 1, 0x);
 + per_cpu(percpu_init_called, cpu) = true;
 + enable_percpu_irq(clk-irq, IRQ_TYPE_NONE);
 + return 0;
 +}
 +
 +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)
 +{
 + /* Use existing clock_event for boot cpu */

That comment is telling me what?

 + if (per_cpu(percpu_init_called, smp_processor_id()))
 + return 0;

And why do you need another percpu variable, if you can deduce the
same information from clk as well.

return clk-name ? 0 : gt_clockevents_init(clk);

Hmm?

 + /* already enabled in gt_clocksource_init. */

Huch?

 + return gt_clockevents_init(clk);
 +}

 +static void __init global_timer_of_register(struct device_node *np)
 +{
 + gt_clk = of_clk_get(np, 0);
 + if (!IS_ERR(gt_clk)) {
 + err = clk_prepare_enable(gt_clk);

If that fails, you happily proceed, right?

 + } else {
 + pr_warn(global-timer: clk not found\n);
 + err = -EINVAL;
 + goto out_unmap;
 + }
 +
 + gt_evt = alloc_percpu(struct clock_event_device *);
 + if (!gt_evt) {
 + pr_warn(global-timer: can't allocate memory\n);
 + err = -ENOMEM;
 + goto out_unmap;

Leaves the clock enabled and prepared.

 + }
 +
 + err = request_percpu_irq(gt_ppi, gt_clockevent_interrupt,
 +  gt, gt_evt);
 + if (err) {
 + pr_warn(global-timer: can't register interrupt %d (%d)\n,
 + gt_ppi, err);
 + goto out_free;

Ditto

 + }
 +
 + gt_clk_rate = clk_get_rate(gt_clk);
 + gt_clocksource_init();
 + gt_clockevents_init(evt);
 +#ifdef CONFIG_LOCAL_TIMERS
 + err =  local_timer_register(gt_lt_ops);
 + if (err) {
 + pr_warn(global-timer: unable to register local timer.\n);
 + free_percpu_irq(gt_ppi, gt_evt);
 + goto out_free;

So that frees your magic pointers, but you still have the clockevent
registered for the boot cpu. The interrupt handler of that device is
happily dereferencing the freed percpu memory.

How is that supposed to work?

Thanks,

tglx
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH v4] clocksource:arm_global_timer: Add ARM global timer support.

2013-06-21 Thread Thomas Gleixner 
On Fri, 21 Jun 2013, Stephen Boyd wrote:

 On 06/21/13 08:56, Thomas Gleixner wrote:
 
  +static irqreturn_t gt_clockevent_interrupt(int irq, void *dev_id)
  +{
  +  struct clock_event_device *evt = *(struct clock_event_device **)dev_id;
  What kind of construct is this?
 
  You are using request_percpu_irq() and the device id is pointing to
  per cpu memory. Why do you need this horrible pointer indirection?
 
  Because a lot of other ARM code uses the same broken construct?
 
 This is an artifact of the ARM local timer API. I have been trying for a

No, it's not an artifact. It's a copy and paste issue. Looking at
drivers/clocksource/arm_arch_timer.c

arch_timer_evt = alloc_percpu(struct clock_event_device);
...
err = request_percpu_irq(ppi, arch_timer_handler_virt,
 arch_timer, arch_timer_evt);

This code is correct and it does not need any of the changes.

Doing it with the pointer madness is just wrong, nothing else. Even if
there is a historic reason why the pointer juggling was necessary at
some point, it's obviously not needed anymore.

And historic crap is no justification for brainlessly copied code.

Thanks,

tglx


___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH v4] clocksource:arm_global_timer: Add ARM global timer support.

2013-06-21 Thread Stephen Boyd 
On 06/21/13 14:00, Thomas Gleixner wrote:
 On Fri, 21 Jun 2013, Stephen Boyd wrote:

 On 06/21/13 08:56, Thomas Gleixner wrote:
 +static irqreturn_t gt_clockevent_interrupt(int irq, void *dev_id)
 +{
 +  struct clock_event_device *evt = *(struct clock_event_device **)dev_id;
 What kind of construct is this?

 You are using request_percpu_irq() and the device id is pointing to
 per cpu memory. Why do you need this horrible pointer indirection?

 Because a lot of other ARM code uses the same broken construct?
 This is an artifact of the ARM local timer API. I have been trying for a
 No, it's not an artifact. It's a copy and paste issue. Looking at
 drivers/clocksource/arm_arch_timer.c

 arch_timer_evt = alloc_percpu(struct clock_event_device);
 ...
   err = request_percpu_irq(ppi, arch_timer_handler_virt,
  arch_timer, arch_timer_evt);

 This code is correct and it does not need any of the changes.

 Doing it with the pointer madness is just wrong, nothing else. Even if
 there is a historic reason why the pointer juggling was necessary at
 some point, it's obviously not needed anymore.

 And historic crap is no justification for brainlessly copied code.

The arm architected timers do not use the ARM local timer API anymore.
So yes that code is correct now that it has been moved off the local
timer API. See commit 1ba1cefc277865a0ac222f53bbbf2ebacad1559a for more
info.

Please merge my patches so the ARM local timer API can go away.

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