On Sat, 11 Jun 2016 00:03:45 +0200
Alexandre Belloni [off-list ref] wrote:

Add a driver for the Atmel Timer Counter Blocks. This driver provides a
clocksource and a clockevent device. The clockevent device is linked to the
clocksource counter and so it will run at the same frequency.

This driver uses regmap and syscon to be able to probe early in the boot
and avoid having to switch on the TCB clocksource later. Using regmap also
means that unused TCB channels may be used by other drivers (PWM for
example).

First of all, thanks for working on this
tcb/libtcb/tcb-clksource/tcb-pwm mess. It looks a lot cleaner after
your changes (both the DT representation and the code itself).

Cc: Daniel Lezcano <redacted>
Cc: Thomas Gleixner <redacted>
Signed-off-by: Alexandre Belloni <redacted>
---
 drivers/clocksource/Kconfig                 |  13 ++
 drivers/clocksource/Makefile                |   3 +-
 drivers/clocksource/timer-atmel-tcbclksrc.c | 305 ++++++++++++++++++++++++++++
 include/soc/at91/atmel_tcb.h                | 220 ++++++++++++++++++++

I think the creation of atmel_tcb.h should be done in a separate commit.

			^ 2 tabs here

+	},
+	.clkevt	= {
+		.name			= "tcb_clkevt",

			^ 3 here

Can you make that consistent?
Actually, I'm not a big fan of those tabs, but if you decide to use
tabs, use the same number of them everywhere.

+		.features		= CLOCK_EVT_FEAT_ONESHOT,
+		/* Should be lower than at91rm9200's system timer */
+		.rating			= 125,
+	},
+};
+

[...]

+
+static int tcb_clkevt_next_event(unsigned long delta,
+				 struct clock_event_device *d)
+{
+	u32 val;
+
+	regmap_read(tc.regmap, ATMEL_TC_CV(tc.channels[0]), &val);
+	regmap_write(tc.regmap, ATMEL_TC_RC(tc.channels[0]), val + delta);
+	regmap_write(tc.regmap, ATMEL_TC_IER(tc.channels[0]), ATMEL_TC_CPCS);

Hm, not sure this is 100% sure. What happens if by the time you write
TC_RC, the delta value has expired? This means you'll have to wait
another round before the TC engine generates the "RC reached" interrupt.

I know this is very unlikely, but should we take the risk?

The core seems to check the ->set_next_event() return value and tries to
adjust ->min_delta_ns if it returns an error, so maybe it's worth
testing if val + delta has already occurred just before enabling the
TC_CPCS interrupt, and if it's the case, return an -ETIME error.

Something like:

	u32 val[2], next;

	regmap_read(tc.regmap, ATMEL_TC_CV(tc.channels[0]), &val[0]);
	next = (val[0] + delta) & GENMASK(tc.bits - 1, 0);
	regmap_write(tc.regmap, ATMEL_TC_RC(tc.channels[0]), next);
	regmap_read(tc.regmap, ATMEL_TC_CV(tc.channels[0]), &val[1]);

	if ((next < val[0] && val[1] < val[0] && val[1] >= next) ||
	    (next > val[0] && (val[1] < val[0] || val[1] >= next))) {
		/*
		 * Clear the CPCS bit in the status register to avoid
		 * generating a spurious interrupt next time a valid
		 * timer event is configured.
		 * FIXME: not sure it's safe, since it also clears the
		 * overflow status, but it seems this flag is not used
		 * by the driver anyway.
		 */
		regmap_read(tc.regmap, ATMEL_TC_SR, &val[0]);
		return -ETIME;
	}

	
	regmap_write(tc.regmap, ATMEL_TC_IER(tc.channels[0]),
		     ATMEL_TC_CPCS);

Thomas, Daniel, what's your opinion? 

+
+	return 0;
+}
+

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com