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 |
Context | Check | Description |
---|---|---|
robh/checkpatch | warning | "total: 0 errors, 1 warnings, 26 lines checked" |
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,
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 >
> 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
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
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); >
> 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 --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>; + };
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