[PATCH v4 0/5] Modernize ARC clocksource/clockevent/intc drivers

[PATCH v4 0/5] Modernize ARC clocksource/clockevent/intc drivers

[Vineet Gupta]

Hi,

This is a partial repost of a series [1] which improves the ARC clock* drivers.
I've also included the patches to switch the core intc from legacy to linear
domains (as suggested by Marc Z a while back).

These are stepping stones for eventual landing into driver/*

Please review !

Thx,
-Vineet


[1] http://lists.infradead.org/pipermail/linux-snps-arc/2016-March/000653.html

Vineet Gupta (5):
  ARC: clockevent: DT based probe
  ARC: clocksource: DT based probe
  ARC: irq: export some IRQs again
  ARC: [intc-*] Do a domain lookup in primary handler for hwirq -> linux
    virq
  ARC: [intc-*] switch to linear domain

 arch/arc/Kconfig               |   1 +
 arch/arc/include/asm/irq.h     |   9 +--
 arch/arc/kernel/intc-arcv2.c   |  17 +++--
 arch/arc/kernel/intc-compact.c |  14 ++--
 arch/arc/kernel/irq.c          |   9 +--
 arch/arc/kernel/mcip.c         |   7 +-
 arch/arc/kernel/setup.c        |   3 -
 arch/arc/kernel/time.c         | 169 +++++++++++++++++++++++++----------------
 8 files changed, 131 insertions(+), 98 deletions(-)

-- 
2.5.0

[PATCH v4 1/5] ARC: clockevent: DT based probe

[Vineet Gupta]

 - timer frequency is derived from DT (no longer rely on top level
   DT "clock-frequency" probed early and exported by asm/clk.h)

 - TIMER0_IRQ need not be exported across arch code, confined to intc as
   it is property of same

Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 arch/arc/include/asm/irq.h     |  9 --------
 arch/arc/kernel/intc-compact.c |  2 ++
 arch/arc/kernel/time.c         | 49 +++++++++++++++++++++++++++++-------------
 3 files changed, 36 insertions(+), 24 deletions(-)

diff --git a/arch/arc/include/asm/irq.h b/arch/arc/include/asm/irq.h
index 5c0b5abda67a..a6ac89dc228f 100644
--- a/arch/arc/include/asm/irq.h
+++ b/arch/arc/include/asm/irq.h
@@ -12,15 +12,6 @@
 #define NR_CPU_IRQS	32  /* number of interrupt lines of ARC770 CPU */
 #define NR_IRQS		128 /* allow some CPU external IRQ handling */
 
-/* Platform Independent IRQs */
-#ifdef CONFIG_ISA_ARCOMPACT
-#define TIMER0_IRQ      3
-#define TIMER1_IRQ      4
-#else
-#define TIMER0_IRQ      16
-#define TIMER1_IRQ      17
-#endif
-
 #include <linux/interrupt.h>
 #include <asm-generic/irq.h>
 
diff --git a/arch/arc/kernel/intc-compact.c b/arch/arc/kernel/intc-compact.c
index 4195eedeb6d1..d31bc647146d 100644
--- a/arch/arc/kernel/intc-compact.c
+++ b/arch/arc/kernel/intc-compact.c
@@ -14,6 +14,8 @@
 #include <linux/irqchip.h>
 #include <asm/irq.h>
 
+#define TIMER0_IRQ	3	/* Fixed by ISA */
+
 /*
  * Early Hardware specific Interrupt setup
  * -Platform independent, needed for each CPU (not foldable into init_IRQ)
diff --git a/arch/arc/kernel/time.c b/arch/arc/kernel/time.c
index 848353a27ac8..693545df9827 100644
--- a/arch/arc/kernel/time.c
+++ b/arch/arc/kernel/time.c
@@ -30,19 +30,15 @@
  */
 
 #include <linux/interrupt.h>
-#include <linux/module.h>
-#include <linux/kernel.h>
-#include <linux/time.h>
-#include <linux/init.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/clocksource.h>
 #include <linux/clockchips.h>
 #include <linux/cpu.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
 #include <asm/irq.h>
 #include <asm/arcregs.h>
-#include <asm/clk.h>
-#include <asm/mach_desc.h>
 
 #include <asm/mcip.h>
 
@@ -59,6 +55,24 @@
 
 #define ARC_TIMER_MAX	0xFFFFFFFF
 
+static unsigned long arc_timer_freq;
+
+static void noinline arc_get_timer_clk(struct device_node *node)
+{
+	struct clk *clk;
+	int ret;
+
+	clk = of_clk_get(node, 0);
+	if (IS_ERR(clk))
+		panic("Can't get timer clock");
+
+	ret = clk_prepare_enable(clk);
+	if (ret)
+		pr_err("Couldn't enable parent clock\n");
+
+	arc_timer_freq = clk_get_rate(clk);
+}
+
 /********** Clock Source Device *********/
 
 #ifdef CONFIG_ARC_HAS_GFRC
@@ -182,7 +196,7 @@ static struct clocksource arc_counter = {
 
 /********** Clock Event Device *********/
 
-static int arc_timer_irq = TIMER0_IRQ;
+static int arc_timer_irq;
 
 /*
  * Arm the timer to interrupt after @cycles
@@ -210,7 +224,7 @@ static int arc_clkevent_set_periodic(struct clock_event_device *dev)
 	 * At X Hz, 1 sec = 1000ms -> X cycles;
 	 *		      10ms -> X / 100 cycles
 	 */
-	arc_timer_event_setup(arc_get_core_freq() / HZ);
+	arc_timer_event_setup(arc_timer_freq / HZ);
 	return 0;
 }
 
@@ -253,7 +267,7 @@ static int arc_timer_cpu_notify(struct notifier_block *self,
 
 	switch (action & ~CPU_TASKS_FROZEN) {
 	case CPU_STARTING:
-		clockevents_config_and_register(evt, arc_get_core_freq(),
+		clockevents_config_and_register(evt, arc_timer_freq,
 						0, ULONG_MAX);
 		enable_percpu_irq(arc_timer_irq, 0);
 		break;
@@ -272,15 +286,22 @@ static struct notifier_block arc_timer_cpu_nb = {
 /*
  * clockevent setup for boot CPU
  */
-static void __init arc_clockevent_setup(void)
+static void __init arc_clockevent_setup(struct device_node *node)
 {
 	struct clock_event_device *evt = this_cpu_ptr(&arc_clockevent_device);
 	int ret;
 
 	register_cpu_notifier(&arc_timer_cpu_nb);
 
+	arc_timer_irq = irq_of_parse_and_map(node, 0);
+	if (arc_timer_irq <= 0)
+		panic("Can't parse IRQ");
+
+	arc_get_timer_clk(node);
+
+	evt->irq = arc_timer_irq;
 	evt->cpumask = cpumask_of(smp_processor_id());
-	clockevents_config_and_register(evt, arc_get_core_freq(),
+	clockevents_config_and_register(evt, arc_timer_freq,
 					0, ARC_TIMER_MAX);
 
 	/* Needs apriori irq_set_percpu_devid() done in intc map function */
@@ -291,6 +312,7 @@ static void __init arc_clockevent_setup(void)
 
 	enable_percpu_irq(arc_timer_irq, 0);
 }
+CLOCKSOURCE_OF_DECLARE(arc_clkevt, "snps,arc-timer", arc_clockevent_setup);
 
 /*
  * Called from start_kernel() - boot CPU only
@@ -299,7 +321,6 @@ static void __init arc_clockevent_setup(void)
  * -Also sets up any global state needed for timer subsystem:
  *    - for "counting" timer, registers a clocksource, usable across CPUs
  *      (provided that underlying counter h/w is synchronized across cores)
- *    - for "event" timer, sets up TIMER0 IRQ (as that is platform agnostic)
  */
 void __init time_init(void)
 {
@@ -315,7 +336,5 @@ void __init time_init(void)
 		 * CLK upto 4.29 GHz can be safely represented in 32 bits
 		 * because Max 32 bit number is 4,294,967,295
 		 */
-		clocksource_register_hz(&arc_counter, arc_get_core_freq());
-
-	arc_clockevent_setup();
+		clocksource_register_hz(&arc_counter, arc_timer_freq);
 }
-- 
2.5.0

Re: [PATCH v4 1/5] ARC: clockevent: DT based probe

[Daniel Lezcano]

On Wed, Apr 13, 2016 at 05:10:01PM +0530, Vineet Gupta wrote:
>  - timer frequency is derived from DT (no longer rely on top level
>    DT "clock-frequency" probed early and exported by asm/clk.h)
> 
>  - TIMER0_IRQ need not be exported across arch code, confined to intc as
>    it is property of same
> 
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> ---

[ ... ]

> +static void noinline arc_get_timer_clk(struct device_node *node)
> +{
> +	struct clk *clk;
> +	int ret;
> +
> +	clk = of_clk_get(node, 0);
> +	if (IS_ERR(clk))
> +		panic("Can't get timer clock");
> +

Don't panic here. Change the function to return an error and let the
caller to handle it.

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

[ ... ]

> +static void __init arc_clockevent_setup(struct device_node *node)
>  {
>  	struct clock_event_device *evt = this_cpu_ptr(&arc_clockevent_device);
>  	int ret;
>  
>  	register_cpu_notifier(&arc_timer_cpu_nb);
>  
> +	arc_timer_irq = irq_of_parse_and_map(node, 0);
> +	if (arc_timer_irq <= 0)
> +		panic("Can't parse IRQ");
> +
> +	arc_get_timer_clk(node);

Connected to the comment above, check the return code.

> +
> +	evt->irq = arc_timer_irq;
>  	evt->cpumask = cpumask_of(smp_processor_id());
> -	clockevents_config_and_register(evt, arc_get_core_freq(),
> +	clockevents_config_and_register(evt, arc_timer_freq,
>  					0, ARC_TIMER_MAX);
>  
>  	/* Needs apriori irq_set_percpu_devid() done in intc map function */
> @@ -291,6 +312,7 @@ static void __init arc_clockevent_setup(void)
>  
>  	enable_percpu_irq(arc_timer_irq, 0);
>  }
> +CLOCKSOURCE_OF_DECLARE(arc_clkevt, "snps,arc-timer", arc_clockevent_setup);
>  
>  /*
>   * Called from start_kernel() - boot CPU only
> @@ -299,7 +321,6 @@ static void __init arc_clockevent_setup(void)
>   * -Also sets up any global state needed for timer subsystem:
>   *    - for "counting" timer, registers a clocksource, usable across CPUs
>   *      (provided that underlying counter h/w is synchronized across cores)
> - *    - for "event" timer, sets up TIMER0 IRQ (as that is platform agnostic)
>   */
>  void __init time_init(void)
>  {
> @@ -315,7 +336,5 @@ void __init time_init(void)
>  		 * CLK upto 4.29 GHz can be safely represented in 32 bits
>  		 * because Max 32 bit number is 4,294,967,295
>  		 */
> -		clocksource_register_hz(&arc_counter, arc_get_core_freq());
> -
> -	arc_clockevent_setup();
> +		clocksource_register_hz(&arc_counter, arc_timer_freq);
>  }
> -- 
> 2.5.0
> 

Re: [PATCH v4 1/5] ARC: clockevent: DT based probe

[Vineet Gupta]

On Wednesday 13 April 2016 06:29 PM, Daniel Lezcano wrote:
> On Wed, Apr 13, 2016 at 05:10:01PM +0530, Vineet Gupta wrote:
>>  - timer frequency is derived from DT (no longer rely on top level
>>    DT "clock-frequency" probed early and exported by asm/clk.h)
>>
>>  - TIMER0_IRQ need not be exported across arch code, confined to intc as
>>    it is property of same
>>
>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>> ---
> 
> [ ... ]
> 
>> +static void noinline arc_get_timer_clk(struct device_node *node)
>> +{
>> +	struct clk *clk;
>> +	int ret;
>> +
>> +	clk = of_clk_get(node, 0);
>> +	if (IS_ERR(clk))
>> +		panic("Can't get timer clock");
>> +
> 
> Don't panic here. Change the function to return an error and let the
> caller to handle it.
> 
>> +	ret = clk_prepare_enable(clk);
>> +	if (ret)
>> +		pr_err("Couldn't enable parent clock\n");

I suppose we need to return here too ?

>> +
>> +	arc_timer_freq = clk_get_rate(clk);
>> +}
>> +
> 
> [ ... ]
> 
>> +static void __init arc_clockevent_setup(struct device_node *node)
>>  {
>>  	struct clock_event_device *evt = this_cpu_ptr(&arc_clockevent_device);
>>  	int ret;
>>  
>>  	register_cpu_notifier(&arc_timer_cpu_nb);
>>  
>> +	arc_timer_irq = irq_of_parse_and_map(node, 0);
>> +	if (arc_timer_irq <= 0)
>> +		panic("Can't parse IRQ");
>> +
>> +	arc_get_timer_clk(node);
> 
> Connected to the comment above, check the return code.

Right and if there's error, bail from here too ? This will leave a system w/o a
working clockevent and our lpj setup loop will spin forever. Better to add a WARN
so that user knows to fix his DT.

> 
>> +
>> +	evt->irq = arc_timer_irq;
>>  	evt->cpumask = cpumask_of(smp_processor_id());
>> -	clockevents_config_and_register(evt, arc_get_core_freq(),
>> +	clockevents_config_and_register(evt, arc_timer_freq,
>>  					0, ARC_TIMER_MAX);
>>  
>>  	/* Needs apriori irq_set_percpu_devid() done in intc map function */
>> @@ -291,6 +312,7 @@ static void __init arc_clockevent_setup(void)
>>  
>>  	enable_percpu_irq(arc_timer_irq, 0);
>>  }
>> +CLOCKSOURCE_OF_DECLARE(arc_clkevt, "snps,arc-timer", arc_clockevent_setup);
>>  
>>  /*
>>   * Called from start_kernel() - boot CPU only
>> @@ -299,7 +321,6 @@ static void __init arc_clockevent_setup(void)
>>   * -Also sets up any global state needed for timer subsystem:
>>   *    - for "counting" timer, registers a clocksource, usable across CPUs
>>   *      (provided that underlying counter h/w is synchronized across cores)
>> - *    - for "event" timer, sets up TIMER0 IRQ (as that is platform agnostic)
>>   */
>>  void __init time_init(void)
>>  {
>> @@ -315,7 +336,5 @@ void __init time_init(void)
>>  		 * CLK upto 4.29 GHz can be safely represented in 32 bits
>>  		 * because Max 32 bit number is 4,294,967,295
>>  		 */
>> -		clocksource_register_hz(&arc_counter, arc_get_core_freq());
>> -
>> -	arc_clockevent_setup();
>> +		clocksource_register_hz(&arc_counter, arc_timer_freq);
>>  }
>> -- 
>> 2.5.0
>>
> 

Re: [PATCH v4 1/5] ARC: clockevent: DT based probe

[Daniel Lezcano]

On Thu, Apr 14, 2016 at 03:02:38PM +0530, Vineet Gupta wrote:
> On Wednesday 13 April 2016 06:29 PM, Daniel Lezcano wrote:
> > On Wed, Apr 13, 2016 at 05:10:01PM +0530, Vineet Gupta wrote:
> >>  - timer frequency is derived from DT (no longer rely on top level
> >>    DT "clock-frequency" probed early and exported by asm/clk.h)
> >>
> >>  - TIMER0_IRQ need not be exported across arch code, confined to intc as
> >>    it is property of same
> >>
> >> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> >> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> >> ---
> > 
> > [ ... ]
> > 
> >> +static void noinline arc_get_timer_clk(struct device_node *node)
> >> +{
> >> +	struct clk *clk;
> >> +	int ret;
> >> +
> >> +	clk = of_clk_get(node, 0);
> >> +	if (IS_ERR(clk))
> >> +		panic("Can't get timer clock");
> >> +
> > 
> > Don't panic here. Change the function to return an error and let the
> > caller to handle it.
> > 
> >> +	ret = clk_prepare_enable(clk);
> >> +	if (ret)
> >> +		pr_err("Couldn't enable parent clock\n");
> 
> I suppose we need to return here too ?
>
> >> +
> >> +	arc_timer_freq = clk_get_rate(clk);
> >> +}
> >> +
> > 
> > [ ... ]
> > 
> >> +static void __init arc_clockevent_setup(struct device_node *node)
> >>  {
> >>  	struct clock_event_device *evt = this_cpu_ptr(&arc_clockevent_device);
> >>  	int ret;
> >>  
> >>  	register_cpu_notifier(&arc_timer_cpu_nb);
> >>  
> >> +	arc_timer_irq = irq_of_parse_and_map(node, 0);
> >> +	if (arc_timer_irq <= 0)
> >> +		panic("Can't parse IRQ");
> >> +
> >> +	arc_get_timer_clk(node);
> > 
> > Connected to the comment above, check the return code.
> 
> Right and if there's error, bail from here too ? This will leave a system w/o a
> working clockevent and our lpj setup loop will spin forever. Better to add a WARN
> so that user knows to fix his DT.

You can handle the error as you wish here. I just don't want panics spread in the
code.

In the patch 2/5, you mention:

"Remove some of the panic() calls if underlying timer is NOT detected as
 fallback clocksource might still be available"

but you add calls to arc_get_timer_clk() which can panic.

The main problem is the macro CLOCKSOURCE_OF_DECLARE() which expect an init function
returning void. I would like to change the code to have it returning an int, so
the panic could be raised in the generic clksrc code when all clocksource inits fail.

In the meantime, it is a good practice to concentrate all panics in a single place,
that is in the init function to facilitate the conversion above which will happen
in the future ...

  -- Daniel


 
> >> +
> >> +	evt->irq = arc_timer_irq;
> >>  	evt->cpumask = cpumask_of(smp_processor_id());
> >> -	clockevents_config_and_register(evt, arc_get_core_freq(),
> >> +	clockevents_config_and_register(evt, arc_timer_freq,
> >>  					0, ARC_TIMER_MAX);
> >>  
> >>  	/* Needs apriori irq_set_percpu_devid() done in intc map function */
> >> @@ -291,6 +312,7 @@ static void __init arc_clockevent_setup(void)
> >>  
> >>  	enable_percpu_irq(arc_timer_irq, 0);
> >>  }
> >> +CLOCKSOURCE_OF_DECLARE(arc_clkevt, "snps,arc-timer", arc_clockevent_setup);
> >>  
> >>  /*
> >>   * Called from start_kernel() - boot CPU only
> >> @@ -299,7 +321,6 @@ static void __init arc_clockevent_setup(void)
> >>   * -Also sets up any global state needed for timer subsystem:
> >>   *    - for "counting" timer, registers a clocksource, usable across CPUs
> >>   *      (provided that underlying counter h/w is synchronized across cores)
> >> - *    - for "event" timer, sets up TIMER0 IRQ (as that is platform agnostic)
> >>   */
> >>  void __init time_init(void)
> >>  {
> >> @@ -315,7 +336,5 @@ void __init time_init(void)
> >>  		 * CLK upto 4.29 GHz can be safely represented in 32 bits
> >>  		 * because Max 32 bit number is 4,294,967,295
> >>  		 */
> >> -		clocksource_register_hz(&arc_counter, arc_get_core_freq());
> >> -
> >> -	arc_clockevent_setup();
> >> +		clocksource_register_hz(&arc_counter, arc_timer_freq);
> >>  }
> >> -- 
> >> 2.5.0
> >>
> > 
> 

[PATCH v5] ARC: clockevent: DT based probe

[Vineet Gupta]

 - timer frequency is derived from DT (no longer rely on top level
   DT "clock-frequency" probed early and exported by asm/clk.h)

 - TIMER0_IRQ need not be exported across arch code, confined to intc as
   it is property of same

 - Any failures in clockevent setup are considered pedantic and system
   panic()'s as there is no generic fallback (unlike clocksource where
   a jiffies based soft clocksource always exists)

Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
Changes since v4 [*]
- clockevent setup failure panic()s bubbled up to top level function [Daniel]
- arc_get_timer_clk() returns error status vs. void [Daniel]
- For consistency, straggler pr_err() in setup function also converted to panic

[*] http://lists.infradead.org/pipermail/linux-snps-arc/2016-April/000903.html
---
 arch/arc/include/asm/irq.h     |  9 -------
 arch/arc/kernel/intc-compact.c |  2 ++
 arch/arc/kernel/time.c         | 58 ++++++++++++++++++++++++++++++------------
 3 files changed, 44 insertions(+), 25 deletions(-)

diff --git a/arch/arc/include/asm/irq.h b/arch/arc/include/asm/irq.h
index 5c0b5abda67a..a6ac89dc228f 100644
--- a/arch/arc/include/asm/irq.h
+++ b/arch/arc/include/asm/irq.h
@@ -12,15 +12,6 @@
 #define NR_CPU_IRQS	32  /* number of interrupt lines of ARC770 CPU */
 #define NR_IRQS		128 /* allow some CPU external IRQ handling */
 
-/* Platform Independent IRQs */
-#ifdef CONFIG_ISA_ARCOMPACT
-#define TIMER0_IRQ      3
-#define TIMER1_IRQ      4
-#else
-#define TIMER0_IRQ      16
-#define TIMER1_IRQ      17
-#endif
-
 #include <linux/interrupt.h>
 #include <asm-generic/irq.h>
 
diff --git a/arch/arc/kernel/intc-compact.c b/arch/arc/kernel/intc-compact.c
index 4195eedeb6d1..d31bc647146d 100644
--- a/arch/arc/kernel/intc-compact.c
+++ b/arch/arc/kernel/intc-compact.c
@@ -14,6 +14,8 @@
 #include <linux/irqchip.h>
 #include <asm/irq.h>
 
+#define TIMER0_IRQ	3	/* Fixed by ISA */
+
 /*
  * Early Hardware specific Interrupt setup
  * -Platform independent, needed for each CPU (not foldable into init_IRQ)
diff --git a/arch/arc/kernel/time.c b/arch/arc/kernel/time.c
index 848353a27ac8..83e2c0eb2d68 100644
--- a/arch/arc/kernel/time.c
+++ b/arch/arc/kernel/time.c
@@ -30,19 +30,15 @@
  */
 
 #include <linux/interrupt.h>
-#include <linux/module.h>
-#include <linux/kernel.h>
-#include <linux/time.h>
-#include <linux/init.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/clocksource.h>
 #include <linux/clockchips.h>
 #include <linux/cpu.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
 #include <asm/irq.h>
 #include <asm/arcregs.h>
-#include <asm/clk.h>
-#include <asm/mach_desc.h>
 
 #include <asm/mcip.h>
 
@@ -59,6 +55,29 @@
 
 #define ARC_TIMER_MAX	0xFFFFFFFF
 
+static unsigned long arc_timer_freq;
+
+static int noinline arc_get_timer_clk(struct device_node *node)
+{
+	struct clk *clk;
+	int ret = -1;
+
+	clk = of_clk_get(node, 0);
+	if (IS_ERR(clk)) {
+		pr_err("timer missing clk");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(clk);
+	if (ret) {
+		pr_err("Couldn't enable parent clk\n");
+		return ret;
+	}
+
+	arc_timer_freq = clk_get_rate(clk);
+	return ret;
+}
+
 /********** Clock Source Device *********/
 
 #ifdef CONFIG_ARC_HAS_GFRC
@@ -182,7 +201,7 @@ static struct clocksource arc_counter = {
 
 /********** Clock Event Device *********/
 
-static int arc_timer_irq = TIMER0_IRQ;
+static int arc_timer_irq;
 
 /*
  * Arm the timer to interrupt after @cycles
@@ -210,7 +229,7 @@ static int arc_clkevent_set_periodic(struct clock_event_device *dev)
 	 * At X Hz, 1 sec = 1000ms -> X cycles;
 	 *		      10ms -> X / 100 cycles
 	 */
-	arc_timer_event_setup(arc_get_core_freq() / HZ);
+	arc_timer_event_setup(arc_timer_freq / HZ);
 	return 0;
 }
 
@@ -253,7 +272,7 @@ static int arc_timer_cpu_notify(struct notifier_block *self,
 
 	switch (action & ~CPU_TASKS_FROZEN) {
 	case CPU_STARTING:
-		clockevents_config_and_register(evt, arc_get_core_freq(),
+		clockevents_config_and_register(evt, arc_timer_freq,
 						0, ULONG_MAX);
 		enable_percpu_irq(arc_timer_irq, 0);
 		break;
@@ -272,25 +291,35 @@ static struct notifier_block arc_timer_cpu_nb = {
 /*
  * clockevent setup for boot CPU
  */
-static void __init arc_clockevent_setup(void)
+static void __init arc_clockevent_setup(struct device_node *node)
 {
 	struct clock_event_device *evt = this_cpu_ptr(&arc_clockevent_device);
 	int ret;
 
 	register_cpu_notifier(&arc_timer_cpu_nb);
 
+	arc_timer_irq = irq_of_parse_and_map(node, 0);
+	if (arc_timer_irq <= 0)
+		panic("clockevent: missing irq");
+
+	ret = arc_get_timer_clk(node);
+	if (ret)
+		panic("clockevent: missing clk");
+
+	evt->irq = arc_timer_irq;
 	evt->cpumask = cpumask_of(smp_processor_id());
-	clockevents_config_and_register(evt, arc_get_core_freq(),
+	clockevents_config_and_register(evt, arc_timer_freq,
 					0, ARC_TIMER_MAX);
 
 	/* Needs apriori irq_set_percpu_devid() done in intc map function */
 	ret = request_percpu_irq(arc_timer_irq, timer_irq_handler,
 				 "Timer0 (per-cpu-tick)", evt);
 	if (ret)
-		pr_err("Unable to register interrupt\n");
+		panic("clockevent: unable to request irq\n");
 
 	enable_percpu_irq(arc_timer_irq, 0);
 }
+CLOCKSOURCE_OF_DECLARE(arc_clkevt, "snps,arc-timer", arc_clockevent_setup);
 
 /*
  * Called from start_kernel() - boot CPU only
@@ -299,7 +328,6 @@ static void __init arc_clockevent_setup(void)
  * -Also sets up any global state needed for timer subsystem:
  *    - for "counting" timer, registers a clocksource, usable across CPUs
  *      (provided that underlying counter h/w is synchronized across cores)
- *    - for "event" timer, sets up TIMER0 IRQ (as that is platform agnostic)
  */
 void __init time_init(void)
 {
@@ -315,7 +343,5 @@ void __init time_init(void)
 		 * CLK upto 4.29 GHz can be safely represented in 32 bits
 		 * because Max 32 bit number is 4,294,967,295
 		 */
-		clocksource_register_hz(&arc_counter, arc_get_core_freq());
-
-	arc_clockevent_setup();
+		clocksource_register_hz(&arc_counter, arc_timer_freq);
 }
-- 
2.5.0

Re: [PATCH v5] ARC: clockevent: DT based probe

[Daniel Lezcano]

On Mon, Apr 18, 2016 at 12:16:10PM +0530, Vineet Gupta wrote:
>  - timer frequency is derived from DT (no longer rely on top level
>    DT "clock-frequency" probed early and exported by asm/clk.h)
> 
>  - TIMER0_IRQ need not be exported across arch code, confined to intc as
>    it is property of same
> 
>  - Any failures in clockevent setup are considered pedantic and system
>    panic()'s as there is no generic fallback (unlike clocksource where
>    a jiffies based soft clocksource always exists)
> 
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> ---
> Changes since v4 [*]
> - clockevent setup failure panic()s bubbled up to top level function [Daniel]
> - arc_get_timer_clk() returns error status vs. void [Daniel]
> - For consistency, straggler pr_err() in setup function also converted to panic
> 
> [*] http://lists.infradead.org/pipermail/linux-snps-arc/2016-April/000903.html

[ ... ]

> +static int noinline arc_get_timer_clk(struct device_node *node)
> +{
> +	struct clk *clk;
> +	int ret = -1;
> +
> +	clk = of_clk_get(node, 0);
> +	if (IS_ERR(clk)) {
> +		pr_err("timer missing clk");
> +		return ret;
> +	}

return PTR_ERR(clk);

> +	ret = clk_prepare_enable(clk);
> +	if (ret) {
> +		pr_err("Couldn't enable parent clk\n");
> +		return ret;
> +	}
> +
> +	arc_timer_freq = clk_get_rate(clk);
> +	return ret;
> +}

With the change above: Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

[PATCH v4 2/5] ARC: clocksource: DT based probe

[Vineet Gupta]

- Remove explicit clocksource setup and let it be done by OF framework
  by defining CLOCKSOURCE_OF_DECLARE() for various timers

- This allows multiple clocksources to be potentially registered
  simultaneouly: previously we could only do one - as all of them had
  same arc_counter_setup() routine for registration

- Setup routines also ensure that the underlying timer actually exists.

- Remove some of the panic() calls if underlying timer is NOT detected as
  fallback clocksource might still be available
  1. If GRFC doesn't exist, jiffies clocksource gets registered anyways
  2. if RTC doesn't exist, TIMER1 can take over (as it is always
     present)

Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 arch/arc/kernel/mcip.c  |   4 +-
 arch/arc/kernel/setup.c |   3 --
 arch/arc/kernel/time.c  | 124 +++++++++++++++++++++++++++---------------------
 3 files changed, 72 insertions(+), 59 deletions(-)

diff --git a/arch/arc/kernel/mcip.c b/arch/arc/kernel/mcip.c
index c41c364b926c..262d9c3771e6 100644
--- a/arch/arc/kernel/mcip.c
+++ b/arch/arc/kernel/mcip.c
@@ -116,15 +116,13 @@ static void mcip_probe_n_setup(void)
 		IS_AVAIL1(mp.dbg, "DEBUG "),
 		IS_AVAIL1(mp.gfrc, "GFRC"));
 
+	cpuinfo_arc700[0].extn.gfrc = mp.gfrc;
 	idu_detected = mp.idu;
 
 	if (mp.dbg) {
 		__mcip_cmd_data(CMD_DEBUG_SET_SELECT, 0, 0xf);
 		__mcip_cmd_data(CMD_DEBUG_SET_MASK, 0xf, 0xf);
 	}
-
-	if (IS_ENABLED(CONFIG_ARC_HAS_GFRC) && !mp.gfrc)
-		panic("kernel trying to use non-existent GFRC\n");
 }
 
 struct plat_smp_ops plat_smp_ops = {
diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
index 507ec523112a..91f79fa447bc 100644
--- a/arch/arc/kernel/setup.c
+++ b/arch/arc/kernel/setup.c
@@ -313,9 +313,6 @@ static void arc_chk_core_config(void)
 	if (!cpu->extn.timer1)
 		panic("Timer1 is not present!\n");
 
-	if (IS_ENABLED(CONFIG_ARC_HAS_RTC) && !cpu->extn.rtc)
-		panic("RTC is not present\n");
-
 #ifdef CONFIG_ARC_HAS_DCCM
 	/*
 	 * DCCM can be arbit placed in hardware.
diff --git a/arch/arc/kernel/time.c b/arch/arc/kernel/time.c
index 693545df9827..72f023440739 100644
--- a/arch/arc/kernel/time.c
+++ b/arch/arc/kernel/time.c
@@ -77,12 +77,7 @@ static void noinline arc_get_timer_clk(struct device_node *node)
 
 #ifdef CONFIG_ARC_HAS_GFRC
 
-static int arc_counter_setup(void)
-{
-	return 1;
-}
-
-static cycle_t arc_counter_read(struct clocksource *cs)
+static cycle_t arc_read_gfrc(struct clocksource *cs)
 {
 	unsigned long flags;
 	union {
@@ -107,15 +102,28 @@ static cycle_t arc_counter_read(struct clocksource *cs)
 	return stamp.full;
 }
 
-static struct clocksource arc_counter = {
+static struct clocksource arc_counter_gfrc = {
 	.name   = "ARConnect GFRC",
 	.rating = 400,
-	.read   = arc_counter_read,
+	.read   = arc_read_gfrc,
 	.mask   = CLOCKSOURCE_MASK(64),
 	.flags  = CLOCK_SOURCE_IS_CONTINUOUS,
 };
 
-#else
+static void __init arc_cs_setup_gfrc(struct device_node *node)
+{
+	int exists = cpuinfo_arc700[0].extn.gfrc;
+
+	if (WARN(!exists, "Global-64-bit-Ctr clocksource not detected"))
+		return;
+
+	arc_get_timer_clk(node);
+
+	clocksource_register_hz(&arc_counter_gfrc, arc_timer_freq);
+}
+CLOCKSOURCE_OF_DECLARE(arc_gfrc, "snps,archs-timer-gfrc", arc_cs_setup_gfrc);
+
+#endif
 
 #ifdef CONFIG_ARC_HAS_RTC
 
@@ -123,15 +131,7 @@ static struct clocksource arc_counter = {
 #define AUX_RTC_LOW	0x104
 #define AUX_RTC_HIGH	0x105
 
-int arc_counter_setup(void)
-{
-	write_aux_reg(AUX_RTC_CTRL, 1);
-
-	/* Not usable in SMP */
-	return !IS_ENABLED(CONFIG_SMP);
-}
-
-static cycle_t arc_counter_read(struct clocksource *cs)
+static cycle_t arc_read_rtc(struct clocksource *cs)
 {
 	unsigned long status;
 	union {
@@ -155,44 +155,66 @@ static cycle_t arc_counter_read(struct clocksource *cs)
 	return stamp.full;
 }
 
-static struct clocksource arc_counter = {
+static struct clocksource arc_counter_rtc = {
 	.name   = "ARCv2 RTC",
 	.rating = 350,
-	.read   = arc_counter_read,
+	.read   = arc_read_rtc,
 	.mask   = CLOCKSOURCE_MASK(64),
 	.flags  = CLOCK_SOURCE_IS_CONTINUOUS,
 };
 
-#else /* !CONFIG_ARC_HAS_RTC */
-
-/*
- * set 32bit TIMER1 to keep counting monotonically and wraparound
- */
-int arc_counter_setup(void)
+static void __init arc_cs_setup_rtc(struct device_node *node)
 {
-	write_aux_reg(ARC_REG_TIMER1_LIMIT, ARC_TIMER_MAX);
-	write_aux_reg(ARC_REG_TIMER1_CNT, 0);
-	write_aux_reg(ARC_REG_TIMER1_CTRL, TIMER_CTRL_NH);
+	int exists = cpuinfo_arc700[smp_processor_id()].extn.rtc;
+
+	if (WARN(!exists, "Local-64-bit-Ctr clocksource not detected"))
+		return;
+
+	/* Local to CPU hence not usable in SMP */
+	if (WARN(IS_ENABLED(CONFIG_SMP), "Local-64-bit-Ctr not usable in SMP"))
+		return;
+
+	arc_get_timer_clk(node);
 
-	/* Not usable in SMP */
-	return !IS_ENABLED(CONFIG_SMP);
+	write_aux_reg(AUX_RTC_CTRL, 1);
+
+	clocksource_register_hz(&arc_counter_rtc, arc_timer_freq);
 }
+CLOCKSOURCE_OF_DECLARE(arc_rtc, "snps,archs-timer-rtc", arc_cs_setup_rtc);
+
+#endif
+
+/*
+ * 32bit TIMER1 to keep counting monotonically and wraparound
+ */
 
-static cycle_t arc_counter_read(struct clocksource *cs)
+static cycle_t arc_read_timer1(struct clocksource *cs)
 {
 	return (cycle_t) read_aux_reg(ARC_REG_TIMER1_CNT);
 }
 
-static struct clocksource arc_counter = {
+static struct clocksource arc_counter_timer1 = {
 	.name   = "ARC Timer1",
 	.rating = 300,
-	.read   = arc_counter_read,
+	.read   = arc_read_timer1,
 	.mask   = CLOCKSOURCE_MASK(32),
 	.flags  = CLOCK_SOURCE_IS_CONTINUOUS,
 };
 
-#endif
-#endif
+static void __init arc_cs_setup_timer1(struct device_node *node)
+{
+	/* Local to CPU hence not usable in SMP */
+	if (IS_ENABLED(CONFIG_SMP))
+		return;
+
+	arc_get_timer_clk(node);
+
+	write_aux_reg(ARC_REG_TIMER1_LIMIT, ARC_TIMER_MAX);
+	write_aux_reg(ARC_REG_TIMER1_CNT, 0);
+	write_aux_reg(ARC_REG_TIMER1_CTRL, TIMER_CTRL_NH);
+
+	clocksource_register_hz(&arc_counter_timer1, arc_timer_freq);
+}
 
 /********** Clock Event Device *********/
 
@@ -312,29 +334,25 @@ static void __init arc_clockevent_setup(struct device_node *node)
 
 	enable_percpu_irq(arc_timer_irq, 0);
 }
-CLOCKSOURCE_OF_DECLARE(arc_clkevt, "snps,arc-timer", arc_clockevent_setup);
+
+static void __init arc_of_timer_init(struct device_node *np)
+{
+	static int init_count = 0;
+
+	if (!init_count) {
+		init_count = 1;
+		arc_clockevent_setup(np);
+	} else {
+		arc_cs_setup_timer1(np);
+	}
+}
+CLOCKSOURCE_OF_DECLARE(arc_clkevt, "snps,arc-timer", arc_of_timer_init);
 
 /*
  * Called from start_kernel() - boot CPU only
- *
- * -Sets up h/w timers as applicable on boot cpu
- * -Also sets up any global state needed for timer subsystem:
- *    - for "counting" timer, registers a clocksource, usable across CPUs
- *      (provided that underlying counter h/w is synchronized across cores)
  */
 void __init time_init(void)
 {
 	of_clk_init(NULL);
 	clocksource_probe();
-
-	/*
-	 * sets up the timekeeping free-flowing counter which also returns
-	 * whether the counter is usable as clocksource
-	 */
-	if (arc_counter_setup())
-		/*
-		 * CLK upto 4.29 GHz can be safely represented in 32 bits
-		 * because Max 32 bit number is 4,294,967,295
-		 */
-		clocksource_register_hz(&arc_counter, arc_timer_freq);
 }
-- 
2.5.0

Re: [PATCH v4 2/5] ARC: clocksource: DT based probe

[Marc Zyngier]

On 13/04/16 12:40, Vineet Gupta wrote:
> - Remove explicit clocksource setup and let it be done by OF framework
>   by defining CLOCKSOURCE_OF_DECLARE() for various timers
> 
> - This allows multiple clocksources to be potentially registered
>   simultaneouly: previously we could only do one - as all of them had
>   same arc_counter_setup() routine for registration
> 
> - Setup routines also ensure that the underlying timer actually exists.
> 
> - Remove some of the panic() calls if underlying timer is NOT detected as
>   fallback clocksource might still be available
>   1. If GRFC doesn't exist, jiffies clocksource gets registered anyways
>   2. if RTC doesn't exist, TIMER1 can take over (as it is always
>      present)
> 
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> ---
>  arch/arc/kernel/mcip.c  |   4 +-
>  arch/arc/kernel/setup.c |   3 --
>  arch/arc/kernel/time.c  | 124 +++++++++++++++++++++++++++---------------------
>  3 files changed, 72 insertions(+), 59 deletions(-)
> 
> diff --git a/arch/arc/kernel/mcip.c b/arch/arc/kernel/mcip.c
> index c41c364b926c..262d9c3771e6 100644
> --- a/arch/arc/kernel/mcip.c
> +++ b/arch/arc/kernel/mcip.c
> @@ -116,15 +116,13 @@ static void mcip_probe_n_setup(void)
>  		IS_AVAIL1(mp.dbg, "DEBUG "),
>  		IS_AVAIL1(mp.gfrc, "GFRC"));
>  
> +	cpuinfo_arc700[0].extn.gfrc = mp.gfrc;
>  	idu_detected = mp.idu;
>  
>  	if (mp.dbg) {
>  		__mcip_cmd_data(CMD_DEBUG_SET_SELECT, 0, 0xf);
>  		__mcip_cmd_data(CMD_DEBUG_SET_MASK, 0xf, 0xf);
>  	}
> -
> -	if (IS_ENABLED(CONFIG_ARC_HAS_GFRC) && !mp.gfrc)
> -		panic("kernel trying to use non-existent GFRC\n");
>  }
>  
>  struct plat_smp_ops plat_smp_ops = {
> diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
> index 507ec523112a..91f79fa447bc 100644
> --- a/arch/arc/kernel/setup.c
> +++ b/arch/arc/kernel/setup.c
> @@ -313,9 +313,6 @@ static void arc_chk_core_config(void)
>  	if (!cpu->extn.timer1)
>  		panic("Timer1 is not present!\n");
>  
> -	if (IS_ENABLED(CONFIG_ARC_HAS_RTC) && !cpu->extn.rtc)
> -		panic("RTC is not present\n");
> -
>  #ifdef CONFIG_ARC_HAS_DCCM
>  	/*
>  	 * DCCM can be arbit placed in hardware.
> diff --git a/arch/arc/kernel/time.c b/arch/arc/kernel/time.c
> index 693545df9827..72f023440739 100644
> --- a/arch/arc/kernel/time.c
> +++ b/arch/arc/kernel/time.c
> @@ -77,12 +77,7 @@ static void noinline arc_get_timer_clk(struct device_node *node)
>  
>  #ifdef CONFIG_ARC_HAS_GFRC
>  
> -static int arc_counter_setup(void)
> -{
> -	return 1;
> -}
> -
> -static cycle_t arc_counter_read(struct clocksource *cs)
> +static cycle_t arc_read_gfrc(struct clocksource *cs)
>  {
>  	unsigned long flags;
>  	union {
> @@ -107,15 +102,28 @@ static cycle_t arc_counter_read(struct clocksource *cs)
>  	return stamp.full;
>  }
>  
> -static struct clocksource arc_counter = {
> +static struct clocksource arc_counter_gfrc = {
>  	.name   = "ARConnect GFRC",
>  	.rating = 400,
> -	.read   = arc_counter_read,
> +	.read   = arc_read_gfrc,
>  	.mask   = CLOCKSOURCE_MASK(64),
>  	.flags  = CLOCK_SOURCE_IS_CONTINUOUS,
>  };
>  
> -#else
> +static void __init arc_cs_setup_gfrc(struct device_node *node)
> +{
> +	int exists = cpuinfo_arc700[0].extn.gfrc;
> +
> +	if (WARN(!exists, "Global-64-bit-Ctr clocksource not detected"))
> +		return;
> +
> +	arc_get_timer_clk(node);
> +
> +	clocksource_register_hz(&arc_counter_gfrc, arc_timer_freq);
> +}
> +CLOCKSOURCE_OF_DECLARE(arc_gfrc, "snps,archs-timer-gfrc", arc_cs_setup_gfrc);
> +
> +#endif
>  
>  #ifdef CONFIG_ARC_HAS_RTC
>  
> @@ -123,15 +131,7 @@ static struct clocksource arc_counter = {
>  #define AUX_RTC_LOW	0x104
>  #define AUX_RTC_HIGH	0x105
>  
> -int arc_counter_setup(void)
> -{
> -	write_aux_reg(AUX_RTC_CTRL, 1);
> -
> -	/* Not usable in SMP */
> -	return !IS_ENABLED(CONFIG_SMP);
> -}
> -
> -static cycle_t arc_counter_read(struct clocksource *cs)
> +static cycle_t arc_read_rtc(struct clocksource *cs)
>  {
>  	unsigned long status;
>  	union {
> @@ -155,44 +155,66 @@ static cycle_t arc_counter_read(struct clocksource *cs)
>  	return stamp.full;
>  }
>  
> -static struct clocksource arc_counter = {
> +static struct clocksource arc_counter_rtc = {
>  	.name   = "ARCv2 RTC",
>  	.rating = 350,
> -	.read   = arc_counter_read,
> +	.read   = arc_read_rtc,
>  	.mask   = CLOCKSOURCE_MASK(64),
>  	.flags  = CLOCK_SOURCE_IS_CONTINUOUS,
>  };
>  
> -#else /* !CONFIG_ARC_HAS_RTC */
> -
> -/*
> - * set 32bit TIMER1 to keep counting monotonically and wraparound
> - */
> -int arc_counter_setup(void)
> +static void __init arc_cs_setup_rtc(struct device_node *node)
>  {
> -	write_aux_reg(ARC_REG_TIMER1_LIMIT, ARC_TIMER_MAX);
> -	write_aux_reg(ARC_REG_TIMER1_CNT, 0);
> -	write_aux_reg(ARC_REG_TIMER1_CTRL, TIMER_CTRL_NH);
> +	int exists = cpuinfo_arc700[smp_processor_id()].extn.rtc;
> +
> +	if (WARN(!exists, "Local-64-bit-Ctr clocksource not detected"))
> +		return;
> +
> +	/* Local to CPU hence not usable in SMP */
> +	if (WARN(IS_ENABLED(CONFIG_SMP), "Local-64-bit-Ctr not usable in SMP"))
> +		return;

Sorry if this outlines my lack of understanding of the ARC architecture,
but what makes per-cpu timer unsuitable for SMP? I'd have thought that
it was actually what you wanted...

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4 2/5] ARC: clocksource: DT based probe

[Vineet Gupta]

On Wednesday 13 April 2016 09:52 PM, Marc Zyngier wrote:
>> -int arc_counter_setup(void)
>> > +static void __init arc_cs_setup_rtc(struct device_node *node)
>> >  {
>> > -	write_aux_reg(ARC_REG_TIMER1_LIMIT, ARC_TIMER_MAX);
>> > -	write_aux_reg(ARC_REG_TIMER1_CNT, 0);
>> > -	write_aux_reg(ARC_REG_TIMER1_CTRL, TIMER_CTRL_NH);
>> > +	int exists = cpuinfo_arc700[smp_processor_id()].extn.rtc;
>> > +
>> > +	if (WARN(!exists, "Local-64-bit-Ctr clocksource not detected"))
>> > +		return;
>> > +
>> > +	/* Local to CPU hence not usable in SMP */
>> > +	if (WARN(IS_ENABLED(CONFIG_SMP), "Local-64-bit-Ctr not usable in SMP"))
>> > +		return;
> Sorry if this outlines my lack of understanding of the ARC architecture,
> but what makes per-cpu timer unsuitable for SMP? I'd have thought that
> it was actually what you wanted...

This is clocksource, not clockevent. cs needs to synchronized across all cores so
that concurrent gtod call from threads on different cores gives you similar
values. This obviously is not true for the local RTC hardware timer.

Re: [PATCH v4 2/5] ARC: clocksource: DT based probe

[Marc Zyngier]

On 14/04/16 10:26, Vineet Gupta wrote:
> On Wednesday 13 April 2016 09:52 PM, Marc Zyngier wrote:
>>> -int arc_counter_setup(void)
>>>> +static void __init arc_cs_setup_rtc(struct device_node *node)
>>>>  {
>>>> -	write_aux_reg(ARC_REG_TIMER1_LIMIT, ARC_TIMER_MAX);
>>>> -	write_aux_reg(ARC_REG_TIMER1_CNT, 0);
>>>> -	write_aux_reg(ARC_REG_TIMER1_CTRL, TIMER_CTRL_NH);
>>>> +	int exists = cpuinfo_arc700[smp_processor_id()].extn.rtc;
>>>> +
>>>> +	if (WARN(!exists, "Local-64-bit-Ctr clocksource not detected"))
>>>> +		return;
>>>> +
>>>> +	/* Local to CPU hence not usable in SMP */
>>>> +	if (WARN(IS_ENABLED(CONFIG_SMP), "Local-64-bit-Ctr not usable in SMP"))
>>>> +		return;
>> Sorry if this outlines my lack of understanding of the ARC architecture,
>> but what makes per-cpu timer unsuitable for SMP? I'd have thought that
>> it was actually what you wanted...
> 
> This is clocksource, not clockevent. cs needs to synchronized across all cores so
> that concurrent gtod call from threads on different cores gives you similar
> values. This obviously is not true for the local RTC hardware timer.

Unsynchronized counters on SMP HW, who would have thought! ;-) I guess
each and every architecture has to repeat the same mistakes.

Thanks for shedding some light on it.

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v4 2/5] ARC: clocksource: DT based probe

[Vineet Gupta]

On Thursday 14 April 2016 03:02 PM, Marc Zyngier wrote:
>> This is clocksource, not clockevent. cs needs to synchronized across all cores so
>> > that concurrent gtod call from threads on different cores gives you similar
>> > values. This obviously is not true for the local RTC hardware timer.
> Unsynchronized counters on SMP HW, who would have thought! ;-) I guess
> each and every architecture has to repeat the same mistakes.

Not really - hardware wise the SMP support is pretty recent and before that we
only had UP cores. So transition from 32 TIMER1 to 64 bit RTC was only a natural
progression in improvements and in theory SoC guys throw in this local timer into
the config. We just prevent SMP linux from using it.

[PATCH v4 3/5] ARC: irq: export some IRQs again

[Vineet Gupta]

This will be needed for switching ti lineaser irq domain as
irq_create_mapping() called by intr code, needs the IRQ numbers as well
in addition to existing usage in mcip.c

Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 arch/arc/include/asm/irq.h | 6 ++++++
 arch/arc/kernel/mcip.c     | 3 ---
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/arc/include/asm/irq.h b/arch/arc/include/asm/irq.h
index a6ac89dc228f..c0fa0d2de400 100644
--- a/arch/arc/include/asm/irq.h
+++ b/arch/arc/include/asm/irq.h
@@ -12,6 +12,12 @@
 #define NR_CPU_IRQS	32  /* number of interrupt lines of ARC770 CPU */
 #define NR_IRQS		128 /* allow some CPU external IRQ handling */
 
+/* Platform Independent IRQs */
+#ifdef CONFIG_ISA_ARCV2
+#define IPI_IRQ		19
+#define SOFTIRQ_IRQ	21
+#endif
+
 #include <linux/interrupt.h>
 #include <asm-generic/irq.h>
 
diff --git a/arch/arc/kernel/mcip.c b/arch/arc/kernel/mcip.c
index 262d9c3771e6..72f9179b1a24 100644
--- a/arch/arc/kernel/mcip.c
+++ b/arch/arc/kernel/mcip.c
@@ -15,9 +15,6 @@
 #include <asm/mcip.h>
 #include <asm/setup.h>
 
-#define IPI_IRQ		19
-#define SOFTIRQ_IRQ	21
-
 static char smp_cpuinfo_buf[128];
 static int idu_detected;
 
-- 
2.5.0

[PATCH v4 4/5] ARC: [intc-*] Do a domain lookup in primary handler for hwirq -> linux virq

[Vineet Gupta]

The primary interrupt handler arch_do_IRQ() was passing hwirq as linux
virq to core code. This was fragile and worked so far as we only had legacy/linear
domains.

This came out of a rant by Marc Zyngier.
http://lists.infradead.org/pipermail/linux-snps-arc/2015-December/000298.html

Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Noam Camus <noamc@ezchip.com>
Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 arch/arc/Kconfig               |  1 +
 arch/arc/kernel/intc-arcv2.c   |  9 ++++++---
 arch/arc/kernel/intc-compact.c | 10 ++++++----
 arch/arc/kernel/irq.c          |  9 ++-------
 4 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/arch/arc/Kconfig b/arch/arc/Kconfig
index 80b222651a90..bf0a4fd45d25 100644
--- a/arch/arc/Kconfig
+++ b/arch/arc/Kconfig
@@ -31,6 +31,7 @@ config ARC
 	select HAVE_MOD_ARCH_SPECIFIC if ARC_DW2_UNWIND
 	select HAVE_OPROFILE
 	select HAVE_PERF_EVENTS
+	select HANDLE_DOMAIN_IRQ
 	select IRQ_DOMAIN
 	select MODULES_USE_ELF_RELA
 	select NO_BOOTMEM
diff --git a/arch/arc/kernel/intc-arcv2.c b/arch/arc/kernel/intc-arcv2.c
index 942526322ae7..592cc977151e 100644
--- a/arch/arc/kernel/intc-arcv2.c
+++ b/arch/arc/kernel/intc-arcv2.c
@@ -137,21 +137,24 @@ static const struct irq_domain_ops arcv2_irq_ops = {
 	.map = arcv2_irq_map,
 };
 
-static struct irq_domain *root_domain;
 
 static int __init
 init_onchip_IRQ(struct device_node *intc, struct device_node *parent)
 {
+	struct irq_domain *root_domain;
+
 	if (parent)
 		panic("DeviceTree incore intc not a root irq controller\n");
 
 	root_domain = irq_domain_add_legacy(intc, NR_CPU_IRQS, 0, 0,
 					    &arcv2_irq_ops, NULL);
-
 	if (!root_domain)
 		panic("root irq domain not avail\n");
 
-	/* with this we don't need to export root_domain */
+	/*
+	 * Needed for primary domain lookup to succeed
+	 * This is a primary irqchip, and can never have a parent
+	 */
 	irq_set_default_host(root_domain);
 
 	return 0;
diff --git a/arch/arc/kernel/intc-compact.c b/arch/arc/kernel/intc-compact.c
index d31bc647146d..48a8b24de23e 100644
--- a/arch/arc/kernel/intc-compact.c
+++ b/arch/arc/kernel/intc-compact.c
@@ -97,21 +97,23 @@ static const struct irq_domain_ops arc_intc_domain_ops = {
 	.map = arc_intc_domain_map,
 };
 
-static struct irq_domain *root_domain;
-
 static int __init
 init_onchip_IRQ(struct device_node *intc, struct device_node *parent)
 {
+	struct irq_domain *root_domain;
+
 	if (parent)
 		panic("DeviceTree incore intc not a root irq controller\n");
 
 	root_domain = irq_domain_add_legacy(intc, NR_CPU_IRQS, 0, 0,
 					    &arc_intc_domain_ops, NULL);
-
 	if (!root_domain)
 		panic("root irq domain not avail\n");
 
-	/* with this we don't need to export root_domain */
+	/*
+	 * Needed for primary domain lookup to succeed
+	 * This is a primary irqchip, and can never have a parent
+	 */
 	irq_set_default_host(root_domain);
 
 	return 0;
diff --git a/arch/arc/kernel/irq.c b/arch/arc/kernel/irq.c
index fb6dede9d05f..538b36afe89e 100644
--- a/arch/arc/kernel/irq.c
+++ b/arch/arc/kernel/irq.c
@@ -41,12 +41,7 @@ void __init init_IRQ(void)
  * "C" Entry point for any ARC ISR, called from low level vector handler
  * @irq is the vector number read from ICAUSE reg of on-chip intc
  */
-void arch_do_IRQ(unsigned int irq, struct pt_regs *regs)
+void arch_do_IRQ(unsigned int hwirq, struct pt_regs *regs)
 {
-	struct pt_regs *old_regs = set_irq_regs(regs);
-
-	irq_enter();
-	generic_handle_irq(irq);
-	irq_exit();
-	set_irq_regs(old_regs);
+	handle_domain_irq(NULL, hwirq, regs);
 }
-- 
2.5.0

[PATCH v4 5/5] ARC: [intc-*] switch to linear domain

[Vineet Gupta]

Now that we have Timers probed from DT, don't need legacy domain

This however requires mapping to be called explicitly for the IRQ which
still can't (and probably never) be probed from DT such as IPI and
SOFTIRQ

Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 arch/arc/kernel/intc-arcv2.c   | 8 ++++++--
 arch/arc/kernel/intc-compact.c | 2 +-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/arc/kernel/intc-arcv2.c b/arch/arc/kernel/intc-arcv2.c
index 592cc977151e..6c24faf48b16 100644
--- a/arch/arc/kernel/intc-arcv2.c
+++ b/arch/arc/kernel/intc-arcv2.c
@@ -146,8 +146,7 @@ init_onchip_IRQ(struct device_node *intc, struct device_node *parent)
 	if (parent)
 		panic("DeviceTree incore intc not a root irq controller\n");
 
-	root_domain = irq_domain_add_legacy(intc, NR_CPU_IRQS, 0, 0,
-					    &arcv2_irq_ops, NULL);
+	root_domain = irq_domain_add_linear(intc, NR_CPU_IRQS, &arcv2_irq_ops, NULL);
 	if (!root_domain)
 		panic("root irq domain not avail\n");
 
@@ -157,6 +156,11 @@ init_onchip_IRQ(struct device_node *intc, struct device_node *parent)
 	 */
 	irq_set_default_host(root_domain);
 
+#ifdef CONFIG_SMP
+	irq_create_mapping(root_domain, IPI_IRQ);
+#endif
+	irq_create_mapping(root_domain, SOFTIRQ_IRQ);
+
 	return 0;
 }
 
diff --git a/arch/arc/kernel/intc-compact.c b/arch/arc/kernel/intc-compact.c
index 48a8b24de23e..c5cceca36118 100644
--- a/arch/arc/kernel/intc-compact.c
+++ b/arch/arc/kernel/intc-compact.c
@@ -105,7 +105,7 @@ init_onchip_IRQ(struct device_node *intc, struct device_node *parent)
 	if (parent)
 		panic("DeviceTree incore intc not a root irq controller\n");
 
-	root_domain = irq_domain_add_legacy(intc, NR_CPU_IRQS, 0, 0,
+	root_domain = irq_domain_add_linear(intc, NR_CPU_IRQS,
 					    &arc_intc_domain_ops, NULL);
 	if (!root_domain)
 		panic("root irq domain not avail\n");
-- 
2.5.0

Re: [PATCH v4 5/5] ARC: [intc-*] switch to linear domain

[Vineet Gupta]

Hi Marc,

On Wednesday 13 April 2016 05:10 PM, Vineet Gupta wrote:
> Now that we have Timers probed from DT, don't need legacy domain
> 
> This however requires mapping to be called explicitly for the IRQ which
> still can't (and probably never) be probed from DT such as IPI and
> SOFTIRQ
> 
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> ---
>  arch/arc/kernel/intc-arcv2.c   | 8 ++++++--
>  arch/arc/kernel/intc-compact.c | 2 +-
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arc/kernel/intc-arcv2.c b/arch/arc/kernel/intc-arcv2.c
> index 592cc977151e..6c24faf48b16 100644
> --- a/arch/arc/kernel/intc-arcv2.c
> +++ b/arch/arc/kernel/intc-arcv2.c
> @@ -146,8 +146,7 @@ init_onchip_IRQ(struct device_node *intc, struct device_node *parent)
>  	if (parent)
>  		panic("DeviceTree incore intc not a root irq controller\n");
>  
> -	root_domain = irq_domain_add_legacy(intc, NR_CPU_IRQS, 0, 0,
> -					    &arcv2_irq_ops, NULL);
> +	root_domain = irq_domain_add_linear(intc, NR_CPU_IRQS, &arcv2_irq_ops, NULL);
>  	if (!root_domain)
>  		panic("root irq domain not avail\n");
>  
> @@ -157,6 +156,11 @@ init_onchip_IRQ(struct device_node *intc, struct device_node *parent)
>  	 */
>  	irq_set_default_host(root_domain);
>  
> +#ifdef CONFIG_SMP
> +	irq_create_mapping(root_domain, IPI_IRQ);
> +#endif
> +	irq_create_mapping(root_domain, SOFTIRQ_IRQ);
> +
>  	return 0;
>  }
>  
> diff --git a/arch/arc/kernel/intc-compact.c b/arch/arc/kernel/intc-compact.c
> index 48a8b24de23e..c5cceca36118 100644
> --- a/arch/arc/kernel/intc-compact.c
> +++ b/arch/arc/kernel/intc-compact.c
> @@ -105,7 +105,7 @@ init_onchip_IRQ(struct device_node *intc, struct device_node *parent)
>  	if (parent)
>  		panic("DeviceTree incore intc not a root irq controller\n");
>  
> -	root_domain = irq_domain_add_legacy(intc, NR_CPU_IRQS, 0, 0,
> +	root_domain = irq_domain_add_linear(intc, NR_CPU_IRQS,
>  					    &arc_intc_domain_ops, NULL);
>  	if (!root_domain)
>  		panic("root irq domain not avail\n");
> 

Does this patch look ok to you ? Do you want to see any other improvements here.

-Vineet

Re: [PATCH v4 5/5] ARC: [intc-*] switch to linear domain

[Marc Zyngier]

On 18/04/16 07:51, Vineet Gupta wrote:
> Hi Marc,
> 
> On Wednesday 13 April 2016 05:10 PM, Vineet Gupta wrote:
>> Now that we have Timers probed from DT, don't need legacy domain
>>
>> This however requires mapping to be called explicitly for the IRQ which
>> still can't (and probably never) be probed from DT such as IPI and
>> SOFTIRQ
>>
>> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>> ---
>>  arch/arc/kernel/intc-arcv2.c   | 8 ++++++--
>>  arch/arc/kernel/intc-compact.c | 2 +-
>>  2 files changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arc/kernel/intc-arcv2.c b/arch/arc/kernel/intc-arcv2.c
>> index 592cc977151e..6c24faf48b16 100644
>> --- a/arch/arc/kernel/intc-arcv2.c
>> +++ b/arch/arc/kernel/intc-arcv2.c
>> @@ -146,8 +146,7 @@ init_onchip_IRQ(struct device_node *intc, struct device_node *parent)
>>  	if (parent)
>>  		panic("DeviceTree incore intc not a root irq controller\n");
>>  
>> -	root_domain = irq_domain_add_legacy(intc, NR_CPU_IRQS, 0, 0,
>> -					    &arcv2_irq_ops, NULL);
>> +	root_domain = irq_domain_add_linear(intc, NR_CPU_IRQS, &arcv2_irq_ops, NULL);
>>  	if (!root_domain)
>>  		panic("root irq domain not avail\n");
>>  
>> @@ -157,6 +156,11 @@ init_onchip_IRQ(struct device_node *intc, struct device_node *parent)
>>  	 */
>>  	irq_set_default_host(root_domain);
>>  
>> +#ifdef CONFIG_SMP
>> +	irq_create_mapping(root_domain, IPI_IRQ);
>> +#endif
>> +	irq_create_mapping(root_domain, SOFTIRQ_IRQ);
>> +
>>  	return 0;
>>  }
>>  
>> diff --git a/arch/arc/kernel/intc-compact.c b/arch/arc/kernel/intc-compact.c
>> index 48a8b24de23e..c5cceca36118 100644
>> --- a/arch/arc/kernel/intc-compact.c
>> +++ b/arch/arc/kernel/intc-compact.c
>> @@ -105,7 +105,7 @@ init_onchip_IRQ(struct device_node *intc, struct device_node *parent)
>>  	if (parent)
>>  		panic("DeviceTree incore intc not a root irq controller\n");
>>  
>> -	root_domain = irq_domain_add_legacy(intc, NR_CPU_IRQS, 0, 0,
>> +	root_domain = irq_domain_add_linear(intc, NR_CPU_IRQS,
>>  					    &arc_intc_domain_ops, NULL);
>>  	if (!root_domain)
>>  		panic("root irq domain not avail\n");
>>
> 
> Does this patch look ok to you ? Do you want to see any other improvements here.

Decoupling HW irqs from Linux irqs is a major progress, so for that:

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...