Re: [PATCH v7 4/5] clocksource: add driver for i.MX EPIT timer

From: ClÃment PÃron
Date: Thu Nov 08 2018 - 08:25:44 EST

Next message: Simon Horman: "Re: [PATCH 4/6] dt-bindings: dmaengine: usb-dmac: Add binding for r8a774a1"
Previous message: Michal Hocko: "Re: [PATCH v2] Document /proc/pid PID reuse behavior"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Daniel,

On Mon, 5 Nov 2018 at 12:48, Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> 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@xxxxxxxxxx> wrote:
> >>
> >> On 11/06/2018 17:50, ClÃment PÃron wrote:
> >>> From: Colin Didier <colin.didier@xxxxxxxxxxxx>
> >>>
> >>> 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@xxxxxxxxxxxx>
> >>> Signed-off-by: ClÃment Peron <clement.peron@xxxxxxxxxxxx>
> >>> Reviewed-by: Vladimir Zapolskiy <vz@xxxxxxxxx>
> >>> Tested-by: Vladimir Zapolskiy <vz@xxxxxxxxx>
> >>> ---
> >>> 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@xxxxxxxxxxxxxx>
> >>> + * Copyright (C) 2018 Colin Didier <colin.didier@xxxxxxxxxxxx>
> >>> + * Copyright (C) 2018 ClÃment PÃron <clement.peron@xxxxxxxxxxxx>
> >>> + */
> >>> +
> >>> +#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
>

Next message: Simon Horman: "Re: [PATCH 4/6] dt-bindings: dmaengine: usb-dmac: Add binding for r8a774a1"
Previous message: Michal Hocko: "Re: [PATCH v2] Document /proc/pid PID reuse behavior"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]