| Message ID | 20170530215139.9983-47-alexandre.belloni@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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.
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.
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.
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.
On Wed, Jun 07, 2017 at 05:27:50PM +0200, Alexandre Belloni wrote: > 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; Yes, but tc.name is only in the scope of the function, so there is no need to include this field in the structure, just a working buffer in the function is enough. > > > 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. Ok. In this case you can check if the interrupt is specified for the node, if yes, then it is a clockevent.
On Wed, Jun 07, 2017 at 05:09:08PM +0200, Alexandre Belloni wrote: > 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 The hardware timer can be shared, the channels used in different subsystem. Each channel are used exclusively. What is the benefit of regmap? It has a cost, and takes a lock at each read. For instance: +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; +} Is: +static u64 tc_get_cycles(struct clocksource *cs) +{ + u32 lower, upper, tmp; + + do { + regmap_read(tc.regmap, ATMEL_TC_CV(1), &upper); lock(); lot-of-things(); unlock(); + regmap_read(tc.regmap, ATMEL_TC_CV(0), &lower); lock(); lot-of-things(); unlock(); + regmap_read(tc.regmap, ATMEL_TC_CV(1), &tmp); lock(); lot-of-things(); unlock(); + } while (upper != tmp); + + return (upper << 16) | lower; +} I suggest to look what is in 'lot-of-things()' and especially what is doing regcache_read(). May be you can reconsider the regmap? This driver is the only one use the regmap AFAICT and I don't think it is adequate. > > > > 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. Mmh, strange. Why is the clk[0]'s rate used in this case?
On 07/06/2017 at 23:38:10 +0200, Daniel Lezcano wrote: > On Wed, Jun 07, 2017 at 05:09:08PM +0200, Alexandre Belloni wrote: > I suggest to look what is in 'lot-of-things()' and especially what is doing > regcache_read(). > I know it does a lot... > May be you can reconsider the regmap? This driver is the only one use the > regmap AFAICT and I don't think it is adequate. > That is not true, I also converted the PWM driver and both a capture and qdec drivers are coming. > > > > > 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. > > Mmh, strange. Why is the clk[0]'s rate used in this case? > That's abusing the fact that is has the same rate as the clock feeding the counter.
On 07/06/2017 at 23:08:48 +0200, Daniel Lezcano wrote: > > 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. > > Ok. In this case you can check if the interrupt is specified for the node, if > yes, then it is a clockevent. > But currently it is always specified in the SoC's dtsi. I don't find that too practical to push that to the board's dts. Also, lying by omission (the IRQ is always wired) in the DT is not different from having a property selecting which timer is the clocksource and which is the clockevent.
Le Thu, 8 Jun 2017 01:17:15 +0200, Alexandre Belloni <alexandre.belloni@free-electrons.com> a écrit : > On 07/06/2017 at 23:08:48 +0200, Daniel Lezcano wrote: > > > 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. > > > > Ok. In this case you can check if the interrupt is specified for the node, if > > yes, then it is a clockevent. > > > > But currently it is always specified in the SoC's dtsi. I don't find > that too practical to push that to the board's dts. Also, lying by > omission (the IRQ is always wired) in the DT is not different from > having a property selecting which timer is the clocksource and which is > the clockevent. > I agree with Alexandre here. Really, there's not much we can do to detect which timer should be used as a clockevent and which one should be used as a clocksource except explicitly specifying it in the DT. Having an interrupt defined in one case (clockevent) and undefined in the other case (clocksource), is just as hack-ish as the detection logic Alexandre developed to avoid explicitly specifying the function assigned to a specific timer. Can we please find a solution that makes everyone happy (DT, clocksoure/clockevent and at91 maintainers)? How about adding a linux,timer-function property to specify which function this timer is providing? Something like that for example: tcb0: timer@fff7c000 { compatible = "atmel,at91rm9200-tcb", "simple-mfd", "syscon"; #address-cells = <1>; #size-cells = <0>; reg = <0xfff7c000 0x100>; interrupts = <18 4>; clocks = <&tcb0_clk>, <&clk32k>; clock-names = "t0_clk", "slow_clk"; timer@0 { compatible = "atmel,tcb-timer"; reg = <0>, <1>; linux,timer-function = "clocksource"; }; timer@2 { compatible = "atmel,tcb-timer"; reg = <2>; linux,timer-function = "clockevent"; }; }; Alternatively, we could have a property or a node in chosen describing which timer should be used: chosen { clockevent { timer = <&timer2>; }; clocksource { timer = <&timer0>; }; /* * or * * clockevent = <&timer2>; * clocksource = <&timer0>; * * but I think the clocksource/clockevent node approach * is more future proof in case we need to add extra * information like the expected resolution/precision or * anything that could be tweakable. */ }; tcb0: timer@fff7c000 { compatible = "atmel,at91rm9200-tcb", "simple-mfd", "syscon"; #address-cells = <1>; #size-cells = <0>; reg = <0xfff7c000 0x100>; interrupts = <18 4>; clocks = <&tcb0_clk>, <&clk32k>; clock-names = "t0_clk", "slow_clk"; timer0: timer@0 { compatible = "atmel,tcb-timer"; reg = <0>, <1>; }; timer2: timer@2 { compatible = "atmel,tcb-timer"; reg = <2>; }; };
Le Wed, 7 Jun 2017 23:08:48 +0200, Daniel Lezcano <daniel.lezcano@linaro.org> a écrit : > On Wed, Jun 07, 2017 at 05:27:50PM +0200, Alexandre Belloni wrote: > > 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; > > Yes, but tc.name is only in the scope of the function, so there is no need to > include this field in the structure, just a working buffer in the function is > enough. Hm, do you mean allocating the buffer dynamically and leaking the resource (not a real leak here, since we're talking about something that cannot be unregistered) or defining a 'static char name[]' variable in the function and passing this pointer to clksrc.name? Note that putting the name var on the stack won't work, because the core does not seem to duplicate the name, and uses clksrc->name after tcb_clksrc_register() has returned. Anyway, I'm not sure dynamically generating the name is really needed because we only accept a single tcb-clocksource device. We can just set tc.clksrc.name to "atmel-tcb-clocksource" and we should be good.
Le Wed, 7 Jun 2017 23:38:10 +0200, Daniel Lezcano <daniel.lezcano@linaro.org> a écrit : > On Wed, Jun 07, 2017 at 05:09:08PM +0200, Alexandre Belloni wrote: > > 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 > > The hardware timer can be shared, the channels used in different subsystem. > > Each channel are used exclusively. Yes, that's correct, but some registers are shared, and we need locking to control accesses to these registers. There's one register in particular that has to be protected against concurrent accesses: TC_BMR. But this register is only written at init time, not in the hot path. > > What is the benefit of regmap? It has a cost, and takes a lock at each read. > > For instance: > > +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; > +} > > Is: > > +static u64 tc_get_cycles(struct clocksource *cs) > +{ > + u32 lower, upper, tmp; > + > + do { > + regmap_read(tc.regmap, ATMEL_TC_CV(1), &upper); > lock(); > lot-of-things(); > unlock(); > + regmap_read(tc.regmap, ATMEL_TC_CV(0), &lower); > lock(); > lot-of-things(); > unlock(); > + regmap_read(tc.regmap, ATMEL_TC_CV(1), &tmp); > lock(); > lot-of-things(); > unlock(); > + } while (upper != tmp); > + > + return (upper << 16) | lower; > +} > > I suggest to look what is in 'lot-of-things()' and especially what is doing > regcache_read(). > > May be you can reconsider the regmap? This driver is the only one use the > regmap AFAICT and I don't think it is adequate. Alexandre, maybe we could of_ioremap() the TCB region in this driver and use regular readl to read TC_CV regs: those are attached to a specific channel, so we shouldn't have concurrency issues here.
+Mark Rutland, +Rob Herring Alexandre, Boris, have a look at https://www.spinics.net/lists/arm-kernel/msg572652.html That will tell you the story. On Thu, Jun 08, 2017 at 07:42:36AM +0200, Boris Brezillon wrote: > Le Thu, 8 Jun 2017 01:17:15 +0200, > Alexandre Belloni <alexandre.belloni@free-electrons.com> a écrit : > > > On 07/06/2017 at 23:08:48 +0200, Daniel Lezcano wrote: > > > > 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. > > > > > > Ok. In this case you can check if the interrupt is specified for the node, if > > > yes, then it is a clockevent. > > > > > > > But currently it is always specified in the SoC's dtsi. I don't find > > that too practical to push that to the board's dts. Also, lying by > > omission (the IRQ is always wired) in the DT is not different from > > having a property selecting which timer is the clocksource and which is > > the clockevent. > > > > I agree with Alexandre here. Really, there's not much we can do to > detect which timer should be used as a clockevent and which one should > be used as a clocksource except explicitly specifying it in the DT. > Having an interrupt defined in one case (clockevent) and undefined in > the other case (clocksource), is just as hack-ish as the detection logic > Alexandre developed to avoid explicitly specifying the function > assigned to a specific timer. > > Can we please find a solution that makes everyone happy (DT, > clocksoure/clockevent and at91 maintainers)? > > How about adding a linux,timer-function property to specify which > function this timer is providing? > > Something like that for example: > > tcb0: timer@fff7c000 { > compatible = "atmel,at91rm9200-tcb", "simple-mfd", "syscon"; > #address-cells = <1>; > #size-cells = <0>; > reg = <0xfff7c000 0x100>; > interrupts = <18 4>; > clocks = <&tcb0_clk>, <&clk32k>; > clock-names = "t0_clk", "slow_clk"; > > timer@0 { > compatible = "atmel,tcb-timer"; > reg = <0>, <1>; > linux,timer-function = "clocksource"; > }; > > timer@2 { > compatible = "atmel,tcb-timer"; > reg = <2>; > linux,timer-function = "clockevent"; > }; > }; > > Alternatively, we could have a property or a node in chosen describing which > timer should be used: > > chosen { > clockevent { > timer = <&timer2>; > }; > > clocksource { > timer = <&timer0>; > }; > > /* > * or > * > * clockevent = <&timer2>; > * clocksource = <&timer0>; > * > * but I think the clocksource/clockevent node approach > * is more future proof in case we need to add extra > * information like the expected resolution/precision or > * anything that could be tweakable. > */ > }; > > tcb0: timer@fff7c000 { > compatible = "atmel,at91rm9200-tcb", "simple-mfd", "syscon"; > #address-cells = <1>; > #size-cells = <0>; > reg = <0xfff7c000 0x100>; > interrupts = <18 4>; > clocks = <&tcb0_clk>, <&clk32k>; > clock-names = "t0_clk", "slow_clk"; > > timer0: timer@0 { > compatible = "atmel,tcb-timer"; > reg = <0>, <1>; > }; > > timer2: timer@2 { > compatible = "atmel,tcb-timer"; > reg = <2>; > }; > };
On 08/06/2017 at 09:44:46 +0200, Daniel Lezcano wrote: > > +Mark Rutland, +Rob Herring > > > Alexandre, Boris, have a look at https://www.spinics.net/lists/arm-kernel/msg572652.html > > That will tell you the story. > Ok, so is the solution putting the driver back in mach-at91 were we can do whatever we want like mach-omap2 is doing? > On Thu, Jun 08, 2017 at 07:42:36AM +0200, Boris Brezillon wrote: > > Le Thu, 8 Jun 2017 01:17:15 +0200, > > Alexandre Belloni <alexandre.belloni@free-electrons.com> a écrit : > > > > > On 07/06/2017 at 23:08:48 +0200, Daniel Lezcano wrote: > > > > > 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. > > > > > > > > Ok. In this case you can check if the interrupt is specified for the node, if > > > > yes, then it is a clockevent. > > > > > > > > > > But currently it is always specified in the SoC's dtsi. I don't find > > > that too practical to push that to the board's dts. Also, lying by > > > omission (the IRQ is always wired) in the DT is not different from > > > having a property selecting which timer is the clocksource and which is > > > the clockevent. > > > > > > > I agree with Alexandre here. Really, there's not much we can do to > > detect which timer should be used as a clockevent and which one should > > be used as a clocksource except explicitly specifying it in the DT. > > Having an interrupt defined in one case (clockevent) and undefined in > > the other case (clocksource), is just as hack-ish as the detection logic > > Alexandre developed to avoid explicitly specifying the function > > assigned to a specific timer. > > > > Can we please find a solution that makes everyone happy (DT, > > clocksoure/clockevent and at91 maintainers)? > > > > How about adding a linux,timer-function property to specify which > > function this timer is providing? > > > > Something like that for example: > > > > tcb0: timer@fff7c000 { > > compatible = "atmel,at91rm9200-tcb", "simple-mfd", "syscon"; > > #address-cells = <1>; > > #size-cells = <0>; > > reg = <0xfff7c000 0x100>; > > interrupts = <18 4>; > > clocks = <&tcb0_clk>, <&clk32k>; > > clock-names = "t0_clk", "slow_clk"; > > > > timer@0 { > > compatible = "atmel,tcb-timer"; > > reg = <0>, <1>; > > linux,timer-function = "clocksource"; > > }; > > > > timer@2 { > > compatible = "atmel,tcb-timer"; > > reg = <2>; > > linux,timer-function = "clockevent"; > > }; > > }; > > > > Alternatively, we could have a property or a node in chosen describing which > > timer should be used: > > > > chosen { > > clockevent { > > timer = <&timer2>; > > }; > > > > clocksource { > > timer = <&timer0>; > > }; > > > > /* > > * or > > * > > * clockevent = <&timer2>; > > * clocksource = <&timer0>; > > * > > * but I think the clocksource/clockevent node approach > > * is more future proof in case we need to add extra > > * information like the expected resolution/precision or > > * anything that could be tweakable. > > */ > > }; > > > > tcb0: timer@fff7c000 { > > compatible = "atmel,at91rm9200-tcb", "simple-mfd", "syscon"; > > #address-cells = <1>; > > #size-cells = <0>; > > reg = <0xfff7c000 0x100>; > > interrupts = <18 4>; > > clocks = <&tcb0_clk>, <&clk32k>; > > clock-names = "t0_clk", "slow_clk"; > > > > timer0: timer@0 { > > compatible = "atmel,tcb-timer"; > > reg = <0>, <1>; > > }; > > > > timer2: timer@2 { > > compatible = "atmel,tcb-timer"; > > reg = <2>; > > }; > > }; > > -- > > <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
On Thu, 8 Jun 2017 09:44:46 +0200 Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > +Mark Rutland, +Rob Herring Mark doesn't seem to be CCed. > > > Alexandre, Boris, have a look at https://www.spinics.net/lists/arm-kernel/msg572652.html > > That will tell you the story. Then we're in a deadlock situation here. I'm tired of hearing this kind of argument "DT is only supposed to describe HW, not configuration, bla bla". The truth is, we already have plenty of bindings that do not strictly describe HW. A simple example: ECC configuration on NAND devices. This is clearly a configuration thing, the NAND controller is usually able to support several kind of strength+ECC-block-size config, but we are able to overload this with the nand-ecc-xxx properties. Another example, still MTD related: MTD partitions, this is purely a software configuration, still we allow users to pass this information in the DT. You want another one? What about the linux,code and linux,input-type properties described here [1]? So please, let's not use these "this is not decribing HW" or "this is linux specific" arguments every time someone tries to encode something that can be considered a configuration detail. Let's be pragmatic. How you want to use your timer counter blocks (I'm talking about atmel TCBs) is clearly board specific. Whether you want to use the PIT for your clocksource or use one or 2 channels of a TCB at a specific resolution is again board specific. We need a solution to assign timer channels to a linux function, and I'm not convinced passing this information through the command line makes much more sense than specifying it in the DT (and it's definitely less intuitive, since you have to reference something defined in the DT from the cmdline). Now, in his review, Mark says: " To me it sounds like what we need is Linux infrastructure that allows one to register a device as having both clockevent/clocksource functionality. That way, we can choose to do something sane at boot time, and if the user really wants specific devices used in specific ways, they can request that. " Does that mean that, after adding this "HW timer" infrastructure, we would have a standard way to assign a function to a specific timer block from the DT? How is this different from what I suggest below (note the linux, prefix on my linux,timer-function property, which clearly shows that this is Linux specific)? > > On Thu, Jun 08, 2017 at 07:42:36AM +0200, Boris Brezillon wrote: > > Le Thu, 8 Jun 2017 01:17:15 +0200, > > Alexandre Belloni <alexandre.belloni@free-electrons.com> a écrit : > > > > > On 07/06/2017 at 23:08:48 +0200, Daniel Lezcano wrote: > > > > > 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. > > > > > > > > Ok. In this case you can check if the interrupt is specified for the node, if > > > > yes, then it is a clockevent. > > > > > > > > > > But currently it is always specified in the SoC's dtsi. I don't find > > > that too practical to push that to the board's dts. Also, lying by > > > omission (the IRQ is always wired) in the DT is not different from > > > having a property selecting which timer is the clocksource and which is > > > the clockevent. > > > > > > > I agree with Alexandre here. Really, there's not much we can do to > > detect which timer should be used as a clockevent and which one should > > be used as a clocksource except explicitly specifying it in the DT. > > Having an interrupt defined in one case (clockevent) and undefined in > > the other case (clocksource), is just as hack-ish as the detection logic > > Alexandre developed to avoid explicitly specifying the function > > assigned to a specific timer. > > > > Can we please find a solution that makes everyone happy (DT, > > clocksoure/clockevent and at91 maintainers)? > > > > How about adding a linux,timer-function property to specify which > > function this timer is providing? > > > > Something like that for example: > > > > tcb0: timer@fff7c000 { > > compatible = "atmel,at91rm9200-tcb", "simple-mfd", "syscon"; > > #address-cells = <1>; > > #size-cells = <0>; > > reg = <0xfff7c000 0x100>; > > interrupts = <18 4>; > > clocks = <&tcb0_clk>, <&clk32k>; > > clock-names = "t0_clk", "slow_clk"; > > > > timer@0 { > > compatible = "atmel,tcb-timer"; > > reg = <0>, <1>; > > linux,timer-function = "clocksource"; > > }; > > > > timer@2 { > > compatible = "atmel,tcb-timer"; > > reg = <2>; > > linux,timer-function = "clockevent"; > > }; > > }; > > > > Alternatively, we could have a property or a node in chosen describing which > > timer should be used: > > > > chosen { > > clockevent { > > timer = <&timer2>; > > }; > > > > clocksource { > > timer = <&timer0>; > > }; > > > > /* > > * or > > * > > * clockevent = <&timer2>; > > * clocksource = <&timer0>; > > * > > * but I think the clocksource/clockevent node approach > > * is more future proof in case we need to add extra > > * information like the expected resolution/precision or > > * anything that could be tweakable. > > */ > > }; > > > > tcb0: timer@fff7c000 { > > compatible = "atmel,at91rm9200-tcb", "simple-mfd", "syscon"; > > #address-cells = <1>; > > #size-cells = <0>; > > reg = <0xfff7c000 0x100>; > > interrupts = <18 4>; > > clocks = <&tcb0_clk>, <&clk32k>; > > clock-names = "t0_clk", "slow_clk"; > > > > timer0: timer@0 { > > compatible = "atmel,tcb-timer"; > > reg = <0>, <1>; > > }; > > > > timer2: timer@2 { > > compatible = "atmel,tcb-timer"; > > reg = <2>; > > }; > > }; > [1]http://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/input/gpio-keys.txt
On Thu, Jun 08, 2017 at 09:59:01AM +0200, Alexandre Belloni wrote: > On 08/06/2017 at 09:44:46 +0200, Daniel Lezcano wrote: > > > > +Mark Rutland, +Rob Herring > > > > > > Alexandre, Boris, have a look at https://www.spinics.net/lists/arm-kernel/msg572652.html > > > > That will tell you the story. > > > > Ok, so is the solution putting the driver back in mach-at91 were we can > do whatever we want like mach-omap2 is doing? No. And putting a driver in mach-<whatever> does not give the permission to do whatever you want. I won't tell you how OSS works, but moving code around or using another tree to circumvent a code review is just the best way to upset maintainers in general and hurt your karma. That said, I think you misunderstood my comment (or I was not clear). In the discussion given in the link above, I am in favor, somehow, to distinguish clockevent and clocksource to solve exactly what you are facing. Rob Herring told me it could be acceptable to have a property to tell if it is a clockevent or a clocksource. Mark Rutland disagreed on this. I was alone in the discussion, no consensus have been found. Now, you have a particular use case and I would like to resurrect the discussion in order to find a solution which can apply to all DT drivers. > > On Thu, Jun 08, 2017 at 07:42:36AM +0200, Boris Brezillon wrote: > > > Le Thu, 8 Jun 2017 01:17:15 +0200, > > > Alexandre Belloni <alexandre.belloni@free-electrons.com> a écrit : > > > > > > > On 07/06/2017 at 23:08:48 +0200, Daniel Lezcano wrote: > > > > > > 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. > > > > > > > > > > Ok. In this case you can check if the interrupt is specified for the node, if > > > > > yes, then it is a clockevent. > > > > > > > > > > > > > But currently it is always specified in the SoC's dtsi. I don't find > > > > that too practical to push that to the board's dts. Also, lying by > > > > omission (the IRQ is always wired) in the DT is not different from > > > > having a property selecting which timer is the clocksource and which is > > > > the clockevent. > > > > > > > > > > I agree with Alexandre here. Really, there's not much we can do to > > > detect which timer should be used as a clockevent and which one should > > > be used as a clocksource except explicitly specifying it in the DT. > > > Having an interrupt defined in one case (clockevent) and undefined in > > > the other case (clocksource), is just as hack-ish as the detection logic > > > Alexandre developed to avoid explicitly specifying the function > > > assigned to a specific timer. > > > > > > Can we please find a solution that makes everyone happy (DT, > > > clocksoure/clockevent and at91 maintainers)? > > > > > > How about adding a linux,timer-function property to specify which > > > function this timer is providing? > > > > > > Something like that for example: > > > > > > tcb0: timer@fff7c000 { > > > compatible = "atmel,at91rm9200-tcb", "simple-mfd", "syscon"; > > > #address-cells = <1>; > > > #size-cells = <0>; > > > reg = <0xfff7c000 0x100>; > > > interrupts = <18 4>; > > > clocks = <&tcb0_clk>, <&clk32k>; > > > clock-names = "t0_clk", "slow_clk"; > > > > > > timer@0 { > > > compatible = "atmel,tcb-timer"; > > > reg = <0>, <1>; > > > linux,timer-function = "clocksource"; > > > }; > > > > > > timer@2 { > > > compatible = "atmel,tcb-timer"; > > > reg = <2>; > > > linux,timer-function = "clockevent"; > > > }; > > > }; > > > > > > Alternatively, we could have a property or a node in chosen describing which > > > timer should be used: > > > > > > chosen { > > > clockevent { > > > timer = <&timer2>; > > > }; > > > > > > clocksource { > > > timer = <&timer0>; > > > }; > > > > > > /* > > > * or > > > * > > > * clockevent = <&timer2>; > > > * clocksource = <&timer0>; > > > * > > > * but I think the clocksource/clockevent node approach > > > * is more future proof in case we need to add extra > > > * information like the expected resolution/precision or > > > * anything that could be tweakable. > > > */ > > > }; > > > > > > tcb0: timer@fff7c000 { > > > compatible = "atmel,at91rm9200-tcb", "simple-mfd", "syscon"; > > > #address-cells = <1>; > > > #size-cells = <0>; > > > reg = <0xfff7c000 0x100>; > > > interrupts = <18 4>; > > > clocks = <&tcb0_clk>, <&clk32k>; > > > clock-names = "t0_clk", "slow_clk"; > > > > > > timer0: timer@0 { > > > compatible = "atmel,tcb-timer"; > > > reg = <0>, <1>; > > > }; > > > > > > timer2: timer@2 { > > > compatible = "atmel,tcb-timer"; > > > reg = <2>; > > > }; > > > }; > > > > -- > > > > <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 > > -- > Alexandre Belloni, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com
On Thu, 8 Jun 2017 10:24:17 +0200 Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > On Thu, Jun 08, 2017 at 09:59:01AM +0200, Alexandre Belloni wrote: > > On 08/06/2017 at 09:44:46 +0200, Daniel Lezcano wrote: > > > > > > +Mark Rutland, +Rob Herring > > > > > > > > > Alexandre, Boris, have a look at https://www.spinics.net/lists/arm-kernel/msg572652.html > > > > > > That will tell you the story. > > > > > > > Ok, so is the solution putting the driver back in mach-at91 were we can > > do whatever we want like mach-omap2 is doing? > > No. And putting a driver in mach-<whatever> does not give the permission to do > whatever you want. I won't tell you how OSS works, but moving code around or > using another tree to circumvent a code review is just the best way to upset > maintainers in general and hurt your karma. > > That said, I think you misunderstood my comment (or I was not clear). In the > discussion given in the link above, I am in favor, somehow, to distinguish > clockevent and clocksource to solve exactly what you are facing. > > Rob Herring told me it could be acceptable to have a property to tell if it is > a clockevent or a clocksource. > > Mark Rutland disagreed on this. > > I was alone in the discussion, no consensus have been found. Indeed, I misunderstood your point. > > Now, you have a particular use case and I would like to resurrect the > discussion in order to find a solution which can apply to all DT drivers. Ok, glad to see we're on the same page. Mark, can we re-open the discussion? Thanks, Boris
On Thu, Jun 08, 2017 at 10:13:34AM +0200, Boris Brezillon wrote: > On Thu, 8 Jun 2017 09:44:46 +0200 > Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > > +Mark Rutland, +Rob Herring > > Mark doesn't seem to be CCed. Ah, yes. Thanks for fixing this. > > > > > > Alexandre, Boris, have a look at https://www.spinics.net/lists/arm-kernel/msg572652.html > > > > That will tell you the story. > > Then we're in a deadlock situation here. I'm tired of hearing this kind > of argument "DT is only supposed to describe HW, not configuration, bla > bla". The truth is, we already have plenty of bindings that do not > strictly describe HW. > > A simple example: ECC configuration on NAND devices. This is clearly a > configuration thing, the NAND controller is usually able to support > several kind of strength+ECC-block-size config, but we are able to > overload this with the nand-ecc-xxx properties. Another example, still > MTD related: MTD partitions, this is purely a software configuration, > still we allow users to pass this information in the DT. You want > another one? What about the linux,code and linux,input-type properties > described here [1]? > > So please, let's not use these "this is not decribing HW" or "this is > linux specific" arguments every time someone tries to encode something > that can be considered a configuration detail. > > Let's be pragmatic. How you want to use your timer counter blocks (I'm > talking about atmel TCBs) is clearly board specific. Whether you want > to use the PIT for your clocksource or use one or 2 channels of a TCB > at a specific resolution is again board specific. We need a solution to > assign timer channels to a linux function, and I'm not convinced > passing this information through the command line makes much more sense > than specifying it in the DT (and it's definitely less intuitive, since > you have to reference something defined in the DT from the cmdline). > > Now, in his review, Mark says: > > " > To me it sounds like what we need is Linux infrastructure that allows > one to register a device as having both clockevent/clocksource > functionality. > > That way, we can choose to do something sane at boot time, and if the > user really wants specific devices used in specific ways, they can > request that. > " > > Does that mean that, after adding this "HW timer" infrastructure, we > would have a standard way to assign a function to a specific timer > block from the DT? How is this different from what I suggest below > (note the linux, prefix on my linux,timer-function property, which > clearly shows that this is Linux specific)? I like the 'chosen' approach with the nodes you are proposing below. Thanks for the constructive suggestion. The binding description matches perfectly what we are trying to achieve. Rob? what do you think? > > On Thu, Jun 08, 2017 at 07:42:36AM +0200, Boris Brezillon wrote: > > > Le Thu, 8 Jun 2017 01:17:15 +0200, > > > Alexandre Belloni <alexandre.belloni@free-electrons.com> a écrit : > > > > > > > On 07/06/2017 at 23:08:48 +0200, Daniel Lezcano wrote: > > > > > > 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. > > > > > > > > > > Ok. In this case you can check if the interrupt is specified for the node, if > > > > > yes, then it is a clockevent. > > > > > > > > > > > > > But currently it is always specified in the SoC's dtsi. I don't find > > > > that too practical to push that to the board's dts. Also, lying by > > > > omission (the IRQ is always wired) in the DT is not different from > > > > having a property selecting which timer is the clocksource and which is > > > > the clockevent. > > > > > > > > > > I agree with Alexandre here. Really, there's not much we can do to > > > detect which timer should be used as a clockevent and which one should > > > be used as a clocksource except explicitly specifying it in the DT. > > > Having an interrupt defined in one case (clockevent) and undefined in > > > the other case (clocksource), is just as hack-ish as the detection logic > > > Alexandre developed to avoid explicitly specifying the function > > > assigned to a specific timer. > > > > > > Can we please find a solution that makes everyone happy (DT, > > > clocksoure/clockevent and at91 maintainers)? > > > > > > How about adding a linux,timer-function property to specify which > > > function this timer is providing? > > > > > > Something like that for example: > > > > > > tcb0: timer@fff7c000 { > > > compatible = "atmel,at91rm9200-tcb", "simple-mfd", "syscon"; > > > #address-cells = <1>; > > > #size-cells = <0>; > > > reg = <0xfff7c000 0x100>; > > > interrupts = <18 4>; > > > clocks = <&tcb0_clk>, <&clk32k>; > > > clock-names = "t0_clk", "slow_clk"; > > > > > > timer@0 { > > > compatible = "atmel,tcb-timer"; > > > reg = <0>, <1>; > > > linux,timer-function = "clocksource"; > > > }; > > > > > > timer@2 { > > > compatible = "atmel,tcb-timer"; > > > reg = <2>; > > > linux,timer-function = "clockevent"; > > > }; > > > }; > > > > > > Alternatively, we could have a property or a node in chosen describing which > > > timer should be used: > > > > > > chosen { > > > clockevent { > > > timer = <&timer2>; > > > }; > > > > > > clocksource { > > > timer = <&timer0>; > > > }; > > > > > > /* > > > * or > > > * > > > * clockevent = <&timer2>; > > > * clocksource = <&timer0>; > > > * > > > * but I think the clocksource/clockevent node approach > > > * is more future proof in case we need to add extra > > > * information like the expected resolution/precision or > > > * anything that could be tweakable. > > > */ > > > }; > > > > > > tcb0: timer@fff7c000 { > > > compatible = "atmel,at91rm9200-tcb", "simple-mfd", "syscon"; > > > #address-cells = <1>; > > > #size-cells = <0>; > > > reg = <0xfff7c000 0x100>; > > > interrupts = <18 4>; > > > clocks = <&tcb0_clk>, <&clk32k>; > > > clock-names = "t0_clk", "slow_clk"; > > > > > > timer0: timer@0 { > > > compatible = "atmel,tcb-timer"; > > > reg = <0>, <1>; > > > }; > > > > > > timer2: timer@2 { > > > compatible = "atmel,tcb-timer"; > > > reg = <2>; > > > }; > > > }; > > > > [1]http://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/input/gpio-keys.txt
On 08/06/2017 at 10:24:17 +0200, Daniel Lezcano wrote: > On Thu, Jun 08, 2017 at 09:59:01AM +0200, Alexandre Belloni wrote: > > On 08/06/2017 at 09:44:46 +0200, Daniel Lezcano wrote: > > > > > > +Mark Rutland, +Rob Herring > > > > > > > > > Alexandre, Boris, have a look at https://www.spinics.net/lists/arm-kernel/msg572652.html > > > > > > That will tell you the story. > > > > > > > Ok, so is the solution putting the driver back in mach-at91 were we can > > do whatever we want like mach-omap2 is doing? > > No. And putting a driver in mach-<whatever> does not give the permission to do > whatever you want. I won't tell you how OSS works, but moving code around or > using another tree to circumvent a code review is just the best way to upset > maintainers in general and hurt your karma. > I know that but some SoCs will not be able to even boot until we have a solution. And it has been one year since we raised the issue without any solution coming from the DT maintainers. > That said, I think you misunderstood my comment (or I was not clear). In the > discussion given in the link above, I am in favor, somehow, to distinguish > clockevent and clocksource to solve exactly what you are facing. > > Rob Herring told me it could be acceptable to have a property to tell if it is > a clockevent or a clocksource. > Ok, then let's do it! > Mark Rutland disagreed on this. > > I was alone in the discussion, no consensus have been found. > > Now, you have a particular use case and I would like to resurrect the > discussion in order to find a solution which can apply to all DT drivers. > Again, it has been one year and it seems nobody actually cares. There is a whole family of SoCs that can't boot because of that. Should I resort to the evil vendor tree argument again? > > > On Thu, Jun 08, 2017 at 07:42:36AM +0200, Boris Brezillon wrote: > > > > Le Thu, 8 Jun 2017 01:17:15 +0200, > > > > Alexandre Belloni <alexandre.belloni@free-electrons.com> a écrit : > > > > > > > > > On 07/06/2017 at 23:08:48 +0200, Daniel Lezcano wrote: > > > > > > > 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. > > > > > > > > > > > > Ok. In this case you can check if the interrupt is specified for the node, if > > > > > > yes, then it is a clockevent. > > > > > > > > > > > > > > > > But currently it is always specified in the SoC's dtsi. I don't find > > > > > that too practical to push that to the board's dts. Also, lying by > > > > > omission (the IRQ is always wired) in the DT is not different from > > > > > having a property selecting which timer is the clocksource and which is > > > > > the clockevent. > > > > > > > > > > > > > I agree with Alexandre here. Really, there's not much we can do to > > > > detect which timer should be used as a clockevent and which one should > > > > be used as a clocksource except explicitly specifying it in the DT. > > > > Having an interrupt defined in one case (clockevent) and undefined in > > > > the other case (clocksource), is just as hack-ish as the detection logic > > > > Alexandre developed to avoid explicitly specifying the function > > > > assigned to a specific timer. > > > > > > > > Can we please find a solution that makes everyone happy (DT, > > > > clocksoure/clockevent and at91 maintainers)? > > > > > > > > How about adding a linux,timer-function property to specify which > > > > function this timer is providing? > > > > > > > > Something like that for example: > > > > > > > > tcb0: timer@fff7c000 { > > > > compatible = "atmel,at91rm9200-tcb", "simple-mfd", "syscon"; > > > > #address-cells = <1>; > > > > #size-cells = <0>; > > > > reg = <0xfff7c000 0x100>; > > > > interrupts = <18 4>; > > > > clocks = <&tcb0_clk>, <&clk32k>; > > > > clock-names = "t0_clk", "slow_clk"; > > > > > > > > timer@0 { > > > > compatible = "atmel,tcb-timer"; > > > > reg = <0>, <1>; > > > > linux,timer-function = "clocksource"; > > > > }; > > > > > > > > timer@2 { > > > > compatible = "atmel,tcb-timer"; > > > > reg = <2>; > > > > linux,timer-function = "clockevent"; > > > > }; > > > > }; > > > > > > > > Alternatively, we could have a property or a node in chosen describing which > > > > timer should be used: > > > > > > > > chosen { > > > > clockevent { > > > > timer = <&timer2>; > > > > }; > > > > > > > > clocksource { > > > > timer = <&timer0>; > > > > }; > > > > > > > > /* > > > > * or > > > > * > > > > * clockevent = <&timer2>; > > > > * clocksource = <&timer0>; > > > > * > > > > * but I think the clocksource/clockevent node approach > > > > * is more future proof in case we need to add extra > > > > * information like the expected resolution/precision or > > > > * anything that could be tweakable. > > > > */ > > > > }; > > > > > > > > tcb0: timer@fff7c000 { > > > > compatible = "atmel,at91rm9200-tcb", "simple-mfd", "syscon"; > > > > #address-cells = <1>; > > > > #size-cells = <0>; > > > > reg = <0xfff7c000 0x100>; > > > > interrupts = <18 4>; > > > > clocks = <&tcb0_clk>, <&clk32k>; > > > > clock-names = "t0_clk", "slow_clk"; > > > > > > > > timer0: timer@0 { > > > > compatible = "atmel,tcb-timer"; > > > > reg = <0>, <1>; > > > > }; > > > > > > > > timer2: timer@2 { > > > > compatible = "atmel,tcb-timer"; > > > > reg = <2>; > > > > }; > > > > }; > > > > > > -- > > > > > > <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 > > > > -- > > Alexandre Belloni, Free Electrons > > Embedded Linux and Kernel engineering > > http://free-electrons.com > > -- > > <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
On Thu, 8 Jun 2017 10:40:26 +0200 Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > > > > > Alexandre, Boris, have a look at https://www.spinics.net/lists/arm-kernel/msg572652.html > > > > > > That will tell you the story. > > > > Then we're in a deadlock situation here. I'm tired of hearing this kind > > of argument "DT is only supposed to describe HW, not configuration, bla > > bla". The truth is, we already have plenty of bindings that do not > > strictly describe HW. > > > > A simple example: ECC configuration on NAND devices. This is clearly a > > configuration thing, the NAND controller is usually able to support > > several kind of strength+ECC-block-size config, but we are able to > > overload this with the nand-ecc-xxx properties. Another example, still > > MTD related: MTD partitions, this is purely a software configuration, > > still we allow users to pass this information in the DT. You want > > another one? What about the linux,code and linux,input-type properties > > described here [1]? > > > > So please, let's not use these "this is not decribing HW" or "this is > > linux specific" arguments every time someone tries to encode something > > that can be considered a configuration detail. > > > > Let's be pragmatic. How you want to use your timer counter blocks (I'm > > talking about atmel TCBs) is clearly board specific. Whether you want > > to use the PIT for your clocksource or use one or 2 channels of a TCB > > at a specific resolution is again board specific. We need a solution to > > assign timer channels to a linux function, and I'm not convinced > > passing this information through the command line makes much more sense > > than specifying it in the DT (and it's definitely less intuitive, since > > you have to reference something defined in the DT from the cmdline). > > > > Now, in his review, Mark says: > > > > " > > To me it sounds like what we need is Linux infrastructure that allows > > one to register a device as having both clockevent/clocksource > > functionality. > > > > That way, we can choose to do something sane at boot time, and if the > > user really wants specific devices used in specific ways, they can > > request that. > > " > > > > Does that mean that, after adding this "HW timer" infrastructure, we > > would have a standard way to assign a function to a specific timer > > block from the DT? How is this different from what I suggest below > > (note the linux, prefix on my linux,timer-function property, which > > clearly shows that this is Linux specific)? > > I like the 'chosen' approach with the nodes you are proposing below. Thanks for > the constructive suggestion. The binding description matches perfectly what we > are trying to achieve. Actually, this is Alexandre who initially suggested the chosen approach (I thought it was important to mention that ;-)).
Le 08/06/2017 à 10:40, Daniel Lezcano a écrit : > On Thu, Jun 08, 2017 at 10:13:34AM +0200, Boris Brezillon wrote: >> On Thu, 8 Jun 2017 09:44:46 +0200 >> Daniel Lezcano <daniel.lezcano@linaro.org> wrote: >> >>> +Mark Rutland, +Rob Herring >> >> Mark doesn't seem to be CCed. > > Ah, yes. Thanks for fixing this. > >>> >>> >>> Alexandre, Boris, have a look at https://www.spinics.net/lists/arm-kernel/msg572652.html >>> >>> That will tell you the story. >> >> Then we're in a deadlock situation here. I'm tired of hearing this kind >> of argument "DT is only supposed to describe HW, not configuration, bla >> bla". The truth is, we already have plenty of bindings that do not >> strictly describe HW. >> >> A simple example: ECC configuration on NAND devices. This is clearly a >> configuration thing, the NAND controller is usually able to support >> several kind of strength+ECC-block-size config, but we are able to >> overload this with the nand-ecc-xxx properties. Another example, still >> MTD related: MTD partitions, this is purely a software configuration, >> still we allow users to pass this information in the DT. You want >> another one? What about the linux,code and linux,input-type properties >> described here [1]? >> >> So please, let's not use these "this is not decribing HW" or "this is >> linux specific" arguments every time someone tries to encode something >> that can be considered a configuration detail. >> >> Let's be pragmatic. How you want to use your timer counter blocks (I'm >> talking about atmel TCBs) is clearly board specific. Whether you want >> to use the PIT for your clocksource or use one or 2 channels of a TCB >> at a specific resolution is again board specific. We need a solution to >> assign timer channels to a linux function, and I'm not convinced >> passing this information through the command line makes much more sense >> than specifying it in the DT (and it's definitely less intuitive, since >> you have to reference something defined in the DT from the cmdline). >> >> Now, in his review, Mark says: >> >> " >> To me it sounds like what we need is Linux infrastructure that allows >> one to register a device as having both clockevent/clocksource >> functionality. >> >> That way, we can choose to do something sane at boot time, and if the >> user really wants specific devices used in specific ways, they can >> request that. >> " >> >> Does that mean that, after adding this "HW timer" infrastructure, we >> would have a standard way to assign a function to a specific timer >> block from the DT? How is this different from what I suggest below >> (note the linux, prefix on my linux,timer-function property, which >> clearly shows that this is Linux specific)? > > I like the 'chosen' approach with the nodes you are proposing below. Thanks for > the constructive suggestion. The binding description matches perfectly what we > are trying to achieve. > > Rob? what do you think? I'm following this work from a distance but as we've just celebrated the 1st anniversary for this patch series (11 June 2016), I propose that we now make up our mind quickly. Everybody seem to be on the same page and willing to make this rework move forward. In Microchip/Atmel we would like to actually use this TCB rework both internally and in our mainline work to avoid having to rely on our own out-of-tree implementation. The newly-added samv7 cortex-M can't boot without this series and a use of our current cortex-A SoCs with TrustZone in Secure World (SWd) is not possible with current mainline code only. On these two examples, the current timer on which we rely, the PIT, is not present. So you probably understand that more than one year without real progress begins to be a little bit frustrating for the AT91 users... Regards, >>> On Thu, Jun 08, 2017 at 07:42:36AM +0200, Boris Brezillon wrote: >>>> Le Thu, 8 Jun 2017 01:17:15 +0200, >>>> Alexandre Belloni <alexandre.belloni@free-electrons.com> a écrit : >>>> >>>>> On 07/06/2017 at 23:08:48 +0200, Daniel Lezcano wrote: >>>>>>> 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. >>>>>> >>>>>> Ok. In this case you can check if the interrupt is specified for the node, if >>>>>> yes, then it is a clockevent. >>>>>> >>>>> >>>>> But currently it is always specified in the SoC's dtsi. I don't find >>>>> that too practical to push that to the board's dts. Also, lying by >>>>> omission (the IRQ is always wired) in the DT is not different from >>>>> having a property selecting which timer is the clocksource and which is >>>>> the clockevent. >>>>> >>>> >>>> I agree with Alexandre here. Really, there's not much we can do to >>>> detect which timer should be used as a clockevent and which one should >>>> be used as a clocksource except explicitly specifying it in the DT. >>>> Having an interrupt defined in one case (clockevent) and undefined in >>>> the other case (clocksource), is just as hack-ish as the detection logic >>>> Alexandre developed to avoid explicitly specifying the function >>>> assigned to a specific timer. >>>> >>>> Can we please find a solution that makes everyone happy (DT, >>>> clocksoure/clockevent and at91 maintainers)? >>>> >>>> How about adding a linux,timer-function property to specify which >>>> function this timer is providing? >>>> >>>> Something like that for example: >>>> >>>> tcb0: timer@fff7c000 { >>>> compatible = "atmel,at91rm9200-tcb", "simple-mfd", "syscon"; >>>> #address-cells = <1>; >>>> #size-cells = <0>; >>>> reg = <0xfff7c000 0x100>; >>>> interrupts = <18 4>; >>>> clocks = <&tcb0_clk>, <&clk32k>; >>>> clock-names = "t0_clk", "slow_clk"; >>>> >>>> timer@0 { >>>> compatible = "atmel,tcb-timer"; >>>> reg = <0>, <1>; >>>> linux,timer-function = "clocksource"; >>>> }; >>>> >>>> timer@2 { >>>> compatible = "atmel,tcb-timer"; >>>> reg = <2>; >>>> linux,timer-function = "clockevent"; >>>> }; >>>> }; >>>> >>>> Alternatively, we could have a property or a node in chosen describing which >>>> timer should be used: >>>> >>>> chosen { >>>> clockevent { >>>> timer = <&timer2>; >>>> }; >>>> >>>> clocksource { >>>> timer = <&timer0>; >>>> }; >>>> >>>> /* >>>> * or >>>> * >>>> * clockevent = <&timer2>; >>>> * clocksource = <&timer0>; >>>> * >>>> * but I think the clocksource/clockevent node approach >>>> * is more future proof in case we need to add extra >>>> * information like the expected resolution/precision or >>>> * anything that could be tweakable. >>>> */ >>>> }; >>>> >>>> tcb0: timer@fff7c000 { >>>> compatible = "atmel,at91rm9200-tcb", "simple-mfd", "syscon"; >>>> #address-cells = <1>; >>>> #size-cells = <0>; >>>> reg = <0xfff7c000 0x100>; >>>> interrupts = <18 4>; >>>> clocks = <&tcb0_clk>, <&clk32k>; >>>> clock-names = "t0_clk", "slow_clk"; >>>> >>>> timer0: timer@0 { >>>> compatible = "atmel,tcb-timer"; >>>> reg = <0>, <1>; >>>> }; >>>> >>>> timer2: timer@2 { >>>> compatible = "atmel,tcb-timer"; >>>> reg = <2>; >>>> }; >>>> }; >>> >> >> [1]http://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/input/gpio-keys.txt >
On 12/06/2017 14:54, Nicolas Ferre wrote: [ ... ] >> I like the 'chosen' approach with the nodes you are proposing below. Thanks for >> the constructive suggestion. The binding description matches perfectly what we >> are trying to achieve. >> >> Rob? what do you think? > > I'm following this work from a distance but as we've just celebrated the > 1st anniversary for this patch series (11 June 2016), I propose that we > now make up our mind quickly. Everybody seem to be on the same page and > willing to make this rework move forward. > > In Microchip/Atmel we would like to actually use this TCB rework both > internally and in our mainline work to avoid having to rely on our own > out-of-tree implementation. > > The newly-added samv7 cortex-M can't boot without this series and a use > of our current cortex-A SoCs with TrustZone in Secure World (SWd) is not > possible with current mainline code only. On these two examples, the > current timer on which we rely, the PIT, is not present. > > So you probably understand that more than one year without real progress > begins to be a little bit frustrating for the AT91 users... Nicolas, who are you exactly blaming? Are you surprised a 58 patches series, with a gazillion of Cc'ed people posted awhile ago, is ignored?
Le 12/06/2017 à 15:25, Daniel Lezcano a écrit : > On 12/06/2017 14:54, Nicolas Ferre wrote: > > [ ... ] > >>> I like the 'chosen' approach with the nodes you are proposing below. Thanks for >>> the constructive suggestion. The binding description matches perfectly what we >>> are trying to achieve. >>> >>> Rob? what do you think? >> >> I'm following this work from a distance but as we've just celebrated the >> 1st anniversary for this patch series (11 June 2016), I propose that we >> now make up our mind quickly. Everybody seem to be on the same page and >> willing to make this rework move forward. >> >> In Microchip/Atmel we would like to actually use this TCB rework both >> internally and in our mainline work to avoid having to rely on our own >> out-of-tree implementation. >> >> The newly-added samv7 cortex-M can't boot without this series and a use >> of our current cortex-A SoCs with TrustZone in Secure World (SWd) is not >> possible with current mainline code only. On these two examples, the >> current timer on which we rely, the PIT, is not present. >> >> So you probably understand that more than one year without real progress >> begins to be a little bit frustrating for the AT91 users... > > Nicolas, > > who are you exactly blaming? > > Are you surprised a 58 patches series, with a gazillion of Cc'ed people > posted awhile ago, is ignored? Daniel, Well, I'm talking about the only patch about DT bindings here, not the other (large number) of soc/board dts changes. Note that Alexandre tried to extract this discussion from the other patches without coming to a conclusion either ("[PATCH v3] ARM: at91: Document new TCB bindings", for instance). I had the feeling that this email thread was about to be fading out on a DT binding discussion, as it had done a couple of times during the last months... Both you and me produced "ping" messages to make this discussion progress with no real success so far... This is why I was a little worried. Regards,
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);
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