This patch series provides an assorted timer cleanups in RISC-V.
Changes from v1->v2:
1. Updated commit text in 1/4.
2. Added a timebase check for each cpu.
3. Added a warning for invalid hartid 4/4.
Atish Patra (3):
RISC-V: Support per-hart timebase-frequency
RISC-V: Remove per cpu clocksource
RISC-V: Fix non-smp kernel boot on SMP systems
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 | 51 +++++++++++++++++++++---
3 files changed, 49 insertions(+), 15 deletions(-)
--
2.7.4
Currently, clocksource registration happens for an invalid cpu
for non-smp kernels. This lead to kernel panic as cpu hotplug
registration will fail for those cpus.
Do not proceed if hartid is invalid. Take this opprtunity to
print appropriate error strings for different failure cases.
Signed-off-by: Atish Patra <[email protected]>
---
drivers/clocksource/riscv_timer.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/clocksource/riscv_timer.c b/drivers/clocksource/riscv_timer.c
index c9e65086..c1e1260e 100644
--- a/drivers/clocksource/riscv_timer.c
+++ b/drivers/clocksource/riscv_timer.c
@@ -117,6 +117,11 @@ static int __init riscv_timer_init_dt(struct device_node *n)
int cpuid, hartid, error;
hartid = riscv_of_processor_hartid(n);
+ if (hartid < 0) {
+ pr_warn("Not valid hartid for node [%pOF] error = [%d]\n",
+ n, hartid);
+ return hartid;
+ }
cpuid = riscv_hartid_to_cpuid(hartid);
riscv_timebase_frequency(n, hartid);
@@ -124,14 +129,19 @@ static int __init riscv_timer_init_dt(struct device_node *n)
return 0;
/* This should be called only for boot cpu */
- clocksource_register_hz(&riscv_clocksource, riscv_timebase);
+ error = clocksource_register_hz(&riscv_clocksource, riscv_timebase);
+ if (error) {
+ pr_err("RISCV timer register failed [%d] for cpu = [%d]\n",
+ error, cpuid);
+ return error;
+ }
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, cpuid);
+ pr_err("cpu hp setup state failed for RISCV timer [%d]\n",
+ error);
return error;
}
--
2.7.4
From: Palmer Dabbelt <[email protected]>
In RISC-V systems, timebase-frequency is per cpu instead of one
instance for entire SOC as there is a individual timer per each CPU.
Fix the DT binding accordingly.
Signed-off-by: Palmer Dabbelt <[email protected]>
Signed-off-by: Christoph Hellwig <[email protected]>
[Atish: Update the commit text]
Signed-off-by: Atish Patra <[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 | 31 +++++++++++++++++++++++++++++++
2 files changed, 32 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..75262409 100644
--- a/drivers/clocksource/riscv_timer.c
+++ b/drivers/clocksource/riscv_timer.c
@@ -83,6 +83,35 @@ void riscv_timer_interrupt(void)
evdev->event_handler(evdev);
}
+static void __init riscv_timebase_frequency(struct device_node *node,
+ int hartid)
+{
+ u32 timebase;
+
+ if (!of_property_read_u32(node, "timebase-frequency", &timebase))
+ goto check;
+
+ /*
+ * As per the DT specification, timebase-frequency should be present
+ * under individual cpu node. Unfortunately, there are already available
+ * HiFive Unleashed devices where the timebase-frequency entry is under
+ * CPUs. check under parent "cpus" node to cover those devices.
+ */
+ if (!of_property_read_u32(node->parent, "timebase-frequency",
+ &timebase))
+ goto check;
+
+ panic("RISC-V system with no timebase-frequency in DTS for hart [%d]\n",
+ hartid);
+
+check:
+ /* RISC-V ISA specification mandates that every cpu has a timer */
+ if (!riscv_timebase)
+ riscv_timebase = timebase;
+ else if (riscv_timebase && riscv_timebase != timebase)
+ pr_warn("RISC-V system with different timebase-frequency\n");
+}
+
static int __init riscv_timer_init_dt(struct device_node *n)
{
int cpuid, hartid, error;
@@ -90,10 +119,12 @@ static int __init riscv_timer_init_dt(struct device_node *n)
hartid = riscv_of_processor_hartid(n);
cpuid = riscv_hartid_to_cpuid(hartid);
+ riscv_timebase_frequency(n, hartid);
if (cpuid != smp_processor_id())
return 0;
+ /* This should be called only for boot cpu */
cs = per_cpu_ptr(&riscv_clocksource, cpuid);
clocksource_register_hz(cs, riscv_timebase);
--
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]>
Reviewed-by: Palmer Dabbelt <[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 75262409..c9e65086 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),
@@ -115,7 +115,6 @@ static void __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);
@@ -125,8 +124,7 @@ static int __init riscv_timer_init_dt(struct device_node *n)
return 0;
/* This should be called only for boot cpu */
- 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
On 14/12/2018 00:14, Atish Patra wrote:
> From: Palmer Dabbelt <[email protected]>
>
> In RISC-V systems, timebase-frequency is per cpu instead of one
> instance for entire SOC as there is a individual timer per each CPU.
> Fix the DT binding accordingly.
Why not use a fixed-clock instead of this timebase property which forces
to declare a global variable to be exported from arch/riscv to
drivers/clocksource ?
In addition, please add the 'Fixes' tag
> Signed-off-by: Palmer Dabbelt <[email protected]>
> Signed-off-by: Christoph Hellwig <[email protected]>
> [Atish: Update the commit text]
> Signed-off-by: Atish Patra <[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>;
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 14/12/2018 00:14, 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]>
> ---
> arch/riscv/kernel/time.c | 9 +--------
> drivers/clocksource/riscv_timer.c | 31 +++++++++++++++++++++++++++++++
> 2 files changed, 32 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..75262409 100644
> --- a/drivers/clocksource/riscv_timer.c
> +++ b/drivers/clocksource/riscv_timer.c
> @@ -83,6 +83,35 @@ void riscv_timer_interrupt(void)
> evdev->event_handler(evdev);
> }
>
> +static void __init riscv_timebase_frequency(struct device_node *node,
> + int hartid)
> +{
> + u32 timebase;
> +
> + if (!of_property_read_u32(node, "timebase-frequency", &timebase))
> + goto check;
> +
> + /*
> + * As per the DT specification, timebase-frequency should be present
> + * under individual cpu node. Unfortunately, there are already available
> + * HiFive Unleashed devices where the timebase-frequency entry is under
> + * CPUs. check under parent "cpus" node to cover those devices.
> + */
> + if (!of_property_read_u32(node->parent, "timebase-frequency",
> + &timebase))
> + goto check;
> +
> + panic("RISC-V system with no timebase-frequency in DTS for hart [%d]\n",
> + hartid);
no panic in the driver code please. Alternatively use pr_err and let the
timer-probe function spit the critical error on the console.
It would be nicer to add a fixed-clock and get the rate from it.
> +check:
> + /* RISC-V ISA specification mandates that every cpu has a timer */
> + if (!riscv_timebase)
> + riscv_timebase = timebase;
> + else if (riscv_timebase && riscv_timebase != timebase)
> + pr_warn("RISC-V system with different timebase-frequency\n");
> +}
> +
> static int __init riscv_timer_init_dt(struct device_node *n)
> {
> int cpuid, hartid, error;
> @@ -90,10 +119,12 @@ static int __init riscv_timer_init_dt(struct device_node *n)
>
> hartid = riscv_of_processor_hartid(n);
> cpuid = riscv_hartid_to_cpuid(hartid);
> + riscv_timebase_frequency(n, hartid);
>
> if (cpuid != smp_processor_id())
> return 0;
Somehow related to this change. Are you sure the test above makes sense?
> + /* This should be called only for boot cpu */
> cs = per_cpu_ptr(&riscv_clocksource, cpuid);
> clocksource_register_hz(cs, riscv_timebase);
>
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On Fri, 14 Dec 2018 01:17:24 PST (-0800), [email protected] wrote:
> On 14/12/2018 00:14, Atish Patra wrote:
>> From: Palmer Dabbelt <[email protected]>
>>
>> In RISC-V systems, timebase-frequency is per cpu instead of one
>> instance for entire SOC as there is a individual timer per each CPU.
>> Fix the DT binding accordingly.
>
> Why not use a fixed-clock instead of this timebase property which forces
> to declare a global variable to be exported from arch/riscv to
> drivers/clocksource ?
That makes sense to me. I've always disliked this global variable and a big
part of why my original version got delayed forever is that I'd hoped to get
rid of it.
Given that this is all a mess anyway I'm OK breaking backwards compatibility
here.
Is there an example of how to do this?
> In addition, please add the 'Fixes' tag
>
>> Signed-off-by: Palmer Dabbelt <[email protected]>
>> Signed-off-by: Christoph Hellwig <[email protected]>
>> [Atish: Update the commit text]
>> Signed-off-by: Atish Patra <[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>;
On 04/01/2019 01:36, Palmer Dabbelt wrote:
> On Fri, 14 Dec 2018 01:17:24 PST (-0800), [email protected] wrote:
>> On 14/12/2018 00:14, Atish Patra wrote:
>>> From: Palmer Dabbelt <[email protected]>
>>>
>>> In RISC-V systems, timebase-frequency is per cpu instead of one
>>> instance for entire SOC as there is a individual timer per each CPU.
>>> Fix the DT binding accordingly.
>>
>> Why not use a fixed-clock instead of this timebase property which forces
>> to declare a global variable to be exported from arch/riscv to
>> drivers/clocksource ?
>
> That makes sense to me. I've always disliked this global variable and a
> big part of why my original version got delayed forever is that I'd
> hoped to get rid of it.
>
> Given that this is all a mess anyway I'm OK breaking backwards
> compatibility here.
>
> Is there an example of how to do this?
Can you give some hardware details? Is the timebase frequency constant?
If it is the case, a fixed-clock shared for each cpu can be used, no?
myclock: myclock {
compatible = "fixed-clock";
#clock-cells = <0>;
clock-frequency = <1000000>;
clock-output-names = "mytimer";
};
Alternatively, may be different output can be specified with the clock,
one for each CPUs.
Or if the timebase frequency is resulting from a clock divisor, it can
be defined as:
clock {
compatible = "fixed-factor-clock";
clocks = <&parentclk>;
#clock-cells = <0>;
clock-div = <2>;
clock-mult = <1>;
};
hardware details can help to narrow down the right description.
>> In addition, please add the 'Fixes' tag
>>
>>> Signed-off-by: Palmer Dabbelt <[email protected]>
>>> Signed-off-by: Christoph Hellwig <[email protected]>
>>> [Atish: Update the commit text]
>>> Signed-off-by: Atish Patra <[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>;
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog