Skip to contentSkip to conversation

clocksource: Conexant CX92755 timers support

PATCH Superseded Baruch Siach 3 participants 7 messages Jan 26, 2015

Subsystems (3)
Device tree bindings Clocksource, clockevent drivers Arm/conexant digicolor machine support

v4 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Baruch Siach

author maintainer
#0 lore raw

v4 cover clocksource: Conexant CX92755 timers support

This short series adds support for the Conexant CX92755 SoC timers. This SoC is part of the Conexant Digicolor series of SoCs.

v4:
Address review comments by Daniel Lezcano:

* Use macros for register configuration values

* Encapsulate static objects in a struct

* Use of_io_request_and_map to exclusively map registers

* Use UINT_MAX instead of ~0

* Use request_irq instead of setup_irq, obviating the need for a separate irqaction struct

v3:
http://article.gmane.org/gmane.linux.kernel/1870628
http://article.gmane.org/gmane.linux.kernel/1870627

* Split into a separate clocksource series

v2:
http://article.gmane.org/gmane.linux.kernel/1861850
http://article.gmane.org/gmane.linux.kernel/1861853

* Change the timer dt binding, so that the 'reg' property points to the first "Agent Communication" register. This should improve the chance of reusing this binding for other SoCs in this series.

* Add the CONTROL() and COUNT() macros to the timer driver to make the code clearer.

* Move arch/arm Kconfig changes from the clocksource driver patch to the base arch support patch to reduce dependency between them

v1:
http://article.gmane.org/gmane.linux.kernel/1855028
http://article.gmane.org/gmane.linux.kernel/1855025

Baruch Siach (2):
clocksource: devicetree: document Conexant Digicolor timer binding
clocksource: driver for Conexant Digicolor SoC timer

 .../devicetree/bindings/timer/digicolor-timer.txt  |  18 +++
 drivers/clocksource/Makefile                       |   1 +
 drivers/clocksource/timer-digicolor.c              | 170 +++++++++++++++++++++
 3 files changed, 189 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/digicolor-timer.txt
 create mode 100644 drivers/clocksource/timer-digicolor.c
Signature · 1 line
2.1.4
author maintainer
#1 lore raw

PATCH v4 1/2 clocksource: devicetree: document Conexant Digicolor timer binding

The Conexant CX92755 SoC provides 8 32-bit timers as part of its so called "Agent Communication" block. Timers can be configures either as free running or one shot. Each timer has a dedicated interrupt source in the CX92755 interrupts controller. The first timer (Timer A) can also be configured as watchdog.

This commit adds devicetree binding definition of this hardware module. The binding defined here should be reusable for other SoCs in the Digicolor series.

Signed-off-by Baruch Siach <[email protected]>
---
 .../devicetree/bindings/timer/digicolor-timer.txt      | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/digicolor-timer.txt
Documentation/devicetree/bindings/timer/digicolor-timer.txt new file +18 -0
new file mode 100644
index 000000000000..d1b659bbc29f
@@ -0,0 +1,18 @@
+Conexant Digicolor SoCs Timer Controller
+
+Required properties:
+
+- compatible : should be "cnxt,cx92755-timer"
+- reg : Specifies base physical address and size of the "Agent Communication"
+  timer registers
+- interrupts : Contains 8 interrupts, one for each timer
+- clocks: phandle to the main clock
+
+Example:
+
+	timer at f0000fc0 {
+		compatible = "cnxt,cx92755-timer";
+		reg = <0xf0000fc0 0x40>;
+		interrupts = <19>, <31>, <34>, <35>, <52>, <53>, <54>, <55>;
+		clocks = <&main_clk>;
+	};
Signature · 1 line
2.1.4
author maintainer
#2 lore raw

PATCH v4 2/2 clocksource: driver for Conexant Digicolor SoC timer

In reply to Baruch Siach

Add clocksource driver to the Conexant CX92755 SoC, part of the Digicolor SoCs series. Hardware provides 8 timers, A to H. Timer A is dedicated to a future watchdog driver so we don't use it here. Use timer B for sched_clock, and timer C for clock_event.

Signed-off-by Baruch Siach <[email protected]>
---
 drivers/clocksource/Makefile          |   1 +
 drivers/clocksource/timer-digicolor.c | 170 ++++++++++++++++++++++++++++++++++
 2 files changed, 171 insertions(+)
 create mode 100644 drivers/clocksource/timer-digicolor.c
drivers/clocksource/Makefile +1 -0
index 94d90b24b56b..a993c108be67 100644
@@ -10,6 +10,7 @@ obj-$(CONFIG_SH_TIMER_TMU)	+= sh_tmu.o
 obj-$(CONFIG_EM_TIMER_STI)	+= em_sti.o
 obj-$(CONFIG_CLKBLD_I8253)	+= i8253.o
 obj-$(CONFIG_CLKSRC_MMIO)	+= mmio.o
+obj-$(CONFIG_ARCH_DIGICOLOR)	+= timer-digicolor.o
 obj-$(CONFIG_DW_APB_TIMER)	+= dw_apb_timer.o
 obj-$(CONFIG_DW_APB_TIMER_OF)	+= dw_apb_timer_of.o
 obj-$(CONFIG_CLKSRC_NOMADIK_MTU)	+= nomadik-mtu.o
drivers/clocksource/timer-digicolor.c new file +170 -0

170 lines not on this page · raw message

Signature · 1 line
2.1.4
#3 lore raw

Re: [PATCH v4 2/2] clocksource: driver for Conexant Digicolor SoC timer

Quoted 82 lines · Baruch Siach

Add clocksource driver to the Conexant CX92755 SoC, part of the Digicolor SoCs series. Hardware provides 8 timers, A to H. Timer A is dedicated to a future watchdog driver so we don't use it here. Use timer B for sched_clock, and timer C for clock_event.

Signed-off-by Baruch Siach <[email protected]>
---
  drivers/clocksource/Makefile          |   1 +
  drivers/clocksource/timer-digicolor.c | 170 ++++++++++++++++++++++++++++++++++
  2 files changed, 171 insertions(+)

create mode 100644 drivers/clocksource/timer-digicolor.c

drivers/clocksource/Makefile +1 -0
index 94d90b24b56b..a993c108be67 100644
@@ -10,6 +10,7 @@ obj-$(CONFIG_SH_TIMER_TMU)	+= sh_tmu.o
  obj-$(CONFIG_EM_TIMER_STI)	+= em_sti.o
  obj-$(CONFIG_CLKBLD_I8253)	+= i8253.o
  obj-$(CONFIG_CLKSRC_MMIO)	+= mmio.o
+obj-$(CONFIG_ARCH_DIGICOLOR)	+= timer-digicolor.o
  obj-$(CONFIG_DW_APB_TIMER)	+= dw_apb_timer.o
  obj-$(CONFIG_DW_APB_TIMER_OF)	+= dw_apb_timer_of.o
  obj-$(CONFIG_CLKSRC_NOMADIK_MTU)	+= nomadik-mtu.o
drivers/clocksource/timer-digicolor.c new file +52 -0
new file mode 100644
index 000000000000..c97ea7eac4a5
@@ -0,0 +1,170 @@
+/*
+ * Conexant Digicolor timer driver
+ *
+ * Author: Baruch Siach <[email protected]>
+ *
+ * Copyright (C) 2014 Paradox Innovation Ltd.
+ *
+ * Based on:
+ *	Allwinner SoCs hstimer driver
+ *
+ * Copyright (C) 2013 Maxime Ripard
+ *
+ * Maxime Ripard <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+/*
+ * Conexant Digicolor SoCs have 8 configurable timers, named from "Timer A" to
+ * "Timer H". Timer A is the only one with watchdog support, so it is dedicated
+ * to the watchdog driver. This driver uses Timer B for sched_clock(), and
+ * Timer C for clockevents.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqreturn.h>
+#include <linux/sched_clock.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+
+enum {
+	TIMER_A,
+	TIMER_B,
+	TIMER_C,
+	TIMER_D,
+	TIMER_E,
+	TIMER_F,
+	TIMER_G,
+	TIMER_H,
+};
+
+#define CONTROL(t)	((t)*8)
+#define COUNT(t)	((t)*8 + 4)
+

drivers/clocksource/timer-digicolor.c line 62

Quoted 10 lines
drivers/clocksource/timer-digicolor.c new file +10 -0
+#define CONTROL_DISABLE		0
+#define CONTROL_ENABLE		BIT(0)
+#define CONTROL_MODE(m)		((m) << 4)
+#define CONTROL_MODE_ONESHOT	CONTROL_MODE(1)
+#define CONTROL_MODE_PERIODIC	CONTROL_MODE(2)
+
+static struct dc_dev_t {
+	void __iomem *timer_base;
+	u32 ticks_per_jiffy;
+} dc_dev;

Hi Baruch,

your code is valid but I think there is a misunderstanding when we talked about the encapsulation.

I was expecting something like:

static struct digicolor_timer {
	void __iomem *base;
	u32 ticks_per_jiffy;
	int channel;
	struct clock_event_device ce;
};

struct digicolor_timer *dg_timer(struct clock_event_device *ce)

{
	return container_of(ce, struct digicolor_timer, ce);
}

So the timer could be accessed directly from the clock_event_device structure. The exception is the 'sched_read' and the init function where you will have to address directly the global variable.

Perhaps, that would be nice if you take the opportunity to encapsulate the enable/disable functions.

static inline void dg_timer_disable(struct clock_event_device *ce)

{
	struct digital_timer *dt = dg_timer(ce);
	writeb(CONTROL_DISABLE, dt->base + CONTROL(dt->channel));	
}

static inline void dg_timer_enable(struct clock_event_device *ce, int mode)

{
	struct digital_timer *dt = dg_timer(ce);
	writeb(CONTROL_ENABLE | mode, dt->base, CONTROL(dt->channel));
}

drivers/clocksource/timer-digicolor.c line 65

Quoted 3 lines
drivers/clocksource/timer-digicolor.c new file +3 -0
+
+static void digicolor_clkevt_mode(enum clock_event_mode mode,
+				  struct clock_event_device *clk)

IMO, 'clk' argument name is confusing. I would suggest to use 'ce' or whatever.

drivers/clocksource/timer-digicolor.c line 73

Quoted 8 lines
drivers/clocksource/timer-digicolor.c new file +8 -0
+{
+	switch (mode) {
+	case CLOCK_EVT_MODE_PERIODIC:
+		writeb(CONTROL_DISABLE, dc_dev.timer_base + CONTROL(TIMER_C));
+		writel(dc_dev.ticks_per_jiffy,
+		       dc_dev.timer_base + COUNT(TIMER_C));
+		writeb(CONTROL_ENABLE | CONTROL_MODE_PERIODIC,
+		       dc_dev.timer_base + CONTROL(TIMER_C));

Becomes:

		dg_timer_disable(ce);
		dg_timer_enable(ce, CONTROL_MODE_PERIODIC);

...

drivers/clocksource/timer-digicolor.c line 78

Quoted 5 lines
drivers/clocksource/timer-digicolor.c new file +5 -0
+		break;
+	case CLOCK_EVT_MODE_ONESHOT:
+		writeb(CONTROL_DISABLE, dc_dev.timer_base + CONTROL(TIMER_C));
+		writeb(CONTROL_ENABLE | CONTROL_MODE_ONESHOT,
+		       dc_dev.timer_base + CONTROL(TIMER_C));

Becomes:

		dg_timer_disable(ce);
		dg_timer_enable(ce, CONTROL_MODE_ONESHOT);

drivers/clocksource/timer-digicolor.c line 83

Quoted 5 lines
drivers/clocksource/timer-digicolor.c new file +5 -0
+		break;
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	default:
+		writeb(CONTROL_DISABLE, dc_dev.timer_base + CONTROL(TIMER_C));

Becomes:

		dg_timer_disable(ce);

drivers/clocksource/timer-digicolor.c line 94

Quoted 11 lines
drivers/clocksource/timer-digicolor.c new file +11 -0
+		break;
+	}
+}
+
+static int digicolor_clkevt_next_event(unsigned long evt,
+				       struct clock_event_device *unused)
+{
+	writeb(CONTROL_DISABLE, dc_dev.timer_base + CONTROL(TIMER_C));
+	writel(evt, dc_dev.timer_base + COUNT(TIMER_C));
+	writeb(CONTROL_ENABLE | CONTROL_MODE_ONESHOT,
+	       dc_dev.timer_base + CONTROL(TIMER_C));

Becomes:

	dg_timer_disable(ce);
	writel(evt, dg_timer(ce)->base + COUNT(dg_timer(ce)->base));
	dg_timer_enable(ce, CONTROL_MODE_ONESHOT);

drivers/clocksource/timer-digicolor.c line 105

Quoted 10 lines
drivers/clocksource/timer-digicolor.c new file +10 -0
+	return 0;
+}
+
+static struct clock_event_device digicolor_clockevent = {
+	.name = "digicolor_tick",
+	.rating = 340,
+	.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+	.set_mode = digicolor_clkevt_mode,
+	.set_next_event = digicolor_clkevt_next_event,
+};

struct digicolor_timer dg_timer = {

.ce = {
.name = "digicolor_tick",
.rating = 340,
.features = CLOCK_EVT_FEAT_PERIODIC |
CLOCK_EVT_FEAT_ONESHOT,
.set_mode = digicolor_clkevt_mode, .set_next_event = digicolor_clkevt_next_event,
},
.channel = TIMER_C,
};

What do you think ?

Quoted 65 lines
drivers/clocksource/timer-digicolor.c new file +64 -0
+static irqreturn_t digicolor_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+
+	evt->event_handler(evt);
+
+	return IRQ_HANDLED;
+}
+
+static u64 digicolor_timer_sched_read(void)
+{
+	return ~readl(dc_dev.timer_base + COUNT(TIMER_B));
+}
+
+static void __init digicolor_timer_init(struct device_node *node)
+{
+	unsigned long rate;
+	struct clk *clk;
+	int ret, irq;
+
+	dc_dev.timer_base =
+		of_io_request_and_map(node, 0, of_node_full_name(node));
+	if (!dc_dev.timer_base) {
+		pr_err("Can't map registers");
+		return;
+	}
+
+	irq = irq_of_parse_and_map(node, TIMER_C);
+	if (irq <= 0) {
+		pr_err("Can't parse IRQ");
+		return;
+	}
+
+	clk = of_clk_get(node, 0);
+	if (IS_ERR(clk)) {
+		pr_err("Can't get timer clock");
+		return;
+	}
+	clk_prepare_enable(clk);
+	rate = clk_get_rate(clk);
+	dc_dev.ticks_per_jiffy = DIV_ROUND_UP(rate, HZ);
+
+	writeb(CONTROL_DISABLE, dc_dev.timer_base + CONTROL(TIMER_B));
+	writel(UINT_MAX, dc_dev.timer_base + COUNT(TIMER_B));
+	writeb(CONTROL_ENABLE, dc_dev.timer_base + CONTROL(TIMER_B));
+
+	sched_clock_register(digicolor_timer_sched_read, 32, rate);
+	clocksource_mmio_init(dc_dev.timer_base + COUNT(TIMER_B), node->name,
+			      rate, 340, 32, clocksource_mmio_readl_down);
+
+	ret = request_irq(irq, digicolor_timer_interrupt,
+			  IRQF_TIMER | IRQF_IRQPOLL, "digicolor_timerC",
+			  &digicolor_clockevent);
+	if (ret)
+		pr_warn("request of timer irq %d failed (%d)\n", irq, ret);
+
+	digicolor_clockevent.cpumask = cpu_possible_mask;
+	digicolor_clockevent.irq = irq;
+
+	clockevents_config_and_register(&digicolor_clockevent, rate, 0,
+					0xffffffff);
+}
+CLOCKSOURCE_OF_DECLARE(conexant_digicolor, "cnxt,cx92755-timer",
+		       digicolor_timer_init);
Signature · 5 lines
  <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
author maintainer
#4 lore raw

Re: [PATCH v4 2/2] clocksource: driver for Conexant Digicolor SoC timer

Hi Daniel,

On Mon, Jan 26, 2015 at 10:43:43AM +0100, Daniel Lezcano wrote:
[...]

Quoted 128 lines

drivers/clocksource/timer-digicolor.c line 62

Quoted 4 lines
drivers/clocksource/timer-digicolor.c new file +4 -0
+static struct dc_dev_t {
+	void __iomem *timer_base;
+	u32 ticks_per_jiffy;
+} dc_dev;

Hi Baruch,

your code is valid but I think there is a misunderstanding when we talked about the encapsulation.

I was expecting something like:

static struct digicolor_timer {
	void __iomem *base;
	u32 ticks_per_jiffy;
	int channel;
	struct clock_event_device ce;
};

struct digicolor_timer *dg_timer(struct clock_event_device *ce)

{
	return container_of(ce, struct digicolor_timer, ce);
}

So the timer could be accessed directly from the clock_event_device structure. The exception is the 'sched_read' and the init function where you will have to address directly the global variable.

Perhaps, that would be nice if you take the opportunity to encapsulate the enable/disable functions.

static inline void dg_timer_disable(struct clock_event_device *ce)

{
	struct digital_timer *dt = dg_timer(ce);
	writeb(CONTROL_DISABLE, dt->base + CONTROL(dt->channel));	
}

static inline void dg_timer_enable(struct clock_event_device *ce, int mode)

{
	struct digital_timer *dt = dg_timer(ce);
	writeb(CONTROL_ENABLE | mode, dt->base, CONTROL(dt->channel));
}

drivers/clocksource/timer-digicolor.c line 65

Quoted 3 lines
drivers/clocksource/timer-digicolor.c new file +3 -0
+
+static void digicolor_clkevt_mode(enum clock_event_mode mode,
+				  struct clock_event_device *clk)

IMO, 'clk' argument name is confusing. I would suggest to use 'ce' or whatever.

drivers/clocksource/timer-digicolor.c line 73

Quoted 8 lines
drivers/clocksource/timer-digicolor.c new file +8 -0
+{
+	switch (mode) {
+	case CLOCK_EVT_MODE_PERIODIC:
+		writeb(CONTROL_DISABLE, dc_dev.timer_base + CONTROL(TIMER_C));
+		writel(dc_dev.ticks_per_jiffy,
+		       dc_dev.timer_base + COUNT(TIMER_C));
+		writeb(CONTROL_ENABLE | CONTROL_MODE_PERIODIC,
+		       dc_dev.timer_base + CONTROL(TIMER_C));

Becomes:

		dg_timer_disable(ce);
		dg_timer_enable(ce, CONTROL_MODE_PERIODIC);

...

drivers/clocksource/timer-digicolor.c line 78

Quoted 5 lines
drivers/clocksource/timer-digicolor.c new file +5 -0
+		break;
+	case CLOCK_EVT_MODE_ONESHOT:
+		writeb(CONTROL_DISABLE, dc_dev.timer_base + CONTROL(TIMER_C));
+		writeb(CONTROL_ENABLE | CONTROL_MODE_ONESHOT,
+		       dc_dev.timer_base + CONTROL(TIMER_C));

Becomes:

		dg_timer_disable(ce);
		dg_timer_enable(ce, CONTROL_MODE_ONESHOT);

drivers/clocksource/timer-digicolor.c line 83

Quoted 5 lines
drivers/clocksource/timer-digicolor.c new file +5 -0
+		break;
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	default:
+		writeb(CONTROL_DISABLE, dc_dev.timer_base + CONTROL(TIMER_C));

Becomes:

		dg_timer_disable(ce);

drivers/clocksource/timer-digicolor.c line 94

Quoted 11 lines
drivers/clocksource/timer-digicolor.c new file +11 -0
+		break;
+	}
+}
+
+static int digicolor_clkevt_next_event(unsigned long evt,
+				       struct clock_event_device *unused)
+{
+	writeb(CONTROL_DISABLE, dc_dev.timer_base + CONTROL(TIMER_C));
+	writel(evt, dc_dev.timer_base + COUNT(TIMER_C));
+	writeb(CONTROL_ENABLE | CONTROL_MODE_ONESHOT,
+	       dc_dev.timer_base + CONTROL(TIMER_C));

Becomes:

	dg_timer_disable(ce);
	writel(evt, dg_timer(ce)->base + COUNT(dg_timer(ce)->base));
	dg_timer_enable(ce, CONTROL_MODE_ONESHOT);

drivers/clocksource/timer-digicolor.c line 105

Quoted 10 lines
drivers/clocksource/timer-digicolor.c new file +10 -0
+	return 0;
+}
+
+static struct clock_event_device digicolor_clockevent = {
+	.name = "digicolor_tick",
+	.rating = 340,
+	.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+	.set_mode = digicolor_clkevt_mode,
+	.set_next_event = digicolor_clkevt_next_event,
+};

struct digicolor_timer dg_timer = {

.ce = {
.name = "digicolor_tick",
.rating = 340,
.features = CLOCK_EVT_FEAT_PERIODIC |
CLOCK_EVT_FEAT_ONESHOT,
.set_mode = digicolor_clkevt_mode, .set_next_event = digicolor_clkevt_next_event,
},
.channel = TIMER_C,
};

Quoted 1 line

What do you think ?

OK. I'll give it a try.

Thanks for your prompt response and thorough review.

baruch

Signature · 3 lines
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
#5 lore raw

Re: [PATCH v4 2/2] clocksource: driver for Conexant Digicolor SoC timer

Quoted 1 line · Baruch Siach

Hi Daniel,

[ ... ]

Quoted 6 lines
Quoted 2 lines

What do you think ?

OK. I'll give it a try.

Thanks for your prompt response and thorough review.

Maxime Ripard just sent a patchset (patch 4/5) with some changes suggested above :)

http://www.spinics.net/lists/arm-kernel/msg394704.html

As you may see, the enable/disable is passing the timer channel as parameter while I suggested to store it in the structure. It is up to you to choose what fits better for your future needs.

-- Daniel

Signature · 5 lines
  <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
#6 lore raw

Re: [PATCH v4 2/2] clocksource: driver for Conexant Digicolor SoC timer

In reply to Baruch Siach
Quoted 8 lines · Baruch Siach

Hi Daniel,

Quoted 1 line · Daniel Lezcano

What do you think ?

OK. I'll give it a try.

Thanks for your prompt response and thorough review.

Can you please trim your replies proper? It's annoying to page through useless quoted text to find the meat of the mail.

Thanks,

tglx