[PATCH 0/4] Timer code cleanup.

[PATCH 0/4] Timer code cleanup.

[Atish Patra]

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

[PATCH 0/4] Timer code cleanup.

[Atish Patra]

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

[PATCH 1/4] dt-bindings: Correct RISC-V's timebase-frequency

[Atish Patra]

From: Palmer Dabbelt <palmer@sifive.com>

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

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

to

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

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

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

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

[PATCH 1/4] dt-bindings: Correct RISC-V's timebase-frequency

[Atish Patra]

From: Palmer Dabbelt <palmer@sifive.com>

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

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

to

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

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

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

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


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

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

[Palmer Dabbelt]

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

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

[Palmer Dabbelt]

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

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

[Palmer Dabbelt]

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

[PATCH 2/4] RISC-V: Support per-hart timebase-frequency

[Atish Patra]

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

Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
 arch/riscv/kernel/time.c          |  9 +--------
 drivers/clocksource/riscv_timer.c | 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

[PATCH 2/4] RISC-V: Support per-hart timebase-frequency

[Atish Patra]

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

Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
 arch/riscv/kernel/time.c          |  9 +--------
 drivers/clocksource/riscv_timer.c | 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

[PATCH 2/4] RISC-V: Support per-hart timebase-frequency

[Atish Patra]

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

Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
 arch/riscv/kernel/time.c          |  9 +--------
 drivers/clocksource/riscv_timer.c | 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

Re: [PATCH 2/4] RISC-V: Support per-hart timebase-frequency

[Palmer Dabbelt]

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.

Re: [PATCH 2/4] RISC-V: Support per-hart timebase-frequency

[Palmer Dabbelt]

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

Re: [PATCH 2/4] RISC-V: Support per-hart timebase-frequency

[Palmer Dabbelt]

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.

Re: [PATCH 2/4] RISC-V: Support per-hart timebase-frequency

[Atish Patra]

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

Re: [PATCH 2/4] RISC-V: Support per-hart timebase-frequency

[Atish Patra]

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

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

[Atish Patra]

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

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

diff --git a/drivers/clocksource/riscv_timer.c b/drivers/clocksource/riscv_timer.c
index 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

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

[Atish Patra]

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

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

diff --git a/drivers/clocksource/riscv_timer.c b/drivers/clocksource/riscv_timer.c
index 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

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

[Atish Patra]

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

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

diff --git a/drivers/clocksource/riscv_timer.c b/drivers/clocksource/riscv_timer.c
index 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

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

[Palmer Dabbelt]

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>

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

[Palmer Dabbelt]

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

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

[Palmer Dabbelt]

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>

[PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems

[Atish Patra]

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

[PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems

[Atish Patra]

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

Re: [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems

[Palmer Dabbelt]

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

Re: [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems

[Palmer Dabbelt]

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

Re: [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems

[Palmer Dabbelt]

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

Re: [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems

[Anup Patel]

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

Re: [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems

[Palmer Dabbelt]

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

Re: [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems

[Palmer Dabbelt]

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

Re: [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems

[Palmer Dabbelt]

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

Re: [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems

[Atish Patra]

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

Re: [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems

[Atish Patra]

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