[02/14] clocksource/drivers/timer-ti-dm: Add clockevent and clocksource support

Commit Message

Tony Lindgren April 17, 2020, 4:55 p.m. UTC

We can move the TI dmtimer clockevent and clocksource to live under
drivers/clocksource if we rely only on the clock framework, and handle
the module configuration directly in the clocksource driver.

This removes the early dependency with system timers to the interconnect
related code, and we can probe pretty much everything else later on at
the module_init level.

Let's first add a new driver for timer-ti-dm-systimer based on existing
arch/arm/mach-omap2/timer.c. Then let's start moving SoCs to probe with
device tree data while still keeping the old timer.c. And eventually we
can just drop the old timer.c.

Note the boards can optionally configure the timer source clock in using
assigned-clock-parents.

Cc: [e-mail]
Cc: [e-mail]
Cc: Daniel Lezcano <[e-mail]>
Cc: Keerthy <[e-mail]>
Cc: Lokesh Vutla <[e-mail]>
Cc: Tero Kristo <[e-mail]>
Cc: Thomas Gleixner <[e-mail]>
Signed-off-by: Tony Lindgren <[e-mail]>
---
 .../devicetree/bindings/timer/ti,timer.txt    |   2 +
 drivers/clocksource/Makefile                  |   1 +
 drivers/clocksource/timer-ti-dm-systimer.c    | 468 ++++++++++++++++++
 include/clocksource/timer-ti-dm.h             |   1 +
 4 files changed, 472 insertions(+)
 create mode 100644 drivers/clocksource/timer-ti-dm-systimer.c

Comments

Daniel Lezcano April 27, 2020, 9:18 a.m. UTC | #1

Hi Tony,

On 17/04/2020 18:55, Tony Lindgren wrote:
> We can move the TI dmtimer clockevent and clocksource to live under
> drivers/clocksource if we rely only on the clock framework, and handle
> the module configuration directly in the clocksource driver.
> 
> This removes the early dependency with system timers to the interconnect
> related code, and we can probe pretty much everything else later on at
> the module_init level.
> 
> Let's first add a new driver for timer-ti-dm-systimer based on existing
> arch/arm/mach-omap2/timer.c. Then let's start moving SoCs to probe with
> device tree data while still keeping the old timer.c. And eventually we
> can just drop the old timer.c.
> 
> Note the boards can optionally configure the timer source clock in using
> assigned-clock-parents.
> 
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: Daniel Lezcano <[e-mail]>
> Cc: Keerthy <[e-mail]>
> Cc: Lokesh Vutla <[e-mail]>
> Cc: Tero Kristo <[e-mail]>
> Cc: Thomas Gleixner <[e-mail]>
> Signed-off-by: Tony Lindgren <[e-mail]>
> ---
>  .../devicetree/bindings/timer/ti,timer.txt    |   2 +
>  drivers/clocksource/Makefile                  |   1 +
>  drivers/clocksource/timer-ti-dm-systimer.c    | 468 ++++++++++++++++++
>  include/clocksource/timer-ti-dm.h             |   1 +
>  4 files changed, 472 insertions(+)
>  create mode 100644 drivers/clocksource/timer-ti-dm-systimer.c
> 
> diff --git a/Documentation/devicetree/bindings/timer/ti,timer.txt b/Documentation/devicetree/bindings/timer/ti,timer.txt
> --- a/Documentation/devicetree/bindings/timer/ti,timer.txt
> +++ b/Documentation/devicetree/bindings/timer/ti,timer.txt
> @@ -14,6 +14,8 @@ Required properties:
>  			ti,omap5430-timer (applicable to OMAP543x devices)
>  			ti,am335x-timer	(applicable to AM335x devices)
>  			ti,am335x-timer-1ms (applicable to AM335x devices)
> +			ti,dmtimer-clockevent (when used as for clockevent)
> +			ti,dmtimer-clocksource (when used as for clocksource)

Please, submit a separate patch for this.

Before you resend as is, this will be nacked as clocksource / clockevent
is not a hardware description but a Linux thing.

Finding a way to characterize that from the DT is an endless discussion
since years, so I suggest to use a single property for the timer eg
<ti,dmtimer> and initialize the clocksource and the clockevent in the
driver.

>  - reg:			Contains timer register address range (base address and
>  			length).
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -18,6 +18,7 @@ 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_OMAP_DM_TIMER)	+= timer-ti-dm-systimer.o
>  obj-$(CONFIG_DW_APB_TIMER)	+= dw_apb_timer.o
>  obj-$(CONFIG_DW_APB_TIMER_OF)	+= dw_apb_timer_of.o
>  obj-$(CONFIG_FTTMR010_TIMER)	+= timer-fttmr010.o
> diff --git a/drivers/clocksource/timer-ti-dm-systimer.c b/drivers/clocksource/timer-ti-dm-systimer.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/clocksource/timer-ti-dm-systimer.c
> @@ -0,0 +1,468 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +#include <linux/clk.h>
> +#include <linux/clocksource.h>
> +#include <linux/clockchips.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/sched_clock.h>
> +
> +#include <linux/clk/clk-conf.h>
> +
> +#include <clocksource/timer-ti-dm.h>
> +#include <dt-bindings/bus/ti-sysc.h>
> +
> +/* For type1, set SYSC_OMAP2_CLOCKACTIVITY for fck off on idle, l4 clock on */
> +#define DMTIMER_TYPE1_ENABLE	((1 << 9) | (SYSC_IDLE_SMART << 3) | \
> +				 SYSC_OMAP2_ENAWAKEUP | SYSC_OMAP2_AUTOIDLE)
> +
> +#define DMTIMER_TYPE2_ENABLE	(SYSC_IDLE_SMART_WKUP << 2)
> +#define DMTIMER_RESET_WAIT	100000
> +
> +struct dmtimer_clockevent {
> +	struct clock_event_device dev;
> +	struct omap_dm_timer timer;
> +};
> +
> +struct dmtimer_clocksource {
> +	struct clocksource dev;
> +	struct omap_dm_timer timer;

The usage of the timer field is to use the __omap_dm_timer_* helpers
function which does a busy looping on the status before read/write the
register. AFAICS, for the clocksource, the posted argument is zero, thus
it is possible to replace the calls to these helpers to a write / read
and perhaps the struct omap_dm_timer could be removed from the
clocksource structure.

The driver gets obfuscated by calls to helpers which does 'posted' things.

Why not remove those and handle the case in the driver to make it
self-encapsuled and remove the omap_dm_timer structure usage in this driver.

> +	unsigned int loadval;
> +};
> +
> +static int dmtimer_systimer_type1_reset(struct omap_dm_timer *timer)
> +{
> +	void __iomem *syss = timer->io_base + OMAP_TIMER_V1_SYS_STAT_OFFSET;
> +	int ret;
> +	u32 l;
> +
> +	__omap_dm_timer_write(timer, OMAP_TIMER_IF_CTRL_REG, 0x06, 0);

As the last argument is zero, may be take the opportunity to do a direct
call to writel?

> +	ret = readl_poll_timeout_atomic(syss, l, l & BIT(0), 100,
> +					DMTIMER_RESET_WAIT);
> +
> +	return ret;
> +}
> +
> +/* Note we must use io_base instead of func_base for type2 OCP regs */
> +static int dmtimer_systimer_type2_reset(struct omap_dm_timer *timer)
> +{
> +	void __iomem *sysc = timer->io_base + OMAP_TIMER_OCP_CFG_OFFSET;
> +	u32 l;
> +

Isn't missing a write here ?

> +	return readl_poll_timeout_atomic(sysc, l, !(l & BIT(0)), 100,
> +					 DMTIMER_RESET_WAIT);
> +}
> +
> +static int dmtimer_systimer_reset(struct omap_dm_timer *timer)
> +{
> +	int ret;
> +
> +	if (timer->revision == 1)
> +		ret = dmtimer_systimer_type1_reset(timer);
> +	else
> +		ret = dmtimer_systimer_type2_reset(timer);
> +	if (ret < 0) {
> +		pr_err("%s failed with %i\n", __func__, ret);
> +
> +		return ret;
> +	}
> +
> +	timer->posted = 0;
> +
> +	return 0;
> +}
> +
> +/* Interface clocks are only available on some SoCs variants */
> +static int __init dmtimer_systimer_init_clock(struct omap_dm_timer *timer,

The timer argument is not used

> +					      struct device_node *np,
> +					      struct clk **clk,
> +					      const char *name)
> +{
> +	struct clk *clock;
> +	int error;
> +
> +	clock = of_clk_get_by_name(np, name);
> +	if ((PTR_ERR(clock) == -EINVAL) && !strncmp(name, "ick", 3))
> +		return 0;
> +	else if (IS_ERR(clock))
> +		return PTR_ERR(clock);
> +
> +	error = clk_prepare_enable(clock);
> +	if (error)
> +		return error;
> +
> +	*clk = clock;
> +
> +	return 0;
> +}
> +
> +static void dmtimer_systimer_enable(struct omap_dm_timer *timer)
> +{
> +	void __iomem *sysc = timer->io_base + OMAP_TIMER_OCP_CFG_OFFSET;
> +	u32 val;
> +
> +	if (timer->revision == 1)
> +		val = DMTIMER_TYPE1_ENABLE;
> +	else
> +		val = DMTIMER_TYPE2_ENABLE;
> +
> +	writel_relaxed(val, sysc);
> +}
> +
> +static void dmtimer_systimer_disable(struct omap_dm_timer *timer)
> +{
> +	void __iomem *sysc = timer->io_base + OMAP_TIMER_OCP_CFG_OFFSET;
> +
> +	writel_relaxed(0, sysc);
> +}
> +
> +static int __init dmtimer_systimer_tag_disabled(struct device_node *np)
> +{
> +	struct property *prop;
> +
> +	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	prop->name = "status";
> +	prop->value = "disabled";
> +	prop->length = strlen(prop->value);
> +
> +	return of_add_property(np, prop);

Why not add this property in the DT instead ? That sounds hackish

> +}
> +
> +static int __init dmtimer_systimer_init(struct omap_dm_timer *timer,
> +					struct device_node *np)
> +{
> +	int error;
> +
> +	if (!of_device_is_compatible(np->parent, "ti,sysc"))
> +		return -EINVAL;
> +
> +	/*
> +	 * Enable optional assigned-clock-parents configured at the timer
> +	 * node level. For regular device drivers, this is done automatically
> +	 * by bus related code such as platform_drv_probe().
> +	 */
> +	error = of_clk_set_defaults(np, false);
> +	if (error < 0)
> +		pr_err("%s: clock source init failed: %i\n", __func__, error);
> +
> +	/* For ti-sysc, we have timer clocks at the parent module level */
> +	error = dmtimer_systimer_init_clock(timer, np->parent,
> +					    &timer->fclk, "fck");
> +	if (error)
> +		return error;
> +
> +	timer->rate = clk_get_rate(timer->fclk);
check rate == 0 for error ?

> +
> +	error = dmtimer_systimer_init_clock(timer, np->parent,
> +					    &timer->iclk, "ick");
> +	if (error)
> +		return error;
> +
> +	__omap_dm_timer_init_regs(timer);

readl_relaxed(timer->io_base) to get the revision ? And then get rid of
all the helpers thing ?


> +	dmtimer_systimer_enable(timer);
> +	dmtimer_systimer_reset(timer);
> +	pr_debug("dmtimer rev %08x sysc %08x\n", readl_relaxed(timer->io_base),
> +		 readl_relaxed(timer->io_base + OMAP_TIMER_OCP_CFG_OFFSET));
> +
> +	if (of_find_property(np, "ti,timer-alwon", NULL))
> +		timer->capability |= OMAP_TIMER_ALWON;

Where is used this capability in this driver ?

> +	/* Tag parent interconnect target module disabled for ti-sysc */
> +	error = dmtimer_systimer_tag_disabled(np->parent);
> +	if (error)
> +		pr_err("%s: failed to set %pOF disabled: %i\n",
> +		       __func__, np, error);
> +
> +	return 0;
> +}
> +
> +/* Clockevent */
> +static struct dmtimer_clockevent *
> +to_dmtimer_clockevent(struct clock_event_device *clockevent)
> +{
> +	return container_of(clockevent, struct dmtimer_clockevent, dev);
> +}
> +
> +static struct omap_dm_timer *
> +clockevent_to_dm_timer(struct clock_event_device *evt)
> +{
> +	struct dmtimer_clockevent *dmtce = to_dmtimer_clockevent(evt);
> +
> +	return &dmtce->timer;
> +}
> +
> +static irqreturn_t dmtimer_clockevent_interrupt(int irq, void *data)
> +{
> +	struct dmtimer_clockevent *dmtce = data;
> +
> +	__omap_dm_timer_write_status(&dmtce->timer, OMAP_TIMER_INT_OVERFLOW);

Please replace with a writel()

> +	dmtce->dev.event_handler(&dmtce->dev);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static struct irqaction dmtimer_clockevent_irq = {
> +	.name		= "gp_timer",
> +	.flags		= IRQF_TIMER | IRQF_IRQPOLL,

Why do you need IRQF_IRQPOLL ?

> +	.handler	= dmtimer_clockevent_interrupt,
> +};
> +
> +static int dmtimer_set_next_event(unsigned long cycles,
> +				  struct clock_event_device *evt)
> +{
> +	struct omap_dm_timer *timer = clockevent_to_dm_timer(evt);
> +
> +	__omap_dm_timer_load_start(timer, OMAP_TIMER_CTRL_ST,
> +				   0xffffffff - cycles, OMAP_TIMER_POSTED);
> +
> +	return 0;
> +}
> +
> +static int dmtimer_clockevent_shutdown(struct clock_event_device *evt)
> +{
> +	struct omap_dm_timer *timer = clockevent_to_dm_timer(evt);
> +
> +	__omap_dm_timer_stop(timer, OMAP_TIMER_POSTED, timer->rate);
> +
> +	return 0;
> +}
> +
> +static int dmtimer_set_periodic(struct clock_event_device *evt)
> +{
> +	struct omap_dm_timer *timer = clockevent_to_dm_timer(evt);
> +	u32 period;
> +
> +	__omap_dm_timer_stop(timer, OMAP_TIMER_POSTED, timer->rate);
> +
> +	period = timer->rate / HZ;
> +	period -= 1;

DIV_ROUND_CLOSEST(rate, HZ); ?

> +	/* Looks like we need to first set the load value separately */
> +	__omap_dm_timer_write(timer, OMAP_TIMER_LOAD_REG, 0xffffffff - period,
> +			      OMAP_TIMER_POSTED);
> +	__omap_dm_timer_load_start(timer,
> +				   OMAP_TIMER_CTRL_AR | OMAP_TIMER_CTRL_ST,
> +				   0xffffffff - period, OMAP_TIMER_POSTED);
> +	return 0;
> +}
> +
> +static void omap_clockevent_idle(struct clock_event_device *evt)
> +{
> +	struct omap_dm_timer *timer = clockevent_to_dm_timer(evt);
> +
> +	dmtimer_systimer_disable(timer);
> +}
> +
> +static void omap_clockevent_unidle(struct clock_event_device *evt)
> +{
> +	struct omap_dm_timer *timer = clockevent_to_dm_timer(evt);
> +
> +	dmtimer_systimer_enable(timer);
> +	__omap_dm_timer_int_enable(timer, OMAP_TIMER_INT_OVERFLOW);
> +}
> +
> +static int __init dmtimer_clockevent_init(struct device_node *np)
> +{
> +	struct dmtimer_clockevent *clkevt;
> +	struct clock_event_device *evt;
> +	struct omap_dm_timer *timer;
> +	int error;
> +	u32 pa;
> +
> +	clkevt = kzalloc(sizeof(*clkevt), GFP_KERNEL | __GFP_NOFAIL);

Remove __GFP_NOFAIL

> +	if (!clkevt)
> +		return -ENOMEM;
> +
> +	timer = &clkevt->timer;
> +	evt = &clkevt->dev;
> +
> +	evt->features = CLOCK_EVT_FEAT_PERIODIC |
> +			CLOCK_EVT_FEAT_ONESHOT;
> +	evt->rating = 300;
> +	evt->set_next_event = dmtimer_set_next_event;
> +	evt->set_state_shutdown = dmtimer_clockevent_shutdown;
> +	evt->set_state_periodic = dmtimer_set_periodic;
> +	evt->set_state_oneshot = dmtimer_clockevent_shutdown;
> +	evt->tick_resume = dmtimer_clockevent_shutdown;
> +	evt->cpumask = cpu_possible_mask;

You may consider the CLOCK_EVT_FEAT_DYNIRQ flag

> +	evt->irq = timer->irq;
> +
> +	timer->irq = irq_of_parse_and_map(np, 0);
> +	if (!timer->irq) {
> +		error = -ENXIO;
> +		goto err_out_free;
> +	}
> +
> +	timer->io_base = of_iomap(np, 0);
> +	if (!timer->io_base) {
> +		error = -ENXIO;
> +		goto err_out_free;
> +	}
> +
> +	error = dmtimer_systimer_init(timer, np);
> +	if (error)
> +		goto err_out_unmap;
> +
> +	/*
> +	 * For clock-event timers we never read the timer counter and
> +	 * so we are not impacted by errata i103 and i767. Therefore,
> +	 * we can safely ignore this errata for clock-event timers.
> +	 */
> +	__omap_dm_timer_override_errata(timer, OMAP_TIMER_ERRATA_I103_I767);

Without all these omap_... helpers you would not have to set this

> +	dmtimer_clockevent_irq.dev_id = clkevt;
> +	setup_irq(timer->irq, &dmtimer_clockevent_irq);

request_irq

> +	__omap_dm_timer_int_enable(timer, OMAP_TIMER_INT_OVERFLOW);
> +
> +	pa = of_translate_address(np, of_get_address(np, 0, NULL, NULL));
> +	pr_info("TI gptimer clockevent: %stimer@%08x at %lu Hz\n",
> +		timer->capability & OMAP_TIMER_ALWON ? "always-on " : "",
> +		pa, timer->rate);

%pOF instead of of_translate_address ?

> +	clockevents_config_and_register(evt, timer->rate,
> +					3, /* Timer internal resynch latency */
> +					0xffffffff);
> +
> +	if (of_device_is_compatible(np, "ti,am33xx") ||
> +	    of_device_is_compatible(np, "ti,am43")) {
> +		evt->suspend = omap_clockevent_idle;
> +		evt->resume = omap_clockevent_unidle;
> +	}
> +
> +	return 0;
> +
> +err_out_unmap:
> +	iounmap(timer->io_base);
> +err_out_free:
> +	kfree(clkevt);
> +
> +	return error;
> +}
> +
> +/* Clocksource */
> +static struct dmtimer_clocksource *
> +to_dmtimer_clocksource(struct clocksource *cs)
> +{
> +	return container_of(cs, struct dmtimer_clocksource, dev);
> +}
> +
> +static struct omap_dm_timer *
> +clocksource_to_dmtimer(struct clocksource *cs)
> +{
> +	struct dmtimer_clocksource *clksrc = to_dmtimer_clocksource(cs);
> +
> +	return &clksrc->timer;
> +}
> +
> +static u64 dmtimer_clocksource_read_cycles(struct clocksource *cs)
> +{
> +	struct omap_dm_timer *timer = clocksource_to_dmtimer(cs);
> +
> +	return (u64)__omap_dm_timer_read_counter(timer,
> +						 OMAP_TIMER_NONPOSTED);
> +}
> +
> +static void __iomem *dmtimer_sched_clock_counter;
> +
> +static u64 notrace dmtimer_read_sched_clock(void)
> +{
> +	return readl_relaxed(dmtimer_sched_clock_counter);
> +}
> +
> +static void dmtimer_clocksource_suspend(struct clocksource *cs)
> +{
> +	struct dmtimer_clocksource *clksrc = to_dmtimer_clocksource(cs);
> +	struct omap_dm_timer *timer = &clksrc->timer;
> +
> +	clksrc->loadval =
> +		__omap_dm_timer_read_counter(timer, OMAP_TIMER_NONPOSTED);
> +	dmtimer_systimer_disable(timer);
> +}
> +
> +static void dmtimer_clocksource_resume(struct clocksource *cs)
> +{
> +	struct dmtimer_clocksource *clksrc = to_dmtimer_clocksource(cs);
> +	struct omap_dm_timer *timer = &clksrc->timer;
> +
> +	dmtimer_systimer_enable(timer);
> +	__omap_dm_timer_load_start(timer,
> +				   OMAP_TIMER_CTRL_ST | OMAP_TIMER_CTRL_AR,
> +				   clksrc->loadval,
> +				   OMAP_TIMER_NONPOSTED);
> +}
> +
> +static int __init dmtimer_clocksource_init(struct device_node *np)
> +{
> +	struct dmtimer_clocksource *clksrc;
> +	struct omap_dm_timer *timer;
> +	struct clocksource *dev;
> +	int error;
> +	u32 pa;
> +
> +	clksrc = kzalloc(sizeof(*clksrc), GFP_KERNEL | __GFP_NOFAIL);
> +	if (!clksrc)
> +		return -ENOMEM;
> +
> +	timer = &clksrc->timer;
> +	dev = &clksrc->dev;
> +
> +	timer->io_base = of_iomap(np, 0);
> +	if (!timer->io_base) {
> +		pr_err("%s: could not iomap\n", __func__);
> +		return -ENODEV;
> +	}
> +
> +	error = dmtimer_systimer_init(timer, np);
> +	if (error)
> +		goto err_out_unmap;
> +
> +	dev->name = "dmtimer";
> +	dev->rating = 300;
> +	dev->read = dmtimer_clocksource_read_cycles;
> +	dev->mask = CLOCKSOURCE_MASK(32);
> +	dev->flags = CLOCK_SOURCE_IS_CONTINUOUS;
> +
> +	if (of_device_is_compatible(np, "ti,am33xx") ||
> +	    of_device_is_compatible(np, "ti,am43")) {
> +		dev->suspend = dmtimer_clocksource_suspend;
> +		dev->resume = dmtimer_clocksource_resume;
> +	}
> +	__omap_dm_timer_load_start(timer,
> +				   OMAP_TIMER_CTRL_ST | OMAP_TIMER_CTRL_AR, 0,
> +				   OMAP_TIMER_NONPOSTED);
> +
> +	pa = of_translate_address(np, of_get_address(np, 0, NULL, NULL));
> +	pr_info("TI gptimer clocksource: %stimer@%08x %lu at Hz\n",

%pOF instead of of_translate_address ?

> +		timer->capability & OMAP_TIMER_ALWON ?
> +		"always-on " : "", pa, timer->rate);
> +
> +	if (!dmtimer_sched_clock_counter) {
> +		/* Mask out write pending bits for raw OMAP_TIMER_COUNTER_REG */
> +		dmtimer_sched_clock_counter =
> +			timer->func_base + (OMAP_TIMER_COUNTER_REG & 0xff);
> +		sched_clock_register(dmtimer_read_sched_clock, 32, timer->rate);
> +	}
> +
> +	if (clocksource_register_hz(dev, timer->rate))
> +		pr_err("Could not register clocksource %pOF\n", np);
> +
> +	return 0;
> +
> +err_out_unmap:
> +	iounmap(timer->io_base);
> +
> +	return -ENODEV;
> +}
> +
> +TIMER_OF_DECLARE(dmtimer_clockevent, "ti,dmtimer-clockevent",
> +		 dmtimer_clockevent_init);
> +TIMER_OF_DECLARE(dmtimer_clocksource, "ti,dmtimer-clocksource",
> +		 dmtimer_clocksource_init);
> diff --git a/include/clocksource/timer-ti-dm.h b/include/clocksource/timer-ti-dm.h
> --- a/include/clocksource/timer-ti-dm.h
> +++ b/include/clocksource/timer-ti-dm.h
> @@ -97,6 +97,7 @@ struct omap_dm_timer {
>  	int id;
>  	int irq;
>  	struct clk *fclk;
> +	struct clk *iclk;

No need of these clocks in the structure as they are used only to
initialize.

>  	void __iomem	*io_base;
>  	void __iomem	*irq_stat;	/* TISR/IRQSTATUS interrupt status */
>

Tony Lindgren April 27, 2020, 2:31 p.m. UTC | #2

Hi,

* Daniel Lezcano <[e-mail]> [200427 09:19]:
> On 17/04/2020 18:55, Tony Lindgren wrote:
> > --- a/Documentation/devicetree/bindings/timer/ti,timer.txt
> > +++ b/Documentation/devicetree/bindings/timer/ti,timer.txt
> > @@ -14,6 +14,8 @@ Required properties:
> >  			ti,omap5430-timer (applicable to OMAP543x devices)
> >  			ti,am335x-timer	(applicable to AM335x devices)
> >  			ti,am335x-timer-1ms (applicable to AM335x devices)
> > +			ti,dmtimer-clockevent (when used as for clockevent)
> > +			ti,dmtimer-clocksource (when used as for clocksource)
> 
> Please, submit a separate patch for this.
> 
> Before you resend as is, this will be nacked as clocksource / clockevent
> is not a hardware description but a Linux thing.
> 
> Finding a way to characterize that from the DT is an endless discussion
> since years, so I suggest to use a single property for the timer eg
> <ti,dmtimer> and initialize the clocksource and the clockevent in the
> driver.

Hmm good point. We still need to specify which timer is a clocksource
and which one a clockevent somehow.

Maybe we could have a generic properties like the clock framework such as:

assigned-system-clocksource
assigned-system-clockevent

Or do we already have something similar that can be used?

> > diff --git a/drivers/clocksource/timer-ti-dm-systimer.c b/drivers/clocksource/timer-ti-dm-systimer.c
> > new file mode 100644
> > --- /dev/null
> > +++ b/drivers/clocksource/timer-ti-dm-systimer.c
> > +struct dmtimer_clocksource {
> > +	struct clocksource dev;
> > +	struct omap_dm_timer timer;
> 
> The usage of the timer field is to use the __omap_dm_timer_* helpers
> function which does a busy looping on the status before read/write the
> register. AFAICS, for the clocksource, the posted argument is zero, thus
> it is possible to replace the calls to these helpers to a write / read
> and perhaps the struct omap_dm_timer could be removed from the
> clocksource structure.
> 
> The driver gets obfuscated by calls to helpers which does 'posted' things.
> 
> Why not remove those and handle the case in the driver to make it
> self-encapsuled and remove the omap_dm_timer structure usage in this driver.

Hmm that's a good comment indeed. If we can just use readl/writel for
clockevent and clocksource driver without worrying about the posted mode
flags, we can make all the old helpers static for the generic dmtimer
driver. I'll take a look.

> > +static int dmtimer_systimer_type2_reset(struct omap_dm_timer *timer)
> > +{
> > +	void __iomem *sysc = timer->io_base + OMAP_TIMER_OCP_CFG_OFFSET;
> > +	u32 l;
> > +
> 
> Isn't missing a write here ?

Oops thanks for spotting it. Without that the mode can be whatever left
over from the bootloader.

> > +static int __init dmtimer_systimer_tag_disabled(struct device_node *np)
> > +{
> > +	struct property *prop;
> > +
> > +	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> > +	if (!prop)
> > +		return -ENOMEM;
> > +
> > +	prop->name = "status";
> > +	prop->value = "disabled";
> > +	prop->length = strlen(prop->value);
> > +
> > +	return of_add_property(np, prop);
> 
> Why not add this property in the DT instead ? That sounds hackish

Yes that's a good point too. We need to configure the source clock anyways
for the clocksource and clockevent in devicetree anyways, so setting it
disabled there totally makes sense.

> > +	dmtimer_systimer_enable(timer);
> > +	dmtimer_systimer_reset(timer);
> > +	pr_debug("dmtimer rev %08x sysc %08x\n", readl_relaxed(timer->io_base),
> > +		 readl_relaxed(timer->io_base + OMAP_TIMER_OCP_CFG_OFFSET));
> > +
> > +	if (of_find_property(np, "ti,timer-alwon", NULL))
> > +		timer->capability |= OMAP_TIMER_ALWON;
> 
> Where is used this capability in this driver ?

That is just something we should show in dmesg for info as that helps to
understand the board specific system timer configuration for PM related
issues folks end up reporting.

> > +static struct irqaction dmtimer_clockevent_irq = {
> > +	.name		= "gp_timer",
> > +	.flags		= IRQF_TIMER | IRQF_IRQPOLL,
> 
> Why do you need IRQF_IRQPOLL ?

I'll check if that's needed. Probably just something that has mutated into
the old timer code somehow.

> > +	pa = of_translate_address(np, of_get_address(np, 0, NULL, NULL));
> > +	pr_info("TI gptimer clockevent: %stimer@%08x at %lu Hz\n",
> > +		timer->capability & OMAP_TIMER_ALWON ? "always-on " : "",
> > +		pa, timer->rate);
> 
> %pOF instead of of_translate_address ?

That just 0 from the interconnect target module here. But doing %pOF
on the np->parent should work here and for the clocksource.

> > diff --git a/include/clocksource/timer-ti-dm.h b/include/clocksource/timer-ti-dm.h
> > --- a/include/clocksource/timer-ti-dm.h
> > +++ b/include/clocksource/timer-ti-dm.h
> > @@ -97,6 +97,7 @@ struct omap_dm_timer {
> >  	int id;
> >  	int irq;
> >  	struct clk *fclk;
> > +	struct clk *iclk;
> 
> No need of these clocks in the structure as they are used only to
> initialize.

OK I'll make them local.

Thanks for the review! I'll fix up the other issues you spotted too,
they all seem good comments.

Regards,

Tony

Daniel Lezcano April 27, 2020, 3:02 p.m. UTC | #3

On 27/04/2020 16:31, Tony Lindgren wrote:
> Hi,
> 
> * Daniel Lezcano <[e-mail]> [200427 09:19]:
>> On 17/04/2020 18:55, Tony Lindgren wrote:
>>> --- a/Documentation/devicetree/bindings/timer/ti,timer.txt
>>> +++ b/Documentation/devicetree/bindings/timer/ti,timer.txt
>>> @@ -14,6 +14,8 @@ Required properties:
>>>  			ti,omap5430-timer (applicable to OMAP543x devices)
>>>  			ti,am335x-timer	(applicable to AM335x devices)
>>>  			ti,am335x-timer-1ms (applicable to AM335x devices)
>>> +			ti,dmtimer-clockevent (when used as for clockevent)
>>> +			ti,dmtimer-clocksource (when used as for clocksource)
>>
>> Please, submit a separate patch for this.
>>
>> Before you resend as is, this will be nacked as clocksource / clockevent
>> is not a hardware description but a Linux thing.
>>
>> Finding a way to characterize that from the DT is an endless discussion
>> since years, so I suggest to use a single property for the timer eg
>> <ti,dmtimer> and initialize the clocksource and the clockevent in the
>> driver.
> 
> Hmm good point. We still need to specify which timer is a clocksource
> and which one a clockevent somehow.
> 
> Maybe we could have a generic properties like the clock framework such as:
> 
> assigned-system-clocksource
> assigned-system-clockevent

I think that will be the same problem :/

Is it possible to check the interrupt for the clockevent ? A timer node
with the interrrupt is the clockevent, without it is a clocksource.

Tony Lindgren April 27, 2020, 3:23 p.m. UTC | #4

* Daniel Lezcano <[e-mail]> [200427 15:03]:
> On 27/04/2020 16:31, Tony Lindgren wrote:
> > Hi,
> > 
> > * Daniel Lezcano <[e-mail]> [200427 09:19]:
> >> On 17/04/2020 18:55, Tony Lindgren wrote:
> >>> --- a/Documentation/devicetree/bindings/timer/ti,timer.txt
> >>> +++ b/Documentation/devicetree/bindings/timer/ti,timer.txt
> >>> @@ -14,6 +14,8 @@ Required properties:
> >>>  			ti,omap5430-timer (applicable to OMAP543x devices)
> >>>  			ti,am335x-timer	(applicable to AM335x devices)
> >>>  			ti,am335x-timer-1ms (applicable to AM335x devices)
> >>> +			ti,dmtimer-clockevent (when used as for clockevent)
> >>> +			ti,dmtimer-clocksource (when used as for clocksource)
> >>
> >> Please, submit a separate patch for this.
> >>
> >> Before you resend as is, this will be nacked as clocksource / clockevent
> >> is not a hardware description but a Linux thing.
> >>
> >> Finding a way to characterize that from the DT is an endless discussion
> >> since years, so I suggest to use a single property for the timer eg
> >> <ti,dmtimer> and initialize the clocksource and the clockevent in the
> >> driver.
> > 
> > Hmm good point. We still need to specify which timer is a clocksource
> > and which one a clockevent somehow.
> > 
> > Maybe we could have a generic properties like the clock framework such as:
> > 
> > assigned-system-clocksource
> > assigned-system-clockevent
> 
> I think that will be the same problem :/

Seems like other SoCs have the same issue too with multiple timers
to configure.

> Is it possible to check the interrupt for the clockevent ? A timer node
> with the interrrupt is the clockevent, without it is a clocksource.

OK let's try that. So the configuration would become then:

compatible = "ti,dmtimer;	/* reserved for system timers */
/delete-property/interrupts;	/* ok so it's a clocksource */
/delete-property/interrupts-extended;

Regards,

Tony

Rob Herring April 30, 2020, 2 p.m. UTC | #5

On Mon, Apr 27, 2020 at 08:23:29AM -0700, Tony Lindgren wrote:
> * Daniel Lezcano <[e-mail]> [200427 15:03]:
> > On 27/04/2020 16:31, Tony Lindgren wrote:
> > > Hi,
> > > 
> > > * Daniel Lezcano <[e-mail]> [200427 09:19]:
> > >> On 17/04/2020 18:55, Tony Lindgren wrote:
> > >>> --- a/Documentation/devicetree/bindings/timer/ti,timer.txt
> > >>> +++ b/Documentation/devicetree/bindings/timer/ti,timer.txt
> > >>> @@ -14,6 +14,8 @@ Required properties:
> > >>>  			ti,omap5430-timer (applicable to OMAP543x devices)
> > >>>  			ti,am335x-timer	(applicable to AM335x devices)
> > >>>  			ti,am335x-timer-1ms (applicable to AM335x devices)
> > >>> +			ti,dmtimer-clockevent (when used as for clockevent)
> > >>> +			ti,dmtimer-clocksource (when used as for clocksource)
> > >>
> > >> Please, submit a separate patch for this.
> > >>
> > >> Before you resend as is, this will be nacked as clocksource / clockevent
> > >> is not a hardware description but a Linux thing.
> > >>
> > >> Finding a way to characterize that from the DT is an endless discussion
> > >> since years, so I suggest to use a single property for the timer eg
> > >> <ti,dmtimer> and initialize the clocksource and the clockevent in the
> > >> driver.
> > > 
> > > Hmm good point. We still need to specify which timer is a clocksource
> > > and which one a clockevent somehow.
> > > 
> > > Maybe we could have a generic properties like the clock framework such as:
> > > 
> > > assigned-system-clocksource
> > > assigned-system-clockevent
> > 
> > I think that will be the same problem :/
> 
> Seems like other SoCs have the same issue too with multiple timers
> to configure.
> 
> > Is it possible to check the interrupt for the clockevent ? A timer node
> > with the interrrupt is the clockevent, without it is a clocksource.
> 
> OK let's try that. So the configuration would become then:
> 
> compatible = "ti,dmtimer;	/* reserved for system timers */
> /delete-property/interrupts;	/* ok so it's a clocksource */
> /delete-property/interrupts-extended;

That's not really what was meant.

Let's say you have N timers. Either every timer is exactly the same and 
the OS can just assign them however it wants or there is some difference 
in the h/w making certain timer better for certain functions. Describe 
that difference. It could be clock rate, number of counter bits, always 
on, secure mode access only, has or doesn't have output compare or input 
capture, etc.

Rob

Tony Lindgren April 30, 2020, 3:31 p.m. UTC | #6

* Rob Herring <[e-mail]> [200430 14:01]:
> On Mon, Apr 27, 2020 at 08:23:29AM -0700, Tony Lindgren wrote:
> > * Daniel Lezcano <[e-mail]> [200427 15:03]:
> > > On 27/04/2020 16:31, Tony Lindgren wrote:
> > > > Hi,
> > > > 
> > > > * Daniel Lezcano <[e-mail]> [200427 09:19]:
> > > >> On 17/04/2020 18:55, Tony Lindgren wrote:
> > > >>> --- a/Documentation/devicetree/bindings/timer/ti,timer.txt
> > > >>> +++ b/Documentation/devicetree/bindings/timer/ti,timer.txt
> > > >>> @@ -14,6 +14,8 @@ Required properties:
> > > >>>  			ti,omap5430-timer (applicable to OMAP543x devices)
> > > >>>  			ti,am335x-timer	(applicable to AM335x devices)
> > > >>>  			ti,am335x-timer-1ms (applicable to AM335x devices)
> > > >>> +			ti,dmtimer-clockevent (when used as for clockevent)
> > > >>> +			ti,dmtimer-clocksource (when used as for clocksource)
> > > >>
> > > >> Please, submit a separate patch for this.
> > > >>
> > > >> Before you resend as is, this will be nacked as clocksource / clockevent
> > > >> is not a hardware description but a Linux thing.
> > > >>
> > > >> Finding a way to characterize that from the DT is an endless discussion
> > > >> since years, so I suggest to use a single property for the timer eg
> > > >> <ti,dmtimer> and initialize the clocksource and the clockevent in the
> > > >> driver.
> > > > 
> > > > Hmm good point. We still need to specify which timer is a clocksource
> > > > and which one a clockevent somehow.
> > > > 
> > > > Maybe we could have a generic properties like the clock framework such as:
> > > > 
> > > > assigned-system-clocksource
> > > > assigned-system-clockevent
> > > 
> > > I think that will be the same problem :/
> > 
> > Seems like other SoCs have the same issue too with multiple timers
> > to configure.
> > 
> > > Is it possible to check the interrupt for the clockevent ? A timer node
> > > with the interrrupt is the clockevent, without it is a clocksource.
> > 
> > OK let's try that. So the configuration would become then:
> > 
> > compatible = "ti,dmtimer;	/* reserved for system timers */
> > /delete-property/interrupts;	/* ok so it's a clocksource */
> > /delete-property/interrupts-extended;
> 
> That's not really what was meant.

OK, so let's figure out something better then.

> Let's say you have N timers. Either every timer is exactly the same and 
> the OS can just assign them however it wants or there is some difference 
> in the h/w making certain timer better for certain functions. Describe 
> that difference. It could be clock rate, number of counter bits, always 
> on, secure mode access only, has or doesn't have output compare or input 
> capture, etc.

Hmm. Trying to detect this automatically will get messy. For example,
we have few omap3 boards with the following options that also need to
consider if the separate 32KiHz counter is available:

1. The best case scenario

ti,omap-counter32k clocksource
ti,sysc-omap2-timer ti,timer-alwon clockevent (timer1)

2. Boards relying on internal clock with unusable 32k counter

ti,sysc-omap2-timer ti,timer-alwon clocksource (timer12)
ti,sysc-omap2-timer clockevent (typically gpt2)

In the second case, the 32k counter is unusable, and timer1
is unusable with the external 32k always on clock. But timer1
can be used with the system clock just fine for other purposes.
So ideally we would not tag timer1 as disabled :)

For the second case, we could remove ti,timer-alwon property
for timer1, and tag the 32k counter as disabled as the source
clock is unreliable. Then somewhere in the code we would need
to check if ti,omap-counter32k is availabe, then check if
timer1 is always-on, then use timer12 if not a secure device
like n900.

If the board wants to use the system clock as the source for
a higher resolution with assigned-clock-parents, then we'd need
to ignore the always-on property and not use the 32k counter as
the clocksource. Basically to somehow figure out that a higher
resolution configuration is preferred over a
low-power configuration.

So what's your take on just adding the generic properties for
assigned-system-clocksource and clockevent?

Regards,

Tony

Rob Herring May 1, 2020, 1:18 p.m. UTC | #7

On Thu, Apr 30, 2020 at 10:31 AM Tony Lindgren <[e-mail]> wrote:
>
> * Rob Herring <[e-mail]> [200430 14:01]:
> > On Mon, Apr 27, 2020 at 08:23:29AM -0700, Tony Lindgren wrote:
> > > * Daniel Lezcano <[e-mail]> [200427 15:03]:
> > > > On 27/04/2020 16:31, Tony Lindgren wrote:
> > > > > Hi,
> > > > >
> > > > > * Daniel Lezcano <[e-mail]> [200427 09:19]:
> > > > >> On 17/04/2020 18:55, Tony Lindgren wrote:
> > > > >>> --- a/Documentation/devicetree/bindings/timer/ti,timer.txt
> > > > >>> +++ b/Documentation/devicetree/bindings/timer/ti,timer.txt
> > > > >>> @@ -14,6 +14,8 @@ Required properties:
> > > > >>>                       ti,omap5430-timer (applicable to OMAP543x devices)
> > > > >>>                       ti,am335x-timer (applicable to AM335x devices)
> > > > >>>                       ti,am335x-timer-1ms (applicable to AM335x devices)
> > > > >>> +                     ti,dmtimer-clockevent (when used as for clockevent)
> > > > >>> +                     ti,dmtimer-clocksource (when used as for clocksource)
> > > > >>
> > > > >> Please, submit a separate patch for this.
> > > > >>
> > > > >> Before you resend as is, this will be nacked as clocksource / clockevent
> > > > >> is not a hardware description but a Linux thing.
> > > > >>
> > > > >> Finding a way to characterize that from the DT is an endless discussion
> > > > >> since years, so I suggest to use a single property for the timer eg
> > > > >> <ti,dmtimer> and initialize the clocksource and the clockevent in the
> > > > >> driver.
> > > > >
> > > > > Hmm good point. We still need to specify which timer is a clocksource
> > > > > and which one a clockevent somehow.
> > > > >
> > > > > Maybe we could have a generic properties like the clock framework such as:
> > > > >
> > > > > assigned-system-clocksource
> > > > > assigned-system-clockevent
> > > >
> > > > I think that will be the same problem :/
> > >
> > > Seems like other SoCs have the same issue too with multiple timers
> > > to configure.
> > >
> > > > Is it possible to check the interrupt for the clockevent ? A timer node
> > > > with the interrrupt is the clockevent, without it is a clocksource.
> > >
> > > OK let's try that. So the configuration would become then:
> > >
> > > compatible = "ti,dmtimer;   /* reserved for system timers */
> > > /delete-property/interrupts;        /* ok so it's a clocksource */
> > > /delete-property/interrupts-extended;
> >
> > That's not really what was meant.
>
> OK, so let's figure out something better then.
>
> > Let's say you have N timers. Either every timer is exactly the same and
> > the OS can just assign them however it wants or there is some difference
> > in the h/w making certain timer better for certain functions. Describe
> > that difference. It could be clock rate, number of counter bits, always
> > on, secure mode access only, has or doesn't have output compare or input
> > capture, etc.
>
> Hmm. Trying to detect this automatically will get messy. For example,
> we have few omap3 boards with the following options that also need to
> consider if the separate 32KiHz counter is available:
>
> 1. The best case scenario
>
> ti,omap-counter32k clocksource
> ti,sysc-omap2-timer ti,timer-alwon clockevent (timer1)
>
> 2. Boards relying on internal clock with unusable 32k counter
>
> ti,sysc-omap2-timer ti,timer-alwon clocksource (timer12)
> ti,sysc-omap2-timer clockevent (typically gpt2)
>
> In the second case, the 32k counter is unusable, and timer1
> is unusable with the external 32k always on clock. But timer1
> can be used with the system clock just fine for other purposes.
> So ideally we would not tag timer1 as disabled :)

I'm perfectly fine with a 'broken 32k clk' type property.

Though I think the compatibility story is not good changing DT for
stuff needed to boot and needed early in boot. It's one thing to break
something not required to get a system booted.

> For the second case, we could remove ti,timer-alwon property
> for timer1, and tag the 32k counter as disabled as the source
> clock is unreliable. Then somewhere in the code we would need
> to check if ti,omap-counter32k is availabe, then check if
> timer1 is always-on, then use timer12 if not a secure device
> like n900.

IIRC, there's some OMAP timer properties for secure vs. non-secure.
(It's not the first time we've had this discussion on TI timers.)

> If the board wants to use the system clock as the source for
> a higher resolution with assigned-clock-parents, then we'd need
> to ignore the always-on property and not use the 32k counter as
> the clocksource. Basically to somehow figure out that a higher
> resolution configuration is preferred over a
> low-power configuration.

That could be something you want to pick at run-time.

> So what's your take on just adding the generic properties for
> assigned-system-clocksource and clockevent?

I'm tired of discussing this for 10 years...

Rob

Tony Lindgren May 1, 2020, 2:11 p.m. UTC | #8

* Rob Herring <[e-mail]> [200501 13:19]:
> On Thu, Apr 30, 2020 at 10:31 AM Tony Lindgren <[e-mail]> wrote:
> > Hmm. Trying to detect this automatically will get messy. For example,
> > we have few omap3 boards with the following options that also need to
> > consider if the separate 32KiHz counter is available:
> >
> > 1. The best case scenario
> >
> > ti,omap-counter32k clocksource
> > ti,sysc-omap2-timer ti,timer-alwon clockevent (timer1)
> >
> > 2. Boards relying on internal clock with unusable 32k counter
> >
> > ti,sysc-omap2-timer ti,timer-alwon clocksource (timer12)
> > ti,sysc-omap2-timer clockevent (typically gpt2)
> >
> > In the second case, the 32k counter is unusable, and timer1
> > is unusable with the external 32k always on clock. But timer1
> > can be used with the system clock just fine for other purposes.
> > So ideally we would not tag timer1 as disabled :)
> 
> I'm perfectly fine with a 'broken 32k clk' type property.

OK. Maybe "unreliable-oscillator" type property or something like
that. Or maybe "shrodingers-cat-oscillator" :)

> Though I think the compatibility story is not good changing DT for
> stuff needed to boot and needed early in boot. It's one thing to break
> something not required to get a system booted.

For the boards with the 32k clock issue the system boots but
is unreliable. I'm not sure how long a 32k clock based timer would
have to be monitored with another system clock based timer to
detect it.. I recall the issues start popping up with PM and
that much later and the system clock may not even be active..

Anyways, the issue is related to how the board is wired, so a
property for it makes sense here.

> > For the second case, we could remove ti,timer-alwon property
> > for timer1, and tag the 32k counter as disabled as the source
> > clock is unreliable. Then somewhere in the code we would need
> > to check if ti,omap-counter32k is availabe, then check if
> > timer1 is always-on, then use timer12 if not a secure device
> > like n900.
> 
> IIRC, there's some OMAP timer properties for secure vs. non-secure.
> (It's not the first time we've had this discussion on TI timers.)

Yes.

> > If the board wants to use the system clock as the source for
> > a higher resolution with assigned-clock-parents, then we'd need
> > to ignore the always-on property and not use the 32k counter as
> > the clocksource. Basically to somehow figure out that a higher
> > resolution configuration is preferred over a
> > low-power configuration.
> 
> That could be something you want to pick at run-time.

OK

> > So what's your take on just adding the generic properties for
> > assigned-system-clocksource and clockevent?
> 
> I'm tired of discussing this for 10 years...

Well luckly most system have basic timers integrated nowadays
rather than each SoC vendor doing their own timers :)

Regards,

Tony

Patch

diff --git a/Documentation/devicetree/bindings/timer/ti,timer.txt b/Documentation/devicetree/bindings/timer/ti,timer.txt
--- a/Documentation/devicetree/bindings/timer/ti,timer.txt
+++ b/Documentation/devicetree/bindings/timer/ti,timer.txt
@@ -14,6 +14,8 @@  Required properties:
 			ti,omap5430-timer (applicable to OMAP543x devices)
 			ti,am335x-timer	(applicable to AM335x devices)
 			ti,am335x-timer-1ms (applicable to AM335x devices)
+			ti,dmtimer-clockevent (when used as for clockevent)
+			ti,dmtimer-clocksource (when used as for clocksource)
 
 - reg:			Contains timer register address range (base address and
 			length).
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -18,6 +18,7 @@  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_OMAP_DM_TIMER)	+= timer-ti-dm-systimer.o
 obj-$(CONFIG_DW_APB_TIMER)	+= dw_apb_timer.o
 obj-$(CONFIG_DW_APB_TIMER_OF)	+= dw_apb_timer_of.o
 obj-$(CONFIG_FTTMR010_TIMER)	+= timer-fttmr010.o
diff --git a/drivers/clocksource/timer-ti-dm-systimer.c b/drivers/clocksource/timer-ti-dm-systimer.c
new file mode 100644
--- /dev/null
+++ b/drivers/clocksource/timer-ti-dm-systimer.c
@@ -0,0 +1,468 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+#include <linux/clk.h>
+#include <linux/clocksource.h>
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/sched_clock.h>
+
+#include <linux/clk/clk-conf.h>
+
+#include <clocksource/timer-ti-dm.h>
+#include <dt-bindings/bus/ti-sysc.h>
+
+/* For type1, set SYSC_OMAP2_CLOCKACTIVITY for fck off on idle, l4 clock on */
+#define DMTIMER_TYPE1_ENABLE	((1 << 9) | (SYSC_IDLE_SMART << 3) | \
+				 SYSC_OMAP2_ENAWAKEUP | SYSC_OMAP2_AUTOIDLE)
+
+#define DMTIMER_TYPE2_ENABLE	(SYSC_IDLE_SMART_WKUP << 2)
+#define DMTIMER_RESET_WAIT	100000
+
+struct dmtimer_clockevent {
+	struct clock_event_device dev;
+	struct omap_dm_timer timer;
+};
+
+struct dmtimer_clocksource {
+	struct clocksource dev;
+	struct omap_dm_timer timer;
+	unsigned int loadval;
+};
+
+static int dmtimer_systimer_type1_reset(struct omap_dm_timer *timer)
+{
+	void __iomem *syss = timer->io_base + OMAP_TIMER_V1_SYS_STAT_OFFSET;
+	int ret;
+	u32 l;
+
+	__omap_dm_timer_write(timer, OMAP_TIMER_IF_CTRL_REG, 0x06, 0);
+	ret = readl_poll_timeout_atomic(syss, l, l & BIT(0), 100,
+					DMTIMER_RESET_WAIT);
+
+	return ret;
+}
+
+/* Note we must use io_base instead of func_base for type2 OCP regs */
+static int dmtimer_systimer_type2_reset(struct omap_dm_timer *timer)
+{
+	void __iomem *sysc = timer->io_base + OMAP_TIMER_OCP_CFG_OFFSET;
+	u32 l;
+
+	return readl_poll_timeout_atomic(sysc, l, !(l & BIT(0)), 100,
+					 DMTIMER_RESET_WAIT);
+}
+
+static int dmtimer_systimer_reset(struct omap_dm_timer *timer)
+{
+	int ret;
+
+	if (timer->revision == 1)
+		ret = dmtimer_systimer_type1_reset(timer);
+	else
+		ret = dmtimer_systimer_type2_reset(timer);
+	if (ret < 0) {
+		pr_err("%s failed with %i\n", __func__, ret);
+
+		return ret;
+	}
+
+	timer->posted = 0;
+
+	return 0;
+}
+
+/* Interface clocks are only available on some SoCs variants */
+static int __init dmtimer_systimer_init_clock(struct omap_dm_timer *timer,
+					      struct device_node *np,
+					      struct clk **clk,
+					      const char *name)
+{
+	struct clk *clock;
+	int error;
+
+	clock = of_clk_get_by_name(np, name);
+	if ((PTR_ERR(clock) == -EINVAL) && !strncmp(name, "ick", 3))
+		return 0;
+	else if (IS_ERR(clock))
+		return PTR_ERR(clock);
+
+	error = clk_prepare_enable(clock);
+	if (error)
+		return error;
+
+	*clk = clock;
+
+	return 0;
+}
+
+static void dmtimer_systimer_enable(struct omap_dm_timer *timer)
+{
+	void __iomem *sysc = timer->io_base + OMAP_TIMER_OCP_CFG_OFFSET;
+	u32 val;
+
+	if (timer->revision == 1)
+		val = DMTIMER_TYPE1_ENABLE;
+	else
+		val = DMTIMER_TYPE2_ENABLE;
+
+	writel_relaxed(val, sysc);
+}
+
+static void dmtimer_systimer_disable(struct omap_dm_timer *timer)
+{
+	void __iomem *sysc = timer->io_base + OMAP_TIMER_OCP_CFG_OFFSET;
+
+	writel_relaxed(0, sysc);
+}
+
+static int __init dmtimer_systimer_tag_disabled(struct device_node *np)
+{
+	struct property *prop;
+
+	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+	if (!prop)
+		return -ENOMEM;
+
+	prop->name = "status";
+	prop->value = "disabled";
+	prop->length = strlen(prop->value);
+
+	return of_add_property(np, prop);
+}
+
+static int __init dmtimer_systimer_init(struct omap_dm_timer *timer,
+					struct device_node *np)
+{
+	int error;
+
+	if (!of_device_is_compatible(np->parent, "ti,sysc"))
+		return -EINVAL;
+
+	/*
+	 * Enable optional assigned-clock-parents configured at the timer
+	 * node level. For regular device drivers, this is done automatically
+	 * by bus related code such as platform_drv_probe().
+	 */
+	error = of_clk_set_defaults(np, false);
+	if (error < 0)
+		pr_err("%s: clock source init failed: %i\n", __func__, error);
+
+	/* For ti-sysc, we have timer clocks at the parent module level */
+	error = dmtimer_systimer_init_clock(timer, np->parent,
+					    &timer->fclk, "fck");
+	if (error)
+		return error;
+
+	timer->rate = clk_get_rate(timer->fclk);
+
+	error = dmtimer_systimer_init_clock(timer, np->parent,
+					    &timer->iclk, "ick");
+	if (error)
+		return error;
+
+	__omap_dm_timer_init_regs(timer);
+	dmtimer_systimer_enable(timer);
+	dmtimer_systimer_reset(timer);
+	pr_debug("dmtimer rev %08x sysc %08x\n", readl_relaxed(timer->io_base),
+		 readl_relaxed(timer->io_base + OMAP_TIMER_OCP_CFG_OFFSET));
+
+	if (of_find_property(np, "ti,timer-alwon", NULL))
+		timer->capability |= OMAP_TIMER_ALWON;
+
+	/* Tag parent interconnect target module disabled for ti-sysc */
+	error = dmtimer_systimer_tag_disabled(np->parent);
+	if (error)
+		pr_err("%s: failed to set %pOF disabled: %i\n",
+		       __func__, np, error);
+
+	return 0;
+}
+
+/* Clockevent */
+static struct dmtimer_clockevent *
+to_dmtimer_clockevent(struct clock_event_device *clockevent)
+{
+	return container_of(clockevent, struct dmtimer_clockevent, dev);
+}
+
+static struct omap_dm_timer *
+clockevent_to_dm_timer(struct clock_event_device *evt)
+{
+	struct dmtimer_clockevent *dmtce = to_dmtimer_clockevent(evt);
+
+	return &dmtce->timer;
+}
+
+static irqreturn_t dmtimer_clockevent_interrupt(int irq, void *data)
+{
+	struct dmtimer_clockevent *dmtce = data;
+
+	__omap_dm_timer_write_status(&dmtce->timer, OMAP_TIMER_INT_OVERFLOW);
+	dmtce->dev.event_handler(&dmtce->dev);
+
+	return IRQ_HANDLED;
+}
+
+static struct irqaction dmtimer_clockevent_irq = {
+	.name		= "gp_timer",
+	.flags		= IRQF_TIMER | IRQF_IRQPOLL,
+	.handler	= dmtimer_clockevent_interrupt,
+};
+
+static int dmtimer_set_next_event(unsigned long cycles,
+				  struct clock_event_device *evt)
+{
+	struct omap_dm_timer *timer = clockevent_to_dm_timer(evt);
+
+	__omap_dm_timer_load_start(timer, OMAP_TIMER_CTRL_ST,
+				   0xffffffff - cycles, OMAP_TIMER_POSTED);
+
+	return 0;
+}
+
+static int dmtimer_clockevent_shutdown(struct clock_event_device *evt)
+{
+	struct omap_dm_timer *timer = clockevent_to_dm_timer(evt);
+
+	__omap_dm_timer_stop(timer, OMAP_TIMER_POSTED, timer->rate);
+
+	return 0;
+}
+
+static int dmtimer_set_periodic(struct clock_event_device *evt)
+{
+	struct omap_dm_timer *timer = clockevent_to_dm_timer(evt);
+	u32 period;
+
+	__omap_dm_timer_stop(timer, OMAP_TIMER_POSTED, timer->rate);
+
+	period = timer->rate / HZ;
+	period -= 1;
+	/* Looks like we need to first set the load value separately */
+	__omap_dm_timer_write(timer, OMAP_TIMER_LOAD_REG, 0xffffffff - period,
+			      OMAP_TIMER_POSTED);
+	__omap_dm_timer_load_start(timer,
+				   OMAP_TIMER_CTRL_AR | OMAP_TIMER_CTRL_ST,
+				   0xffffffff - period, OMAP_TIMER_POSTED);
+	return 0;
+}
+
+static void omap_clockevent_idle(struct clock_event_device *evt)
+{
+	struct omap_dm_timer *timer = clockevent_to_dm_timer(evt);
+
+	dmtimer_systimer_disable(timer);
+}
+
+static void omap_clockevent_unidle(struct clock_event_device *evt)
+{
+	struct omap_dm_timer *timer = clockevent_to_dm_timer(evt);
+
+	dmtimer_systimer_enable(timer);
+	__omap_dm_timer_int_enable(timer, OMAP_TIMER_INT_OVERFLOW);
+}
+
+static int __init dmtimer_clockevent_init(struct device_node *np)
+{
+	struct dmtimer_clockevent *clkevt;
+	struct clock_event_device *evt;
+	struct omap_dm_timer *timer;
+	int error;
+	u32 pa;
+
+	clkevt = kzalloc(sizeof(*clkevt), GFP_KERNEL | __GFP_NOFAIL);
+	if (!clkevt)
+		return -ENOMEM;
+
+	timer = &clkevt->timer;
+	evt = &clkevt->dev;
+
+	evt->features = CLOCK_EVT_FEAT_PERIODIC |
+			CLOCK_EVT_FEAT_ONESHOT;
+	evt->rating = 300;
+	evt->set_next_event = dmtimer_set_next_event;
+	evt->set_state_shutdown = dmtimer_clockevent_shutdown;
+	evt->set_state_periodic = dmtimer_set_periodic;
+	evt->set_state_oneshot = dmtimer_clockevent_shutdown;
+	evt->tick_resume = dmtimer_clockevent_shutdown;
+	evt->cpumask = cpu_possible_mask;
+	evt->irq = timer->irq;
+
+	timer->irq = irq_of_parse_and_map(np, 0);
+	if (!timer->irq) {
+		error = -ENXIO;
+		goto err_out_free;
+	}
+
+	timer->io_base = of_iomap(np, 0);
+	if (!timer->io_base) {
+		error = -ENXIO;
+		goto err_out_free;
+	}
+
+	error = dmtimer_systimer_init(timer, np);
+	if (error)
+		goto err_out_unmap;
+
+	/*
+	 * For clock-event timers we never read the timer counter and
+	 * so we are not impacted by errata i103 and i767. Therefore,
+	 * we can safely ignore this errata for clock-event timers.
+	 */
+	__omap_dm_timer_override_errata(timer, OMAP_TIMER_ERRATA_I103_I767);
+
+	dmtimer_clockevent_irq.dev_id = clkevt;
+	setup_irq(timer->irq, &dmtimer_clockevent_irq);
+
+	__omap_dm_timer_int_enable(timer, OMAP_TIMER_INT_OVERFLOW);
+
+	pa = of_translate_address(np, of_get_address(np, 0, NULL, NULL));
+	pr_info("TI gptimer clockevent: %stimer@%08x at %lu Hz\n",
+		timer->capability & OMAP_TIMER_ALWON ? "always-on " : "",
+		pa, timer->rate);
+
+	clockevents_config_and_register(evt, timer->rate,
+					3, /* Timer internal resynch latency */
+					0xffffffff);
+
+	if (of_device_is_compatible(np, "ti,am33xx") ||
+	    of_device_is_compatible(np, "ti,am43")) {
+		evt->suspend = omap_clockevent_idle;
+		evt->resume = omap_clockevent_unidle;
+	}
+
+	return 0;
+
+err_out_unmap:
+	iounmap(timer->io_base);
+err_out_free:
+	kfree(clkevt);
+
+	return error;
+}
+
+/* Clocksource */
+static struct dmtimer_clocksource *
+to_dmtimer_clocksource(struct clocksource *cs)
+{
+	return container_of(cs, struct dmtimer_clocksource, dev);
+}
+
+static struct omap_dm_timer *
+clocksource_to_dmtimer(struct clocksource *cs)
+{
+	struct dmtimer_clocksource *clksrc = to_dmtimer_clocksource(cs);
+
+	return &clksrc->timer;
+}
+
+static u64 dmtimer_clocksource_read_cycles(struct clocksource *cs)
+{
+	struct omap_dm_timer *timer = clocksource_to_dmtimer(cs);
+
+	return (u64)__omap_dm_timer_read_counter(timer,
+						 OMAP_TIMER_NONPOSTED);
+}
+
+static void __iomem *dmtimer_sched_clock_counter;
+
+static u64 notrace dmtimer_read_sched_clock(void)
+{
+	return readl_relaxed(dmtimer_sched_clock_counter);
+}
+
+static void dmtimer_clocksource_suspend(struct clocksource *cs)
+{
+	struct dmtimer_clocksource *clksrc = to_dmtimer_clocksource(cs);
+	struct omap_dm_timer *timer = &clksrc->timer;
+
+	clksrc->loadval =
+		__omap_dm_timer_read_counter(timer, OMAP_TIMER_NONPOSTED);
+	dmtimer_systimer_disable(timer);
+}
+
+static void dmtimer_clocksource_resume(struct clocksource *cs)
+{
+	struct dmtimer_clocksource *clksrc = to_dmtimer_clocksource(cs);
+	struct omap_dm_timer *timer = &clksrc->timer;
+
+	dmtimer_systimer_enable(timer);
+	__omap_dm_timer_load_start(timer,
+				   OMAP_TIMER_CTRL_ST | OMAP_TIMER_CTRL_AR,
+				   clksrc->loadval,
+				   OMAP_TIMER_NONPOSTED);
+}
+
+static int __init dmtimer_clocksource_init(struct device_node *np)
+{
+	struct dmtimer_clocksource *clksrc;
+	struct omap_dm_timer *timer;
+	struct clocksource *dev;
+	int error;
+	u32 pa;
+
+	clksrc = kzalloc(sizeof(*clksrc), GFP_KERNEL | __GFP_NOFAIL);
+	if (!clksrc)
+		return -ENOMEM;
+
+	timer = &clksrc->timer;
+	dev = &clksrc->dev;
+
+	timer->io_base = of_iomap(np, 0);
+	if (!timer->io_base) {
+		pr_err("%s: could not iomap\n", __func__);
+		return -ENODEV;
+	}
+
+	error = dmtimer_systimer_init(timer, np);
+	if (error)
+		goto err_out_unmap;
+
+	dev->name = "dmtimer";
+	dev->rating = 300;
+	dev->read = dmtimer_clocksource_read_cycles;
+	dev->mask = CLOCKSOURCE_MASK(32);
+	dev->flags = CLOCK_SOURCE_IS_CONTINUOUS;
+
+	if (of_device_is_compatible(np, "ti,am33xx") ||
+	    of_device_is_compatible(np, "ti,am43")) {
+		dev->suspend = dmtimer_clocksource_suspend;
+		dev->resume = dmtimer_clocksource_resume;
+	}
+
+	__omap_dm_timer_load_start(timer,
+				   OMAP_TIMER_CTRL_ST | OMAP_TIMER_CTRL_AR, 0,
+				   OMAP_TIMER_NONPOSTED);
+
+	pa = of_translate_address(np, of_get_address(np, 0, NULL, NULL));
+	pr_info("TI gptimer clocksource: %stimer@%08x %lu at Hz\n",
+		timer->capability & OMAP_TIMER_ALWON ?
+		"always-on " : "", pa, timer->rate);
+
+	if (!dmtimer_sched_clock_counter) {
+		/* Mask out write pending bits for raw OMAP_TIMER_COUNTER_REG */
+		dmtimer_sched_clock_counter =
+			timer->func_base + (OMAP_TIMER_COUNTER_REG & 0xff);
+		sched_clock_register(dmtimer_read_sched_clock, 32, timer->rate);
+	}
+
+	if (clocksource_register_hz(dev, timer->rate))
+		pr_err("Could not register clocksource %pOF\n", np);
+
+	return 0;
+
+err_out_unmap:
+	iounmap(timer->io_base);
+
+	return -ENODEV;
+}
+
+TIMER_OF_DECLARE(dmtimer_clockevent, "ti,dmtimer-clockevent",
+		 dmtimer_clockevent_init);
+TIMER_OF_DECLARE(dmtimer_clocksource, "ti,dmtimer-clocksource",
+		 dmtimer_clocksource_init);
diff --git a/include/clocksource/timer-ti-dm.h b/include/clocksource/timer-ti-dm.h
--- a/include/clocksource/timer-ti-dm.h
+++ b/include/clocksource/timer-ti-dm.h
@@ -97,6 +97,7 @@  struct omap_dm_timer {
 	int id;
 	int irq;
 	struct clk *fclk;
+	struct clk *iclk;
 
 	void __iomem	*io_base;
 	void __iomem	*irq_stat;	/* TISR/IRQSTATUS interrupt status */