[v3,2/2] clocksource: driver for Conexant Digicolor SoC timer
diff mbox

Message ID 762d81b2cf12922ba28fb58b3ea4d5b7072abefb.1421821466.git.baruch@tkos.co.il
State New, archived
Headers show

Commit Message

Baruch Siach Jan. 21, 2015, 6:36 a.m. UTC
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

Comments

Daniel Lezcano Jan. 22, 2015, 12:54 p.m. UTC | #1
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);
>
Baruch Siach Jan. 25, 2015, 2:46 p.m. UTC | #2
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
Daniel Lezcano Jan. 25, 2015, 3:03 p.m. UTC | #3
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

Patch
diff mbox

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