* [RFC 0/2] clocksource: davinci-timer: new driver
@ 2019-04-17 14:47 Bartosz Golaszewski
2019-04-17 14:47 ` [RFC 1/2] clocksource: davinci-timer: add support for clockevents Bartosz Golaszewski
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Bartosz Golaszewski @ 2019-04-17 14:47 UTC (permalink / raw)
To: Sekhar Nori, Kevin Hilman, Daniel Lezcano, Thomas Gleixner,
David Lechner
Cc: linux-arm-kernel, linux-kernel, Bartosz Golaszewski
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Hi Daniel,
as discussed - this is the davinci timer driver split into the clockevent
and clocksource parts.
Since it won't work without all the other (left out for now) changes, I'm
marking it as RFC.
The code has been simplified as requested, the duplicated enums and the
davinci_timer structure have been removed.
Please let me know if that's better. I retested it on da850-lcdk, da830-evm
and dm365-evm.
Bartosz Golaszewski (2):
clocksource: davinci-timer: add support for clockevents
clocksource: timer-davinci: add support for clocksource
drivers/clocksource/Kconfig | 5 +
drivers/clocksource/Makefile | 1 +
drivers/clocksource/timer-davinci.c | 342 ++++++++++++++++++++++++++++
include/clocksource/timer-davinci.h | 44 ++++
4 files changed, 392 insertions(+)
create mode 100644 drivers/clocksource/timer-davinci.c
create mode 100644 include/clocksource/timer-davinci.h
--
2.21.0
^ permalink raw reply [flat|nested] 9+ messages in thread* [RFC 1/2] clocksource: davinci-timer: add support for clockevents 2019-04-17 14:47 [RFC 0/2] clocksource: davinci-timer: new driver Bartosz Golaszewski @ 2019-04-17 14:47 ` Bartosz Golaszewski 2019-05-14 19:55 ` Daniel Lezcano 2019-04-17 14:47 ` [RFC 2/2] clocksource: timer-davinci: add support for clocksource Bartosz Golaszewski 2019-05-13 7:54 ` [RFC 0/2] clocksource: davinci-timer: new driver Bartosz Golaszewski 2 siblings, 1 reply; 9+ messages in thread From: Bartosz Golaszewski @ 2019-04-17 14:47 UTC (permalink / raw) To: Sekhar Nori, Kevin Hilman, Daniel Lezcano, Thomas Gleixner, David Lechner Cc: linux-arm-kernel, linux-kernel, Bartosz Golaszewski From: Bartosz Golaszewski <bgolaszewski@baylibre.com> Currently the clocksource and clockevent support for davinci platforms lives in mach-davinci. It hard-codes many things, uses global variables, implements functionalities unused by any platform and has code fragments scattered across many (often unrelated) files. Implement a new, modern and simplified timer driver and put it into drivers/clocksource. We still need to support legacy board files so export a config structure and a function that allows machine code to register the timer. The timer we're using is 64-bit but can be programmed in dual 32-bit mode (both chained and unchained). We're using dual 32-bit mode to have separate counters for clockevents and clocksource. This patch contains the core code and support for clockevent. The clocksource code will be included in a subsequent patch. Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> --- drivers/clocksource/Kconfig | 5 + drivers/clocksource/Makefile | 1 + drivers/clocksource/timer-davinci.c | 272 ++++++++++++++++++++++++++++ include/clocksource/timer-davinci.h | 44 +++++ 4 files changed, 322 insertions(+) create mode 100644 drivers/clocksource/timer-davinci.c create mode 100644 include/clocksource/timer-davinci.h diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig index 171502a356aa..974f9b50ebf4 100644 --- a/drivers/clocksource/Kconfig +++ b/drivers/clocksource/Kconfig @@ -42,6 +42,11 @@ config BCM_KONA_TIMER help Enables the support for the BCM Kona mobile timer driver. +config DAVINCI_TIMER + bool "Texas Instruments DaVinci timer driver" if COMPILE_TEST + help + Enables the support for the TI DaVinci timer driver. + config DIGICOLOR_TIMER bool "Digicolor timer driver" if COMPILE_TEST select CLKSRC_MMIO diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile index be6e0fbc7489..3c73d0e58b45 100644 --- a/drivers/clocksource/Makefile +++ b/drivers/clocksource/Makefile @@ -15,6 +15,7 @@ obj-$(CONFIG_SH_TIMER_TMU) += sh_tmu.o obj-$(CONFIG_EM_TIMER_STI) += em_sti.o obj-$(CONFIG_CLKBLD_I8253) += i8253.o obj-$(CONFIG_CLKSRC_MMIO) += mmio.o +obj-$(CONFIG_DAVINCI_TIMER) += timer-davinci.o obj-$(CONFIG_DIGICOLOR_TIMER) += timer-digicolor.o obj-$(CONFIG_OMAP_DM_TIMER) += timer-ti-dm.o obj-$(CONFIG_DW_APB_TIMER) += dw_apb_timer.o diff --git a/drivers/clocksource/timer-davinci.c b/drivers/clocksource/timer-davinci.c new file mode 100644 index 000000000000..d30f81a4088e --- /dev/null +++ b/drivers/clocksource/timer-davinci.c @@ -0,0 +1,272 @@ +// SPDX-License-Identifier: GPL-2.0-only +// +// TI DaVinci clocksource driver +// +// Copyright (C) 2019 Texas Instruments +// Author: Bartosz Golaszewski <bgolaszewski@baylibre.com> +// (with tiny parts adopted from code by Kevin Hilman <khilman@baylibre.com>) + +#include <linux/clk.h> +#include <linux/clockchips.h> +#include <linux/interrupt.h> +#include <linux/kernel.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> +#include <linux/sched_clock.h> + +#include <clocksource/timer-davinci.h> + +#undef pr_fmt +#define pr_fmt(fmt) "%s: " fmt "\n", __func__ + +#define DAVINCI_TIMER_REG_TIM12 0x10 +#define DAVINCI_TIMER_REG_TIM34 0x14 +#define DAVINCI_TIMER_REG_PRD12 0x18 +#define DAVINCI_TIMER_REG_PRD34 0x1c +#define DAVINCI_TIMER_REG_TCR 0x20 +#define DAVINCI_TIMER_REG_TGCR 0x24 + +#define DAVINCI_TIMER_TIMMODE_MASK GENMASK(3, 2) +#define DAVINCI_TIMER_RESET_MASK GENMASK(1, 0) +#define DAVINCI_TIMER_TIMMODE_32BIT_UNCHAINED BIT(2) +#define DAVINCI_TIMER_UNRESET GENMASK(1, 0) + +#define DAVINCI_TIMER_ENAMODE_MASK GENMASK(1, 0) +#define DAVINCI_TIMER_ENAMODE_DISABLED 0x00 +#define DAVINCI_TIMER_ENAMODE_ONESHOT BIT(0) +#define DAVINCI_TIMER_ENAMODE_PERIODIC BIT(1) + +#define DAVINCI_TIMER_ENAMODE_SHIFT_TIM12 6 +#define DAVINCI_TIMER_ENAMODE_SHIFT_TIM34 22 + +#define DAVINCI_TIMER_MIN_DELTA 0x01 +#define DAVINCI_TIMER_MAX_DELTA 0xfffffffe + +#define DAVINCI_TIMER_TGCR_DEFAULT \ + (DAVINCI_TIMER_TIMMODE_32BIT_UNCHAINED | DAVINCI_TIMER_UNRESET) + +struct davinci_clockevent { + struct clock_event_device dev; + void __iomem *base; + + unsigned int tim_off; + unsigned int prd_off; + unsigned int cmp_off; + + unsigned int enamode_disabled; + unsigned int enamode_oneshot; + unsigned int enamode_periodic; + unsigned int enamode_mask; +}; + +static struct davinci_clockevent * +to_davinci_clockevent(struct clock_event_device *clockevent) +{ + return container_of(clockevent, struct davinci_clockevent, dev); +} + +static unsigned int +davinci_clockevent_read(struct davinci_clockevent *clockevent, + unsigned int reg) +{ + return readl_relaxed(clockevent->base + reg); +} + +static void davinci_clockevent_write(struct davinci_clockevent *clockevent, + unsigned int reg, unsigned int val) +{ + writel_relaxed(val, clockevent->base + reg); +} + +static void davinci_reg_update(void __iomem *base, unsigned int reg, + unsigned int mask, unsigned int val) +{ + unsigned int new, orig; + + orig = readl_relaxed(base + reg); + new = orig & ~mask; + new |= val & mask; + + writel_relaxed(new, base + reg); +} + +static void davinci_clockevent_update(struct davinci_clockevent *clockevent, + unsigned int reg, unsigned int mask, + unsigned int val) +{ + davinci_reg_update(clockevent->base, reg, mask, val); +} + +static int +davinci_clockevent_set_next_event_std(unsigned long cycles, + struct clock_event_device *dev) +{ + struct davinci_clockevent *clockevent; + unsigned int enamode; + + clockevent = to_davinci_clockevent(dev); + enamode = clockevent->enamode_disabled; + + davinci_clockevent_update(clockevent, DAVINCI_TIMER_REG_TCR, + clockevent->enamode_mask, + clockevent->enamode_disabled); + + davinci_clockevent_write(clockevent, clockevent->tim_off, 0x0); + davinci_clockevent_write(clockevent, clockevent->prd_off, cycles); + + if (clockevent_state_oneshot(&clockevent->dev)) + enamode = clockevent->enamode_oneshot; + else if (clockevent_state_periodic(&clockevent->dev)) + enamode = clockevent->enamode_periodic; + + davinci_clockevent_update(clockevent, DAVINCI_TIMER_REG_TCR, + clockevent->enamode_mask, enamode); + + return 0; +} + +static int +davinci_clockevent_set_next_event_cmp(unsigned long cycles, + struct clock_event_device *dev) +{ + struct davinci_clockevent *clockevent = to_davinci_clockevent(dev); + unsigned int curr_time; + + curr_time = davinci_clockevent_read(clockevent, clockevent->tim_off); + davinci_clockevent_write(clockevent, + clockevent->cmp_off, curr_time + cycles); + + return 0; +} + +static irqreturn_t davinci_timer_irq_timer(int irq, void *data) +{ + struct davinci_clockevent *clockevent = data; + + clockevent->dev.event_handler(&clockevent->dev); + + return IRQ_HANDLED; +} + +static void davinci_timer_init(void __iomem *base) +{ + /* Set clock to internal mode and disable it. */ + writel_relaxed(0x0, base + DAVINCI_TIMER_REG_TCR); + /* + * Reset both 32-bit timers, set no prescaler for timer 34, set the + * timer to dual 32-bit unchained mode, unreset both 32-bit timers. + */ + writel_relaxed(DAVINCI_TIMER_TGCR_DEFAULT, + base + DAVINCI_TIMER_REG_TGCR); + /* Init both counters to zero. */ + writel_relaxed(0x0, base + DAVINCI_TIMER_REG_TIM12); + writel_relaxed(0x0, base + DAVINCI_TIMER_REG_TIM34); +} + +int __init davinci_timer_register(struct clk *clk, + const struct davinci_timer_cfg *timer_cfg) +{ + struct davinci_clockevent *clockevent; + unsigned int tick_rate, shift; + void __iomem *base; + int rv; + + rv = clk_prepare_enable(clk); + if (rv) { + pr_err("Unable to prepare and enable the timer clock"); + return rv; + } + + base = request_mem_region(timer_cfg->reg.start, + resource_size(&timer_cfg->reg), + "davinci-timer"); + if (!base) { + pr_err("Unable to request memory region"); + return -EBUSY; + } + + base = ioremap(timer_cfg->reg.start, resource_size(&timer_cfg->reg)); + if (!base) { + pr_err("Unable to map the register range"); + return -ENOMEM; + } + + davinci_timer_init(base); + tick_rate = clk_get_rate(clk); + + clockevent = kzalloc(sizeof(*clockevent), GFP_KERNEL); + if (!clockevent) { + pr_err("Error allocating memory for clockevent data"); + return -ENOMEM; + } + + clockevent->dev.name = "tim12"; + clockevent->dev.features = CLOCK_EVT_FEAT_ONESHOT; + clockevent->dev.cpumask = cpumask_of(0); + + clockevent->base = base; + clockevent->tim_off = DAVINCI_TIMER_REG_TIM12; + clockevent->prd_off = DAVINCI_TIMER_REG_PRD12; + + shift = DAVINCI_TIMER_ENAMODE_SHIFT_TIM12; + clockevent->enamode_disabled = DAVINCI_TIMER_ENAMODE_DISABLED << shift; + clockevent->enamode_oneshot = DAVINCI_TIMER_ENAMODE_ONESHOT << shift; + clockevent->enamode_periodic = DAVINCI_TIMER_ENAMODE_PERIODIC << shift; + clockevent->enamode_mask = DAVINCI_TIMER_ENAMODE_MASK << shift; + + if (timer_cfg->cmp_off) { + clockevent->cmp_off = timer_cfg->cmp_off; + clockevent->dev.set_next_event = + davinci_clockevent_set_next_event_cmp; + } else { + clockevent->dev.set_next_event = + davinci_clockevent_set_next_event_std; + } + + rv = request_irq(timer_cfg->irq[DAVINCI_TIMER_CLOCKEVENT_IRQ].start, + davinci_timer_irq_timer, IRQF_TIMER, + "clockevent", clockevent); + if (rv) { + pr_err("Unable to request the clockevent interrupt"); + return rv; + } + + clockevents_config_and_register(&clockevent->dev, tick_rate, + DAVINCI_TIMER_MIN_DELTA, + DAVINCI_TIMER_MAX_DELTA); + + return 0; +} + +static int __init of_davinci_timer_register(struct device_node *np) +{ + struct davinci_timer_cfg timer_cfg = { }; + struct clk *clk; + int rv; + + rv = of_address_to_resource(np, 0, &timer_cfg.reg); + if (rv) { + pr_err("Unable to get the register range for timer"); + return rv; + } + + rv = of_irq_to_resource_table(np, timer_cfg.irq, + DAVINCI_TIMER_NUM_IRQS); + if (rv != DAVINCI_TIMER_NUM_IRQS) { + pr_err("Unable to get the interrupts for timer"); + return rv; + } + + clk = of_clk_get(np, 0); + if (IS_ERR(clk)) { + pr_err("Unable to get the timer clock"); + return PTR_ERR(clk); + } + + rv = davinci_timer_register(clk, &timer_cfg); + if (rv) + clk_put(clk); + + return rv; +} +TIMER_OF_DECLARE(davinci_timer, "ti,da830-timer", of_davinci_timer_register); diff --git a/include/clocksource/timer-davinci.h b/include/clocksource/timer-davinci.h new file mode 100644 index 000000000000..1dcc1333fbc8 --- /dev/null +++ b/include/clocksource/timer-davinci.h @@ -0,0 +1,44 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * TI DaVinci clocksource driver + * + * Copyright (C) 2019 Texas Instruments + * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com> + */ + +#ifndef __TIMER_DAVINCI_H__ +#define __TIMER_DAVINCI_H__ + +#include <linux/clk.h> +#include <linux/ioport.h> + +enum { + DAVINCI_TIMER_CLOCKEVENT_IRQ, + DAVINCI_TIMER_CLOCKSOURCE_IRQ, + DAVINCI_TIMER_NUM_IRQS, +}; + +/** + * struct davinci_timer_cfg - davinci clocksource driver configuration struct + * @reg: register range resource + * @irq: clockevent and clocksource interrupt resources + * @cmp_off: if set - it specifies the compare register used for clockevent + * + * Note: if the compare register is specified, the driver will use the bottom + * clock half for both clocksource and clockevent and the compare register + * to generate event irqs. The user must supply the correct compare register + * interrupt number. + * + * This is only used by da830 the DSP of which uses the top half. The timer + * driver still configures the top half to run in free-run mode. + */ +struct davinci_timer_cfg { + struct resource reg; + struct resource irq[DAVINCI_TIMER_NUM_IRQS]; + unsigned int cmp_off; +}; + +int __init davinci_timer_register(struct clk *clk, + const struct davinci_timer_cfg *data); + +#endif /* __TIMER_DAVINCI_H__ */ -- 2.21.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC 1/2] clocksource: davinci-timer: add support for clockevents 2019-04-17 14:47 ` [RFC 1/2] clocksource: davinci-timer: add support for clockevents Bartosz Golaszewski @ 2019-05-14 19:55 ` Daniel Lezcano 2019-05-23 12:58 ` Bartosz Golaszewski 0 siblings, 1 reply; 9+ messages in thread From: Daniel Lezcano @ 2019-05-14 19:55 UTC (permalink / raw) To: Bartosz Golaszewski, Sekhar Nori, Kevin Hilman, Thomas Gleixner, David Lechner Cc: linux-arm-kernel, linux-kernel, Bartosz Golaszewski On 17/04/2019 16:47, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Currently the clocksource and clockevent support for davinci platforms > lives in mach-davinci. It hard-codes many things, uses global variables, > implements functionalities unused by any platform and has code fragments > scattered across many (often unrelated) files. > > Implement a new, modern and simplified timer driver and put it into > drivers/clocksource. We still need to support legacy board files so > export a config structure and a function that allows machine code to > register the timer. > > The timer we're using is 64-bit but can be programmed in dual 32-bit > mode (both chained and unchained). We're using dual 32-bit mode to > have separate counters for clockevents and clocksource. > > This patch contains the core code and support for clockevent. The > clocksource code will be included in a subsequent patch. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- > drivers/clocksource/Kconfig | 5 + > drivers/clocksource/Makefile | 1 + > drivers/clocksource/timer-davinci.c | 272 ++++++++++++++++++++++++++++ > include/clocksource/timer-davinci.h | 44 +++++ > 4 files changed, 322 insertions(+) > create mode 100644 drivers/clocksource/timer-davinci.c > create mode 100644 include/clocksource/timer-davinci.h > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > index 171502a356aa..974f9b50ebf4 100644 > --- a/drivers/clocksource/Kconfig > +++ b/drivers/clocksource/Kconfig > @@ -42,6 +42,11 @@ config BCM_KONA_TIMER > help > Enables the support for the BCM Kona mobile timer driver. > > +config DAVINCI_TIMER > + bool "Texas Instruments DaVinci timer driver" if COMPILE_TEST > + help > + Enables the support for the TI DaVinci timer driver. > + > config DIGICOLOR_TIMER > bool "Digicolor timer driver" if COMPILE_TEST > select CLKSRC_MMIO > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile > index be6e0fbc7489..3c73d0e58b45 100644 > --- a/drivers/clocksource/Makefile > +++ b/drivers/clocksource/Makefile > @@ -15,6 +15,7 @@ obj-$(CONFIG_SH_TIMER_TMU) += sh_tmu.o > obj-$(CONFIG_EM_TIMER_STI) += em_sti.o > obj-$(CONFIG_CLKBLD_I8253) += i8253.o > obj-$(CONFIG_CLKSRC_MMIO) += mmio.o > +obj-$(CONFIG_DAVINCI_TIMER) += timer-davinci.o > obj-$(CONFIG_DIGICOLOR_TIMER) += timer-digicolor.o > obj-$(CONFIG_OMAP_DM_TIMER) += timer-ti-dm.o > obj-$(CONFIG_DW_APB_TIMER) += dw_apb_timer.o > diff --git a/drivers/clocksource/timer-davinci.c b/drivers/clocksource/timer-davinci.c > new file mode 100644 > index 000000000000..d30f81a4088e > --- /dev/null > +++ b/drivers/clocksource/timer-davinci.c > @@ -0,0 +1,272 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +// > +// TI DaVinci clocksource driver > +// > +// Copyright (C) 2019 Texas Instruments > +// Author: Bartosz Golaszewski <bgolaszewski@baylibre.com> > +// (with tiny parts adopted from code by Kevin Hilman <khilman@baylibre.com>) The header format is wrong, it should be: // SPDX-License-Identifier: GPL-2.0-only /* * TI DaVinci clocksource driver * * ... * ... * */ > +#include <linux/clk.h> > +#include <linux/clockchips.h> > +#include <linux/interrupt.h> > +#include <linux/kernel.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h> > +#include <linux/sched_clock.h> > + > +#include <clocksource/timer-davinci.h> > + > +#undef pr_fmt > +#define pr_fmt(fmt) "%s: " fmt "\n", __func__ > + > +#define DAVINCI_TIMER_REG_TIM12 0x10 > +#define DAVINCI_TIMER_REG_TIM34 0x14 > +#define DAVINCI_TIMER_REG_PRD12 0x18 > +#define DAVINCI_TIMER_REG_PRD34 0x1c > +#define DAVINCI_TIMER_REG_TCR 0x20 > +#define DAVINCI_TIMER_REG_TGCR 0x24 > + > +#define DAVINCI_TIMER_TIMMODE_MASK GENMASK(3, 2) > +#define DAVINCI_TIMER_RESET_MASK GENMASK(1, 0) > +#define DAVINCI_TIMER_TIMMODE_32BIT_UNCHAINED BIT(2) > +#define DAVINCI_TIMER_UNRESET GENMASK(1, 0) > + > +#define DAVINCI_TIMER_ENAMODE_MASK GENMASK(1, 0) > +#define DAVINCI_TIMER_ENAMODE_DISABLED 0x00 > +#define DAVINCI_TIMER_ENAMODE_ONESHOT BIT(0) > +#define DAVINCI_TIMER_ENAMODE_PERIODIC BIT(1) > + > +#define DAVINCI_TIMER_ENAMODE_SHIFT_TIM12 6 > +#define DAVINCI_TIMER_ENAMODE_SHIFT_TIM34 22 > + > +#define DAVINCI_TIMER_MIN_DELTA 0x01 > +#define DAVINCI_TIMER_MAX_DELTA 0xfffffffe > + > +#define DAVINCI_TIMER_TGCR_DEFAULT \ > + (DAVINCI_TIMER_TIMMODE_32BIT_UNCHAINED | DAVINCI_TIMER_UNRESET) > + > +struct davinci_clockevent { > + struct clock_event_device dev; > + void __iomem *base; > + > + unsigned int tim_off; > + unsigned int prd_off; > + unsigned int cmp_off; > + > + unsigned int enamode_disabled; > + unsigned int enamode_oneshot; > + unsigned int enamode_periodic; > + unsigned int enamode_mask; > +}; > + > +static struct davinci_clockevent * > +to_davinci_clockevent(struct clock_event_device *clockevent) > +{ > + return container_of(clockevent, struct davinci_clockevent, dev); > +} > + > +static unsigned int > +davinci_clockevent_read(struct davinci_clockevent *clockevent, > + unsigned int reg) > +{ > + return readl_relaxed(clockevent->base + reg); > +} > + > +static void davinci_clockevent_write(struct davinci_clockevent *clockevent, > + unsigned int reg, unsigned int val) > +{ > + writel_relaxed(val, clockevent->base + reg); > +} > + > +static void davinci_reg_update(void __iomem *base, unsigned int reg, > + unsigned int mask, unsigned int val) > +{ > + unsigned int new, orig; > + > + orig = readl_relaxed(base + reg); > + new = orig & ~mask; > + new |= val & mask; > + > + writel_relaxed(new, base + reg); May be worth to improve this routine? https://lkml.org/lkml/2019/4/5/837 > +} > + > +static void davinci_clockevent_update(struct davinci_clockevent *clockevent, > + unsigned int reg, unsigned int mask, > + unsigned int val) > +{ > + davinci_reg_update(clockevent->base, reg, mask, val); > +} > + > +static int > +davinci_clockevent_set_next_event_std(unsigned long cycles, > + struct clock_event_device *dev) > +{ > + struct davinci_clockevent *clockevent; > + unsigned int enamode; > + > + clockevent = to_davinci_clockevent(dev); > + enamode = clockevent->enamode_disabled; > + > + davinci_clockevent_update(clockevent, DAVINCI_TIMER_REG_TCR, > + clockevent->enamode_mask, > + clockevent->enamode_disabled); What is for this function with the DAVINCI_TIMER_REG_TCR parameter? > + davinci_clockevent_write(clockevent, clockevent->tim_off, 0x0); > + davinci_clockevent_write(clockevent, clockevent->prd_off, cycles); > + > + if (clockevent_state_oneshot(&clockevent->dev)) > + enamode = clockevent->enamode_oneshot; > + else if (clockevent_state_periodic(&clockevent->dev)) > + enamode = clockevent->enamode_periodic; If none of the conditions above are fulfilled, davinci_clockevent_update will be called with the default enamode set to enomode_disabled, we don't want to disable the timer again. Use the set_state_oneshot / set_state_periodic callbacks to fill the right value for the 'enamode' and use it unconditionally here. > + davinci_clockevent_update(clockevent, DAVINCI_TIMER_REG_TCR, > + clockevent->enamode_mask, enamode); > + > + return 0; > +} > + > +static int > +davinci_clockevent_set_next_event_cmp(unsigned long cycles, > + struct clock_event_device *dev) > +{ > + struct davinci_clockevent *clockevent = to_davinci_clockevent(dev); > + unsigned int curr_time; > + > + curr_time = davinci_clockevent_read(clockevent, clockevent->tim_off); > + davinci_clockevent_write(clockevent, > + clockevent->cmp_off, curr_time + cycles); > + > + return 0; > +} > + > +static irqreturn_t davinci_timer_irq_timer(int irq, void *data) > +{ > + struct davinci_clockevent *clockevent = data; > + > + clockevent->dev.event_handler(&clockevent->dev); Why isn't disabled here if non-periodic? And set next event if periodic ? cf. timer-stm32.c > + return IRQ_HANDLED; > +} > + > +static void davinci_timer_init(void __iomem *base) > +{ > + /* Set clock to internal mode and disable it. */ > + writel_relaxed(0x0, base + DAVINCI_TIMER_REG_TCR); > + /* > + * Reset both 32-bit timers, set no prescaler for timer 34, set the > + * timer to dual 32-bit unchained mode, unreset both 32-bit timers. > + */ > + writel_relaxed(DAVINCI_TIMER_TGCR_DEFAULT, > + base + DAVINCI_TIMER_REG_TGCR); > + /* Init both counters to zero. */ > + writel_relaxed(0x0, base + DAVINCI_TIMER_REG_TIM12); > + writel_relaxed(0x0, base + DAVINCI_TIMER_REG_TIM34); > +} > + > +int __init davinci_timer_register(struct clk *clk, > + const struct davinci_timer_cfg *timer_cfg) > +{ > + struct davinci_clockevent *clockevent; > + unsigned int tick_rate, shift; > + void __iomem *base; > + int rv; > + > + rv = clk_prepare_enable(clk); > + if (rv) { > + pr_err("Unable to prepare and enable the timer clock"); > + return rv; > + } > + > + base = request_mem_region(timer_cfg->reg.start, > + resource_size(&timer_cfg->reg), > + "davinci-timer"); > + if (!base) { > + pr_err("Unable to request memory region"); > + return -EBUSY; > + } > + > + base = ioremap(timer_cfg->reg.start, resource_size(&timer_cfg->reg)); > + if (!base) { > + pr_err("Unable to map the register range"); > + return -ENOMEM; > + } > + > + davinci_timer_init(base); > + tick_rate = clk_get_rate(clk); > + > + clockevent = kzalloc(sizeof(*clockevent), GFP_KERNEL); > + if (!clockevent) { > + pr_err("Error allocating memory for clockevent data"); > + return -ENOMEM; > + } > + > + clockevent->dev.name = "tim12"; > + clockevent->dev.features = CLOCK_EVT_FEAT_ONESHOT; If the timer does not have the periodic feature, why all the enamode_periodic ? > + clockevent->dev.cpumask = cpumask_of(0); > + > + clockevent->base = base; > + clockevent->tim_off = DAVINCI_TIMER_REG_TIM12; > + clockevent->prd_off = DAVINCI_TIMER_REG_PRD12; > + > + shift = DAVINCI_TIMER_ENAMODE_SHIFT_TIM12; > + clockevent->enamode_disabled = DAVINCI_TIMER_ENAMODE_DISABLED << shift; > + clockevent->enamode_oneshot = DAVINCI_TIMER_ENAMODE_ONESHOT << shift; > + clockevent->enamode_periodic = DAVINCI_TIMER_ENAMODE_PERIODIC << shift; > + clockevent->enamode_mask = DAVINCI_TIMER_ENAMODE_MASK << shift; I don't see where 'shift' can be different from TIM12 here neither in the second patch for those values. Why create these fields instead of pre-computed macros? > + if (timer_cfg->cmp_off) { > + clockevent->cmp_off = timer_cfg->cmp_off; > + clockevent->dev.set_next_event = > + davinci_clockevent_set_next_event_cmp; > + } else { > + clockevent->dev.set_next_event = > + davinci_clockevent_set_next_event_std; > + } > + > + rv = request_irq(timer_cfg->irq[DAVINCI_TIMER_CLOCKEVENT_IRQ].start, > + davinci_timer_irq_timer, IRQF_TIMER, > + "clockevent", clockevent); May be replace "clockevent" by eg. "tim12"? > + if (rv) { > + pr_err("Unable to request the clockevent interrupt"); > + return rv; > + } > + > + clockevents_config_and_register(&clockevent->dev, tick_rate, > + DAVINCI_TIMER_MIN_DELTA, > + DAVINCI_TIMER_MAX_DELTA); > + > + return 0; > +} > + > +static int __init of_davinci_timer_register(struct device_node *np) > +{ > + struct davinci_timer_cfg timer_cfg = { }; > + struct clk *clk; > + int rv; > + > + rv = of_address_to_resource(np, 0, &timer_cfg.reg); > + if (rv) { > + pr_err("Unable to get the register range for timer"); > + return rv; > + } > + > + rv = of_irq_to_resource_table(np, timer_cfg.irq, > + DAVINCI_TIMER_NUM_IRQS); > + if (rv != DAVINCI_TIMER_NUM_IRQS) { > + pr_err("Unable to get the interrupts for timer"); > + return rv; > + } > + > + clk = of_clk_get(np, 0); > + if (IS_ERR(clk)) { > + pr_err("Unable to get the timer clock"); > + return PTR_ERR(clk); > + } > + > + rv = davinci_timer_register(clk, &timer_cfg); > + if (rv) > + clk_put(clk); > + > + return rv; > +} > +TIMER_OF_DECLARE(davinci_timer, "ti,da830-timer", of_davinci_timer_register); > diff --git a/include/clocksource/timer-davinci.h b/include/clocksource/timer-davinci.h > new file mode 100644 > index 000000000000..1dcc1333fbc8 > --- /dev/null > +++ b/include/clocksource/timer-davinci.h > @@ -0,0 +1,44 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * TI DaVinci clocksource driver > + * > + * Copyright (C) 2019 Texas Instruments > + * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com> > + */ > + > +#ifndef __TIMER_DAVINCI_H__ > +#define __TIMER_DAVINCI_H__ > + > +#include <linux/clk.h> > +#include <linux/ioport.h> > + > +enum { > + DAVINCI_TIMER_CLOCKEVENT_IRQ, > + DAVINCI_TIMER_CLOCKSOURCE_IRQ, > + DAVINCI_TIMER_NUM_IRQS, > +}; > + > +/** > + * struct davinci_timer_cfg - davinci clocksource driver configuration struct > + * @reg: register range resource > + * @irq: clockevent and clocksource interrupt resources > + * @cmp_off: if set - it specifies the compare register used for clockevent > + * > + * Note: if the compare register is specified, the driver will use the bottom > + * clock half for both clocksource and clockevent and the compare register > + * to generate event irqs. The user must supply the correct compare register > + * interrupt number. > + * > + * This is only used by da830 the DSP of which uses the top half. The timer > + * driver still configures the top half to run in free-run mode. > + */ > +struct davinci_timer_cfg { > + struct resource reg; > + struct resource irq[DAVINCI_TIMER_NUM_IRQS]; > + unsigned int cmp_off; > +}; > + > +int __init davinci_timer_register(struct clk *clk, > + const struct davinci_timer_cfg *data); > + > +#endif /* __TIMER_DAVINCI_H__ */ > -- <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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC 1/2] clocksource: davinci-timer: add support for clockevents 2019-05-14 19:55 ` Daniel Lezcano @ 2019-05-23 12:58 ` Bartosz Golaszewski 2019-05-23 13:25 ` Daniel Lezcano 0 siblings, 1 reply; 9+ messages in thread From: Bartosz Golaszewski @ 2019-05-23 12:58 UTC (permalink / raw) To: Daniel Lezcano Cc: Sekhar Nori, Kevin Hilman, Thomas Gleixner, David Lechner, Linux ARM, Linux Kernel Mailing List, Bartosz Golaszewski wt., 14 maj 2019 o 21:55 Daniel Lezcano <daniel.lezcano@linaro.org> napisał(a): > > On 17/04/2019 16:47, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > Currently the clocksource and clockevent support for davinci platforms > > lives in mach-davinci. It hard-codes many things, uses global variables, > > implements functionalities unused by any platform and has code fragments > > scattered across many (often unrelated) files. > > > > Implement a new, modern and simplified timer driver and put it into > > drivers/clocksource. We still need to support legacy board files so > > export a config structure and a function that allows machine code to > > register the timer. > > > > The timer we're using is 64-bit but can be programmed in dual 32-bit > > mode (both chained and unchained). We're using dual 32-bit mode to > > have separate counters for clockevents and clocksource. > > > > This patch contains the core code and support for clockevent. The > > clocksource code will be included in a subsequent patch. > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > --- > > drivers/clocksource/Kconfig | 5 + > > drivers/clocksource/Makefile | 1 + > > drivers/clocksource/timer-davinci.c | 272 ++++++++++++++++++++++++++++ > > include/clocksource/timer-davinci.h | 44 +++++ > > 4 files changed, 322 insertions(+) > > create mode 100644 drivers/clocksource/timer-davinci.c > > create mode 100644 include/clocksource/timer-davinci.h > > > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > > index 171502a356aa..974f9b50ebf4 100644 > > --- a/drivers/clocksource/Kconfig > > +++ b/drivers/clocksource/Kconfig > > @@ -42,6 +42,11 @@ config BCM_KONA_TIMER > > help > > Enables the support for the BCM Kona mobile timer driver. > > > > +config DAVINCI_TIMER > > + bool "Texas Instruments DaVinci timer driver" if COMPILE_TEST > > + help > > + Enables the support for the TI DaVinci timer driver. > > + > > config DIGICOLOR_TIMER > > bool "Digicolor timer driver" if COMPILE_TEST > > select CLKSRC_MMIO > > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile > > index be6e0fbc7489..3c73d0e58b45 100644 > > --- a/drivers/clocksource/Makefile > > +++ b/drivers/clocksource/Makefile > > @@ -15,6 +15,7 @@ obj-$(CONFIG_SH_TIMER_TMU) += sh_tmu.o > > obj-$(CONFIG_EM_TIMER_STI) += em_sti.o > > obj-$(CONFIG_CLKBLD_I8253) += i8253.o > > obj-$(CONFIG_CLKSRC_MMIO) += mmio.o > > +obj-$(CONFIG_DAVINCI_TIMER) += timer-davinci.o > > obj-$(CONFIG_DIGICOLOR_TIMER) += timer-digicolor.o > > obj-$(CONFIG_OMAP_DM_TIMER) += timer-ti-dm.o > > obj-$(CONFIG_DW_APB_TIMER) += dw_apb_timer.o > > diff --git a/drivers/clocksource/timer-davinci.c b/drivers/clocksource/timer-davinci.c > > new file mode 100644 > > index 000000000000..d30f81a4088e > > --- /dev/null > > +++ b/drivers/clocksource/timer-davinci.c > > @@ -0,0 +1,272 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +// > > +// TI DaVinci clocksource driver > > +// > > +// Copyright (C) 2019 Texas Instruments > > +// Author: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > +// (with tiny parts adopted from code by Kevin Hilman <khilman@baylibre.com>) > > The header format is wrong, it should be: > > // SPDX-License-Identifier: GPL-2.0-only > /* > * TI DaVinci clocksource driver > * > * ... > * ... > * > */ It's not wrong. It looks like it's at the maintainers discretion and I've been asked to use both forms by different maintainers. Seems you just can't get it right. :) I've changed it in v2 though. > > > +#include <linux/clk.h> > > +#include <linux/clockchips.h> > > +#include <linux/interrupt.h> > > +#include <linux/kernel.h> > > +#include <linux/of_address.h> > > +#include <linux/of_irq.h> > > +#include <linux/sched_clock.h> > > + > > +#include <clocksource/timer-davinci.h> > > + > > +#undef pr_fmt > > +#define pr_fmt(fmt) "%s: " fmt "\n", __func__ > > + > > +#define DAVINCI_TIMER_REG_TIM12 0x10 > > +#define DAVINCI_TIMER_REG_TIM34 0x14 > > +#define DAVINCI_TIMER_REG_PRD12 0x18 > > +#define DAVINCI_TIMER_REG_PRD34 0x1c > > +#define DAVINCI_TIMER_REG_TCR 0x20 > > +#define DAVINCI_TIMER_REG_TGCR 0x24 > > + > > +#define DAVINCI_TIMER_TIMMODE_MASK GENMASK(3, 2) > > +#define DAVINCI_TIMER_RESET_MASK GENMASK(1, 0) > > +#define DAVINCI_TIMER_TIMMODE_32BIT_UNCHAINED BIT(2) > > +#define DAVINCI_TIMER_UNRESET GENMASK(1, 0) > > + > > +#define DAVINCI_TIMER_ENAMODE_MASK GENMASK(1, 0) > > +#define DAVINCI_TIMER_ENAMODE_DISABLED 0x00 > > +#define DAVINCI_TIMER_ENAMODE_ONESHOT BIT(0) > > +#define DAVINCI_TIMER_ENAMODE_PERIODIC BIT(1) > > + > > +#define DAVINCI_TIMER_ENAMODE_SHIFT_TIM12 6 > > +#define DAVINCI_TIMER_ENAMODE_SHIFT_TIM34 22 > > + > > +#define DAVINCI_TIMER_MIN_DELTA 0x01 > > +#define DAVINCI_TIMER_MAX_DELTA 0xfffffffe > > + > > +#define DAVINCI_TIMER_TGCR_DEFAULT \ > > + (DAVINCI_TIMER_TIMMODE_32BIT_UNCHAINED | DAVINCI_TIMER_UNRESET) > > + > > +struct davinci_clockevent { > > + struct clock_event_device dev; > > + void __iomem *base; > > + > > + unsigned int tim_off; > > + unsigned int prd_off; > > + unsigned int cmp_off; > > + > > + unsigned int enamode_disabled; > > + unsigned int enamode_oneshot; > > + unsigned int enamode_periodic; > > + unsigned int enamode_mask; > > +}; > > + > > +static struct davinci_clockevent * > > +to_davinci_clockevent(struct clock_event_device *clockevent) > > +{ > > + return container_of(clockevent, struct davinci_clockevent, dev); > > +} > > + > > +static unsigned int > > +davinci_clockevent_read(struct davinci_clockevent *clockevent, > > + unsigned int reg) > > +{ > > + return readl_relaxed(clockevent->base + reg); > > +} > > + > > +static void davinci_clockevent_write(struct davinci_clockevent *clockevent, > > + unsigned int reg, unsigned int val) > > +{ > > + writel_relaxed(val, clockevent->base + reg); > > +} > > + > > +static void davinci_reg_update(void __iomem *base, unsigned int reg, > > + unsigned int mask, unsigned int val) > > +{ > > + unsigned int new, orig; > > + > > + orig = readl_relaxed(base + reg); > > + new = orig & ~mask; > > + new |= val & mask; > > + > > + writel_relaxed(new, base + reg); > > May be worth to improve this routine? > > https://lkml.org/lkml/2019/4/5/837 > > > +} > > + > > +static void davinci_clockevent_update(struct davinci_clockevent *clockevent, > > + unsigned int reg, unsigned int mask, > > + unsigned int val) > > +{ > > + davinci_reg_update(clockevent->base, reg, mask, val); > > +} > > + > > +static int > > +davinci_clockevent_set_next_event_std(unsigned long cycles, > > + struct clock_event_device *dev) > > +{ > > + struct davinci_clockevent *clockevent; > > + unsigned int enamode; > > + > > + clockevent = to_davinci_clockevent(dev); > > + enamode = clockevent->enamode_disabled; > > + > > + davinci_clockevent_update(clockevent, DAVINCI_TIMER_REG_TCR, > > + clockevent->enamode_mask, > > + clockevent->enamode_disabled); > > What is for this function with the DAVINCI_TIMER_REG_TCR parameter? I'm not sure I understand the question. :( > > > + davinci_clockevent_write(clockevent, clockevent->tim_off, 0x0); > > + davinci_clockevent_write(clockevent, clockevent->prd_off, cycles); > > + > > + if (clockevent_state_oneshot(&clockevent->dev)) > > + enamode = clockevent->enamode_oneshot; > > + else if (clockevent_state_periodic(&clockevent->dev)) > > + enamode = clockevent->enamode_periodic; > > If none of the conditions above are fulfilled, davinci_clockevent_update > will be called with the default enamode set to enomode_disabled, we > don't want to disable the timer again. > > Use the set_state_oneshot / set_state_periodic callbacks to fill the > right value for the 'enamode' and use it unconditionally here. > > > + davinci_clockevent_update(clockevent, DAVINCI_TIMER_REG_TCR, > > + clockevent->enamode_mask, enamode); > > + > > + return 0; > > +} > > + > > +static int > > +davinci_clockevent_set_next_event_cmp(unsigned long cycles, > > + struct clock_event_device *dev) > > +{ > > + struct davinci_clockevent *clockevent = to_davinci_clockevent(dev); > > + unsigned int curr_time; > > + > > + curr_time = davinci_clockevent_read(clockevent, clockevent->tim_off); > > + davinci_clockevent_write(clockevent, > > + clockevent->cmp_off, curr_time + cycles); > > + > > + return 0; > > +} > > + > > +static irqreturn_t davinci_timer_irq_timer(int irq, void *data) > > +{ > > + struct davinci_clockevent *clockevent = data; > > + > > + clockevent->dev.event_handler(&clockevent->dev); > > > Why isn't disabled here if non-periodic? > > And set next event if periodic ? > > cf. timer-stm32.c > > > + return IRQ_HANDLED; > > +} > > + > > +static void davinci_timer_init(void __iomem *base) > > +{ > > + /* Set clock to internal mode and disable it. */ > > + writel_relaxed(0x0, base + DAVINCI_TIMER_REG_TCR); > > + /* > > + * Reset both 32-bit timers, set no prescaler for timer 34, set the > > + * timer to dual 32-bit unchained mode, unreset both 32-bit timers. > > + */ > > + writel_relaxed(DAVINCI_TIMER_TGCR_DEFAULT, > > + base + DAVINCI_TIMER_REG_TGCR); > > + /* Init both counters to zero. */ > > + writel_relaxed(0x0, base + DAVINCI_TIMER_REG_TIM12); > > + writel_relaxed(0x0, base + DAVINCI_TIMER_REG_TIM34); > > +} > > + > > +int __init davinci_timer_register(struct clk *clk, > > + const struct davinci_timer_cfg *timer_cfg) > > +{ > > + struct davinci_clockevent *clockevent; > > + unsigned int tick_rate, shift; > > + void __iomem *base; > > + int rv; > > + > > + rv = clk_prepare_enable(clk); > > + if (rv) { > > + pr_err("Unable to prepare and enable the timer clock"); > > + return rv; > > + } > > + > > + base = request_mem_region(timer_cfg->reg.start, > > + resource_size(&timer_cfg->reg), > > + "davinci-timer"); > > + if (!base) { > > + pr_err("Unable to request memory region"); > > + return -EBUSY; > > + } > > + > > + base = ioremap(timer_cfg->reg.start, resource_size(&timer_cfg->reg)); > > + if (!base) { > > + pr_err("Unable to map the register range"); > > + return -ENOMEM; > > + } > > + > > + davinci_timer_init(base); > > + tick_rate = clk_get_rate(clk); > > + > > + clockevent = kzalloc(sizeof(*clockevent), GFP_KERNEL); > > + if (!clockevent) { > > + pr_err("Error allocating memory for clockevent data"); > > + return -ENOMEM; > > + } > > + > > + clockevent->dev.name = "tim12"; > > + clockevent->dev.features = CLOCK_EVT_FEAT_ONESHOT; > > If the timer does not have the periodic feature, why all the > enamode_periodic ? > > > + clockevent->dev.cpumask = cpumask_of(0); > > + > > + clockevent->base = base; > > + clockevent->tim_off = DAVINCI_TIMER_REG_TIM12; > > + clockevent->prd_off = DAVINCI_TIMER_REG_PRD12; > > + > > + shift = DAVINCI_TIMER_ENAMODE_SHIFT_TIM12; > > + clockevent->enamode_disabled = DAVINCI_TIMER_ENAMODE_DISABLED << shift; > > + clockevent->enamode_oneshot = DAVINCI_TIMER_ENAMODE_ONESHOT << shift; > > + clockevent->enamode_periodic = DAVINCI_TIMER_ENAMODE_PERIODIC << shift; > > + clockevent->enamode_mask = DAVINCI_TIMER_ENAMODE_MASK << shift; > > I don't see where 'shift' can be different from TIM12 here neither in > the second patch for those values. Why create these fields instead of > pre-computed macros? > The variable 'shift' here is only to avoid breaking the lines (just a helper). The shift itself can be different though in the second patch - specifically when calling davinci_clocksource_init(). If I were to use predefined values for clockevent, we'd still need another set of values for clocksource. I think it's clearer the way it is. > > > + if (timer_cfg->cmp_off) { > > + clockevent->cmp_off = timer_cfg->cmp_off; > > + clockevent->dev.set_next_event = > > + davinci_clockevent_set_next_event_cmp; > > + } else { > > + clockevent->dev.set_next_event = > > + davinci_clockevent_set_next_event_std; > > + } > > + > > + rv = request_irq(timer_cfg->irq[DAVINCI_TIMER_CLOCKEVENT_IRQ].start, > > + davinci_timer_irq_timer, IRQF_TIMER, > > + "clockevent", clockevent); > > May be replace "clockevent" by eg. "tim12"? > I don't think this is a good idea. Now if you look at /proc/interrupts you can tell immediately what the interrupt is for ("clockevent"). With "tim12" it's no longer that clear. > > + if (rv) { > > + pr_err("Unable to request the clockevent interrupt"); > > + return rv; > > + } > > + > > + clockevents_config_and_register(&clockevent->dev, tick_rate, > > + DAVINCI_TIMER_MIN_DELTA, > > + DAVINCI_TIMER_MAX_DELTA); > > + > > + return 0; > > +} > > + > > +static int __init of_davinci_timer_register(struct device_node *np) > > +{ > > + struct davinci_timer_cfg timer_cfg = { }; > > + struct clk *clk; > > + int rv; > > + > > + rv = of_address_to_resource(np, 0, &timer_cfg.reg); > > + if (rv) { > > + pr_err("Unable to get the register range for timer"); > > + return rv; > > + } > > + > > + rv = of_irq_to_resource_table(np, timer_cfg.irq, > > + DAVINCI_TIMER_NUM_IRQS); > > + if (rv != DAVINCI_TIMER_NUM_IRQS) { > > + pr_err("Unable to get the interrupts for timer"); > > + return rv; > > + } > > + > > + clk = of_clk_get(np, 0); > > + if (IS_ERR(clk)) { > > + pr_err("Unable to get the timer clock"); > > + return PTR_ERR(clk); > > + } > > + > > + rv = davinci_timer_register(clk, &timer_cfg); > > + if (rv) > > + clk_put(clk); > > + > > + return rv; > > +} > > +TIMER_OF_DECLARE(davinci_timer, "ti,da830-timer", of_davinci_timer_register); > > diff --git a/include/clocksource/timer-davinci.h b/include/clocksource/timer-davinci.h > > new file mode 100644 > > index 000000000000..1dcc1333fbc8 > > --- /dev/null > > +++ b/include/clocksource/timer-davinci.h > > @@ -0,0 +1,44 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > +/* > > + * TI DaVinci clocksource driver > > + * > > + * Copyright (C) 2019 Texas Instruments > > + * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > + */ > > + > > +#ifndef __TIMER_DAVINCI_H__ > > +#define __TIMER_DAVINCI_H__ > > + > > +#include <linux/clk.h> > > +#include <linux/ioport.h> > > + > > +enum { > > + DAVINCI_TIMER_CLOCKEVENT_IRQ, > > + DAVINCI_TIMER_CLOCKSOURCE_IRQ, > > + DAVINCI_TIMER_NUM_IRQS, > > +}; > > + > > +/** > > + * struct davinci_timer_cfg - davinci clocksource driver configuration struct > > + * @reg: register range resource > > + * @irq: clockevent and clocksource interrupt resources > > + * @cmp_off: if set - it specifies the compare register used for clockevent > > + * > > + * Note: if the compare register is specified, the driver will use the bottom > > + * clock half for both clocksource and clockevent and the compare register > > + * to generate event irqs. The user must supply the correct compare register > > + * interrupt number. > > + * > > + * This is only used by da830 the DSP of which uses the top half. The timer > > + * driver still configures the top half to run in free-run mode. > > + */ > > +struct davinci_timer_cfg { > > + struct resource reg; > > + struct resource irq[DAVINCI_TIMER_NUM_IRQS]; > > + unsigned int cmp_off; > > +}; > > + > > +int __init davinci_timer_register(struct clk *clk, > > + const struct davinci_timer_cfg *data); > > + > > +#endif /* __TIMER_DAVINCI_H__ */ > > > > > -- > <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 > For all other comments - they were addressed in v2. Best regards, Bartosz Golaszewski ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC 1/2] clocksource: davinci-timer: add support for clockevents 2019-05-23 12:58 ` Bartosz Golaszewski @ 2019-05-23 13:25 ` Daniel Lezcano 2019-05-23 15:34 ` Bartosz Golaszewski 0 siblings, 1 reply; 9+ messages in thread From: Daniel Lezcano @ 2019-05-23 13:25 UTC (permalink / raw) To: Bartosz Golaszewski Cc: Sekhar Nori, Kevin Hilman, Thomas Gleixner, David Lechner, Linux ARM, Linux Kernel Mailing List, Bartosz Golaszewski Hi Bartosz, On 23/05/2019 14:58, Bartosz Golaszewski wrote: [ ... ] >>> +++ b/drivers/clocksource/timer-davinci.c >>> @@ -0,0 +1,272 @@ >>> +// SPDX-License-Identifier: GPL-2.0-only >>> +// >>> +// TI DaVinci clocksource driver >>> +// >>> +// Copyright (C) 2019 Texas Instruments >>> +// Author: Bartosz Golaszewski <bgolaszewski@baylibre.com> >>> +// (with tiny parts adopted from code by Kevin Hilman <khilman@baylibre.com>) >> >> The header format is wrong, it should be: >> >> // SPDX-License-Identifier: GPL-2.0-only >> /* >> * TI DaVinci clocksource driver >> * >> * ... >> * ... >> * >> */ > > It's not wrong. It looks like it's at the maintainers discretion and > I've been asked to use both forms by different maintainers. Seems you > just can't get it right. :) I've changed it in v2 though. Right, I've been through the documentation but it is still unclear for me. So let's stick to whatever you want for now. [ ... ] >>> +static int >>> +davinci_clockevent_set_next_event_std(unsigned long cycles, >>> + struct clock_event_device *dev) >>> +{ >>> + struct davinci_clockevent *clockevent; >>> + unsigned int enamode; >>> + >>> + clockevent = to_davinci_clockevent(dev); >>> + enamode = clockevent->enamode_disabled; >>> + >>> + davinci_clockevent_update(clockevent, DAVINCI_TIMER_REG_TCR, >>> + clockevent->enamode_mask, >>> + clockevent->enamode_disabled); >> >> What is for this function with the DAVINCI_TIMER_REG_TCR parameter? > > I'm not sure I understand the question. :( I meant davinci_clockevent_update is always called with the DAVINCI_TIMER_REG_TCR parameter. So it can be changed to: static void davinci_clockevent_update(struct davinci_clockevent *clockevent, unsigned int mask, unsigned int val) { davinci_reg_update(clockevent->base, DAVINCI_TIMER_REG_TCR, mask, val); } Alternatively davinci_clockevent_update can be replaced by a direct call to davinci_reg_update. [ ... ] >>> + clockevent->dev.cpumask = cpumask_of(0); >>> + >>> + clockevent->base = base; >>> + clockevent->tim_off = DAVINCI_TIMER_REG_TIM12; >>> + clockevent->prd_off = DAVINCI_TIMER_REG_PRD12; >>> + >>> + shift = DAVINCI_TIMER_ENAMODE_SHIFT_TIM12; >>> + clockevent->enamode_disabled = DAVINCI_TIMER_ENAMODE_DISABLED << shift; >>> + clockevent->enamode_oneshot = DAVINCI_TIMER_ENAMODE_ONESHOT << shift; >>> + clockevent->enamode_periodic = DAVINCI_TIMER_ENAMODE_PERIODIC << shift; >>> + clockevent->enamode_mask = DAVINCI_TIMER_ENAMODE_MASK << shift; >> >> I don't see where 'shift' can be different from TIM12 here neither in >> the second patch for those values. Why create these fields instead of >> pre-computed macros? >> > > The variable 'shift' here is only to avoid breaking the lines (just a helper). > > The shift itself can be different though in the second patch - > specifically when calling davinci_clocksource_init(). > > If I were to use predefined values for clockevent, we'd still need > another set of values for clocksource. I think it's clearer the way it > is. Ah yes, I see, it is passed as parameter. Ok, let's keep it this way if you prefer. >>> + if (timer_cfg->cmp_off) { >>> + clockevent->cmp_off = timer_cfg->cmp_off; >>> + clockevent->dev.set_next_event = >>> + davinci_clockevent_set_next_event_cmp; >>> + } else { >>> + clockevent->dev.set_next_event = >>> + davinci_clockevent_set_next_event_std; >>> + } >>> + >>> + rv = request_irq(timer_cfg->irq[DAVINCI_TIMER_CLOCKEVENT_IRQ].start, >>> + davinci_timer_irq_timer, IRQF_TIMER, >>> + "clockevent", clockevent); >> >> May be replace "clockevent" by eg. "tim12"? >> > > I don't think this is a good idea. Now if you look at /proc/interrupts > you can tell immediately what the interrupt is for ("clockevent"). > With "tim12" it's no longer that clear. Yes, "tim12" can be confusing. However, it is good practice to add a device name aside with its purpose in case there are several timers defined on the system. "clockevent" is a kernel internal representation of a timer, so may be a name like "timer/tim12" or something in the same spirit would be more adequate. -- <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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC 1/2] clocksource: davinci-timer: add support for clockevents 2019-05-23 13:25 ` Daniel Lezcano @ 2019-05-23 15:34 ` Bartosz Golaszewski 0 siblings, 0 replies; 9+ messages in thread From: Bartosz Golaszewski @ 2019-05-23 15:34 UTC (permalink / raw) To: Daniel Lezcano Cc: Sekhar Nori, Kevin Hilman, Thomas Gleixner, David Lechner, Linux ARM, Linux Kernel Mailing List, Bartosz Golaszewski czw., 23 maj 2019 o 15:25 Daniel Lezcano <daniel.lezcano@linaro.org> napisał(a): > > > Hi Bartosz, > > > > On 23/05/2019 14:58, Bartosz Golaszewski wrote: > > [ ... ] > > >>> +++ b/drivers/clocksource/timer-davinci.c > >>> @@ -0,0 +1,272 @@ > >>> +// SPDX-License-Identifier: GPL-2.0-only > >>> +// > >>> +// TI DaVinci clocksource driver > >>> +// > >>> +// Copyright (C) 2019 Texas Instruments > >>> +// Author: Bartosz Golaszewski <bgolaszewski@baylibre.com> > >>> +// (with tiny parts adopted from code by Kevin Hilman <khilman@baylibre.com>) > >> > >> The header format is wrong, it should be: > >> > >> // SPDX-License-Identifier: GPL-2.0-only > >> /* > >> * TI DaVinci clocksource driver > >> * > >> * ... > >> * ... > >> * > >> */ > > > > It's not wrong. It looks like it's at the maintainers discretion and > > I've been asked to use both forms by different maintainers. Seems you > > just can't get it right. :) I've changed it in v2 though. > > Right, I've been through the documentation but it is still unclear for > me. So let's stick to whatever you want for now. > > [ ... ] > > >>> +static int > >>> +davinci_clockevent_set_next_event_std(unsigned long cycles, > >>> + struct clock_event_device *dev) > >>> +{ > >>> + struct davinci_clockevent *clockevent; > >>> + unsigned int enamode; > >>> + > >>> + clockevent = to_davinci_clockevent(dev); > >>> + enamode = clockevent->enamode_disabled; > >>> + > >>> + davinci_clockevent_update(clockevent, DAVINCI_TIMER_REG_TCR, > >>> + clockevent->enamode_mask, > >>> + clockevent->enamode_disabled); > >> > >> What is for this function with the DAVINCI_TIMER_REG_TCR parameter? > > > > I'm not sure I understand the question. :( > > I meant davinci_clockevent_update is always called with the > DAVINCI_TIMER_REG_TCR parameter. > > So it can be changed to: > static void davinci_clockevent_update(struct davinci_clockevent > *clockevent, > unsigned int mask, > unsigned int val) > { > davinci_reg_update(clockevent->base, DAVINCI_TIMER_REG_TCR, > mask, val); > } > Yes, this is pretty much what I did in v2. > > Alternatively davinci_clockevent_update can be replaced by a direct call > to davinci_reg_update. > > [ ... ] > > >>> + clockevent->dev.cpumask = cpumask_of(0); > >>> + > >>> + clockevent->base = base; > >>> + clockevent->tim_off = DAVINCI_TIMER_REG_TIM12; > >>> + clockevent->prd_off = DAVINCI_TIMER_REG_PRD12; > >>> + > >>> + shift = DAVINCI_TIMER_ENAMODE_SHIFT_TIM12; > >>> + clockevent->enamode_disabled = DAVINCI_TIMER_ENAMODE_DISABLED << shift; > >>> + clockevent->enamode_oneshot = DAVINCI_TIMER_ENAMODE_ONESHOT << shift; > >>> + clockevent->enamode_periodic = DAVINCI_TIMER_ENAMODE_PERIODIC << shift; > >>> + clockevent->enamode_mask = DAVINCI_TIMER_ENAMODE_MASK << shift; > >> > >> I don't see where 'shift' can be different from TIM12 here neither in > >> the second patch for those values. Why create these fields instead of > >> pre-computed macros? > >> > > > > The variable 'shift' here is only to avoid breaking the lines (just a helper). > > > > The shift itself can be different though in the second patch - > > specifically when calling davinci_clocksource_init(). > > > > If I were to use predefined values for clockevent, we'd still need > > another set of values for clocksource. I think it's clearer the way it > > is. > > Ah yes, I see, it is passed as parameter. Ok, let's keep it this way if > you prefer. > > >>> + if (timer_cfg->cmp_off) { > >>> + clockevent->cmp_off = timer_cfg->cmp_off; > >>> + clockevent->dev.set_next_event = > >>> + davinci_clockevent_set_next_event_cmp; > >>> + } else { > >>> + clockevent->dev.set_next_event = > >>> + davinci_clockevent_set_next_event_std; > >>> + } > >>> + > >>> + rv = request_irq(timer_cfg->irq[DAVINCI_TIMER_CLOCKEVENT_IRQ].start, > >>> + davinci_timer_irq_timer, IRQF_TIMER, > >>> + "clockevent", clockevent); > >> > >> May be replace "clockevent" by eg. "tim12"? > >> > > > > I don't think this is a good idea. Now if you look at /proc/interrupts > > you can tell immediately what the interrupt is for ("clockevent"). > > With "tim12" it's no longer that clear. > > Yes, "tim12" can be confusing. However, it is good practice to add a > device name aside with its purpose in case there are several timers > defined on the system. "clockevent" is a kernel internal representation > of a timer, so may be a name like "timer/tim12" or something in the same > spirit would be more adequate. > I'll wait for your comments on v2 before changing it in the final submission. Thanks, Bart > > > > -- > <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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [RFC 2/2] clocksource: timer-davinci: add support for clocksource 2019-04-17 14:47 [RFC 0/2] clocksource: davinci-timer: new driver Bartosz Golaszewski 2019-04-17 14:47 ` [RFC 1/2] clocksource: davinci-timer: add support for clockevents Bartosz Golaszewski @ 2019-04-17 14:47 ` Bartosz Golaszewski 2019-05-13 7:54 ` [RFC 0/2] clocksource: davinci-timer: new driver Bartosz Golaszewski 2 siblings, 0 replies; 9+ messages in thread From: Bartosz Golaszewski @ 2019-04-17 14:47 UTC (permalink / raw) To: Sekhar Nori, Kevin Hilman, Daniel Lezcano, Thomas Gleixner, David Lechner Cc: linux-arm-kernel, linux-kernel, Bartosz Golaszewski From: Bartosz Golaszewski <bgolaszewski@baylibre.com> Extend the davinci-timer driver to also register a clock source. Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> --- drivers/clocksource/timer-davinci.c | 70 +++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/drivers/clocksource/timer-davinci.c b/drivers/clocksource/timer-davinci.c index d30f81a4088e..d630fca98123 100644 --- a/drivers/clocksource/timer-davinci.c +++ b/drivers/clocksource/timer-davinci.c @@ -42,6 +42,8 @@ #define DAVINCI_TIMER_MIN_DELTA 0x01 #define DAVINCI_TIMER_MAX_DELTA 0xfffffffe +#define DAVINCI_TIMER_CLKSRC_BITS 32 + #define DAVINCI_TIMER_TGCR_DEFAULT \ (DAVINCI_TIMER_TIMMODE_32BIT_UNCHAINED | DAVINCI_TIMER_UNRESET) @@ -59,6 +61,16 @@ struct davinci_clockevent { unsigned int enamode_mask; }; +/* + * This must be globally accessible by davinci_timer_read_sched_clock(), so + * let's keep it here. + */ +static struct { + struct clocksource dev; + void __iomem *base; + unsigned int tim_off; +} davinci_clocksource; + static struct davinci_clockevent * to_davinci_clockevent(struct clock_event_device *clockevent) { @@ -148,6 +160,32 @@ static irqreturn_t davinci_timer_irq_timer(int irq, void *data) return IRQ_HANDLED; } +static u64 notrace davinci_timer_read_sched_clock(void) +{ + return readl_relaxed(davinci_clocksource.base + + davinci_clocksource.tim_off); +} + +static u64 davinci_clocksource_read(struct clocksource *dev) +{ + return davinci_timer_read_sched_clock(); +} + +static void davinci_clocksource_init(void __iomem *base, unsigned int tim_off, + unsigned int prd_off, unsigned int shift) +{ + davinci_reg_update(base, DAVINCI_TIMER_REG_TCR, + DAVINCI_TIMER_ENAMODE_MASK << shift, + DAVINCI_TIMER_ENAMODE_DISABLED << shift); + + writel_relaxed(0x0, base + tim_off); + writel_relaxed(UINT_MAX, base + prd_off); + + davinci_reg_update(base, DAVINCI_TIMER_REG_TCR, + DAVINCI_TIMER_ENAMODE_MASK << shift, + DAVINCI_TIMER_ENAMODE_PERIODIC << shift); +} + static void davinci_timer_init(void __iomem *base) { /* Set clock to internal mode and disable it. */ @@ -235,6 +273,38 @@ int __init davinci_timer_register(struct clk *clk, DAVINCI_TIMER_MIN_DELTA, DAVINCI_TIMER_MAX_DELTA); + davinci_clocksource.dev.rating = 300; + davinci_clocksource.dev.read = davinci_clocksource_read; + davinci_clocksource.dev.mask = + CLOCKSOURCE_MASK(DAVINCI_TIMER_CLKSRC_BITS); + davinci_clocksource.dev.flags = CLOCK_SOURCE_IS_CONTINUOUS; + davinci_clocksource.base = base; + + if (timer_cfg->cmp_off) { + davinci_clocksource.dev.name = "tim12"; + davinci_clocksource.tim_off = DAVINCI_TIMER_REG_TIM12; + davinci_clocksource_init(base, + DAVINCI_TIMER_REG_TIM12, + DAVINCI_TIMER_REG_PRD12, + DAVINCI_TIMER_ENAMODE_SHIFT_TIM12); + } else { + davinci_clocksource.dev.name = "tim34"; + davinci_clocksource.tim_off = DAVINCI_TIMER_REG_TIM34; + davinci_clocksource_init(base, + DAVINCI_TIMER_REG_TIM34, + DAVINCI_TIMER_REG_PRD34, + DAVINCI_TIMER_ENAMODE_SHIFT_TIM34); + } + + rv = clocksource_register_hz(&davinci_clocksource.dev, tick_rate); + if (rv) { + pr_err("Unable to register clocksource"); + return rv; + } + + sched_clock_register(davinci_timer_read_sched_clock, + DAVINCI_TIMER_CLKSRC_BITS, tick_rate); + return 0; } -- 2.21.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC 0/2] clocksource: davinci-timer: new driver 2019-04-17 14:47 [RFC 0/2] clocksource: davinci-timer: new driver Bartosz Golaszewski 2019-04-17 14:47 ` [RFC 1/2] clocksource: davinci-timer: add support for clockevents Bartosz Golaszewski 2019-04-17 14:47 ` [RFC 2/2] clocksource: timer-davinci: add support for clocksource Bartosz Golaszewski @ 2019-05-13 7:54 ` Bartosz Golaszewski 2019-05-13 8:04 ` Daniel Lezcano 2 siblings, 1 reply; 9+ messages in thread From: Bartosz Golaszewski @ 2019-05-13 7:54 UTC (permalink / raw) To: Daniel Lezcano Cc: Linux ARM, Sekhar Nori, David Lechner, Thomas Gleixner, Kevin Hilman, Linux Kernel Mailing List, Bartosz Golaszewski śr., 17 kwi 2019 o 16:47 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a): > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Hi Daniel, > > as discussed - this is the davinci timer driver split into the clockevent > and clocksource parts. > > Since it won't work without all the other (left out for now) changes, I'm > marking it as RFC. > > The code has been simplified as requested, the duplicated enums and the > davinci_timer structure have been removed. > > Please let me know if that's better. I retested it on da850-lcdk, da830-evm > and dm365-evm. > > Bartosz Golaszewski (2): > clocksource: davinci-timer: add support for clockevents > clocksource: timer-davinci: add support for clocksource > > drivers/clocksource/Kconfig | 5 + > drivers/clocksource/Makefile | 1 + > drivers/clocksource/timer-davinci.c | 342 ++++++++++++++++++++++++++++ > include/clocksource/timer-davinci.h | 44 ++++ > 4 files changed, 392 insertions(+) > create mode 100644 drivers/clocksource/timer-davinci.c > create mode 100644 include/clocksource/timer-davinci.h > > -- > 2.21.0 > Hi Daniel, it's been almost a month so a gentle ping. Any comments on that? Best regards, Bartosz Golaszewski ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC 0/2] clocksource: davinci-timer: new driver 2019-05-13 7:54 ` [RFC 0/2] clocksource: davinci-timer: new driver Bartosz Golaszewski @ 2019-05-13 8:04 ` Daniel Lezcano 0 siblings, 0 replies; 9+ messages in thread From: Daniel Lezcano @ 2019-05-13 8:04 UTC (permalink / raw) To: Bartosz Golaszewski Cc: Linux ARM, Sekhar Nori, David Lechner, Thomas Gleixner, Kevin Hilman, Linux Kernel Mailing List, Bartosz Golaszewski On 13/05/2019 09:54, Bartosz Golaszewski wrote: > śr., 17 kwi 2019 o 16:47 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a): >> >> From: Bartosz Golaszewski <bgolaszewski@baylibre.com> >> >> Hi Daniel, >> >> as discussed - this is the davinci timer driver split into the clockevent >> and clocksource parts. >> >> Since it won't work without all the other (left out for now) changes, I'm >> marking it as RFC. >> >> The code has been simplified as requested, the duplicated enums and the >> davinci_timer structure have been removed. >> >> Please let me know if that's better. I retested it on da850-lcdk, da830-evm >> and dm365-evm. >> >> Bartosz Golaszewski (2): >> clocksource: davinci-timer: add support for clockevents >> clocksource: timer-davinci: add support for clocksource >> >> drivers/clocksource/Kconfig | 5 + >> drivers/clocksource/Makefile | 1 + >> drivers/clocksource/timer-davinci.c | 342 ++++++++++++++++++++++++++++ >> include/clocksource/timer-davinci.h | 44 ++++ >> 4 files changed, 392 insertions(+) >> create mode 100644 drivers/clocksource/timer-davinci.c >> create mode 100644 include/clocksource/timer-davinci.h >> >> -- >> 2.21.0 >> > > Hi Daniel, > > it's been almost a month so a gentle ping. Any comments on that? Oh right, I've been distracted with other things, sorry for that. Let me review it today. -- <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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-05-23 15:34 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-17 14:47 [RFC 0/2] clocksource: davinci-timer: new driver Bartosz Golaszewski 2019-04-17 14:47 ` [RFC 1/2] clocksource: davinci-timer: add support for clockevents Bartosz Golaszewski 2019-05-14 19:55 ` Daniel Lezcano 2019-05-23 12:58 ` Bartosz Golaszewski 2019-05-23 13:25 ` Daniel Lezcano 2019-05-23 15:34 ` Bartosz Golaszewski 2019-04-17 14:47 ` [RFC 2/2] clocksource: timer-davinci: add support for clocksource Bartosz Golaszewski 2019-05-13 7:54 ` [RFC 0/2] clocksource: davinci-timer: new driver Bartosz Golaszewski 2019-05-13 8:04 ` Daniel Lezcano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).