[PATCH v3 1/2] dt-bindings: timer: Add binding doc for nxp system counter timer

[PATCH v3 1/2] dt-bindings: timer: Add binding doc for nxp system counter timer

[Jacky Bai]

From: Bai Ping <ping.bai@nxp.com>

Add the binding doc for nxp system counter timer module.

Signed-off-by: Bai Ping <ping.bai@nxp.com>
---
change v1->v2
 - remove the blank line at EOF
change v2->v3
 - update the binding example based on the driver change
---
 .../devicetree/bindings/timer/nxp,sysctr-timer.txt | 25 ++++++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt

diff --git a/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt b/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt
new file mode 100644
index 0000000..d576599
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt
@@ -0,0 +1,25 @@
+NXP System Counter Module(sys_ctr)
+
+The system counter(sys_ctr) is a programmable system counter which provides
+a shared time base to Cortex A15, A7, A53, A73, etc. it is intended for use in
+applications where the counter is always powered and support multiple,
+unrelated clocks. The compare frame inside can be used for timer purpose.
+
+Required properties:
+
+- compatible :      should be "nxp,sysctr-timer"
+- reg :             Specifies the base physical address and size of the comapre
+                    frame and the counter control, read & compare.
+- interrupts :      should be the first compare frames' interrupt
+- clocks : 	    Specifies the counter clock.
+- clock-names: 	    Specifies the clock's name of this module
+
+Example:
+
+	system_counter: timer@306a0000 {
+		compatible = "nxp,sysctr-timer";
+		reg = <0x306a0000 0x20000>;/* system-counter-rd & compare */
+		clocks = <&clk_8m>;
+		clock-names = "per";
+		interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>;
+	};
-- 
1.9.1

[PATCH v3 2/2] driver: clocksource: Add nxp system counter timer driver support

[Jacky Bai]

From: Bai Ping <ping.bai@nxp.com>

The system counter (sys_ctr) is a programmable system counter
which provides a shared time base to the Cortex A15, A7, A53 etc cores.
It is intended for use in applications where the counter is always
powered on and supports multiple, unrelated clocks. The sys_ctr hardware
supports:
 - 56-bit counter width (roll-over time greater than 40 years)
 - compare frame(64-bit compare value) contains programmable interrupt
   generation

Signed-off-by: Bai Ping <ping.bai@nxp.com>
---
change v1->v2:
 - no change 
change v2->v3:
 - remove the clocksource, we only need to use this module for timer purpose,
   so register it as clockevent is enough.
 - use the timer_of_init to init the irq, clock, etc.
 - remove some unnecessary comments.
---
 drivers/clocksource/Kconfig            |   7 ++
 drivers/clocksource/Makefile           |   1 +
 drivers/clocksource/timer-imx-sysctr.c | 149 +++++++++++++++++++++++++++++++++
 3 files changed, 157 insertions(+)
 create mode 100644 drivers/clocksource/timer-imx-sysctr.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 171502a..90cd908 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -598,6 +598,13 @@ config CLKSRC_IMX_TPM
 	  Enable this option to use IMX Timer/PWM Module (TPM) timer as
 	  clocksource.
 
+config TIMER_IMX_SYS_CTR
+	bool "i.MX system counter timer" if COMPILE_TEST
+	depends on ARCH_MXC
+	select TIMER_OF
+	help
+	  Enable this option to use i.MX system counter timer for clockevent.
+
 config CLKSRC_ST_LPC
 	bool "Low power clocksource found in the LPC" if COMPILE_TEST
 	select TIMER_OF if OF
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index be6e0fb..bc5ce50 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -73,6 +73,7 @@ obj-$(CONFIG_CLKSRC_MIPS_GIC)		+= mips-gic-timer.o
 obj-$(CONFIG_CLKSRC_TANGO_XTAL)		+= timer-tango-xtal.o
 obj-$(CONFIG_CLKSRC_IMX_GPT)		+= timer-imx-gpt.o
 obj-$(CONFIG_CLKSRC_IMX_TPM)		+= timer-imx-tpm.o
+obj-$(CONFIG_TIMER_IMX_SYS_CTR)		+= timer-imx-sysctr.o
 obj-$(CONFIG_ASM9260_TIMER)		+= asm9260_timer.o
 obj-$(CONFIG_H8300_TMR8)		+= h8300_timer8.o
 obj-$(CONFIG_H8300_TMR16)		+= h8300_timer16.o
diff --git a/drivers/clocksource/timer-imx-sysctr.c b/drivers/clocksource/timer-imx-sysctr.c
new file mode 100644
index 0000000..d3afc3b
--- /dev/null
+++ b/drivers/clocksource/timer-imx-sysctr.c
@@ -0,0 +1,149 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// Copyright 2017-2019 NXP
+
+#include <linux/interrupt.h>
+#include <linux/clockchips.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+
+#include "timer-of.h"
+
+#define CMP_OFFSET	0x10000
+
+#define CNTCV_LO	0x8
+#define CNTCV_HI	0xc
+#define CMPCV_LO	(CMP_OFFSET + 0x20)
+#define CMPCV_HI	(CMP_OFFSET + 0x24)
+#define CMPCR		(CMP_OFFSET + 0x2c)
+
+#define SYS_CTR_EN		0x1
+#define SYS_CTR_IRQ_MASK	0x2
+
+static void __iomem *sys_ctr_base;
+
+static void sysctr_timer_enable(bool enable)
+{
+	u32 val;
+
+	val = readl(sys_ctr_base + CMPCR);
+	val &= ~SYS_CTR_EN;
+	if (enable)
+		val |= SYS_CTR_EN;
+
+	writel(val, sys_ctr_base + CMPCR);
+}
+
+static void sysctr_irq_acknowledge(void)
+{
+	/*
+	 * clear the enable bit(EN =0) will clear
+	 * the status bit(ISTAT = 0), then the interrupt
+	 * signal will be negated(acknowledged).
+	 */
+	sysctr_timer_enable(false);
+}
+
+static inline u64 sysctr_read_counter(void)
+{
+	u32 cnt_hi, tmp_hi, cnt_lo;
+
+	do {
+		cnt_hi = readl_relaxed(sys_ctr_base + CNTCV_HI);
+		cnt_lo = readl_relaxed(sys_ctr_base + CNTCV_LO);
+		tmp_hi = readl_relaxed(sys_ctr_base + CNTCV_HI);
+	} while (tmp_hi != cnt_hi);
+
+	return  ((u64) cnt_hi << 32) | cnt_lo;
+}
+
+static int sysctr_set_next_event(unsigned long delta,
+				 struct clock_event_device *evt)
+{
+	u32 cmp_hi, cmp_lo;
+	u64 next;
+
+	sysctr_timer_enable(false);
+
+	next = sysctr_read_counter();
+
+	next += delta;
+
+	cmp_hi = (next >> 32) & 0x00fffff;
+	cmp_lo = next & 0xffffffff;
+
+	writel_relaxed(cmp_hi, sys_ctr_base + CMPCV_HI);
+	writel_relaxed(cmp_lo, sys_ctr_base + CMPCV_LO);
+
+	sysctr_timer_enable(true);
+
+	return 0;
+}
+
+static int sysctr_set_state_oneshot(struct clock_event_device *evt)
+{
+	sysctr_timer_enable(true);
+
+	return 0;
+}
+
+static int sysctr_set_state_shutdown(struct clock_event_device *evt)
+{
+	sysctr_timer_enable(false);
+
+	return 0;
+}
+
+static irqreturn_t sysctr_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+
+	sysctr_irq_acknowledge();
+
+	evt->event_handler(evt);
+
+	return IRQ_HANDLED;
+}
+
+static struct timer_of to_sysctr = {
+	.flags = TIMER_OF_IRQ | TIMER_OF_CLOCK | TIMER_OF_BASE,
+	.clkevt = {
+		.name			= "i.MX system counter timer",
+		.features		= CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_DYNIRQ,
+		.set_state_oneshot	= sysctr_set_state_oneshot,
+		.set_next_event		= sysctr_set_next_event,
+		.set_state_shutdown	= sysctr_set_state_shutdown,
+		.rating			= 200,
+	},
+	.of_irq = {
+		.handler		= sysctr_timer_interrupt,
+		.flags			= IRQF_TIMER | IRQF_IRQPOLL,
+	},
+	.of_clk = {
+		.name = "per",
+	},
+};
+
+static void __init sysctr_clockevent_init(void)
+{
+	to_sysctr.clkevt.cpumask = cpumask_of(0);
+
+	clockevents_config_and_register(&to_sysctr.clkevt, timer_of_rate(&to_sysctr),
+					0xff, 0x7fffffff);
+}
+
+static int __init sysctr_timer_init(struct device_node *np)
+{
+	int ret = 0;
+
+	ret = timer_of_init(np, &to_sysctr);
+	if (ret)
+		return ret;
+
+	sys_ctr_base = timer_of_base(&to_sysctr);
+
+	sysctr_clockevent_init();
+
+	return 0;
+}
+TIMER_OF_DECLARE(sysctr_timer, "nxp,sysctr-timer", sysctr_timer_init);
-- 
1.9.1

Re: [PATCH v3 2/2] driver: clocksource: Add nxp system counter timer driver support

[Thomas Gleixner]

On Wed, 3 Apr 2019, Jacky Bai wrote:

> From: Bai Ping <ping.bai@nxp.com>
> 
> The system counter (sys_ctr) is a programmable system counter
> which provides a shared time base to the Cortex A15, A7, A53 etc cores.
> It is intended for use in applications where the counter is always
> powered on and supports multiple, unrelated clocks. The sys_ctr hardware
> supports:
>  - 56-bit counter width (roll-over time greater than 40 years)
>  - compare frame(64-bit compare value) contains programmable interrupt
>    generation

I hope that's a <= compare and not a == ....

> +static void sysctr_timer_enable(bool enable)
> +{
> +	u32 val;
> +
> +	val = readl(sys_ctr_base + CMPCR);
> +	val &= ~SYS_CTR_EN;
> +	if (enable)
> +		val |= SYS_CTR_EN;
> +
> +	writel(val, sys_ctr_base + CMPCR);

This read is really just overhead. Why aren't you caching the control
register value? It's not a self modifying register and I don't see
concurrency here either.

> +}
> +
> +static void sysctr_irq_acknowledge(void)
> +{
> +	/*
> +	 * clear the enable bit(EN =0) will clear
> +	 * the status bit(ISTAT = 0), then the interrupt
> +	 * signal will be negated(acknowledged).
> +	 */
> +	sysctr_timer_enable(false);
> +}
> +
> +static inline u64 sysctr_read_counter(void)
> +{
> +	u32 cnt_hi, tmp_hi, cnt_lo;
> +
> +	do {
> +		cnt_hi = readl_relaxed(sys_ctr_base + CNTCV_HI);
> +		cnt_lo = readl_relaxed(sys_ctr_base + CNTCV_LO);
> +		tmp_hi = readl_relaxed(sys_ctr_base + CNTCV_HI);
> +	} while (tmp_hi != cnt_hi);

When will hardware people finally get it? Is it so damned hard to make the
readout do:

	lo = read_lo()		-> internally latches HI in hardware
	hi = read_hi()		-> reads the latched value

It's not rocket science, but it would spare these horrible read loops. But
sure, performance happens in whitepapers and marketing slides ....

> +
> +	return  ((u64) cnt_hi << 32) | cnt_lo;
> +}
> +
> +static int sysctr_set_next_event(unsigned long delta,
> +				 struct clock_event_device *evt)
> +{
> +	u32 cmp_hi, cmp_lo;
> +	u64 next;
> +
> +	sysctr_timer_enable(false);
> +
> +	next = sysctr_read_counter();
> +
> +	next += delta;
> +
> +	cmp_hi = (next >> 32) & 0x00fffff;
> +	cmp_lo = next & 0xffffffff;
> +
> +	writel_relaxed(cmp_hi, sys_ctr_base + CMPCV_HI);
> +	writel_relaxed(cmp_lo, sys_ctr_base + CMPCV_LO);

Please document that this is a <= comparator. If that's not true then this
function is broken for small deltas and delays between read_counter() and
enable.

> +
> +	sysctr_timer_enable(true);
> +
> +	return 0;
> +}
> +
> +static int sysctr_set_state_oneshot(struct clock_event_device *evt)
> +{
> +	sysctr_timer_enable(true);

That's wrong. Why do you want to enable the timer here? When the state is
set to one shot then the next operation is set_next_event() but before that
nothing should ever come out of the timer.

Thanks,

	tglx

Re: [PATCH v3 1/2] dt-bindings: timer: Add binding doc for nxp system counter timer

[Rob Herring]

On Wed, 3 Apr 2019 10:20:25 +0000, Jacky Bai wrote:
> From: Bai Ping <ping.bai@nxp.com>
> 
> Add the binding doc for nxp system counter timer module.
> 
> Signed-off-by: Bai Ping <ping.bai@nxp.com>
> ---
> change v1->v2
>  - remove the blank line at EOF
> change v2->v3
>  - update the binding example based on the driver change
> ---
>  .../devicetree/bindings/timer/nxp,sysctr-timer.txt | 25 ++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 2/2] driver: clocksource: Add nxp system counter timer driver support

[Jacky Bai]

Sorry for delayed response to you, my mail client did something wrong. :(

> On Wed, 3 Apr 2019, Jacky Bai wrote:
> 
> > From: Bai Ping <ping.bai@nxp.com>
> >
> > The system counter (sys_ctr) is a programmable system counter which
> > provides a shared time base to the Cortex A15, A7, A53 etc cores.
> > It is intended for use in applications where the counter is always
> > powered on and supports multiple, unrelated clocks. The sys_ctr
> > hardware
> > supports:
> >  - 56-bit counter width (roll-over time greater than 40 years)
> >  - compare frame(64-bit compare value) contains programmable interrupt
> >    generation
> 
> I hope that's a <= compare and not a == ....
> 

Yes, it is <= compare, when the free running counter value >= the compare value, then interrupt is triggered.

> > +static void sysctr_timer_enable(bool enable) {
> > +     u32 val;
> > +
> > +     val = readl(sys_ctr_base + CMPCR);
> > +     val &= ~SYS_CTR_EN;
> > +     if (enable)
> > +             val |= SYS_CTR_EN;
> > +
> > +     writel(val, sys_ctr_base + CMPCR);
> 
> This read is really just overhead. Why aren't you caching the control register
> value? It's not a self modifying register and I don't see concurrency here
> either.
> 

Thanks, I will use a cached value for it.

> > +}
> > +
> > +static void sysctr_irq_acknowledge(void) {
> > +     /*
> > +      * clear the enable bit(EN =0) will clear
> > +      * the status bit(ISTAT = 0), then the interrupt
> > +      * signal will be negated(acknowledged).
> > +      */
> > +     sysctr_timer_enable(false);
> > +}
> > +
> > +static inline u64 sysctr_read_counter(void) {
> > +     u32 cnt_hi, tmp_hi, cnt_lo;
> > +
> > +     do {
> > +             cnt_hi = readl_relaxed(sys_ctr_base + CNTCV_HI);
> > +             cnt_lo = readl_relaxed(sys_ctr_base + CNTCV_LO);
> > +             tmp_hi = readl_relaxed(sys_ctr_base + CNTCV_HI);
> > +     } while (tmp_hi != cnt_hi);
> 
> When will hardware people finally get it? Is it so damned hard to make the
> readout do:
> 
>         lo = read_lo()          -> internally latches HI in hardware
>         hi = read_hi()          -> reads the latched value
> 
> It's not rocket science, but it would spare these horrible read loops. But sure,
> performance happens in whitepapers and marketing slides ....
> 

Sadly, hardware people don't implement such internal latch logic, so we need to use such read loop.

> > +
> > +     return  ((u64) cnt_hi << 32) | cnt_lo; }
> > +
> > +static int sysctr_set_next_event(unsigned long delta,
> > +                              struct clock_event_device *evt) {
> > +     u32 cmp_hi, cmp_lo;
> > +     u64 next;
> > +
> > +     sysctr_timer_enable(false);
> > +
> > +     next = sysctr_read_counter();
> > +
> > +     next += delta;
> > +
> > +     cmp_hi = (next >> 32) & 0x00fffff;
> > +     cmp_lo = next & 0xffffffff;
> > +
> > +     writel_relaxed(cmp_hi, sys_ctr_base + CMPCV_HI);
> > +     writel_relaxed(cmp_lo, sys_ctr_base + CMPCV_LO);
> 
> Please document that this is a <= comparator. If that's not true then this
> function is broken for small deltas and delays between read_counter() and
> enable.

It is <= comparator, so it is safe.

> 
> > +
> > +     sysctr_timer_enable(true);
> > +
> > +     return 0;
> > +}
> > +
> > +static int sysctr_set_state_oneshot(struct clock_event_device *evt) {
> > +     sysctr_timer_enable(true);
> 
> That's wrong. Why do you want to enable the timer here? When the state is
> set to one shot then the next operation is set_next_event() but before that
> nothing should ever come out of the timer.
> 

Thanks, I will remove it in V4.

BR
Jacky Bai

> Thanks,
> 
>         tglx