From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: Alexandre Belloni <alexandre.belloni@free-electrons.com>,
Nicolas Ferre <nicolas.ferre@atmel.com>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>,
Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org,
Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH 42/48] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks
Date: Fri, 24 Jun 2016 12:07:01 +0200 [thread overview]
Message-ID: <576D0645.2070805@linaro.org> (raw)
In-Reply-To: <1465596231-21766-43-git-send-email-alexandre.belloni@free-electrons.com>
On 06/11/2016 12:03 AM, 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.
>
> 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 | 305 ++++++++++++++++++++++++++++
> include/soc/at91/atmel_tcb.h | 220 ++++++++++++++++++++
> 4 files changed, 540 insertions(+), 1 deletion(-)
> create mode 100644 drivers/clocksource/timer-atmel-tcbclksrc.c
> create mode 100644 include/soc/at91/atmel_tcb.h
>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 47352d25c15e..ff7f4022c749 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -258,6 +258,19 @@ config ATMEL_ST
> select CLKSRC_OF
> select MFD_SYSCON
>
> +config ATMEL_ARM_TCB_CLKSRC
> + bool "TC Block Clocksource"
The Kconfig options are set now with the COMPILE_TEST option in order to
increase the compilation test coverage.
Please, add bool "TC Block Clocksource" if COMPILE_TEST, ...
> + select REGMAP_MMIO
> + depends on GENERIC_CLOCKEVENTS
> + depends on SOC_AT91RM9200 || SOC_AT91SAM9 || SOC_SAMA5
> + default SOC_AT91RM9200 || SOC_AT91SAM9 || SOC_SAMA5
... remove these dependencies and let the SoC's Kconfig to select the
timer like the other timers are.
> + 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 473974f9590a..988f33de5808 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -1,7 +1,8 @@
> obj-$(CONFIG_CLKSRC_PROBE) += clksrc-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..af0b1aab7a98
> --- /dev/null
> +++ b/drivers/clocksource/timer-atmel-tcbclksrc.c
> @@ -0,0 +1,305 @@
> +#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>
> +
> +struct atmel_tcb_clksrc {
> + struct clocksource clksrc;
> + struct clock_event_device clkevt;
> + struct regmap *regmap;
> + struct clk *clk[2];
> + int channels[2];
> + u8 bits;
> + unsigned int irq;
The test in the init function checks against < 0 but 'irq' is 'unsigned'
> + bool registered;
> + bool irq_requested;
> +};
> +
> +static struct atmel_tcb_clksrc tc = {
> + .clksrc = {
> + .name = "tcb_clksrc",
> + .rating = 200,
> + .mask = CLOCKSOURCE_MASK(32),
> + .flags = CLOCK_SOURCE_IS_CONTINUOUS,
> + },
> + .clkevt = {
> + .name = "tcb_clkevt",
> + .features = CLOCK_EVT_FEAT_ONESHOT,
> + /* Should be lower than at91rm9200's system timer */
> + .rating = 125,
> + },
> +};
> +
> +static cycle_t tc_get_cycles(struct clocksource *cs)
> +{
> + unsigned long flags;
> + u32 lower, upper, tmp;
> +
> + raw_local_irq_save(flags);
> + 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);
> +
> + raw_local_irq_restore(flags);
Why is this lock needed ?
> + return (upper << 16) | lower;
> +}
> +
> +static cycle_t tc_get_cycles32(struct clocksource *cs)
> +{
> + u32 val;
> +
> + regmap_read(tc.regmap, ATMEL_TC_CV(tc.channels[0]), &val);
> +
> + return val;
> +}
> +
> +static u64 tc_sched_clock_read(void)
> +{
> + return tc_get_cycles(&tc.clksrc);
> +}
> +
> +static u64 tc_sched_clock_read32(void)
> +{
> + return tc_get_cycles32(&tc.clksrc);
> +}
'notrace' should be added here to avoid recursion with ftrace.
> +static int tcb_clkevt_next_event(unsigned long delta,
> + struct clock_event_device *d)
> +{
> + u32 val;
> +
> + regmap_read(tc.regmap, ATMEL_TC_CV(tc.channels[0]), &val);
> + regmap_write(tc.regmap, ATMEL_TC_RC(tc.channels[0]), val + delta);
> + regmap_write(tc.regmap, ATMEL_TC_IER(tc.channels[0]), ATMEL_TC_CPCS);
> +
> + return 0;
> +}
> +
> +static irqreturn_t tc_clkevt_irq(int irq, void *handle)
> +{
> + unsigned int sr;
> +
> + regmap_read(tc.regmap, ATMEL_TC_SR(tc.channels[0]), &sr);
> + if (sr & ATMEL_TC_CPCS) {
> + tc.clkevt.event_handler(&tc.clkevt);
> + return IRQ_HANDLED;
> + }
> +
> + return IRQ_NONE;
> +}
> +
> +static int tcb_clkevt_oneshot(struct clock_event_device *dev)
> +{
> + int ret;
> +
> + if (tc.irq_requested)
> + return 0;
> +
> + ret = request_irq(tc.irq, tc_clkevt_irq, IRQF_TIMER | IRQF_SHARED,
> + "tcb_clkevt", &tc);
> + if (!ret)
> + tc.irq_requested = true;
The legacy driver checks clockevent_state_detached() and disables the clock.
Why is 'irq_requested' and request_irq/free_irq cleaner ?
Isn't there a configuration with the TCB register to disable the
clockevent only ?
> + return ret;
> +}
> +
> +static int tcb_clkevt_shutdown(struct clock_event_device *dev)
> +{
> + regmap_write(tc.regmap, ATMEL_TC_IDR(tc.channels[0]), 0xff);
> + if (tc.bits == 16)
> + regmap_write(tc.regmap, ATMEL_TC_IDR(tc.channels[1]), 0xff);
> +
> + if (tc.irq_requested) {
> + free_irq(tc.irq, &tc);
> + tc.irq_requested = false;
> + }
> +
> + return 0;
> +}
[ ... ]
> +static void __init tcb_clksrc_init(struct device_node *node)
> +{
> + const struct of_device_id *match;
> + u32 rate, divided_rate = 0;
> + int best_divisor_idx = -1;
> + int i, err;
> +
> + if (tc.registered)
> + return;
That's getting annoying. It is not your fault but that's a repeating
pattern when multiple nodes are defined for a timer. Someday we will
have to sit down and think about that.
> + tc.regmap = syscon_node_to_regmap(node->parent);
> + if (IS_ERR(tc.regmap))
> + return;
> +
> + match = of_match_node(atmel_tcb_dt_ids, node->parent);
> + tc.bits = (int)match->data;
> +
> + err = of_property_read_u32_index(node, "reg", 0, &tc.channels[0]);
> + if (err)
> + return;
> +
> + tc.channels[1] = -1;
> +
> + if (tc.bits == 16) {
> + of_property_read_u32_index(node, "reg", 1, &tc.channels[1]);
> + if (tc.channels[1] == -1) {
> + pr_err("%s: clocksource needs two channels\n",
> + node->parent->full_name);
> + }
> + }
> +
> + tc.irq = tcb_irq_get(node, tc.channels[0]);
> + if (tc.irq < 0)
> + return;
> +
> + tc.clk[0] = tcb_clk_get(node, tc.channels[0]);
> + if (IS_ERR(tc.clk[0]))
> + return;
> + err = clk_prepare_enable(tc.clk[0]);
> + if (err) {
> + pr_debug("can't enable T0 clk\n");
> + goto err_clk;
> + }
> +
> + if (tc.bits == 16) {
> + tc.clk[1] = tcb_clk_get(node, tc.channels[1]);
> + if (IS_ERR(tc.clk[1]))
> + goto err_disable_t0;
> + }
> +
> + /* 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;
> + }
> +
> + pr_debug("%s: %s at %d.%03d MHz\n", tc.clksrc.name,
> + node->parent->full_name, divided_rate / 1000000,
> + ((divided_rate + 500000) % 1000000) / 1000);
> +
> + if (tc.bits == 32) {
> + tc.clksrc.read = tc_get_cycles32;
> + tcb_setup_single_chan(&tc, best_divisor_idx);
> + } else {
> + 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);
> + }
> +
> + err = clocksource_register_hz(&tc.clksrc, divided_rate);
> + if (err)
> + goto err_disable_t1;
> +
> + if (tc.bits == 32)
Can't you can group the code under tc.bits == 16 and tc.bits == 32
instead of multiple check against the reg width ?
> + sched_clock_register(tc_sched_clock_read32, 32, divided_rate);
> + else
> + sched_clock_register(tc_sched_clock_read, 32, divided_rate);
> +
> + tc.registered = true;
> +
> + /* Set up and register clockevents */
> + tc.clkevt.cpumask = cpumask_of(0);
> + tc.clkevt.set_next_event = tcb_clkevt_next_event;
> + tc.clkevt.set_state_oneshot = tcb_clkevt_oneshot;
> + tc.clkevt.set_state_shutdown = tcb_clkevt_shutdown;
> + if (tc.bits == 16)
> + clockevents_config_and_register(&tc.clkevt, divided_rate, 1,
> + 0xffff);
> + else
> + clockevents_config_and_register(&tc.clkevt, divided_rate, 1,
> + 0xffffffff);
> + return;
clockevents_config_and_register(&tc.clkevt, divided_rate, 1,
BIT(tc.bits) - 1);
IIRC, there is a macro somewhere doing BIT(n) - 1. CLOCKSOURCE_MASK does
it but it is weird to set a clockevent with a clocksource mask.
> +
> +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);
> +}
> +CLOCKSOURCE_OF_DECLARE(atmel_tcb_clksrc, "atmel,tcb-clksrc",
> + tcb_clksrc_init);
[ ... ]
--
<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
next prev parent reply other threads:[~2016-06-24 10:07 UTC|newest]
Thread overview: 75+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-10 22:03 [PATCH 00/48] ARM: at91: rework Atmel TCB drivers Alexandre Belloni
2016-06-10 22:03 ` [PATCH 01/48] clk: at91: replace usleep() by udelay() calls Alexandre Belloni
2016-06-10 22:30 ` Arnd Bergmann
2016-06-10 22:37 ` Alexandre Belloni
2016-06-13 15:24 ` Alexandre Belloni
2016-06-13 19:26 ` Arnd Bergmann
2016-06-14 16:05 ` Afzal Mohammed
2016-06-14 16:18 ` Boris Brezillon
2016-06-11 7:49 ` Boris Brezillon
2016-06-10 22:03 ` [PATCH 02/48] ARM: at91: Document new TCB bindings Alexandre Belloni
2016-06-14 21:47 ` Rob Herring
2016-06-15 7:29 ` Boris Brezillon
2016-06-21 20:08 ` Rob Herring
2016-06-21 20:28 ` Rob Herring
2016-06-10 22:03 ` [PATCH 03/48] ARM: dts: at91: at91rm9200: TC blocks are also simple-mfd and syscon devices Alexandre Belloni
2016-06-10 22:03 ` [PATCH 04/48] ARM: dts: at91: at91rm9200ek; use TCB0 as clocksource Alexandre Belloni
2016-06-10 22:03 ` [PATCH 05/48] ARM: dts: at91: mpa1600; " Alexandre Belloni
2016-06-10 22:03 ` [PATCH 06/48] ARM: dts: at91: at91sam9260: TC blocks are also simple-mfd and syscon devices Alexandre Belloni
2016-06-10 22:03 ` [PATCH 07/48] ARM: dts: at91: ethernut5: use TCB0 as clocksource Alexandre Belloni
2016-06-10 22:03 ` [PATCH 08/48] ARM: dts: at91: foxg20: " Alexandre Belloni
2016-06-10 22:03 ` [PATCH 09/48] ARM: dts: at91: animeo_ip: " Alexandre Belloni
2016-06-10 22:03 ` [PATCH 10/48] ARM: dts: at91: kizbox: " Alexandre Belloni
2016-06-10 22:03 ` [PATCH 11/48] ARM: dts: at91: at91sam9g20ek: " Alexandre Belloni
2016-06-10 22:03 ` [PATCH 12/48] ARM: dts: at91: ge863-pro3: " Alexandre Belloni
2016-06-10 22:03 ` [PATCH 13/48] ARM: dts: at91: at91sam9261: TC blocks are also simple-mfd and syscon devices Alexandre Belloni
2016-06-10 22:03 ` [PATCH 14/48] ARM: dts: at91: at91sam9261ek: use TCB0 as clocksource Alexandre Belloni
2016-06-10 22:03 ` [PATCH 15/48] ARM: dts: at91: at91sam9263: TC blocks are also simple-mfd and syscon devices Alexandre Belloni
2016-06-10 22:03 ` [PATCH 16/48] ARM: dts: at91: at91sam9263ek: use TCB0 as clocksource Alexandre Belloni
2016-06-10 22:03 ` [PATCH 17/48] ARM: dts: at91: calao: " Alexandre Belloni
2016-06-10 22:03 ` [PATCH 18/48] ARM: dts: at91: at91sam9g45: TC blocks are also simple-mfd and syscon devices Alexandre Belloni
2016-06-10 22:03 ` [PATCH 19/48] ARM: dts: at91: at91sam9m10g45ek: use TCB0 as clocksource Alexandre Belloni
2016-06-10 22:03 ` [PATCH 20/48] ARM: dts: at91: pm9g45: " Alexandre Belloni
2016-06-10 22:03 ` [PATCH 21/48] ARM: dts: at91: at91sam9rl: TC blocks are also simple-mfd and syscon devices Alexandre Belloni
2016-06-10 22:03 ` [PATCH 22/48] ARM: dts: at91: at91sam9rlek: use TCB0 as clocksource Alexandre Belloni
2016-06-10 22:03 ` [PATCH 23/48] ARM: dts: at91: at91sam9n12: TC blocks are also simple-mfd and syscon devices Alexandre Belloni
2016-06-10 22:03 ` [PATCH 24/48] ARM: dts: at91: at91sam9n12ek: use TCB0 as clocksource Alexandre Belloni
2016-06-10 22:03 ` [PATCH 25/48] ARM: dts: at91: at91sam9x5: TC blocks are also simple-mfd and syscon devices Alexandre Belloni
2016-06-10 22:03 ` [PATCH 26/48] ARM: dts: at91: at91sam9x5cm: use TCB0 as clocksource Alexandre Belloni
2016-06-10 22:03 ` [PATCH 27/48] ARM: dts: at91: acme/g25: " Alexandre Belloni
2016-06-10 22:03 ` [PATCH 28/48] ARM: dts: at91: cosino: " Alexandre Belloni
2016-06-10 22:03 ` [PATCH 29/48] ARM: dts: at91: kizboxmini: " Alexandre Belloni
2016-06-10 22:03 ` [PATCH 30/48] ARM: dts: at91: sama5d3: TC blocks are also simple-mfd and syscon devices Alexandre Belloni
2016-06-10 22:03 ` [PATCH 31/48] ARM: dts: at91: sama5d3xek; use TCB0 as clocksource Alexandre Belloni
2016-06-10 22:03 ` [PATCH 32/48] ARM: dts: at91: sama5d3 Xplained: " Alexandre Belloni
2016-06-10 22:03 ` [PATCH 33/48] ARM: dts: at91: kizbox2: " Alexandre Belloni
2016-06-10 22:03 ` [PATCH 34/48] ARM: dts: at91: sama5d4: TC blocks are also simple-mfd and syscon devices Alexandre Belloni
2016-06-10 22:03 ` [PATCH 35/48] ARM: dts: at91: sama5d4: Add TCB2 Alexandre Belloni
2016-06-10 22:03 ` [PATCH 36/48] ARM: dts: at91: sama5d4ek: use TCB2 as clocksource Alexandre Belloni
2016-06-10 22:03 ` [PATCH 37/48] ARM: dts: at91: sama5d4 Xplained: " Alexandre Belloni
2016-06-10 22:03 ` [PATCH 38/48] ARM: dts: at91: ma5d4: " Alexandre Belloni
2016-06-10 22:03 ` [PATCH 39/48] ARM: dts: at91: vinco: " Alexandre Belloni
2016-06-10 22:03 ` [PATCH 40/48] ARM: dts: at91: sama5d2: TC blocks are also simple-mfd and syscon devices Alexandre Belloni
2016-06-10 22:03 ` [PATCH 41/48] ARM: dts: at91: sama5d2 Xplained: use TCB0 as clocksource Alexandre Belloni
2016-06-10 22:03 ` [PATCH 42/48] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks Alexandre Belloni
2016-06-11 12:48 ` Boris Brezillon
2016-06-22 13:07 ` Daniel Lezcano
2016-06-22 13:13 ` Boris Brezillon
2016-06-24 10:07 ` Daniel Lezcano [this message]
2017-05-16 11:59 ` Alexandre Belloni
2017-05-16 14:25 ` Daniel Lezcano
2017-05-16 14:35 ` Alexandre Belloni
2016-06-10 22:03 ` [PATCH 43/48] clocksource/drivers: Add a clockevent driver for Atmel " Alexandre Belloni
2016-06-10 22:03 ` [PATCH 44/48] clocksource: atmel-pit: allow unselecting ATMEL_PIT Alexandre Belloni
2016-06-11 9:43 ` Thomas Gleixner
2016-06-11 10:53 ` Alexandre Belloni
2016-06-13 19:04 ` Boris Brezillon
2016-06-13 19:14 ` Thomas Gleixner
2016-06-16 14:41 ` Boris Brezillon
2016-06-10 22:03 ` [PATCH 45/48] ARM: at91/defconfig: sama5: unselect ATMEL_PIT Alexandre Belloni
2016-06-10 22:03 ` [PATCH 46/48] ARM: at91/defconfig: at91_dt " Alexandre Belloni
2016-06-10 22:03 ` [PATCH 47/48] PWM: atmel-tcb: switch to new binding Alexandre Belloni
2016-06-11 2:22 ` kbuild test robot
2016-06-10 22:03 ` [PATCH 48/48] ARM: dts: at91: kizbox: switch to new pwm-atmel-tcb binding Alexandre Belloni
2016-06-11 0:14 ` kbuild test robot
2016-06-11 10:25 ` Alexandre Belloni
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=576D0645.2070805@linaro.org \
--to=daniel.lezcano@linaro.org \
--cc=alexandre.belloni@free-electrons.com \
--cc=boris.brezillon@free-electrons.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nicolas.ferre@atmel.com \
--cc=plagnioj@jcrosoft.com \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).