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
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 <[email protected]>
---
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
From: Palmer Dabbelt <[email protected]>
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 <[email protected]>
Signed-off-by: Christoph Hellwig <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
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
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 <[email protected]>
---
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
On Fri, Sep 14, 2018 at 02:54:54PM -0700, Atish Patra wrote:
> From: Palmer Dabbelt <[email protected]>
>
> 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.
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 <[email protected]>
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 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);
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
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.
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
On Mon, Sep 17, 2018 at 8:35 PM Thomas Gleixner <[email protected]> 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
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.
>
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 <[email protected]>
>
> 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
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 <[email protected]>
>>
>> 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
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 <[email protected]>
>
> 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 :)).