Re: [PATCH v1 02/20] clocksource: Add NPS400 timers driver

From: Daniel Lezcano
Date: Sun Nov 01 2015 - 15:44:28 EST

On 10/31/2015 02:15 PM, Noam Camus wrote:
From: Noam Camus <noamc@xxxxxxxxxx>

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@xxxxxxxxxx>
Cc: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
Cc: Rob Herring <robh+dt@xxxxxxxxxx>

Hi Noam,

Added Thomas Gleixner and John Stultz.

---
.../bindings/timer/ezchip,nps400-timer.txt | 11 ++
drivers/clocksource/Makefile | 1 +
drivers/clocksource/timer-nps.c | 103 ++++++++++++++++++++
3 files changed, 115 insertions(+), 0 deletions(-)
create mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
create mode 100644 drivers/clocksource/timer-nps.c

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/Makefile b/drivers/clocksource/Makefile
index 5c00863..28c17dc 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -45,6 +45,7 @@ obj-$(CONFIG_VF_PIT_TIMER) += vf_pit_timer.o
obj-$(CONFIG_CLKSRC_QCOM) += qcom-timer.o
obj-$(CONFIG_MTK_TIMER) += mtk_timer.o
obj-$(CONFIG_CLKSRC_PISTACHIO) += time-pistachio.o
+obj-$(CONFIG_ARC_PLAT_EZNPS) += timer-nps.o

Please add an entry in the clocksource's Kconfig.

eg:

config NPS400_TIMER
bool "NPS400 clocksource driver" if COMPILE_TEST
help
NPS400 clocksource support.

and in the platform's Kconfig:

select NPS400_TIMER


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..83a0a9d
--- /dev/null
+++ b/drivers/clocksource/timer-nps.c
@@ -0,0 +1,103 @@
+/*
+ * 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/clk.h>
+#include <linux/clockchips.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqreturn.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_fdt.h>
+#include <plat/ctop.h>

Are you sure all the headers are needed ?

+#define NPS_MSU_TICK_LOW 0xC8
+#define NPS_CLUSTER_OFFSET 8
+#define NPS_CLUSTER_NUM 16
+
+static u32 nps_msu_reg_low_addr[NPS_CLUSTER_NUM] __read_mostly;
+
+/*
+ * To get the value from the Global Timer Counter register proceed as follows:
+ * 1. Read the upper 32-bit timer counter register
+ * 2. Read the lower 32-bit timer counter register
+ * 3. Read the upper 32-bit timer counter register again. If the value is
+ * different to the 32-bit upper value read previously, go back to step 2.
+ * Otherwise the 64-bit timer counter value is correct.
+ */
+static cycle_t nps_clksrc_read(struct clocksource *clksrc)
+{
+ u64 counter;
+ u32 lower;
+ u32 upper, old_upper;
+ int cpu;
+ int cluster;
+ void *lower_p, *upper_p;
+ unsigned long flags;
+
+ local_irq_save(flags);

Why do you need to disable the interrupt here ?

+ cpu = smp_processor_id();
+ cluster = cpu >> NPS_CLUSTER_OFFSET;
+ lower_p = (void *)nps_msu_reg_low_addr[cluster];
+ upper_p = lower_p + 4;
+ local_irq_restore(flags);
+
+ upper = ioread32be(upper_p);
+ do {
+ old_upper = upper;
+ lower = ioread32be(lower_p);
+ upper = ioread32be(upper_p);
+ } while (upper != old_upper);
+
+ counter = upper;
+ counter <<= 32;
+ counter |= lower;
+ return (cycle_t)counter;

May be you can consider using only the 32bits. Sometimes it is faster than using 64bits arithmetic and reading the register three times.

https://lkml.org/lkml/2014/6/20/431

+}
+
+static struct clocksource nps_counter = {
+ .name = "EZnps-tick",
+ .rating = 301,
+ .read = nps_clksrc_read,
+ .mask = CLOCKSOURCE_MASK(64),
+ .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);

Why are you using 'of_get_flat_dt_root' / 'of_get_flat_dt_prop' ?

+ ret = clocksource_register_hz(clksrc, rate);
+ if (ret)
+ pr_err("Couldn't register clock source.\n");
+}
+
+CLOCKSOURCE_OF_DECLARE(nps_400, "nps,400-timer",
+ nps_setup_clocksource);



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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/