Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1653631 > unrolled thread

[PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks

Started byAlexandre Belloni <alexandre.belloni@free-electrons.com>
First post2017-05-31 00:00 +0200
Last post2017-06-07 17:30 +0200
Articles 6 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-05-31 00:00 +0200
    Re: [PATCH 46/58] clocksource/drivers: Add a new driver for the  Atmel ARM TC blocks Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-06-06 17:30 +0200
      Re: [PATCH 46/58] clocksource/drivers: Add a new driver for the  Atmel ARM TC blocks Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-06-06 20:10 +0200
        Re: [PATCH 46/58] clocksource/drivers: Add a new driver for the  Atmel ARM TC blocks Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-06-07 16:20 +0200
          Re: [PATCH 46/58] clocksource/drivers: Add a new driver for the  Atmel ARM TC blocks Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-06-07 17:10 +0200
          Re: [PATCH 46/58] clocksource/drivers: Add a new driver for the  Atmel ARM TC blocks Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-06-07 17:30 +0200

#1653631 — [PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks

FromAlexandre Belloni <alexandre.belloni@free-electrons.com>
Date2017-05-31 00:00 +0200
Subject[PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks
Message-ID<tMXli-4i6-55@gated-at.bofh.it>
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).

Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/clocksource/Kconfig                 |  13 ++
 drivers/clocksource/Makefile                |   3 +-
 drivers/clocksource/timer-atmel-tcbclksrc.c | 252 ++++++++++++++++++++++++++++
 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];
+	struct clocksource clksrc;
+	struct regmap *regmap;
+	struct clk *clk[2];
+	int channels[2];
+	int bits;
+	int irq;
+	bool registered;
+} tc = {
+	.clksrc = {
+		.rating		= 200,
+		.mask		= CLOCKSOURCE_MASK(32),
+		.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
+	},
+};
+
+static u64 tc_get_cycles(struct clocksource *cs)
+{
+	u32		lower, upper, tmp;
+
+	do {
+		regmap_read(tc.regmap, ATMEL_TC_CV(1), &upper);
+		regmap_read(tc.regmap, ATMEL_TC_CV(0), &lower);
+		regmap_read(tc.regmap, ATMEL_TC_CV(1), &tmp);
+	} while (upper != tmp);
+
+	return (upper << 16) | lower;
+}
+
+static u64 tc_get_cycles32(struct clocksource *cs)
+{
+	u32 val;
+
+	regmap_read(tc.regmap, ATMEL_TC_CV(tc.channels[0]), &val);
+
+	return val;
+}
+
+static u64 notrace tc_sched_clock_read(void)
+{
+	return tc_get_cycles(&tc.clksrc);
+}
+
+static u64 notrace tc_sched_clock_read32(void)
+{
+	return tc_get_cycles32(&tc.clksrc);
+}
+
+static void __init tcb_setup_dual_chan(struct atmel_tcb_clksrc *tc,
+				       int mck_divisor_idx)
+{
+	/* first channel: waveform mode, input mclk/8, clock TIOA on overflow */
+	regmap_write(tc->regmap, ATMEL_TC_CMR(tc->channels[0]),
+		     mck_divisor_idx	/* likely divide-by-8 */
+			| ATMEL_TC_CMR_WAVE
+			| ATMEL_TC_CMR_WAVESEL_UP	/* free-run */
+			| ATMEL_TC_CMR_ACPA(SET)	/* TIOA rises at 0 */
+			| ATMEL_TC_CMR_ACPC(CLEAR));	/* (duty cycle 50%) */
+	regmap_write(tc->regmap, ATMEL_TC_RA(tc->channels[0]), 0x0000);
+	regmap_write(tc->regmap, ATMEL_TC_RC(tc->channels[0]), 0x8000);
+	regmap_write(tc->regmap, ATMEL_TC_IDR(tc->channels[0]), 0xff);	/* no irqs */
+	regmap_write(tc->regmap, ATMEL_TC_CCR(tc->channels[0]),
+		     ATMEL_TC_CCR_CLKEN);
+
+	/* second channel: waveform mode, input TIOA */
+	regmap_write(tc->regmap, ATMEL_TC_CMR(tc->channels[1]),
+		     ATMEL_TC_CMR_XC(tc->channels[1])	/* input: TIOA */
+		     | ATMEL_TC_CMR_WAVE
+		     | ATMEL_TC_CMR_WAVESEL_UP);	/* free-run */
+	regmap_write(tc->regmap, ATMEL_TC_IDR(tc->channels[1]), 0xff);	/* no irqs */
+	regmap_write(tc->regmap, ATMEL_TC_CCR(tc->channels[1]),
+		     ATMEL_TC_CCR_CLKEN);
+
+	/* chain both channel, we assume the previous channel */
+	regmap_write(tc->regmap, ATMEL_TC_BMR,
+		     ATMEL_TC_BMR_TCXC(1 + tc->channels[1], tc->channels[1]));
+	/* then reset all the timers */
+	regmap_write(tc->regmap, ATMEL_TC_BCR, ATMEL_TC_BCR_SYNC);
+}
+
+static void __init tcb_setup_single_chan(struct atmel_tcb_clksrc *tc,
+					 int mck_divisor_idx)
+{
+	/* channel 0:  waveform mode, input mclk/8 */
+	regmap_write(tc->regmap, ATMEL_TC_CMR(tc->channels[0]),
+		     mck_divisor_idx	/* likely divide-by-8 */
+			| ATMEL_TC_CMR_WAVE
+			| ATMEL_TC_CMR_WAVESEL_UP	/* free-run */
+			);
+	regmap_write(tc->regmap, ATMEL_TC_IDR(tc->channels[0]), 0xff);	/* no irqs */
+	regmap_write(tc->regmap, ATMEL_TC_CCR(tc->channels[0]),
+		     ATMEL_TC_CCR_CLKEN);
+
+	/* then reset all the timers */
+	regmap_write(tc->regmap, ATMEL_TC_BCR, ATMEL_TC_BCR_SYNC);
+}
+
+static int __init tcb_clksrc_register(struct device_node *node,
+				      struct regmap *regmap, int channel,
+				      int channel1, int irq, int bits)
+{
+	u32 rate, divided_rate = 0;
+	int best_divisor_idx = -1;
+	int i, err = -1;
+	u64 (*tc_sched_clock)(void);
+
+	tc.regmap = regmap;
+	tc.channels[0] = channel;
+	tc.channels[1] = channel1;
+	tc.irq = irq;
+	tc.bits = bits;
+
+	tc.clk[0] = tcb_clk_get(node, tc.channels[0]);
+	if (IS_ERR(tc.clk[0]))
+		return PTR_ERR(tc.clk[0]);
+	err = clk_prepare_enable(tc.clk[0]);
+	if (err) {
+		pr_debug("can't enable T0 clk\n");
+		goto err_clk;
+	}
+
+	/* How fast will we be counting?  Pick something over 5 MHz.  */
+	rate = (u32)clk_get_rate(tc.clk[0]);
+	for (i = 0; i < 5; i++) {
+		unsigned int divisor = atmel_tc_divisors[i];
+		unsigned int tmp;
+
+		if (!divisor)
+			continue;
+
+		tmp = rate / divisor;
+		pr_debug("TC: %u / %-3u [%d] --> %u\n", rate, divisor, i, tmp);
+		if (best_divisor_idx > 0) {
+			if (tmp < 5 * 1000 * 1000)
+				continue;
+		}
+		divided_rate = tmp;
+		best_divisor_idx = i;
+	}
+
+	if (tc.bits == 32) {
+		tc.clksrc.read = tc_get_cycles32;
+		tcb_setup_single_chan(&tc, best_divisor_idx);
+		tc_sched_clock = tc_sched_clock_read32;
+		snprintf(tc.name, sizeof(tc.name), "%s:%d",
+			 kbasename(node->parent->full_name), tc.channels[0]);
+	} else {
+		tc.clk[1] = tcb_clk_get(node, tc.channels[1]);
+		if (IS_ERR(tc.clk[1]))
+			goto err_disable_t0;
+
+		err = clk_prepare_enable(tc.clk[1]);
+		if (err) {
+			pr_debug("can't enable T1 clk\n");
+			goto err_clk1;
+		}
+		tc.clksrc.read = tc_get_cycles,
+		tcb_setup_dual_chan(&tc, best_divisor_idx);
+		tc_sched_clock = tc_sched_clock_read;
+		snprintf(tc.name, sizeof(tc.name), "%s:%d,%d",
+			 kbasename(node->parent->full_name), tc.channels[0],
+			 tc.channels[1]);
+	}
+
+	pr_debug("%s at %d.%03d MHz\n", tc.name,
+		 divided_rate / 1000000,
+		 ((divided_rate + 500000) % 1000000) / 1000);
+
+	tc.clksrc.name = tc.name;
+
+	err = clocksource_register_hz(&tc.clksrc, divided_rate);
+	if (err)
+		goto err_disable_t1;
+
+	sched_clock_register(tc_sched_clock, 32, divided_rate);
+
+	tc.registered = true;
+
+	return 0;
+
+err_disable_t1:
+	if (tc.bits == 16)
+		clk_disable_unprepare(tc.clk[1]);
+
+err_clk1:
+	if (tc.bits == 16)
+		clk_put(tc.clk[1]);
+
+err_disable_t0:
+	clk_disable_unprepare(tc.clk[0]);
+
+err_clk:
+	clk_put(tc.clk[0]);
+
+	pr_err("%s: unable to register clocksource/clockevent\n",
+	       tc.clksrc.name);
+
+	return err;
+}
+
+static int __init tcb_clksrc_init(struct device_node *node)
+{
+	const struct of_device_id *match;
+	const struct atmel_tcb_info *tcb_info;
+	struct regmap *regmap;
+	u32 channel;
+	int bits, irq, err, chan1 = -1;
+
+	if (tc.registered)
+		return -ENODEV;
+
+	regmap = syscon_node_to_regmap(node->parent);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	match = of_match_node(atmel_tcb_dt_ids, node->parent);
+	tcb_info = match->data;
+	bits = tcb_info->bits;
+
+	err = of_property_read_u32_index(node, "reg", 0, &channel);
+	if (err)
+		return err;
+
+	irq = tcb_irq_get(node, channel);
+	if (irq < 0)
+		return irq;
+
+	if (bits == 16) {
+		of_property_read_u32_index(node, "reg", 1, &chan1);
+		if (chan1 == -1) {
+			pr_err("%s: clocksource needs two channels\n",
+			       node->parent->full_name);
+			return -EINVAL;
+		}
+	}
+
+	return tcb_clksrc_register(node, regmap, channel, chan1, irq, bits);
+}
+CLOCKSOURCE_OF_DECLARE(atmel_tcb_clksrc, "atmel,tcb-timer",
+		       tcb_clksrc_init);
-- 
2.11.0

[toc] | [next] | [standalone]

#1658866 — Re: [PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks

FromDaniel Lezcano <daniel.lezcano@linaro.org>
Date2017-06-06 17:30 +0200
SubjectRe: [PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks
Message-ID<tPoAG-27S-23@gated-at.bofh.it>
In reply to#1653631
On Tue, May 30, 2017 at 11:51:27PM +0200, Alexandre Belloni 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.

Where is the clockevent in this driver?

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.

> 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()?

> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>  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

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

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

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

Can you explain why we have two clocks here?

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

> +	int bits;

	^^^^^^^^

This field is pointless.

> +	int irq;

irq belongs to clockevents changes.

> +	bool registered;

What is the purpose of this registered boolean?

> +} tc = {
> +	.clksrc = {
> +		.rating		= 200,
> +		.mask		= CLOCKSOURCE_MASK(32),
> +		.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
> +	},
> +};
> +
> +static u64 tc_get_cycles(struct clocksource *cs)
> +{
> +	u32		lower, upper, tmp;

Fix these trailing spaces/tab.

> +
> +	do {
> +		regmap_read(tc.regmap, ATMEL_TC_CV(1), &upper);
> +		regmap_read(tc.regmap, ATMEL_TC_CV(0), &lower);
> +		regmap_read(tc.regmap, ATMEL_TC_CV(1), &tmp);
> +	} while (upper != tmp);
> +
> +	return (upper << 16) | lower;
> +}
> +
> +static u64 tc_get_cycles32(struct clocksource *cs)
> +{
> +	u32 val;
> +
> +	regmap_read(tc.regmap, ATMEL_TC_CV(tc.channels[0]), &val);
> +
> +	return val;
> +}
> +
> +static u64 notrace tc_sched_clock_read(void)
> +{
> +	return tc_get_cycles(&tc.clksrc);
> +}
> +
> +static u64 notrace tc_sched_clock_read32(void)
> +{
> +	return tc_get_cycles32(&tc.clksrc);
> +}
> +
> +static void __init tcb_setup_dual_chan(struct atmel_tcb_clksrc *tc,
> +				       int mck_divisor_idx)
> +{
> +	/* first channel: waveform mode, input mclk/8, clock TIOA on overflow */
> +	regmap_write(tc->regmap, ATMEL_TC_CMR(tc->channels[0]),
> +		     mck_divisor_idx	/* likely divide-by-8 */
> +			| ATMEL_TC_CMR_WAVE
> +			| ATMEL_TC_CMR_WAVESEL_UP	/* free-run */
> +			| ATMEL_TC_CMR_ACPA(SET)	/* TIOA rises at 0 */
> +			| ATMEL_TC_CMR_ACPC(CLEAR));	/* (duty cycle 50%) */
> +	regmap_write(tc->regmap, ATMEL_TC_RA(tc->channels[0]), 0x0000);
> +	regmap_write(tc->regmap, ATMEL_TC_RC(tc->channels[0]), 0x8000);
> +	regmap_write(tc->regmap, ATMEL_TC_IDR(tc->channels[0]), 0xff);	/* no irqs */
> +	regmap_write(tc->regmap, ATMEL_TC_CCR(tc->channels[0]),
> +		     ATMEL_TC_CCR_CLKEN);
> +
> +	/* second channel: waveform mode, input TIOA */
> +	regmap_write(tc->regmap, ATMEL_TC_CMR(tc->channels[1]),
> +		     ATMEL_TC_CMR_XC(tc->channels[1])	/* input: TIOA */
> +		     | ATMEL_TC_CMR_WAVE
> +		     | ATMEL_TC_CMR_WAVESEL_UP);	/* free-run */
> +	regmap_write(tc->regmap, ATMEL_TC_IDR(tc->channels[1]), 0xff);	/* no irqs */
> +	regmap_write(tc->regmap, ATMEL_TC_CCR(tc->channels[1]),
> +		     ATMEL_TC_CCR_CLKEN);
> +
> +	/* chain both channel, we assume the previous channel */
> +	regmap_write(tc->regmap, ATMEL_TC_BMR,
> +		     ATMEL_TC_BMR_TCXC(1 + tc->channels[1], tc->channels[1]));
> +	/* then reset all the timers */
> +	regmap_write(tc->regmap, ATMEL_TC_BCR, ATMEL_TC_BCR_SYNC);
> +}
> +
> +static void __init tcb_setup_single_chan(struct atmel_tcb_clksrc *tc,
> +					 int mck_divisor_idx)
> +{
> +	/* channel 0:  waveform mode, input mclk/8 */
> +	regmap_write(tc->regmap, ATMEL_TC_CMR(tc->channels[0]),
> +		     mck_divisor_idx	/* likely divide-by-8 */
> +			| ATMEL_TC_CMR_WAVE
> +			| ATMEL_TC_CMR_WAVESEL_UP	/* free-run */
> +			);
> +	regmap_write(tc->regmap, ATMEL_TC_IDR(tc->channels[0]), 0xff);	/* no irqs */
> +	regmap_write(tc->regmap, ATMEL_TC_CCR(tc->channels[0]),
> +		     ATMEL_TC_CCR_CLKEN);
> +
> +	/* then reset all the timers */
> +	regmap_write(tc->regmap, ATMEL_TC_BCR, ATMEL_TC_BCR_SYNC);
> +}
> +
> +static int __init tcb_clksrc_register(struct device_node *node,
> +				      struct regmap *regmap, int channel,
> +				      int channel1, int irq, int bits)
> +{
> +	u32 rate, divided_rate = 0;
> +	int best_divisor_idx = -1;
> +	int i, err = -1;
> +	u64 (*tc_sched_clock)(void);
> +
> +	tc.regmap = regmap;
> +	tc.channels[0] = channel;
> +	tc.channels[1] = channel1;
> +	tc.irq = irq;
> +	tc.bits = bits;
> +
> +	tc.clk[0] = tcb_clk_get(node, tc.channels[0]);
> +	if (IS_ERR(tc.clk[0]))
> +		return PTR_ERR(tc.clk[0]);
> +	err = clk_prepare_enable(tc.clk[0]);
> +	if (err) {
> +		pr_debug("can't enable T0 clk\n");
> +		goto err_clk;
> +	}
> +
> +	/* How fast will we be counting?  Pick something over 5 MHz.  */
> +	rate = (u32)clk_get_rate(tc.clk[0]);
> +	for (i = 0; i < 5; i++) {
> +		unsigned int divisor = atmel_tc_divisors[i];
> +		unsigned int tmp;
> +
> +		if (!divisor)
> +			continue;
> +
> +		tmp = rate / divisor;
> +		pr_debug("TC: %u / %-3u [%d] --> %u\n", rate, divisor, i, tmp);
> +		if (best_divisor_idx > 0) {
> +			if (tmp < 5 * 1000 * 1000)
> +				continue;
> +		}
> +		divided_rate = tmp;
> +		best_divisor_idx = i;
> +	}
> +
> +	if (tc.bits == 32) {
> +		tc.clksrc.read = tc_get_cycles32;
> +		tcb_setup_single_chan(&tc, best_divisor_idx);
> +		tc_sched_clock = tc_sched_clock_read32;
> +		snprintf(tc.name, sizeof(tc.name), "%s:%d",
> +			 kbasename(node->parent->full_name), tc.channels[0]);
> +	} else {
> +		tc.clk[1] = tcb_clk_get(node, tc.channels[1]);
> +		if (IS_ERR(tc.clk[1]))
> +			goto err_disable_t0;
> +
> +		err = clk_prepare_enable(tc.clk[1]);
> +		if (err) {
> +			pr_debug("can't enable T1 clk\n");
> +			goto err_clk1;
> +		}
> +		tc.clksrc.read = tc_get_cycles,
> +		tcb_setup_dual_chan(&tc, best_divisor_idx);
> +		tc_sched_clock = tc_sched_clock_read;
> +		snprintf(tc.name, sizeof(tc.name), "%s:%d,%d",
> +			 kbasename(node->parent->full_name), tc.channels[0],
> +			 tc.channels[1]);
> +	}
> +
> +	pr_debug("%s at %d.%03d MHz\n", tc.name,
> +		 divided_rate / 1000000,
> +		 ((divided_rate + 500000) % 1000000) / 1000);
> +
> +	tc.clksrc.name = tc.name;
> +
> +	err = clocksource_register_hz(&tc.clksrc, divided_rate);
> +	if (err)
> +		goto err_disable_t1;
> +
> +	sched_clock_register(tc_sched_clock, 32, divided_rate);
> +
> +	tc.registered = true;
> +
> +	return 0;
> +
> +err_disable_t1:
> +	if (tc.bits == 16)
> +		clk_disable_unprepare(tc.clk[1]);
> +
> +err_clk1:
> +	if (tc.bits == 16)
> +		clk_put(tc.clk[1]);
> +
> +err_disable_t0:
> +	clk_disable_unprepare(tc.clk[0]);
> +
> +err_clk:
> +	clk_put(tc.clk[0]);
> +
> +	pr_err("%s: unable to register clocksource/clockevent\n",
> +	       tc.clksrc.name);
> +
> +	return err;
> +}
> +
> +static int __init tcb_clksrc_init(struct device_node *node)
> +{
> +	const struct of_device_id *match;
> +	const struct atmel_tcb_info *tcb_info;
> +	struct regmap *regmap;
> +	u32 channel;
> +	int bits, irq, err, chan1 = -1;
> +
> +	if (tc.registered)
> +		return -ENODEV;
> +
> +	regmap = syscon_node_to_regmap(node->parent);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	match = of_match_node(atmel_tcb_dt_ids, node->parent);
> +	tcb_info = match->data;
> +	bits = tcb_info->bits;
> +
> +	err = of_property_read_u32_index(node, "reg", 0, &channel);
> +	if (err)
> +		return err;
> +
> +	irq = tcb_irq_get(node, channel);
> +	if (irq < 0)
> +		return irq;
> +
> +	if (bits == 16) {
> +		of_property_read_u32_index(node, "reg", 1, &chan1);
> +		if (chan1 == -1) {
> +			pr_err("%s: clocksource needs two channels\n",
> +			       node->parent->full_name);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return tcb_clksrc_register(node, regmap, channel, chan1, irq, bits);
> +}
> +CLOCKSOURCE_OF_DECLARE(atmel_tcb_clksrc, "atmel,tcb-timer",
> +		       tcb_clksrc_init);
> -- 
> 2.11.0
> 

-- 

 <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

[toc] | [prev] | [next] | [standalone]

#1659012 — Re: [PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks

FromAlexandre Belloni <alexandre.belloni@free-electrons.com>
Date2017-06-06 20:10 +0200
SubjectRe: [PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks
Message-ID<tPr5w-3PW-21@gated-at.bofh.it>
In reply to#1658866
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:
> > 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.

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


> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > ---
> >  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
> 
> >  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?

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

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

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

> > +	int bits;
> 
> 	^^^^^^^^
> 
> This field is pointless.
> 
> > +	int irq;
> 
> irq belongs to clockevents changes.
> 
> > +	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

[toc] | [prev] | [next] | [standalone]

#1659843 — Re: [PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks

FromDaniel Lezcano <daniel.lezcano@linaro.org>
Date2017-06-07 16:20 +0200
SubjectRe: [PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks
Message-ID<tPJYu-7IG-19@gated-at.bofh.it>
In reply to#1659012
On Tue, Jun 06, 2017 at 08:05:59PM +0200, Alexandre Belloni wrote:
> 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:
> > > 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.

As you wish. A single driver or a split into consistent parts.

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

So is it still necesary to use regmap? I would like to take the opportunity to
move the init routine to the common init routine if possible:

	https://patchwork.kernel.org/patch/9768845/

> > > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > > ---
> > >  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
> > 
> > >  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?

The field is not needed, the only place where it is used is where we affect it.
 
> > > +	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.
>
> > > +	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.

I don't understand. Why do we have two clocks?

One channel is driven by one clock and the second one takes the overflow signal
from the first one, so no second clock is involved there, no?

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

Ok, I see.

> > > +	int bits;
> > 
> > 	^^^^^^^^
> > 
> > This field is pointless.
> > 
> > > +	int irq;
> > 
> > irq belongs to clockevents changes.
> > 
> > > +	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.

Yes, we had this discussion some weeks ago.

This registered hack forces the DT to define first the clocksource, then the
clockevent.

So, I suggest you fold the timer definition into a single one like the other
drivers.


-- 

 <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

[toc] | [prev] | [next] | [standalone]

#1659907 — Re: [PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks

FromAlexandre Belloni <alexandre.belloni@free-electrons.com>
Date2017-06-07 17:10 +0200
SubjectRe: [PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks
Message-ID<tPKKR-8hH-1@gated-at.bofh.it>
In reply to#1659843
On 07/06/2017 at 16:17:35 +0200, Daniel Lezcano wrote:
> > > > 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.
> 
> So is it still necesary to use regmap? I would like to take the opportunity to
> move the init routine to the common init routine if possible:
> 
> 	https://patchwork.kernel.org/patch/9768845/
> 

It is still necessary because we want to be able to share the timer
between multiple drivers. For example, you can have the clocksource on
channel 0, clockevent on channel 1 and a pwm on channel 2

> > > Can you explain why we have two clocks here?
> > > 
> > 
> > Each channel have its clock, I can add a comment if you want.
> 
> I don't understand. Why do we have two clocks?
> 
> One channel is driven by one clock and the second one takes the overflow signal
> from the first one, so no second clock is involved there, no?
> 

Those are the peripheral clocks, they are not used by the counters but
used to be able to read/write the registers.

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

[toc] | [prev] | [next] | [standalone]

#1659928 — Re: [PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks

FromAlexandre Belloni <alexandre.belloni@free-electrons.com>
Date2017-06-07 17:30 +0200
SubjectRe: [PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks
Message-ID<tPL4e-8ou-23@gated-at.bofh.it>
In reply to#1659843
On 07/06/2017 at 16:17:35 +0200, Daniel Lezcano wrote:
> > You mean you don't like how it is used? Or you don't think having the
> > timer full name is useful?
> 
> The field is not needed, the only place where it is used is where we affect it.
>  

It is used in tcb_clksrc_register:

tc.clksrc.name = tc.name;

> > 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.
> 
> Yes, we had this discussion some weeks ago.
> 
> This registered hack forces the DT to define first the clocksource, then the
> clockevent.
> 
> So, I suggest you fold the timer definition into a single one like the other
> drivers.

I was going to agree but this is not flexible enough because the
quadrature decoder always uses the first two channels. So on some
products, we may have:
 - TCB0:
   o channels 0,1: qdec
   o channel 2: clocksource

 - TCB1:
   o channels 0,1: qdec
   o channel 2: clockevent

This avoids wasting TCB channels.

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web