[v3,2/3] clocksource: Add NPS400 timers driver
diff mbox

Message ID 1454775406-25277-3-git-send-email-noamc@ezchip.com
State Superseded
Headers show

Commit Message

Noam Camus Feb. 6, 2016, 4:16 p.m. UTC
From: Noam Camus <noamc@ezchip.com>

Add internal tick generator which is shared by all cores.
Each cluster of cores view it through dedicated address.
This is used for SMP system where all CPUs synced by same
clock source.

Signed-off-by: Noam Camus <noamc@ezchip.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Stultz <john.stultz@linaro.org>
Acked-by: Vineet Gupta <vgupta@synopsys.com>
---
 .../bindings/timer/ezchip,nps400-timer.txt         |   15 ++++
 drivers/clocksource/Kconfig                        |    9 ++
 drivers/clocksource/Makefile                       |    1 +
 drivers/clocksource/timer-nps.c                    |   84 ++++++++++++++++++++
 4 files changed, 109 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
 create mode 100644 drivers/clocksource/timer-nps.c

Comments

Daniel Lezcano Feb. 8, 2016, 2:21 p.m. UTC | #1
On 02/06/2016 05:16 PM, Noam Camus wrote:
> From: Noam Camus <noamc@ezchip.com>
>
> Add internal tick generator which is shared by all cores.
> Each cluster of cores view it through dedicated address.
> This is used for SMP system where all CPUs synced by same
> clock source.
>
> Signed-off-by: Noam Camus <noamc@ezchip.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: John Stultz <john.stultz@linaro.org>
> Acked-by: Vineet Gupta <vgupta@synopsys.com>
> ---

[ ... ]

> +static cycle_t nps_clksrc_read(struct clocksource *clksrc)
> +{
> +	int cluster = raw_smp_processor_id() >> NPS_CLUSTER_OFFSET;
> +
> +	return (cycle_t)ioread32be(nps_msu_reg_low_addr[cluster]);
> +}
> +
> +static struct clocksource nps_counter = {
> +	.name	= "EZnps-tick",
> +	.rating = 301,
> +	.read   = nps_clksrc_read,
> +	.mask   = CLOCKSOURCE_MASK(32),
> +	.flags  = CLOCK_SOURCE_IS_CONTINUOUS,
> +};
> +
> +static void __init nps_setup_clocksource(struct device_node *node,
> +					 struct clk *clk)
> +{
> +	struct clocksource *clksrc = &nps_counter;
> +	int ret, cluster;
> +
> +	for (cluster = 0; cluster < NPS_CLUSTER_NUM; cluster++)
> +		nps_msu_reg_low_addr[cluster] =
> +			nps_host_reg((cluster << NPS_CLUSTER_OFFSET),
> +				 NPS_MSU_BLKID, NPS_MSU_TICK_LOW);
> +
> +	ret = clk_prepare_enable(clk);
> +	if (ret)
> +		pr_err("Couldn't enable parent clock\n");
> +
> +	nps_timer_rate = clk_get_rate(clk);

If there is an error, you continue the execution of the code. I guess 
you expect the system to hang in any case with the error in the console, 
right ?

> +	ret = clocksource_register_hz(clksrc, nps_timer_rate);

You can simplify the driver even more by using clocksource_mmio_init.
Noam Camus Feb. 9, 2016, 12:36 p.m. UTC | #2
>From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] 
>Sent: Monday, February 08, 2016 4:22 PM

>> +	ret = clk_prepare_enable(clk);
>> +	if (ret)
>> +		pr_err("Couldn't enable parent clock\n");
>> +
>> +	nps_timer_rate = clk_get_rate(clk);

>If there is an error, you continue the execution of the code. I guess you expect the system to hang in any case with the error in the >console, right ?

Since our clock is root then returned value will always be valid.
I am far from being expert here, but no one checks for clk_get_rate() return value for error.
Could you refer to a single place at clocksource drivers that checks for error in the return value.

>> +	ret = clocksource_register_hz(clksrc, nps_timer_rate);

>You can simplify the driver even more by using clocksource_mmio_init.
Since my base address depends on cluster number, which CPU is part of,  this interface is not much of a use. On top of that it assumes that I am little endian by using readl family accessories.

-Noam
Daniel Lezcano Feb. 9, 2016, 1:38 p.m. UTC | #3
On 02/09/2016 01:36 PM, Noam Camus wrote:
>> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] Sent:
>> Monday, February 08, 2016 4:22 PM
>
>>> +	ret = clk_prepare_enable(clk); +	if (ret) +		pr_err("Couldn't
>>> enable parent clock\n"); + +	nps_timer_rate = clk_get_rate(clk);
>
>> If there is an error, you continue the execution of the code. I
>> guess you expect the system to hang in any case with the error in
>> the >console, right ?
>
> Since our clock is root then returned value will always be valid. I
> am far from being expert here, but no one checks for clk_get_rate()
> return value for error. Could you refer to a single place at
> clocksource drivers that checks for error in the return value.

Actually I was referring to clk_prepare_enable, clocksource_register_hz.
Agree clk_get_rate is always valid.

>>> +	ret = clocksource_register_hz(clksrc, nps_timer_rate);
>
>> You can simplify the driver even more by using
>> clocksource_mmio_init.
> Since my base address depends on cluster number, which CPU is part
> of,  this interface is not much of a use. On top of that it assumes
> that I am little endian by using readl family accessories.

Why can't you use ?

clocksource_mmio_init(nps_msu_reg_low_addr, "EZnps-tick", 
nps_timer_rate, 32, nps_clksrc_read);
Noam Camus Feb. 9, 2016, 9:47 p.m. UTC | #4
>From: Daniel Lezcano <daniel.lezcano@linaro.org>
>Sent: Tuesday, February 9, 2016 3:38 PM

>Actually I was referring to clk_prepare_enable, clocksource_register_hz.
>Agree clk_get_rate is always valid.
Thanks for making this clear.
Any way as you can see I do call pr_err() in case of error just like most drivers around.
By "hang" do you mean calling panic()?
What if there is another clocksource in DT (even with worse rating)?
I still prefer using it then having non workable machine.

>>
>>> You can simplify the driver even more by using
>>> clocksource_mmio_init.
>> Since my base address depends on cluster number, which CPU is part
>> of,  this interface is not much of a use. On top of that it assumes
>> that I am little endian by using readl family accessories.

>Why can't you use ?

>clocksource_mmio_init(nps_msu_reg_low_addr, "EZnps-tick",
>nps_timer_rate, 32, nps_clksrc_read);

I believe that the simplification is meant for drivers that can actually use the clocksource_mmio..() accessories. Could you explain what is the advantage here, for my case, to use clocksource_mmio driver?

Thanks for your patience
-Noam
Daniel Lezcano Feb. 9, 2016, 10:55 p.m. UTC | #5
On 02/09/2016 10:47 PM, Noam Camus wrote:
>> From: Daniel Lezcano <daniel.lezcano@linaro.org> Sent: Tuesday,
>> February 9, 2016 3:38 PM
>
>> Actually I was referring to clk_prepare_enable,
>> clocksource_register_hz. Agree clk_get_rate is always valid.
> Thanks for making this clear. Any way as you can see I do call
> pr_err() in case of error just like most drivers around. By "hang" do
> you mean calling panic()?

No. I meant the errors are caught but no action is taken, the execution
continues normally as nothing wrong happened. This is why I asked if you
expect the host to hang at boot time with the last error as a hint.

I was expecting to see a call to clk_disable_unprepare if
clocksource_register_hz fails, and returning 'ret' if clk_prepare_enable
fails.

> What if there is another clocksource in DT (even with worse rating)?
> I still prefer using it then having non workable machine.

You are right. This is why failing gracefully in the init function is
important.

>>>> You can simplify the driver even more by using
>>>> clocksource_mmio_init.
>>> Since my base address depends on cluster number, which CPU is
>>> part of,  this interface is not much of a use. On top of that it
>>> assumes that I am little endian by using readl family
>>> accessories.
>
>> Why can't you use ?
>
>> clocksource_mmio_init(nps_msu_reg_low_addr, "EZnps-tick",
>> nps_timer_rate, 32, nps_clksrc_read);
>
> I believe that the simplification is meant for drivers that can
> actually use the clocksource_mmio..() accessories. Could you explain
> what is the advantage here, for my case, to use clocksource_mmio
> driver?

Using the mmio generic code will save:

+static struct clocksource nps_counter = {
+	.name	= "EZnps-tick",
+	.rating = 301,
+	.read   = nps_clksrc_read,
+	.mask   = CLOCKSOURCE_MASK(32),
+	.flags  = CLOCK_SOURCE_IS_CONTINUOUS,
+};

Up to you.

   -- Daniel
Noam Camus Feb. 10, 2016, 8:01 a.m. UTC | #6
>From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] 

>Sent: Wednesday, February 10, 2016 12:55 AM


>> pr_err() in case of error just like most drivers around. By "hang" do 

>> you mean calling panic()?


>No. I meant the errors are caught but no action is taken, the execution continues normally as nothing wrong happened. This is why I asked if you expect the host to hang at boot time with the last error as a hint.


>I was expecting to see a call to clk_disable_unprepare if clocksource_register_hz fails, and returning 'ret' if clk_prepare_enable fails.

Ok, I will fix that, and handle gracefull return. Thanks


>Using the mmio generic code will save:


>+static struct clocksource nps_counter = {

>+	.name	= "EZnps-tick",

>+	.rating = 301,

>+	.read   = nps_clksrc_read,

>+	.mask   = CLOCKSOURCE_MASK(32),

>+	.flags  = CLOCK_SOURCE_IS_CONTINUOUS,

>+};


>Up to you.

I will do that, thanks again

Noam

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

diff --git a/Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
new file mode 100644
index 0000000..c8c03d7
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
@@ -0,0 +1,15 @@ 
+NPS Network Processor
+
+Required properties:
+
+- compatible :	should be "ezchip,nps400-timer"
+
+Clocks required for compatible = "ezchip,nps400-timer":
+- clocks : Must contain a single entry describing the clock input
+
+Example:
+
+timer {
+	compatible = "ezchip,nps400-timer";
+	clocks = <&sysclk>;
+};
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 2eb5f0e..0d0ba91 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -132,6 +132,15 @@  config CLKSRC_TI_32K
 	  This option enables support for Texas Instruments 32.768 Hz clocksource
 	  available on many OMAP-like platforms.
 
+config CLKSRC_NPS
+	bool "NPS400 clocksource driver" if COMPILE_TEST
+	depends on !PHYS_ADDR_T_64BIT
+	select CLKSRC_OF if OF
+	help
+	  NPS400 clocksource support.
+	  Got 64 bit counter with update rate up to 1000MHz.
+	  This counter is accessed via couple of 32 bit memory mapped registers.
+
 config CLKSRC_STM32
 	bool "Clocksource for STM32 SoCs" if !ARCH_STM32
 	depends on OF && ARM && (ARCH_STM32 || COMPILE_TEST)
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 56bd16e..056cffd 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -46,6 +46,7 @@  obj-$(CONFIG_CLKSRC_QCOM)	+= qcom-timer.o
 obj-$(CONFIG_MTK_TIMER)		+= mtk_timer.o
 obj-$(CONFIG_CLKSRC_PISTACHIO)	+= time-pistachio.o
 obj-$(CONFIG_CLKSRC_TI_32K)	+= timer-ti-32k.o
+obj-$(CONFIG_CLKSRC_NPS)	+= timer-nps.o
 
 obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
 obj-$(CONFIG_ARM_GLOBAL_TIMER)		+= arm_global_timer.o
diff --git a/drivers/clocksource/timer-nps.c b/drivers/clocksource/timer-nps.c
new file mode 100644
index 0000000..bf9a490
--- /dev/null
+++ b/drivers/clocksource/timer-nps.c
@@ -0,0 +1,84 @@ 
+/*
+ * Copyright(c) 2015 EZchip Technologies.
+ *
+ * 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.
+ *
+ * The full GNU General Public License is included in this distribution in
+ * the file called "COPYING".
+ */
+
+#include <linux/interrupt.h>
+#include <linux/clocksource.h>
+#include <linux/clockchips.h>
+#include <linux/clk.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/cpu.h>
+#include <soc/nps/common.h>
+
+#define NPS_MSU_TICK_LOW	0xC8
+#define NPS_CLUSTER_OFFSET	8
+#define NPS_CLUSTER_NUM		16
+
+/* This array is per cluster of CPUs (Each NPS400 cluster got 256 CPUs) */
+static void *nps_msu_reg_low_addr[NPS_CLUSTER_NUM] __read_mostly;
+
+static unsigned long nps_timer_rate;
+
+static cycle_t nps_clksrc_read(struct clocksource *clksrc)
+{
+	int cluster = raw_smp_processor_id() >> NPS_CLUSTER_OFFSET;
+
+	return (cycle_t)ioread32be(nps_msu_reg_low_addr[cluster]);
+}
+
+static struct clocksource nps_counter = {
+	.name	= "EZnps-tick",
+	.rating = 301,
+	.read   = nps_clksrc_read,
+	.mask   = CLOCKSOURCE_MASK(32),
+	.flags  = CLOCK_SOURCE_IS_CONTINUOUS,
+};
+
+static void __init nps_setup_clocksource(struct device_node *node,
+					 struct clk *clk)
+{
+	struct clocksource *clksrc = &nps_counter;
+	int ret, cluster;
+
+	for (cluster = 0; cluster < NPS_CLUSTER_NUM; cluster++)
+		nps_msu_reg_low_addr[cluster] =
+			nps_host_reg((cluster << NPS_CLUSTER_OFFSET),
+				 NPS_MSU_BLKID, NPS_MSU_TICK_LOW);
+
+	ret = clk_prepare_enable(clk);
+	if (ret)
+		pr_err("Couldn't enable parent clock\n");
+
+	nps_timer_rate = clk_get_rate(clk);
+
+	ret = clocksource_register_hz(clksrc, nps_timer_rate);
+	if (ret)
+		pr_err("Couldn't register clock source.\n");
+}
+
+static void __init nps_timer_init(struct device_node *node)
+{
+	struct clk *clk;
+
+	clk = of_clk_get(node, 0);
+	if (IS_ERR(clk))
+		panic("Can't get timer clock");
+
+	nps_setup_clocksource(node, clk);
+}
+
+CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clksrc, "ezchip,nps400-timer",
+		       nps_timer_init);