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

[Jacky Bai]

> -----Original Message-----
> From: Daniel Lezcano [mailto:daniel.lezcano@xxxxxxxxxx]
> Sent: Tuesday, May 21, 2019 6:08 PM
> To: Jacky Bai <ping.bai@xxxxxxx>; tglx@xxxxxxxxxxxxx; robh+dt@xxxxxxxxxx;
> shawnguo@xxxxxxxxxx; mark.rutland@xxxxxxx; Aisheng Dong
> <aisheng.dong@xxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; dl-linux-imx
> <linux-imx@xxxxxxx>
> Subject: Re: [PATCH v4 2/2] driver: clocksource: Add nxp system counter timer
> driver support
> 
> On 21/05/2019 09:18, Jacky Bai wrote:
> > From: Bai Ping <ping.bai@xxxxxxx>
> >
> > 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)
> 
> The benefit of using more than 32bits on a 32bits system is not proven.
> 

It is mainly used on 64bit ARMv8 system.

> The function to read and build the 56bits value can have a very significant
> impact on the performance of your platform.
> 
> Using a 32bits counter can be enough if it does not wrap too fast.
> 
> Can you consider a 32 bits counter ?

this counter is ARMv8 arch timer's counter source. As it also has timer function, so I choose it
to act as a broadcast timer for cpuidle. The timer interrupt can only be triggered when 'compare[55:0] <= counter[55:0]'.
So you mean that only use the lower 32bit to implement this timer? If so, I can change to use only the lower 32bit.

> 
> >  - compare frame(64-bit compare value) contains programmable interrupt
> >    generation when compare value <= counter value.
> >
> > Signed-off-by: Bai Ping <ping.bai@xxxxxxx>
> > ---
> > 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.
> > change v3->v4:
> >  - use cached value for CMPCR,
> >  - remove unnecessary timer enabe from set_state_oneshot function.
> > ---
> >  drivers/clocksource/Kconfig            |   7 ++
> >  drivers/clocksource/Makefile           |   1 +
> >  drivers/clocksource/timer-imx-sysctr.c | 146
> > +++++++++++++++++++++++++
> >  3 files changed, 154 insertions(+)
> >  create mode 100644 drivers/clocksource/timer-imx-sysctr.c
> >
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index 6bcaa4e2e72c..ee48620a4561 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -616,6 +616,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
> 
> Do you really need this dep?
> 

Not really necessary, I can remove it.

> > +	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 236858fa7fbf..5fba39e81a40 100644
> > --- a/drivers/clocksource/Makefile
> > +++ b/drivers/clocksource/Makefile
> > @@ -74,6 +74,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 000000000000..d0428d3189f8
> > --- /dev/null
> > +++ b/drivers/clocksource/timer-imx-sysctr.c
> > @@ -0,0 +1,146 @@
> > +// 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 u32 cmpcr;
> > +
> > +static void sysctr_timer_enable(bool enable) {
> > +	cmpcr &= ~SYS_CTR_EN;
> 
> Do the computation after reading the value in the init function...
> 
> > +	if (enable)
> > +		cmpcr |= SYS_CTR_EN;
> 
> ... then
> 
> writel(enable ? cmpcr | SYS_CTR_EN : cmpcr, sys_ctr_base);
> 

Ok, thank you.

BR
Jacky Bai

> > +	writel(cmpcr, 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) {
> > +	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);
> > +	cmpcr = readl(sys_ctr_base + CMPCR);
> > +
> > +	sysctr_clockevent_init();
> > +
> > +	return 0;
> > +}
> > +TIMER_OF_DECLARE(sysctr_timer, "nxp,sysctr-timer",
> > +sysctr_timer_init);
> >
> 
> 
> --