Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1512475 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 3/3] clocksource: Add clockevent support to NPS400  driver Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-10-31 12:00 +0100
    Re: [PATCH v2 3/3] clocksource: Add clockevent support to NPS400  driver Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-10-31 19:00 +0100
      [PATCH 7/9] ARC: breakout timer stuff into a seperate header Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-10-31 23:50 +0100
        Re: [PATCH 7/9] ARC: breakout timer stuff into a seperate header Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-03 18:30 +0100
      [PATCH 5/9] ARC: breakout aux handling into a seperate header Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-10-31 23:50 +0100
        RE: [PATCH 5/9] ARC: breakout aux handling into a seperate header Noam Camus <noamca@mellanox.com> - 2016-11-01 10:30 +0100
      [PATCH 3/9] ARC: timer: gfrc: boot print alongside other timers Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-10-31 23:50 +0100
        Re: [PATCH 3/9] ARC: timer: gfrc: boot print alongside other timers Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-03 18:20 +0100
          Re: [PATCH 3/9] ARC: timer: gfrc: boot print alongside other timers Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-03 18:50 +0100
            Re: [PATCH 3/9] ARC: timer: gfrc: boot print alongside other timers Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-03 19:00 +0100
      [PATCH 1/9] ARC: timer: gfrc, rtc: Read BCR to detect whether hardware exists ... Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-10-31 23:50 +0100
        Re: [PATCH 1/9] ARC: timer: gfrc, rtc: Read BCR to detect whether  hardware exists ... Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-03 18:10 +0100
          Re: [PATCH 1/9] ARC: timer: gfrc, rtc: Read BCR to detect whether  hardware exists ... Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-03 18:50 +0100
      [PATCH 8/9] ARC: timer: rename config options Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-10-31 23:50 +0100
      [PATCH 0/9] Move ARC timer code into drivers/clocksource/ Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-10-31 23:50 +0100
        [PATCH 9/9] clocksource: import ARC timer driver Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-10-31 23:50 +0100
          Re: [PATCH 9/9] clocksource: import ARC timer driver Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-01 01:50 +0100
          Re: [PATCH 9/9] clocksource: import ARC timer driver Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-01 21:50 +0100
            Re: [PATCH 9/9] clocksource: import ARC timer driver Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-01 22:00 +0100
              Re: [PATCH 9/9] clocksource: import ARC timer driver Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-02 01:30 +0100
                Re: [PATCH 9/9] clocksource: import ARC timer driver Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-02 02:10 +0100
                  Re: [PATCH 9/9] clocksource: import ARC timer driver Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-03 17:50 +0100
                    Re: [PATCH 9/9] clocksource: import ARC timer driver Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-03 18:00 +0100
                      Re: [PATCH 9/9] clocksource: import ARC timer driver Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-03 19:00 +0100
                        Re: [PATCH 9/9] clocksource: import ARC timer driver Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-03 19:20 +0100
                          Re: [PATCH 9/9] clocksource: import ARC timer driver Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-03 19:50 +0100
              Re: [PATCH 9/9] clocksource: import ARC timer driver Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-03 18:40 +0100
                Re: [PATCH 9/9] clocksource: import ARC timer driver Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-03 19:20 +0100
                Re: [PATCH 9/9] clocksource: import ARC timer driver Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-03 20:00 +0100
        [PATCH 4/9] ARC: time: move time_init() out of the driver Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-10-31 23:50 +0100
          Re: [PATCH 4/9] ARC: time: move time_init() out of the driver Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-03 18:20 +0100
        [PATCH 6/9] ARC: move mcip.h into include/soc and adjust the includes Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-01 00:00 +0100
          Re: [PATCH 6/9] ARC: move mcip.h into include/soc and adjust the  includes Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-03 18:30 +0100
        [PATCH 2/9] ARC: timer: rtc: implement read loop in "C" vs. inline asm Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-01 00:00 +0100
          Re: [PATCH 2/9] ARC: timer: rtc: implement read loop in "C" vs.  inline asm Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-03 18:10 +0100
            Re: [PATCH 2/9] ARC: timer: rtc: implement read loop in "C" vs.  inline asm Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-03 18:50 +0100
        Re: [PATCH 0/9] Move ARC timer code into drivers/clocksource/ Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-03 18:30 +0100
    RE: [PATCH v2 3/3] clocksource: Add clockevent support to NPS400  driver Noam Camus <noamca@mellanox.com> - 2016-11-01 01:40 +0100
      Re: [PATCH v2 3/3] clocksource: Add clockevent support to NPS400  driver Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-01 21:10 +0100
        RE: [PATCH v2 3/3] clocksource: Add clockevent support to NPS400  driver Noam Camus <noamca@mellanox.com> - 2016-11-08 12:10 +0100
          Re: [PATCH v2 3/3] clocksource: Add clockevent support to NPS400  driver Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-10 11:40 +0100
            RE: [PATCH v2 3/3] clocksource: Add clockevent support to NPS400  driver Noam Camus <noamca@mellanox.com> - 2016-11-10 14:10 +0100

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1513599 — Re: [PATCH 9/9] clocksource: import ARC timer driver

On 11/01/2016 05:19 PM, Daniel Lezcano wrote:
>>>
>>> One question:
>>>
>>> Why ARC_TIMER_RTC can't be used in a SMP system ? Doesn't have each core its
>>> own clocksource ? It seems you are assuming a clocksource can be used on SMP
>>> only if the clocksource is unique and shared across the cores.
>>
>> Thats what I thought so far. Thing is, the individual core's counters could get
>> out of sync, simply because non masters cores were halted to begin with and came
>> up at different points in real time. so a gtod might return different value
>> depending on what core it landed on. Does clocksource also does ticks broadcasts
>> and such to keep things in sync ?
> 
> Sounds like it is similar than the TSC. Do you agree to have a try by setting
> the CONFIG_HAVE_UNSTABLE_SCHED_CLOCK option ?

I'm not sure why we would want to enable extra stuff - I see work queues and bunch
of per cpu counting / math to adjust for the variance, if this was enabled. Anyhow
see more below.

> If you can use those per cpu clocksource, performances on your system may
> improve with the sched_clock().

Couple of things

1. Currently we don't hookup sched clock to any counter at all (on my todo list
for a while). So we only get jiffies64 based value - I know that sucks - causes
scheduling to be not super accurate etc - potentially affects benchmarks etc - but
that can be fixed easily / independent of this.

2. Say we did have sched_clock() driven by hardware - in SMP system I would still
prefer it to be driven by "common" GFRC and not "per cpu" RTC. The overhead of
HAVE_UNSTABLE_SCHED_CLOCK looks way way more than reading GFRC counter like this.

	local_irq_save(flags);

	__mcip_cmd(CMD_GFRC_READ_LO, 0);
	stamp.l = read_aux_reg(ARC_REG_MCIP_READBACK);

	__mcip_cmd(CMD_GFRC_READ_HI, 0);
	stamp.h = read_aux_reg(ARC_REG_MCIP_READBACK);

	local_irq_restore(flags);

GFRC reading by 2 cores concurrently doesn't require any synchronization at all.
The irq disabling around it is to make sure we didn't get a bogus readout lest an
interrupt came in between the read of 2 words. But if sched_clock can guarantee
that irqs are disable - I can probably even remove it at least for the purpose of
sched clock.

However I think we are digressing here a bit. IMHO, what clock we choose to drive
sched should not really be driven by the driver. It must be for the arch to decide.

We should first focus on how the clockevent/sources are programmed first and then
dive into sched_clock_xx as that doesn't exist at the moment for ARC.

>  
>> Because of the git mv you, diff didn't include bulk of driver code which would
>> make for bulk of review anyways. So perhaps in v2 I don't do the git mv. OK ?
> 
> That means I will review and comment existing code. It is not a problem for me
> if you agree to do the changes.

Sure, the whole point is to make things better as an outcome of review. I have no
issues changing code provided we don't add major performance regressions.

Thx,
-Vineet

[toc] | [prev] | [next] | [standalone]

#1514692 — Re: [PATCH 9/9] clocksource: import ARC timer driver

On 11/01/2016 06:03 PM, Vineet Gupta wrote:
>>> Because of the git mv you, diff didn't include bulk of driver code which would
>>> >> make for bulk of review anyways. So perhaps in v2 I don't do the git mv. OK ?
>> > 
>> > That means I will review and comment existing code. It is not a problem for me
>> > if you agree to do the changes.
> Sure, the whole point is to make things better as an outcome of review. I have no
> issues changing code provided we don't add major performance regressions.

So just wondering if I could have some comments on the initial import of driver
before I send out a v2.

The issue is git mv didn't show bulk of code being moved. Shall I send a v2 with a
different ordering so I introduce the driver first, with new headers, new Kconfig
items etc and then as a subsequent patch prune those bits from arch/arc/* ?

-Vineet

[toc] | [prev] | [next] | [standalone]

#1514695 — Re: [PATCH 9/9] clocksource: import ARC timer driver

On Thu, Nov 03, 2016 at 09:40:23AM -0700, Vineet Gupta wrote:
> On 11/01/2016 06:03 PM, Vineet Gupta wrote:
> >>> Because of the git mv you, diff didn't include bulk of driver code which would
> >>> >> make for bulk of review anyways. So perhaps in v2 I don't do the git mv. OK ?
> >> > 
> >> > That means I will review and comment existing code. It is not a problem for me
> >> > if you agree to do the changes.
> > Sure, the whole point is to make things better as an outcome of review. I have no
> > issues changing code provided we don't add major performance regressions.
> 
> So just wondering if I could have some comments on the initial import of driver
> before I send out a v2.

Yeah, ok. Let me comment the other patches of the series and then you can send a V2.
 
> The issue is git mv didn't show bulk of code being moved. Shall I send a v2 with a
> different ordering so I introduce the driver first, with new headers, new Kconfig
> items etc and then as a subsequent patch prune those bits from arch/arc/* ?
> 
> -Vineet

[toc] | [prev] | [next] | [standalone]

#1514751 — Re: [PATCH 9/9] clocksource: import ARC timer driver

Hi Daniel,

On 11/03/2016 09:50 AM, Daniel Lezcano wrote:
> On Thu, Nov 03, 2016 at 09:40:23AM -0700, Vineet Gupta wrote:
>> On 11/01/2016 06:03 PM, Vineet Gupta wrote:
>>>>> Because of the git mv you, diff didn't include bulk of driver code which would
>>>>>>> make for bulk of review anyways. So perhaps in v2 I don't do the git mv. OK ?
>>>>>
>>>>> That means I will review and comment existing code. It is not a problem for me
>>>>> if you agree to do the changes.
>>> Sure, the whole point is to make things better as an outcome of review. I have no
>>> issues changing code provided we don't add major performance regressions.
>>
>> So just wondering if I could have some comments on the initial import of driver
>> before I send out a v2.
> 
> Yeah, ok. Let me comment the other patches of the series and then you can send a V2.

Thx for taking a quick look - this is a good start. How about the actual driver
itself, do you want to take a quick look there as well before v2 ?

>  
>> The issue is git mv didn't show bulk of code being moved. Shall I send a v2 with a
>> different ordering so I introduce the driver first, with new headers, new Kconfig
>> items etc and then as a subsequent patch prune those bits from arch/arc/* ?

[toc] | [prev] | [next] | [standalone]

#1514769 — Re: [PATCH 9/9] clocksource: import ARC timer driver

On Thu, Nov 03, 2016 at 10:57:48AM -0700, Vineet Gupta wrote:
> Hi Daniel,
> 
> On 11/03/2016 09:50 AM, Daniel Lezcano wrote:
> > On Thu, Nov 03, 2016 at 09:40:23AM -0700, Vineet Gupta wrote:
> >> On 11/01/2016 06:03 PM, Vineet Gupta wrote:
> >>>>> Because of the git mv you, diff didn't include bulk of driver code which would
> >>>>>>> make for bulk of review anyways. So perhaps in v2 I don't do the git mv. OK ?
> >>>>>
> >>>>> That means I will review and comment existing code. It is not a problem for me
> >>>>> if you agree to do the changes.
> >>> Sure, the whole point is to make things better as an outcome of review. I have no
> >>> issues changing code provided we don't add major performance regressions.
> >>
> >> So just wondering if I could have some comments on the initial import of driver
> >> before I send out a v2.
> > 
> > Yeah, ok. Let me comment the other patches of the series and then you can send a V2.
> 
> Thx for taking a quick look - this is a good start. How about the actual driver
> itself, do you want to take a quick look there as well before v2 ?

At the first glance, with your changes it is acceptable to be moved. Perhaps,
you can have a look to remove the BIG_ENDIAN stuff in the clock read function.

[toc] | [prev] | [next] | [standalone]

#1514785 — Re: [PATCH 9/9] clocksource: import ARC timer driver

On 11/03/2016 11:11 AM, Daniel Lezcano wrote:
>> Thx for taking a quick look - this is a good start. How about the actual driver
>> > itself, do you want to take a quick look there as well before v2 ?
> At the first glance, with your changes it is acceptable to be moved. Perhaps,
> you can have a look to remove the BIG_ENDIAN stuff in the clock read function.
> 

OK addressed that as well !

[toc] | [prev] | [next] | [standalone]

#1514729 — Re: [PATCH 9/9] clocksource: import ARC timer driver

On Tue, Nov 01, 2016 at 01:57:05PM -0700, Vineet Gupta wrote:
> Hi Daniel,
> 
> On 11/01/2016 01:42 PM, Daniel Lezcano wrote:
> > Please stay consistent with the rest of the Kconfig.
> > 
> > config ARC_TIMER_RTC
> > 	bool "64-bit cycle counter in HS38 cores" if COMPILE_TEST
> > 	select CLKSRC_OF
> > 	help
> > 	  This counter provides 64-bit resolution vs. the 32-bit TIMER1.
> > 	  It is implemented inside the core thus can't be used in SMP systems.
> > 
> > config ARC_TIMER_GFRC
> > 	bool "64-bit cycle counter in ARConnect block in HS38x cores" if COMPILE_TEST
> > 	select CLKSRC_OF
> > 	help
> > 	  This counter can be used as clocksource in SMP HS38 SoCs.
> > 	  It sits outside the core thus can be used in SMP systems
> > 
> 
> Yes I did so already :-) Although I also added a default y if ARC to both, but as
> you say that is better done in ARC Kconfig.
> 
> > Then in the ARC's Kconfig you select ARC_TIMER_RTC or ARC_TIMER_GFRC depending
> > it is SMP or not.
> > 
> > One question:
> > 
> > Why ARC_TIMER_RTC can't be used in a SMP system ? Doesn't have each core its
> > own clocksource ? It seems you are assuming a clocksource can be used on SMP
> > only if the clocksource is unique and shared across the cores.
> 

As now the clksrc-probe is correctly handling the errors, if the rtc and the
gfrc are both defined in the DT, you can fail to init the rtc one with a simple
test in the init function:

	if (IS_DEFINED(CONFIG_SMP))
		return -EINVAL;

So, you can inconditionaly compile in both RTC and GFRC, no ? That would be
cleaner and prevent a different kernel config.

[toc] | [prev] | [next] | [standalone]

#1514767 — Re: [PATCH 9/9] clocksource: import ARC timer driver

On Thu, Nov 03, 2016 at 06:33:17PM +0100, Daniel Lezcano wrote:

[ ... ]

> As now the clksrc-probe is correctly handling the errors, if the rtc and the
> gfrc are both defined in the DT, you can fail to init the rtc one with a simple
> test in the init function:
> 
> 	if (IS_DEFINED(CONFIG_SMP))
> 		return -EINVAL;
> 
> So, you can inconditionaly compile in both RTC and GFRC, no ? That would be
> cleaner and prevent a different kernel config.

Ah, actually I suggested something which is already there :)

Perhaps, the if SMP does not make sense in the Kconfig, no ?

[toc] | [prev] | [next] | [standalone]

#1514788 — Re: [PATCH 9/9] clocksource: import ARC timer driver

On 11/03/2016 10:33 AM, Daniel Lezcano wrote:
> As now the clksrc-probe is correctly handling the errors, if the rtc and the
> gfrc are both defined in the DT, you can fail to init the rtc one with a simple
> test in the init function:
> 
> 	if (IS_DEFINED(CONFIG_SMP))
> 		return -EINVAL;
> 
> So, you can inconditionaly compile in both RTC and GFRC, no ? That would be
> cleaner and prevent a different kernel config.

That's a very good idea. So now I envision
CONFIG_ARC_TIMERS		# legacy TIMER0 / TIMER1
CONFIG_ARC_64BIT_TIMERS		# rtc, gfrc

I need this distinction at the min to be able to select them from ARC Kconfig.

-Vineet

[toc] | [prev] | [next] | [standalone]

#1512971 — [PATCH 4/9] ARC: time: move time_init() out of the driver

Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 arch/arc/kernel/setup.c | 11 +++++++++++
 arch/arc/kernel/time.c  |  9 ---------
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
index 595d06900061..5865bd34a7fa 100644
--- a/arch/arc/kernel/setup.c
+++ b/arch/arc/kernel/setup.c
@@ -10,6 +10,8 @@
 #include <linux/fs.h>
 #include <linux/delay.h>
 #include <linux/root_dev.h>
+#include <linux/clk-provider.h>
+#include <linux/clocksource.h>
 #include <linux/console.h>
 #include <linux/module.h>
 #include <linux/cpu.h>
@@ -449,6 +451,15 @@ void __init setup_arch(char **cmdline_p)
 	arc_unwind_init();
 }
 
+/*
+ * Called from start_kernel() - boot CPU only
+ */
+void __init time_init(void)
+{
+	of_clk_init(NULL);
+	clocksource_probe();
+}
+
 static int __init customize_machine(void)
 {
 	if (machine_desc->init_machine)
diff --git a/arch/arc/kernel/time.c b/arch/arc/kernel/time.c
index 2c51e3cafad0..00ece39a8ae7 100644
--- a/arch/arc/kernel/time.c
+++ b/arch/arc/kernel/time.c
@@ -367,12 +367,3 @@ static int __init arc_of_timer_init(struct device_node *np)
 	return ret;
 }
 CLOCKSOURCE_OF_DECLARE(arc_clkevt, "snps,arc-timer", arc_of_timer_init);
-
-/*
- * Called from start_kernel() - boot CPU only
- */
-void __init time_init(void)
-{
-	of_clk_init(NULL);
-	clocksource_probe();
-}
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1514710 — Re: [PATCH 4/9] ARC: time: move time_init() out of the driver

On Mon, Oct 31, 2016 at 03:48:11PM -0700, Vineet Gupta wrote:
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

[toc] | [prev] | [next] | [standalone]

#1512974 — [PATCH 6/9] ARC: move mcip.h into include/soc and adjust the includes

Also remove the depedency on ARCv2, to increase compile coverage for
!ARCV2 builds

Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 arch/arc/kernel/mcip.c                           | 2 +-
 arch/arc/kernel/time.c                           | 2 +-
 arch/arc/plat-axs10x/axs10x.c                    | 2 +-
 {arch/arc/include/asm => include/soc/arc}/mcip.h | 8 ++------
 4 files changed, 5 insertions(+), 9 deletions(-)
 rename {arch/arc/include/asm => include/soc/arc}/mcip.h (96%)

diff --git a/arch/arc/kernel/mcip.c b/arch/arc/kernel/mcip.c
index c424d5abc318..0651e0a2e8b1 100644
--- a/arch/arc/kernel/mcip.c
+++ b/arch/arc/kernel/mcip.c
@@ -11,8 +11,8 @@
 #include <linux/smp.h>
 #include <linux/irq.h>
 #include <linux/spinlock.h>
+#include <soc/arc/mcip.h>
 #include <asm/irqflags-arcv2.h>
-#include <asm/mcip.h>
 #include <asm/setup.h>
 
 static DEFINE_RAW_SPINLOCK(mcip_lock);
diff --git a/arch/arc/kernel/time.c b/arch/arc/kernel/time.c
index 00ece39a8ae7..f1ebe45bfcdf 100644
--- a/arch/arc/kernel/time.c
+++ b/arch/arc/kernel/time.c
@@ -40,7 +40,7 @@
 #include <asm/irq.h>
 #include <asm/arcregs.h>
 
-#include <asm/mcip.h>
+#include <soc/arc/mcip.h>
 
 /* Timer related Aux registers */
 #define ARC_REG_TIMER0_LIMIT	0x23	/* timer 0 limit */
diff --git a/arch/arc/plat-axs10x/axs10x.c b/arch/arc/plat-axs10x/axs10x.c
index 86548701023c..38ff349d7f2a 100644
--- a/arch/arc/plat-axs10x/axs10x.c
+++ b/arch/arc/plat-axs10x/axs10x.c
@@ -21,7 +21,7 @@
 #include <asm/asm-offsets.h>
 #include <asm/io.h>
 #include <asm/mach_desc.h>
-#include <asm/mcip.h>
+#include <soc/arc/mcip.h>
 
 #define AXS_MB_CGU		0xE0010000
 #define AXS_MB_CREG		0xE0011000
diff --git a/arch/arc/include/asm/mcip.h b/include/soc/arc/mcip.h
similarity index 96%
rename from arch/arc/include/asm/mcip.h
rename to include/soc/arc/mcip.h
index fc28d0944801..6902c2a8bd23 100644
--- a/arch/arc/include/asm/mcip.h
+++ b/include/soc/arc/mcip.h
@@ -8,10 +8,8 @@
  * published by the Free Software Foundation.
  */
 
-#ifndef __ASM_MCIP_H
-#define __ASM_MCIP_H
-
-#ifdef CONFIG_ISA_ARCV2
+#ifndef __SOC_ARC_MCIP_H
+#define __SOC_ARC_MCIP_H
 
 #include <soc/arc/aux.h>
 
@@ -103,5 +101,3 @@ static inline void __mcip_cmd_data(unsigned int cmd, unsigned int param,
 }
 
 #endif
-
-#endif
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1514721 — Re: [PATCH 6/9] ARC: move mcip.h into include/soc and adjust the includes

On Mon, Oct 31, 2016 at 03:48:13PM -0700, Vineet Gupta wrote:
> Also remove the depedency on ARCv2, to increase compile coverage for
> !ARCV2 builds

s/depedency/dependency/

Acked-by: Daniel Lezcano <daniel.lezcnao@linaro.org>

> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> ---
>  arch/arc/kernel/mcip.c                           | 2 +-
>  arch/arc/kernel/time.c                           | 2 +-
>  arch/arc/plat-axs10x/axs10x.c                    | 2 +-
>  {arch/arc/include/asm => include/soc/arc}/mcip.h | 8 ++------
>  4 files changed, 5 insertions(+), 9 deletions(-)
>  rename {arch/arc/include/asm => include/soc/arc}/mcip.h (96%)
> 
> diff --git a/arch/arc/kernel/mcip.c b/arch/arc/kernel/mcip.c
> index c424d5abc318..0651e0a2e8b1 100644
> --- a/arch/arc/kernel/mcip.c
> +++ b/arch/arc/kernel/mcip.c
> @@ -11,8 +11,8 @@
>  #include <linux/smp.h>
>  #include <linux/irq.h>
>  #include <linux/spinlock.h>
> +#include <soc/arc/mcip.h>
>  #include <asm/irqflags-arcv2.h>
> -#include <asm/mcip.h>
>  #include <asm/setup.h>
>  
>  static DEFINE_RAW_SPINLOCK(mcip_lock);
> diff --git a/arch/arc/kernel/time.c b/arch/arc/kernel/time.c
> index 00ece39a8ae7..f1ebe45bfcdf 100644
> --- a/arch/arc/kernel/time.c
> +++ b/arch/arc/kernel/time.c
> @@ -40,7 +40,7 @@
>  #include <asm/irq.h>
>  #include <asm/arcregs.h>
>  
> -#include <asm/mcip.h>
> +#include <soc/arc/mcip.h>
>  
>  /* Timer related Aux registers */
>  #define ARC_REG_TIMER0_LIMIT	0x23	/* timer 0 limit */
> diff --git a/arch/arc/plat-axs10x/axs10x.c b/arch/arc/plat-axs10x/axs10x.c
> index 86548701023c..38ff349d7f2a 100644
> --- a/arch/arc/plat-axs10x/axs10x.c
> +++ b/arch/arc/plat-axs10x/axs10x.c
> @@ -21,7 +21,7 @@
>  #include <asm/asm-offsets.h>
>  #include <asm/io.h>
>  #include <asm/mach_desc.h>
> -#include <asm/mcip.h>
> +#include <soc/arc/mcip.h>
>  
>  #define AXS_MB_CGU		0xE0010000
>  #define AXS_MB_CREG		0xE0011000
> diff --git a/arch/arc/include/asm/mcip.h b/include/soc/arc/mcip.h
> similarity index 96%
> rename from arch/arc/include/asm/mcip.h
> rename to include/soc/arc/mcip.h
> index fc28d0944801..6902c2a8bd23 100644
> --- a/arch/arc/include/asm/mcip.h
> +++ b/include/soc/arc/mcip.h
> @@ -8,10 +8,8 @@
>   * published by the Free Software Foundation.
>   */
>  
> -#ifndef __ASM_MCIP_H
> -#define __ASM_MCIP_H
> -
> -#ifdef CONFIG_ISA_ARCV2
> +#ifndef __SOC_ARC_MCIP_H
> +#define __SOC_ARC_MCIP_H
>  
>  #include <soc/arc/aux.h>
>  
> @@ -103,5 +101,3 @@ static inline void __mcip_cmd_data(unsigned int cmd, unsigned int param,
>  }
>  
>  #endif
> -
> -#endif
> -- 
> 2.7.4
> 

[toc] | [prev] | [next] | [standalone]

#1512975 — [PATCH 2/9] ARC: timer: rtc: implement read loop in "C" vs. inline asm

To allow for easy movement into drivers/clocksource

Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 arch/arc/kernel/time.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/arch/arc/kernel/time.c b/arch/arc/kernel/time.c
index a2db010cde18..2c51e3cafad0 100644
--- a/arch/arc/kernel/time.c
+++ b/arch/arc/kernel/time.c
@@ -153,14 +153,11 @@ static cycle_t arc_read_rtc(struct clocksource *cs)
 		cycle_t  full;
 	} stamp;
 
-
-	__asm__ __volatile(
-	"1:						\n"
-	"	lr		%0, [AUX_RTC_LOW]	\n"
-	"	lr		%1, [AUX_RTC_HIGH]	\n"
-	"	lr		%2, [AUX_RTC_CTRL]	\n"
-	"	bbit0.nt	%2, 31, 1b		\n"
-	: "=r" (stamp.low), "=r" (stamp.high), "=r" (status));
+	do {
+		stamp.low = read_aux_reg(AUX_RTC_LOW);
+		stamp.high = read_aux_reg(AUX_RTC_HIGH);
+		status = read_aux_reg(AUX_RTC_CTRL);
+	} while (!(status & 0x80000000UL));
 
 	return stamp.full;
 }
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1514703 — Re: [PATCH 2/9] ARC: timer: rtc: implement read loop in "C" vs. inline asm

On Mon, Oct 31, 2016 at 03:48:09PM -0700, Vineet Gupta wrote:
> To allow for easy movement into drivers/clocksource
> 
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> ---
>  arch/arc/kernel/time.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arc/kernel/time.c b/arch/arc/kernel/time.c
> index a2db010cde18..2c51e3cafad0 100644
> --- a/arch/arc/kernel/time.c
> +++ b/arch/arc/kernel/time.c
> @@ -153,14 +153,11 @@ static cycle_t arc_read_rtc(struct clocksource *cs)
>  		cycle_t  full;
>  	} stamp;
>  
> -
> -	__asm__ __volatile(
> -	"1:						\n"
> -	"	lr		%0, [AUX_RTC_LOW]	\n"
> -	"	lr		%1, [AUX_RTC_HIGH]	\n"
> -	"	lr		%2, [AUX_RTC_CTRL]	\n"
> -	"	bbit0.nt	%2, 31, 1b		\n"
> -	: "=r" (stamp.low), "=r" (stamp.high), "=r" (status));
> +	do {
> +		stamp.low = read_aux_reg(AUX_RTC_LOW);
> +		stamp.high = read_aux_reg(AUX_RTC_HIGH);
> +		status = read_aux_reg(AUX_RTC_CTRL);
> +	} while (!(status & 0x80000000UL));

Replace the literal 0x80000000UL by a macro.

What is the 'status' for ?
  
>  	return stamp.full;
>  }
> -- 
> 2.7.4
> 

[toc] | [prev] | [next] | [standalone]

#1514744 — Re: [PATCH 2/9] ARC: timer: rtc: implement read loop in "C" vs. inline asm

On 11/03/2016 10:02 AM, Daniel Lezcano wrote:
> On Mon, Oct 31, 2016 at 03:48:09PM -0700, Vineet Gupta wrote:
>> To allow for easy movement into drivers/clocksource
>>
>> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>> ---
>>  arch/arc/kernel/time.c | 13 +++++--------
>>  1 file changed, 5 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arc/kernel/time.c b/arch/arc/kernel/time.c
>> index a2db010cde18..2c51e3cafad0 100644
>> --- a/arch/arc/kernel/time.c
>> +++ b/arch/arc/kernel/time.c
>> @@ -153,14 +153,11 @@ static cycle_t arc_read_rtc(struct clocksource *cs)
>>  		cycle_t  full;
>>  	} stamp;
>>  
>> -
>> -	__asm__ __volatile(
>> -	"1:						\n"
>> -	"	lr		%0, [AUX_RTC_LOW]	\n"
>> -	"	lr		%1, [AUX_RTC_HIGH]	\n"
>> -	"	lr		%2, [AUX_RTC_CTRL]	\n"
>> -	"	bbit0.nt	%2, 31, 1b		\n"
>> -	: "=r" (stamp.low), "=r" (stamp.high), "=r" (status));
>> +	do {
>> +		stamp.low = read_aux_reg(AUX_RTC_LOW);
>> +		stamp.high = read_aux_reg(AUX_RTC_HIGH);
>> +		status = read_aux_reg(AUX_RTC_CTRL);
>> +	} while (!(status & 0x80000000UL));
> 
> Replace the literal 0x80000000UL by a macro.

OK !


> What is the 'status' for ?

Hardware keeps a internal state machine for atomic readout of low/high. So if an
interrupt is taken between reading low and high, or if high increments after low
is read, then the bit forces a loop to retry.

>   
>>  	return stamp.full;
>>  }
>> -- 
>> 2.7.4
>>

[toc] | [prev] | [next] | [standalone]

#1514724 — Re: [PATCH 0/9] Move ARC timer code into drivers/clocksource/

On Mon, Oct 31, 2016 at 03:48:07PM -0700, Vineet Gupta wrote:
> Hi,
> 
> This series addresses the long pending move of ARC timer code into
> drivers/clocksource/.
> 
> - patches [1-4]/9 are improvements to arc code, paving way for later code motion.
> - patches [5-8]/9 refactor the arc headers to be shared between arc and drivers
> - patch 9/9 moves out the driver code/build/Kconfig bits
> 
> As review progresses/concludes, I'd like to merge [1-8] for this merge window and
> clocksource maintainers can pick up the actual switch for 4.10 or even 4.9 as they
> prefer.
> 

The different patches deserve a longer change description.

[toc] | [prev] | [next] | [standalone]

#1513016

>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 

[toc] | [prev] | [next] | [standalone]

#1513466

On Mon, Oct 31, 2016 at 05:03:40PM +0000, Noam Camus wrote:
> >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.

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.

[toc] | [prev] | [next] | [standalone]

#1517075

> 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

[toc] | [prev] | [next] | [standalone]

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web