This patch series provides an assorted timer cleanups in RISC-V.
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 | 39 ++++++++++++++++++++----
3 files changed, 37 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 | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/clocksource/riscv_timer.c b/drivers/clocksource/riscv_timer.c
index 39de6e49..4af4af47 100644
--- a/drivers/clocksource/riscv_timer.c
+++ b/drivers/clocksource/riscv_timer.c
@@ -108,6 +108,8 @@ static int __init riscv_timer_init_dt(struct device_node *n)
int cpuid, hartid, error;
hartid = riscv_of_processor_hartid(n);
+ if (hartid < 0)
+ return hartid;
cpuid = riscv_hartid_to_cpuid(hartid);
if (cpuid != smp_processor_id())
@@ -115,14 +117,19 @@ static int __init riscv_timer_init_dt(struct device_node *n)
/* This should be called only for boot cpu */
riscv_timebase = riscv_timebase_frequency(n);
- 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]>
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 | 22 ++++++++++++++++++++++
2 files changed, 23 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..96af7058 100644
--- a/drivers/clocksource/riscv_timer.c
+++ b/drivers/clocksource/riscv_timer.c
@@ -83,6 +83,26 @@ 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;
+
+ /*
+ * 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))
+ 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 +114,8 @@ static int __init riscv_timer_init_dt(struct device_node *n)
if (cpuid != smp_processor_id())
return 0;
+ /* This should be called only for boot cpu */
+ riscv_timebase = riscv_timebase_frequency(n);
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]>
---
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 96af7058..39de6e49 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),
@@ -106,7 +106,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);
@@ -116,8 +115,7 @@ static int __init riscv_timer_init_dt(struct device_node *n)
/* This should be called only for boot cpu */
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
On Mon, 03 Dec 2018 12:57:28 PST (-0800), [email protected] 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
>
> / {
> 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.
I think I was actually wrong here: the top version is preferred if
timebase-frequency is the same between all CPUs, while the bottom is if it's
different.
Updating the documentation is still good, because it matches the hardware, but
the commit message should be a bit less assertive... :)
> 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>;
On Mon, 03 Dec 2018 12:57:29 PST (-0800), [email protected] 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 | 22 ++++++++++++++++++++++
> 2 files changed, 23 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..96af7058 100644
> --- a/drivers/clocksource/riscv_timer.c
> +++ b/drivers/clocksource/riscv_timer.c
> @@ -83,6 +83,26 @@ 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;
> +
> + /*
> + * 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))
> + 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 +114,8 @@ static int __init riscv_timer_init_dt(struct device_node *n)
> if (cpuid != smp_processor_id())
> return 0;
>
> + /* This should be called only for boot cpu */
> + riscv_timebase = riscv_timebase_frequency(n);
> cs = per_cpu_ptr(&riscv_clocksource, cpuid);
> clocksource_register_hz(cs, riscv_timebase);
We need to check to make sure the timebase-frequency of each hart is the same.
This is mandated by the RISC-V ISA specification but should be checked in the
code.
On Mon, 03 Dec 2018 12:57:31 PST (-0800), [email protected] wrote:
> 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 | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clocksource/riscv_timer.c b/drivers/clocksource/riscv_timer.c
> index 39de6e49..4af4af47 100644
> --- a/drivers/clocksource/riscv_timer.c
> +++ b/drivers/clocksource/riscv_timer.c
> @@ -108,6 +108,8 @@ static int __init riscv_timer_init_dt(struct device_node *n)
> int cpuid, hartid, error;
>
> hartid = riscv_of_processor_hartid(n);
> + if (hartid < 0)
> + return hartid;
This seems like it's just hiding a bug somewhere else. We should at least put
out a WARN here, as I'm not sure the error will propagate anywhere useful.
> cpuid = riscv_hartid_to_cpuid(hartid);
>
> if (cpuid != smp_processor_id())
> @@ -115,14 +117,19 @@ static int __init riscv_timer_init_dt(struct device_node *n)
>
> /* This should be called only for boot cpu */
> riscv_timebase = riscv_timebase_frequency(n);
> - 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;
> }
On Mon, 03 Dec 2018 12:57:30 PST (-0800), [email protected] wrote:
> 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 96af7058..39de6e49 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),
> @@ -106,7 +106,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);
> @@ -116,8 +115,7 @@ static int __init riscv_timer_init_dt(struct device_node *n)
>
> /* This should be called only for boot cpu */
> 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",
Reviewed-by: Palmer Dabbelt <[email protected]>
On 12/7/18 9:00 AM, Palmer Dabbelt wrote:
> On Mon, 03 Dec 2018 12:57:31 PST (-0800), [email protected] wrote:
>> 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 | 13 ++++++++++---
>> 1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clocksource/riscv_timer.c b/drivers/clocksource/riscv_timer.c
>> index 39de6e49..4af4af47 100644
>> --- a/drivers/clocksource/riscv_timer.c
>> +++ b/drivers/clocksource/riscv_timer.c
>> @@ -108,6 +108,8 @@ static int __init riscv_timer_init_dt(struct device_node *n)
>> int cpuid, hartid, error;
>>
>> hartid = riscv_of_processor_hartid(n);
>> + if (hartid < 0)
>> + return hartid;
>
> This seems like it's just hiding a bug somewhere else. We should at least put
> out a WARN here, as I'm not sure the error will propagate anywhere useful.
>
ok. I will add a warning here. That's what we are doing in plic as well.
Regards,
Atish
>> cpuid = riscv_hartid_to_cpuid(hartid);
>>
>> if (cpuid != smp_processor_id())
>> @@ -115,14 +117,19 @@ static int __init riscv_timer_init_dt(struct device_node *n)
>>
>> /* This should be called only for boot cpu */
>> riscv_timebase = riscv_timebase_frequency(n);
>> - 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;
>> }
>
On 12/7/18 8:42 AM, Palmer Dabbelt wrote:
> On Mon, 03 Dec 2018 12:57:29 PST (-0800), [email protected] 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 | 22 ++++++++++++++++++++++
>> 2 files changed, 23 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..96af7058 100644
>> --- a/drivers/clocksource/riscv_timer.c
>> +++ b/drivers/clocksource/riscv_timer.c
>> @@ -83,6 +83,26 @@ 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;
>> +
>> + /*
>> + * 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))
>> + 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 +114,8 @@ static int __init riscv_timer_init_dt(struct device_node *n)
>> if (cpuid != smp_processor_id())
>> return 0;
>>
>> + /* This should be called only for boot cpu */
>> + riscv_timebase = riscv_timebase_frequency(n);
>> cs = per_cpu_ptr(&riscv_clocksource, cpuid);
>> clocksource_register_hz(cs, riscv_timebase);
>
> We need to check to make sure the timebase-frequency of each hart is the same.
> This is mandated by the RISC-V ISA specification but should be checked in the
> code.
>
Fair enough. I will add a timebase-frequency verification function that
will be executed for every cpu instead of boot cpu.
If any cpu's timebase-frequency doesn't match with boot cpu, should we
just WARN? or Do we need panic given that DT is not following something
that is mandated by ISA ?
Regards,
Atish
On Fri, 07 Dec 2018 09:20:57 PST (-0800), [email protected] wrote:
> On Fri, 7 Dec, 2018, 10:30 PM Palmer Dabbelt <[email protected] wrote:
>
>> On Mon, 03 Dec 2018 12:57:31 PST (-0800), [email protected] wrote:
>> > 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 | 13 ++++++++++---
>> > 1 file changed, 10 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/clocksource/riscv_timer.c
>> b/drivers/clocksource/riscv_timer.c
>> > index 39de6e49..4af4af47 100644
>> > --- a/drivers/clocksource/riscv_timer.c
>> > +++ b/drivers/clocksource/riscv_timer.c
>> > @@ -108,6 +108,8 @@ static int __init riscv_timer_init_dt(struct
>> device_node *n)
>> > int cpuid, hartid, error;
>> >
>> > hartid = riscv_of_processor_hartid(n);
>> > + if (hartid < 0)
>> > + return hartid;
>>
>> This seems like it's just hiding a bug somewhere else. We should at least
>> put
>> out a WARN here, as I'm not sure the error will propagate anywhere useful.
>>
>
> We need separate DT node for riscv_timer. The riscv_timer is nothing but
> SOC timer accessed via rdtime and SBI calls. It can be viewed as one
> device. In fact, this is how it's done in ARM/ARM64.
We had that at some point, but this was changed. The logic was that, since the
RISC-V ISA mandates the presence of this timer for all harts, the RISC-V CPU
node is sufficient to encode the presence of a RISC-V timer.
I'm OK changing this, but you should look at the old thread (which I can't
find) to make sure all the arguments are taken into account.
>> > cpuid = riscv_hartid_to_cpuid(hartid);
>> >
>> > if (cpuid != smp_processor_id())
>> > @@ -115,14 +117,19 @@ static int __init riscv_timer_init_dt(struct
>> device_node *n)
>> >
>> > /* This should be called only for boot cpu */
>> > riscv_timebase = riscv_timebase_frequency(n);
>> > - 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;
>> > }
>>
>
> Regards,
> Anup
>
>>