[PATCH v5 0/3] Add clockevet for timer-nps driver to NPS400 SoC

[PATCH v5 0/3] Add clockevet for timer-nps driver to NPS400 SoC

[Noam Camus]

From: Noam Camus <noamca@mellanox.com>

Change log
---
V4 --> V5
Apply several comments made by Daneil Lezcano:
1) Add __init attribute to nps_get_timer_clk()
2) Fix return value of nps_get_timer_clk()
 when failing to get clk rate
3) Change clocksource rate from 301 -> 300

V3 --> V4
Main changes are [Thanks for the review]:
Fix many typos at log [Daniel]
Add handling for bad return values [Daniel and Thomas]
Replace use of internal irqchip pointers with existing IRQ API [Thomas]
Provide interrupt handler (percpu) with dev_id equal to evt [Thomas]
Fix passing *clk by reference to nps_get_timer_clk() [Daniel]

V2 --> V3
Apply Rob Herring comment about backword compatibility

V1 --> V2
Apply Daniel Lezcano comments:
	CLOCKSOURCE_OF_DECLARE return value
	update hotplug callbacks usage
	squash of 2 first commits.
In this version I created new commit to serve as preperation for adding clockevents.
This way the last patch is more readable with clockevent content.
---

In first version of this driver we supported clocksource for the NPS400.
The support for clockevent was taken from Synopsys ARC timer driver.
This was good for working with our simulator of NPS400.
However in NPS400 ASIC the timers behave differently than simulation.
The timers in ASIC are shared between all threads whithin a core
and hence need different driver to support this behaviour.

The idea of this design is that we got 16 HW threads per core
each represented at bimask in a shared register in this core.
So when thread wants that next clockevent expiration will produce
timer interrupt to itself the correspondance bit in this register
should be set.
So theoretically if all 16 bits are set then all HW threads will get
timer interrupt on next expiration of timer 0.

Note that we use Synopsys ARC design naming convention for the timers
where:
timer0 is used for clockevents
timer1 is used for clocksource.

Noam Camus (3):
  soc: Support for NPS HW scheduling
  clocksource: update "fn" at CLOCKSOURCE_OF_DECLARE() of nps400 timer
  clocksource: Add clockevent support to NPS400 driver

 .../bindings/timer/ezchip,nps400-timer.txt         |   15 --
 .../bindings/timer/ezchip,nps400-timer0.txt        |   17 ++
 .../bindings/timer/ezchip,nps400-timer1.txt        |   15 ++
 arch/arc/plat-eznps/include/plat/ctop.h            |    2 -
 drivers/clocksource/timer-nps.c                    |  264 ++++++++++++++++++--
 include/soc/nps/mtm.h                              |   59 +++++
 6 files changed, 335 insertions(+), 37 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
 create mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt
 create mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt
 create mode 100644 include/soc/nps/mtm.h

[PATCH v5 0/3] Add clockevet for timer-nps driver to NPS400 SoC

[Noam Camus]

From: Noam Camus <noamca@mellanox.com>

Change log
---
V4 --> V5
Apply several comments made by Daneil Lezcano:
1) Add __init attribute to nps_get_timer_clk()
2) Fix return value of nps_get_timer_clk()
 when failing to get clk rate
3) Change clocksource rate from 301 -> 300

V3 --> V4
Main changes are [Thanks for the review]:
Fix many typos at log [Daniel]
Add handling for bad return values [Daniel and Thomas]
Replace use of internal irqchip pointers with existing IRQ API [Thomas]
Provide interrupt handler (percpu) with dev_id equal to evt [Thomas]
Fix passing *clk by reference to nps_get_timer_clk() [Daniel]

V2 --> V3
Apply Rob Herring comment about backword compatibility

V1 --> V2
Apply Daniel Lezcano comments:
	CLOCKSOURCE_OF_DECLARE return value
	update hotplug callbacks usage
	squash of 2 first commits.
In this version I created new commit to serve as preperation for adding clockevents.
This way the last patch is more readable with clockevent content.
---

In first version of this driver we supported clocksource for the NPS400.
The support for clockevent was taken from Synopsys ARC timer driver.
This was good for working with our simulator of NPS400.
However in NPS400 ASIC the timers behave differently than simulation.
The timers in ASIC are shared between all threads whithin a core
and hence need different driver to support this behaviour.

The idea of this design is that we got 16 HW threads per core
each represented at bimask in a shared register in this core.
So when thread wants that next clockevent expiration will produce
timer interrupt to itself the correspondance bit in this register
should be set.
So theoretically if all 16 bits are set then all HW threads will get
timer interrupt on next expiration of timer 0.

Note that we use Synopsys ARC design naming convention for the timers
where:
timer0 is used for clockevents
timer1 is used for clocksource.

Noam Camus (3):
  soc: Support for NPS HW scheduling
  clocksource: update "fn" at CLOCKSOURCE_OF_DECLARE() of nps400 timer
  clocksource: Add clockevent support to NPS400 driver

 .../bindings/timer/ezchip,nps400-timer.txt         |   15 --
 .../bindings/timer/ezchip,nps400-timer0.txt        |   17 ++
 .../bindings/timer/ezchip,nps400-timer1.txt        |   15 ++
 arch/arc/plat-eznps/include/plat/ctop.h            |    2 -
 drivers/clocksource/timer-nps.c                    |  264 ++++++++++++++++++--
 include/soc/nps/mtm.h                              |   59 +++++
 6 files changed, 335 insertions(+), 37 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
 create mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt
 create mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt
 create mode 100644 include/soc/nps/mtm.h

[PATCH v5 1/3] soc: Support for NPS HW scheduling

[Noam Camus]

From: Noam Camus <noamca@mellanox.com>

This new header file is for NPS400 SoC (part of ARC architecture).
The header file includes macros for save/restore of HW scheduling.
The control of HW scheduling is achieved by writing core registers.
This code was moved from arc/plat-eznps so it can be used
from drivers/clocksource/, available only for CONFIG_EZNPS_MTM_EXT.

Signed-off-by: Noam Camus <noamca@mellanox.com>
---
 arch/arc/plat-eznps/include/plat/ctop.h |    2 -
 include/soc/nps/mtm.h                   |   59 +++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 2 deletions(-)
 create mode 100644 include/soc/nps/mtm.h

diff --git a/arch/arc/plat-eznps/include/plat/ctop.h b/arch/arc/plat-eznps/include/plat/ctop.h
index 9d6718c..ee2e32d 100644
--- a/arch/arc/plat-eznps/include/plat/ctop.h
+++ b/arch/arc/plat-eznps/include/plat/ctop.h
@@ -46,9 +46,7 @@
 #define CTOP_AUX_UDMC				(CTOP_AUX_BASE + 0x300)
 
 /* EZchip core instructions */
-#define CTOP_INST_HWSCHD_OFF_R3			0x3B6F00BF
 #define CTOP_INST_HWSCHD_OFF_R4			0x3C6F00BF
-#define CTOP_INST_HWSCHD_RESTORE_R3		0x3E6F70C3
 #define CTOP_INST_HWSCHD_RESTORE_R4		0x3E6F7103
 #define CTOP_INST_SCHD_RW			0x3E6F7004
 #define CTOP_INST_SCHD_RD			0x3E6F7084
diff --git a/include/soc/nps/mtm.h b/include/soc/nps/mtm.h
new file mode 100644
index 0000000..d2f5e7e
--- /dev/null
+++ b/include/soc/nps/mtm.h
@@ -0,0 +1,59 @@
+/*
+ * Copyright (c) 2016, Mellanox Technologies. All rights reserved.
+ *
+ * This software is available to you under a choice of one of two
+ * licenses.  You may choose to be licensed under the terms of the GNU
+ * General Public License (GPL) Version 2, available from the file
+ * COPYING in the main directory of this source tree, or the
+ * OpenIB.org BSD license below:
+ *
+ *     Redistribution and use in source and binary forms, with or
+ *     without modification, are permitted provided that the following
+ *     conditions are met:
+ *
+ *      - Redistributions of source code must retain the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer.
+ *
+ *      - Redistributions in binary form must reproduce the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer in the documentation and/or other materials
+ *        provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#ifndef SOC_NPS_MTM_H
+#define SOC_NPS_MTM_H
+
+#define CTOP_INST_HWSCHD_OFF_R3                 0x3B6F00BF
+#define CTOP_INST_HWSCHD_RESTORE_R3             0x3E6F70C3
+
+static inline void hw_schd_save(unsigned int *flags)
+{
+	__asm__ __volatile__(
+	"       .word %1\n"
+	"       st r3,[%0]\n"
+	:
+	: "r"(flags), "i"(CTOP_INST_HWSCHD_OFF_R3)
+	: "r3", "memory");
+}
+
+static inline void hw_schd_restore(unsigned int flags)
+{
+	__asm__ __volatile__(
+	"       mov r3, %0\n"
+	"       .word %1\n"
+	:
+	: "r"(flags), "i"(CTOP_INST_HWSCHD_RESTORE_R3)
+	: "r3");
+}
+
+#endif /* SOC_NPS_MTM_H */
-- 
1.7.1

[PATCH v5 1/3] soc: Support for NPS HW scheduling

[Noam Camus]

From: Noam Camus <noamca-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

This new header file is for NPS400 SoC (part of ARC architecture).
The header file includes macros for save/restore of HW scheduling.
The control of HW scheduling is achieved by writing core registers.
This code was moved from arc/plat-eznps so it can be used
from drivers/clocksource/, available only for CONFIG_EZNPS_MTM_EXT.

Signed-off-by: Noam Camus <noamca-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 arch/arc/plat-eznps/include/plat/ctop.h |    2 -
 include/soc/nps/mtm.h                   |   59 +++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 2 deletions(-)
 create mode 100644 include/soc/nps/mtm.h

diff --git a/arch/arc/plat-eznps/include/plat/ctop.h b/arch/arc/plat-eznps/include/plat/ctop.h
index 9d6718c..ee2e32d 100644
--- a/arch/arc/plat-eznps/include/plat/ctop.h
+++ b/arch/arc/plat-eznps/include/plat/ctop.h
@@ -46,9 +46,7 @@
 #define CTOP_AUX_UDMC				(CTOP_AUX_BASE + 0x300)
 
 /* EZchip core instructions */
-#define CTOP_INST_HWSCHD_OFF_R3			0x3B6F00BF
 #define CTOP_INST_HWSCHD_OFF_R4			0x3C6F00BF
-#define CTOP_INST_HWSCHD_RESTORE_R3		0x3E6F70C3
 #define CTOP_INST_HWSCHD_RESTORE_R4		0x3E6F7103
 #define CTOP_INST_SCHD_RW			0x3E6F7004
 #define CTOP_INST_SCHD_RD			0x3E6F7084
diff --git a/include/soc/nps/mtm.h b/include/soc/nps/mtm.h
new file mode 100644
index 0000000..d2f5e7e
--- /dev/null
+++ b/include/soc/nps/mtm.h
@@ -0,0 +1,59 @@
+/*
+ * Copyright (c) 2016, Mellanox Technologies. All rights reserved.
+ *
+ * This software is available to you under a choice of one of two
+ * licenses.  You may choose to be licensed under the terms of the GNU
+ * General Public License (GPL) Version 2, available from the file
+ * COPYING in the main directory of this source tree, or the
+ * OpenIB.org BSD license below:
+ *
+ *     Redistribution and use in source and binary forms, with or
+ *     without modification, are permitted provided that the following
+ *     conditions are met:
+ *
+ *      - Redistributions of source code must retain the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer.
+ *
+ *      - Redistributions in binary form must reproduce the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer in the documentation and/or other materials
+ *        provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#ifndef SOC_NPS_MTM_H
+#define SOC_NPS_MTM_H
+
+#define CTOP_INST_HWSCHD_OFF_R3                 0x3B6F00BF
+#define CTOP_INST_HWSCHD_RESTORE_R3             0x3E6F70C3
+
+static inline void hw_schd_save(unsigned int *flags)
+{
+	__asm__ __volatile__(
+	"       .word %1\n"
+	"       st r3,[%0]\n"
+	:
+	: "r"(flags), "i"(CTOP_INST_HWSCHD_OFF_R3)
+	: "r3", "memory");
+}
+
+static inline void hw_schd_restore(unsigned int flags)
+{
+	__asm__ __volatile__(
+	"       mov r3, %0\n"
+	"       .word %1\n"
+	:
+	: "r"(flags), "i"(CTOP_INST_HWSCHD_RESTORE_R3)
+	: "r3");
+}
+
+#endif /* SOC_NPS_MTM_H */
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 2/3] clocksource: update "fn" at CLOCKSOURCE_OF_DECLARE() of nps400 timer

[Noam Camus]

From: Noam Camus <noamca@mellanox.com>

nps_setup_clocksource() should take node as only argument i.e.:
replace
int __init nps_setup_clocksource(struct device_node *node, struct clk *clk)
with
int __init nps_setup_clocksource(struct device_node *node)

This is also serve as preperation for next patch which adds support
for clockevents to nps400.
Specifically we add new function nps_get_timer_clk() to serve clocksource
and later clockevent registration.

Signed-off-by: Noam Camus <noamca@mellanox.com>
---
 drivers/clocksource/timer-nps.c |   63 +++++++++++++++++++++++----------------
 1 files changed, 37 insertions(+), 26 deletions(-)

diff --git a/drivers/clocksource/timer-nps.c b/drivers/clocksource/timer-nps.c
index 70c149a..eeef9e9 100644
--- a/drivers/clocksource/timer-nps.c
+++ b/drivers/clocksource/timer-nps.c
@@ -46,7 +46,33 @@
 /* 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 int __init nps_get_timer_clk(struct device_node *node,
+			     unsigned long *timer_freq,
+			     struct clk **clk)
+{
+	int ret;
+
+	*clk = of_clk_get(node, 0);
+	if (IS_ERR(*clk)) {
+		pr_err("timer missing clk");
+		return PTR_ERR(*clk);
+	}
+
+	ret = clk_prepare_enable(*clk);
+	if (ret) {
+		pr_err("Couldn't enable parent clk\n");
+		return ret;
+	}
+
+	*timer_freq = clk_get_rate(*clk);
+	if (!(*timer_freq)) {
+		pr_err("Couldn't get clk rate\n");
+		clk_disable_unprepare(*clk);
+		return 1;
+	}
+
+	return 0;
+}
 
 static cycle_t nps_clksrc_read(struct clocksource *clksrc)
 {
@@ -55,26 +81,24 @@ static cycle_t nps_clksrc_read(struct clocksource *clksrc)
 	return (cycle_t)ioread32be(nps_msu_reg_low_addr[cluster]);
 }
 
-static int __init nps_setup_clocksource(struct device_node *node,
-					struct clk *clk)
+static int __init nps_setup_clocksource(struct device_node *node)
 {
 	int ret, cluster;
+	struct clk *clk;
+	unsigned long nps_timer1_freq;
+
 
 	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);
+				     NPS_MSU_BLKID, NPS_MSU_TICK_LOW);
 
-	ret = clk_prepare_enable(clk);
-	if (ret) {
-		pr_err("Couldn't enable parent clock\n");
+	ret = nps_get_timer_clk(node, &nps_timer1_freq, &clk);
+	if (ret)
 		return ret;
-	}
 
-	nps_timer_rate = clk_get_rate(clk);
-
-	ret = clocksource_mmio_init(nps_msu_reg_low_addr, "EZnps-tick",
-				    nps_timer_rate, 301, 32, nps_clksrc_read);
+	ret = clocksource_mmio_init(nps_msu_reg_low_addr, "nps-tick",
+				    nps_timer1_freq, 300, 32, nps_clksrc_read);
 	if (ret) {
 		pr_err("Couldn't register clock source.\n");
 		clk_disable_unprepare(clk);
@@ -83,18 +107,5 @@ static int __init nps_setup_clocksource(struct device_node *node,
 	return ret;
 }
 
-static int __init nps_timer_init(struct device_node *node)
-{
-	struct clk *clk;
-
-	clk = of_clk_get(node, 0);
-	if (IS_ERR(clk)) {
-		pr_err("Can't get timer clock.\n");
-		return PTR_ERR(clk);
-	}
-
-	return nps_setup_clocksource(node, clk);
-}
-
 CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clksrc, "ezchip,nps400-timer",
-		       nps_timer_init);
+		       nps_setup_clocksource);
-- 
1.7.1

[PATCH v5 2/3] clocksource: update "fn" at CLOCKSOURCE_OF_DECLARE() of nps400 timer

[Noam Camus]

From: Noam Camus <noamca-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

nps_setup_clocksource() should take node as only argument i.e.:
replace
int __init nps_setup_clocksource(struct device_node *node, struct clk *clk)
with
int __init nps_setup_clocksource(struct device_node *node)

This is also serve as preperation for next patch which adds support
for clockevents to nps400.
Specifically we add new function nps_get_timer_clk() to serve clocksource
and later clockevent registration.

Signed-off-by: Noam Camus <noamca-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/clocksource/timer-nps.c |   63 +++++++++++++++++++++++----------------
 1 files changed, 37 insertions(+), 26 deletions(-)

diff --git a/drivers/clocksource/timer-nps.c b/drivers/clocksource/timer-nps.c
index 70c149a..eeef9e9 100644
--- a/drivers/clocksource/timer-nps.c
+++ b/drivers/clocksource/timer-nps.c
@@ -46,7 +46,33 @@
 /* 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 int __init nps_get_timer_clk(struct device_node *node,
+			     unsigned long *timer_freq,
+			     struct clk **clk)
+{
+	int ret;
+
+	*clk = of_clk_get(node, 0);
+	if (IS_ERR(*clk)) {
+		pr_err("timer missing clk");
+		return PTR_ERR(*clk);
+	}
+
+	ret = clk_prepare_enable(*clk);
+	if (ret) {
+		pr_err("Couldn't enable parent clk\n");
+		return ret;
+	}
+
+	*timer_freq = clk_get_rate(*clk);
+	if (!(*timer_freq)) {
+		pr_err("Couldn't get clk rate\n");
+		clk_disable_unprepare(*clk);
+		return 1;
+	}
+
+	return 0;
+}
 
 static cycle_t nps_clksrc_read(struct clocksource *clksrc)
 {
@@ -55,26 +81,24 @@ static cycle_t nps_clksrc_read(struct clocksource *clksrc)
 	return (cycle_t)ioread32be(nps_msu_reg_low_addr[cluster]);
 }
 
-static int __init nps_setup_clocksource(struct device_node *node,
-					struct clk *clk)
+static int __init nps_setup_clocksource(struct device_node *node)
 {
 	int ret, cluster;
+	struct clk *clk;
+	unsigned long nps_timer1_freq;
+
 
 	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);
+				     NPS_MSU_BLKID, NPS_MSU_TICK_LOW);
 
-	ret = clk_prepare_enable(clk);
-	if (ret) {
-		pr_err("Couldn't enable parent clock\n");
+	ret = nps_get_timer_clk(node, &nps_timer1_freq, &clk);
+	if (ret)
 		return ret;
-	}
 
-	nps_timer_rate = clk_get_rate(clk);
-
-	ret = clocksource_mmio_init(nps_msu_reg_low_addr, "EZnps-tick",
-				    nps_timer_rate, 301, 32, nps_clksrc_read);
+	ret = clocksource_mmio_init(nps_msu_reg_low_addr, "nps-tick",
+				    nps_timer1_freq, 300, 32, nps_clksrc_read);
 	if (ret) {
 		pr_err("Couldn't register clock source.\n");
 		clk_disable_unprepare(clk);
@@ -83,18 +107,5 @@ static int __init nps_setup_clocksource(struct device_node *node,
 	return ret;
 }
 
-static int __init nps_timer_init(struct device_node *node)
-{
-	struct clk *clk;
-
-	clk = of_clk_get(node, 0);
-	if (IS_ERR(clk)) {
-		pr_err("Can't get timer clock.\n");
-		return PTR_ERR(clk);
-	}
-
-	return nps_setup_clocksource(node, clk);
-}
-
 CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clksrc, "ezchip,nps400-timer",
-		       nps_timer_init);
+		       nps_setup_clocksource);
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 3/3] clocksource: Add clockevent support to NPS400 driver

[Noam Camus]

From: Noam Camus <noamca@mellanox.com>

Till now we used clockevent from generic ARC driver.
This was enough as long as we worked with simple multicore SoC.
When we are working with multithread SoC each HW thread can be
scheduled to receive timer interrupt using timer mask register.
This patch will provide a way to control clock events per HW thread.

The design idea is that for each core there is dedicated regirtser
(TSI) serving all 16 HW threads.
The register is a bitmask with one bit for each HW thread.
When HW thread wants that next expiration of timer interrupt will
hit it then the proper bit should be set in this dedicated register.
When timer expires all HW threads within this core which their bit
is set at the TSI register will be interrupted.

Driver can be used from device tree by:
compatible = "ezchip,nps400-timer0" <-- for clocksource
compatible = "ezchip,nps400-timer1" <-- for clockevent

Note that name convention for timer0/timer1 was taken from legacy
ARC design. This design is our base before adding HW threads.
For backward compatibility we keep "ezchip,nps400-timer" for clocksource

Signed-off-by: Noam Camus <noamca@mellanox.com>
---
 .../bindings/timer/ezchip,nps400-timer.txt         |   15 --
 .../bindings/timer/ezchip,nps400-timer0.txt        |   17 ++
 .../bindings/timer/ezchip,nps400-timer1.txt        |   15 ++
 drivers/clocksource/timer-nps.c                    |  213 ++++++++++++++++++++
 4 files changed, 245 insertions(+), 15 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
 create mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt
 create mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt

diff --git a/Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
deleted file mode 100644
index c8c03d7..0000000
--- a/Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
+++ /dev/null
@@ -1,15 +0,0 @@
-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/Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt
new file mode 100644
index 0000000..e3cfce8
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt
@@ -0,0 +1,17 @@
+NPS Network Processor
+
+Required properties:
+
+- compatible :	should be "ezchip,nps400-timer0"
+
+Clocks required for compatible = "ezchip,nps400-timer0":
+- interrupts : The interrupt of the first timer
+- clocks : Must contain a single entry describing the clock input
+
+Example:
+
+timer {
+	compatible = "ezchip,nps400-timer0";
+	interrupts = <3>;
+	clocks = <&sysclk>;
+};
diff --git a/Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt
new file mode 100644
index 0000000..c0ab419
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt
@@ -0,0 +1,15 @@
+NPS Network Processor
+
+Required properties:
+
+- compatible :	should be "ezchip,nps400-timer1"
+
+Clocks required for compatible = "ezchip,nps400-timer1":
+- clocks : Must contain a single entry describing the clock input
+
+Example:
+
+timer {
+	compatible = "ezchip,nps400-timer1";
+	clocks = <&sysclk>;
+};
diff --git a/drivers/clocksource/timer-nps.c b/drivers/clocksource/timer-nps.c
index eeef9e9..ed4bce4 100644
--- a/drivers/clocksource/timer-nps.c
+++ b/drivers/clocksource/timer-nps.c
@@ -109,3 +109,216 @@ static int __init nps_setup_clocksource(struct device_node *node)
 
 CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clksrc, "ezchip,nps400-timer",
 		       nps_setup_clocksource);
+CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clk_src, "ezchip,nps400-timer1",
+		       nps_setup_clocksource);
+
+#ifdef CONFIG_EZNPS_MTM_EXT
+#include <soc/nps/mtm.h>
+
+/* Timer related Aux registers */
+#define NPS_REG_TIMER0_TSI	0xFFFFF850
+#define NPS_REG_TIMER0_LIMIT	0x23
+#define NPS_REG_TIMER0_CTRL	0x22
+#define NPS_REG_TIMER0_CNT	0x21
+
+/*
+ * Interrupt Enabled (IE) - re-arm the timer
+ * Not Halted (NH) - is cleared when working with JTAG (for debug)
+ */
+#define TIMER0_CTRL_IE		BIT(0)
+#define TIMER0_CTRL_NH		BIT(1)
+
+static unsigned long nps_timer0_freq;
+static unsigned long nps_timer0_irq;
+
+/*
+ * Arm the timer to interrupt after @cycles
+ */
+static void nps_clkevent_timer_event_setup(unsigned int cycles)
+{
+	write_aux_reg(NPS_REG_TIMER0_LIMIT, cycles);
+	write_aux_reg(NPS_REG_TIMER0_CNT, 0);
+
+	write_aux_reg(NPS_REG_TIMER0_CTRL, TIMER0_CTRL_IE | TIMER0_CTRL_NH);
+}
+
+/*
+ * Clear from TSI the bit for this thread (if not in periodic mode)
+ * If still there are pending HW treads set next timer event
+ */
+static void nps_clkevent_rm_thread(bool remove_thread)
+{
+	unsigned int cflags;
+	unsigned int enabled_threads = 0;
+	int thread;
+
+	hw_schd_save(&cflags);
+
+	enabled_threads = read_aux_reg(NPS_REG_TIMER0_TSI);
+
+	/* remove thread from TSI1 */
+	if (remove_thread) {
+		thread = read_aux_reg(CTOP_AUX_THREAD_ID);
+		enabled_threads &= ~(1 << thread);
+		write_aux_reg(NPS_REG_TIMER0_TSI, enabled_threads);
+	}
+
+	/* Re-arm the timer if needed */
+	if (!enabled_threads)
+		write_aux_reg(NPS_REG_TIMER0_CTRL, TIMER0_CTRL_NH);
+	else
+		write_aux_reg(NPS_REG_TIMER0_CTRL,
+			      TIMER0_CTRL_IE | TIMER0_CTRL_NH);
+
+	hw_schd_restore(cflags);
+}
+
+static void nps_clkevent_add_thread(bool set_event)
+{
+	int thread;
+	unsigned int cflags, enabled_threads;
+
+	hw_schd_save(&cflags);
+
+	/* add thread to TSI1 */
+	thread = read_aux_reg(CTOP_AUX_THREAD_ID);
+	enabled_threads = read_aux_reg(NPS_REG_TIMER0_TSI);
+	enabled_threads |= (1 << thread);
+	write_aux_reg(NPS_REG_TIMER0_TSI, enabled_threads);
+
+	/* set next timer event */
+	if (set_event)
+		write_aux_reg(NPS_REG_TIMER0_CTRL,
+			      TIMER0_CTRL_IE | TIMER0_CTRL_NH);
+
+	hw_schd_restore(cflags);
+}
+
+static int nps_clkevent_set_next_event(unsigned long delta,
+				       struct clock_event_device *dev)
+{
+	nps_clkevent_add_thread(true);
+	enable_percpu_irq(nps_timer0_irq, IRQ_TYPE_NONE);
+
+	return 0;
+}
+
+/*
+ * Whenever anyone tries to change modes, we just mask interrupts
+ * and wait for the next event to get set.
+ */
+static int nps_clkevent_timer_shutdown(struct clock_event_device *dev)
+{
+	disable_percpu_irq(nps_timer0_irq);
+
+	return 0;
+}
+
+/*
+ * For each HW thread set its relevant bit at the TSI register
+ * To arm the timer only thread 0 is needed since it is shared
+ * by all HW threads within same core.
+ */
+static int nps_clkevent_set_periodic(struct clock_event_device *dev)
+{
+	nps_clkevent_add_thread(false);
+	if (read_aux_reg(CTOP_AUX_THREAD_ID) == 0)
+		nps_clkevent_timer_event_setup(nps_timer0_freq / HZ);
+
+	return 0;
+}
+
+static int nps_clkevent_set_oneshot(struct clock_event_device *dev)
+{
+	nps_clkevent_rm_thread(true);
+	nps_clkevent_timer_shutdown(dev);
+
+	return 0;
+}
+
+static DEFINE_PER_CPU(struct clock_event_device, nps_clockevent_device) = {
+	.name				=	"NPS Timer0",
+	.features			=	CLOCK_EVT_FEAT_ONESHOT |
+						CLOCK_EVT_FEAT_PERIODIC,
+	.rating				=	300,
+	.set_next_event			=	nps_clkevent_set_next_event,
+	.set_state_periodic		=	nps_clkevent_set_periodic,
+	.set_state_oneshot		=	nps_clkevent_set_oneshot,
+	.set_state_oneshot_stopped	=	nps_clkevent_timer_shutdown,
+	.set_state_shutdown		=	nps_clkevent_timer_shutdown,
+	.tick_resume			=	nps_clkevent_timer_shutdown,
+};
+
+static irqreturn_t timer_irq_handler(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+	int irq_reenable = clockevent_state_periodic(evt);
+
+	/* Remove HW thread from TSI only if NOT in periodic state */
+	nps_clkevent_rm_thread(!irq_reenable);
+
+	evt->event_handler(evt);
+
+	return IRQ_HANDLED;
+}
+
+static int nps_timer_starting_cpu(unsigned int cpu)
+{
+	struct clock_event_device *evt = this_cpu_ptr(&nps_clockevent_device);
+
+	evt->cpumask = cpumask_of(smp_processor_id());
+
+	clockevents_config_and_register(evt, nps_timer0_freq, 0, ULONG_MAX);
+	enable_percpu_irq(nps_timer0_irq, IRQ_TYPE_NONE);
+
+	return 0;
+}
+
+static int nps_timer_dying_cpu(unsigned int cpu)
+{
+	disable_percpu_irq(nps_timer0_irq);
+	return 0;
+}
+
+static int __init nps_setup_clockevent(struct device_node *node)
+{
+	struct clk *clk;
+	int ret;
+
+	nps_timer0_irq = irq_of_parse_and_map(node, 0);
+	if (nps_timer0_irq <= 0) {
+		pr_err("clockevent: missing irq");
+		return -EINVAL;
+	}
+
+	ret = nps_get_timer_clk(node, &nps_timer0_freq, &clk);
+	if (ret)
+		return ret;
+
+	/* Needs apriori irq_set_percpu_devid() done in intc map function */
+	ret = request_percpu_irq(nps_timer0_irq, timer_irq_handler,
+				 "Timer0 (per-cpu-tick)",
+				 &nps_clockevent_device);
+	if (ret) {
+		pr_err("Couldn't request irq\n");
+		clk_disable_unprepare(clk);
+		return ret;
+	}
+
+	ret = cpuhp_setup_state(CPUHP_AP_ARC_TIMER_STARTING,
+				"clockevents/nps:starting",
+				nps_timer_starting_cpu,
+				nps_timer_dying_cpu);
+	if (ret) {
+		pr_err("Failed to setup hotplug state");
+		clk_disable_unprepare(clk);
+		free_percpu_irq(nps_timer0_irq, &nps_clockevent_device);
+		return ret;
+	}
+
+	return 0;
+}
+
+CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clk_evt, "ezchip,nps400-timer0",
+		       nps_setup_clockevent);
+#endif /* CONFIG_EZNPS_MTM_EXT */
-- 
1.7.1

[PATCH v5 3/3] clocksource: Add clockevent support to NPS400 driver

[Noam Camus]

From: Noam Camus <noamca@mellanox.com>

Till now we used clockevent from generic ARC driver.
This was enough as long as we worked with simple multicore SoC.
When we are working with multithread SoC each HW thread can be
scheduled to receive timer interrupt using timer mask register.
This patch will provide a way to control clock events per HW thread.

The design idea is that for each core there is dedicated regirtser
(TSI) serving all 16 HW threads.
The register is a bitmask with one bit for each HW thread.
When HW thread wants that next expiration of timer interrupt will
hit it then the proper bit should be set in this dedicated register.
When timer expires all HW threads within this core which their bit
is set at the TSI register will be interrupted.

Driver can be used from device tree by:
compatible = "ezchip,nps400-timer0" <-- for clocksource
compatible = "ezchip,nps400-timer1" <-- for clockevent

Note that name convention for timer0/timer1 was taken from legacy
ARC design. This design is our base before adding HW threads.
For backward compatibility we keep "ezchip,nps400-timer" for clocksource

Signed-off-by: Noam Camus <noamca@mellanox.com>
---
 .../bindings/timer/ezchip,nps400-timer.txt         |   15 --
 .../bindings/timer/ezchip,nps400-timer0.txt        |   17 ++
 .../bindings/timer/ezchip,nps400-timer1.txt        |   15 ++
 drivers/clocksource/timer-nps.c                    |  213 ++++++++++++++++++++
 4 files changed, 245 insertions(+), 15 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
 create mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt
 create mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt

diff --git a/Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
deleted file mode 100644
index c8c03d7..0000000
--- a/Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
+++ /dev/null
@@ -1,15 +0,0 @@
-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/Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt
new file mode 100644
index 0000000..e3cfce8
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt
@@ -0,0 +1,17 @@
+NPS Network Processor
+
+Required properties:
+
+- compatible :	should be "ezchip,nps400-timer0"
+
+Clocks required for compatible = "ezchip,nps400-timer0":
+- interrupts : The interrupt of the first timer
+- clocks : Must contain a single entry describing the clock input
+
+Example:
+
+timer {
+	compatible = "ezchip,nps400-timer0";
+	interrupts = <3>;
+	clocks = <&sysclk>;
+};
diff --git a/Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt
new file mode 100644
index 0000000..c0ab419
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt
@@ -0,0 +1,15 @@
+NPS Network Processor
+
+Required properties:
+
+- compatible :	should be "ezchip,nps400-timer1"
+
+Clocks required for compatible = "ezchip,nps400-timer1":
+- clocks : Must contain a single entry describing the clock input
+
+Example:
+
+timer {
+	compatible = "ezchip,nps400-timer1";
+	clocks = <&sysclk>;
+};
diff --git a/drivers/clocksource/timer-nps.c b/drivers/clocksource/timer-nps.c
index eeef9e9..ed4bce4 100644
--- a/drivers/clocksource/timer-nps.c
+++ b/drivers/clocksource/timer-nps.c
@@ -109,3 +109,216 @@ static int __init nps_setup_clocksource(struct device_node *node)
 
 CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clksrc, "ezchip,nps400-timer",
 		       nps_setup_clocksource);
+CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clk_src, "ezchip,nps400-timer1",
+		       nps_setup_clocksource);
+
+#ifdef CONFIG_EZNPS_MTM_EXT
+#include <soc/nps/mtm.h>
+
+/* Timer related Aux registers */
+#define NPS_REG_TIMER0_TSI	0xFFFFF850
+#define NPS_REG_TIMER0_LIMIT	0x23
+#define NPS_REG_TIMER0_CTRL	0x22
+#define NPS_REG_TIMER0_CNT	0x21
+
+/*
+ * Interrupt Enabled (IE) - re-arm the timer
+ * Not Halted (NH) - is cleared when working with JTAG (for debug)
+ */
+#define TIMER0_CTRL_IE		BIT(0)
+#define TIMER0_CTRL_NH		BIT(1)
+
+static unsigned long nps_timer0_freq;
+static unsigned long nps_timer0_irq;
+
+/*
+ * Arm the timer to interrupt after @cycles
+ */
+static void nps_clkevent_timer_event_setup(unsigned int cycles)
+{
+	write_aux_reg(NPS_REG_TIMER0_LIMIT, cycles);
+	write_aux_reg(NPS_REG_TIMER0_CNT, 0);
+
+	write_aux_reg(NPS_REG_TIMER0_CTRL, TIMER0_CTRL_IE | TIMER0_CTRL_NH);
+}
+
+/*
+ * Clear from TSI the bit for this thread (if not in periodic mode)
+ * If still there are pending HW treads set next timer event
+ */
+static void nps_clkevent_rm_thread(bool remove_thread)
+{
+	unsigned int cflags;
+	unsigned int enabled_threads = 0;
+	int thread;
+
+	hw_schd_save(&cflags);
+
+	enabled_threads = read_aux_reg(NPS_REG_TIMER0_TSI);
+
+	/* remove thread from TSI1 */
+	if (remove_thread) {
+		thread = read_aux_reg(CTOP_AUX_THREAD_ID);
+		enabled_threads &= ~(1 << thread);
+		write_aux_reg(NPS_REG_TIMER0_TSI, enabled_threads);
+	}
+
+	/* Re-arm the timer if needed */
+	if (!enabled_threads)
+		write_aux_reg(NPS_REG_TIMER0_CTRL, TIMER0_CTRL_NH);
+	else
+		write_aux_reg(NPS_REG_TIMER0_CTRL,
+			      TIMER0_CTRL_IE | TIMER0_CTRL_NH);
+
+	hw_schd_restore(cflags);
+}
+
+static void nps_clkevent_add_thread(bool set_event)
+{
+	int thread;
+	unsigned int cflags, enabled_threads;
+
+	hw_schd_save(&cflags);
+
+	/* add thread to TSI1 */
+	thread = read_aux_reg(CTOP_AUX_THREAD_ID);
+	enabled_threads = read_aux_reg(NPS_REG_TIMER0_TSI);
+	enabled_threads |= (1 << thread);
+	write_aux_reg(NPS_REG_TIMER0_TSI, enabled_threads);
+
+	/* set next timer event */
+	if (set_event)
+		write_aux_reg(NPS_REG_TIMER0_CTRL,
+			      TIMER0_CTRL_IE | TIMER0_CTRL_NH);
+
+	hw_schd_restore(cflags);
+}
+
+static int nps_clkevent_set_next_event(unsigned long delta,
+				       struct clock_event_device *dev)
+{
+	nps_clkevent_add_thread(true);
+	enable_percpu_irq(nps_timer0_irq, IRQ_TYPE_NONE);
+
+	return 0;
+}
+
+/*
+ * Whenever anyone tries to change modes, we just mask interrupts
+ * and wait for the next event to get set.
+ */
+static int nps_clkevent_timer_shutdown(struct clock_event_device *dev)
+{
+	disable_percpu_irq(nps_timer0_irq);
+
+	return 0;
+}
+
+/*
+ * For each HW thread set its relevant bit at the TSI register
+ * To arm the timer only thread 0 is needed since it is shared
+ * by all HW threads within same core.
+ */
+static int nps_clkevent_set_periodic(struct clock_event_device *dev)
+{
+	nps_clkevent_add_thread(false);
+	if (read_aux_reg(CTOP_AUX_THREAD_ID) == 0)
+		nps_clkevent_timer_event_setup(nps_timer0_freq / HZ);
+
+	return 0;
+}
+
+static int nps_clkevent_set_oneshot(struct clock_event_device *dev)
+{
+	nps_clkevent_rm_thread(true);
+	nps_clkevent_timer_shutdown(dev);
+
+	return 0;
+}
+
+static DEFINE_PER_CPU(struct clock_event_device, nps_clockevent_device) = {
+	.name				=	"NPS Timer0",
+	.features			=	CLOCK_EVT_FEAT_ONESHOT |
+						CLOCK_EVT_FEAT_PERIODIC,
+	.rating				=	300,
+	.set_next_event			=	nps_clkevent_set_next_event,
+	.set_state_periodic		=	nps_clkevent_set_periodic,
+	.set_state_oneshot		=	nps_clkevent_set_oneshot,
+	.set_state_oneshot_stopped	=	nps_clkevent_timer_shutdown,
+	.set_state_shutdown		=	nps_clkevent_timer_shutdown,
+	.tick_resume			=	nps_clkevent_timer_shutdown,
+};
+
+static irqreturn_t timer_irq_handler(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+	int irq_reenable = clockevent_state_periodic(evt);
+
+	/* Remove HW thread from TSI only if NOT in periodic state */
+	nps_clkevent_rm_thread(!irq_reenable);
+
+	evt->event_handler(evt);
+
+	return IRQ_HANDLED;
+}
+
+static int nps_timer_starting_cpu(unsigned int cpu)
+{
+	struct clock_event_device *evt = this_cpu_ptr(&nps_clockevent_device);
+
+	evt->cpumask = cpumask_of(smp_processor_id());
+
+	clockevents_config_and_register(evt, nps_timer0_freq, 0, ULONG_MAX);
+	enable_percpu_irq(nps_timer0_irq, IRQ_TYPE_NONE);
+
+	return 0;
+}
+
+static int nps_timer_dying_cpu(unsigned int cpu)
+{
+	disable_percpu_irq(nps_timer0_irq);
+	return 0;
+}
+
+static int __init nps_setup_clockevent(struct device_node *node)
+{
+	struct clk *clk;
+	int ret;
+
+	nps_timer0_irq = irq_of_parse_and_map(node, 0);
+	if (nps_timer0_irq <= 0) {
+		pr_err("clockevent: missing irq");
+		return -EINVAL;
+	}
+
+	ret = nps_get_timer_clk(node, &nps_timer0_freq, &clk);
+	if (ret)
+		return ret;
+
+	/* Needs apriori irq_set_percpu_devid() done in intc map function */
+	ret = request_percpu_irq(nps_timer0_irq, timer_irq_handler,
+				 "Timer0 (per-cpu-tick)",
+				 &nps_clockevent_device);
+	if (ret) {
+		pr_err("Couldn't request irq\n");
+		clk_disable_unprepare(clk);
+		return ret;
+	}
+
+	ret = cpuhp_setup_state(CPUHP_AP_ARC_TIMER_STARTING,
+				"clockevents/nps:starting",
+				nps_timer_starting_cpu,
+				nps_timer_dying_cpu);
+	if (ret) {
+		pr_err("Failed to setup hotplug state");
+		clk_disable_unprepare(clk);
+		free_percpu_irq(nps_timer0_irq, &nps_clockevent_device);
+		return ret;
+	}
+
+	return 0;
+}
+
+CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clk_evt, "ezchip,nps400-timer0",
+		       nps_setup_clockevent);
+#endif /* CONFIG_EZNPS_MTM_EXT */
-- 
1.7.1

Re: [PATCH v5 2/3] clocksource: update "fn" at CLOCKSOURCE_OF_DECLARE() of nps400 timer

[Daniel Lezcano]

On Sun, Nov 13, 2016 at 09:24:31AM +0200, Noam Camus wrote:
> From: Noam Camus <noamca@mellanox.com>
> 
> nps_setup_clocksource() should take node as only argument i.e.:
> replace
> int __init nps_setup_clocksource(struct device_node *node, struct clk *clk)
> with
> int __init nps_setup_clocksource(struct device_node *node)

because ...

> This is also serve as preperation for next patch which adds support

s/This is also/This also/
s/preperation/preparation/


> for clockevents to nps400.
> Specifically we add new function nps_get_timer_clk() to serve clocksource
> and later clockevent registration.
> 
> Signed-off-by: Noam Camus <noamca@mellanox.com>
> ---
>  drivers/clocksource/timer-nps.c |   63 +++++++++++++++++++++++----------------
>  1 files changed, 37 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-nps.c b/drivers/clocksource/timer-nps.c
> index 70c149a..eeef9e9 100644
> --- a/drivers/clocksource/timer-nps.c
> +++ b/drivers/clocksource/timer-nps.c
> @@ -46,7 +46,33 @@
>  /* 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 int __init nps_get_timer_clk(struct device_node *node,
> +			     unsigned long *timer_freq,
> +			     struct clk **clk)
> +{
> +	int ret;
> +
> +	*clk = of_clk_get(node, 0);
> +	if (IS_ERR(*clk)) {
> +		pr_err("timer missing clk");
> +		return PTR_ERR(*clk);
> +	}
> +
> +	ret = clk_prepare_enable(*clk);
> +	if (ret) {
> +		pr_err("Couldn't enable parent clk\n");
> +		return ret;
> +	}
> +
> +	*timer_freq = clk_get_rate(*clk);
> +	if (!(*timer_freq)) {
> +		pr_err("Couldn't get clk rate\n");
> +		clk_disable_unprepare(*clk);
> +		return 1;

s/return 1/return -EINVAL/

What about clk_put() ?

> +	}
> +
> +	return 0;
> +}
>  
>  static cycle_t nps_clksrc_read(struct clocksource *clksrc)
>  {
> @@ -55,26 +81,24 @@ static cycle_t nps_clksrc_read(struct clocksource *clksrc)
>  	return (cycle_t)ioread32be(nps_msu_reg_low_addr[cluster]);
>  }
>  
> -static int __init nps_setup_clocksource(struct device_node *node,
> -					struct clk *clk)
> +static int __init nps_setup_clocksource(struct device_node *node)
>  {
>  	int ret, cluster;
> +	struct clk *clk;
> +	unsigned long nps_timer1_freq;
> +
>  
>  	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);
> +				     NPS_MSU_BLKID, NPS_MSU_TICK_LOW);
>  
> -	ret = clk_prepare_enable(clk);
> -	if (ret) {
> -		pr_err("Couldn't enable parent clock\n");
> +	ret = nps_get_timer_clk(node, &nps_timer1_freq, &clk);
> +	if (ret)
>  		return ret;
> -	}
>  
> -	nps_timer_rate = clk_get_rate(clk);
> -
> -	ret = clocksource_mmio_init(nps_msu_reg_low_addr, "EZnps-tick",
> -				    nps_timer_rate, 301, 32, nps_clksrc_read);
> +	ret = clocksource_mmio_init(nps_msu_reg_low_addr, "nps-tick",
> +				    nps_timer1_freq, 300, 32, nps_clksrc_read);
>  	if (ret) {
>  		pr_err("Couldn't register clock source.\n");
>  		clk_disable_unprepare(clk);
> @@ -83,18 +107,5 @@ static int __init nps_setup_clocksource(struct device_node *node,
>  	return ret;
>  }
>  
> -static int __init nps_timer_init(struct device_node *node)
> -{
> -	struct clk *clk;
> -
> -	clk = of_clk_get(node, 0);
> -	if (IS_ERR(clk)) {
> -		pr_err("Can't get timer clock.\n");
> -		return PTR_ERR(clk);
> -	}
> -
> -	return nps_setup_clocksource(node, clk);
> -}
> -
>  CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clksrc, "ezchip,nps400-timer",
> -		       nps_timer_init);
> +		       nps_setup_clocksource);
> -- 
> 1.7.1
> 

-- 

 <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

Re: [PATCH v5 2/3] clocksource: update "fn" at CLOCKSOURCE_OF_DECLARE() of nps400 timer

[Daniel Lezcano]

On Sun, Nov 13, 2016 at 09:24:31AM +0200, Noam Camus wrote:
> From: Noam Camus <noamca-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> nps_setup_clocksource() should take node as only argument i.e.:
> replace
> int __init nps_setup_clocksource(struct device_node *node, struct clk *clk)
> with
> int __init nps_setup_clocksource(struct device_node *node)

because ...

> This is also serve as preperation for next patch which adds support

s/This is also/This also/
s/preperation/preparation/


> for clockevents to nps400.
> Specifically we add new function nps_get_timer_clk() to serve clocksource
> and later clockevent registration.
> 
> Signed-off-by: Noam Camus <noamca-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/clocksource/timer-nps.c |   63 +++++++++++++++++++++++----------------
>  1 files changed, 37 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-nps.c b/drivers/clocksource/timer-nps.c
> index 70c149a..eeef9e9 100644
> --- a/drivers/clocksource/timer-nps.c
> +++ b/drivers/clocksource/timer-nps.c
> @@ -46,7 +46,33 @@
>  /* 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 int __init nps_get_timer_clk(struct device_node *node,
> +			     unsigned long *timer_freq,
> +			     struct clk **clk)
> +{
> +	int ret;
> +
> +	*clk = of_clk_get(node, 0);
> +	if (IS_ERR(*clk)) {
> +		pr_err("timer missing clk");
> +		return PTR_ERR(*clk);
> +	}
> +
> +	ret = clk_prepare_enable(*clk);
> +	if (ret) {
> +		pr_err("Couldn't enable parent clk\n");
> +		return ret;
> +	}
> +
> +	*timer_freq = clk_get_rate(*clk);
> +	if (!(*timer_freq)) {
> +		pr_err("Couldn't get clk rate\n");
> +		clk_disable_unprepare(*clk);
> +		return 1;

s/return 1/return -EINVAL/

What about clk_put() ?

> +	}
> +
> +	return 0;
> +}
>  
>  static cycle_t nps_clksrc_read(struct clocksource *clksrc)
>  {
> @@ -55,26 +81,24 @@ static cycle_t nps_clksrc_read(struct clocksource *clksrc)
>  	return (cycle_t)ioread32be(nps_msu_reg_low_addr[cluster]);
>  }
>  
> -static int __init nps_setup_clocksource(struct device_node *node,
> -					struct clk *clk)
> +static int __init nps_setup_clocksource(struct device_node *node)
>  {
>  	int ret, cluster;
> +	struct clk *clk;
> +	unsigned long nps_timer1_freq;
> +
>  
>  	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);
> +				     NPS_MSU_BLKID, NPS_MSU_TICK_LOW);
>  
> -	ret = clk_prepare_enable(clk);
> -	if (ret) {
> -		pr_err("Couldn't enable parent clock\n");
> +	ret = nps_get_timer_clk(node, &nps_timer1_freq, &clk);
> +	if (ret)
>  		return ret;
> -	}
>  
> -	nps_timer_rate = clk_get_rate(clk);
> -
> -	ret = clocksource_mmio_init(nps_msu_reg_low_addr, "EZnps-tick",
> -				    nps_timer_rate, 301, 32, nps_clksrc_read);
> +	ret = clocksource_mmio_init(nps_msu_reg_low_addr, "nps-tick",
> +				    nps_timer1_freq, 300, 32, nps_clksrc_read);
>  	if (ret) {
>  		pr_err("Couldn't register clock source.\n");
>  		clk_disable_unprepare(clk);
> @@ -83,18 +107,5 @@ static int __init nps_setup_clocksource(struct device_node *node,
>  	return ret;
>  }
>  
> -static int __init nps_timer_init(struct device_node *node)
> -{
> -	struct clk *clk;
> -
> -	clk = of_clk_get(node, 0);
> -	if (IS_ERR(clk)) {
> -		pr_err("Can't get timer clock.\n");
> -		return PTR_ERR(clk);
> -	}
> -
> -	return nps_setup_clocksource(node, clk);
> -}
> -
>  CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clksrc, "ezchip,nps400-timer",
> -		       nps_timer_init);
> +		       nps_setup_clocksource);
> -- 
> 1.7.1
> 

-- 

 <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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 3/3] clocksource: Add clockevent support to NPS400 driver

[Daniel Lezcano]

On Sun, Nov 13, 2016 at 09:24:32AM +0200, Noam Camus wrote:
> From: Noam Camus <noamca@mellanox.com>
> 
> Till now we used clockevent from generic ARC driver.
> This was enough as long as we worked with simple multicore SoC.
> When we are working with multithread SoC each HW thread can be
> scheduled to receive timer interrupt using timer mask register.
> This patch will provide a way to control clock events per HW thread.
> 
> The design idea is that for each core there is dedicated regirtser

s/regirtser/register/ (already spotted in v2).

> (TSI) serving all 16 HW threads.
> The register is a bitmask with one bit for each HW thread.
> When HW thread wants that next expiration of timer interrupt will
> hit it then the proper bit should be set in this dedicated register.
> When timer expires all HW threads within this core which their bit
> is set at the TSI register will be interrupted.
> 
> Driver can be used from device tree by:
> compatible = "ezchip,nps400-timer0" <-- for clocksource
> compatible = "ezchip,nps400-timer1" <-- for clockevent
> 
> Note that name convention for timer0/timer1 was taken from legacy
> ARC design. This design is our base before adding HW threads.
> For backward compatibility we keep "ezchip,nps400-timer" for clocksource
> 
> Signed-off-by: Noam Camus <noamca@mellanox.com>
> ---
>  .../bindings/timer/ezchip,nps400-timer.txt         |   15 --
>  .../bindings/timer/ezchip,nps400-timer0.txt        |   17 ++
>  .../bindings/timer/ezchip,nps400-timer1.txt        |   15 ++
>  drivers/clocksource/timer-nps.c                    |  213 ++++++++++++++++++++
>  4 files changed, 245 insertions(+), 15 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
>  create mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt
>  create mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt
> 
> diff --git a/Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
> deleted file mode 100644
> index c8c03d7..0000000
> --- a/Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
> +++ /dev/null
> @@ -1,15 +0,0 @@
> -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/Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt
> new file mode 100644
> index 0000000..e3cfce8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt
> @@ -0,0 +1,17 @@
> +NPS Network Processor
> +
> +Required properties:
> +
> +- compatible :	should be "ezchip,nps400-timer0"
> +
> +Clocks required for compatible = "ezchip,nps400-timer0":
> +- interrupts : The interrupt of the first timer
> +- clocks : Must contain a single entry describing the clock input
> +
> +Example:
> +
> +timer {
> +	compatible = "ezchip,nps400-timer0";
> +	interrupts = <3>;
> +	clocks = <&sysclk>;
> +};
> diff --git a/Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt
> new file mode 100644
> index 0000000..c0ab419
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt
> @@ -0,0 +1,15 @@
> +NPS Network Processor
> +
> +Required properties:
> +
> +- compatible :	should be "ezchip,nps400-timer1"
> +
> +Clocks required for compatible = "ezchip,nps400-timer1":
> +- clocks : Must contain a single entry describing the clock input
> +
> +Example:
> +
> +timer {
> +	compatible = "ezchip,nps400-timer1";
> +	clocks = <&sysclk>;
> +};
> diff --git a/drivers/clocksource/timer-nps.c b/drivers/clocksource/timer-nps.c
> index eeef9e9..ed4bce4 100644
> --- a/drivers/clocksource/timer-nps.c
> +++ b/drivers/clocksource/timer-nps.c
> @@ -109,3 +109,216 @@ static int __init nps_setup_clocksource(struct device_node *node)
>  
>  CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clksrc, "ezchip,nps400-timer",
>  		       nps_setup_clocksource);
> +CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clk_src, "ezchip,nps400-timer1",
> +		       nps_setup_clocksource);
> +
> +#ifdef CONFIG_EZNPS_MTM_EXT
> +#include <soc/nps/mtm.h>
> +
> +/* Timer related Aux registers */
> +#define NPS_REG_TIMER0_TSI	0xFFFFF850
> +#define NPS_REG_TIMER0_LIMIT	0x23
> +#define NPS_REG_TIMER0_CTRL	0x22
> +#define NPS_REG_TIMER0_CNT	0x21
> +
> +/*
> + * Interrupt Enabled (IE) - re-arm the timer
> + * Not Halted (NH) - is cleared when working with JTAG (for debug)
> + */
> +#define TIMER0_CTRL_IE		BIT(0)
> +#define TIMER0_CTRL_NH		BIT(1)
> +
> +static unsigned long nps_timer0_freq;
> +static unsigned long nps_timer0_irq;
> +
> +/*
> + * Arm the timer to interrupt after @cycles
> + */
> +static void nps_clkevent_timer_event_setup(unsigned int cycles)
> +{
> +	write_aux_reg(NPS_REG_TIMER0_LIMIT, cycles);
> +	write_aux_reg(NPS_REG_TIMER0_CNT, 0);
> +
> +	write_aux_reg(NPS_REG_TIMER0_CTRL, TIMER0_CTRL_IE | TIMER0_CTRL_NH);
> +}
> +
> +/*
> + * Clear from TSI the bit for this thread (if not in periodic mode)
> + * If still there are pending HW treads set next timer event

s/treads/threads/

> + */
> +static void nps_clkevent_rm_thread(bool remove_thread)
> +{
> +	unsigned int cflags;
> +	unsigned int enabled_threads = 0;
> +	int thread;
> +
> +	hw_schd_save(&cflags);

I'm not used with hardware scheduling. Can you explain why this is needed
here ? What window race we want to close ?

> +
> +	enabled_threads = read_aux_reg(NPS_REG_TIMER0_TSI);
> +
> +	/* remove thread from TSI1 */
> +	if (remove_thread) {
> +		thread = read_aux_reg(CTOP_AUX_THREAD_ID);
> +		enabled_threads &= ~(1 << thread);
> +		write_aux_reg(NPS_REG_TIMER0_TSI, enabled_threads);
> +	}
> +
> +	/* Re-arm the timer if needed */
> +	if (!enabled_threads)
> +		write_aux_reg(NPS_REG_TIMER0_CTRL, TIMER0_CTRL_NH);
> +	else
> +		write_aux_reg(NPS_REG_TIMER0_CTRL,
> +			      TIMER0_CTRL_IE | TIMER0_CTRL_NH);
> +
> +	hw_schd_restore(cflags);
> +}
> +
> +static void nps_clkevent_add_thread(bool set_event)
> +{
> +	int thread;
> +	unsigned int cflags, enabled_threads;
> +
> +	hw_schd_save(&cflags);
> +
> +	/* add thread to TSI1 */
> +	thread = read_aux_reg(CTOP_AUX_THREAD_ID);
> +	enabled_threads = read_aux_reg(NPS_REG_TIMER0_TSI);
> +	enabled_threads |= (1 << thread);
> +	write_aux_reg(NPS_REG_TIMER0_TSI, enabled_threads);
> +
> +	/* set next timer event */
> +	if (set_event)
> +		write_aux_reg(NPS_REG_TIMER0_CTRL,
> +			      TIMER0_CTRL_IE | TIMER0_CTRL_NH);
> +
> +	hw_schd_restore(cflags);
> +}

Not sure the boolean parameters for *_rm_thread and *_add_thread helps
to clarify the code. Depending on the race window with hw_schd_save/restore
We should be able to simplify it.

> +static int nps_clkevent_set_next_event(unsigned long delta,
> +				       struct clock_event_device *dev)
> +{
> +	nps_clkevent_add_thread(true);
> +	enable_percpu_irq(nps_timer0_irq, IRQ_TYPE_NONE);
> +
> +	return 0;
> +}
> +
> +/*
> + * Whenever anyone tries to change modes, we just mask interrupts
> + * and wait for the next event to get set.
> + */
> +static int nps_clkevent_timer_shutdown(struct clock_event_device *dev)
> +{
> +	disable_percpu_irq(nps_timer0_irq);
> +
> +	return 0;
> +}
> +
> +/*
> + * For each HW thread set its relevant bit at the TSI register
> + * To arm the timer only thread 0 is needed since it is shared
> + * by all HW threads within same core.
> + */
> +static int nps_clkevent_set_periodic(struct clock_event_device *dev)
> +{
> +	nps_clkevent_add_thread(false);
> +	if (read_aux_reg(CTOP_AUX_THREAD_ID) == 0)
> +		nps_clkevent_timer_event_setup(nps_timer0_freq / HZ);
> +
> +	return 0;
> +}
> +
> +static int nps_clkevent_set_oneshot(struct clock_event_device *dev)
> +{
> +	nps_clkevent_rm_thread(true);
> +	nps_clkevent_timer_shutdown(dev);
> +
> +	return 0;
> +}
> +
> +static DEFINE_PER_CPU(struct clock_event_device, nps_clockevent_device) = {
> +	.name				=	"NPS Timer0",
> +	.features			=	CLOCK_EVT_FEAT_ONESHOT |
> +						CLOCK_EVT_FEAT_PERIODIC,
> +	.rating				=	300,
> +	.set_next_event			=	nps_clkevent_set_next_event,
> +	.set_state_periodic		=	nps_clkevent_set_periodic,
> +	.set_state_oneshot		=	nps_clkevent_set_oneshot,
> +	.set_state_oneshot_stopped	=	nps_clkevent_timer_shutdown,
> +	.set_state_shutdown		=	nps_clkevent_timer_shutdown,

Doesn't set_state_shutdown and set_state_oneshot_stopped need to remove
the HW thread from the TSI ?

> +	.tick_resume			=	nps_clkevent_timer_shutdown,
> +};
> +
> +static irqreturn_t timer_irq_handler(int irq, void *dev_id)
> +{
> +	struct clock_event_device *evt = dev_id;
> +	int irq_reenable = clockevent_state_periodic(evt);
> +
> +	/* Remove HW thread from TSI only if NOT in periodic state */
> +	nps_clkevent_rm_thread(!irq_reenable);
> +
> +	evt->event_handler(evt);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int nps_timer_starting_cpu(unsigned int cpu)
> +{
> +	struct clock_event_device *evt = this_cpu_ptr(&nps_clockevent_device);
> +
> +	evt->cpumask = cpumask_of(smp_processor_id());
> +
> +	clockevents_config_and_register(evt, nps_timer0_freq, 0, ULONG_MAX);
> +	enable_percpu_irq(nps_timer0_irq, IRQ_TYPE_NONE);
> +
> +	return 0;
> +}
> +
> +static int nps_timer_dying_cpu(unsigned int cpu)
> +{
> +	disable_percpu_irq(nps_timer0_irq);
> +	return 0;
> +}
> +
> +static int __init nps_setup_clockevent(struct device_node *node)
> +{
> +	struct clk *clk;
> +	int ret;
> +
> +	nps_timer0_irq = irq_of_parse_and_map(node, 0);
> +	if (nps_timer0_irq <= 0) {
> +		pr_err("clockevent: missing irq");
> +		return -EINVAL;
> +	}
> +
> +	ret = nps_get_timer_clk(node, &nps_timer0_freq, &clk);
> +	if (ret)
> +		return ret;
> +
> +	/* Needs apriori irq_set_percpu_devid() done in intc map function */
> +	ret = request_percpu_irq(nps_timer0_irq, timer_irq_handler,
> +				 "Timer0 (per-cpu-tick)",
> +				 &nps_clockevent_device);
> +	if (ret) {
> +		pr_err("Couldn't request irq\n");
> +		clk_disable_unprepare(clk);
> +		return ret;
> +	}
> +
> +	ret = cpuhp_setup_state(CPUHP_AP_ARC_TIMER_STARTING,
> +				"clockevents/nps:starting",
> +				nps_timer_starting_cpu,
> +				nps_timer_dying_cpu);
> +	if (ret) {
> +		pr_err("Failed to setup hotplug state");
> +		clk_disable_unprepare(clk);
> +		free_percpu_irq(nps_timer0_irq, &nps_clockevent_device);
> +		return ret;
> +	}
> +
> +	return 0;



> +}
> +
> +CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clk_evt, "ezchip,nps400-timer0",
> +		       nps_setup_clockevent);
> +#endif /* CONFIG_EZNPS_MTM_EXT */
> -- 
> 1.7.1
> 

-- 

 <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

Re: [PATCH v5 3/3] clocksource: Add clockevent support to NPS400 driver

[Daniel Lezcano]

On Sun, Nov 13, 2016 at 09:24:32AM +0200, Noam Camus wrote:
> From: Noam Camus <noamca-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> Till now we used clockevent from generic ARC driver.
> This was enough as long as we worked with simple multicore SoC.
> When we are working with multithread SoC each HW thread can be
> scheduled to receive timer interrupt using timer mask register.
> This patch will provide a way to control clock events per HW thread.
> 
> The design idea is that for each core there is dedicated regirtser

s/regirtser/register/ (already spotted in v2).

> (TSI) serving all 16 HW threads.
> The register is a bitmask with one bit for each HW thread.
> When HW thread wants that next expiration of timer interrupt will
> hit it then the proper bit should be set in this dedicated register.
> When timer expires all HW threads within this core which their bit
> is set at the TSI register will be interrupted.
> 
> Driver can be used from device tree by:
> compatible = "ezchip,nps400-timer0" <-- for clocksource
> compatible = "ezchip,nps400-timer1" <-- for clockevent
> 
> Note that name convention for timer0/timer1 was taken from legacy
> ARC design. This design is our base before adding HW threads.
> For backward compatibility we keep "ezchip,nps400-timer" for clocksource
> 
> Signed-off-by: Noam Camus <noamca-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
>  .../bindings/timer/ezchip,nps400-timer.txt         |   15 --
>  .../bindings/timer/ezchip,nps400-timer0.txt        |   17 ++
>  .../bindings/timer/ezchip,nps400-timer1.txt        |   15 ++
>  drivers/clocksource/timer-nps.c                    |  213 ++++++++++++++++++++
>  4 files changed, 245 insertions(+), 15 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
>  create mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt
>  create mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt
> 
> diff --git a/Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
> deleted file mode 100644
> index c8c03d7..0000000
> --- a/Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
> +++ /dev/null
> @@ -1,15 +0,0 @@
> -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/Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt
> new file mode 100644
> index 0000000..e3cfce8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt
> @@ -0,0 +1,17 @@
> +NPS Network Processor
> +
> +Required properties:
> +
> +- compatible :	should be "ezchip,nps400-timer0"
> +
> +Clocks required for compatible = "ezchip,nps400-timer0":
> +- interrupts : The interrupt of the first timer
> +- clocks : Must contain a single entry describing the clock input
> +
> +Example:
> +
> +timer {
> +	compatible = "ezchip,nps400-timer0";
> +	interrupts = <3>;
> +	clocks = <&sysclk>;
> +};
> diff --git a/Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt
> new file mode 100644
> index 0000000..c0ab419
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt
> @@ -0,0 +1,15 @@
> +NPS Network Processor
> +
> +Required properties:
> +
> +- compatible :	should be "ezchip,nps400-timer1"
> +
> +Clocks required for compatible = "ezchip,nps400-timer1":
> +- clocks : Must contain a single entry describing the clock input
> +
> +Example:
> +
> +timer {
> +	compatible = "ezchip,nps400-timer1";
> +	clocks = <&sysclk>;
> +};
> diff --git a/drivers/clocksource/timer-nps.c b/drivers/clocksource/timer-nps.c
> index eeef9e9..ed4bce4 100644
> --- a/drivers/clocksource/timer-nps.c
> +++ b/drivers/clocksource/timer-nps.c
> @@ -109,3 +109,216 @@ static int __init nps_setup_clocksource(struct device_node *node)
>  
>  CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clksrc, "ezchip,nps400-timer",
>  		       nps_setup_clocksource);
> +CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clk_src, "ezchip,nps400-timer1",
> +		       nps_setup_clocksource);
> +
> +#ifdef CONFIG_EZNPS_MTM_EXT
> +#include <soc/nps/mtm.h>
> +
> +/* Timer related Aux registers */
> +#define NPS_REG_TIMER0_TSI	0xFFFFF850
> +#define NPS_REG_TIMER0_LIMIT	0x23
> +#define NPS_REG_TIMER0_CTRL	0x22
> +#define NPS_REG_TIMER0_CNT	0x21
> +
> +/*
> + * Interrupt Enabled (IE) - re-arm the timer
> + * Not Halted (NH) - is cleared when working with JTAG (for debug)
> + */
> +#define TIMER0_CTRL_IE		BIT(0)
> +#define TIMER0_CTRL_NH		BIT(1)
> +
> +static unsigned long nps_timer0_freq;
> +static unsigned long nps_timer0_irq;
> +
> +/*
> + * Arm the timer to interrupt after @cycles
> + */
> +static void nps_clkevent_timer_event_setup(unsigned int cycles)
> +{
> +	write_aux_reg(NPS_REG_TIMER0_LIMIT, cycles);
> +	write_aux_reg(NPS_REG_TIMER0_CNT, 0);
> +
> +	write_aux_reg(NPS_REG_TIMER0_CTRL, TIMER0_CTRL_IE | TIMER0_CTRL_NH);
> +}
> +
> +/*
> + * Clear from TSI the bit for this thread (if not in periodic mode)
> + * If still there are pending HW treads set next timer event

s/treads/threads/

> + */
> +static void nps_clkevent_rm_thread(bool remove_thread)
> +{
> +	unsigned int cflags;
> +	unsigned int enabled_threads = 0;
> +	int thread;
> +
> +	hw_schd_save(&cflags);

I'm not used with hardware scheduling. Can you explain why this is needed
here ? What window race we want to close ?

> +
> +	enabled_threads = read_aux_reg(NPS_REG_TIMER0_TSI);
> +
> +	/* remove thread from TSI1 */
> +	if (remove_thread) {
> +		thread = read_aux_reg(CTOP_AUX_THREAD_ID);
> +		enabled_threads &= ~(1 << thread);
> +		write_aux_reg(NPS_REG_TIMER0_TSI, enabled_threads);
> +	}
> +
> +	/* Re-arm the timer if needed */
> +	if (!enabled_threads)
> +		write_aux_reg(NPS_REG_TIMER0_CTRL, TIMER0_CTRL_NH);
> +	else
> +		write_aux_reg(NPS_REG_TIMER0_CTRL,
> +			      TIMER0_CTRL_IE | TIMER0_CTRL_NH);
> +
> +	hw_schd_restore(cflags);
> +}
> +
> +static void nps_clkevent_add_thread(bool set_event)
> +{
> +	int thread;
> +	unsigned int cflags, enabled_threads;
> +
> +	hw_schd_save(&cflags);
> +
> +	/* add thread to TSI1 */
> +	thread = read_aux_reg(CTOP_AUX_THREAD_ID);
> +	enabled_threads = read_aux_reg(NPS_REG_TIMER0_TSI);
> +	enabled_threads |= (1 << thread);
> +	write_aux_reg(NPS_REG_TIMER0_TSI, enabled_threads);
> +
> +	/* set next timer event */
> +	if (set_event)
> +		write_aux_reg(NPS_REG_TIMER0_CTRL,
> +			      TIMER0_CTRL_IE | TIMER0_CTRL_NH);
> +
> +	hw_schd_restore(cflags);
> +}

Not sure the boolean parameters for *_rm_thread and *_add_thread helps
to clarify the code. Depending on the race window with hw_schd_save/restore
We should be able to simplify it.

> +static int nps_clkevent_set_next_event(unsigned long delta,
> +				       struct clock_event_device *dev)
> +{
> +	nps_clkevent_add_thread(true);
> +	enable_percpu_irq(nps_timer0_irq, IRQ_TYPE_NONE);
> +
> +	return 0;
> +}
> +
> +/*
> + * Whenever anyone tries to change modes, we just mask interrupts
> + * and wait for the next event to get set.
> + */
> +static int nps_clkevent_timer_shutdown(struct clock_event_device *dev)
> +{
> +	disable_percpu_irq(nps_timer0_irq);
> +
> +	return 0;
> +}
> +
> +/*
> + * For each HW thread set its relevant bit at the TSI register
> + * To arm the timer only thread 0 is needed since it is shared
> + * by all HW threads within same core.
> + */
> +static int nps_clkevent_set_periodic(struct clock_event_device *dev)
> +{
> +	nps_clkevent_add_thread(false);
> +	if (read_aux_reg(CTOP_AUX_THREAD_ID) == 0)
> +		nps_clkevent_timer_event_setup(nps_timer0_freq / HZ);
> +
> +	return 0;
> +}
> +
> +static int nps_clkevent_set_oneshot(struct clock_event_device *dev)
> +{
> +	nps_clkevent_rm_thread(true);
> +	nps_clkevent_timer_shutdown(dev);
> +
> +	return 0;
> +}
> +
> +static DEFINE_PER_CPU(struct clock_event_device, nps_clockevent_device) = {
> +	.name				=	"NPS Timer0",
> +	.features			=	CLOCK_EVT_FEAT_ONESHOT |
> +						CLOCK_EVT_FEAT_PERIODIC,
> +	.rating				=	300,
> +	.set_next_event			=	nps_clkevent_set_next_event,
> +	.set_state_periodic		=	nps_clkevent_set_periodic,
> +	.set_state_oneshot		=	nps_clkevent_set_oneshot,
> +	.set_state_oneshot_stopped	=	nps_clkevent_timer_shutdown,
> +	.set_state_shutdown		=	nps_clkevent_timer_shutdown,

Doesn't set_state_shutdown and set_state_oneshot_stopped need to remove
the HW thread from the TSI ?

> +	.tick_resume			=	nps_clkevent_timer_shutdown,
> +};
> +
> +static irqreturn_t timer_irq_handler(int irq, void *dev_id)
> +{
> +	struct clock_event_device *evt = dev_id;
> +	int irq_reenable = clockevent_state_periodic(evt);
> +
> +	/* Remove HW thread from TSI only if NOT in periodic state */
> +	nps_clkevent_rm_thread(!irq_reenable);
> +
> +	evt->event_handler(evt);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int nps_timer_starting_cpu(unsigned int cpu)
> +{
> +	struct clock_event_device *evt = this_cpu_ptr(&nps_clockevent_device);
> +
> +	evt->cpumask = cpumask_of(smp_processor_id());
> +
> +	clockevents_config_and_register(evt, nps_timer0_freq, 0, ULONG_MAX);
> +	enable_percpu_irq(nps_timer0_irq, IRQ_TYPE_NONE);
> +
> +	return 0;
> +}
> +
> +static int nps_timer_dying_cpu(unsigned int cpu)
> +{
> +	disable_percpu_irq(nps_timer0_irq);
> +	return 0;
> +}
> +
> +static int __init nps_setup_clockevent(struct device_node *node)
> +{
> +	struct clk *clk;
> +	int ret;
> +
> +	nps_timer0_irq = irq_of_parse_and_map(node, 0);
> +	if (nps_timer0_irq <= 0) {
> +		pr_err("clockevent: missing irq");
> +		return -EINVAL;
> +	}
> +
> +	ret = nps_get_timer_clk(node, &nps_timer0_freq, &clk);
> +	if (ret)
> +		return ret;
> +
> +	/* Needs apriori irq_set_percpu_devid() done in intc map function */
> +	ret = request_percpu_irq(nps_timer0_irq, timer_irq_handler,
> +				 "Timer0 (per-cpu-tick)",
> +				 &nps_clockevent_device);
> +	if (ret) {
> +		pr_err("Couldn't request irq\n");
> +		clk_disable_unprepare(clk);
> +		return ret;
> +	}
> +
> +	ret = cpuhp_setup_state(CPUHP_AP_ARC_TIMER_STARTING,
> +				"clockevents/nps:starting",
> +				nps_timer_starting_cpu,
> +				nps_timer_dying_cpu);
> +	if (ret) {
> +		pr_err("Failed to setup hotplug state");
> +		clk_disable_unprepare(clk);
> +		free_percpu_irq(nps_timer0_irq, &nps_clockevent_device);
> +		return ret;
> +	}
> +
> +	return 0;



> +}
> +
> +CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clk_evt, "ezchip,nps400-timer0",
> +		       nps_setup_clockevent);
> +#endif /* CONFIG_EZNPS_MTM_EXT */
> -- 
> 1.7.1
> 

-- 

 <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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v5 3/3] clocksource: Add clockevent support to NPS400 driver

[Noam Camus]

> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] 
> Sent: Monday, November 14, 2016 1:23 PM


>> + */
>> +static void nps_clkevent_rm_thread(bool remove_thread) {
>> +	unsigned int cflags;
>> +	unsigned int enabled_threads = 0;
>> +	int thread;
>> +
>> +	hw_schd_save(&cflags);

>I'm not used with hardware scheduling. Can you explain why this is needed here ? What >window race we want to close ?
We are using HW scheduling off/on in order to keep consistency of auxiliary registers shared among HW threads within the same core.
Example to such registers NPS_REG_TIMER0_TSI and NPS_REG_TIMER0_CTRL.
Since update procedure of these registers is not atomic we use save/restore macros to turn off/on the HW scheduling. This way we insure that no HW scheduling occurs and another HW thread (represented as another CPU) will execute in this same critical code path.
If we take for example nps_clkevent_add_thread() we can see that we are doing some read modify write to NPS_REG_TIMER0_TSI and optionally writing to NPS_REG_TIMER0_CTRL. This flow should be atomic and is protected by our save/restore macros.
Do note that interrupts are disabled at this point so we are safe from all asynchronous events. 

...
>> +static void nps_clkevent_add_thread(bool set_event) {
>> +	int thread;
>> +	unsigned int cflags, enabled_threads;
>> +
>> +	hw_schd_save(&cflags);
>> +
>> +	/* add thread to TSI1 */
>> +	thread = read_aux_reg(CTOP_AUX_THREAD_ID);
>> +	enabled_threads = read_aux_reg(NPS_REG_TIMER0_TSI);
>> +	enabled_threads |= (1 << thread);
>> +	write_aux_reg(NPS_REG_TIMER0_TSI, enabled_threads);
>> +
>> +	/* set next timer event */
>> +	if (set_event)
>> +		write_aux_reg(NPS_REG_TIMER0_CTRL,
>> +			      TIMER0_CTRL_IE | TIMER0_CTRL_NH);
>> +
>> +	hw_schd_restore(cflags);
>> +}

>Not sure the boolean parameters for *_rm_thread and *_add_thread helps to clarify the code. Depending on the race window with hw_schd_save/restore We should be able to simplify it.
I am not sure I am following you here, how race window may simplify this code?
If those routines will get no parameter I can't determine when to add or not (same as remove).
...
>> +
>> +static DEFINE_PER_CPU(struct clock_event_device, nps_clockevent_device) = {
>> +	.name				=	"NPS Timer0",
>> +	.features			=	CLOCK_EVT_FEAT_ONESHOT |
>> +						CLOCK_EVT_FEAT_PERIODIC,
>> +	.rating				=	300,
>> +	.set_next_event			=	nps_clkevent_set_next_event,
>> +	.set_state_periodic		=	nps_clkevent_set_periodic,
>> +	.set_state_oneshot		=	nps_clkevent_set_oneshot,
>> +	.set_state_oneshot_stopped	=	nps_clkevent_timer_shutdown,
>> +	.set_state_shutdown		=	nps_clkevent_timer_shutdown,

>Doesn't set_state_shutdown and set_state_oneshot_stopped need to remove the HW thread from the TSI ?
You are correct, I will fix that.

Thanks
--Noam

Re: [PATCH v5 3/3] clocksource: Add clockevent support to NPS400 driver

[Daniel Lezcano]

On Mon, Nov 14, 2016 at 01:58:15PM +0000, Noam Camus wrote:
> > From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] 
> > Sent: Monday, November 14, 2016 1:23 PM
> 
> 
> >> + */
> >> +static void nps_clkevent_rm_thread(bool remove_thread) {
> >> +	unsigned int cflags;
> >> +	unsigned int enabled_threads = 0;
> >> +	int thread;
> >> +
> >> +	hw_schd_save(&cflags);
> 
> >I'm not used with hardware scheduling. Can you explain why this is needed
> >here ? What >window race we want to close ?
> We are using HW scheduling off/on in order to keep consistency of auxiliary
> registers shared among HW threads within the same core.  Example to such
> registers NPS_REG_TIMER0_TSI and NPS_REG_TIMER0_CTRL.  Since update procedure
> of these registers is not atomic we use save/restore macros to turn off/on
> the HW scheduling. This way we insure that no HW scheduling occurs and
> another HW thread (represented as another CPU) will execute in this same
> critical code path.  If we take for example nps_clkevent_add_thread() we can
> see that we are doing some read modify write to NPS_REG_TIMER0_TSI and
> optionally writing to NPS_REG_TIMER0_CTRL. This flow should be atomic and is
> protected by our save/restore macros.  Do note that interrupts are disabled
> at this point so we are safe from all asynchronous events. 
 
The function nps_clkevent_timer_event_setup() writes into the NPS_REG_TIMER0_CTRL
register but there is no critical section there. What prevents another HW thread
to write this register at the same time ?

I do believe we have a framework to access shared registers, otherwise a simple
spinlock would be simpler and perhaps faster than disabling the entire hardware
scheduling for the system, no ?

> >> +static void nps_clkevent_add_thread(bool set_event) {
> >> +	int thread;
> >> +	unsigned int cflags, enabled_threads;
> >> +
> >> +	hw_schd_save(&cflags);
> >> +
> >> +	/* add thread to TSI1 */
> >> +	thread = read_aux_reg(CTOP_AUX_THREAD_ID);
> >> +	enabled_threads = read_aux_reg(NPS_REG_TIMER0_TSI);
> >> +	enabled_threads |= (1 << thread);
> >> +	write_aux_reg(NPS_REG_TIMER0_TSI, enabled_threads);
> >> +
> >> +	/* set next timer event */
> >> +	if (set_event)
> >> +		write_aux_reg(NPS_REG_TIMER0_CTRL,
> >> +			      TIMER0_CTRL_IE | TIMER0_CTRL_NH);
> >> +
> >> +	hw_schd_restore(cflags);
> >> +}
> 
> >Not sure the boolean parameters for *_rm_thread and *_add_thread helps to
> >clarify the code. Depending on the race window with hw_schd_save/restore We
> >should be able to simplify it.
> I am not sure I am following you here, how race window may simplify this
> code?  If those routines will get no parameter I can't determine when to add
> or not (same as remove).  ...

Regarding the comment I did above, it is possible the critical section is
reduced and moved into the shutdown function. Thus, the boolean wouldn't be
needed anymore, well that is conditional to the above comment. Discard the
comment for the moment, until the hw sched vs spinlock vs NPS_REG_TIMER0_CTRL
is sorted out.

> >> +static DEFINE_PER_CPU(struct clock_event_device, nps_clockevent_device) = {
> >> +	.name				=	"NPS Timer0",
> >> +	.features			=	CLOCK_EVT_FEAT_ONESHOT |
> >> +						CLOCK_EVT_FEAT_PERIODIC,
> >> +	.rating				=	300,
> >> +	.set_next_event			=	nps_clkevent_set_next_event,
> >> +	.set_state_periodic		=	nps_clkevent_set_periodic,
> >> +	.set_state_oneshot		=	nps_clkevent_set_oneshot,
> >> +	.set_state_oneshot_stopped	=	nps_clkevent_timer_shutdown,
> >> +	.set_state_shutdown		=	nps_clkevent_timer_shutdown,
> 
> >Doesn't set_state_shutdown and set_state_oneshot_stopped need to remove the HW thread from the TSI ?
> You are correct, I will fix that.

And tick_resume. Perhaps, that is the reason why NO_HZ hangs.

-- 

 <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

RE: [PATCH v5 3/3] clocksource: Add clockevent support to NPS400 driver

[Noam Camus]

> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] 
> Sent: Monday, November 14, 2016 4:35 PM

 
>The function nps_clkevent_timer_event_setup() writes into the NPS_REG_TIMER0_CTRL register but there is no critical section there. What prevents another HW thread to write this register at the same time ?
Correct, during my last email to you I noticed that fact and already started fixing it.

>I do believe we have a framework to access shared registers, otherwise a simple spinlock would be simpler and perhaps faster than disabling the entire hardware scheduling for the system, no ?
When you are saying "we have a framework" do you mean to some generic framework in the kernel?
Anyway to my understanding I cannot guarantee this atomics during my routines without preventing HW from changing the HW thread this core executes.
As SW I am not aware to such HW scheduling, It is much same as with interrupts that we disable them when we reach code that might be shared by the interrupt handler.

>Regarding the comment I did above, it is possible the critical section is reduced and moved into the shutdown function. Thus, the boolean wouldn't be needed anymore, well that is conditional to the above comment. Discard the comment for the moment, until the hw sched vs spinlock vs NPS_REG_TIMER0_CTRL is sorted out.
OK, I will discard that in the meantime.

...
>> >> +	.set_state_shutdown		=	nps_clkevent_timer_shutdown,
>> 
>> >Doesn't set_state_shutdown and set_state_oneshot_stopped need to remove the HW thread from the TSI ?
>> You are correct, I will fix that.

>And tick_resume. Perhaps, that is the reason why NO_HZ hangs.
What NO_HZ hang are you referring to in this case?
How calling nps_clkevent_rm_thread() explain such hang?
Anyway I agree, and will add nps_clkevent_rm_thread() to tick_resume.

Appreciating your effort and will gladly provide any more information needed about our SoC.
-Noam

Re: [PATCH v5 3/3] clocksource: Add clockevent support to NPS400 driver

[Daniel Lezcano]

On Mon, Nov 14, 2016 at 03:17:48PM +0000, Noam Camus wrote:
> > From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] 
> > Sent: Monday, November 14, 2016 4:35 PM
> 
>  
> >The function nps_clkevent_timer_event_setup() writes into the
> >NPS_REG_TIMER0_CTRL register but there is no critical section there. What
> >prevents another HW thread to write this register at the same time ?
> Correct, during my last email to you I noticed that fact and already started
> fixing it.
> 
> >I do believe we have a framework to access shared registers, otherwise a
> >simple spinlock would be simpler and perhaps faster than disabling the
> >entire hardware scheduling for the system, no ?
> When you are saying "we have a framework" do you mean to some generic
> framework in the kernel? 

Yes, IIRC it is regmap but I'm not sure.

> Anyway to my understanding I cannot guarantee this
> atomics during my routines without preventing HW from changing the HW thread
> this core executes.  As SW I am not aware to such HW scheduling, It is much
> same as with interrupts that we disable them when we reach code that might be
> shared by the interrupt handler.

I think there is something I am missing with this HW scheduling thing. Why are
these hw_schd_save/hw_schd_restore functions needed to be called from the
timer driver ? Regarding the explanation, the HW scheduling can happen everywhere
at any time, not only in the timer code but this one is the only one which need
the hw_schd_save/hw_schd_restore calls, why ?

Why,

spin_lock(&lock);
write_aux_reg(...)
spin_unlock(&lock);

can't work ?

IIUC, there can be more than 16 cpus/threads, so calling hw_schd_save /
hw_schd_restore will disable the HW scheduling for the entire system while one
cpu is processing something with these couple of registers, no ?

> >Regarding the comment I did above, it is possible the critical section is
> >reduced and moved into the shutdown function. Thus, the boolean wouldn't be
> >needed anymore, well that is conditional to the above comment. Discard the
> >comment for the moment, until the hw sched vs spinlock vs
> >NPS_REG_TIMER0_CTRL is sorted out.
> OK, I will discard that in the meantime.
> 
> ...
> >> >> +	.set_state_shutdown		=
> >> >> nps_clkevent_timer_shutdown,
> >> 
> >> >Doesn't set_state_shutdown and set_state_oneshot_stopped need to remove
> >> >the HW thread from the TSI ?
> >> You are correct, I will fix that.
> 
> >And tick_resume. Perhaps, that is the reason why NO_HZ hangs.
> What NO_HZ hang are you referring to in this case?  How calling
> nps_clkevent_rm_thread() explain such hang?  Anyway I agree, and will add
> nps_clkevent_rm_thread() to tick_resume.

Actually I meant NOHZ_FULL.
 
> Appreciating your effort and will gladly provide any more information needed
> about our SoC.  -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

RE: [PATCH v5 3/3] clocksource: Add clockevent support to NPS400 driver

[Noam Camus]

> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] 
> Sent: Monday, November 14, 2016 5:42 PM

>> When you are saying "we have a framework" do you mean to some generic 
>> framework in the kernel?

> Yes, IIRC it is regmap but I'm not sure.
Indeed regmap is a generic framework and it primarily meant for registers which can be mapped to our virtual/logical address space which is not the case here. For our auxiliary registers access we use dedicated instructions (lr/sr) and not LOAD/STORE like GCC produce.
It is possible to use such regmap but this driver will be filled with all regmap handling just to hide couple of lines. This will not serve this driver readability well.

>I think there is something I am missing with this HW scheduling thing. Why are these hw_schd_save/hw_schd_restore functions needed to be called from the timer driver ? Regarding the explanation, the HW scheduling can happen everywhere at any time, not only in the timer code but this one is the only one which need the hw_schd_save/hw_schd_restore calls, why ?
I use them not just here they are also serve to protect our L1 cache and TLB which are also shared within same core. You can't see this yet since patch are not still push to arch/arc tree.
>Why,

>spin_lock(&lock);
>write_aux_reg(...)
>spin_unlock(&lock);

>can't work ?
Because I can't use spinlock in interrupt context (I call to nps_clkevent_rm_thread() in irq_handler).

>IIUC, there can be more than 16 cpus/threads, so calling hw_schd_save / hw_schd_restore will disable the HW scheduling for the entire system while one cpu is processing something with these couple of registers, no ?
NO, HW scheduling will be disabled only for this specific core, all other cores will not be affected since they got their own private registers.

...
>> >And tick_resume. Perhaps, that is the reason why NO_HZ hangs.
>> What NO_HZ hang are you referring to in this case?  How calling
>> nps_clkevent_rm_thread() explain such hang?  Anyway I agree, and will 
>> add
>> nps_clkevent_rm_thread() to tick_resume.

>Actually I meant NOHZ_FULL.
 Still got no clue what hang we are talking about here!

Note:
I looked at arch/tile timer driver again and noticed that I can work without periodic mode. This is exactly what I need here (pure oneshot mode).
With this fact I can define
Static void nps_clkevent_rm_thread(void)
Static void nps_clkevent_add_thread(void)

Also HW scheduling save/restore is only used in *rm_thread/*add_thread since I can now remove nps_clkevent_set_periodic() and nps_clkevent_timer_event_setup().
This way clockevent driver seem much simpler and it is clearer to understanding.
I hope that this approach of not having periodic mode is acceptable.

-Noam

Re: [PATCH v5 3/3] clocksource: Add clockevent support to NPS400 driver

[Daniel Lezcano]

On Mon, Nov 14, 2016 at 04:45:44PM +0000, Noam Camus wrote:
> > From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] Sent: Monday,
> > November 14, 2016 5:42 PM
> 
> >> When you are saying "we have a framework" do you mean to some generic
> >> framework in the kernel?
> 
> > Yes, IIRC it is regmap but I'm not sure.
> Indeed regmap is a generic framework and it primarily meant for registers
> which can be mapped to our virtual/logical address space which is not the
> case here. For our auxiliary registers access we use dedicated instructions
> (lr/sr) and not LOAD/STORE like GCC produce.  It is possible to use such
> regmap but this driver will be filled with all regmap handling just to hide
> couple of lines. This will not serve this driver readability well.

Indeed.
 
> >I think there is something I am missing with this HW scheduling thing. Why
> >are these hw_schd_save/hw_schd_restore functions needed to be called from
> >the timer driver ? Regarding the explanation, the HW scheduling can happen
> >everywhere at any time, not only in the timer code but this one is the only
> >one which need the hw_schd_save/hw_schd_restore calls, why ?
> I use them not just here they are also serve to protect our L1 cache and TLB
> which are also shared within same core. You can't see this yet since patch
> are not still push to arch/arc tree.
> >Why,
> 
> >spin_lock(&lock); write_aux_reg(...) spin_unlock(&lock);
> 
> >can't work ?
> Because I can't use spinlock in interrupt context (I call to
> nps_clkevent_rm_thread() in irq_handler).
> 
> >IIUC, there can be more than 16 cpus/threads, so calling hw_schd_save /
> >hw_schd_restore will disable the HW scheduling for the entire system while
> >one cpu is processing something with these couple of registers, no ?
> NO, HW scheduling will be disabled only for this specific core, all other
> cores will not be affected since they got their own private registers.
> 
> ...
> >> >And tick_resume. Perhaps, that is the reason why NO_HZ hangs.
> >> What NO_HZ hang are you referring to in this case?  How calling
> >> nps_clkevent_rm_thread() explain such hang?  Anyway I agree, and will add
> >> nps_clkevent_rm_thread() to tick_resume.
> 
> >Actually I meant NOHZ_FULL.
>  Still got no clue what hang we are talking about here!

Never mind, I read in a previous email from v2 "hanging" instead of "handling".
 
> Note: I looked at arch/tile timer driver again and noticed that I can work
> without periodic mode. This is exactly what I need here (pure oneshot mode).
> With this fact I can define Static void nps_clkevent_rm_thread(void) Static
> void nps_clkevent_add_thread(void)
> 
> Also HW scheduling save/restore is only used in *rm_thread/*add_thread since
> I can now remove nps_clkevent_set_periodic() and
> nps_clkevent_timer_event_setup().  This way clockevent driver seem much
> simpler and it is clearer to understanding.  I hope that this approach of not
> having periodic mode is acceptable.

AFAICT, oneshot mode is more accurate than periodic mode. The time framework
will take care of emulating the periodic timer. There are several timers in
drivers/clocksource which are oneshot more only.

-- 

 <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