diff mbox

[v3,3/5] ARM: bcm4760: Add system timer

Message ID 20130814221237.695587612@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Domenico Andreoli Aug. 14, 2013, 10:10 p.m. UTC 
From: Domenico Andreoli <domenico.andreoli@linux.com>

System timer implementation for the BCM4760 based SoCs.

v3:
* unchanged

v2:
* dropped clock-frequency property, its allowed value was anyway fixed
  to 24MHz and not clearly related to any known clock

v1:
* initial release

Cc: John Stultz <john.stultz@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: devicetree@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Domenico Andreoli <domenico.andreoli@linux.com>
---
 Documentation/devicetree/bindings/timer/brcm,bcm4760-system-timer.txt |   21 +
 arch/arm/boot/dts/bcm4760.dtsi                                        |    8 +
 arch/arm/mach-bcm/Kconfig                                             |    1 +
 drivers/clocksource/Makefile                                          |    1 +
 drivers/clocksource/bcm4760_timer.c                                   |  163 ++++++++++
 5 files changed, 194 insertions(+)

Comments

Stephen Boyd Aug. 15, 2013, 12:30 a.m. UTC | #1 
On 08/15, Domenico Andreoli wrote:
> +
> +static inline void __iomem *to_load(struct bcm4760_timer *timer)
> +{
> +	return timer->base + TIMER_LOAD_OFFSET;
> +}
> +
> +static inline void __iomem *to_control(struct bcm4760_timer *timer)
> +{
> +	return timer->base + TIMER_CONTROL_OFFSET;
> +}
> +
> +static inline void __iomem *to_intclr(struct bcm4760_timer *timer)
> +{
> +	return timer->base + TIMER_INTCLR_OFFSET;
> +}
> +
> +static inline void __iomem *to_ris(struct bcm4760_timer *timer)
> +{
> +	return timer->base + TIMER_RIS_OFFSET;
> +}
> +
> +static inline void __iomem *to_mis(struct bcm4760_timer *timer)
> +{
> +	return timer->base + TIMER_MIS_OFFSET;
> +}

Style Nit: This is new. Usually people either make a
<my_driver>_{readl,writel}() function that takes the struct and
an offset or they just add the offset directly in their
readl/writel calls. Can you do that? Probably save some lines of
code.

> +static irqreturn_t bcm4760_timer_interrupt(int irq, void *dev_id)
> +{
> +	struct bcm4760_timer *timer = dev_id;
> +	void (*event_handler)(struct clock_event_device *);
> +
> +	/* check the (masked) interrupt status */
> +	if (!readl_relaxed(to_mis(timer)))
> +		return IRQ_NONE;
> +
> +	/* clear the timer interrupt */
> +	writel_relaxed(1, to_intclr(timer));
> +
> +	event_handler = ACCESS_ONCE(timer->evt.event_handler);
> +	if (event_handler)
> +		event_handler(&timer->evt);

This is unfortunate. Do you have a pending timer interrupt left
by the bootloader?

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void __init bcm4760_init_time(struct device_node *node)
> +{
> +	void __iomem *base;
> +	u32 freq = 24000000;

Why have freq in the DT binding at all then?

> +	int irq;
> +	struct bcm4760_timer *timer;
> +
> +	base = of_iomap(node, 0);
> +	if (!base)
> +		panic("Can't remap timer registers");
> +
> +	timer = kzalloc(sizeof(*timer), GFP_KERNEL);
> +	if (!timer)
> +		panic("Can't allocate timer struct\n");
> +
> +	irq = irq_of_parse_and_map(node, 0);
> +	if (irq <= 0)
> +		panic("Can't parse timer IRQ");
> +
> +	timer->base = base;
> +	timer->evt.name = node->name;
> +	timer->evt.rating = 300;
> +	timer->evt.features = CLOCK_EVT_FEAT_ONESHOT;
> +	timer->evt.set_mode = bcm4760_timer_set_mode;
> +	timer->evt.set_next_event = bcm4760_timer_set_next_event;
> +	timer->evt.cpumask = cpumask_of(0);
> +	timer->act.name = node->name;
> +	timer->act.flags = IRQF_TIMER | IRQF_SHARED;
> +	timer->act.dev_id = timer;
> +	timer->act.handler = bcm4760_timer_interrupt;
> +
> +	if (setup_irq(irq, &timer->act))
> +		panic("Can't set up timer IRQ\n");
> +
> +	clockevents_config_and_register(&timer->evt, freq, 0xf, 0xffffffff);

If you switch this registration and the setup_irq() call you
shouldn't need the ACCESS_ONCE() and that check in the irq handler.
Please switch the order and or clear the interrupt before
registering the clockevent and remove the checks in the interrupt
handler.
Domenico Andreoli Aug. 15, 2013, 6:32 a.m. UTC | #2 
On Wed, Aug 14, 2013 at 05:30:38PM -0700, Stephen Boyd wrote:
> On 08/15, Domenico Andreoli wrote:
> > +
> > +static inline void __iomem *to_load(struct bcm4760_timer *timer)
> > +{
> > +	return timer->base + TIMER_LOAD_OFFSET;
> > +}
> > +
> > +static inline void __iomem *to_control(struct bcm4760_timer *timer)
> > +{
> > +	return timer->base + TIMER_CONTROL_OFFSET;
> > +}
> > +
> > +static inline void __iomem *to_intclr(struct bcm4760_timer *timer)
> > +{
> > +	return timer->base + TIMER_INTCLR_OFFSET;
> > +}
> > +
> > +static inline void __iomem *to_ris(struct bcm4760_timer *timer)
> > +{
> > +	return timer->base + TIMER_RIS_OFFSET;
> > +}
> > +
> > +static inline void __iomem *to_mis(struct bcm4760_timer *timer)
> > +{
> > +	return timer->base + TIMER_MIS_OFFSET;
> > +}
> 
> Style Nit: This is new. Usually people either make a
> <my_driver>_{readl,writel}() function that takes the struct and
> an offset or they just add the offset directly in their
> readl/writel calls. Can you do that? Probably save some lines of
> code.

yes, sure.

> 
> > +static irqreturn_t bcm4760_timer_interrupt(int irq, void *dev_id)
> > +{
> > +	struct bcm4760_timer *timer = dev_id;
> > +	void (*event_handler)(struct clock_event_device *);
> > +
> > +	/* check the (masked) interrupt status */
> > +	if (!readl_relaxed(to_mis(timer)))
> > +		return IRQ_NONE;
> > +
> > +	/* clear the timer interrupt */
> > +	writel_relaxed(1, to_intclr(timer));
> > +
> > +	event_handler = ACCESS_ONCE(timer->evt.event_handler);
> > +	if (event_handler)
> > +		event_handler(&timer->evt);
> 
> This is unfortunate. Do you have a pending timer interrupt left
> by the bootloader?

Do you mean that if no interrupts are expected beween the irq request and
the call clockevents_config_and_register(), I can assume event_handler is
always set?

> 
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static void __init bcm4760_init_time(struct device_node *node)
> > +{
> > +	void __iomem *base;
> > +	u32 freq = 24000000;
> 
> Why have freq in the DT binding at all then?

To get an HW address and remap it at runtime, I guess. I copied it but
would also prefer to use DT only where really needed.

> 
> > +	int irq;
> > +	struct bcm4760_timer *timer;
> > +
> > +	base = of_iomap(node, 0);
> > +	if (!base)
> > +		panic("Can't remap timer registers");
> > +
> > +	timer = kzalloc(sizeof(*timer), GFP_KERNEL);
> > +	if (!timer)
> > +		panic("Can't allocate timer struct\n");
> > +
> > +	irq = irq_of_parse_and_map(node, 0);
> > +	if (irq <= 0)
> > +		panic("Can't parse timer IRQ");
> > +
> > +	timer->base = base;
> > +	timer->evt.name = node->name;
> > +	timer->evt.rating = 300;
> > +	timer->evt.features = CLOCK_EVT_FEAT_ONESHOT;
> > +	timer->evt.set_mode = bcm4760_timer_set_mode;
> > +	timer->evt.set_next_event = bcm4760_timer_set_next_event;
> > +	timer->evt.cpumask = cpumask_of(0);
> > +	timer->act.name = node->name;
> > +	timer->act.flags = IRQF_TIMER | IRQF_SHARED;
> > +	timer->act.dev_id = timer;
> > +	timer->act.handler = bcm4760_timer_interrupt;
> > +
> > +	if (setup_irq(irq, &timer->act))
> > +		panic("Can't set up timer IRQ\n");
> > +
> > +	clockevents_config_and_register(&timer->evt, freq, 0xf, 0xffffffff);
> 
> If you switch this registration and the setup_irq() call you
> shouldn't need the ACCESS_ONCE() and that check in the irq handler.
> Please switch the order and or clear the interrupt before
> registering the clockevent and remove the checks in the interrupt
> handler.

got it. Thank you.

Domenico
Olof Johansson Aug. 29, 2013, 11:20 p.m. UTC | #3 
On Thu, Aug 15, 2013 at 12:10:46AM +0200, Domenico Andreoli wrote:
> From: Domenico Andreoli <domenico.andreoli@linux.com>
> 
> System timer implementation for the BCM4760 based SoCs.
> 
> v3:
> * unchanged
> 
> v2:
> * dropped clock-frequency property, its allowed value was anyway fixed
>   to 24MHz and not clearly related to any known clock
> 
> v1:
> * initial release
> 
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: devicetree@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Signed-off-by: Domenico Andreoli <domenico.andreoli@linux.com>

Acked-by: Olof Johansson <olof@lixom.net>
Olof Johansson Aug. 29, 2013, 11:21 p.m. UTC | #4 
On Thu, Aug 29, 2013 at 4:20 PM, Olof Johansson <olof@lixom.net> wrote:

> Acked-by: Olof Johansson <olof@lixom.net>

Sorry, should have clarified: Acked if the changes requested by
Stephen are done.


-Olof
Domenico Andreoli Aug. 30, 2013, 7:54 a.m. UTC | #5 
On Thu, Aug 29, 2013 at 04:21:51PM -0700, Olof Johansson wrote:
> On Thu, Aug 29, 2013 at 4:20 PM, Olof Johansson <olof@lixom.net> wrote:
> 
> > Acked-by: Olof Johansson <olof@lixom.net>
> 
> Sorry, should have clarified: Acked if the changes requested by
> Stephen are done.

I've already prepared them. Will repost later.

> 
> 
> -Olof

Domenico
diff mbox

Patch

Index: b/Documentation/devicetree/bindings/timer/brcm,bcm4760-system-timer.txt
===================================================================
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/brcm,bcm4760-system-timer.txt
@@ -0,0 +1,21 @@ 
+Broadcom BCM4760 System Timer device tree bindings
+--------------------------------------------------
+
+The BCM4760 Timer peripheral provides either two or four 32-bit timer
+channels. Three timer blocks are available at 0xba000, 0xbb000 and
+0xd1000. The first two provide four channels, the last (in the AON -
+Always ON power domain) provides only two.
+
+Required properties:
+
+- compatible : should be "brcm,bcm4760-system-timer"
+- reg : Specifies base physical address and size of the registers.
+- interrupts : A list of 2 or 4 interrupt sinks; one per timer channel.
+
+Example:
+
+timer@ba000 {
+	compatible = "brcm,bcm4760-system-timer";
+	reg = <0xba000 0x1000>;
+	interrupts = <4>, <11>;
+};
Index: b/arch/arm/boot/dts/bcm4760.dtsi
===================================================================
--- a/arch/arm/boot/dts/bcm4760.dtsi
+++ b/arch/arm/boot/dts/bcm4760.dtsi
@@ -27,6 +27,14 @@ 
 		#size-cells = <1>;
 		ranges;
 
+		timer@ba000 {
+			compatible = "brcm,bcm4760-system-timer";
+			reg = <0xba000 0x1000>;
+			interrupt-parent = <&vic0>;
+			interrupts = <4>, <11>;
+			clock-frequency = <24000000>;
+		};
+
 		vic0: interrupt-controller@80000 {
 			compatible = "brcm,bcm4760-pl192", "arm,pl192-vic", "arm,primecell";
 			reg = <0x80000 0x1000>;
Index: b/drivers/clocksource/Makefile
===================================================================
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -17,6 +17,7 @@  obj-$(CONFIG_CLKSRC_DBX500_PRCMU)	+= clk
 obj-$(CONFIG_ARMADA_370_XP_TIMER)	+= time-armada-370-xp.o
 obj-$(CONFIG_ORION_TIMER)	+= time-orion.o
 obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835_timer.o
+obj-$(CONFIG_ARCH_BCM4760)	+= bcm4760_timer.o
 obj-$(CONFIG_ARCH_MARCO)	+= timer-marco.o
 obj-$(CONFIG_ARCH_MXS)		+= mxs_timer.o
 obj-$(CONFIG_ARCH_PRIMA2)	+= timer-prima2.o
Index: b/drivers/clocksource/bcm4760_timer.c
===================================================================
--- /dev/null
+++ b/drivers/clocksource/bcm4760_timer.c
@@ -0,0 +1,163 @@ 
+/*
+ * Broadcom BCM4760 based ARM11 SoCs system timer
+ *
+ * Copyright (C) 2012 Domenico Andreoli <domenico.andreoli@linux.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <linux/clockchips.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+
+#define TIMER_LOAD_OFFSET          0x00          /* load */
+#define TIMER_VALUE_OFFSET         0x04          /* value */
+#define TIMER_CONTROL_OFFSET       0x08          /* control */
+#define TIMER_INTCLR_OFFSET        0x0c          /* interrupt clear */
+#define TIMER_RIS_OFFSET           0x10          /* raw interrupt */
+#define TIMER_MIS_OFFSET           0x14          /* masked interrupt status */
+#define TIMER_BGLOAD_OFFSET        0x18          /* background load */
+
+#define TIMER_CTRL_ONESHOTMODE     BIT(0)        /* One shot mode */
+#define TIMER_CTRL_32BIT           BIT(1)        /* 32-bit counter mode */
+#define TIMER_CTRL_IE              BIT(5)        /* Interrupt enable */
+#define TIMER_CTRL_PERIODIC        BIT(6)        /* Periodic mode */
+#define TIMER_CTRL_EN              BIT(7)        /* Timer enable */
+#define TIMER_CTRL_CLK2            BIT(9)        /* Clock 2 selected */
+#define TIMER_CTRL_PREBY16         (1 << 2)      /* prescale divide by 16 */
+#define TIMER_CTRL_PREBY256        (2 << 2)      /* prescale divide by 256 */
+
+struct bcm4760_timer {
+	void __iomem *base;
+	struct clock_event_device evt;
+	struct irqaction act;
+};
+
+static inline void __iomem *to_load(struct bcm4760_timer *timer)
+{
+	return timer->base + TIMER_LOAD_OFFSET;
+}
+
+static inline void __iomem *to_control(struct bcm4760_timer *timer)
+{
+	return timer->base + TIMER_CONTROL_OFFSET;
+}
+
+static inline void __iomem *to_intclr(struct bcm4760_timer *timer)
+{
+	return timer->base + TIMER_INTCLR_OFFSET;
+}
+
+static inline void __iomem *to_ris(struct bcm4760_timer *timer)
+{
+	return timer->base + TIMER_RIS_OFFSET;
+}
+
+static inline void __iomem *to_mis(struct bcm4760_timer *timer)
+{
+	return timer->base + TIMER_MIS_OFFSET;
+}
+
+static void bcm4760_timer_set_mode(enum clock_event_mode mode,
+	struct clock_event_device *evt_dev)
+{
+	struct bcm4760_timer *timer;
+	u32 val;
+
+	timer = container_of(evt_dev, struct bcm4760_timer, evt);
+	val = TIMER_CTRL_CLK2 | TIMER_CTRL_32BIT |
+	      TIMER_CTRL_IE | TIMER_CTRL_EN;
+
+	switch (mode) {
+	case CLOCK_EVT_MODE_ONESHOT:
+		writel(val | TIMER_CTRL_ONESHOTMODE, to_control(timer));
+		break;
+	case CLOCK_EVT_MODE_RESUME:
+	case CLOCK_EVT_MODE_SHUTDOWN:
+		break;
+	default:
+		WARN(1, "%s: unhandled event mode %d\n", __func__, mode);
+		break;
+	}
+}
+
+static int bcm4760_timer_set_next_event(unsigned long event,
+	struct clock_event_device *evt_dev)
+{
+	struct bcm4760_timer *timer;
+
+	timer = container_of(evt_dev, struct bcm4760_timer, evt);
+	writel(event, to_load(timer));
+	return 0;
+}
+
+static irqreturn_t bcm4760_timer_interrupt(int irq, void *dev_id)
+{
+	struct bcm4760_timer *timer = dev_id;
+	void (*event_handler)(struct clock_event_device *);
+
+	/* check the (masked) interrupt status */
+	if (!readl_relaxed(to_mis(timer)))
+		return IRQ_NONE;
+
+	/* clear the timer interrupt */
+	writel_relaxed(1, to_intclr(timer));
+
+	event_handler = ACCESS_ONCE(timer->evt.event_handler);
+	if (event_handler)
+		event_handler(&timer->evt);
+
+	return IRQ_HANDLED;
+}
+
+static void __init bcm4760_init_time(struct device_node *node)
+{
+	void __iomem *base;
+	u32 freq = 24000000;
+	int irq;
+	struct bcm4760_timer *timer;
+
+	base = of_iomap(node, 0);
+	if (!base)
+		panic("Can't remap timer registers");
+
+	timer = kzalloc(sizeof(*timer), GFP_KERNEL);
+	if (!timer)
+		panic("Can't allocate timer struct\n");
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (irq <= 0)
+		panic("Can't parse timer IRQ");
+
+	timer->base = base;
+	timer->evt.name = node->name;
+	timer->evt.rating = 300;
+	timer->evt.features = CLOCK_EVT_FEAT_ONESHOT;
+	timer->evt.set_mode = bcm4760_timer_set_mode;
+	timer->evt.set_next_event = bcm4760_timer_set_next_event;
+	timer->evt.cpumask = cpumask_of(0);
+	timer->act.name = node->name;
+	timer->act.flags = IRQF_TIMER | IRQF_SHARED;
+	timer->act.dev_id = timer;
+	timer->act.handler = bcm4760_timer_interrupt;
+
+	if (setup_irq(irq, &timer->act))
+		panic("Can't set up timer IRQ\n");
+
+	clockevents_config_and_register(&timer->evt, freq, 0xf, 0xffffffff);
+}
+
+CLOCKSOURCE_OF_DECLARE(bcm4760, "brcm,bcm4760-system-timer", bcm4760_init_time);
Index: b/arch/arm/mach-bcm/Kconfig
===================================================================
--- a/arch/arm/mach-bcm/Kconfig
+++ b/arch/arm/mach-bcm/Kconfig
@@ -23,4 +23,5 @@  config ARCH_BCM4760
 	select ARM_AMBA
 	select ARM_VIC
 	select CLKSRC_OF
+	select GENERIC_CLOCKEVENTS
 	select SOC_BUS