[RFC,1/2] clocksource: davinci-timer: add support for clockevents
diff mbox series

Message ID 20190417144709.19588-2-brgl@bgdev.pl
State RFC
Headers show
Series
  • clocksource: davinci-timer: new driver
Related show

Commit Message

Bartosz Golaszewski April 17, 2019, 2:47 p.m. UTC
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

Comments

Daniel Lezcano May 14, 2019, 7:55 p.m. UTC | #1
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__ */
>
Bartosz Golaszewski May 23, 2019, 12:58 p.m. UTC | #2
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
Daniel Lezcano May 23, 2019, 1:25 p.m. UTC | #3
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.
Bartosz Golaszewski May 23, 2019, 3:34 p.m. UTC | #4
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
>

Patch
diff mbox series

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__ */