[7/7] clocksource/drivers/integrator-ap: parse the chosen node
diff mbox series

Message ID 1568123236-767-8-git-send-email-claudiu.beznea@microchip.com
State Not Applicable
Headers show
Series
  • add support for clocksource/clockevent DT selection
Related show

Commit Message

Claudiu Beznea Sept. 10, 2019, 1:47 p.m. UTC
From: Alexandre Belloni <alexandre.belloni@bootlin.com>

The driver currently uses aliases to know whether the timer is the
clocksource or the clockevent. Add the /chosen/linux,clocksource and
/chosen/linux,clockevent parsing while keeping backward compatibility.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/clocksource/Kconfig               |  1 +
 drivers/clocksource/timer-integrator-ap.c | 21 ++++++++++++++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

Comments

Linus Walleij Sept. 10, 2019, 11:48 p.m. UTC | #1
On Tue, Sep 10, 2019 at 2:50 PM Claudiu Beznea
<claudiu.beznea@microchip.com> wrote:
> From: Alexandre Belloni <alexandre.belloni@bootlin.com>
>
> The driver currently uses aliases to know whether the timer is the
> clocksource or the clockevent.

OK maybe that wasn't the most elegant solution.

> Add the /chosen/linux,clocksource and
> /chosen/linux,clockevent parsing while keeping backward compatibility.

This is not how I would solve this today.

I would simply remove/comment out the IRQ from the timer
that cannot be used for clockevent from the device tree
(apparently it doesn't work anyway), and make the code only
pick a timer with a valid interrupt assigned as clock event,
while a timer without interrupt can be used for clock source.

This has the upside of not needing any special aliases or
chosen things.

Yours,
Linus Walleij
Claudiu Beznea Sept. 11, 2019, 7:14 a.m. UTC | #2
On 11.09.2019 02:48, Linus Walleij wrote:
> External E-Mail
> 
> 
> On Tue, Sep 10, 2019 at 2:50 PM Claudiu Beznea
> <claudiu.beznea@microchip.com> wrote:
>> From: Alexandre Belloni <alexandre.belloni@bootlin.com>
>>
>> The driver currently uses aliases to know whether the timer is the
>> clocksource or the clockevent.
> 
> OK maybe that wasn't the most elegant solution.
> 
>> Add the /chosen/linux,clocksource and
>> /chosen/linux,clockevent parsing while keeping backward compatibility.
> 
> This is not how I would solve this today.
> 
> I would simply remove/comment out the IRQ from the timer
> that cannot be used for clockevent from the device tree
> (apparently it doesn't work anyway), and make the code only
> pick a timer with a valid interrupt assigned as clock event,
> while a timer without interrupt can be used for clock source.
This could also be used but it will not be compatible with old device
trees. There are different ideas implemented in timer drivers with regards
to this issue. Some of them are registering 1st timer to work as
clocksource/clockevent and the 2nd one to work as clockevent/clocksource.
Some are using different compatibles, one for clocksource, one for
clockevent although these compatible seems to be for the same hardware type
(I can point drivers/clocksource/timer-sprd.c).

The idea with this series was, at it has been proposed in [1] to have one
single mechanism for this kind of situations.

Thank you,
Claudiu Beznea

[1]
https://lore.kernel.org/lkml/2f831f1b-c87d-48bd-cf02-2ebb334b964c@linaro.org/

> 
> This has the upside of not needing any special aliases or
> chosen things.
> 
> Yours,
> Linus Walleij
>

Patch
diff mbox series

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index a642c23b2fba..e1742c0abb03 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -240,6 +240,7 @@  config KEYSTONE_TIMER
 config INTEGRATOR_AP_TIMER
 	bool "Integrator-ap timer driver" if COMPILE_TEST
 	select CLKSRC_MMIO
+	select TIMER_OF
 	help
 	  Enables support for the Integrator-ap timer.
 
diff --git a/drivers/clocksource/timer-integrator-ap.c b/drivers/clocksource/timer-integrator-ap.c
index 8d6f814ace36..78af89e73125 100644
--- a/drivers/clocksource/timer-integrator-ap.c
+++ b/drivers/clocksource/timer-integrator-ap.c
@@ -14,6 +14,7 @@ 
 #include <linux/interrupt.h>
 #include <linux/sched_clock.h>
 
+#include "timer-of.h"
 #include "timer-sp.h"
 
 static void __iomem * sched_clk_base;
@@ -160,6 +161,12 @@  static int integrator_clockevent_init(unsigned long inrate,
 	return 0;
 }
 
+static struct timer_of to[] = {
+	{ .flags = TIMER_OF_TYPE_CS, },
+	{ .flags = TIMER_OF_TYPE_CE, },
+	{ /* sentinel */ }
+};
+
 static int __init integrator_ap_timer_init_of(struct device_node *node)
 {
 	const char *path;
@@ -169,6 +176,7 @@  static int __init integrator_ap_timer_init_of(struct device_node *node)
 	struct clk *clk;
 	unsigned long rate;
 	struct device_node *alias_node;
+	struct timer_of *to = node->data;
 
 	base = of_io_request_and_map(node, 0, "integrator-timer");
 	if (IS_ERR(base))
@@ -183,6 +191,17 @@  static int __init integrator_ap_timer_init_of(struct device_node *node)
 	rate = clk_get_rate(clk);
 	writel(0, base + TIMER_CTRL);
 
+	if (timer_of_is_clocksource(to))
+		/* The primary timer lacks IRQ, use as clocksource */
+		return integrator_clocksource_init(rate, base);
+
+	if (timer_of_is_clockevent(to)) {
+		/* The secondary timer will drive the clock event */
+		irq = irq_of_parse_and_map(node, 0);
+		return integrator_clockevent_init(rate, base, irq);
+	}
+
+	/* DT ABI compatibility below */
 	err = of_property_read_string(of_aliases,
 				"arm,timer-primary", &path);
 	if (err) {
@@ -227,4 +246,4 @@  static int __init integrator_ap_timer_init_of(struct device_node *node)
 }
 
 TIMER_OF_DECLARE(integrator_ap_timer, "arm,integrator-timer",
-		       integrator_ap_timer_init_of, NULL);
+		       integrator_ap_timer_init_of, to);