diff mbox series

[v2,1/2] Doc: bindings: Add binding doc for nxp system counter timer

Message ID 1544592770-19996-1-git-send-email-ping.bai@nxp.com
State Changes Requested, archived
Headers show
Series [v2,1/2] Doc: bindings: Add binding doc for nxp system counter timer | expand

Checks

Context Check Description
robh/checkpatch warning "total: 0 errors, 1 warnings, 26 lines checked"

Commit Message

Jacky Bai Dec. 12, 2018, 5:28 a.m. UTC 
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
---
 .../devicetree/bindings/timer/nxp,sysctr_timer.txt | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/nxp,sysctr_timer.txt

Comments

Randy Dunlap Dec. 12, 2018, 6:48 p.m. UTC | #1 
Hi,

On 12/11/18 9:28 PM, Jacky Bai wrote:
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index c57b156..7e5f2de 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -579,6 +579,14 @@ config CLKSRC_IMX_TPM
>  	  Enable this option to use IMX Timer/PWM Module (TPM) timer as
>  	  clocksource.
>  
> +config CLKSRC_IMX_SYS_CTR
> +	bool "Clocksource using i.MX system counter" if COMPILE_TEST

Looks like a "make *config" user only gets this prompt if COMPILE_TEST is set.

If COMPILE_TEST is not set, how does this driver become enabled/set?


> +	depends on (ARM || ARM64) && CLKDEV_LOOKUP
> +	select CLKSRC_MMIO
> +	help
> +	  Enable this option to use IMX system counter timer as
> +	  clocksource.
> +
>  config CLKSRC_ST_LPC
>  	bool "Low power clocksource found in the LPC" if COMPILE_TEST
>  	select TIMER_OF if OF

thanks,
Rob Herring Dec. 18, 2018, 5:03 p.m. UTC | #2 
On Wed, Dec 12, 2018 at 05:28:23AM +0000, Jacky Bai wrote:
> 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
> ---
>  .../devicetree/bindings/timer/nxp,sysctr_timer.txt | 26 ++++++++++++++++++++++
>  1 file changed, 26 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..21f1527
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/nxp,sysctr_timer.txt
> @@ -0,0 +1,26 @@
> +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.
> +
> +Required properties:
> +
> +- compatible :      should be "nxp,sysctr-timer"

Only 1 version? Should be SoC specific.

> +- reg :             Specifies the base physical address and size of the comapre

s/comapre/compare/

> +                    frame and the counter control, read & compare.
> +- interrupts :      should be the compare frames' interrupt
> +- clock-frequency : Specifies the counter clock frequency.
> +
> +Example:
> +
> +	system_counter: timer@306a0000 {
> +		compatible = "nxp,sysctr-timer";
> +		reg = <0x0 0x306a0000 0x0 0x10000>, /* system-counter-rd base */
> +		      <0x0 0x306b0000 0x0 0x10000>, /* system-counter-cmp base */
> +		      <0x0 0x306c0000 0x0 0x10000>; /* system-counter-ctrl base */

Are these really 64K in length? That wastes virtual space which can be 
important on 32-bit systems.

> +		clock-frequency = <8000000>;
> +		interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>;
> +	};
> -- 
> 1.9.1
>
Jacky Bai Jan. 8, 2019, 9:33 a.m. UTC | #3 
> Subject: Re: [PATCH v2 2/2] driver: clocksource: Add nxp system counter timer
> driver support
> 
> Hi,
> 
> On 12/11/18 9:28 PM, Jacky Bai wrote:
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index c57b156..7e5f2de 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -579,6 +579,14 @@ config CLKSRC_IMX_TPM
> >  	  Enable this option to use IMX Timer/PWM Module (TPM) timer as
> >  	  clocksource.
> >
> > +config CLKSRC_IMX_SYS_CTR
> > +	bool "Clocksource using i.MX system counter" if COMPILE_TEST
> 
> Looks like a "make *config" user only gets this prompt if COMPILE_TEST is set.
> 
> If COMPILE_TEST is not set, how does this driver become enabled/set?
> 
> 

Sorry for reply to you so later. this driver need to be enable explicitly when IMX soc is built.

BR
> > +	depends on (ARM || ARM64) && CLKDEV_LOOKUP
> > +	select CLKSRC_MMIO
> > +	help
> > +	  Enable this option to use IMX system counter timer as
> > +	  clocksource.
> > +
> >  config CLKSRC_ST_LPC
> >  	bool "Low power clocksource found in the LPC" if COMPILE_TEST
> >  	select TIMER_OF if OF
> 
> thanks,
> --
> ~Randy
Jacky Bai Jan. 14, 2019, 6:16 a.m. UTC | #4 
Ping...

BR
Jacky Bai
> -----Original Message-----
> From: Jacky Bai
> Sent: 2018年12月12日 13:28
> To: daniel.lezcano@linaro.org; tglx@linutronix.de; robh+dt@kernel.org;
> shawnguo@kernel.org; mark.rutland@arm.com; Aisheng Dong
> <aisheng.dong@nxp.com>
> Cc: linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: [PATCH v2 2/2] driver: clocksource: Add nxp system counter timer
> driver support
> 
> 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
> ---
>  drivers/clocksource/Kconfig            |   8 ++
>  drivers/clocksource/Makefile           |   1 +
>  drivers/clocksource/timer-imx-sysctr.c | 204
> +++++++++++++++++++++++++++++++++
>  3 files changed, 213 insertions(+)
>  create mode 100644 drivers/clocksource/timer-imx-sysctr.c
> 
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig index
> c57b156..7e5f2de 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -579,6 +579,14 @@ config CLKSRC_IMX_TPM
>  	  Enable this option to use IMX Timer/PWM Module (TPM) timer as
>  	  clocksource.
> 
> +config CLKSRC_IMX_SYS_CTR
> +	bool "Clocksource using i.MX system counter" if COMPILE_TEST
> +	depends on (ARM || ARM64) && CLKDEV_LOOKUP
> +	select CLKSRC_MMIO
> +	help
> +	  Enable this option to use IMX system counter timer as
> +	  clocksource.
> +
>  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 dd91381..372bf4e 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -71,6 +71,7 @@ obj-$(CONFIG_CLKSRC_MIPS_GIC)		+=
> mips-gic-timer.o
>  obj-$(CONFIG_CLKSRC_TANGO_XTAL)		+= tango_xtal.o
>  obj-$(CONFIG_CLKSRC_IMX_GPT)		+= timer-imx-gpt.o
>  obj-$(CONFIG_CLKSRC_IMX_TPM)		+= timer-imx-tpm.o
> +obj-$(CONFIG_CLKSRC_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..ad3c27f
> --- /dev/null
> +++ b/drivers/clocksource/timer-imx-sysctr.c
> @@ -0,0 +1,204 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +//
> +// Copyright 2017-2018 NXP
> +
> +#include <linux/interrupt.h>
> +#include <linux/clockchips.h>
> +#include <linux/clocksource.h>
> +#include <linux/delay.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/sched_clock.h>
> +
> +#define CNTCV_LO	0x8
> +#define CNTCV_HI	0xc
> +#define CMPCV_LO	0x20
> +#define CMPCV_HI	0x24
> +#define CMPCR		0x2c
> +
> +#define SYS_CTR_EN		0x1
> +#define SYS_CTR_IRQ_MASK	0x2
> +
> +static void __iomem *sys_ctr_rd_base;
> +static void __iomem *sys_ctr_cmp_base;
> +static struct clock_event_device clockevent_sysctr;
> +
> +static inline void sysctr_timer_enable(bool enable) {
> +	u32 val;
> +
> +	val = readl(sys_ctr_cmp_base + CMPCR);
> +	val &= ~SYS_CTR_EN;
> +	if (enable)
> +		val |= SYS_CTR_EN;
> +
> +	writel(val, sys_ctr_cmp_base + CMPCR); }
> +
> +static void sysctr_irq_acknowledge(void) {
> +	u32 val;
> +
> +	/* clear th enable bit(EN=0) to clear the ISTAT */
> +	val = readl(sys_ctr_cmp_base + CMPCR);
> +	val &= ~SYS_CTR_EN;
> +	writel(val, sys_ctr_cmp_base + CMPCR); }
> +
> +static inline u64 sysctr_read_counter(void) {
> +	u32 cnt_hi, tmp_hi, cnt_lo;
> +
> +	do {
> +		cnt_hi = readl_relaxed(sys_ctr_rd_base + CNTCV_HI);
> +		cnt_lo = readl_relaxed(sys_ctr_rd_base + CNTCV_LO);
> +		tmp_hi = readl_relaxed(sys_ctr_rd_base + CNTCV_HI);
> +	} while (tmp_hi != cnt_hi);
> +
> +	return  ((u64) cnt_hi << 32) | cnt_lo; }
> +
> +static u64 notrace sysctr_read_sched_clock(void) {
> +	return sysctr_read_counter();
> +}
> +
> +static u64 sysctr_clocksource_read(struct clocksource *cs) {
> +	return sysctr_read_counter();
> +}
> +
> +static int __init sysctr_clocksource_init(unsigned int rate) {
> +	sched_clock_register(sysctr_read_sched_clock, 56, rate);
> +	return clocksource_mmio_init(sys_ctr_rd_base, "i.MX sys_ctr",
> +				     rate, 200, 56, sysctr_clocksource_read); }
> +
> +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_cmp_base + CMPCV_HI);
> +	writel_relaxed(cmp_lo, sys_ctr_cmp_base + CMPCV_LO);
> +
> +	sysctr_timer_enable(true);
> +
> +	return 0;
> +}
> +
> +static int sysctr_set_state_oneshot(struct clock_event_device *evt) {
> +	/* enable timer */
> +	sysctr_timer_enable(true);
> +
> +	return 0;
> +}
> +
> +static int sysctr_set_state_shutdown(struct clock_event_device *evt) {
> +	/* disable the timer */
> +	sysctr_timer_enable(false);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t sysctr_timer_interrupt(int irq, void *dev_id) {
> +	struct clock_event_device *evt = &clockevent_sysctr;
> +
> +	sysctr_irq_acknowledge();
> +
> +	evt->event_handler(evt);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static struct clock_event_device clockevent_sysctr = {
> +	.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,
> +};
> +
> +static int __init sysctr_clockevent_init(unsigned long rate, int irq) {
> +	int ret;
> +
> +	ret = request_irq(irq, sysctr_timer_interrupt, IRQF_TIMER |
> IRQF_IRQPOLL,
> +		    "i.MX system counter timer", &clockevent_sysctr);
> +	if (ret) {
> +		pr_err("Failed to request i.MX sysctr timer irq\n");
> +		return ret;
> +	}
> +
> +	clockevent_sysctr.cpumask = cpumask_of(0);
> +	clockevent_sysctr.irq = irq;
> +	clockevents_config_and_register(&clockevent_sysctr,
> +			rate, 0xff, 0x7fffffff);
> +
> +	return 0;
> +}
> +
> +static int __init sysctr_timer_init(struct device_node *np) {
> +	u32 rate;
> +	int irq, ret = 0;
> +
> +	/* map the system counter's CNTreadbase */
> +	sys_ctr_rd_base = of_iomap(np, 0);
> +	if (!sys_ctr_rd_base) {
> +		pr_err("Failed to map sys_ctr rd base%pOF\n", np);
> +		return -ENXIO;
> +	}
> +
> +	/* map the system counter's CNTcomparebase */
> +	sys_ctr_cmp_base = of_iomap(np, 1);
> +	if (!sys_ctr_cmp_base) {
> +		pr_err("Failed to map sys_ctr compare base%pOF\n", np);
> +		ret = -ENXIO;
> +		goto out_free2;
> +	}
> +
> +	/*
> +	 * the purpose of this driver is to provide a global timer,
> +	 * So only use one compare frame, request frame0's irq only.
> +	 */
> +	irq = irq_of_parse_and_map(np, 0);
> +	if (!irq) {
> +		pr_err("Failed to map interrupt for %pOF\n", np);
> +		ret = -EINVAL;
> +		goto out_free1;
> +	}
> +
> +	if (of_property_read_u32(np, "clock-frequency", &rate)) {
> +		pr_err("Failed to get clock frequency %pOF\n", np);
> +		ret = -EINVAL;
> +		goto out_free1;
> +	}
> +
> +	sysctr_clocksource_init(rate);
> +	sysctr_clockevent_init(rate, irq);
> +
> +	return 0;
> +
> +out_free1:
> +	iounmap(sys_ctr_cmp_base);
> +out_free2:
> +	iounmap(sys_ctr_rd_base);
> +	return ret;
> +}
> +TIMER_OF_DECLARE(sysctr_timer, "nxp,sysctr-timer", sysctr_timer_init);
> --
> 1.9.1
Daniel Lezcano Jan. 14, 2019, 3:45 p.m. UTC | #5 
On 12/12/2018 06:28, Jacky Bai wrote:
> 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 
> ---
>  drivers/clocksource/Kconfig            |   8 ++
>  drivers/clocksource/Makefile           |   1 +
>  drivers/clocksource/timer-imx-sysctr.c | 204 +++++++++++++++++++++++++++++++++
>  3 files changed, 213 insertions(+)
>  create mode 100644 drivers/clocksource/timer-imx-sysctr.c
> 
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index c57b156..7e5f2de 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -579,6 +579,14 @@ config CLKSRC_IMX_TPM
>  	  Enable this option to use IMX Timer/PWM Module (TPM) timer as
>  	  clocksource.
>  
> +config CLKSRC_IMX_SYS_CTR

As the driver is a timer, please replace the CLKSRC by TIMER

> +	bool "Clocksource using i.MX system counter" if COMPILE_TEST
> +	depends on (ARM || ARM64) && CLKDEV_LOOKUP

CLKDEV_LOOKUP is not needed, it is selected with COMMON_CLK

> +	select CLKSRC_MMIO
> +	help
> +	  Enable this option to use IMX system counter timer as
> +	  clocksource.
> +
>  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 dd91381..372bf4e 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -71,6 +71,7 @@ obj-$(CONFIG_CLKSRC_MIPS_GIC)		+= mips-gic-timer.o
>  obj-$(CONFIG_CLKSRC_TANGO_XTAL)		+= tango_xtal.o
>  obj-$(CONFIG_CLKSRC_IMX_GPT)		+= timer-imx-gpt.o
>  obj-$(CONFIG_CLKSRC_IMX_TPM)		+= timer-imx-tpm.o
> +obj-$(CONFIG_CLKSRC_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..ad3c27f
> --- /dev/null
> +++ b/drivers/clocksource/timer-imx-sysctr.c
> @@ -0,0 +1,204 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +//
> +// Copyright 2017-2018 NXP
> +
> +#include <linux/interrupt.h>
> +#include <linux/clockchips.h>
> +#include <linux/clocksource.h>
> +#include <linux/delay.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/sched_clock.h>
> +
> +#define CNTCV_LO	0x8
> +#define CNTCV_HI	0xc
> +#define CMPCV_LO	0x20
> +#define CMPCV_HI	0x24
> +#define CMPCR		0x2c
> +
> +#define SYS_CTR_EN		0x1
> +#define SYS_CTR_IRQ_MASK	0x2
> +
> +static void __iomem *sys_ctr_rd_base;
> +static void __iomem *sys_ctr_cmp_base;
> +static struct clock_event_device clockevent_sysctr;
> +
> +static inline void sysctr_timer_enable(bool enable)

Remove 'inline' tag

> +{
> +	u32 val;
> +
> +	val = readl(sys_ctr_cmp_base + CMPCR);
> +	val &= ~SYS_CTR_EN;
> +	if (enable)
> +		val |= SYS_CTR_EN;
> +
> +	writel(val, sys_ctr_cmp_base + CMPCR);
> +}
> +
> +static void sysctr_irq_acknowledge(void)
> +{
> +	u32 val;
> +
> +	/* clear th enable bit(EN=0) to clear the ISTAT */

What is the ISTAT?

Thanks for adding comments, however the format is:

/*
 *
 */

> +	val = readl(sys_ctr_cmp_base + CMPCR);
> +	val &= ~SYS_CTR_EN;
> +	writel(val, sys_ctr_cmp_base + CMPCR);

This is duplicate with the sysctr_timer_enable() function, call it
instead of duplicating code.

> +}
> +
> +static inline u64 sysctr_read_counter(void)
> +{
> +	u32 cnt_hi, tmp_hi, cnt_lo;
> +
> +	do {
> +		cnt_hi = readl_relaxed(sys_ctr_rd_base + CNTCV_HI);
> +		cnt_lo = readl_relaxed(sys_ctr_rd_base + CNTCV_LO);
> +		tmp_hi = readl_relaxed(sys_ctr_rd_base + CNTCV_HI);
> +	} while (tmp_hi != cnt_hi);
> +
> +	return  ((u64) cnt_hi << 32) | cnt_lo;
> +}

It has been showed a 64bit counter usage adds a non negligible overhead,
especially on 32bits. Are you really sure you want this ?

> +static u64 notrace sysctr_read_sched_clock(void)
> +{
> +	return sysctr_read_counter();
> +}
> +
> +static u64 sysctr_clocksource_read(struct clocksource *cs)
> +{
> +	return sysctr_read_counter();
> +}
> +
> +static int __init sysctr_clocksource_init(unsigned int rate)
> +{
> +	sched_clock_register(sysctr_read_sched_clock, 56, rate);
> +	return clocksource_mmio_init(sys_ctr_rd_base, "i.MX sys_ctr",
> +				     rate, 200, 56, sysctr_clocksource_read);
> +}
> +
> +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_cmp_base + CMPCV_HI);
> +	writel_relaxed(cmp_lo, sys_ctr_cmp_base + CMPCV_LO);
> +
> +	sysctr_timer_enable(true);
> +
> +	return 0;
> +}
> +
> +static int sysctr_set_state_oneshot(struct clock_event_device *evt)
> +{
> +	/* enable timer */

The function name is explicit enough, no need to add this comment

> +	sysctr_timer_enable(true);
> +
> +	return 0;
> +}
> +
> +static int sysctr_set_state_shutdown(struct clock_event_device *evt)
> +{
> +	/* disable the timer */

Ditto

> +	sysctr_timer_enable(false);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t sysctr_timer_interrupt(int irq, void *dev_id)
> +{
> +	struct clock_event_device *evt = &clockevent_sysctr;
> +
> +	sysctr_irq_acknowledge();
> +
> +	evt->event_handler(evt);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static struct clock_event_device clockevent_sysctr = {
> +	.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,
> +};
> +
> +static int __init sysctr_clockevent_init(unsigned long rate, int irq)
> +{
> +	int ret;
> +
> +	ret = request_irq(irq, sysctr_timer_interrupt, IRQF_TIMER | IRQF_IRQPOLL,
> +		    "i.MX system counter timer", &clockevent_sysctr);
> +	if (ret) {
> +		pr_err("Failed to request i.MX sysctr timer irq\n");
> +		return ret;
> +	}
> +
> +	clockevent_sysctr.cpumask = cpumask_of(0);
> +	clockevent_sysctr.irq = irq;
> +	clockevents_config_and_register(&clockevent_sysctr,
> +			rate, 0xff, 0x7fffffff);
> +	return 0;
> +}
> +
> +static int __init sysctr_timer_init(struct device_node *np)
> +{
> +	u32 rate;
> +	int irq, ret = 0;
> +
> +	/* map the system counter's CNTreadbase */

Comment not useful, remove it

> +	sys_ctr_rd_base = of_iomap(np, 0);
> +	if (!sys_ctr_rd_base) {
> +		pr_err("Failed to map sys_ctr rd base%pOF\n", np);

nit: space missing after 'base'

> +		return -ENXIO;
> +	}
> +
> +	/* map the system counter's CNTcomparebase */

Remove comment

> +	sys_ctr_cmp_base = of_iomap(np, 1);
> +	if (!sys_ctr_cmp_base) {
> +		pr_err("Failed to map sys_ctr compare base%pOF\n", np);
> +		ret = -ENXIO;
> +		goto out_free2;
> +	}
> +
> +	/*
> +	 * the purpose of this driver is to provide a global timer,
> +	 * So only use one compare frame, request frame0's irq only.
> +	 */
> +	irq = irq_of_parse_and_map(np, 0);
> +	if (!irq) {
> +		pr_err("Failed to map interrupt for %pOF\n", np);
> +		ret = -EINVAL;
> +		goto out_free1;
> +	}
> +
> +	if (of_property_read_u32(np, "clock-frequency", &rate)) {
> +		pr_err("Failed to get clock frequency %pOF\n", np);
> +		ret = -EINVAL;
> +		goto out_free1;
> +	}

Please replace it with a 'fixed-clock' in the DT and use the clk API.

In addition you can use the timer-of API (cf timer-of.h)

> +
> +	sysctr_clocksource_init(rate);
> +	sysctr_clockevent_init(rate, irq);
> +
> +	return 0;
> +
> +out_free1:

label names are inadequate, use out_iounmap_cmp: for example

> +	iounmap(sys_ctr_cmp_base);
> +out_free2:
> +	iounmap(sys_ctr_rd_base);
> +	return ret;
> +}
> +TIMER_OF_DECLARE(sysctr_timer, "nxp,sysctr-timer", sysctr_timer_init);
>
Jacky Bai Jan. 15, 2019, 2:49 a.m. UTC | #6 
> Subject: Re: [PATCH v2 2/2] driver: clocksource: Add nxp system counter timer
> driver support
> 
> On 12/12/2018 06:28, Jacky Bai wrote:
> > 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
> > ---
> >  drivers/clocksource/Kconfig            |   8 ++
> >  drivers/clocksource/Makefile           |   1 +
> >  drivers/clocksource/timer-imx-sysctr.c | 204
> > +++++++++++++++++++++++++++++++++
> >  3 files changed, 213 insertions(+)
> >  create mode 100644 drivers/clocksource/timer-imx-sysctr.c
> >
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index c57b156..7e5f2de 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -579,6 +579,14 @@ config CLKSRC_IMX_TPM
> >  	  Enable this option to use IMX Timer/PWM Module (TPM) timer as
> >  	  clocksource.
> >
> > +config CLKSRC_IMX_SYS_CTR
> 
> As the driver is a timer, please replace the CLKSRC by TIMER

Ok, will fix it.

> 
> > +	bool "Clocksource using i.MX system counter" if COMPILE_TEST
> > +	depends on (ARM || ARM64) && CLKDEV_LOOKUP
> 
> CLKDEV_LOOKUP is not needed, it is selected with COMMON_CLK
> 
> > +	select CLKSRC_MMIO
> > +	help
> > +	  Enable this option to use IMX system counter timer as
> > +	  clocksource.
> > +
> >  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 dd91381..372bf4e 100644
> > --- a/drivers/clocksource/Makefile
> > +++ b/drivers/clocksource/Makefile
> > @@ -71,6 +71,7 @@ obj-$(CONFIG_CLKSRC_MIPS_GIC)		+=
> mips-gic-timer.o
> >  obj-$(CONFIG_CLKSRC_TANGO_XTAL)		+= tango_xtal.o
> >  obj-$(CONFIG_CLKSRC_IMX_GPT)		+= timer-imx-gpt.o
> >  obj-$(CONFIG_CLKSRC_IMX_TPM)		+= timer-imx-tpm.o
> > +obj-$(CONFIG_CLKSRC_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..ad3c27f
> > --- /dev/null
> > +++ b/drivers/clocksource/timer-imx-sysctr.c
> > @@ -0,0 +1,204 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +//
> > +// Copyright 2017-2018 NXP
> > +
> > +#include <linux/interrupt.h>
> > +#include <linux/clockchips.h>
> > +#include <linux/clocksource.h>
> > +#include <linux/delay.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/sched_clock.h>
> > +
> > +#define CNTCV_LO	0x8
> > +#define CNTCV_HI	0xc
> > +#define CMPCV_LO	0x20
> > +#define CMPCV_HI	0x24
> > +#define CMPCR		0x2c
> > +
> > +#define SYS_CTR_EN		0x1
> > +#define SYS_CTR_IRQ_MASK	0x2
> > +
> > +static void __iomem *sys_ctr_rd_base; static void __iomem
> > +*sys_ctr_cmp_base; static struct clock_event_device
> > +clockevent_sysctr;
> > +
> > +static inline void sysctr_timer_enable(bool enable)
> 
> Remove 'inline' tag
> 

OK.

> > +{
> > +	u32 val;
> > +
> > +	val = readl(sys_ctr_cmp_base + CMPCR);
> > +	val &= ~SYS_CTR_EN;
> > +	if (enable)
> > +		val |= SYS_CTR_EN;
> > +
> > +	writel(val, sys_ctr_cmp_base + CMPCR); }
> > +
> > +static void sysctr_irq_acknowledge(void) {
> > +	u32 val;
> > +
> > +	/* clear th enable bit(EN=0) to clear the ISTAT */
> 
> What is the ISTAT?
> 
> Thanks for adding comments, however the format is:
> 
> /*
>  *
>  */
> 

Th ISTAT is interrupt status bit. I will fix it.

> > +	val = readl(sys_ctr_cmp_base + CMPCR);
> > +	val &= ~SYS_CTR_EN;
> > +	writel(val, sys_ctr_cmp_base + CMPCR);
> 
> This is duplicate with the sysctr_timer_enable() function, call it instead of
> duplicating code.
> 
Sure.

> > +}
> > +
> > +static inline u64 sysctr_read_counter(void) {
> > +	u32 cnt_hi, tmp_hi, cnt_lo;
> > +
> > +	do {
> > +		cnt_hi = readl_relaxed(sys_ctr_rd_base + CNTCV_HI);
> > +		cnt_lo = readl_relaxed(sys_ctr_rd_base + CNTCV_LO);
> > +		tmp_hi = readl_relaxed(sys_ctr_rd_base + CNTCV_HI);
> > +	} while (tmp_hi != cnt_hi);
> > +
> > +	return  ((u64) cnt_hi << 32) | cnt_lo; }
> 
> It has been showed a 64bit counter usage adds a non negligible overhead,
> especially on 32bits. Are you really sure you want this ?
> 

This HW is designed as 56 bit counter & timer. when doing timer 'set_next_event',
The 56bit counter value must be read out, and then adding the delta value to set the
next event. Can you give me some suggestion on how to handle this?

> > +static u64 notrace sysctr_read_sched_clock(void) {
> > +	return sysctr_read_counter();
> > +}
> > +
> > +static u64 sysctr_clocksource_read(struct clocksource *cs) {
> > +	return sysctr_read_counter();
> > +}
> > +
> > +static int __init sysctr_clocksource_init(unsigned int rate) {
> > +	sched_clock_register(sysctr_read_sched_clock, 56, rate);
> > +	return clocksource_mmio_init(sys_ctr_rd_base, "i.MX sys_ctr",
> > +				     rate, 200, 56, sysctr_clocksource_read); }
> > +
> > +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_cmp_base + CMPCV_HI);
> > +	writel_relaxed(cmp_lo, sys_ctr_cmp_base + CMPCV_LO);
> > +
> > +	sysctr_timer_enable(true);
> > +
> > +	return 0;
> > +}
> > +
> > +static int sysctr_set_state_oneshot(struct clock_event_device *evt) {
> > +	/* enable timer */
> 
> The function name is explicit enough, no need to add this comment
> 

OK, will remove it in V3.

> > +	sysctr_timer_enable(true);
> > +
> > +	return 0;
> > +}
> > +
> > +static int sysctr_set_state_shutdown(struct clock_event_device *evt)
> > +{
> > +	/* disable the timer */
> 
> Ditto
> 
> > +	sysctr_timer_enable(false);
> > +
> > +	return 0;
> > +}
> > +
> > +static irqreturn_t sysctr_timer_interrupt(int irq, void *dev_id) {
> > +	struct clock_event_device *evt = &clockevent_sysctr;
> > +
> > +	sysctr_irq_acknowledge();
> > +
> > +	evt->event_handler(evt);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static struct clock_event_device clockevent_sysctr = {
> > +	.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,
> > +};
> > +
> > +static int __init sysctr_clockevent_init(unsigned long rate, int irq)
> > +{
> > +	int ret;
> > +
> > +	ret = request_irq(irq, sysctr_timer_interrupt, IRQF_TIMER |
> IRQF_IRQPOLL,
> > +		    "i.MX system counter timer", &clockevent_sysctr);
> > +	if (ret) {
> > +		pr_err("Failed to request i.MX sysctr timer irq\n");
> > +		return ret;
> > +	}
> > +
> > +	clockevent_sysctr.cpumask = cpumask_of(0);
> > +	clockevent_sysctr.irq = irq;
> > +	clockevents_config_and_register(&clockevent_sysctr,
> > +			rate, 0xff, 0x7fffffff);
> > +	return 0;
> > +}
> > +
> > +static int __init sysctr_timer_init(struct device_node *np) {
> > +	u32 rate;
> > +	int irq, ret = 0;
> > +
> > +	/* map the system counter's CNTreadbase */
> 
> Comment not useful, remove it
> 
> > +	sys_ctr_rd_base = of_iomap(np, 0);
> > +	if (!sys_ctr_rd_base) {
> > +		pr_err("Failed to map sys_ctr rd base%pOF\n", np);
> 
> nit: space missing after 'base'
> 
> > +		return -ENXIO;
> > +	}
> > +
> > +	/* map the system counter's CNTcomparebase */
> 
> Remove comment
> 
> > +	sys_ctr_cmp_base = of_iomap(np, 1);
> > +	if (!sys_ctr_cmp_base) {
> > +		pr_err("Failed to map sys_ctr compare base%pOF\n", np);
> > +		ret = -ENXIO;
> > +		goto out_free2;
> > +	}
> > +
> > +	/*
> > +	 * the purpose of this driver is to provide a global timer,
> > +	 * So only use one compare frame, request frame0's irq only.
> > +	 */
> > +	irq = irq_of_parse_and_map(np, 0);
> > +	if (!irq) {
> > +		pr_err("Failed to map interrupt for %pOF\n", np);
> > +		ret = -EINVAL;
> > +		goto out_free1;
> > +	}
> > +
> > +	if (of_property_read_u32(np, "clock-frequency", &rate)) {
> > +		pr_err("Failed to get clock frequency %pOF\n", np);
> > +		ret = -EINVAL;
> > +		goto out_free1;
> > +	}
> 
> Please replace it with a 'fixed-clock' in the DT and use the clk API.
> 
> In addition you can use the timer-of API (cf timer-of.h)
> 

Thanks, I will fix it in V3.

> > +
> > +	sysctr_clocksource_init(rate);
> > +	sysctr_clockevent_init(rate, irq);
> > +
> > +	return 0;
> > +
> > +out_free1:
> 
> label names are inadequate, use out_iounmap_cmp: for example
> 

Will fix it in V3.

BR

> > +	iounmap(sys_ctr_cmp_base);
> > +out_free2:
> > +	iounmap(sys_ctr_rd_base);
> > +	return ret;
> > +}
> > +TIMER_OF_DECLARE(sysctr_timer, "nxp,sysctr-timer",
> > +sysctr_timer_init);
> >
diff mbox series

Patch

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..21f1527
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/nxp,sysctr_timer.txt
@@ -0,0 +1,26 @@ 
+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.
+
+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 compare frames' interrupt
+- clock-frequency : Specifies the counter clock frequency.
+
+Example:
+
+	system_counter: timer@306a0000 {
+		compatible = "nxp,sysctr-timer";
+		reg = <0x0 0x306a0000 0x0 0x10000>, /* system-counter-rd base */
+		      <0x0 0x306b0000 0x0 0x10000>, /* system-counter-cmp base */
+		      <0x0 0x306c0000 0x0 0x10000>; /* system-counter-ctrl base */
+		clock-frequency = <8000000>;
+		interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>;
+	};