[PATCH v3 0/2] clocksource: Conexant CX92755 timers support

[PATCH v3 0/2] clocksource: Conexant CX92755 timers support

[Baruch Siach]

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

v3:
   * 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              | 164 +++++++++++++++++++++
 3 files changed, 183 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/digicolor-timer.txt
 create mode 100644 drivers/clocksource/timer-digicolor.c

-- 
2.1.4

[PATCH v3 1/2] clocksource: devicetree: document Conexant Digicolor timer binding

[Baruch Siach]

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 .../devicetree/bindings/timer/digicolor-timer.txt      | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/digicolor-timer.txt

diff --git a/Documentation/devicetree/bindings/timer/digicolor-timer.txt b/Documentation/devicetree/bindings/timer/digicolor-timer.txt
new file mode 100644
index 000000000000..d1b659bbc29f
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/digicolor-timer.txt
@@ -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>;
+	};
-- 
2.1.4

[PATCH v3 1/2] clocksource: devicetree: document Conexant Digicolor timer binding

[Daniel Lezcano]

On 01/21/2015 07:36 AM, Baruch Siach wrote:
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>

Please add a changelog and a more verbose description: see for example 
commit 2f2b7fb2 or 926b65e2. If there is a public documentation please 
add a link to it or give a quick description of how the timer behaves.

Also that will help to review the code in the timer.

> ---
>   .../devicetree/bindings/timer/digicolor-timer.txt      | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/timer/digicolor-timer.txt
>
> diff --git a/Documentation/devicetree/bindings/timer/digicolor-timer.txt b/Documentation/devicetree/bindings/timer/digicolor-timer.txt
> new file mode 100644
> index 000000000000..d1b659bbc29f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/digicolor-timer.txt
> @@ -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>;
> +	};
>


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

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

[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 <baruch@tkos.co.il>
---
 drivers/clocksource/Makefile          |   1 +
 drivers/clocksource/timer-digicolor.c | 164 ++++++++++++++++++++++++++++++++++
 2 files changed, 165 insertions(+)
 create mode 100644 drivers/clocksource/timer-digicolor.c

diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 94d90b24b56b..a993c108be67 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -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
diff --git a/drivers/clocksource/timer-digicolor.c b/drivers/clocksource/timer-digicolor.c
new file mode 100644
index 000000000000..014c50675980
--- /dev/null
+++ b/drivers/clocksource/timer-digicolor.c
@@ -0,0 +1,164 @@
+/*
+ * Conexant Digicolor timer driver
+ *
+ * Author: Baruch Siach <baruch@tkos.co.il>
+ *
+ * Copyright (C) 2014 Paradox Innovation Ltd.
+ *
+ * Based on:
+ *	Allwinner SoCs hstimer driver
+ *
+ * Copyright (C) 2013 Maxime Ripard
+ *
+ * Maxime Ripard <maxime.ripard@free-electrons.com>
+ *
+ * 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)
+
+static void __iomem *timer_base;
+static u32 ticks_per_jiffy;
+
+static void digicolor_clkevt_mode(enum clock_event_mode mode,
+				  struct clock_event_device *clk)
+{
+	switch (mode) {
+	case CLOCK_EVT_MODE_PERIODIC:
+		writeb(0, timer_base + CONTROL(TIMER_C));
+		writel(ticks_per_jiffy, timer_base + COUNT(TIMER_C));
+		writeb(0x21, timer_base + CONTROL(TIMER_C));
+		break;
+	case CLOCK_EVT_MODE_ONESHOT:
+		writeb(0, timer_base + CONTROL(TIMER_C));
+		writeb(0x11, timer_base + CONTROL(TIMER_C));
+		break;
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	default:
+		writeb(0, timer_base + CONTROL(TIMER_C));
+		break;
+	}
+}
+
+static int digicolor_clkevt_next_event(unsigned long evt,
+				       struct clock_event_device *unused)
+{
+	writeb(0, timer_base + CONTROL(TIMER_C));
+	writel(evt, timer_base + COUNT(TIMER_C));
+	writeb(0x11, timer_base + CONTROL(TIMER_C));
+
+	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,
+};
+
+
+static irqreturn_t digicolor_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = (struct clock_event_device *)dev_id;
+
+	evt->event_handler(evt);
+
+	return IRQ_HANDLED;
+}
+
+static struct irqaction digicolor_timer_irq = {
+	.name = "digicolor_timerC",
+	.flags = IRQF_TIMER | IRQF_IRQPOLL,
+	.handler = digicolor_timer_interrupt,
+	.dev_id = &digicolor_clockevent,
+};
+
+static u64 digicolor_timer_sched_read(void)
+{
+	return ~readl(timer_base + COUNT(TIMER_B));
+}
+
+static void __init digicolor_timer_init(struct device_node *node)
+{
+	unsigned long rate;
+	struct clk *clk;
+	int ret, irq;
+
+	timer_base = of_iomap(node, 0);
+	if (!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);
+
+	writeb(0, timer_base + CONTROL(TIMER_B));
+	writel(~0, timer_base + COUNT(TIMER_B));
+	writeb(1, timer_base + CONTROL(TIMER_B));
+
+	sched_clock_register(digicolor_timer_sched_read, 32, rate);
+	clocksource_mmio_init(timer_base + COUNT(TIMER_B), node->name,
+			      rate, 340, 32, clocksource_mmio_readl_down);
+
+	ticks_per_jiffy = DIV_ROUND_UP(rate, HZ);
+
+	ret = setup_irq(irq, &digicolor_timer_irq);
+	if (ret)
+		pr_warn("failed to setup timer irq %d (%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);
-- 
2.1.4

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

[Daniel Lezcano]

On 01/21/2015 07:36 AM, Baruch Siach wrote:
> 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 <baruch@tkos.co.il>
> ---
>   drivers/clocksource/Makefile          |   1 +
>   drivers/clocksource/timer-digicolor.c | 164 ++++++++++++++++++++++++++++++++++
>   2 files changed, 165 insertions(+)
>   create mode 100644 drivers/clocksource/timer-digicolor.c
>
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 94d90b24b56b..a993c108be67 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -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
> diff --git a/drivers/clocksource/timer-digicolor.c b/drivers/clocksource/timer-digicolor.c
> new file mode 100644
> index 000000000000..014c50675980
> --- /dev/null
> +++ b/drivers/clocksource/timer-digicolor.c
> @@ -0,0 +1,164 @@
> +/*
> + * Conexant Digicolor timer driver
> + *
> + * Author: Baruch Siach <baruch@tkos.co.il>
> + *
> + * Copyright (C) 2014 Paradox Innovation Ltd.
> + *
> + * Based on:
> + *	Allwinner SoCs hstimer driver


If this is based on the Allwinnner driver, may be you can have a look at 
the patchset sent by Maxim to make sure your implementation is complete ?

https://lkml.org/lkml/2015/1/11/52

> + * Copyright (C) 2013 Maxime Ripard
> + *
> + * Maxime Ripard <maxime.ripard@free-electrons.com>
> + *
> + * 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)
> +
> +static void __iomem *timer_base;
> +static u32 ticks_per_jiffy;

Mind to encapsulate those global variables in a structure and use 
container_of to access those fields like:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/clocksource/mtk_timer.c#n56

?

> +static void digicolor_clkevt_mode(enum clock_event_mode mode,
> +				  struct clock_event_device *clk)
> +{
> +	switch (mode) {
> +	case CLOCK_EVT_MODE_PERIODIC:
> +		writeb(0, timer_base + CONTROL(TIMER_C));

Even if that sounds overkill, please replace '0', '1' with whatever 
macro name (eg. TIMER_DISABLE/ENABLE).

> +		writel(ticks_per_jiffy, timer_base + COUNT(TIMER_C));
> +		writeb(0x21, timer_base + CONTROL(TIMER_C));

Please replace 0x21 occurrences by a macro.

> +		break;
> +	case CLOCK_EVT_MODE_ONESHOT:
> +		writeb(0, timer_base + CONTROL(TIMER_C));
> +		writeb(0x11, timer_base + CONTROL(TIMER_C));

Please replace 0x11 occurrences by a macro.

> +		break;
> +	case CLOCK_EVT_MODE_UNUSED:
> +	case CLOCK_EVT_MODE_SHUTDOWN:
> +	default:
> +		writeb(0, timer_base + CONTROL(TIMER_C));
> +		break;
> +	}
> +}
> +
> +static int digicolor_clkevt_next_event(unsigned long evt,
> +				       struct clock_event_device *unused)
> +{
> +	writeb(0, timer_base + CONTROL(TIMER_C));
> +	writel(evt, timer_base + COUNT(TIMER_C));
> +	writeb(0x11, timer_base + CONTROL(TIMER_C));

ditto.

> +	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,
> +};
> +
> +
> +static irqreturn_t digicolor_timer_interrupt(int irq, void *dev_id)
> +{
> +	struct clock_event_device *evt = (struct clock_event_device *)dev_id;
> +
> +	evt->event_handler(evt);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static struct irqaction digicolor_timer_irq = {
> +	.name = "digicolor_timerC",
> +	.flags = IRQF_TIMER | IRQF_IRQPOLL,
> +	.handler = digicolor_timer_interrupt,
> +	.dev_id = &digicolor_clockevent,
> +};

The current trend is to use 'request_irq', so no such structure 
declaration/initialization is needed.

> +static u64 digicolor_timer_sched_read(void)
> +{
> +	return ~readl(timer_base + COUNT(TIMER_B));
> +}
> +
> +static void __init digicolor_timer_init(struct device_node *node)
> +{
> +	unsigned long rate;
> +	struct clk *clk;
> +	int ret, irq;
> +
> +	timer_base = of_iomap(node, 0);
> +	if (!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);
> +
> +	writeb(0, timer_base + CONTROL(TIMER_B));
> +	writel(~0, timer_base + COUNT(TIMER_B));

s/~0/UINT_MAX/ ?

> +	writeb(1, timer_base + CONTROL(TIMER_B));
> +
> +	sched_clock_register(digicolor_timer_sched_read, 32, rate);
> +	clocksource_mmio_init(timer_base + COUNT(TIMER_B), node->name,
> +			      rate, 340, 32, clocksource_mmio_readl_down);
> +
> +	ticks_per_jiffy = DIV_ROUND_UP(rate, HZ);
> +
> +	ret = setup_irq(irq, &digicolor_timer_irq);
> +	if (ret)
> +		pr_warn("failed to setup timer irq %d (%d)\n", irq, ret);

s/setup_irq/request_irq/

> +
> +	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);
>


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

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

[Baruch Siach]

Hi Daniel,

On Thu, Jan 22, 2015 at 01:54:39PM +0100, Daniel Lezcano wrote:
> On 01/21/2015 07:36 AM, Baruch Siach wrote:
> >+ * Based on:
> >+ *	Allwinner SoCs hstimer driver
> 
> If this is based on the Allwinnner driver, may be you can have a look at the
> patchset sent by Maxim to make sure your implementation is complete ?
> 
> https://lkml.org/lkml/2015/1/11/52

Thanks for the tip. I'll take the applicable parts from that series.

[...]

> >+static void __iomem *timer_base;
> >+static u32 ticks_per_jiffy;
> 
> Mind to encapsulate those global variables in a structure and use
> container_of to access those fields like:
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/clocksource/mtk_timer.c#n56
> 
> ?

The trouble is that the sched_clock callback (digicolor_timer_sched_read) 
needs a static timer_base to find the timer counter. Do you think adding a 
structure is worth it only for ticks_per_jiffy?

> >+static void digicolor_clkevt_mode(enum clock_event_mode mode,
> >+				  struct clock_event_device *clk)
> >+{
> >+	switch (mode) {
> >+	case CLOCK_EVT_MODE_PERIODIC:
> >+		writeb(0, timer_base + CONTROL(TIMER_C));
> 
> Even if that sounds overkill, please replace '0', '1' with whatever macro
> name (eg. TIMER_DISABLE/ENABLE).

Will do for the entire file.

[...]

> >+static struct irqaction digicolor_timer_irq = {
> >+	.name = "digicolor_timerC",
> >+	.flags = IRQF_TIMER | IRQF_IRQPOLL,
> >+	.handler = digicolor_timer_interrupt,
> >+	.dev_id = &digicolor_clockevent,
> >+};
> 
> The current trend is to use 'request_irq', so no such structure
> declaration/initialization is needed.

Thanks. Will do.

> >+	writel(~0, timer_base + COUNT(TIMER_B));
> 
> s/~0/UINT_MAX/ ?

Ack.

Thanks for reviewing. I'll post an updated series shortly.

baruch

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

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

[Daniel Lezcano]

On 01/25/2015 03:46 PM, Baruch Siach wrote:
> Hi Daniel,
>
> On Thu, Jan 22, 2015 at 01:54:39PM +0100, Daniel Lezcano wrote:
>> On 01/21/2015 07:36 AM, Baruch Siach wrote:
>>> + * Based on:
>>> + *	Allwinner SoCs hstimer driver
>>
>> If this is based on the Allwinnner driver, may be you can have a look at the
>> patchset sent by Maxim to make sure your implementation is complete ?
>>
>> https://lkml.org/lkml/2015/1/11/52
>
> Thanks for the tip. I'll take the applicable parts from that series.
>
> [...]
>
>>> +static void __iomem *timer_base;
>>> +static u32 ticks_per_jiffy;
>>
>> Mind to encapsulate those global variables in a structure and use
>> container_of to access those fields like:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/clocksource/mtk_timer.c#n56
>>
>> ?
>
> The trouble is that the sched_clock callback (digicolor_timer_sched_read)
> needs a static timer_base to find the timer counter. Do you think adding a
> structure is worth it only for ticks_per_jiffy?

Well you will have to define the global variable for this structure. So 
you should be able to access the timer base directly. I agree it is not 
perfect but at least that will encapsulate the code for the clockevents 
side.

>>> +static void digicolor_clkevt_mode(enum clock_event_mode mode,
>>> +				  struct clock_event_device *clk)
>>> +{
>>> +	switch (mode) {
>>> +	case CLOCK_EVT_MODE_PERIODIC:
>>> +		writeb(0, timer_base + CONTROL(TIMER_C));
>>
>> Even if that sounds overkill, please replace '0', '1' with whatever macro
>> name (eg. TIMER_DISABLE/ENABLE).
>
> Will do for the entire file.
>
> [...]
>
>>> +static struct irqaction digicolor_timer_irq = {
>>> +	.name = "digicolor_timerC",
>>> +	.flags = IRQF_TIMER | IRQF_IRQPOLL,
>>> +	.handler = digicolor_timer_interrupt,
>>> +	.dev_id = &digicolor_clockevent,
>>> +};
>>
>> The current trend is to use 'request_irq', so no such structure
>> declaration/initialization is needed.
>
> Thanks. Will do.
>
>>> +	writel(~0, timer_base + COUNT(TIMER_B));
>>
>> s/~0/UINT_MAX/ ?
>
> Ack.
>
> Thanks for reviewing. I'll post an updated series shortly.

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