Re: [PATCH 4/5] clocksource: add driver for i.MX EPIT timer
Hi Clément,
please find basic review comments below.
On 05/28/2018 08:34 PM, Clément Péron wrote:
> From: Colin Didier
>
> Add driver for NXP's EPIT timer used in i.MX 6 family of SoC.
>
The first author's signed-off-by tag is missing.
> Signed-off-by: Clément Peron
> ---
> drivers/clocksource/Kconfig | 12 ++
> drivers/clocksource/Makefile | 1 +
> drivers/clocksource/timer-imx-epit.c | 254 +++
> 3 files changed, 267 insertions(+)
> create mode 100644 drivers/clocksource/timer-imx-epit.c
>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 8e8a09755d10..cc1ed592fa6f 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -576,6 +576,18 @@ config H8300_TPU
> This enables the clocksource for the H8300 platform with the
> H8S2678 cpu.
>
> +config CLKSRC_IMX_EPIT
> + bool "Clocksource using i.MX EPIT"
> + depends on ARM && CLKDEV_LOOKUP && (ARCH_MXC || COMPILE_TEST)
> + select CLKSRC_OF if OF
The driver strictly depends on OF, and this has to be specified.
> + select CLKSRC_MMIO
> + help
> + This enables EPIT support available on some i.MX platforms.
> + Normally you don't have a reason to do so as the EPIT has
> + the same features and uses the same clocks as the GPT.
> + Anyway, on some systems the GPT may be in use for other
> + purposes.
> +
> config CLKSRC_IMX_GPT
> bool "Clocksource using i.MX GPT" if COMPILE_TEST
> depends on ARM && CLKDEV_LOOKUP
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 00caf37e52f9..d9426f69ec69 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -69,6 +69,7 @@ obj-$(CONFIG_INTEGRATOR_AP_TIMER) += timer-integrator-ap.o
> obj-$(CONFIG_CLKSRC_VERSATILE) += versatile.o
> obj-$(CONFIG_CLKSRC_MIPS_GIC)+= mips-gic-timer.o
> obj-$(CONFIG_CLKSRC_TANGO_XTAL) += tango_xtal.o
> +obj-$(CONFIG_CLKSRC_IMX_EPIT)+= timer-imx-epit.o
> obj-$(CONFIG_CLKSRC_IMX_GPT) += timer-imx-gpt.o
> obj-$(CONFIG_CLKSRC_IMX_TPM) += timer-imx-tpm.o
> obj-$(CONFIG_ASM9260_TIMER) += asm9260_timer.o
> diff --git a/drivers/clocksource/timer-imx-epit.c
> b/drivers/clocksource/timer-imx-epit.c
> new file mode 100644
> index ..96eb6435a9c3
> --- /dev/null
> +++ b/drivers/clocksource/timer-imx-epit.c
> @@ -0,0 +1,254 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * i.MX EPIT Timer
> + *
> + * Copyright (C) 2010 Sascha Hauer
> + * Copyright (C) 2018 Colin Didier
> + */
> +
> +#define EPITCR 0x00
> +#define EPITSR 0x04
> +#define EPITLR 0x08
> +#define EPITCMPR 0x0c
> +#define EPITCNR 0x10
> +
> +#define EPITCR_EN(1 << 0)
> +#define EPITCR_ENMOD (1 << 1)
> +#define EPITCR_OCIEN (1 << 2)
> +#define EPITCR_RLD (1 << 3)
> +#define EPITCR_PRESC(x) (((x) & 0xfff) << 4)
> +#define EPITCR_SWR (1 << 16)
> +#define EPITCR_IOVW (1 << 17)
> +#define EPITCR_DBGEN (1 << 18)
> +#define EPITCR_WAITEN(1 << 19)
> +#define EPITCR_RES (1 << 20)
> +#define EPITCR_STOPEN(1 << 21)
> +#define EPITCR_OM_DISCON (0 << 22)
> +#define EPITCR_OM_TOGGLE (1 << 22)
> +#define EPITCR_OM_CLEAR (2 << 22)
> +#define EPITCR_OM_SET(3 << 22)
> +#define EPITCR_CLKSRC_OFF(0 << 24)
> +#define EPITCR_CLKSRC_PERIPHERAL (1 << 24)
> +#define EPITCR_CLKSRC_REF_HIGH (2 << 24)
> +#define EPITCR_CLKSRC_REF_LOW(3 << 24)
> +
> +#define EPITSR_OCIF (1 << 0)
> +
Please place all macro definitions after the list of included header files.
Also for bit field definitions please use BIT() macro, and remove
(0 << x) macro, they are anyway unused in the code.
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Please sort out the list of headers alphabetically.
> +
> +
Surplus empty line, please remove it.
> +struct epit_timer {
> + void __iomem *base;
> + int irq;
> + struct clk *clk_per;
> + struct clock_event_device ced;
> + struct irqaction act;
> +};
> +
> +static inline struct epit_timer *to_epit_timer(struct clock_event_device
> *ced)
> +{
> + return container_of(ced, struct epit_timer, ced);
> +}
> +
> +static inline void epit_irq_disable(struct epit_timer *epittm)
> +{
> + u32 val;
> +
> + val = readl_relaxed(epittm->base + EPITCR);
> + writel_relaxed(val & ~EPITCR_OCIEN, epittm->base + EPITCR);
> +}
> +
> +static inline void epit_irq_enable(struct epit_timer *epittm)
> +{
> + u32 val;
>
[PATCH 4/5] clocksource: add driver for i.MX EPIT timer
From: Colin Didier
Add driver for NXP's EPIT timer used in i.MX 6 family of SoC.
Signed-off-by: Clément Peron
---
drivers/clocksource/Kconfig | 12 ++
drivers/clocksource/Makefile | 1 +
drivers/clocksource/timer-imx-epit.c | 254 +++
3 files changed, 267 insertions(+)
create mode 100644 drivers/clocksource/timer-imx-epit.c
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 8e8a09755d10..cc1ed592fa6f 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -576,6 +576,18 @@ config H8300_TPU
This enables the clocksource for the H8300 platform with the
H8S2678 cpu.
+config CLKSRC_IMX_EPIT
+ bool "Clocksource using i.MX EPIT"
+ depends on ARM && CLKDEV_LOOKUP && (ARCH_MXC || COMPILE_TEST)
+ select CLKSRC_OF if OF
+ select CLKSRC_MMIO
+ help
+ This enables EPIT support available on some i.MX platforms.
+ Normally you don't have a reason to do so as the EPIT has
+ the same features and uses the same clocks as the GPT.
+ Anyway, on some systems the GPT may be in use for other
+ purposes.
+
config CLKSRC_IMX_GPT
bool "Clocksource using i.MX GPT" if COMPILE_TEST
depends on ARM && CLKDEV_LOOKUP
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 00caf37e52f9..d9426f69ec69 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -69,6 +69,7 @@ obj-$(CONFIG_INTEGRATOR_AP_TIMER) += timer-integrator-ap.o
obj-$(CONFIG_CLKSRC_VERSATILE) += versatile.o
obj-$(CONFIG_CLKSRC_MIPS_GIC) += mips-gic-timer.o
obj-$(CONFIG_CLKSRC_TANGO_XTAL)+= tango_xtal.o
+obj-$(CONFIG_CLKSRC_IMX_EPIT) += timer-imx-epit.o
obj-$(CONFIG_CLKSRC_IMX_GPT) += timer-imx-gpt.o
obj-$(CONFIG_CLKSRC_IMX_TPM) += timer-imx-tpm.o
obj-$(CONFIG_ASM9260_TIMER)+= asm9260_timer.o
diff --git a/drivers/clocksource/timer-imx-epit.c
b/drivers/clocksource/timer-imx-epit.c
new file mode 100644
index ..96eb6435a9c3
--- /dev/null
+++ b/drivers/clocksource/timer-imx-epit.c
@@ -0,0 +1,254 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * i.MX EPIT Timer
+ *
+ * Copyright (C) 2010 Sascha Hauer
+ * Copyright (C) 2018 Colin Didier
+ */
+
+#define EPITCR 0x00
+#define EPITSR 0x04
+#define EPITLR 0x08
+#define EPITCMPR 0x0c
+#define EPITCNR0x10
+
+#define EPITCR_EN (1 << 0)
+#define EPITCR_ENMOD (1 << 1)
+#define EPITCR_OCIEN (1 << 2)
+#define EPITCR_RLD (1 << 3)
+#define EPITCR_PRESC(x)(((x) & 0xfff) << 4)
+#define EPITCR_SWR (1 << 16)
+#define EPITCR_IOVW(1 << 17)
+#define EPITCR_DBGEN (1 << 18)
+#define EPITCR_WAITEN (1 << 19)
+#define EPITCR_RES (1 << 20)
+#define EPITCR_STOPEN (1 << 21)
+#define EPITCR_OM_DISCON (0 << 22)
+#define EPITCR_OM_TOGGLE (1 << 22)
+#define EPITCR_OM_CLEAR(2 << 22)
+#define EPITCR_OM_SET (3 << 22)
+#define EPITCR_CLKSRC_OFF (0 << 24)
+#define EPITCR_CLKSRC_PERIPHERAL (1 << 24)
+#define EPITCR_CLKSRC_REF_HIGH (2 << 24)
+#define EPITCR_CLKSRC_REF_LOW (3 << 24)
+
+#define EPITSR_OCIF(1 << 0)
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+
+struct epit_timer {
+ void __iomem *base;
+ int irq;
+ struct clk *clk_per;
+ struct clock_event_device ced;
+ struct irqaction act;
+};
+
+static inline struct epit_timer *to_epit_timer(struct clock_event_device *ced)
+{
+ return container_of(ced, struct epit_timer, ced);
+}
+
+static inline void epit_irq_disable(struct epit_timer *epittm)
+{
+ u32 val;
+
+ val = readl_relaxed(epittm->base + EPITCR);
+ writel_relaxed(val & ~EPITCR_OCIEN, epittm->base + EPITCR);
+}
+
+static inline void epit_irq_enable(struct epit_timer *epittm)
+{
+ u32 val;
+
+ val = readl_relaxed(epittm->base + EPITCR);
+ writel_relaxed(val | EPITCR_OCIEN, epittm->base + EPITCR);
+}
+
+static void epit_irq_acknowledge(struct epit_timer *epittm)
+{
+ writel_relaxed(EPITSR_OCIF, epittm->base + EPITSR);
+}
+
+static void __iomem *sched_clock_reg;
+
+static u64 notrace epit_read_sched_clock(void)
+{
+ return ~readl_relaxed(sched_clock_reg);
+}
+
+static int __init epit_clocksource_init(struct epit_timer *epittm)
+{
+ unsigned int c = clk_get_rate(epittm->clk_per);
+
+ sched_clock_reg = epittm->base + EPITCNR;
+ sched_clock_register(epit_read_sched_clock, 32, c);
+
+ return clocksource_mmio_init(epittm->base + EPITCNR, "epit", c, 200, 32,
+
