[v3,03/18] clocksource: Add NPS400 timers driver
diff mbox

Message ID 1448974985-11487-4-git-send-email-noamc@ezchip.com
State Superseded
Headers show

Commit Message

Noam Camus Dec. 1, 2015, 1:02 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         |   11 ++++
 drivers/clocksource/Kconfig                        |    7 ++
 drivers/clocksource/Makefile                       |    1 +
 drivers/clocksource/timer-nps.c                    |   63 ++++++++++++++++++++
 4 files changed, 82 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 Dec. 4, 2015, 9:13 a.m. UTC | #1
On 12/01/2015 02:02 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>

[ ... ]

> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 56bd16e..20969b0 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_ARC_PLAT_EZNPS)	+= timer-nps.o

CONFIG_CLKSRC_NPS

>
>   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..ef8f287
> --- /dev/null
> +++ b/drivers/clocksource/timer-nps.c
> @@ -0,0 +1,63 @@
> +/*
> + * 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/clocksource.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +#include <plat/ctop.h>

Why do you need this header ? nps_host_reg ?

We prevent to include headers from <plat> in the drivers directory. You 
should find a way to get rid of it.

> +#define NPS_MSU_TICK_LOW	0xC8
> +#define NPS_CLUSTER_OFFSET	8
> +#define NPS_CLUSTER_NUM		16
> +
> +static void *nps_msu_reg_low_addr[NPS_CLUSTER_NUM] __read_mostly;

Perhaps a small optimization...

static DEFINE_PER_CPU_READ_MOSTLY(void __iomem *, baseaddr);

static cycle_t nps_clksrc_read(struct clocksource *clksrc)
{
	void __iomem *base = per_cpu(baseaddr, raw_smp_processor_id());

	return (cycle_t)ioread32be(base);
}

and in the init function:

for_each_cpu(cpu) {
	per_cpu(baseaddr, cpu) = nps_host_reg(cpu,
					NPS_MSU_BLKID,
					NPS_MSU_TICK_LOW
}

> +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]);

AFAICT, there is a memory barrier with ioread32be, are you really sure 
we have to use it in this code path ?

> +}
> +
> +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 clocksource *clksrc = &nps_counter;
> +	unsigned long rate, dt_root;
> +	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);

> +
> +	dt_root = of_get_flat_dt_root();
> +	rate = (u32)of_get_flat_dt_prop(dt_root, "clock-frequency", NULL);

I don't get why this is done this way. The Kconfig option help says the 
clocksource rate is 1GHz but in the DT the clock is 88MHz.

It would be cleaner to define a fixed clock and then add a phandle in 
the DT.

	timer_clk: timer_clk {
		#clock-cells = <0>;
		compatible = "fixed-clock";
		clock-frequency = <123456789>;
	};

	timer {
		compatible = "ezchip,nps400-timer";
		clocks = <&timer_clk>;
	}

That will result in the same code than the other drivers.

	clk = of_clk_get(np, 0);
	if (IS_ERR(clk)) {
		pr_err("%s: invalid clock\n", np->full_name);
		return;
	}

  	rate = clk_get_rate(clk);


> +	ret = clocksource_register_hz(clksrc, rate);
> +	if (ret)
> +		pr_err("Couldn't register clock source.\n");
> +}
> +
> +CLOCKSOURCE_OF_DECLARE(nps_400, "ezchip,nps400-timer",
> +		       nps_setup_clocksource);
>
Noam Camus Dec. 4, 2015, 12:26 p.m. UTC | #2
>From: Daniel Lezcano <daniel.lezcano@linaro.org>
>Sent: Friday, December 4, 2015 11:13 AM
>> +obj-$(CONFIG_ARC_PLAT_EZNPS) += timer-nps.o

>CONFIG_CLKSRC_NPS
I wish this driver to be build only for this specific ARC platform.
This clock source is embedded in our SoC.
It is not meant to be built for any other architecture.
This is why below I include header from our ARC platform to avoid code duplicity.

>> +#include <plat/ctop.h>

>Why do you need this header ? nps_host_reg ?
Correct we use common code from our platform.

>We prevent to include headers from <plat> in the drivers directory. You
>should find a way to get rid of it.

The Only way I can think of is code duplicity and I prefer not to.
I see some include to asm/mach headers in clocksource, what is the difference?
Could you suggest a better place for me to place my header file.

>> +static void *nps_msu_reg_low_addr[NPS_CLUSTER_NUM] __read_mostly;
>
>Perhaps a small optimization...
Thanks

>static DEFINE_PER_CPU_READ_MOSTLY(void __iomem *, baseaddr);

>static cycle_t nps_clksrc_read(struct clocksource *clksrc)
>{
>       void __iomem *base = per_cpu(baseaddr, raw_smp_processor_id());
>
 >       return (cycle_t)ioread32be(base);
>}

>and in the init function:

>for_each_cpu(cpu) {
>       per_cpu(baseaddr, cpu) = nps_host_reg(cpu,
>                                      NPS_MSU_BLKID,
>                                    NPS_MSU_TICK_LOW
>}
Thanks again


>> +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]);

>,AFAICT, there is a memory barrier with ioread32be, are you really sure
>we have to use it in this code path ?
Are you saying to remove use of ioread32be?
What should I use instead?

>> +
>> +     dt_root = of_get_flat_dt_root();
>> +     rate = (u32)of_get_flat_dt_prop(dt_root, "clock-frequency", NULL);

>I don't get why this is done this way. The Kconfig option help says the
>clocksource rate is 1GHz but in the DT the clock is 88MHz.
It says that clock source is up to 1GHz

>It would be cleaner to define a fixed clock and then add a phandle in
>the DT.

>        timer_clk: timer_clk {
>               #clock-cells = <0>;
>                compatible = "fixed-clock";
>               clock-frequency = <123456789>;
>        };

>        timer {
>               compatible = "ezchip,nps400-timer";
>                clocks = <&timer_clk>;
>       }

>That will result in the same code than the other drivers.

>      clk = of_clk_get(np, 0);
>     if (IS_ERR(clk)) {
>              pr_err("%s: invalid clock\n", np->full_name);
>               return;
>        }

>      rate = clk_get_rate(clk);
Once again thanks I will fix this.

-Noam
Daniel Lezcano Dec. 4, 2015, 12:52 p.m. UTC | #3
On 12/04/2015 01:26 PM, Noam Camus wrote:
>> From: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Sent: Friday, December 4, 2015 11:13 AM
>>> +obj-$(CONFIG_ARC_PLAT_EZNPS) += timer-nps.o
>
>> CONFIG_CLKSRC_NPS
> I wish this driver to be build only for this specific ARC platform.
> This clock source is embedded in our SoC.

I understand but we are removing all platform specific Kconfig options 
from the drivers. If CONFIG_CLKSRC_NPS is selected by 
CONFIG_ARC_PLAT_EZNPS only, the result is the same.

By the way, it is probable the "if COMPILE_TEST" option is added later 
to the option. We are trying to have the drivers to be compilable on 
different platforms in order to increase the compilation test coverage. 
It is useful for instance when we touch a common code which impact all 
the drivers, we don't need the specific platform to compile on it and we 
minimize a cross compile toolchain usage. Hence compilation failure on 
ARC will be detected sooner, before the next merge window happens.

For this reason, having common code in the driver is important and 
including a platform breaks the current effort.

> It is not meant to be built for any other architecture.
> This is why below I include header from our ARC platform to avoid code duplicity.
>
>>> +#include <plat/ctop.h>
>
>> Why do you need this header ? nps_host_reg ?
> Correct we use common code from our platform.
>
>> We prevent to include headers from <plat> in the drivers directory. You
>> should find a way to get rid of it.
>
> The Only way I can think of is code duplicity and I prefer not to.
> I see some include to asm/mach headers in clocksource, what is the difference?

The difference is <asm> includes are architecture dependent and could be 
acceptable if there is no choice (all arm timers use the same 
functionality from these headers), while <plat> includes are platform 
specific, so restricted to a specific platform.

> Could you suggest a better place for me to place my header file.

Yes, perhaps:

include/soc/nps
drivers/soc/nps

>>> +static void *nps_msu_reg_low_addr[NPS_CLUSTER_NUM] __read_mostly;
>>
>> Perhaps a small optimization...
> Thanks
>
>> static DEFINE_PER_CPU_READ_MOSTLY(void __iomem *, baseaddr);
>
>> static cycle_t nps_clksrc_read(struct clocksource *clksrc)
>> {
>>        void __iomem *base = per_cpu(baseaddr, raw_smp_processor_id());
>>
>   >       return (cycle_t)ioread32be(base);
>> }
>
>> and in the init function:
>
>> for_each_cpu(cpu) {
>>        per_cpu(baseaddr, cpu) = nps_host_reg(cpu,
>>                                       NPS_MSU_BLKID,
>>                                     NPS_MSU_TICK_LOW
>> }
> Thanks again
>
>
>>> +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]);
>
>> ,AFAICT, there is a memory barrier with ioread32be, are you really sure
>> we have to use it in this code path ?
> Are you saying to remove use of ioread32be?
> What should I use instead?

Never mind, I looked at the arm's ioread32be definition where there is a 
memory barrier. With this architecture ioread32be uses the generic one 
without the memory barrier (well, yes it uses one in the raw_readl asm 
implementation but we can't do anything).

>>> +
>>> +     dt_root = of_get_flat_dt_root();
>>> +     rate = (u32)of_get_flat_dt_prop(dt_root, "clock-frequency", NULL);
>
>> I don't get why this is done this way. The Kconfig option help says the
>> clocksource rate is 1GHz but in the DT the clock is 88MHz.
> It says that clock source is up to 1GHz

ah ok.

>> It would be cleaner to define a fixed clock and then add a phandle in
>> the DT.
>
>>         timer_clk: timer_clk {
>>                #clock-cells = <0>;
>>                 compatible = "fixed-clock";
>>                clock-frequency = <123456789>;
>>         };
>
>>         timer {
>>                compatible = "ezchip,nps400-timer";
>>                 clocks = <&timer_clk>;
>>        }
>
>> That will result in the same code than the other drivers.
>
>>       clk = of_clk_get(np, 0);
>>      if (IS_ERR(clk)) {
>>               pr_err("%s: invalid clock\n", np->full_name);
>>                return;
>>         }
>
>>       rate = clk_get_rate(clk);
> Once again thanks I will fix this.
>
> -Noam
>
Noam Camus Dec. 8, 2015, 1 p.m. UTC | #4
From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] 
Sent: Friday, December 04, 2015 11:13 AM


>It would be cleaner to define a fixed clock and then add a phandle in the DT.

>	timer_clk: timer_clk {
>		#clock-cells = <0>;
>		compatible = "fixed-clock";
>		clock-frequency = <123456789>;
>	};

>	timer {
>		compatible = "ezchip,nps400-timer";
>		clocks = <&timer_clk>;
>	}

>That will result in the same code than the other drivers.

>	clk = of_clk_get(np, 0);
>	if (IS_ERR(clk)) {
>		pr_err("%s: invalid clock\n", np->full_name);
>		return;
>	}

>  	rate = clk_get_rate(clk);

Seem like my driver comes before "fixed-clock" driver so I failed since I got no clocksource provider.
How can this be solved?

-Noam

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..c5102c2
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
@@ -0,0 +1,11 @@ 
+NPS Network Processor
+
+Required properties:
+
+- compatible :	should be "ezchip,nps400-timer"
+
+Example:
+
+timer {
+	compatible = "ezchip,nps400-timer";
+};
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 71cfdf7..98b12c5 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -131,6 +131,13 @@  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
+	select CLKSRC_OF if OF
+	help
+	  NPS400 clocksource support.
+	  Got 64 bit counter with update rate up to 1000MHz.
+
 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..20969b0 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_ARC_PLAT_EZNPS)	+= 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..ef8f287
--- /dev/null
+++ b/drivers/clocksource/timer-nps.c
@@ -0,0 +1,63 @@ 
+/*
+ * 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/clocksource.h>
+#include <linux/of.h>
+#include <linux/of_fdt.h>
+#include <plat/ctop.h>
+
+#define NPS_MSU_TICK_LOW	0xC8
+#define NPS_CLUSTER_OFFSET	8
+#define NPS_CLUSTER_NUM		16
+
+static void *nps_msu_reg_low_addr[NPS_CLUSTER_NUM] __read_mostly;
+
+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 clocksource *clksrc = &nps_counter;
+	unsigned long rate, dt_root;
+	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);
+
+	dt_root = of_get_flat_dt_root();
+	rate = (u32)of_get_flat_dt_prop(dt_root, "clock-frequency", NULL);
+
+	ret = clocksource_register_hz(clksrc, rate);
+	if (ret)
+		pr_err("Couldn't register clock source.\n");
+}
+
+CLOCKSOURCE_OF_DECLARE(nps_400, "ezchip,nps400-timer",
+		       nps_setup_clocksource);