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

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

[Noam Camus]

From: Noam Camus <noamca@mellanox.com>

Change log
---
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                    |  253 ++++++++++++++++++--
 include/linux/cpuhotplug.h                         |    1 +
 include/soc/nps/mtm.h                              |   59 +++++
 7 files changed, 325 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 v2 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 acheived 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

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

[Daniel Lezcano]

On Sun, Oct 23, 2016 at 03:12:26PM +0300, Noam Camus wrote:
> 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 acheived by writing core registers.

s/acheived/achieved/

> 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");
> +}
> +

Wouldn't make sense to change the macro name to CTOP_INST_HWSCHD_SAVE_R3 ?

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

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

[Noam Camus]

> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] 
> Sent: Monday, October 31, 2016 12:27 PM
>> 
>> 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 acheived by writing core registers.

>s/acheived/achieved/
Thanks will update in V4 of this patch set
...

>> +#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");
>> +}
>> +

>Wouldn't make sense to change the macro name to CTOP_INST_HWSCHD_SAVE_R3 ?
The save of state into register (R3) by this dedicated instruction is only part of action which main purpose
Is to turn off the HW scheduler within this core (we call CTOP).
We use it  (Off/On) at places we wish to keep consistency of data where more than one HW thread can access.
So I believe this way macro name reflects properly the functionality of this instruction and the API functions
schd_save()/schd_restore() provide similar format like with IRQs.

> +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 v2 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 |   49 ++++++++++++++++++++------------------
 1 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/drivers/clocksource/timer-nps.c b/drivers/clocksource/timer-nps.c
index 70c149a..6156e54 100644
--- a/drivers/clocksource/timer-nps.c
+++ b/drivers/clocksource/timer-nps.c
@@ -47,6 +47,28 @@
 static void *nps_msu_reg_low_addr[NPS_CLUSTER_NUM] __read_mostly;
 
 static unsigned long nps_timer_rate;
+static int 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);
+
+	return 0;
+}
 
 static cycle_t nps_clksrc_read(struct clocksource *clksrc)
 {
@@ -55,23 +77,17 @@ 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;
 
 	for (cluster = 0; cluster < NPS_CLUSTER_NUM; cluster++)
 		nps_msu_reg_low_addr[cluster] =
 			nps_host_reg((cluster << NPS_CLUSTER_OFFSET),
 				 NPS_MSU_BLKID, NPS_MSU_TICK_LOW);
 
-	ret = clk_prepare_enable(clk);
-	if (ret) {
-		pr_err("Couldn't enable parent clock\n");
-		return ret;
-	}
-
-	nps_timer_rate = clk_get_rate(clk);
+	nps_get_timer_clk(node, &nps_timer_rate, clk);
 
 	ret = clocksource_mmio_init(nps_msu_reg_low_addr, "EZnps-tick",
 				    nps_timer_rate, 301, 32, nps_clksrc_read);
@@ -83,18 +99,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

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

[Daniel Lezcano]

On Sun, Oct 23, 2016 at 03:12:27PM +0300, 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)
> 
> This is also serve as preperation for next patch which adds support

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 |   49 ++++++++++++++++++++------------------
>  1 files changed, 26 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-nps.c b/drivers/clocksource/timer-nps.c
> index 70c149a..6156e54 100644
> --- a/drivers/clocksource/timer-nps.c
> +++ b/drivers/clocksource/timer-nps.c
> @@ -47,6 +47,28 @@
>  static void *nps_msu_reg_low_addr[NPS_CLUSTER_NUM] __read_mostly;
>  
>  static unsigned long nps_timer_rate;
> +static int nps_get_timer_clk(struct device_node *node,
> +			     unsigned long *timer_freq,
> +			     struct clk *clk)

This function prototype does not make sense. A pointer to a clock is passed
for nothing here.

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

	timer_freq check.

	rollback on error.

> +	return 0;
> +}
>  
>  static cycle_t nps_clksrc_read(struct clocksource *clksrc)
>  {
> @@ -55,23 +77,17 @@ 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;
>  
>  	for (cluster = 0; cluster < NPS_CLUSTER_NUM; cluster++)
>  		nps_msu_reg_low_addr[cluster] =
>  			nps_host_reg((cluster << NPS_CLUSTER_OFFSET),
>  				 NPS_MSU_BLKID, NPS_MSU_TICK_LOW);
>  
> -	ret = clk_prepare_enable(clk);
> -	if (ret) {
> -		pr_err("Couldn't enable parent clock\n");
> -		return ret;
> -	}
> -
> -	nps_timer_rate = clk_get_rate(clk);
> +	nps_get_timer_clk(node, &nps_timer_rate, clk);

Return code check ?
  
>  	ret = clocksource_mmio_init(nps_msu_reg_low_addr, "EZnps-tick",
>  				    nps_timer_rate, 301, 32, nps_clksrc_read);
> @@ -83,18 +99,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

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

[Noam Camus]

> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] 
> Sent: Monday, October 31, 2016 12:28 PM

>> 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
>
>s/preperation/preparation/
Thanks, will fix in V4 of this patch set
...

>> +static int nps_get_timer_clk(struct device_node *node,
>> +			     unsigned long *timer_freq,
>> +			     struct clk *clk)
>
>This function prototype does not make sense. A pointer to a clock is passed for nothing here.
Thanks, I passed *clk in order for one to do rollback on error (pass clk to clk_disable_unprepare).
I will change prototype to **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);
>> +
>
>	timer_freq check.
>
>	rollback on error.
Thanks, will fix at V4

...
>> -	if (ret) {
>> -		pr_err("Couldn't enable parent clock\n");
>> -		return ret;
>> -	}
>> -
>> -	nps_timer_rate = clk_get_rate(clk);
>> +	nps_get_timer_clk(node, &nps_timer_rate, clk);

>Return code check ?
Thanks, will fix at V4
  
-Noam

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

[Noam Camus]

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

Signed-off-by: Noam Camus <noamca-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Change-Id: Ib351e6fc7a6b691293040ae655f202f3cc2c1298
---
 .../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                    |  220 +++++++++++++++++++-
 include/linux/cpuhotplug.h                         |    1 +
 5 files changed, 248 insertions(+), 20 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 6156e54..0757328 100644
--- a/drivers/clocksource/timer-nps.c
+++ b/drivers/clocksource/timer-nps.c
@@ -46,7 +46,7 @@
 /* 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 unsigned long nps_timer1_freq;
 static int nps_get_timer_clk(struct device_node *node,
 			     unsigned long *timer_freq,
 			     struct clk *clk)
@@ -87,10 +87,10 @@ static int __init nps_setup_clocksource(struct device_node *node)
 			nps_host_reg((cluster << NPS_CLUSTER_OFFSET),
 				 NPS_MSU_BLKID, NPS_MSU_TICK_LOW);
 
-	nps_get_timer_clk(node, &nps_timer_rate, clk);
+	nps_get_timer_clk(node, &nps_timer1_freq, 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, 301, 32, nps_clksrc_read);
 	if (ret) {
 		pr_err("Couldn't register clock source.\n");
 		clk_disable_unprepare(clk);
@@ -99,5 +99,215 @@ static int __init nps_setup_clocksource(struct device_node *node)
 	return ret;
 }
 
-CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clksrc, "ezchip,nps400-timer",
+CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clksrc, "ezchip,nps400-timer1",
 		       nps_setup_clocksource);
+
+#ifdef CONFIG_EZNPS_MTM_EXT
+#include <soc/nps/mtm.h>
+
+/* Timer related Aux registers */
+#define AUX_REG_TIMER0_TSI	0xFFFFF850	/* timer 0 HW threads mask */
+#define NPS_REG_TIMER0_LIMIT	0x23		/* timer 0 limit */
+#define NPS_REG_TIMER0_CTRL	0x22		/* timer 0 control */
+#define NPS_REG_TIMER0_CNT	0x21		/* timer 0 count */
+
+#define TIMER0_CTRL_IE	(1 << 0) /* Interrupt when Count reaches limit */
+#define TIMER0_CTRL_NH	(1 << 1) /* Count only when CPU NOT halted */
+
+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);   /* start from 0 */
+
+	write_aux_reg(NPS_REG_TIMER0_CTRL, TIMER0_CTRL_IE | TIMER0_CTRL_NH);
+}
+
+static void nps_clkevent_rm_thread(bool remove_thread)
+{
+	unsigned int cflags;
+	unsigned int enabled_threads;
+	unsigned long flags;
+	int thread;
+
+	local_irq_save(flags);
+	hw_schd_save(&cflags);
+
+	enabled_threads = read_aux_reg(AUX_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(AUX_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);
+	local_irq_restore(flags);
+}
+
+static void nps_clkevent_add_thread(bool set_event)
+{
+	int thread;
+	unsigned int cflags, enabled_threads;
+	unsigned long flags;
+
+	local_irq_save(flags);
+	hw_schd_save(&cflags);
+
+	/* add thread to TSI1 */
+	thread = read_aux_reg(CTOP_AUX_THREAD_ID);
+	enabled_threads = read_aux_reg(AUX_REG_TIMER0_TSI);
+	enabled_threads |= (1 << thread);
+	write_aux_reg(AUX_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);
+	local_irq_restore(flags);
+}
+
+static int nps_clkevent_set_next_event(unsigned long delta,
+				       struct clock_event_device *dev)
+{
+	struct irq_desc *desc = irq_to_desc(nps_timer0_irq);
+	struct irq_chip *chip = irq_data_get_irq_chip(&desc->irq_data);
+
+	nps_clkevent_add_thread(true);
+	chip->irq_unmask(&desc->irq_data);
+
+	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)
+{
+	struct irq_desc *desc = irq_to_desc(nps_timer0_irq);
+	struct irq_chip *chip = irq_data_get_irq_chip(&desc->irq_data);
+
+	chip->irq_mask(&desc->irq_data);
+
+	return 0;
+}
+
+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)
+{
+	/*
+	 * Note that generic IRQ core could have passed @evt for @dev_id if
+	 * irq_set_chip_and_handler() asked for handle_percpu_devid_irq()
+	 */
+	struct clock_event_device *evt = this_cpu_ptr(&nps_clockevent_device);
+	int irq_reenable = clockevent_state_periodic(evt);
+
+	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, 0);
+
+	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 clock_event_device *evt = this_cpu_ptr(&nps_clockevent_device);
+	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;
+	}
+
+	nps_get_timer_clk(node, &nps_timer0_freq, clk);
+
+	/* 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)", evt);
+	if (ret) {
+		pr_err("Couldn't request irq\n");
+		clk_disable_unprepare(clk);
+		return ret;
+	}
+
+	ret = cpuhp_setup_state(CPUHP_AP_NPS_TIMER_STARTING,
+				"AP_NPS_TIMER_STARTING",
+				nps_timer_starting_cpu,
+				nps_timer_dying_cpu);
+	if (ret) {
+		pr_err("Failed to setup hotplug state");
+		clk_disable_unprepare(clk);
+		return ret;
+	}
+
+	return 0;
+}
+
+CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clkevt, "ezchip,nps400-timer0",
+		       nps_setup_clockevent);
+#endif /* CONFIG_EZNPS_MTM_EXT */
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 34bd805..9efc1a3 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -60,6 +60,7 @@ enum cpuhp_state {
 	CPUHP_AP_MARCO_TIMER_STARTING,
 	CPUHP_AP_MIPS_GIC_TIMER_STARTING,
 	CPUHP_AP_ARC_TIMER_STARTING,
+	CPUHP_AP_NPS_TIMER_STARTING,
 	CPUHP_AP_KVM_STARTING,
 	CPUHP_AP_KVM_ARM_VGIC_INIT_STARTING,
 	CPUHP_AP_KVM_ARM_VGIC_STARTING,
-- 
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

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

[Rob Herring]

On Sun, Oct 23, 2016 at 03:12:28PM +0300, 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
> (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.
> 
> Signed-off-by: Noam Camus <noamca@mellanox.com>
> 
> Change-Id: Ib351e6fc7a6b691293040ae655f202f3cc2c1298
> ---
>  .../bindings/timer/ezchip,nps400-timer.txt         |   15 --
>  .../bindings/timer/ezchip,nps400-timer0.txt        |   17 ++
>  .../bindings/timer/ezchip,nps400-timer1.txt        |   15 ++

For the binding,

Acked-by: Rob Herring <robh@kernel.org>

However, one issue below...

>  drivers/clocksource/timer-nps.c                    |  220 +++++++++++++++++++-
>  include/linux/cpuhotplug.h                         |    1 +
>  5 files changed, 248 insertions(+), 20 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/drivers/clocksource/timer-nps.c b/drivers/clocksource/timer-nps.c
> index 6156e54..0757328 100644
> --- a/drivers/clocksource/timer-nps.c
> +++ b/drivers/clocksource/timer-nps.c
> @@ -46,7 +46,7 @@
>  /* 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 unsigned long nps_timer1_freq;
>  static int nps_get_timer_clk(struct device_node *node,
>  			     unsigned long *timer_freq,
>  			     struct clk *clk)
> @@ -87,10 +87,10 @@ static int __init nps_setup_clocksource(struct device_node *node)
>  			nps_host_reg((cluster << NPS_CLUSTER_OFFSET),
>  				 NPS_MSU_BLKID, NPS_MSU_TICK_LOW);
>  
> -	nps_get_timer_clk(node, &nps_timer_rate, clk);
> +	nps_get_timer_clk(node, &nps_timer1_freq, 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, 301, 32, nps_clksrc_read);
>  	if (ret) {
>  		pr_err("Couldn't register clock source.\n");
>  		clk_disable_unprepare(clk);
> @@ -99,5 +99,215 @@ static int __init nps_setup_clocksource(struct device_node *node)
>  	return ret;
>  }
>  
> -CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clksrc, "ezchip,nps400-timer",
> +CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clksrc, "ezchip,nps400-timer1",
>  		       nps_setup_clocksource);

You should keep the old string here for backwards compatiblity.

Rob

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

[Daniel Lezcano]

On Sun, Oct 23, 2016 at 03:12:28PM +0300, 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/

Hey ! re-read yourself.

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

I'm not sure to understand this sentence. Do you mean each cpu/thread must
set a flag in the TSI register at BIT(phys_id) when they set a timer ?

> When timer expires all HW threads within this core which their bit
> is set at the TSI register will be interrupted.

Does it mean some HW threads will be wake up for nothing ?

eg. 

HWT1 sets the timer to expire within 2 seconds
HWT2 sets the timer to expire within 2 hours

When the first timer expires, that will wake up HWT1 *and* HWT2 ?

The code is a bit confusing because of the very specific design of this timer.

Below more questions for clarification.

> 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.
> 
> Signed-off-by: Noam Camus <noamca@mellanox.com>
> 
> Change-Id: Ib351e6fc7a6b691293040ae655f202f3cc2c1298
> ---
>  .../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                    |  220 +++++++++++++++++++-
>  include/linux/cpuhotplug.h                         |    1 +
>  5 files changed, 248 insertions(+), 20 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 6156e54..0757328 100644
> --- a/drivers/clocksource/timer-nps.c
> +++ b/drivers/clocksource/timer-nps.c
> @@ -46,7 +46,7 @@
>  /* 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 unsigned long nps_timer1_freq;

Why declare a global static variable for a local use in nps_setup_clocksource ? 

>  static int nps_get_timer_clk(struct device_node *node,
>  			     unsigned long *timer_freq,
>  			     struct clk *clk)
> @@ -87,10 +87,10 @@ static int __init nps_setup_clocksource(struct device_node *node)
>  			nps_host_reg((cluster << NPS_CLUSTER_OFFSET),
>  				 NPS_MSU_BLKID, NPS_MSU_TICK_LOW);
>  
> -	nps_get_timer_clk(node, &nps_timer_rate, clk);
> +	nps_get_timer_clk(node, &nps_timer1_freq, 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, 301, 32, nps_clksrc_read);
>  	if (ret) {
>  		pr_err("Couldn't register clock source.\n");
>  		clk_disable_unprepare(clk);
> @@ -99,5 +99,215 @@ static int __init nps_setup_clocksource(struct device_node *node)
>  	return ret;
>  }
>  
> -CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clksrc, "ezchip,nps400-timer",
> +CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clksrc, "ezchip,nps400-timer1",
>  		       nps_setup_clocksource);
> +
> +#ifdef CONFIG_EZNPS_MTM_EXT
> +#include <soc/nps/mtm.h>
> +
> +/* Timer related Aux registers */
> +#define AUX_REG_TIMER0_TSI	0xFFFFF850	/* timer 0 HW threads mask */
> +#define NPS_REG_TIMER0_LIMIT	0x23		/* timer 0 limit */
> +#define NPS_REG_TIMER0_CTRL	0x22		/* timer 0 control */
> +#define NPS_REG_TIMER0_CNT	0x21		/* timer 0 count */
> +
> +#define TIMER0_CTRL_IE	(1 << 0) /* Interrupt when Count reaches limit */
> +#define TIMER0_CTRL_NH	(1 << 1) /* Count only when CPU NOT halted */

Please, use BIT(nr) macro.

Can you elaborate "Count only when CPU NOT halted" ?

> +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);   /* start from 0 */
> +
> +	write_aux_reg(NPS_REG_TIMER0_CTRL, TIMER0_CTRL_IE | TIMER0_CTRL_NH);
> +}
> +
> +static void nps_clkevent_rm_thread(bool remove_thread)
> +{
> +	unsigned int cflags;
> +	unsigned int enabled_threads;
> +	unsigned long flags;
> +	int thread;
> +
> +	local_irq_save(flags);
> +	hw_schd_save(&cflags);

Can you explain why those two lines are needed ?

> +	enabled_threads = read_aux_reg(AUX_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(AUX_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);
> +	local_irq_restore(flags);
> +}
> +
> +static void nps_clkevent_add_thread(bool set_event)
> +{
> +	int thread;
> +	unsigned int cflags, enabled_threads;
> +	unsigned long flags;
> +
> +	local_irq_save(flags);
> +	hw_schd_save(&cflags);
> +
> +	/* add thread to TSI1 */
> +	thread = read_aux_reg(CTOP_AUX_THREAD_ID);
> +	enabled_threads = read_aux_reg(AUX_REG_TIMER0_TSI);
> +	enabled_threads |= (1 << thread);
> +	write_aux_reg(AUX_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);
> +	local_irq_restore(flags);
> +}
> +
> +static int nps_clkevent_set_next_event(unsigned long delta,
> +				       struct clock_event_device *dev)
> +{
> +	struct irq_desc *desc = irq_to_desc(nps_timer0_irq);
> +	struct irq_chip *chip = irq_data_get_irq_chip(&desc->irq_data);
> +
> +	nps_clkevent_add_thread(true);
> +	chip->irq_unmask(&desc->irq_data);

Can you explain why invoking low level IRQ callbacks is needed here ?

> +	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)
> +{
> +	struct irq_desc *desc = irq_to_desc(nps_timer0_irq);
> +	struct irq_chip *chip = irq_data_get_irq_chip(&desc->irq_data);
> +
> +	chip->irq_mask(&desc->irq_data);
> +
> +	return 0;
> +}
> +
> +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);

Please explain this. I read only CPU0 can set the periodic timer.

> +	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)
> +{
> +	/*
> +	 * Note that generic IRQ core could have passed @evt for @dev_id if
> +	 * irq_set_chip_and_handler() asked for handle_percpu_devid_irq()
> +	 */
> +	struct clock_event_device *evt = this_cpu_ptr(&nps_clockevent_device);
> +	int irq_reenable = clockevent_state_periodic(evt);
> +
> +	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, 0);
> +
> +	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 clock_event_device *evt = this_cpu_ptr(&nps_clockevent_device);
> +	struct clk *clk;

clk = 0xDEADBEEF

> +	int ret;
> +
> +	nps_timer0_irq = irq_of_parse_and_map(node, 0);
> +	if (nps_timer0_irq <= 0) {
> +		pr_err("clockevent: missing irq");
> +		return -EINVAL;
> +	}
> +
> +	nps_get_timer_clk(node, &nps_timer0_freq, clk);
> +
> +	/* 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)", evt);
> +	if (ret) {
> +		pr_err("Couldn't request irq\n");
> +		clk_disable_unprepare(clk);

clk is on the stack, hence returning back from the function, clk is undefined.

clk_disable_unprepare(0xDEADBEEF) ==> kernel panic

It does not make sense to add the nps_get_timer_clk() function.

Better to have a couple of duplicated lines and properly rollback
from the right place instead of rollbacking supposed actions taken
from inside a function.

> +		return ret;
> +	}
> +
> +	ret = cpuhp_setup_state(CPUHP_AP_NPS_TIMER_STARTING,
> +				"AP_NPS_TIMER_STARTING",
> +				nps_timer_starting_cpu,
> +				nps_timer_dying_cpu);
> +	if (ret) {
> +		pr_err("Failed to setup hotplug state");
> +		clk_disable_unprepare(clk);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clkevt, "ezchip,nps400-timer0",
> +		       nps_setup_clockevent);
> +#endif /* CONFIG_EZNPS_MTM_EXT */
> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> index 34bd805..9efc1a3 100644
> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -60,6 +60,7 @@ enum cpuhp_state {
>  	CPUHP_AP_MARCO_TIMER_STARTING,
>  	CPUHP_AP_MIPS_GIC_TIMER_STARTING,
>  	CPUHP_AP_ARC_TIMER_STARTING,
> +	CPUHP_AP_NPS_TIMER_STARTING,

Oops, wait. Here, arch/arc/kernel/time.c should be moved in
drivers/clocksource and consolidated with this driver.

Very likely, CPUHP_AP_ARC_TIMER_STARTING can be used for all ARC
timers.

>  	CPUHP_AP_KVM_STARTING,
>  	CPUHP_AP_KVM_ARM_VGIC_INIT_STARTING,
>  	CPUHP_AP_KVM_ARM_VGIC_STARTING,
> -- 
> 1.7.1
> 

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

[Noam Camus]

>From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] 
>Sent: Monday, October 31, 2016 12:53 PM

>> 
>> The design idea is that for each core there is dedicated regirtser

>s/regirtser/register/

>Hey ! re-read yourself.
Thanks, I do but sometimes reading over and over and still left with such typos
Will Fix on next patch Set V4

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

>I'm not sure to understand this sentence. Do you mean each cpu/thread must set a flag in the TSI register at BIT(phys_id) when they set a timer ?
Correct, each thread needs to set its bit in TSI register, otherwise the he won't be interrupted by timer.

>> When timer expires all HW threads within this core which their bit is 
>> set at the TSI register will be interrupted.

>Does it mean some HW threads will be wake up for nothing ?
See below after the example you gave

>eg. 

>HWT1 sets the timer to expire within 2 seconds
>HWT2 sets the timer to expire within 2 hours

>When the first timer expires, that will wake up HWT1 *and* HWT2 ?

You are correct, indeed not optimal but much simpler than managing book keeping of all 16 threads.
Functionality wise one who registered this timer will notice that it expired too soon and will register a new one.
This is how it works now in our machines.

>The code is a bit confusing because of the very specific design of this timer.

>Below more questions for clarification.

...
>> diff --git a/drivers/clocksource/timer-nps.c 
>> b/drivers/clocksource/timer-nps.c index 6156e54..0757328 100644
>> --- a/drivers/clocksource/timer-nps.c
>> +++ b/drivers/clocksource/timer-nps.c
>> @@ -46,7 +46,7 @@
>>  /* 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 unsigned long nps_timer1_freq;
>
>Why declare a global static variable for a local use in nps_setup_clocksource ? 
Indeed no need. It will be fixed at V4

...
>> +/* Timer related Aux registers */
>> +#define AUX_REG_TIMER0_TSI	0xFFFFF850	/* timer 0 HW threads mask */
>> +#define NPS_REG_TIMER0_LIMIT	0x23		/* timer 0 limit */
>> +#define NPS_REG_TIMER0_CTRL	0x22		/* timer 0 control */
>> +#define NPS_REG_TIMER0_CNT	0x21		/* timer 0 count */
>> +
>> +#define TIMER0_CTRL_IE	(1 << 0) /* Interrupt when Count reaches limit */
>> +#define TIMER0_CTRL_NH	(1 << 1) /* Count only when CPU NOT halted */

>Please, use BIT(nr) macro.
Will fix that on V4.

>Can you elaborate "Count only when CPU NOT halted" ?
The Idea here is:
The Not Halted mode flag (NH) causes cycles to be counted only when the processor is running (not halted). When set to 0 the timer will count every clock cycle. When set to 1 the timer will only count when the processor is running. The NH flag is set to 0 when the processor is Reset.
It may be used when working with JTAG (I never used it this way though).

>> +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);   /* start from 0 */
>> +
>> +	write_aux_reg(NPS_REG_TIMER0_CTRL, TIMER0_CTRL_IE | TIMER0_CTRL_NH); 
>> +}
>> +
>> +static void nps_clkevent_rm_thread(bool remove_thread) {
>> +	unsigned int cflags;
>> +	unsigned int enabled_threads;
>> +	unsigned long flags;
>> +	int thread;
>> +
>> +	local_irq_save(flags);
>> +	hw_schd_save(&cflags);
>
>Can you explain why those two lines are needed ?
The idea is that access to shared core registers (among threads) is not done in parallel to keep their consistency.
For example Read Modified Write of TSI register is not atomic, so using two lines above avoid any interference during
this code execution.

...
>> +
>> +static int nps_clkevent_set_next_event(unsigned long delta,
>> +				       struct clock_event_device *dev) {
>> +	struct irq_desc *desc = irq_to_desc(nps_timer0_irq);
>> +	struct irq_chip *chip = irq_data_get_irq_chip(&desc->irq_data);
>> +
>> +	nps_clkevent_add_thread(true);
>> +	chip->irq_unmask(&desc->irq_data);

>Can you explain why invoking low level IRQ callbacks is needed here ?
I needed those callbacks functionality and didn't want to duplicate it here.

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

>Please explain this. I read only CPU0 can set the periodic timer.
When system works in periodic mode for clock events we just need to set for all HW threads within same core their respective bit at TSI register. 
We also need but only once to arm the shared timer control register.
Since that for each core thread 0 is always available we choose HW thread 0 to do that.
...
>> +
>> +static int __init nps_setup_clockevent(struct device_node *node) {
>> +	struct clock_event_device *evt = this_cpu_ptr(&nps_clockevent_device);
>> +	struct clk *clk;

>clk = 0xDEADBEEF
>> +	int ret;
>> +
>> +	nps_timer0_irq = irq_of_parse_and_map(node, 0);
>> +	if (nps_timer0_irq <= 0) {
>> +		pr_err("clockevent: missing irq");
>> +		return -EINVAL;
>> +	}
>> +
>> +	nps_get_timer_clk(node, &nps_timer0_freq, clk);
>> +
>> +	/* 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)", evt);
>> +	if (ret) {
>> +		pr_err("Couldn't request irq\n");
>> +		clk_disable_unprepare(clk);

>clk is on the stack, hence returning back from the function, clk is undefined.

>clk_disable_unprepare(0xDEADBEEF) ==> kernel panic

>It does not make sense to add the nps_get_timer_clk() function.

>Better to have a couple of duplicated lines and properly rollback from the right place instead of rollbacking supposed actions taken from inside a function.
As I wrote above I will use **clk for the rollback, so nps_get_timer_clk() will make sense and avoid code duplication.

>> +		return ret;
>> +	}
>> +
>> +	ret = cpuhp_setup_state(CPUHP_AP_NPS_TIMER_STARTING,
>> +				"AP_NPS_TIMER_STARTING",
>> +				nps_timer_starting_cpu,
>> +				nps_timer_dying_cpu);
>> +	if (ret) {
>> +		pr_err("Failed to setup hotplug state");
>> +		clk_disable_unprepare(clk);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clkevt, "ezchip,nps400-timer0",
>> +		       nps_setup_clockevent);
>> +#endif /* CONFIG_EZNPS_MTM_EXT */
>> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h 
>> index 34bd805..9efc1a3 100644
>> --- a/include/linux/cpuhotplug.h
>> +++ b/include/linux/cpuhotplug.h
>> @@ -60,6 +60,7 @@ enum cpuhp_state {
>> 	CPUHP_AP_MARCO_TIMER_STARTING,
>>  	CPUHP_AP_MIPS_GIC_TIMER_STARTING,
>>  	CPUHP_AP_ARC_TIMER_STARTING,
>> +	CPUHP_AP_NPS_TIMER_STARTING,

>Oops, wait. Here, arch/arc/kernel/time.c should be moved in drivers/clocksource and consolidated with this driver.

>Very likely, CPUHP_AP_ARC_TIMER_STARTING can be used for all ARC timers.

Indeed the ARC timer driver served as my inspiration but due to HW threads handling they are not the same.
Moving drivers from arch/arc to driver/clocksource is not my call (Vineet Gupta is the maintainer of ARC)
And I think they quiet differ now so consolidation gain is not obvious.

-Noam 

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

[Vineet Gupta]

Hi Daniel,

On 10/31/2016 10:03 AM, Noam Camus wrote:
>> Oops, wait. Here, arch/arc/kernel/time.c should be moved in drivers/clocksource and consolidated with this driver.
>> >Very likely, CPUHP_AP_ARC_TIMER_STARTING can be used for all ARC timers.
> Indeed the ARC timer driver served as my inspiration but due to HW threads handling they are not the same.
> Moving drivers from arch/arc to driver/clocksource is not my call (Vineet Gupta is the maintainer of ARC)
> And I think they quiet differ now so consolidation gain is not obvious.

Sorry this was on my todo list at low priority. Anyways took a quick look at it now.

The problem is timer code overlaps pure driver land and ARC specific code. e.g.
CONFIG_ARC_HAS_RTC clocksource needs to check whether hardware exists or not. We
currently conveniently check that from cpuinfo populated at boot time. Now either
I need to export that info as a function (which should be done anyways) - but then
define stub for !ARC. Either that or duplicate the detection code here (orig copy
in arc setup.c)

So doing this exercise cleanly would mean exporting much of
arch/arc/include/asm/{arcregs.h, mcip.h} out of ARC into include/soc/arc/*. Isn't
this kind of going back to include/asm-<arch>/* days ?

Honestly I do want to move the driver in the right place but not sure how to do
this cleanly.

Thoughts ?

-Vineet

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

[Daniel Lezcano]

On Mon, Oct 31, 2016 at 05:03:40PM +0000, Noam Camus wrote:
> >From: Daniel Lezcano [mailto:daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org] 
> >Sent: Monday, October 31, 2016 12:53 PM
> 
> >> 
> >> The design idea is that for each core there is dedicated regirtser
> 
> >s/regirtser/register/
> 
> >Hey ! re-read yourself.
> Thanks, I do but sometimes reading over and over and still left with such typos
> Will Fix on next patch Set V4
> 
> >> (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.
> 
> >I'm not sure to understand this sentence. Do you mean each cpu/thread must
> >set a flag in the TSI register at BIT(phys_id) when they set a timer ?
> Correct, each thread needs to set its bit in TSI register, otherwise the he
> won't be interrupted by timer.
> 
> >> When timer expires all HW threads within this core which their bit is 
> >> set at the TSI register will be interrupted.
> 
> >Does it mean some HW threads will be wake up for nothing ?
> See below after the example you gave
> 
> >eg. 
> 
> >HWT1 sets the timer to expire within 2 seconds
> >HWT2 sets the timer to expire within 2 hours
> 
> >When the first timer expires, that will wake up HWT1 *and* HWT2 ?
> 
> You are correct, indeed not optimal but much simpler than managing book
> keeping of all 16 threads.  Functionality wise one who registered this timer
> will notice that it expired too soon and will register a new one.  This is how
> it works now in our machines.

Assuming cpu0 and cpu1 are sibling, does

taskset 0x1 time sleep 2 & taskset 0x2 time sleep 3

give a correct result without a dmesg log ?

Can you give the content of the /proc/timer_list ?

I'm not sure it is safe to have this kind of spurious interrupt for the time
framework.

> >The code is a bit confusing because of the very specific design of this timer.
> 
> >Below more questions for clarification.
> 
> ...
> >> diff --git a/drivers/clocksource/timer-nps.c 
> >> b/drivers/clocksource/timer-nps.c index 6156e54..0757328 100644
> >> --- a/drivers/clocksource/timer-nps.c
> >> +++ b/drivers/clocksource/timer-nps.c
> >> @@ -46,7 +46,7 @@
> >>  /* 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 unsigned long nps_timer1_freq;
> >
> >Why declare a global static variable for a local use in nps_setup_clocksource ? 
> Indeed no need. It will be fixed at V4
> 
> ...
> >> +/* Timer related Aux registers */
> >> +#define AUX_REG_TIMER0_TSI	0xFFFFF850	/* timer 0 HW threads mask */
> >> +#define NPS_REG_TIMER0_LIMIT	0x23		/* timer 0 limit */
> >> +#define NPS_REG_TIMER0_CTRL	0x22		/* timer 0 control */
> >> +#define NPS_REG_TIMER0_CNT	0x21		/* timer 0 count */
> >> +
> >> +#define TIMER0_CTRL_IE	(1 << 0) /* Interrupt when Count reaches limit */
> >> +#define TIMER0_CTRL_NH	(1 << 1) /* Count only when CPU NOT halted */
> 
> >Please, use BIT(nr) macro.
> Will fix that on V4.
> 
> >Can you elaborate "Count only when CPU NOT halted" ?
> The Idea here is:
> The Not Halted mode flag (NH) causes cycles to be counted only when the
> processor is running (not halted). When set to 0 the timer will count every
> clock cycle. When set to 1 the timer will only count when the processor is
> running. The NH flag is set to 0 when the processor is Reset.

When is the processor halted ? At idle time ?

There is an inconsistency here:

 - If the power management of the CPU allows to power it down and the timer
   belongs to the same power domain to the CPU, then it will be powered down
   also. In this case, the C3STOP flag must be used and the CPU wakeup must be
   delegated to a backup timer.

 - If there is no power management on the CPU. The timer must continue to
   count cycles in order to wake up the CPU if it is halted (assuming it
   is clock gated).

In both cases, TIMER0_CTRL_NH is not needed.

Or is the CPU halted *only* when debugging it ?

> It may be used when working with JTAG (I never used it this way though).
> 
> >> +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);   /* start from 0 */
> >> +
> >> +	write_aux_reg(NPS_REG_TIMER0_CTRL, TIMER0_CTRL_IE | TIMER0_CTRL_NH); 
> >> +}
> >> +
> >> +static void nps_clkevent_rm_thread(bool remove_thread) {
> >> +	unsigned int cflags;
> >> +	unsigned int enabled_threads;
> >> +	unsigned long flags;
> >> +	int thread;
> >> +
> >> +	local_irq_save(flags);
> >> +	hw_schd_save(&cflags);
> >
> >Can you explain why those two lines are needed ?
> The idea is that access to shared core registers (among threads) is not done
> in parallel to keep their consistency.  For example Read Modified Write of TSI
> register is not atomic, so using two lines above avoid any interference during
> this code execution.

Mmmh, I'm not used to hardware scheduling. Why hw_schd_save() is needed ?

regmap provides the API to deal with read/write of shared register.

> >> +
> >> +static int nps_clkevent_set_next_event(unsigned long delta,
> >> +				       struct clock_event_device *dev) {
> >> +	struct irq_desc *desc = irq_to_desc(nps_timer0_irq);
> >> +	struct irq_chip *chip = irq_data_get_irq_chip(&desc->irq_data);
> >> +
> >> +	nps_clkevent_add_thread(true);
> >> +	chip->irq_unmask(&desc->irq_data);
> 
> >Can you explain why invoking low level IRQ callbacks is needed here ?
> I needed those callbacks functionality and didn't want to duplicate it here.

If you need these callbacks here, then there is probably an issue with the
driver design.

> >> +
> >> +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);
> 
> >Please explain this. I read only CPU0 can set the periodic timer.
> When system works in periodic mode for clock events we just need to set for
> all HW threads within same core their respective bit at TSI register.  We also
> need but only once to arm the shared timer control register.  Since that for
> each core thread 0 is always available we choose HW thread 0 to do that.  ...

I see. Please, add the explanation as a comment in the code.

> >> +static int __init nps_setup_clockevent(struct device_node *node) {
> >> +	struct clock_event_device *evt = this_cpu_ptr(&nps_clockevent_device);
> >> +	struct clk *clk;
> 
> >clk = 0xDEADBEEF
> >> +	int ret;
> >> +
> >> +	nps_timer0_irq = irq_of_parse_and_map(node, 0);
> >> +	if (nps_timer0_irq <= 0) {
> >> +		pr_err("clockevent: missing irq");
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	nps_get_timer_clk(node, &nps_timer0_freq, clk);
> >> +
> >> +	/* 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)", evt);
> >> +	if (ret) {
> >> +		pr_err("Couldn't request irq\n");
> >> +		clk_disable_unprepare(clk);
> 
> >clk is on the stack, hence returning back from the function, clk is
> >undefined.
> 
> >clk_disable_unprepare(0xDEADBEEF) ==> kernel panic
> 
> >It does not make sense to add the nps_get_timer_clk() function.
> 
> >Better to have a couple of duplicated lines and properly rollback from the
> >right place instead of rollbacking supposed actions taken from inside a
> >function.
> As I wrote above I will use **clk for the rollback, so nps_get_timer_clk()
> will make sense and avoid code duplication.
> 
> >> +		return ret;
> >> +	}
> >> +
> >> +	ret = cpuhp_setup_state(CPUHP_AP_NPS_TIMER_STARTING,
> >> +				"AP_NPS_TIMER_STARTING",
> >> +				nps_timer_starting_cpu,
> >> +				nps_timer_dying_cpu);
> >> +	if (ret) {
> >> +		pr_err("Failed to setup hotplug state");
> >> +		clk_disable_unprepare(clk);
> >> +		return ret;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clkevt, "ezchip,nps400-timer0",
> >> +		       nps_setup_clockevent);
> >> +#endif /* CONFIG_EZNPS_MTM_EXT */
> >> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h 
> >> index 34bd805..9efc1a3 100644
> >> --- a/include/linux/cpuhotplug.h
> >> +++ b/include/linux/cpuhotplug.h
> >> @@ -60,6 +60,7 @@ enum cpuhp_state {
> >> 	CPUHP_AP_MARCO_TIMER_STARTING,
> >>  	CPUHP_AP_MIPS_GIC_TIMER_STARTING,
> >>  	CPUHP_AP_ARC_TIMER_STARTING,
> >> +	CPUHP_AP_NPS_TIMER_STARTING,
> 
> >Oops, wait. Here, arch/arc/kernel/time.c should be moved in
> >drivers/clocksource and consolidated with this driver.
> 
> >Very likely, CPUHP_AP_ARC_TIMER_STARTING can be used for all ARC timers.
> 
> Indeed the ARC timer driver served as my inspiration but due to HW threads
> handling they are not the same.  Moving drivers from arch/arc to
> driver/clocksource is not my call (Vineet Gupta is the maintainer of ARC) And
> I think they quiet differ now so consolidation gain is not obvious.


--
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 v2 3/3] clocksource: Add clockevent support to NPS400 driver

[Noam Camus]

> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] 
> Sent: Tuesday, November 1, 2016 10:02 PM
...
>Assuming cpu0 and cpu1 are sibling, does

>taskset 0x1 time sleep 2 & taskset 0x2 time sleep 3

I will use 16,17 instead of 0,1
>give a correct result without a dmesg log ?
[root@192.168.8.2 /]$ [root@192.168.8.2 /]$ taskset 65536 time sleep 2 & taskset 131072 time sleep 3
real    0m 2.54s
user    0m 0.04s
sys     0m 0.14s
real    0m 3.47s
user    0m 0.00s
sys     0m 0.15s
[1]+  Done                       taskset 65536 time sleep 2

Seem OK to me.

> Can you give the content of the /proc/timer_list ?
[root@192.168.8.2 /]$ cat /proc/timer_list
Timer List Version: v0.8
HRTIMER_MAX_CLOCK_BASES: 4
now at 2421277626774 nsecs

cpu: 0
 clock 0:
  .base:       9fccb540
  .index:      0
  .resolution: 1 nsecs
  .get_time:   ktime_get
  .offset:     0 nsecs
active timers:
 #0: <9fccb69c>, tick_sched_timer, S:01
 # expires at 2421140000000-2421140000000 nsecs [in -137626774 to -137626774 nsecs]
 clock 1:
  .base:       9fccb560
  .index:      1
  .resolution: 1 nsecs
  .get_time:   ktime_get_real
  .offset:     0 nsecs
active timers:
 clock 2:
  .base:       9fccb580
  .index:      2
  .resolution: 1 nsecs
  .get_time:   ktime_get_boottime
  .offset:     0 nsecs
active timers:
 clock 3:
  .base:       9fccb5a0
  .index:      3
  .resolution: 1 nsecs
  .get_time:   ktime_get_clocktai
  .offset:     0 nsecs
active timers:
  .expires_next   : 2421140000000 nsecs
  .hres_active    : 1
  .nr_events      : 615427
  .nr_retries     : 10052
  .nr_hangs       : 37
  .max_hang_time  : 682411010
  .nohz_mode      : 2
  .last_tick      : 0 nsecs
  .tick_stopped   : 0
  .idle_jiffies   : 0
  .idle_calls     : 0
  .idle_sleeps    : 0
  .idle_entrytime : 2421131605769 nsecs
  .idle_waketime  : 0 nsecs
  .idle_exittime  : 0 nsecs
  .idle_sleeptime : 1900903609165 nsecs
  .iowait_sleeptime: 0 nsecs
  .last_jiffies   : 0
  .next_timer     : 0
  .idle_expires   : 0 nsecs
jiffies: 212114

cpu: 16
 clock 0:
  .base:       9fcd7540
  .index:      0
  .resolution: 1 nsecs
  .get_time:   ktime_get
  .offset:     0 nsecs
active timers:
 clock 1:
  .base:       9fcd7560
  .index:      1
  .resolution: 1 nsecs
  .get_time:   ktime_get_real
  .offset:     0 nsecs
active timers:
 clock 2:
  .base:       9fcd7580
  .index:      2
  .resolution: 1 nsecs
  .get_time:   ktime_get_boottime
  .offset:     0 nsecs
active timers:
 clock 3:
  .base:       9fcd75a0
  .index:      3
  .resolution: 1 nsecs
  .get_time:   ktime_get_clocktai
  .offset:     0 nsecs
active timers:
  .expires_next   : 9223372036854775807 nsecs
  .hres_active    : 1
  .nr_events      : 18
  .nr_retries     : 1
  .nr_hangs       : 0
  .max_hang_time  : 0
  .nohz_mode      : 2
  .last_tick      : 2410120000000 nsecs
  .tick_stopped   : 1
  .idle_jiffies   : 211017
  .idle_calls     : 27
  .idle_sleeps    : 27
  .idle_entrytime : 2410189597725 nsecs
  .idle_waketime  : 2410189342725 nsecs
  .idle_exittime  : 2410110197721 nsecs
  .idle_sleeptime : 2408852044732 nsecs
  .iowait_sleeptime: 0 nsecs
  .last_jiffies   : 211019
  .next_timer     : 9223372036854775807
  .idle_expires   : 9223372036854775807 nsecs
jiffies: 212114

cpu: 17
 clock 0:
  .base:       9fce3540
  .index:      0
  .resolution: 1 nsecs
  .get_time:   ktime_get
  .offset:     0 nsecs
active timers:
 clock 1:
  .base:       9fce3560
  .index:      1
  .resolution: 1 nsecs
  .get_time:   ktime_get_real
  .offset:     0 nsecs
active timers:
 clock 2:
  .base:       9fce3580
  .index:      2
  .resolution: 1 nsecs
  .get_time:   ktime_get_boottime
  .offset:     0 nsecs
active timers:
 clock 3:
  .base:       9fce35a0
  .index:      3
  .resolution: 1 nsecs
  .get_time:   ktime_get_clocktai
  .offset:     0 nsecs
active timers:
  .expires_next   : 9223372036854775807 nsecs
  .hres_active    : 1
  .nr_events      : 22
  .nr_retries     : 1
  .nr_hangs       : 0
  .max_hang_time  : 0
  .nohz_mode      : 2
  .last_tick      : 2412120000000 nsecs
  .tick_stopped   : 1
  .idle_jiffies   : 211212
  .idle_calls     : 32
  .idle_sleeps    : 32
  .idle_entrytime : 2412123353729 nsecs
  .idle_waketime  : 2412123049733 nsecs
  .idle_exittime  : 2412110161733 nsecs
  .idle_sleeptime : 2410832354720 nsecs
  .iowait_sleeptime: 0 nsecs
  .last_jiffies   : 211213
  .next_timer     : 9223372036854775807
  .idle_expires   : 9223372036854775807 nsecs
jiffies: 212114

Tick Device: mode:     1
Per CPU device: 0
Clock Event Device: ARC Timer0
 max_delta_ns:   51539607733
 min_delta_ns:   1000
 mult:           178956970
 shift:          31
 mode:           3
 next_event:     2421140000000 nsecs
 set_next_event: arc_clkevent_set_next_event
 periodic: arc_clkevent_set_periodic
 event_handler:  hrtimer_interrupt
 retries:        0

Tick Device: mode:     1
Per CPU device: 16
Clock Event Device: ARC Timer0
 max_delta_ns:   51539607733
 min_delta_ns:   1000
 mult:           178956970
 shift:          31
 mode:           3
 next_event:     9223372036854775807 nsecs
 set_next_event: arc_clkevent_set_next_event
 periodic: arc_clkevent_set_periodic
 event_handler:  hrtimer_interrupt
 retries:        2

Tick Device: mode:     1
Per CPU device: 17
Clock Event Device: ARC Timer0
 max_delta_ns:   51539607733
 min_delta_ns:   1000
 mult:           178956970
 shift:          31
 mode:           3
 next_event:     9223372036854775807 nsecs
 set_next_event: arc_clkevent_set_next_event
 periodic: arc_clkevent_set_periodic
 event_handler:  hrtimer_interrupt
 retries:        2

-Noam

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

[Daniel Lezcano]

On 08/11/2016 09:30, Noam Camus wrote:
>> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] 
>> Sent: Tuesday, November 1, 2016 10:02 PM
> ...
>> Assuming cpu0 and cpu1 are sibling, does
> 
>> taskset 0x1 time sleep 2 & taskset 0x2 time sleep 3
> 
> I will use 16,17 instead of 0,1
>> give a correct result without a dmesg log ?
> [root@192.168.8.2 /]$ [root@192.168.8.2 /]$ taskset 65536 time sleep 2 & taskset 131072 time sleep 3

Thanks for providing the numbers.

So there is no weird messages in dmesg ?

> real    0m 2.54s
> user    0m 0.04s
> sys     0m 0.14s
> real    0m 3.47s
> user    0m 0.00s
> sys     0m 0.15s
> [1]+  Done                       taskset 65536 time sleep 2
> 
> Seem OK to me.

I'm not sure of that.

2.54 instead of 2
3.47 instead of 3

0.02 time drift could be acceptable but 0.54 or 0.47 is too much.

And 0.15 in sys also a big number AFAICT.

Is the system in NO_HZ ?



-- 
 <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 v2 3/3] clocksource: Add clockevent support to NPS400 driver

[Noam Camus]

>From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] 
>Sent: Thursday, November 10, 2016 12:34 PM

>>> give a correct result without a dmesg log ?
>> [root@192.168.8.2 /]$ [root@192.168.8.2 /]$ taskset 65536 time sleep 2 
>> & taskset 131072 time sleep 3

>Thanks for providing the numbers.

>So there is no weird messages in dmesg ?
Yes, no weird messages, all seem normal.

>> real    0m 2.54s
>> user    0m 0.04s
>> sys     0m 0.14s
>> real    0m 3.47s
>> user    0m 0.00s
>> sys     0m 0.15s
>> [1]+  Done                       taskset 65536 time sleep 2
>> 
>> Seem OK to me.

>I'm not sure of that.

>2.54 instead of 2
>3.47 instead of 3

>0.02 time drift could be acceptable but 0.54 or 0.47 is too much.

>And 0.15 in sys also a big number AFAICT.

>Is the system in NO_HZ ?

We are handling system with NOHZ_FULL 
This was run under simulation and result are reasonable (indeed not perfect) AFAICT.

-Noam