clockevents: Add a clkevt-of mechanism like clksrc-of

Message ID 1485346876-28506-1-git-send-email-daniel.lezcano@linaro.org
State Superseded
Headers show

Commit Message

Daniel Lezcano Jan. 25, 2017, 12:21 p.m.
The current code uses the CLOCKSOURCE_OF_DECLARE macro to fill the clksrc
table with a t-uple (name, init_function).

Unfortunately it ends up to the clockevent and the clocksource being
both initialized with this macro. It is not a problem by itself but there
is not a clear distinction between a clockevent and a clocksource in the
code initialization path. Somebody can argue there are the same IP block
and the same DT node. But conceptually from the software side, there are
two distincts entities and as is they should be initialized separetely.
Some drivers which do not have a clocksource end up by using the
CLOCKSOURCE_OF_DECLARE macro to declare a clockevent.

Another result is the fuzzy organization in the clocksource directory,
where the clockevents are implemented in the same file than the
clocksources or file labelled timer-something implementing a clocksource.

This patch provides another macro to specifically declare a clockevent in
the same way than the clocksource and gives the opportunity to write two
separate drivers, one for the clocksource and another for the clockevents.

Hopefully, that can help to do some housework in the directory, perhaps
split the drivers in to entities, for example:
	- clksrc-rockchip.c
	- clkevt-rockchip.c

Also, it gives the possibility to declare clocksources separately in the
DT and then use a clocksource from IP block while while clockevents are
used from another IP block.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

---
 drivers/clocksource/Kconfig        |  7 +++++
 drivers/clocksource/Makefile       |  1 +
 drivers/clocksource/clkevt-probe.c | 56 ++++++++++++++++++++++++++++++++++++++
 include/linux/clockchips.h         |  9 ++++++
 4 files changed, 73 insertions(+)
 create mode 100644 drivers/clocksource/clkevt-probe.c

-- 
2.7.4

Comments

Alexander Kochetkov Jan. 30, 2017, 11:04 a.m. | #1
> 25 янв. 2017 г., в 15:21, Daniel Lezcano <daniel.lezcano@linaro.org> написал(а):

> 

> Hopefully, that can help to do some housework in the directory, perhaps

> split the drivers in to entities, for example:

> 	- clksrc-rockchip.c

> 	- clkevt-rockchip.c

> 

> Also, it gives the possibility to declare clocksources separately in the

> DT and then use a clocksource from IP block while while clockevents are

> used from another IP block.


Hello, Daniel!

There is no way to do declare clocksources and clockevents in the DT now.
As I understood, it is incorrect to add properties like ‘clocksource’ or ‘clockevent’
to timer entries because this sounds like linux specific configuration. While
device tree must contain only hardware description. Thus many timer
drivers has implicit assumption: first DT timer became clocksource, second
clockevent.

If we can find the way to describe timer role in the DT, than many timer drivers
could be rewritten such a way.

Also, there is timers what use single IP to provide clocksource and clockevent
at the same time. So splitting them into two files result into more complex code.

Regards,
Alexander.
Daniel Lezcano Jan. 30, 2017, 11:36 a.m. | #2
On Mon, Jan 30, 2017 at 02:04:42PM +0300, Alexander Kochetkov wrote:
> 

> > 25 янв. 2017 г., в 15:21, Daniel Lezcano <daniel.lezcano@linaro.org> написал(а):

> > 

> > Hopefully, that can help to do some housework in the directory, perhaps

> > split the drivers in to entities, for example:

> > 	- clksrc-rockchip.c

> > 	- clkevt-rockchip.c

> > 

> > Also, it gives the possibility to declare clocksources separately in the

> > DT and then use a clocksource from IP block while while clockevents are

> > used from another IP block.

> 

> Hello, Daniel!

> 

> There is no way to do declare clocksources and clockevents in the DT now.

> As I understood, it is incorrect to add properties like ‘clocksource’ or ‘clockevent’

> to timer entries because this sounds like linux specific configuration. While

> device tree must contain only hardware description. Thus many timer

> drivers has implicit assumption: first DT timer became clocksource, second

> clockevent.


Hi Alexander,

actually, if we keep the current definition, the macro can help to split the
drivers, for instance:

CLOCKEVENT_OF_DECLARE(rk3288_timer, "rockchip,rk3288-timer",
			rk3288_clkevt_init);

CLOCKSOURCE_OF_DECLARE(rk3288_timer, "rockchip,rk3288-timer",
			rk3288_clksrc_init);

So in the code, there will be clearly a code path for the clocksource
initialization and another one for the clockevent. That will help to clarify the code
and will let to replace the global 'already initialized' variable by something
more adequate.

I'm realizing it would be simpler to do the changes with a driver directly
instead of giving a long explanation of what we can do.

Beside, Adding a 'clksrc' / 'clkevt' property (or whatever the name) is
acceptable, but the node name *must* remain 'timer'.

> If we can find the way to describe timer role in the DT, than many timer drivers

> could be rewritten such a way.

> 

> Also, there is timers what use single IP to provide clocksource and clockevent

> at the same time. So splitting them into two files result into more complex code.


Yes, physically both can be in the same IP and that is usually the case. IMO,
we should not stick necessarly to a physical view of the hardware but to a
software vision of what the hardware provides. The only pitfall is code
duplication, but I will be watching to prevent that ;)

As mentionned above, I will send a patchset with a driver using the macro and
the resulting changes, that will help to illustrate the purpose of this patch.

  -- Daniel

-- 

 <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

Patch

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 4866f7a..3dc06f0 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -5,6 +5,10 @@  config CLKSRC_OF
 	bool
 	select CLKSRC_PROBE
 
+config CLKEVT_OF
+	bool
+	select CLKEVT_PROBE
+
 config CLKSRC_ACPI
 	bool
 	select CLKSRC_PROBE
@@ -12,6 +16,9 @@  config CLKSRC_ACPI
 config CLKSRC_PROBE
 	bool
 
+config CLKEVT_PROBE
+	bool
+
 config CLKSRC_I8253
 	bool
 
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index a14111e..4b7e9a2 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -1,4 +1,5 @@ 
 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
diff --git a/drivers/clocksource/clkevt-probe.c b/drivers/clocksource/clkevt-probe.c
new file mode 100644
index 0000000..8c30fec
--- /dev/null
+++ b/drivers/clocksource/clkevt-probe.c
@@ -0,0 +1,56 @@ 
+/*
+ * Copyright (c) 2016, Linaro Ltd.  All rights reserved.
+ * Daniel Lezcano <daniel.lezcano@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/init.h>
+#include <linux/of.h>
+#include <linux/clockchip.h>
+
+extern struct of_device_id __clkevt_of_table[];
+
+static const struct of_device_id __clkevt_of_table_sentinel
+	__used __section(__clkevt_of_table_end);
+
+int __init clockevent_probe(void)
+{
+	struct device_node *np;
+	const struct of_device_id *match;
+	of_init_fn_1_ret init_func;
+	int ret, clockevents = 0;
+
+	for_each_matching_node_and_match(np, __clkevt_of_table, &match) {
+		if (!of_device_is_available(np))
+			continue;
+
+		init_func = match->data;
+
+		ret = init_func(np);
+		if (ret) {
+			pr_warn("Failed to initialize '%s' (%d)\n",
+				np->name, ret);
+			continue;
+		}
+
+		clockevents++;
+	}
+
+	if (!clockevents) {
+		pr_crit("%s: no matching clockevent found\n", __func__);
+		return -ENODEV;
+	}
+
+	return 0;
+}
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index 0d442e3..5d3053c 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -224,4 +224,13 @@  static inline void tick_setup_hrtimer_broadcast(void) { }
 
 #endif /* !CONFIG_GENERIC_CLOCKEVENTS */
 
+#define CLOCKEVENT_OF_DECLARE(name, compat, fn) \
+	OF_DECLARE_1_RET(clkevt, name, compat, fn)
+
+#ifdef CONFIG_CLKEVT_PROBE
+extern int clockevent_probe(void);
+#els
+static inline int clockevent_probe(void) { return 0; }
+#endif
+
 #endif /* _LINUX_CLOCKCHIPS_H */