[RFC 0/3] Timer code cleanup

[RFC 0/3] Timer code cleanup

[Atish Patra]

This patch series address some required timer cleanups in RISC-V.

Atish Patra (2):
  RISC-V:Support per-hart timebase-frequency
  RISC-V: Remove per cpu clocksource

Palmer Dabbelt (1):
  dt-bindings: Correct RISC-V's timebase-frequency

 Documentation/devicetree/bindings/riscv/cpus.txt |  4 +++-
 arch/riscv/kernel/time.c                         |  9 +--------
 drivers/clocksource/riscv_timer.c                | 22 ++++++++++++++++++----
 3 files changed, 22 insertions(+), 13 deletions(-)

-- 
2.7.4

[RFC 1/3] dt-bindings: Correct RISC-V's timebase-frequency

[Atish Patra]

From: Palmer Dabbelt <palmer@sifive.com>

Someone must have read the device tree specification incorrectly,
because we were putting timebase-frequency in the wrong place.  This
corrects the issue, moving it from

/ {
        cpus {
                timebase-frequency = X;
        }
}

to

/ {
        cpus {
                cpu@0 {
                        timebase-frequency = X;
                }
        }
}

This is great, because the timer's frequency should really be a per-cpu
quantity on RISC-V systems since there's a timer per CPU.  This should
lead to some cleanups in our timer driver.

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/riscv/cpus.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/riscv/cpus.txt b/Documentation/devicetree/bindings/riscv/cpus.txt
index adf7b7af..b0b038d6 100644
--- a/Documentation/devicetree/bindings/riscv/cpus.txt
+++ b/Documentation/devicetree/bindings/riscv/cpus.txt
@@ -93,9 +93,9 @@ Linux is allowed to run on.
         cpus {
                 #address-cells = <1>;
                 #size-cells = <0>;
-                timebase-frequency = <1000000>;
                 cpu@0 {
                         clock-frequency = <1600000000>;
+                        timebase-frequency = <1000000>;
                         compatible = "sifive,rocket0", "riscv";
                         device_type = "cpu";
                         i-cache-block-size = <64>;
@@ -113,6 +113,7 @@ Linux is allowed to run on.
                 };
                 cpu@1 {
                         clock-frequency = <1600000000>;
+                        timebase-frequency = <1000000>;
                         compatible = "sifive,rocket0", "riscv";
                         d-cache-block-size = <64>;
                         d-cache-sets = <64>;
@@ -145,6 +146,7 @@ Example: Spike ISA Simulator with 1 Hart
 This device tree matches the Spike ISA golden model as run with `spike -p1`.
 
         cpus {
+                timebase-frequency = <1000000>;
                 cpu@0 {
                         device_type = "cpu";
                         reg = <0x00000000>;
-- 
2.7.4

[RFC 2/3] RISC-V:Support per-hart timebase-frequency

[Atish Patra]

Follow the updated DT specs and read the timebase-frequency
from the boot cpu. Keep the old DT reading as well for backward
compatibility. This patch is rework of old patch from Palmer.

Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
 arch/riscv/kernel/time.c          |  9 +--------
 drivers/clocksource/riscv_timer.c | 16 ++++++++++++++++
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/arch/riscv/kernel/time.c b/arch/riscv/kernel/time.c
index 1911c8f6..225fe743 100644
--- a/arch/riscv/kernel/time.c
+++ b/arch/riscv/kernel/time.c
@@ -20,14 +20,7 @@ unsigned long riscv_timebase;
 
 void __init time_init(void)
 {
-	struct device_node *cpu;
-	u32 prop;
-
-	cpu = of_find_node_by_path("/cpus");
-	if (!cpu || of_property_read_u32(cpu, "timebase-frequency", &prop))
-		panic(KERN_WARNING "RISC-V system with no 'timebase-frequency' in DTS\n");
-	riscv_timebase = prop;
+	timer_probe();
 
 	lpj_fine = riscv_timebase / HZ;
-	timer_probe();
 }
diff --git a/drivers/clocksource/riscv_timer.c b/drivers/clocksource/riscv_timer.c
index 084e97dc..335bdb91 100644
--- a/drivers/clocksource/riscv_timer.c
+++ b/drivers/clocksource/riscv_timer.c
@@ -83,6 +83,21 @@ void riscv_timer_interrupt(void)
 	evdev->event_handler(evdev);
 }
 
+static long __init riscv_timebase_frequency(struct device_node *node)
+{
+	u32 timebase;
+
+	if (!of_property_read_u32(node, "timebase-frequency", &timebase))
+		return timebase;
+
+	/* check under parent "cpus" node */
+	if (!of_property_read_u32(node->parent, "timebase-frequency",
+				  &timebase))
+		return timebase;
+
+	panic("RISC-V system with no 'timebase-frequency' in DTS\n");
+}
+
 static int __init riscv_timer_init_dt(struct device_node *n)
 {
 	int cpuid, hartid, error;
@@ -94,6 +109,7 @@ static int __init riscv_timer_init_dt(struct device_node *n)
 	if (cpuid != smp_processor_id())
 		return 0;
 
+	riscv_timebase = riscv_timebase_frequency(n);
 	cs = per_cpu_ptr(&riscv_clocksource, cpuid);
 	clocksource_register_hz(cs, riscv_timebase);
 
-- 
2.7.4

[RFC 3/3] RISC-V: Remove per cpu clocksource

[Atish Patra]

There is only one clocksource in RISC-V. The boot cpu initializes
that clocksource. No need to keep a percpu data structure.

Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
 drivers/clocksource/riscv_timer.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/clocksource/riscv_timer.c b/drivers/clocksource/riscv_timer.c
index 335bdb91..537c5c70 100644
--- a/drivers/clocksource/riscv_timer.c
+++ b/drivers/clocksource/riscv_timer.c
@@ -49,7 +49,7 @@ static unsigned long long riscv_clocksource_rdtime(struct clocksource *cs)
 	return get_cycles64();
 }
 
-static DEFINE_PER_CPU(struct clocksource, riscv_clocksource) = {
+static struct clocksource riscv_clocksource = {
 	.name		= "riscv_clocksource",
 	.rating		= 300,
 	.mask		= CLOCKSOURCE_MASK(BITS_PER_LONG),
@@ -101,7 +101,6 @@ static long __init riscv_timebase_frequency(struct device_node *node)
 static int __init riscv_timer_init_dt(struct device_node *n)
 {
 	int cpuid, hartid, error;
-	struct clocksource *cs;
 
 	hartid = riscv_of_processor_hartid(n);
 	cpuid = riscv_hartid_to_cpuid(hartid);
@@ -110,8 +109,7 @@ static int __init riscv_timer_init_dt(struct device_node *n)
 		return 0;
 
 	riscv_timebase = riscv_timebase_frequency(n);
-	cs = per_cpu_ptr(&riscv_clocksource, cpuid);
-	clocksource_register_hz(cs, riscv_timebase);
+	clocksource_register_hz(&riscv_clocksource, riscv_timebase);
 
 	error = cpuhp_setup_state(CPUHP_AP_RISCV_TIMER_STARTING,
 			 "clockevents/riscv/timer:starting",
-- 
2.7.4

Re: [RFC 1/3] dt-bindings: Correct RISC-V's timebase-frequency

[Christoph Hellwig]

On Fri, Sep 14, 2018 at 02:54:54PM -0700, Atish Patra wrote:
> From: Palmer Dabbelt <palmer@sifive.com>
> 
> Someone must have read the device tree specification incorrectly,
> because we were putting timebase-frequency in the wrong place.  This
> corrects the issue, moving it from

Last time this was brought up we actually had realy life DTs with
the timebase-frequency under cpus and not the invidual cpu node.

I think we need to just document both possibilities and live with
them.

Re: [RFC 2/3] RISC-V:Support per-hart timebase-frequency

[Christoph Hellwig]

On Fri, Sep 14, 2018 at 02:54:55PM -0700, Atish Patra wrote:
> Follow the updated DT specs and read the timebase-frequency
> from the boot cpu. Keep the old DT reading as well for backward
> compatibility. This patch is rework of old patch from Palmer.
> 
> Signed-off-by: Atish Patra <atish.patra@wdc.com>

This setup looks a bit odd because it keeps blindly overwriting
riscv_timebase for every cpu found.  Shouldn't we at least check that
they all match for now as the rest of the port assumes that?

Re: [RFC 3/3] RISC-V: Remove per cpu clocksource

[Christoph Hellwig]

I think only having one clocksource is indeed the right thing.

But that makes the whole TIMER_OF_DECLARE for each hart (cpu core for
those not RISC-V savvy) even more questionable than it already is.

I think we should just initialize the clocksource directly as it is
architectually guaranteed to exist.  Below is a completely untested
(not even compiled) version of your patch that does what I think
we should be doing here.  But I'd rather hear from more timer and/or
DT savvy folks before proceeding.

diff --git a/drivers/clocksource/riscv_timer.c b/drivers/clocksource/riscv_timer.c
index 4e8b347e43e2..2fe497c67283 100644
--- a/drivers/clocksource/riscv_timer.c
+++ b/drivers/clocksource/riscv_timer.c
@@ -48,7 +48,7 @@ static unsigned long long riscv_clocksource_rdtime(struct clocksource *cs)
 	return get_cycles64();
 }
 
-static DEFINE_PER_CPU(struct clocksource, riscv_clocksource) = {
+static struct clocksource, riscv_clocksource = {
 	.name		= "riscv_clocksource",
 	.rating		= 300,
 	.mask		= CLOCKSOURCE_MASK(BITS_PER_LONG),
@@ -82,24 +82,16 @@ void riscv_timer_interrupt(void)
 	evdev->event_handler(evdev);
 }
 
-static int __init riscv_timer_init_dt(struct device_node *n)
+static int __init riscv_timer_init(void)
 {
-	int cpu_id = riscv_of_processor_hart(n), error;
-	struct clocksource *cs;
-
-	if (cpu_id != smp_processor_id())
-		return 0;
-
-	cs = per_cpu_ptr(&riscv_clocksource, cpu_id);
-	clocksource_register_hz(cs, riscv_timebase);
+	int error;
 
+	clocksource_register_hz(&riscv_clocksource, riscv_timebase);
 	error = cpuhp_setup_state(CPUHP_AP_RISCV_TIMER_STARTING,
 			 "clockevents/riscv/timer:starting",
 			 riscv_timer_starting_cpu, riscv_timer_dying_cpu);
 	if (error)
-		pr_err("RISCV timer register failed [%d] for cpu = [%d]\n",
-		       error, cpu_id);
+		pr_err("RISCV timer register failed: %d.\n", error);
 	return error;
 }
-
-TIMER_OF_DECLARE(riscv_timer, "riscv", riscv_timer_init_dt);
+core_initcall(riscv_timer_init);

Re: [RFC 3/3] RISC-V: Remove per cpu clocksource

[Thomas Gleixner]

On Mon, 17 Sep 2018, Christoph Hellwig wrote:

> I think only having one clocksource is indeed the right thing.
> 
> But that makes the whole TIMER_OF_DECLARE for each hart (cpu core for
> those not RISC-V savvy) even more questionable than it already is.
> 
> I think we should just initialize the clocksource directly as it is
> architectually guaranteed to exist.  Below is a completely untested
> (not even compiled) version of your patch that does what I think
> we should be doing here.  But I'd rather hear from more timer and/or
> DT savvy folks before proceeding.

If this really does not need configuration and all actual implementations
are not "allowed" to screw the timer up, then this surely can do without
DT.

Just for the record, this would be the first (architected) timer ever which
just works. I'm having a hard time to believe this, but I'd certainly
welcome it.

> -TIMER_OF_DECLARE(riscv_timer, "riscv", riscv_timer_init_dt);
> +core_initcall(riscv_timer_init);

Are you sure that core_initcall is not too late?

Thanks,

	tglx

Re: [RFC 3/3] RISC-V: Remove per cpu clocksource

[Christoph Hellwig]

On Mon, Sep 17, 2018 at 04:52:44PM +0200, Thomas Gleixner wrote:
> If this really does not need configuration and all actual implementations
> are not "allowed" to screw the timer up, then this surely can do without
> DT.

That would be the plan.

> 
> Just for the record, this would be the first (architected) timer ever which
> just works. I'm having a hard time to believe this, but I'd certainly
> welcome it.

And that would be the contact with reality.  Note that the current
scheme which just matches for the riscv hart (aka cpu core) nodes
would not exactly help either.

> 
> > -TIMER_OF_DECLARE(riscv_timer, "riscv", riscv_timer_init_dt);
> > +core_initcall(riscv_timer_init);
> 
> Are you sure that core_initcall is not too late?

No, I'm not at all.  This is just intended as a quick throw-away draft.

Re: [RFC 3/3] RISC-V: Remove per cpu clocksource

[Thomas Gleixner]

On Mon, 17 Sep 2018, Christoph Hellwig wrote:
> > Just for the record, this would be the first (architected) timer ever which
> > just works. I'm having a hard time to believe this, but I'd certainly
> > welcome it.
> 
> And that would be the contact with reality.

I've dealt with the reality of timers for a long time ....

Thanks,

	tglx

Re: [RFC 3/3] RISC-V: Remove per cpu clocksource

[Anup Patel]

On Mon, Sep 17, 2018 at 8:35 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Mon, 17 Sep 2018, Christoph Hellwig wrote:
> > > Just for the record, this would be the first (architected) timer ever which
> > > just works. I'm having a hard time to believe this, but I'd certainly
> > > welcome it.
> >
> > And that would be the contact with reality.
>
> I've dealt with the reality of timers for a long time ....

I think the problem is we don't have separate DT node
for RISC-V timer. Instead, we have been probing timer
for each CPU DT node.

Ideally, we should have one DT node for RISC-V timer
and the DT should should also describe the local interrupts
to be used for RISC-V timer.

Regards,
Anup

Re: [RFC 3/3] RISC-V: Remove per cpu clocksource

[Atish Patra]

On 9/17/18 8:01 AM, Christoph Hellwig wrote:
> On Mon, Sep 17, 2018 at 04:52:44PM +0200, Thomas Gleixner wrote:
>> If this really does not need configuration and all actual implementations
>> are not "allowed" to screw the timer up, then this surely can do without
>> DT.
> 
> That would be the plan.
> 
>>
>> Just for the record, this would be the first (architected) timer ever which
>> just works. I'm having a hard time to believe this, but I'd certainly
>> welcome it.
> 
> And that would be the contact with reality.  Note that the current
> scheme which just matches for the riscv hart (aka cpu core) nodes
> would not exactly help either.
> 

I thought we will have a dedicated timer node (like all other arch) 
sooner or later and we will just change TIMER_OF to match that node 
instead of riscv hart.

In this way, we will not do something entirely different from any other 
architecture.

Regards,
Atish
>>
>>> -TIMER_OF_DECLARE(riscv_timer, "riscv", riscv_timer_init_dt);
>>> +core_initcall(riscv_timer_init);
>>
>> Are you sure that core_initcall is not too late?
> 
> No, I'm not at all.  This is just intended as a quick throw-away draft.
> 

Re: [RFC 2/3] RISC-V:Support per-hart timebase-frequency

[Atish Patra]

On 9/17/18 7:23 AM, Christoph Hellwig wrote:
> On Fri, Sep 14, 2018 at 02:54:55PM -0700, Atish Patra wrote:
>> Follow the updated DT specs and read the timebase-frequency
>> from the boot cpu. Keep the old DT reading as well for backward
>> compatibility. This patch is rework of old patch from Palmer.
>>
>> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> 
> This setup looks a bit odd because it keeps blindly overwriting
> riscv_timebase for every cpu found.  Shouldn't we at least check that
> they all match for now as the rest of the port assumes that?
> 

It will be only updated for boot cpu as it is done after this check.

if (cpuid != smp_processor_id())
  		return 0;

Regards,
Atish

Re: [RFC 1/3] dt-bindings: Correct RISC-V's timebase-frequency

[Atish Patra]

On 9/17/18 7:20 AM, Christoph Hellwig wrote:
> On Fri, Sep 14, 2018 at 02:54:54PM -0700, Atish Patra wrote:
>> From: Palmer Dabbelt <palmer@sifive.com>
>>
>> Someone must have read the device tree specification incorrectly,
>> because we were putting timebase-frequency in the wrong place.  This
>> corrects the issue, moving it from
> 
> Last time this was brought up we actually had realy life DTs with
> the timebase-frequency under cpus and not the invidual cpu node.
> 

Unfortunately, it still is. I hope SiFive can publish the updated 
firmware with correct DT entries. I can patch BBL to fix these DT issues 
until the firmware update if that's accepted.


> I think we need to just document both possibilities and live with
> them.
> 
Yup. That was the intention. I will update the document/commit text to 
reflect that.


Regards,
Atish

Re: [RFC 2/3] RISC-V:Support per-hart timebase-frequency

[Palmer Dabbelt]

On Mon, 17 Sep 2018 07:23:08 PDT (-0700), Christoph Hellwig wrote:
> On Fri, Sep 14, 2018 at 02:54:55PM -0700, Atish Patra wrote:
>> Follow the updated DT specs and read the timebase-frequency
>> from the boot cpu. Keep the old DT reading as well for backward
>> compatibility. This patch is rework of old patch from Palmer.
>>
>> Signed-off-by: Atish Patra <atish.patra@wdc.com>
>
> This setup looks a bit odd because it keeps blindly overwriting
> riscv_timebase for every cpu found.  Shouldn't we at least check that
> they all match for now as the rest of the port assumes that?

I agree.  There should be at least a warning if they don't match, but ideally 
we'd just support per-CPU timebases.  We have those systems already, they just 
don't boot Linux (at least, not yet :)).