| Message ID | 20180611155001.3506-1-peron.clem@gmail.com |
|---|---|
| Headers | show |
| Series | Reintroduce i.MX EPIT Timer | expand |
Hi, Is there still some remark against merging this ? Thanks, Clement On Mon, 11 Jun 2018 at 17:50, Clément Péron <peron.clem@gmail.com> wrote: > > From: Clément Peron <clement.peron@devialet.com> > > As suggested in the commit message we have added the device tree support, > proper bindings and we moved the driver into the correct folder. > > Moreover we made some changes like use of relaxed IO accesor, > implement sched_clock, delay_timer and reduce the clockevents min_delta. > > Changes since v6: > -fix indent > > Changes since v5: > - change epit to timer in doc example > - fix typo in imx6sl.dtsi > > Changes since v4: > - removed ipg clk > - change in dt epit to timer > - add introduction in doc > - add all compatibles in doc > - update epit entry for other i.MX device-trees > > Changes since v3: > - Clean Kconfig > - Rename imx6q-epit to imx31-epit > - Update doc and bindings > - Indent and fix > > Changes since v2 (Thanks Fabio Estevam): > - Removed unused ckil clock > - Add out_iounmap > - Check and handle if clk_prepare_enable failed > - Fix comment typo > > Changes since v1 (Thanks Vladimir Zapolskiy): > - Add OF dependency in Kconfig > - Sort header > - Use BIT macro > - Remove useless comments > - Fix incorrect indent > - Fix memory leak > - Add check and handle possible returned error > > Clément Peron (2): > ARM: imx: remove inexistant EPIT timer init > dt-bindings: timer: add i.MX EPIT timer binding > > Colin Didier (3): > clk: imx6: add EPIT clock support > clocksource: add driver for i.MX EPIT timer > ARM: dts: imx: add missing compatible and clock properties for EPIT > > .../devicetree/bindings/timer/fsl,imxepit.txt | 21 ++ > arch/arm/boot/dts/imx25.dtsi | 8 +- > arch/arm/boot/dts/imx6qdl.dtsi | 10 +- > arch/arm/boot/dts/imx6sl.dtsi | 10 +- > arch/arm/boot/dts/imx6sx.dtsi | 10 +- > arch/arm/boot/dts/imx6ul.dtsi | 10 +- > arch/arm/mach-imx/common.h | 1 - > drivers/clk/imx/clk-imx6q.c | 2 + > drivers/clocksource/Kconfig | 11 + > drivers/clocksource/Makefile | 1 + > drivers/clocksource/timer-imx-epit.c | 265 ++++++++++++++++++ > include/dt-bindings/clock/imx6qdl-clock.h | 4 +- > 12 files changed, 341 insertions(+), 12 deletions(-) > create mode 100644 Documentation/devicetree/bindings/timer/fsl,imxepit.txt > create mode 100644 drivers/clocksource/timer-imx-epit.c > > -- > 2.17.1 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/07/2018 16:55, Clément Péron wrote: > Hi, > > Is there still some remark against merging this ? Thanks for the heads up.
On 10/07/2018 16:55, Clément Péron wrote: > Hi, > > Is there still some remark against merging this ? How do you want this to be merged ? Shall I take the two patches related to the timer ? Or Ack them ?
Hi Daniel, On Tue, 10 Jul 2018 at 17:12, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 10/07/2018 16:55, Clément Péron wrote: > > Hi, > > > > Is there still some remark against merging this ? > > How do you want this to be merged ? > > Shall I take the two patches related to the timer ? Or Ack them ? I'm not an expert on how submitting patches works, it's the first driver I submit. But as this has been tested and no NXP team member seems against I would be happy to see this patch merged. Thanks, Clement > > -- > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > > Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > <http://twitter.com/#!/linaroorg> Twitter | > <http://www.linaro.org/linaro-blog/> Blog > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/07/2018 17:22, Clément Péron wrote: > Hi Daniel, > > On Tue, 10 Jul 2018 at 17:12, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: >> >> On 10/07/2018 16:55, Clément Péron wrote: >>> Hi, >>> >>> Is there still some remark against merging this ? >> >> How do you want this to be merged ? >> >> Shall I take the two patches related to the timer ? Or Ack them ? > > I'm not an expert on how submitting patches works, it's the first > driver I submit. Ok, so usually this is what happens when there is a set of changes: 1. they all touch the same subsystem (eg. drivers/clocksource), you just send all the patches to the maintainer(s) + mailing list + related people 2. they touch different subsystems: 2.1) the changes are not connected (not related together), you have to split in smaller parts and send the patches to the right subsystem maintainer (so falling back to 1.) 2.2) the changes are connected: 2.2.1) Ask all the different subsystem maintainers to acknowledge the changes and submit the patches to arm-soc@ 2.2.2) Ask each maintainer to take their part if the changes are connected but not interdependent (patches individually won't break the system) You are in the 2.2) situation. My question is do you want 2.2.1) or 2.2.2) ?
HI Daniel, On Tue, 10 Jul 2018 at 17:37, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 10/07/2018 17:22, Clément Péron wrote: > > Hi Daniel, > > > > On Tue, 10 Jul 2018 at 17:12, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > >> > >> On 10/07/2018 16:55, Clément Péron wrote: > >>> Hi, > >>> > >>> Is there still some remark against merging this ? > >> > >> How do you want this to be merged ? > >> > >> Shall I take the two patches related to the timer ? Or Ack them ? > > > > I'm not an expert on how submitting patches works, it's the first > > driver I submit. > > Ok, so usually this is what happens when there is a set of changes: > > 1. they all touch the same subsystem (eg. drivers/clocksource), you > just send all the patches to the maintainer(s) + mailing list + related > people > > 2. they touch different subsystems: > > 2.1) the changes are not connected (not related together), you have > to split in smaller parts and send the patches to the right subsystem > maintainer (so falling back to 1.) > > 2.2) the changes are connected: > > 2.2.1) Ask all the different subsystem maintainers to acknowledge > the changes and submit the patches to arm-soc@ > > 2.2.2) Ask each maintainer to take their part if the changes are > connected but not interdependent (patches individually won't break the > system) > > > You are in the 2.2) situation. My question is do you want 2.2.1) or 2.2.2) ? Thanks for the explanation, I think in 2.2.1 as I still need an Ack for the "ARM: dts" patch Regards, Clement > > > -- > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > > Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > <http://twitter.com/#!/linaroorg> Twitter | > <http://www.linaro.org/linaro-blog/> Blog > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/06/2018 17:50, Clément Péron wrote: > From: Colin Didier <colin.didier@devialet.com> > > Add driver for NXP's EPIT timer used in i.MX SoC. Give a description on how works this timer. > Signed-off-by: Colin Didier <colin.didier@devialet.com> > Signed-off-by: Clément Peron <clement.peron@devialet.com> > Reviewed-by: Vladimir Zapolskiy <vz@mleia.com> > Tested-by: Vladimir Zapolskiy <vz@mleia.com> > --- > drivers/clocksource/Kconfig | 11 ++ > drivers/clocksource/Makefile | 1 + > drivers/clocksource/timer-imx-epit.c | 265 +++++++++++++++++++++++++++ > 3 files changed, 277 insertions(+) > create mode 100644 drivers/clocksource/timer-imx-epit.c > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > index 8e8a09755d10..790478afd02c 100644 > --- a/drivers/clocksource/Kconfig > +++ b/drivers/clocksource/Kconfig > @@ -576,6 +576,17 @@ 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 CLKDEV_LOOKUP && (ARCH_MXC || COMPILE_TEST) > + 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 000000000000..7f1c8e2e00aa > --- /dev/null > +++ b/drivers/clocksource/timer-imx-epit.c > @@ -0,0 +1,265 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * i.MX EPIT Timer > + * > + * Copyright (C) 2010 Sascha Hauer <s.hauer@pengutronix.de> > + * Copyright (C) 2018 Colin Didier <colin.didier@devialet.com> > + * Copyright (C) 2018 Clément Péron <clement.peron@devialet.com> > + */ > + > +#include <linux/clk.h> > +#include <linux/clockchips.h> > +#include <linux/interrupt.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h> > +#include <linux/sched_clock.h> > +#include <linux/slab.h> > + > +#define EPITCR 0x00 > +#define EPITSR 0x04 > +#define EPITLR 0x08 > +#define EPITCMPR 0x0c > +#define EPITCNR 0x10 > + > +#define EPITCR_EN BIT(0) > +#define EPITCR_ENMOD BIT(1) > +#define EPITCR_OCIEN BIT(2) > +#define EPITCR_RLD BIT(3) > +#define EPITCR_PRESC(x) (((x) & 0xfff) << 4) > +#define EPITCR_SWR BIT(16) > +#define EPITCR_IOVW BIT(17) > +#define EPITCR_DBGEN BIT(18) > +#define EPITCR_WAITEN BIT(19) > +#define EPITCR_RES BIT(20) > +#define EPITCR_STOPEN BIT(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 BIT(0) > + > +struct epit_timer { > + void __iomem *base; > + int irq; > + struct clk *clk; > + struct clock_event_device ced; > + struct irqaction act; > +}; > + > +static void __iomem *sched_clock_reg; > + > +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 u64 notrace epit_read_sched_clock(void) > +{ > + return ~readl_relaxed(sched_clock_reg); > +} > + > +static int epit_set_next_event(unsigned long cycles, > + struct clock_event_device *ced) > +{ > + struct epit_timer *epittm = to_epit_timer(ced); > + unsigned long tcmp; > + > + tcmp = readl_relaxed(epittm->base + EPITCNR) - cycles; > + writel_relaxed(tcmp, epittm->base + EPITCMPR); > + > + return 0; > +} > + > +/* Left event sources disabled, no more interrupts appear */ > +static int epit_shutdown(struct clock_event_device *ced) > +{ > + struct epit_timer *epittm = to_epit_timer(ced); > + unsigned long flags; > + > + /* > + * The timer interrupt generation is disabled at least > + * for enough time to call epit_set_next_event() > + */ > + local_irq_save(flags); This is not necessary. This function is called with interrupt disabled. > + /* Disable interrupt in EPIT module */ > + epit_irq_disable(epittm); > + > + /* Clear pending interrupt */ > + epit_irq_acknowledge(epittm); No irq ack is needed here. Neither disabling the interrupt. Why not just stop the counter ? > + local_irq_restore(flags); > + > + return 0; > +} > + > +static int epit_set_oneshot(struct clock_event_device *ced) > +{ > + struct epit_timer *epittm = to_epit_timer(ced); > + unsigned long flags; > + > + /* > + * The timer interrupt generation is disabled at least > + * for enough time to call epit_set_next_event() > + */ > + local_irq_save(flags); This is not necessary. This function is called with interrupt disabled. > + /* Disable interrupt in EPIT module */ > + epit_irq_disable(epittm); > + > + /* Clear pending interrupt, only while switching mode */ > + if (!clockevent_state_oneshot(ced)) > + epit_irq_acknowledge(epittm); Why do you need to ack the interrupt here ? > + /* > + * Do not put overhead of interrupt enable/disable into > + * epit_set_next_event(), the core has about 4 minutes > + * to call epit_set_next_event() or shutdown clock after > + * mode switching > + */ > + epit_irq_enable(epittm); > + local_irq_restore(flags); > + > + return 0; > +} > + > +static irqreturn_t epit_timer_interrupt(int irq, void *dev_id) > +{ > + struct clock_event_device *ced = dev_id; > + struct epit_timer *epittm = to_epit_timer(ced); > + > + epit_irq_acknowledge(epittm); > + > + ced->event_handler(ced); > + > + return IRQ_HANDLED; > +} > + > +static int __init epit_clocksource_init(struct epit_timer *epittm) > +{ > + unsigned int c = clk_get_rate(epittm->clk); > + > + 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, > + clocksource_mmio_readl_down); > +} > + > +static int __init epit_clockevent_init(struct epit_timer *epittm) > +{ > + struct clock_event_device *ced = &epittm->ced; > + struct irqaction *act = &epittm->act; > + > + ced->name = "epit"; > + ced->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_DYNIRQ; > + ced->set_state_shutdown = epit_shutdown; > + ced->tick_resume = epit_shutdown; > + ced->set_state_oneshot = epit_set_oneshot; > + ced->set_next_event = epit_set_next_event; > + ced->rating = 200; > + ced->cpumask = cpumask_of(0); > + ced->irq = epittm->irq; > + clockevents_config_and_register(ced, clk_get_rate(epittm->clk), > + 0xff, 0xfffffffe); > + > + act->name = "i.MX EPIT Timer Tick", > + act->flags = IRQF_TIMER | IRQF_IRQPOLL; > + act->handler = epit_timer_interrupt; > + act->dev_id = ced; > + > + /* Make irqs happen */ > + return setup_irq(epittm->irq, act); > +} > + > +static int __init epit_timer_init(struct device_node *np) > +{ > + struct epit_timer *epittm; > + int ret; > + > + epittm = kzalloc(sizeof(*epittm), GFP_KERNEL); > + if (!epittm) > + return -ENOMEM; > + > + epittm->base = of_iomap(np, 0); > + if (!epittm->base) { > + ret = -ENXIO; > + goto out_kfree; > + } > + > + epittm->irq = irq_of_parse_and_map(np, 0); > + if (!epittm->irq) { > + ret = -EINVAL; > + goto out_iounmap; > + } > + > + /* Get EPIT clock */ > + epittm->clk = of_clk_get(np, 0); > + if (IS_ERR(epittm->clk)) { > + pr_err("i.MX EPIT: unable to get clk\n"); > + ret = PTR_ERR(epittm->clk); > + goto out_iounmap; > + } > + > + ret = clk_prepare_enable(epittm->clk); > + if (ret) { > + pr_err("i.MX EPIT: unable to prepare+enable clk\n"); > + goto out_iounmap; > + } Please replace all the above with the timer-of API as: https://patchwork.kernel.org/patch/10510443/ > + /* Initialise to a known state (all timers off, and timing reset) */ > + writel_relaxed(0x0, epittm->base + EPITCR); > + writel_relaxed(0xffffffff, epittm->base + EPITLR); > + writel_relaxed(EPITCR_EN | EPITCR_CLKSRC_REF_HIGH | EPITCR_WAITEN, > + epittm->base + EPITCR); > + > + ret = epit_clocksource_init(epittm); > + if (ret) { > + pr_err("i.MX EPIT: failed to init clocksource\n"); > + goto out_clk_disable; > + } > + > + ret = epit_clockevent_init(epittm); > + if (ret) { > + pr_err("i.MX EPIT: failed to init clockevent\n"); > + goto out_clk_disable; > + } > + > + return 0; > + > +out_clk_disable: > + clk_disable_unprepare(epittm->clk); > +out_iounmap: > + iounmap(epittm->base); > +out_kfree: > + kfree(epittm); > + > + return ret; > +} > +TIMER_OF_DECLARE(epit_timer, "fsl,imx31-epit", epit_timer_init); >
Hi Daniel, Thanks for the review and sorry for the delay. On Tue, 10 Jul 2018 at 18:20, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 11/06/2018 17:50, Clément Péron wrote: > > From: Colin Didier <colin.didier@devialet.com> > > > > Add driver for NXP's EPIT timer used in i.MX SoC. > > Give a description on how works this timer. Ok, > > > > Signed-off-by: Colin Didier <colin.didier@devialet.com> > > Signed-off-by: Clément Peron <clement.peron@devialet.com> > > Reviewed-by: Vladimir Zapolskiy <vz@mleia.com> > > Tested-by: Vladimir Zapolskiy <vz@mleia.com> > > --- > > drivers/clocksource/Kconfig | 11 ++ > > drivers/clocksource/Makefile | 1 + > > drivers/clocksource/timer-imx-epit.c | 265 +++++++++++++++++++++++++++ > > 3 files changed, 277 insertions(+) > > create mode 100644 drivers/clocksource/timer-imx-epit.c > > > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > > index 8e8a09755d10..790478afd02c 100644 > > --- a/drivers/clocksource/Kconfig > > +++ b/drivers/clocksource/Kconfig > > @@ -576,6 +576,17 @@ 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 CLKDEV_LOOKUP && (ARCH_MXC || COMPILE_TEST) > > + 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 000000000000..7f1c8e2e00aa > > --- /dev/null > > +++ b/drivers/clocksource/timer-imx-epit.c > > @@ -0,0 +1,265 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * i.MX EPIT Timer > > + * > > + * Copyright (C) 2010 Sascha Hauer <s.hauer@pengutronix.de> > > + * Copyright (C) 2018 Colin Didier <colin.didier@devialet.com> > > + * Copyright (C) 2018 Clément Péron <clement.peron@devialet.com> > > + */ > > + > > +#include <linux/clk.h> > > +#include <linux/clockchips.h> > > +#include <linux/interrupt.h> > > +#include <linux/of_address.h> > > +#include <linux/of_irq.h> > > +#include <linux/sched_clock.h> > > +#include <linux/slab.h> > > + > > +#define EPITCR 0x00 > > +#define EPITSR 0x04 > > +#define EPITLR 0x08 > > +#define EPITCMPR 0x0c > > +#define EPITCNR 0x10 > > + > > +#define EPITCR_EN BIT(0) > > +#define EPITCR_ENMOD BIT(1) > > +#define EPITCR_OCIEN BIT(2) > > +#define EPITCR_RLD BIT(3) > > +#define EPITCR_PRESC(x) (((x) & 0xfff) << 4) > > +#define EPITCR_SWR BIT(16) > > +#define EPITCR_IOVW BIT(17) > > +#define EPITCR_DBGEN BIT(18) > > +#define EPITCR_WAITEN BIT(19) > > +#define EPITCR_RES BIT(20) > > +#define EPITCR_STOPEN BIT(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 BIT(0) > > + > > +struct epit_timer { > > + void __iomem *base; > > + int irq; > > + struct clk *clk; > > + struct clock_event_device ced; > > + struct irqaction act; > > +}; > > + > > +static void __iomem *sched_clock_reg; > > + > > +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 u64 notrace epit_read_sched_clock(void) > > +{ > > + return ~readl_relaxed(sched_clock_reg); > > +} > > + > > +static int epit_set_next_event(unsigned long cycles, > > + struct clock_event_device *ced) > > +{ > > + struct epit_timer *epittm = to_epit_timer(ced); > > + unsigned long tcmp; > > + > > + tcmp = readl_relaxed(epittm->base + EPITCNR) - cycles; > > + writel_relaxed(tcmp, epittm->base + EPITCMPR); > > + > > + return 0; > > +} > > + > > +/* Left event sources disabled, no more interrupts appear */ > > +static int epit_shutdown(struct clock_event_device *ced) > > +{ > > + struct epit_timer *epittm = to_epit_timer(ced); > > + unsigned long flags; > > + > > + /* > > + * The timer interrupt generation is disabled at least > > + * for enough time to call epit_set_next_event() > > + */ > > + local_irq_save(flags); > > This is not necessary. This function is called with interrupt disabled. I took this from the i.MX GPT Timer : https://elixir.bootlin.com/linux/latest/source/drivers/clocksource/timer-imx-gpt.c#L208 Do you think i should also fix on the imx gpt timer ? > > > + /* Disable interrupt in EPIT module */ > > + epit_irq_disable(epittm); > > + > > + /* Clear pending interrupt */ > > + epit_irq_acknowledge(epittm); > > No irq ack is needed here. Neither disabling the interrupt. Is it done by the framework ? I took also this from the IMX GPT driver > > Why not just stop the counter ? I will check the datasheet. > > > + local_irq_restore(flags); > > + > > + return 0; > > +} > > + > > +static int epit_set_oneshot(struct clock_event_device *ced) > > +{ > > + struct epit_timer *epittm = to_epit_timer(ced); > > + unsigned long flags; > > + > > + /* > > + * The timer interrupt generation is disabled at least > > + * for enough time to call epit_set_next_event() > > + */ > > + local_irq_save(flags); > > This is not necessary. This function is called with interrupt disabled. > > > + /* Disable interrupt in EPIT module */ > > + epit_irq_disable(epittm); > > + > > + /* Clear pending interrupt, only while switching mode */ > > + if (!clockevent_state_oneshot(ced)) > > + epit_irq_acknowledge(epittm); > > Why do you need to ack the interrupt here ? > > > + /* > > + * Do not put overhead of interrupt enable/disable into > > + * epit_set_next_event(), the core has about 4 minutes > > + * to call epit_set_next_event() or shutdown clock after > > + * mode switching > > + */ > > + epit_irq_enable(epittm); > > + local_irq_restore(flags); > > + > > + return 0; > > +} > > + > > +static irqreturn_t epit_timer_interrupt(int irq, void *dev_id) > > +{ > > + struct clock_event_device *ced = dev_id; > > + struct epit_timer *epittm = to_epit_timer(ced); > > + > > + epit_irq_acknowledge(epittm); > > + > > + ced->event_handler(ced); > > + > > + return IRQ_HANDLED; > > +} > > + > > +static int __init epit_clocksource_init(struct epit_timer *epittm) > > +{ > > + unsigned int c = clk_get_rate(epittm->clk); > > + > > + 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, > > + clocksource_mmio_readl_down); > > +} > > + > > +static int __init epit_clockevent_init(struct epit_timer *epittm) > > +{ > > + struct clock_event_device *ced = &epittm->ced; > > + struct irqaction *act = &epittm->act; > > + > > + ced->name = "epit"; > > + ced->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_DYNIRQ; > > + ced->set_state_shutdown = epit_shutdown; > > + ced->tick_resume = epit_shutdown; > > + ced->set_state_oneshot = epit_set_oneshot; > > + ced->set_next_event = epit_set_next_event; > > + ced->rating = 200; > > + ced->cpumask = cpumask_of(0); > > + ced->irq = epittm->irq; > > + clockevents_config_and_register(ced, clk_get_rate(epittm->clk), > > + 0xff, 0xfffffffe); > > + > > + act->name = "i.MX EPIT Timer Tick", > > + act->flags = IRQF_TIMER | IRQF_IRQPOLL; > > + act->handler = epit_timer_interrupt; > > + act->dev_id = ced; > > + > > + /* Make irqs happen */ > > + return setup_irq(epittm->irq, act); > > +} > > + > > +static int __init epit_timer_init(struct device_node *np) > > +{ > > + struct epit_timer *epittm; > > + int ret; > > + > > + epittm = kzalloc(sizeof(*epittm), GFP_KERNEL); > > + if (!epittm) > > + return -ENOMEM; > > + > > + epittm->base = of_iomap(np, 0); > > + if (!epittm->base) { > > + ret = -ENXIO; > > + goto out_kfree; > > + } > > + > > + epittm->irq = irq_of_parse_and_map(np, 0); > > + if (!epittm->irq) { > > + ret = -EINVAL; > > + goto out_iounmap; > > + } > > + > > + /* Get EPIT clock */ > > + epittm->clk = of_clk_get(np, 0); > > + if (IS_ERR(epittm->clk)) { > > + pr_err("i.MX EPIT: unable to get clk\n"); > > + ret = PTR_ERR(epittm->clk); > > + goto out_iounmap; > > + } > > + > > + ret = clk_prepare_enable(epittm->clk); > > + if (ret) { > > + pr_err("i.MX EPIT: unable to prepare+enable clk\n"); > > + goto out_iounmap; > > + } > > Please replace all the above with the timer-of API as: Ok I will, Thanks Clement > > https://patchwork.kernel.org/patch/10510443/ > > > > + /* Initialise to a known state (all timers off, and timing reset) */ > > + writel_relaxed(0x0, epittm->base + EPITCR); > > + writel_relaxed(0xffffffff, epittm->base + EPITLR); > > + writel_relaxed(EPITCR_EN | EPITCR_CLKSRC_REF_HIGH | EPITCR_WAITEN, > > + epittm->base + EPITCR); > > + > > + ret = epit_clocksource_init(epittm); > > + if (ret) { > > + pr_err("i.MX EPIT: failed to init clocksource\n"); > > + goto out_clk_disable; > > + } > > + > > + ret = epit_clockevent_init(epittm); > > + if (ret) { > > + pr_err("i.MX EPIT: failed to init clockevent\n"); > > + goto out_clk_disable; > > + } > > + > > + return 0; > > + > > +out_clk_disable: > > + clk_disable_unprepare(epittm->clk); > > +out_iounmap: > > + iounmap(epittm->base); > > +out_kfree: > > + kfree(epittm); > > + > > + return ret; > > +} > > +TIMER_OF_DECLARE(epit_timer, "fsl,imx31-epit", epit_timer_init); > > > > > -- > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > > Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > <http://twitter.com/#!/linaroorg> Twitter | > <http://www.linaro.org/linaro-blog/> Blog >
Hi Clément, On 04/11/2018 17:07, Clément Péron wrote: > Hi Daniel, > > Thanks for the review and sorry for the delay. no problem. > On Tue, 10 Jul 2018 at 18:20, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: >> >> On 11/06/2018 17:50, Clément Péron wrote: >>> From: Colin Didier <colin.didier@devialet.com> >>> >>> Add driver for NXP's EPIT timer used in i.MX SoC. >> >> Give a description on how works this timer. > Ok, > >> >> >>> Signed-off-by: Colin Didier <colin.didier@devialet.com> >>> Signed-off-by: Clément Peron <clement.peron@devialet.com> >>> Reviewed-by: Vladimir Zapolskiy <vz@mleia.com> >>> Tested-by: Vladimir Zapolskiy <vz@mleia.com> >>> --- >>> drivers/clocksource/Kconfig | 11 ++ >>> drivers/clocksource/Makefile | 1 + >>> drivers/clocksource/timer-imx-epit.c | 265 +++++++++++++++++++++++++++ >>> 3 files changed, 277 insertions(+) >>> create mode 100644 drivers/clocksource/timer-imx-epit.c >>> >>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig >>> index 8e8a09755d10..790478afd02c 100644 >>> --- a/drivers/clocksource/Kconfig >>> +++ b/drivers/clocksource/Kconfig >>> @@ -576,6 +576,17 @@ 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 CLKDEV_LOOKUP && (ARCH_MXC || COMPILE_TEST) >>> + 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 000000000000..7f1c8e2e00aa >>> --- /dev/null >>> +++ b/drivers/clocksource/timer-imx-epit.c >>> @@ -0,0 +1,265 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * i.MX EPIT Timer >>> + * >>> + * Copyright (C) 2010 Sascha Hauer <s.hauer@pengutronix.de> >>> + * Copyright (C) 2018 Colin Didier <colin.didier@devialet.com> >>> + * Copyright (C) 2018 Clément Péron <clement.peron@devialet.com> >>> + */ >>> + >>> +#include <linux/clk.h> >>> +#include <linux/clockchips.h> >>> +#include <linux/interrupt.h> >>> +#include <linux/of_address.h> >>> +#include <linux/of_irq.h> >>> +#include <linux/sched_clock.h> >>> +#include <linux/slab.h> >>> + >>> +#define EPITCR 0x00 >>> +#define EPITSR 0x04 >>> +#define EPITLR 0x08 >>> +#define EPITCMPR 0x0c >>> +#define EPITCNR 0x10 >>> + >>> +#define EPITCR_EN BIT(0) >>> +#define EPITCR_ENMOD BIT(1) >>> +#define EPITCR_OCIEN BIT(2) >>> +#define EPITCR_RLD BIT(3) >>> +#define EPITCR_PRESC(x) (((x) & 0xfff) << 4) >>> +#define EPITCR_SWR BIT(16) >>> +#define EPITCR_IOVW BIT(17) >>> +#define EPITCR_DBGEN BIT(18) >>> +#define EPITCR_WAITEN BIT(19) >>> +#define EPITCR_RES BIT(20) >>> +#define EPITCR_STOPEN BIT(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 BIT(0) >>> + >>> +struct epit_timer { >>> + void __iomem *base; >>> + int irq; >>> + struct clk *clk; >>> + struct clock_event_device ced; >>> + struct irqaction act; >>> +}; >>> + >>> +static void __iomem *sched_clock_reg; >>> + >>> +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 u64 notrace epit_read_sched_clock(void) >>> +{ >>> + return ~readl_relaxed(sched_clock_reg); >>> +} >>> + >>> +static int epit_set_next_event(unsigned long cycles, >>> + struct clock_event_device *ced) >>> +{ >>> + struct epit_timer *epittm = to_epit_timer(ced); >>> + unsigned long tcmp; >>> + >>> + tcmp = readl_relaxed(epittm->base + EPITCNR) - cycles; >>> + writel_relaxed(tcmp, epittm->base + EPITCMPR); >>> + >>> + return 0; >>> +} >>> + >>> +/* Left event sources disabled, no more interrupts appear */ >>> +static int epit_shutdown(struct clock_event_device *ced) >>> +{ >>> + struct epit_timer *epittm = to_epit_timer(ced); >>> + unsigned long flags; >>> + >>> + /* >>> + * The timer interrupt generation is disabled at least >>> + * for enough time to call epit_set_next_event() >>> + */ >>> + local_irq_save(flags); >> >> This is not necessary. This function is called with interrupt disabled. > > I took this from the i.MX GPT Timer : > https://elixir.bootlin.com/linux/latest/source/drivers/clocksource/timer-imx-gpt.c#L208 > > Do you think i should also fix on the imx gpt timer ? Yes, the pointed code is 10 years old, so if you are willing to do the changes in a separate patchset, that would be nice. >>> + /* Disable interrupt in EPIT module */ >>> + epit_irq_disable(epittm); >>> + >>> + /* Clear pending interrupt */ >>> + epit_irq_acknowledge(epittm); >> >> No irq ack is needed here. Neither disabling the interrupt. > Is it done by the framework ? > > I took also this from the IMX GPT driver If we reach this point of code with a pending timer interrupt, that means it should be processed after the clockevent shutdown path is finished. Otherwise it is like we cancel the timer on the back of the system. >> Why not just stop the counter ? > I will check the datasheet. >>> + local_irq_restore(flags); >>> + >>> + return 0; >>> +} >>> + >>> +static int epit_set_oneshot(struct clock_event_device *ced) >>> +{ >>> + struct epit_timer *epittm = to_epit_timer(ced); >>> + unsigned long flags; >>> + >>> + /* >>> + * The timer interrupt generation is disabled at least >>> + * for enough time to call epit_set_next_event() >>> + */ >>> + local_irq_save(flags); >> >> This is not necessary. This function is called with interrupt disabled. >> >>> + /* Disable interrupt in EPIT module */ >>> + epit_irq_disable(epittm); >>> + >>> + /* Clear pending interrupt, only while switching mode */ >>> + if (!clockevent_state_oneshot(ced)) >>> + epit_irq_acknowledge(epittm); >> >> Why do you need to ack the interrupt here ? >> >>> + /* >>> + * Do not put overhead of interrupt enable/disable into >>> + * epit_set_next_event(), the core has about 4 minutes >>> + * to call epit_set_next_event() or shutdown clock after >>> + * mode switching >>> + */ >>> + epit_irq_enable(epittm); >>> + local_irq_restore(flags); >>> + >>> + return 0; >>> +} >>> + >>> +static irqreturn_t epit_timer_interrupt(int irq, void *dev_id) >>> +{ >>> + struct clock_event_device *ced = dev_id; >>> + struct epit_timer *epittm = to_epit_timer(ced); >>> + >>> + epit_irq_acknowledge(epittm); >>> + >>> + ced->event_handler(ced); >>> + >>> + return IRQ_HANDLED; >>> +} >>> + >>> +static int __init epit_clocksource_init(struct epit_timer *epittm) >>> +{ >>> + unsigned int c = clk_get_rate(epittm->clk); >>> + >>> + 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, >>> + clocksource_mmio_readl_down); >>> +} >>> + >>> +static int __init epit_clockevent_init(struct epit_timer *epittm) >>> +{ >>> + struct clock_event_device *ced = &epittm->ced; >>> + struct irqaction *act = &epittm->act; >>> + >>> + ced->name = "epit"; >>> + ced->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_DYNIRQ; >>> + ced->set_state_shutdown = epit_shutdown; >>> + ced->tick_resume = epit_shutdown; >>> + ced->set_state_oneshot = epit_set_oneshot; >>> + ced->set_next_event = epit_set_next_event; >>> + ced->rating = 200; >>> + ced->cpumask = cpumask_of(0); >>> + ced->irq = epittm->irq; >>> + clockevents_config_and_register(ced, clk_get_rate(epittm->clk), >>> + 0xff, 0xfffffffe); >>> + >>> + act->name = "i.MX EPIT Timer Tick", >>> + act->flags = IRQF_TIMER | IRQF_IRQPOLL; >>> + act->handler = epit_timer_interrupt; >>> + act->dev_id = ced; >>> + >>> + /* Make irqs happen */ >>> + return setup_irq(epittm->irq, act); >>> +} >>> + >>> +static int __init epit_timer_init(struct device_node *np) >>> +{ >>> + struct epit_timer *epittm; >>> + int ret; >>> + >>> + epittm = kzalloc(sizeof(*epittm), GFP_KERNEL); >>> + if (!epittm) >>> + return -ENOMEM; >>> + >>> + epittm->base = of_iomap(np, 0); >>> + if (!epittm->base) { >>> + ret = -ENXIO; >>> + goto out_kfree; >>> + } >>> + >>> + epittm->irq = irq_of_parse_and_map(np, 0); >>> + if (!epittm->irq) { >>> + ret = -EINVAL; >>> + goto out_iounmap; >>> + } >>> + >>> + /* Get EPIT clock */ >>> + epittm->clk = of_clk_get(np, 0); >>> + if (IS_ERR(epittm->clk)) { >>> + pr_err("i.MX EPIT: unable to get clk\n"); >>> + ret = PTR_ERR(epittm->clk); >>> + goto out_iounmap; >>> + } >>> + >>> + ret = clk_prepare_enable(epittm->clk); >>> + if (ret) { >>> + pr_err("i.MX EPIT: unable to prepare+enable clk\n"); >>> + goto out_iounmap; >>> + } >> >> Please replace all the above with the timer-of API as: > Ok I will, > > Thanks > Clement > >> >> https://patchwork.kernel.org/patch/10510443/ >> >> >>> + /* Initialise to a known state (all timers off, and timing reset) */ >>> + writel_relaxed(0x0, epittm->base + EPITCR); >>> + writel_relaxed(0xffffffff, epittm->base + EPITLR); >>> + writel_relaxed(EPITCR_EN | EPITCR_CLKSRC_REF_HIGH | EPITCR_WAITEN, >>> + epittm->base + EPITCR); >>> + >>> + ret = epit_clocksource_init(epittm); >>> + if (ret) { >>> + pr_err("i.MX EPIT: failed to init clocksource\n"); >>> + goto out_clk_disable; >>> + } >>> + >>> + ret = epit_clockevent_init(epittm); >>> + if (ret) { >>> + pr_err("i.MX EPIT: failed to init clockevent\n"); >>> + goto out_clk_disable; >>> + } >>> + >>> + return 0; >>> + >>> +out_clk_disable: >>> + clk_disable_unprepare(epittm->clk); >>> +out_iounmap: >>> + iounmap(epittm->base); >>> +out_kfree: >>> + kfree(epittm); >>> + >>> + return ret; >>> +} >>> +TIMER_OF_DECLARE(epit_timer, "fsl,imx31-epit", epit_timer_init); >>> >> >> >> -- >> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs >> >> Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | >> <http://twitter.com/#!/linaroorg> Twitter | >> <http://www.linaro.org/linaro-blog/> Blog >>
Hi Daniel, On Mon, 5 Nov 2018 at 12:48, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > > Hi Clément, > > On 04/11/2018 17:07, Clément Péron wrote: > > Hi Daniel, > > > > Thanks for the review and sorry for the delay. > > no problem. > > > On Tue, 10 Jul 2018 at 18:20, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > >> > >> On 11/06/2018 17:50, Clément Péron wrote: > >>> From: Colin Didier <colin.didier@devialet.com> > >>> > >>> Add driver for NXP's EPIT timer used in i.MX SoC. > >> > >> Give a description on how works this timer. > > Ok, > > > >> > >> > >>> Signed-off-by: Colin Didier <colin.didier@devialet.com> > >>> Signed-off-by: Clément Peron <clement.peron@devialet.com> > >>> Reviewed-by: Vladimir Zapolskiy <vz@mleia.com> > >>> Tested-by: Vladimir Zapolskiy <vz@mleia.com> > >>> --- > >>> drivers/clocksource/Kconfig | 11 ++ > >>> drivers/clocksource/Makefile | 1 + > >>> drivers/clocksource/timer-imx-epit.c | 265 +++++++++++++++++++++++++++ > >>> 3 files changed, 277 insertions(+) > >>> create mode 100644 drivers/clocksource/timer-imx-epit.c > >>> > >>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > >>> index 8e8a09755d10..790478afd02c 100644 > >>> --- a/drivers/clocksource/Kconfig > >>> +++ b/drivers/clocksource/Kconfig > >>> @@ -576,6 +576,17 @@ 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 CLKDEV_LOOKUP && (ARCH_MXC || COMPILE_TEST) > >>> + 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 000000000000..7f1c8e2e00aa > >>> --- /dev/null > >>> +++ b/drivers/clocksource/timer-imx-epit.c > >>> @@ -0,0 +1,265 @@ > >>> +// SPDX-License-Identifier: GPL-2.0 > >>> +/* > >>> + * i.MX EPIT Timer > >>> + * > >>> + * Copyright (C) 2010 Sascha Hauer <s.hauer@pengutronix.de> > >>> + * Copyright (C) 2018 Colin Didier <colin.didier@devialet.com> > >>> + * Copyright (C) 2018 Clément Péron <clement.peron@devialet.com> > >>> + */ > >>> + > >>> +#include <linux/clk.h> > >>> +#include <linux/clockchips.h> > >>> +#include <linux/interrupt.h> > >>> +#include <linux/of_address.h> > >>> +#include <linux/of_irq.h> > >>> +#include <linux/sched_clock.h> > >>> +#include <linux/slab.h> > >>> + > >>> +#define EPITCR 0x00 > >>> +#define EPITSR 0x04 > >>> +#define EPITLR 0x08 > >>> +#define EPITCMPR 0x0c > >>> +#define EPITCNR 0x10 > >>> + > >>> +#define EPITCR_EN BIT(0) > >>> +#define EPITCR_ENMOD BIT(1) > >>> +#define EPITCR_OCIEN BIT(2) > >>> +#define EPITCR_RLD BIT(3) > >>> +#define EPITCR_PRESC(x) (((x) & 0xfff) << 4) > >>> +#define EPITCR_SWR BIT(16) > >>> +#define EPITCR_IOVW BIT(17) > >>> +#define EPITCR_DBGEN BIT(18) > >>> +#define EPITCR_WAITEN BIT(19) > >>> +#define EPITCR_RES BIT(20) > >>> +#define EPITCR_STOPEN BIT(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 BIT(0) > >>> + > >>> +struct epit_timer { > >>> + void __iomem *base; > >>> + int irq; > >>> + struct clk *clk; > >>> + struct clock_event_device ced; > >>> + struct irqaction act; > >>> +}; > >>> + > >>> +static void __iomem *sched_clock_reg; > >>> + > >>> +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 u64 notrace epit_read_sched_clock(void) > >>> +{ > >>> + return ~readl_relaxed(sched_clock_reg); > >>> +} > >>> + > >>> +static int epit_set_next_event(unsigned long cycles, > >>> + struct clock_event_device *ced) > >>> +{ > >>> + struct epit_timer *epittm = to_epit_timer(ced); > >>> + unsigned long tcmp; > >>> + > >>> + tcmp = readl_relaxed(epittm->base + EPITCNR) - cycles; > >>> + writel_relaxed(tcmp, epittm->base + EPITCMPR); > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +/* Left event sources disabled, no more interrupts appear */ > >>> +static int epit_shutdown(struct clock_event_device *ced) > >>> +{ > >>> + struct epit_timer *epittm = to_epit_timer(ced); > >>> + unsigned long flags; > >>> + > >>> + /* > >>> + * The timer interrupt generation is disabled at least > >>> + * for enough time to call epit_set_next_event() > >>> + */ > >>> + local_irq_save(flags); > >> > >> This is not necessary. This function is called with interrupt disabled. > > > > I took this from the i.MX GPT Timer : > > https://elixir.bootlin.com/linux/latest/source/drivers/clocksource/timer-imx-gpt.c#L208 > > > > Do you think i should also fix on the imx gpt timer ? > > Yes, the pointed code is 10 years old, so if you are willing to do the > changes in a separate patchset, that would be nice. > > >>> + /* Disable interrupt in EPIT module */ > >>> + epit_irq_disable(epittm); > >>> + > >>> + /* Clear pending interrupt */ > >>> + epit_irq_acknowledge(epittm); > >> > >> No irq ack is needed here. Neither disabling the interrupt. > > Is it done by the framework ? > > > > I took also this from the IMX GPT driver > > If we reach this point of code with a pending timer interrupt, that > means it should be processed after the clockevent shutdown path is > finished. Otherwise it is like we cancel the timer on the back of the > system. Thanks for pointing out that. This is actually the first timer driver that I touch. I found all this a bit crappy and I have lazily copy/paste from imx-gpt driver. for me it should more propre to have something like the "timer-npcm7xx.c" driver. I'm looking to understand a bit more the framework before cleaning this. Could you point me any documentation about the framework, Specially what should set_state_**, and tick_resume should do. I'm septic about this "ced->tick_resume = epit_shutdown;" Also do you have some test that i could run to confirm the driver works fine ? Thanks, Clement > > >> Why not just stop the counter ? > > I will check the datasheet. > >>> + local_irq_restore(flags); > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static int epit_set_oneshot(struct clock_event_device *ced) > >>> +{ > >>> + struct epit_timer *epittm = to_epit_timer(ced); > >>> + unsigned long flags; > >>> + > >>> + /* > >>> + * The timer interrupt generation is disabled at least > >>> + * for enough time to call epit_set_next_event() > >>> + */ > >>> + local_irq_save(flags); > >> > >> This is not necessary. This function is called with interrupt disabled. > >> > >>> + /* Disable interrupt in EPIT module */ > >>> + epit_irq_disable(epittm); > >>> + > >>> + /* Clear pending interrupt, only while switching mode */ > >>> + if (!clockevent_state_oneshot(ced)) > >>> + epit_irq_acknowledge(epittm); > >> > >> Why do you need to ack the interrupt here ? > >> > >>> + /* > >>> + * Do not put overhead of interrupt enable/disable into > >>> + * epit_set_next_event(), the core has about 4 minutes > >>> + * to call epit_set_next_event() or shutdown clock after > >>> + * mode switching > >>> + */ > >>> + epit_irq_enable(epittm); > >>> + local_irq_restore(flags); > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static irqreturn_t epit_timer_interrupt(int irq, void *dev_id) > >>> +{ > >>> + struct clock_event_device *ced = dev_id; > >>> + struct epit_timer *epittm = to_epit_timer(ced); > >>> + > >>> + epit_irq_acknowledge(epittm); > >>> + > >>> + ced->event_handler(ced); > >>> + > >>> + return IRQ_HANDLED; > >>> +} > >>> + > >>> +static int __init epit_clocksource_init(struct epit_timer *epittm) > >>> +{ > >>> + unsigned int c = clk_get_rate(epittm->clk); > >>> + > >>> + 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, > >>> + clocksource_mmio_readl_down); > >>> +} > >>> + > >>> +static int __init epit_clockevent_init(struct epit_timer *epittm) > >>> +{ > >>> + struct clock_event_device *ced = &epittm->ced; > >>> + struct irqaction *act = &epittm->act; > >>> + > >>> + ced->name = "epit"; > >>> + ced->features = CLOCK_set_state_shutdownEVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_DYNIRQ; > >>> + ced->set_state_shutdown = epit_shutdown; > >>> + ced->tick_resume = epit_shutdown; > >>> + ced->set_state_oneshot = epit_set_oneshot; > >>> + ced->set_next_event = epit_set_next_event; > >>> + ced->rating = 200; > >>> + ced->cpumask = cpumask_of(0); > >>> + ced->irq = epittm->irq; > >>> + clockevents_config_and_register(ced, clk_get_rate(epittm->clk), > >>> + 0xff, 0xfffffffe); > >>> + > >>> + act->name = "i.MX EPIT Timer Tick", > >>> + act->flags = IRQF_TIMER | IRQF_IRQPOLL; > >>> + act->handler = epit_timer_interrupt; > >>> + act->dev_id = ced; > >>> + > >>> + /* Make irqs happen */ > >>> + return setup_irq(epittm->irq, act); > >>> +} > >>> + > >>> +static int __init epit_timer_init(struct device_node *np) > >>> +{ > >>> + struct epit_timer *epittm; > >>> + int ret; > >>> + > >>> + epittm = kzalloc(sizeof(*epittm), GFP_KERNEL); > >>> + if (!epittm) > >>> + return -ENOMEM; > >>> + > >>> + epittm->base = of_iomap(np, 0); > >>> + if (!epittm->base) { > >>> + ret = -ENXIO; > >>> + goto out_kfree; > >>> + } > >>> + > >>> + epittm->irq = irq_of_parse_and_map(np, 0); > >>> + if (!epittm->irq) { > >>> + ret = -EINVAL; > >>> + goto out_iounmap; > >>> + } > >>> + > >>> + /* Get EPIT clock */ > >>> + epittm->clk = of_clk_get(np, 0); > >>> + if (IS_ERR(epittm->clk)) { > >>> + pr_err("i.MX EPIT: unable to get clk\n"); > >>> + ret = PTR_ERR(epittm->clk); > >>> + goto out_iounmap; > >>> + } > >>> + > >>> + ret = clk_prepare_enable(epittm->clk); > >>> + if (ret) { > >>> + pr_err("i.MX EPIT: unable to prepare+enable clk\n"); > >>> + goto out_iounmap; > >>> + } > >> > >> Please replace all the above with the timer-of API as: > > Ok I will, > > > > Thanks > > Clement > > > >> > >> https://patchwork.kernel.org/patch/10510443/ > >> > >> > >>> + /* Initialise to a known state (all timers off, and timing reset) */ > >>> + writel_relaxed(0x0, epittm->base + EPITCR); > >>> + writel_relaxed(0xffffffff, epittm->base + EPITLR); > >>> + writel_relaxed(EPITCR_EN | EPITCR_CLKSRC_REF_HIGH | EPITCR_WAITEN, > >>> + epittm->base + EPITCR); > >>> + > >>> + ret = epit_clocksource_init(epittm); > >>> + if (ret) { > >>> + pr_err("i.MX EPIT: failed to init clocksource\n"); > >>> + goto out_clk_disable; > >>> + } > >>> + > >>> + ret = epit_clockevent_init(epittm); > >>> + if (ret) { > >>> + pr_err("i.MX EPIT: failed to init clockevent\n"); > >>> + goto out_clk_disable; > >>> + } > >>> + > >>> + return 0; > >>> + > >>> +out_clk_disable: > >>> + clk_disable_unprepare(epittm->clk); > >>> +out_iounmap: > >>> + iounmap(epittm->base); > >>> +out_kfree: > >>> + kfree(epittm); > >>> + > >>> + return ret; > >>> +} > >>> +TIMER_OF_DECLARE(epit_timer, "fsl,imx31-epit", epit_timer_init); > >>> > >> > >> > >> -- > >> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > >> > >> Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > >> <http://twitter.com/#!/linaroorg> Twitter | > >> <http://www.linaro.org/linaro-blog/> Blog > >> > > > -- > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > > Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | > <http://twitter.com/#!/linaroorg> Twitter | > <http://www.linaro.org/linaro-blog/> Blog >
From: Clément Peron <clement.peron@devialet.com> As suggested in the commit message we have added the device tree support, proper bindings and we moved the driver into the correct folder. Moreover we made some changes like use of relaxed IO accesor, implement sched_clock, delay_timer and reduce the clockevents min_delta. Changes since v6: -fix indent Changes since v5: - change epit to timer in doc example - fix typo in imx6sl.dtsi Changes since v4: - removed ipg clk - change in dt epit to timer - add introduction in doc - add all compatibles in doc - update epit entry for other i.MX device-trees Changes since v3: - Clean Kconfig - Rename imx6q-epit to imx31-epit - Update doc and bindings - Indent and fix Changes since v2 (Thanks Fabio Estevam): - Removed unused ckil clock - Add out_iounmap - Check and handle if clk_prepare_enable failed - Fix comment typo Changes since v1 (Thanks Vladimir Zapolskiy): - Add OF dependency in Kconfig - Sort header - Use BIT macro - Remove useless comments - Fix incorrect indent - Fix memory leak - Add check and handle possible returned error Clément Peron (2): ARM: imx: remove inexistant EPIT timer init dt-bindings: timer: add i.MX EPIT timer binding Colin Didier (3): clk: imx6: add EPIT clock support clocksource: add driver for i.MX EPIT timer ARM: dts: imx: add missing compatible and clock properties for EPIT .../devicetree/bindings/timer/fsl,imxepit.txt | 21 ++ arch/arm/boot/dts/imx25.dtsi | 8 +- arch/arm/boot/dts/imx6qdl.dtsi | 10 +- arch/arm/boot/dts/imx6sl.dtsi | 10 +- arch/arm/boot/dts/imx6sx.dtsi | 10 +- arch/arm/boot/dts/imx6ul.dtsi | 10 +- arch/arm/mach-imx/common.h | 1 - drivers/clk/imx/clk-imx6q.c | 2 + drivers/clocksource/Kconfig | 11 + drivers/clocksource/Makefile | 1 + drivers/clocksource/timer-imx-epit.c | 265 ++++++++++++++++++ include/dt-bindings/clock/imx6qdl-clock.h | 4 +- 12 files changed, 341 insertions(+), 12 deletions(-) create mode 100644 Documentation/devicetree/bindings/timer/fsl,imxepit.txt create mode 100644 drivers/clocksource/timer-imx-epit.c