All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] Timer code cleanup.
@ 2018-12-03 20:57 ` Atish Patra
  0 siblings, 0 replies; 32+ messages in thread
From: Atish Patra @ 2018-12-03 20:57 UTC (permalink / raw)
  To: linux-kernel
  Cc: Atish Patra, Albert Ou, Daniel Lezcano, devicetree,
	Dmitriy Cherkasov, linux-riscv, Mark Rutland, Palmer Dabbelt,
	Rob Herring, Thomas Gleixner, Anup Patel, Damien Le Moal


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


^ permalink raw reply	[flat|nested] 32+ messages in thread
* [PATCH 0/4] Timer code cleanup.
@ 2018-12-03 20:57 ` Atish Patra
  0 siblings, 0 replies; 32+ messages in thread
From: Atish Patra @ 2018-12-03 20:57 UTC (permalink / raw)
  To: linux-kernel
  Cc: Mark Rutland, devicetree, Damien Le Moal, Albert Ou,
	Dmitriy Cherkasov, Anup Patel, Daniel Lezcano, Atish Patra,
	Rob Herring, Palmer Dabbelt, linux-riscv, Thomas Gleixner


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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 32+ messages in thread
* [PATCH 1/4] dt-bindings: Correct RISC-V's timebase-frequency
  2018-12-03 20:57 ` Atish Patra
@ 2018-12-03 20:57   ` Atish Patra
  -1 siblings, 0 replies; 32+ messages in thread
From: Atish Patra @ 2018-12-03 20:57 UTC (permalink / raw)
  To: linux-kernel
  Cc: Palmer Dabbelt, Christoph Hellwig, Albert Ou, Atish Patra,
	Daniel Lezcano, devicetree, Dmitriy Cherkasov, linux-riscv,
	Mark Rutland, Rob Herring, Thomas Gleixner, Anup Patel,
	Damien Le Moal

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


^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [PATCH 1/4] dt-bindings: Correct RISC-V's timebase-frequency
@ 2018-12-03 20:57   ` Atish Patra
  0 siblings, 0 replies; 32+ messages in thread
From: Atish Patra @ 2018-12-03 20:57 UTC (permalink / raw)
  To: linux-kernel
  Cc: Mark Rutland, devicetree, Damien Le Moal, Palmer Dabbelt,
	Dmitriy Cherkasov, Anup Patel, Daniel Lezcano, Rob Herring,
	Atish Patra, Albert Ou, Thomas Gleixner, linux-riscv,
	Christoph Hellwig

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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [PATCH 2/4] RISC-V: Support per-hart timebase-frequency
  2018-12-03 20:57 ` Atish Patra
  (?)
@ 2018-12-03 20:57   ` Atish Patra
  -1 siblings, 0 replies; 32+ messages in thread
From: Atish Patra @ 2018-12-03 20:57 UTC (permalink / raw)
  To: linux-kernel
  Cc: Atish Patra, Albert Ou, Daniel Lezcano, devicetree,
	Dmitriy Cherkasov, linux-riscv, Mark Rutland, Palmer Dabbelt,
	Rob Herring, Thomas Gleixner, Anup Patel, Damien Le Moal

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 | 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


^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [PATCH 2/4] RISC-V: Support per-hart timebase-frequency
@ 2018-12-03 20:57   ` Atish Patra
  0 siblings, 0 replies; 32+ messages in thread
From: Atish Patra @ 2018-12-03 20:57 UTC (permalink / raw)
  To: linux-kernel
  Cc: Mark Rutland, devicetree, Damien Le Moal, Albert Ou,
	Dmitriy Cherkasov, Anup Patel, Daniel Lezcano, Atish Patra,
	Rob Herring, Palmer Dabbelt, linux-riscv, Thomas Gleixner

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 | 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

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [PATCH 2/4] RISC-V: Support per-hart timebase-frequency
@ 2018-12-03 20:57   ` Atish Patra
  0 siblings, 0 replies; 32+ messages in thread
From: Atish Patra @ 2018-12-03 20:57 UTC (permalink / raw)
  To: linux-kernel
  Cc: Mark Rutland, devicetree, Damien Le Moal, Albert Ou,
	Dmitriy Cherkasov, Anup Patel, Daniel Lezcano, Atish Patra,
	Rob Herring, Palmer Dabbelt, linux-riscv, Thomas Gleixner

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 | 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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [PATCH 3/4] RISC-V: Remove per cpu clocksource
  2018-12-03 20:57 ` Atish Patra
  (?)
@ 2018-12-03 20:57   ` Atish Patra
  -1 siblings, 0 replies; 32+ messages in thread
From: Atish Patra @ 2018-12-03 20:57 UTC (permalink / raw)
  To: linux-kernel
  Cc: Atish Patra, Albert Ou, Daniel Lezcano, devicetree,
	Dmitriy Cherkasov, linux-riscv, Mark Rutland, Palmer Dabbelt,
	Rob Herring, Thomas Gleixner, Anup Patel, Damien Le Moal

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 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


^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [PATCH 3/4] RISC-V: Remove per cpu clocksource
@ 2018-12-03 20:57   ` Atish Patra
  0 siblings, 0 replies; 32+ messages in thread
From: Atish Patra @ 2018-12-03 20:57 UTC (permalink / raw)
  To: linux-kernel
  Cc: Mark Rutland, devicetree, Damien Le Moal, Albert Ou,
	Dmitriy Cherkasov, Anup Patel, Daniel Lezcano, Atish Patra,
	Rob Herring, Palmer Dabbelt, linux-riscv, Thomas Gleixner

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 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

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [PATCH 3/4] RISC-V: Remove per cpu clocksource
@ 2018-12-03 20:57   ` Atish Patra
  0 siblings, 0 replies; 32+ messages in thread
From: Atish Patra @ 2018-12-03 20:57 UTC (permalink / raw)
  To: linux-kernel
  Cc: Mark Rutland, devicetree, Damien Le Moal, Albert Ou,
	Dmitriy Cherkasov, Anup Patel, Daniel Lezcano, Atish Patra,
	Rob Herring, Palmer Dabbelt, linux-riscv, Thomas Gleixner

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 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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems
  2018-12-03 20:57 ` Atish Patra
@ 2018-12-03 20:57   ` Atish Patra
  -1 siblings, 0 replies; 32+ messages in thread
From: Atish Patra @ 2018-12-03 20:57 UTC (permalink / raw)
  To: linux-kernel
  Cc: Atish Patra, Albert Ou, Daniel Lezcano, devicetree,
	Dmitriy Cherkasov, linux-riscv, Mark Rutland, Palmer Dabbelt,
	Rob Herring, Thomas Gleixner, Anup Patel, Damien Le Moal

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 <atish.patra@wdc.com>
---
 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


^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems
@ 2018-12-03 20:57   ` Atish Patra
  0 siblings, 0 replies; 32+ messages in thread
From: Atish Patra @ 2018-12-03 20:57 UTC (permalink / raw)
  To: linux-kernel
  Cc: Mark Rutland, devicetree, Damien Le Moal, Albert Ou,
	Dmitriy Cherkasov, Anup Patel, Daniel Lezcano, Atish Patra,
	Rob Herring, Palmer Dabbelt, linux-riscv, Thomas Gleixner

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 <atish.patra@wdc.com>
---
 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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: Correct RISC-V's timebase-frequency
  2018-12-03 20:57   ` Atish Patra
  (?)
@ 2018-12-07 16:29     ` Palmer Dabbelt
  -1 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2018-12-07 16:29 UTC (permalink / raw)
  To: atish.patra
  Cc: linux-kernel, Christoph Hellwig, aou, atish.patra,
	daniel.lezcano, devicetree, dmitriy, linux-riscv, mark.rutland,
	robh+dt, tglx, anup, Damien.LeMoal

On Mon, 03 Dec 2018 12:57:28 PST (-0800), atish.patra@wdc.com 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
>
> / {
>         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 <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>;

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: Correct RISC-V's timebase-frequency
@ 2018-12-07 16:29     ` Palmer Dabbelt
  0 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2018-12-07 16:29 UTC (permalink / raw)
  Cc: linux-kernel, Christoph Hellwig, aou, atish.patra,
	daniel.lezcano, devicetree, dmitriy, linux-riscv, mark.rutland,
	robh+dt, tglx, anup, Damien.LeMoal

On Mon, 03 Dec 2018 12:57:28 PST (-0800), atish.patra@wdc.com 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
>
> / {
>         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 <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>;

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] dt-bindings: Correct RISC-V's timebase-frequency
@ 2018-12-07 16:29     ` Palmer Dabbelt
  0 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2018-12-07 16:29 UTC (permalink / raw)
  To: atish.patra
  Cc: mark.rutland, devicetree, Damien.LeMoal, aou, dmitriy, anup,
	daniel.lezcano, linux-kernel, atish.patra, robh+dt, tglx,
	linux-riscv, Christoph Hellwig

On Mon, 03 Dec 2018 12:57:28 PST (-0800), atish.patra@wdc.com 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
>
> / {
>         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 <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>;

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] RISC-V: Support per-hart timebase-frequency
  2018-12-03 20:57   ` Atish Patra
  (?)
@ 2018-12-07 16:42     ` Palmer Dabbelt
  -1 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2018-12-07 16:42 UTC (permalink / raw)
  To: atish.patra
  Cc: linux-kernel, atish.patra, aou, daniel.lezcano, devicetree,
	dmitriy, linux-riscv, mark.rutland, robh+dt, tglx, anup,
	Damien.LeMoal

On Mon, 03 Dec 2018 12:57:29 PST (-0800), atish.patra@wdc.com 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>
> ---
>  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.

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] RISC-V: Support per-hart timebase-frequency
@ 2018-12-07 16:42     ` Palmer Dabbelt
  0 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2018-12-07 16:42 UTC (permalink / raw)
  Cc: linux-kernel, atish.patra, aou, daniel.lezcano, devicetree,
	dmitriy, linux-riscv, mark.rutland, robh+dt, tglx, anup,
	Damien.LeMoal

On Mon, 03 Dec 2018 12:57:29 PST (-0800), atish.patra@wdc.com 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>
> ---
>  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.

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] RISC-V: Support per-hart timebase-frequency
@ 2018-12-07 16:42     ` Palmer Dabbelt
  0 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2018-12-07 16:42 UTC (permalink / raw)
  To: atish.patra
  Cc: mark.rutland, devicetree, Damien.LeMoal, aou, dmitriy, anup,
	daniel.lezcano, linux-kernel, atish.patra, robh+dt, linux-riscv,
	tglx

On Mon, 03 Dec 2018 12:57:29 PST (-0800), atish.patra@wdc.com 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>
> ---
>  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.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 3/4] RISC-V: Remove per cpu clocksource
  2018-12-03 20:57   ` Atish Patra
  (?)
@ 2018-12-07 17:00     ` Palmer Dabbelt
  -1 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2018-12-07 17:00 UTC (permalink / raw)
  To: atish.patra
  Cc: linux-kernel, atish.patra, aou, daniel.lezcano, devicetree,
	dmitriy, linux-riscv, mark.rutland, robh+dt, tglx, anup,
	Damien.LeMoal

On Mon, 03 Dec 2018 12:57:30 PST (-0800), atish.patra@wdc.com 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 <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 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 <palmer@sifive.com>

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 3/4] RISC-V: Remove per cpu clocksource
@ 2018-12-07 17:00     ` Palmer Dabbelt
  0 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2018-12-07 17:00 UTC (permalink / raw)
  Cc: linux-kernel, atish.patra, aou, daniel.lezcano, devicetree,
	dmitriy, linux-riscv, mark.rutland, robh+dt, tglx, anup,
	Damien.LeMoal

On Mon, 03 Dec 2018 12:57:30 PST (-0800), atish.patra@wdc.com 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 <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 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 <palmer@sifive.com>

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 3/4] RISC-V: Remove per cpu clocksource
@ 2018-12-07 17:00     ` Palmer Dabbelt
  0 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2018-12-07 17:00 UTC (permalink / raw)
  To: atish.patra
  Cc: mark.rutland, devicetree, Damien.LeMoal, aou, dmitriy, anup,
	daniel.lezcano, linux-kernel, atish.patra, robh+dt, linux-riscv,
	tglx

On Mon, 03 Dec 2018 12:57:30 PST (-0800), atish.patra@wdc.com 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 <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 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 <palmer@sifive.com>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems
  2018-12-03 20:57   ` Atish Patra
  (?)
@ 2018-12-07 17:00     ` Palmer Dabbelt
  -1 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2018-12-07 17:00 UTC (permalink / raw)
  To: atish.patra
  Cc: linux-kernel, atish.patra, aou, daniel.lezcano, devicetree,
	dmitriy, linux-riscv, mark.rutland, robh+dt, tglx, anup,
	Damien.LeMoal

On Mon, 03 Dec 2018 12:57:31 PST (-0800), atish.patra@wdc.com 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 <atish.patra@wdc.com>
> ---
>  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;
>  }

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems
@ 2018-12-07 17:00     ` Palmer Dabbelt
  0 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2018-12-07 17:00 UTC (permalink / raw)
  Cc: linux-kernel, atish.patra, aou, daniel.lezcano, devicetree,
	dmitriy, linux-riscv, mark.rutland, robh+dt, tglx, anup,
	Damien.LeMoal

On Mon, 03 Dec 2018 12:57:31 PST (-0800), atish.patra@wdc.com 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 <atish.patra@wdc.com>
> ---
>  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;
>  }

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems
@ 2018-12-07 17:00     ` Palmer Dabbelt
  0 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2018-12-07 17:00 UTC (permalink / raw)
  To: atish.patra
  Cc: mark.rutland, devicetree, Damien.LeMoal, aou, dmitriy, anup,
	daniel.lezcano, linux-kernel, atish.patra, robh+dt, linux-riscv,
	tglx

On Mon, 03 Dec 2018 12:57:31 PST (-0800), atish.patra@wdc.com 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 <atish.patra@wdc.com>
> ---
>  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;
>  }

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems
  2018-12-07 17:00     ` Palmer Dabbelt
  (?)
  (?)
@ 2018-12-07 17:20     ` Anup Patel
  2018-12-08 20:25         ` Palmer Dabbelt
  -1 siblings, 1 reply; 32+ messages in thread
From: Anup Patel @ 2018-12-07 17:20 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: Atish Patra, linux-kernel@vger.kernel.org List, aou,
	daniel.lezcano, devicetree, dmitriy, linux-riscv, mark.rutland,
	robh+dt, tglx, Damien.LeMoal

[-- Attachment #1: Type: text/plain, Size: 2593 bytes --]

On Fri, 7 Dec, 2018, 10:30 PM Palmer Dabbelt <palmer@sifive.com wrote:

> On Mon, 03 Dec 2018 12:57:31 PST (-0800), atish.patra@wdc.com 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 <atish.patra@wdc.com>
> > ---
> >  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.


> >       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

>

[-- Attachment #2: Type: text/html, Size: 3966 bytes --]

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems
  2018-12-07 17:00     ` Palmer Dabbelt
@ 2018-12-07 23:31       ` Atish Patra
  -1 siblings, 0 replies; 32+ messages in thread
From: Atish Patra @ 2018-12-07 23:31 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: linux-kernel, aou, daniel.lezcano, devicetree, dmitriy,
	linux-riscv, mark.rutland, robh+dt, tglx, anup, Damien Le Moal

On 12/7/18 9:00 AM, Palmer Dabbelt wrote:
> On Mon, 03 Dec 2018 12:57:31 PST (-0800), atish.patra@wdc.com 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 <atish.patra@wdc.com>
>> ---
>>   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;
>>   }
> 


^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems
@ 2018-12-07 23:31       ` Atish Patra
  0 siblings, 0 replies; 32+ messages in thread
From: Atish Patra @ 2018-12-07 23:31 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: mark.rutland, devicetree, Damien Le Moal, aou, dmitriy, anup,
	daniel.lezcano, linux-kernel, robh+dt, linux-riscv, tglx

On 12/7/18 9:00 AM, Palmer Dabbelt wrote:
> On Mon, 03 Dec 2018 12:57:31 PST (-0800), atish.patra@wdc.com 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 <atish.patra@wdc.com>
>> ---
>>   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;
>>   }
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] RISC-V: Support per-hart timebase-frequency
  2018-12-07 16:42     ` Palmer Dabbelt
@ 2018-12-07 23:36       ` Atish Patra
  -1 siblings, 0 replies; 32+ messages in thread
From: Atish Patra @ 2018-12-07 23:36 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: linux-kernel, aou, daniel.lezcano, devicetree, dmitriy,
	linux-riscv, mark.rutland, robh+dt, tglx, anup, Damien Le Moal

On 12/7/18 8:42 AM, Palmer Dabbelt wrote:
> On Mon, 03 Dec 2018 12:57:29 PST (-0800), atish.patra@wdc.com 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>
>> ---
>>   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

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] RISC-V: Support per-hart timebase-frequency
@ 2018-12-07 23:36       ` Atish Patra
  0 siblings, 0 replies; 32+ messages in thread
From: Atish Patra @ 2018-12-07 23:36 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: mark.rutland, devicetree, Damien Le Moal, aou, dmitriy, anup,
	daniel.lezcano, linux-kernel, robh+dt, linux-riscv, tglx

On 12/7/18 8:42 AM, Palmer Dabbelt wrote:
> On Mon, 03 Dec 2018 12:57:29 PST (-0800), atish.patra@wdc.com 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>
>> ---
>>   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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems
  2018-12-07 17:20     ` Anup Patel
  2018-12-08 20:25         ` Palmer Dabbelt
@ 2018-12-08 20:25         ` Palmer Dabbelt
  0 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2018-12-08 20:25 UTC (permalink / raw)
  To: anup
  Cc: atish.patra, linux-kernel, aou, daniel.lezcano, devicetree,
	dmitriy, linux-riscv, mark.rutland, robh+dt, tglx, Damien.LeMoal

On Fri, 07 Dec 2018 09:20:57 PST (-0800), anup@brainfault.org wrote:
> On Fri, 7 Dec, 2018, 10:30 PM Palmer Dabbelt <palmer@sifive.com wrote:
>
>> On Mon, 03 Dec 2018 12:57:31 PST (-0800), atish.patra@wdc.com 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 <atish.patra@wdc.com>
>> > ---
>> >  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
>
>>

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems
@ 2018-12-08 20:25         ` Palmer Dabbelt
  0 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2018-12-08 20:25 UTC (permalink / raw)
  To: anup
  Cc: mark.rutland, devicetree, Damien.LeMoal, aou, dmitriy,
	daniel.lezcano, linux-kernel, atish.patra, robh+dt, linux-riscv,
	tglx

On Fri, 07 Dec 2018 09:20:57 PST (-0800), anup@brainfault.org wrote:
> On Fri, 7 Dec, 2018, 10:30 PM Palmer Dabbelt <palmer@sifive.com wrote:
>
>> On Mon, 03 Dec 2018 12:57:31 PST (-0800), atish.patra@wdc.com 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 <atish.patra@wdc.com>
>> > ---
>> >  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
>
>>

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems
@ 2018-12-08 20:25         ` Palmer Dabbelt
  0 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2018-12-08 20:25 UTC (permalink / raw)
  To: anup
  Cc: mark.rutland, devicetree, Damien.LeMoal, aou, dmitriy,
	daniel.lezcano, linux-kernel, atish.patra, robh+dt, linux-riscv,
	tglx

On Fri, 07 Dec 2018 09:20:57 PST (-0800), anup@brainfault.org wrote:
> On Fri, 7 Dec, 2018, 10:30 PM Palmer Dabbelt <palmer@sifive.com wrote:
>
>> On Mon, 03 Dec 2018 12:57:31 PST (-0800), atish.patra@wdc.com 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 <atish.patra@wdc.com>
>> > ---
>> >  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
>
>>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 32+ messages in thread
end of thread, other threads:[~2018-12-08 20:26 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-03 20:57 [PATCH 0/4] Timer code cleanup Atish Patra
2018-12-03 20:57 ` Atish Patra
2018-12-03 20:57 ` [PATCH 1/4] dt-bindings: Correct RISC-V's timebase-frequency Atish Patra
2018-12-03 20:57   ` Atish Patra
2018-12-07 16:29   ` Palmer Dabbelt
2018-12-07 16:29     ` Palmer Dabbelt
2018-12-07 16:29     ` Palmer Dabbelt
2018-12-03 20:57 ` [PATCH 2/4] RISC-V: Support per-hart timebase-frequency Atish Patra
2018-12-03 20:57   ` Atish Patra
2018-12-03 20:57   ` Atish Patra
2018-12-07 16:42   ` Palmer Dabbelt
2018-12-07 16:42     ` Palmer Dabbelt
2018-12-07 16:42     ` Palmer Dabbelt
2018-12-07 23:36     ` Atish Patra
2018-12-07 23:36       ` Atish Patra
2018-12-03 20:57 ` [PATCH 3/4] RISC-V: Remove per cpu clocksource Atish Patra
2018-12-03 20:57   ` Atish Patra
2018-12-03 20:57   ` Atish Patra
2018-12-07 17:00   ` Palmer Dabbelt
2018-12-07 17:00     ` Palmer Dabbelt
2018-12-07 17:00     ` Palmer Dabbelt
2018-12-03 20:57 ` [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems Atish Patra
2018-12-03 20:57   ` Atish Patra
2018-12-07 17:00   ` Palmer Dabbelt
2018-12-07 17:00     ` Palmer Dabbelt
2018-12-07 17:00     ` Palmer Dabbelt
2018-12-07 17:20     ` Anup Patel
2018-12-08 20:25       ` Palmer Dabbelt
2018-12-08 20:25         ` Palmer Dabbelt
2018-12-08 20:25         ` Palmer Dabbelt
2018-12-07 23:31     ` Atish Patra
2018-12-07 23:31       ` Atish Patra

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.