On 06/06/2017 at 17:21:05 +0200, Daniel Lezcano wrote:

On Tue, May 30, 2017 at 11:51:27PM +0200, Alexandre Belloni wrote:

quoted

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.

Where is the clockevent in this driver?

That's a leftover from v1, it seems I forgot to rework that commit
message.

It seems the cutting out of this driver is a bit fuzzy and hard to follow.

Please re-org the changes in a logical manner when resubmitting.

I can submit the whole driver as a single patch if that is easier for
you to review.

quoted

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).

Can you give more details, I fail to understand how regmap and syscon help to
probe sooner than timer_init()?

Because before that, the tcb driver relied on atmel_tclib to share the
TCBs and it happened way too late, at arch_initcall() time.

quoted

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 | 252 ++++++++++++++++++++++++++++

As it is a clksrc + clkevt, please change the name to something more adequate:

eg. timer-tcb.c

quoted

 3 files changed, 267 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clocksource/timer-atmel-tcbclksrc.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 545d541ae20e..cbd710db3c09 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig

@@ -418,6 +418,19 @@ config ATMEL_ST
 	help
 	  Support for the Atmel ST timer.
 
+config ATMEL_ARM_TCB_CLKSRC
+	bool "TC Block Clocksource"
+	select REGMAP_MMIO
+	depends on GENERIC_CLOCKEVENTS
+	depends on SOC_AT91RM9200 || SOC_AT91SAM9 || SOC_SAMA5 || COMPILE_TEST
+	default SOC_AT91RM9200 || SOC_AT91SAM9 || SOC_SAMA5
+	help
+	  Select this to get a high precision clocksource based on a
+	  TC block with a 5+ MHz base clock rate.
+	  On platforms with 16-bit counters, two timer channels are combined
+	  to make a single 32-bit timer.
+	  It can also be used as a clock event device supporting oneshot mode.
+
 config CLKSRC_METAG_GENERIC
 	def_bool y if METAG
 	help

diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 2b5b56a6f00f..53a0b40e0db2 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile

@@ -2,7 +2,8 @@ obj-$(CONFIG_CLKSRC_PROBE)	+= clksrc-probe.o
 obj-$(CONFIG_CLKEVT_PROBE)	+= clkevt-probe.o
 obj-$(CONFIG_ATMEL_PIT)		+= timer-atmel-pit.o
 obj-$(CONFIG_ATMEL_ST)		+= timer-atmel-st.o
-obj-$(CONFIG_ATMEL_TCB_CLKSRC)	+= tcb_clksrc.o
+obj-$(CONFIG_ATMEL_TCB_CLKSRC) += tcb_clksrc.o
+obj-$(CONFIG_ATMEL_ARM_TCB_CLKSRC)	+= timer-atmel-tcbclksrc.o
 obj-$(CONFIG_X86_PM_TIMER)	+= acpi_pm.o
 obj-$(CONFIG_SCx200HR_TIMER)	+= scx200_hrt.o
 obj-$(CONFIG_CS5535_CLOCK_EVENT_SRC)	+= cs5535-clockevt.o

diff --git a/drivers/clocksource/timer-atmel-tcbclksrc.c b/drivers/clocksource/timer-atmel-tcbclksrc.c
new file mode 100644
index 000000000000..f18d177bfdea
--- /dev/null
+++ b/drivers/clocksource/timer-atmel-tcbclksrc.c

@@ -0,0 +1,252 @@
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/clocksource.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_irq.h>
+#include <linux/regmap.h>
+#include <linux/sched_clock.h>
+#include <soc/at91/atmel_tcb.h>
+
+static struct atmel_tcb_clksrc {
+	char name[20];

	^^^^^^^^^^^^^^
This field is pointless.

You mean you don't like how it is used? Or you don't think having the
timer full name is useful?

quoted

+	struct clocksource clksrc;

Why is this field defined and passed around to functions which do not use it?

Please consider using clocksource_mmio_init() and remove this field.

Well, this doesn't work with a regmap so it doesn't fit.

quoted

+	struct regmap *regmap;
+	struct clk *clk[2];

Can you explain why we have two clocks here?

Each channel have its clock, I can add a comment if you want.

quoted

+	int channels[2];

Instead of dealing with 2 channels and a costly bits shifting in the hot path,
why not use a single channel with a different wrap up? IOW mask is 16 or 32.

The resulting code will be simpler, nicer and perhaps more efficient if you
save the tc_get_cycles() loop?

I think the rationale behind it is that 16 bits at 5MHz makes it
wrap every 13ms which is too fast to be useful.

quoted

+	int bits;

	^^^^^^^^

This field is pointless.

quoted

+	int irq;

irq belongs to clockevents changes.

quoted

+	bool registered;

What is the purpose of this registered boolean?

The main reason is that RobH doesn't want to have the use (clocksource
or clockevent) of the timer in the DT so when probing a timer, I need to
know whether I already have a clocksource to decide when it is time to
register a clockevent.

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