linux-riscv.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/4] Dedicated CLINT timer driver
@ 2020-07-17  7:50 Anup Patel
  2020-07-17  7:50 ` [PATCH v4 1/4] RISC-V: Add mechanism to provide custom IPI operations Anup Patel
                   ` (3 more replies)
  0 siblings, 4 replies; 19+ messages in thread
From: Anup Patel @ 2020-07-17  7:50 UTC (permalink / raw)
  To: Palmer Dabbelt, Paul Walmsley, Albert Ou, Rob Herring,
	Daniel Lezcano, Thomas Gleixner
  Cc: devicetree, Damien Le Moal, Anup Patel, Anup Patel, linux-kernel,
	Atish Patra, Alistair Francis, linux-riscv

The current RISC-V timer driver is convoluted and implements two
distinct timers:
 1. S-mode timer: This is for Linux RISC-V S-mode with MMU. The
    clocksource is implemented using TIME CSR and clockevent device
    is implemented using SBI Timer calls.
 2. M-mode timer: This is for Linux RISC-V M-mode without MMU. The
    clocksource is implemented using CLINT MMIO time register and
    clockevent device is implemented using CLINT MMIO timecmp registers.

This patchset removes clint related code from RISC-V timer driver and
arch/riscv directory. Instead, the series adds a dedicated MMIO based
CLINT driver under drivers/clocksource directory which can be used by
Linux RISC-V M-mode (i.e NoMMU Linux RISC-V).

The patchset is based up Linux-5.8-rc5 and can be found at riscv_clint_v4
branch of: https://github.com/avpatel/linux.git

This series is tested on:
 1. QEMU RV64 virt machine using Linux RISC-V S-mode
 2. QEMU RV32 virt machine using Linux RISC-V S-mode
 3. QEMU RV64 virt machine using Linux RISC-V M-mode (i.e. NoMMU)

Changes since v3:
 - Updated commit description of PATCH2
 - Use clint_get_cycles64() in clint_rdtime() of PATCH2
 - Call clockevents_config_and_register() only once for each CPU in
   clint_timer_starting_cpu of PATCH2
 - Select CLINT timer driver from platform Kconfig in PATCH3
 - Fixed 'make dt_binding_check' for PATCH4

Changes since v2:
 - Rebased series on Linux-5.8-rc5
 - Squashed PATCH3 onto PATCH2 to preserve GIT bisectability
 - Moved PATCH4 before PATCH2 to preserve GIT bisectability
 - Replaced CLINT dt-bindings text document with YAML schema
 - Use SiFive CLINT compatible string as per SiFive IP block versioning

Changes since v1:
 - Rebased series on Linux-5.8-rc2
 - Added pr_warn() for case where ipi_ops not available in PATCH1
 - Updated ipi_inject() prototype to use "struct cpumask *" in PATCH1
 - Updated CLINT_TIMER kconfig option to depend on RISCV_M_MODE in PATCH4
 - Added riscv,clint0 compatible string in DT bindings document

Anup Patel (4):
  RISC-V: Add mechanism to provide custom IPI operations
  clocksource/drivers: Add CLINT timer driver
  RISC-V: Remove CLINT related code from timer and arch
  dt-bindings: timer: Add CLINT bindings

 .../bindings/timer/sifive,clint.yaml          |  58 +++++
 arch/riscv/Kconfig                            |   2 +-
 arch/riscv/Kconfig.socs                       |   2 +
 arch/riscv/configs/nommu_virt_defconfig       |   7 +-
 arch/riscv/include/asm/clint.h                |  39 ---
 arch/riscv/include/asm/smp.h                  |  19 ++
 arch/riscv/include/asm/timex.h                |  28 +--
 arch/riscv/kernel/Makefile                    |   2 +-
 arch/riscv/kernel/clint.c                     |  44 ----
 arch/riscv/kernel/sbi.c                       |  14 ++
 arch/riscv/kernel/setup.c                     |   2 -
 arch/riscv/kernel/smp.c                       |  44 ++--
 arch/riscv/kernel/smpboot.c                   |   4 +-
 drivers/clocksource/Kconfig                   |  11 +-
 drivers/clocksource/Makefile                  |   1 +
 drivers/clocksource/timer-clint.c             | 231 ++++++++++++++++++
 drivers/clocksource/timer-riscv.c             |  17 +-
 include/linux/cpuhotplug.h                    |   1 +
 18 files changed, 374 insertions(+), 152 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/timer/sifive,clint.yaml
 delete mode 100644 arch/riscv/include/asm/clint.h
 delete mode 100644 arch/riscv/kernel/clint.c
 create mode 100644 drivers/clocksource/timer-clint.c

-- 
2.25.1


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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v4 1/4] RISC-V: Add mechanism to provide custom IPI operations
  2020-07-17  7:50 [PATCH v4 0/4] Dedicated CLINT timer driver Anup Patel
@ 2020-07-17  7:50 ` Anup Patel
  2020-07-21  0:46   ` Atish Patra
  2020-07-17  7:50 ` [PATCH v4 2/4] clocksource/drivers: Add CLINT timer driver Anup Patel
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 19+ messages in thread
From: Anup Patel @ 2020-07-17  7:50 UTC (permalink / raw)
  To: Palmer Dabbelt, Paul Walmsley, Albert Ou, Rob Herring,
	Daniel Lezcano, Thomas Gleixner
  Cc: devicetree, Damien Le Moal, Emil Renner Berhing, Anup Patel,
	Anup Patel, linux-kernel, Atish Patra, Alistair Francis,
	linux-riscv

We add mechanism to set custom IPI operations so that CLINT driver
from drivers directory can provide custom IPI operations.

Signed-off-by: Anup Patel <anup.patel@wdc.com>
Tested-by: Emil Renner Berhing <kernel@esmil.dk>
---
 arch/riscv/include/asm/clint.h | 25 --------------------
 arch/riscv/include/asm/smp.h   | 19 +++++++++++++++
 arch/riscv/kernel/clint.c      | 23 ++++++++++++++++--
 arch/riscv/kernel/sbi.c        | 14 +++++++++++
 arch/riscv/kernel/smp.c        | 43 +++++++++++++++++++---------------
 arch/riscv/kernel/smpboot.c    |  3 +--
 6 files changed, 79 insertions(+), 48 deletions(-)

diff --git a/arch/riscv/include/asm/clint.h b/arch/riscv/include/asm/clint.h
index a279b17a6aad..adaba98a7d6c 100644
--- a/arch/riscv/include/asm/clint.h
+++ b/arch/riscv/include/asm/clint.h
@@ -6,34 +6,9 @@
 #include <linux/smp.h>
 
 #ifdef CONFIG_RISCV_M_MODE
-extern u32 __iomem *clint_ipi_base;
-
 void clint_init_boot_cpu(void);
-
-static inline void clint_send_ipi_single(unsigned long hartid)
-{
-	writel(1, clint_ipi_base + hartid);
-}
-
-static inline void clint_send_ipi_mask(const struct cpumask *mask)
-{
-	int cpu;
-
-	for_each_cpu(cpu, mask)
-		clint_send_ipi_single(cpuid_to_hartid_map(cpu));
-}
-
-static inline void clint_clear_ipi(unsigned long hartid)
-{
-	writel(0, clint_ipi_base + hartid);
-}
 #else /* CONFIG_RISCV_M_MODE */
 #define clint_init_boot_cpu()	do { } while (0)
-
-/* stubs to for code is only reachable under IS_ENABLED(CONFIG_RISCV_M_MODE): */
-void clint_send_ipi_single(unsigned long hartid);
-void clint_send_ipi_mask(const struct cpumask *hartid_mask);
-void clint_clear_ipi(unsigned long hartid);
 #endif /* CONFIG_RISCV_M_MODE */
 
 #endif /* _ASM_RISCV_CLINT_H */
diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
index 40bb1c15a731..68de78a8eba6 100644
--- a/arch/riscv/include/asm/smp.h
+++ b/arch/riscv/include/asm/smp.h
@@ -15,6 +15,11 @@
 struct seq_file;
 extern unsigned long boot_cpu_hartid;
 
+struct riscv_ipi_ops {
+	void (*ipi_inject)(const struct cpumask *target);
+	void (*ipi_clear)(void);
+};
+
 #ifdef CONFIG_SMP
 /*
  * Mapping between linux logical cpu index and hartid.
@@ -40,6 +45,12 @@ void arch_send_call_function_single_ipi(int cpu);
 int riscv_hartid_to_cpuid(int hartid);
 void riscv_cpuid_to_hartid_mask(const struct cpumask *in, struct cpumask *out);
 
+/* Set custom IPI operations */
+void riscv_set_ipi_ops(struct riscv_ipi_ops *ops);
+
+/* Clear IPI for current CPU */
+void riscv_clear_ipi(void);
+
 /*
  * Obtains the hart ID of the currently executing task.  This relies on
  * THREAD_INFO_IN_TASK, but we define that unconditionally.
@@ -78,6 +89,14 @@ static inline void riscv_cpuid_to_hartid_mask(const struct cpumask *in,
 	cpumask_set_cpu(boot_cpu_hartid, out);
 }
 
+static inline void riscv_set_ipi_ops(struct riscv_ipi_ops *ops)
+{
+}
+
+static inline void riscv_clear_ipi(void)
+{
+}
+
 #endif /* CONFIG_SMP */
 
 #if defined(CONFIG_HOTPLUG_CPU) && (CONFIG_SMP)
diff --git a/arch/riscv/kernel/clint.c b/arch/riscv/kernel/clint.c
index 3647980d14c3..a9845ee023e2 100644
--- a/arch/riscv/kernel/clint.c
+++ b/arch/riscv/kernel/clint.c
@@ -5,11 +5,11 @@
 
 #include <linux/io.h>
 #include <linux/of_address.h>
+#include <linux/smp.h>
 #include <linux/types.h>
 #include <asm/clint.h>
 #include <asm/csr.h>
 #include <asm/timex.h>
-#include <asm/smp.h>
 
 /*
  * This is the layout used by the SiFive clint, which is also shared by the qemu
@@ -21,6 +21,24 @@
 
 u32 __iomem *clint_ipi_base;
 
+static void clint_send_ipi(const struct cpumask *target)
+{
+	unsigned int cpu;
+
+	for_each_cpu(cpu, target)
+		writel(1, clint_ipi_base + cpuid_to_hartid_map(cpu));
+}
+
+static void clint_clear_ipi(void)
+{
+	writel(0, clint_ipi_base + cpuid_to_hartid_map(smp_processor_id()));
+}
+
+static struct riscv_ipi_ops clint_ipi_ops = {
+	.ipi_inject = clint_send_ipi,
+	.ipi_clear = clint_clear_ipi,
+};
+
 void clint_init_boot_cpu(void)
 {
 	struct device_node *np;
@@ -40,5 +58,6 @@ void clint_init_boot_cpu(void)
 	riscv_time_cmp = base + CLINT_TIME_CMP_OFF;
 	riscv_time_val = base + CLINT_TIME_VAL_OFF;
 
-	clint_clear_ipi(boot_cpu_hartid);
+	clint_clear_ipi();
+	riscv_set_ipi_ops(&clint_ipi_ops);
 }
diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
index f383ef5672b2..226ccce0f9e0 100644
--- a/arch/riscv/kernel/sbi.c
+++ b/arch/riscv/kernel/sbi.c
@@ -547,6 +547,18 @@ static inline long sbi_get_firmware_version(void)
 	return __sbi_base_ecall(SBI_EXT_BASE_GET_IMP_VERSION);
 }
 
+static void sbi_send_cpumask_ipi(const struct cpumask *target)
+{
+	struct cpumask hartid_mask;
+
+	riscv_cpuid_to_hartid_mask(target, &hartid_mask);
+
+	sbi_send_ipi(cpumask_bits(&hartid_mask));
+}
+
+static struct riscv_ipi_ops sbi_ipi_ops = {
+	.ipi_inject = sbi_send_cpumask_ipi
+};
 
 int __init sbi_init(void)
 {
@@ -587,5 +599,7 @@ int __init sbi_init(void)
 		__sbi_rfence	= __sbi_rfence_v01;
 	}
 
+	riscv_set_ipi_ops(&sbi_ipi_ops);
+
 	return 0;
 }
diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
index b1d4f452f843..8b85683ce203 100644
--- a/arch/riscv/kernel/smp.c
+++ b/arch/riscv/kernel/smp.c
@@ -84,9 +84,25 @@ static void ipi_stop(void)
 		wait_for_interrupt();
 }
 
+static struct riscv_ipi_ops *ipi_ops;
+
+void riscv_set_ipi_ops(struct riscv_ipi_ops *ops)
+{
+	ipi_ops = ops;
+}
+EXPORT_SYMBOL_GPL(riscv_set_ipi_ops);
+
+void riscv_clear_ipi(void)
+{
+	if (ipi_ops && ipi_ops->ipi_clear)
+		ipi_ops->ipi_clear();
+
+	csr_clear(CSR_IP, IE_SIE);
+}
+EXPORT_SYMBOL_GPL(riscv_clear_ipi);
+
 static void send_ipi_mask(const struct cpumask *mask, enum ipi_message_type op)
 {
-	struct cpumask hartid_mask;
 	int cpu;
 
 	smp_mb__before_atomic();
@@ -94,33 +110,22 @@ static void send_ipi_mask(const struct cpumask *mask, enum ipi_message_type op)
 		set_bit(op, &ipi_data[cpu].bits);
 	smp_mb__after_atomic();
 
-	riscv_cpuid_to_hartid_mask(mask, &hartid_mask);
-	if (IS_ENABLED(CONFIG_RISCV_SBI))
-		sbi_send_ipi(cpumask_bits(&hartid_mask));
+	if (ipi_ops && ipi_ops->ipi_inject)
+		ipi_ops->ipi_inject(mask);
 	else
-		clint_send_ipi_mask(mask);
+		pr_warn("SMP: IPI inject method not available\n");
 }
 
 static void send_ipi_single(int cpu, enum ipi_message_type op)
 {
-	int hartid = cpuid_to_hartid_map(cpu);
-
 	smp_mb__before_atomic();
 	set_bit(op, &ipi_data[cpu].bits);
 	smp_mb__after_atomic();
 
-	if (IS_ENABLED(CONFIG_RISCV_SBI))
-		sbi_send_ipi(cpumask_bits(cpumask_of(hartid)));
-	else
-		clint_send_ipi_single(hartid);
-}
-
-static inline void clear_ipi(void)
-{
-	if (IS_ENABLED(CONFIG_RISCV_SBI))
-		csr_clear(CSR_IP, IE_SIE);
+	if (ipi_ops && ipi_ops->ipi_inject)
+		ipi_ops->ipi_inject(cpumask_of(cpu));
 	else
-		clint_clear_ipi(cpuid_to_hartid_map(smp_processor_id()));
+		pr_warn("SMP: IPI inject method not available\n");
 }
 
 void handle_IPI(struct pt_regs *regs)
@@ -131,7 +136,7 @@ void handle_IPI(struct pt_regs *regs)
 
 	irq_enter();
 
-	clear_ipi();
+	riscv_clear_ipi();
 
 	while (true) {
 		unsigned long ops;
diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
index 4e9922790f6e..5fe849791bf0 100644
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -147,8 +147,7 @@ asmlinkage __visible void smp_callin(void)
 {
 	struct mm_struct *mm = &init_mm;
 
-	if (!IS_ENABLED(CONFIG_RISCV_SBI))
-		clint_clear_ipi(cpuid_to_hartid_map(smp_processor_id()));
+	riscv_clear_ipi();
 
 	/* All kernel threads share the same mm context.  */
 	mmgrab(mm);
-- 
2.25.1


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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v4 2/4] clocksource/drivers: Add CLINT timer driver
  2020-07-17  7:50 [PATCH v4 0/4] Dedicated CLINT timer driver Anup Patel
  2020-07-17  7:50 ` [PATCH v4 1/4] RISC-V: Add mechanism to provide custom IPI operations Anup Patel
@ 2020-07-17  7:50 ` Anup Patel
  2020-07-21  1:11   ` Atish Patra
  2020-07-21 11:02   ` Daniel Lezcano
  2020-07-17  7:51 ` [PATCH v4 3/4] RISC-V: Remove CLINT related code from timer and arch Anup Patel
  2020-07-17  7:51 ` [PATCH v4 4/4] dt-bindings: timer: Add CLINT bindings Anup Patel
  3 siblings, 2 replies; 19+ messages in thread
From: Anup Patel @ 2020-07-17  7:50 UTC (permalink / raw)
  To: Palmer Dabbelt, Paul Walmsley, Albert Ou, Rob Herring,
	Daniel Lezcano, Thomas Gleixner
  Cc: devicetree, Damien Le Moal, Emil Renner Berhing, Anup Patel,
	Anup Patel, linux-kernel, Atish Patra, Alistair Francis,
	linux-riscv

We add a separate CLINT timer driver for Linux RISC-V M-mode (i.e.
RISC-V NoMMU kernel).

The CLINT MMIO device provides three things:
1. 64bit free running counter register
2. 64bit per-CPU time compare registers
3. 32bit per-CPU inter-processor interrupt registers

Unlike other timer devices, CLINT provides IPI registers along with
timer registers. To use CLINT IPI registers, the CLINT timer driver
provides IPI related callbacks to arch/riscv.

Signed-off-by: Anup Patel <anup.patel@wdc.com>
Tested-by: Emil Renner Berhing <kernel@esmil.dk>
---
 drivers/clocksource/Kconfig       |   9 ++
 drivers/clocksource/Makefile      |   1 +
 drivers/clocksource/timer-clint.c | 231 ++++++++++++++++++++++++++++++
 include/linux/cpuhotplug.h        |   1 +
 4 files changed, 242 insertions(+)
 create mode 100644 drivers/clocksource/timer-clint.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 91418381fcd4..e1ce0d510a03 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -658,6 +658,15 @@ config RISCV_TIMER
 	  is accessed via both the SBI and the rdcycle instruction.  This is
 	  required for all RISC-V systems.
 
+config CLINT_TIMER
+	bool "Timer for the RISC-V platform"
+	depends on GENERIC_SCHED_CLOCK && RISCV_M_MODE
+	select TIMER_PROBE
+	select TIMER_OF
+	help
+	  This option enables the CLINT timer for RISC-V systems. The CLINT
+	  driver is usually used for NoMMU RISC-V systems.
+
 config CSKY_MP_TIMER
 	bool "SMP Timer for the C-SKY platform" if COMPILE_TEST
 	depends on CSKY
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index bdda1a2e4097..18e700e703a0 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -87,6 +87,7 @@ obj-$(CONFIG_CLKSRC_ST_LPC)		+= clksrc_st_lpc.o
 obj-$(CONFIG_X86_NUMACHIP)		+= numachip.o
 obj-$(CONFIG_ATCPIT100_TIMER)		+= timer-atcpit100.o
 obj-$(CONFIG_RISCV_TIMER)		+= timer-riscv.o
+obj-$(CONFIG_CLINT_TIMER)		+= timer-clint.o
 obj-$(CONFIG_CSKY_MP_TIMER)		+= timer-mp-csky.o
 obj-$(CONFIG_GX6605S_TIMER)		+= timer-gx6605s.o
 obj-$(CONFIG_HYPERV_TIMER)		+= hyperv_timer.o
diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer-clint.c
new file mode 100644
index 000000000000..e1698efa73a1
--- /dev/null
+++ b/drivers/clocksource/timer-clint.c
@@ -0,0 +1,231 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 Western Digital Corporation or its affiliates.
+ *
+ * Most of the M-mode (i.e. NoMMU) RISC-V systems usually have a
+ * CLINT MMIO timer device.
+ */
+
+#define pr_fmt(fmt) "clint: " fmt
+#include <linux/bitops.h>
+#include <linux/clocksource.h>
+#include <linux/clockchips.h>
+#include <linux/cpu.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/sched_clock.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/interrupt.h>
+#include <linux/of_irq.h>
+#include <linux/smp.h>
+
+#define CLINT_IPI_OFF		0
+#define CLINT_TIMER_CMP_OFF	0x4000
+#define CLINT_TIMER_VAL_OFF	0xbff8
+
+/* CLINT manages IPI and Timer for RISC-V M-mode  */
+static u32 __iomem *clint_ipi_base;
+static u64 __iomem *clint_timer_cmp;
+static u64 __iomem *clint_timer_val;
+static unsigned long clint_timer_freq;
+static unsigned int clint_timer_irq;
+
+static void clint_send_ipi(const struct cpumask *target)
+{
+	unsigned int cpu;
+
+	for_each_cpu(cpu, target)
+		writel(1, clint_ipi_base + cpuid_to_hartid_map(cpu));
+}
+
+static void clint_clear_ipi(void)
+{
+	writel(0, clint_ipi_base + cpuid_to_hartid_map(smp_processor_id()));
+}
+
+static struct riscv_ipi_ops clint_ipi_ops = {
+	.ipi_inject = clint_send_ipi,
+	.ipi_clear = clint_clear_ipi,
+};
+
+#ifdef CONFIG_64BIT
+#define clint_get_cycles()	readq_relaxed(clint_timer_val)
+#else
+#define clint_get_cycles()	readl_relaxed(clint_timer_val)
+#define clint_get_cycles_hi()	readl_relaxed(((u32 *)clint_timer_val) + 1)
+#endif
+
+#ifdef CONFIG_64BIT
+static u64 notrace clint_get_cycles64(void)
+{
+	return clint_get_cycles();
+}
+#else /* CONFIG_64BIT */
+static u64 notrace clint_get_cycles64(void)
+{
+	u32 hi, lo;
+
+	do {
+		hi = clint_get_cycles_hi();
+		lo = clint_get_cycles();
+	} while (hi != clint_get_cycles_hi());
+
+	return ((u64)hi << 32) | lo;
+}
+#endif /* CONFIG_64BIT */
+
+static u64 clint_rdtime(struct clocksource *cs)
+{
+	return clint_get_cycles64();
+}
+
+static struct clocksource clint_clocksource = {
+	.name		= "clint_clocksource",
+	.rating	= 300,
+	.mask		= CLOCKSOURCE_MASK(64),
+	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
+	.read		= clint_rdtime,
+};
+
+static int clint_clock_next_event(unsigned long delta,
+				   struct clock_event_device *ce)
+{
+	void __iomem *r = clint_timer_cmp +
+			  cpuid_to_hartid_map(smp_processor_id());
+
+	csr_set(CSR_IE, IE_TIE);
+	writeq_relaxed(clint_get_cycles64() + delta, r);
+	return 0;
+}
+
+static DEFINE_PER_CPU(struct clock_event_device, clint_clock_event) = {
+	.name			= "clint_clockevent",
+	.features		= CLOCK_EVT_FEAT_ONESHOT,
+	.rating		= 100,
+	.set_next_event	= clint_clock_next_event,
+};
+
+static DEFINE_PER_CPU(bool, clint_clock_event_registered);
+
+static int clint_timer_starting_cpu(unsigned int cpu)
+{
+	bool *registered = per_cpu_ptr(&clint_clock_event_registered, cpu);
+	struct clock_event_device *ce = per_cpu_ptr(&clint_clock_event, cpu);
+
+	if (!(*registered)) {
+		ce->cpumask = cpumask_of(cpu);
+		clockevents_config_and_register(ce, clint_timer_freq, 200,
+						 ULONG_MAX);
+		*registered = true;
+	}
+
+	enable_percpu_irq(clint_timer_irq,
+			  irq_get_trigger_type(clint_timer_irq));
+	return 0;
+}
+
+static int clint_timer_dying_cpu(unsigned int cpu)
+{
+	disable_percpu_irq(clint_timer_irq);
+	return 0;
+}
+
+static irqreturn_t clint_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evdev = this_cpu_ptr(&clint_clock_event);
+
+	csr_clear(CSR_IE, IE_TIE);
+	evdev->event_handler(evdev);
+
+	return IRQ_HANDLED;
+}
+
+static int __init clint_timer_init_dt(struct device_node *np)
+{
+	int rc;
+	u32 i, nr_irqs;
+	void __iomem *base;
+	struct of_phandle_args oirq;
+
+	/*
+	 * Ensure that CLINT device interrupts are either RV_IRQ_TIMER or
+	 * RV_IRQ_SOFT. If it's anything else then we ignore the device.
+	 */
+	nr_irqs = of_irq_count(np);
+	for (i = 0; i < nr_irqs; i++) {
+		if (of_irq_parse_one(np, i, &oirq)) {
+			pr_err("%pOFP: failed to parse irq %d.\n", np, i);
+			continue;
+		}
+
+		if ((oirq.args_count != 1) ||
+		    (oirq.args[0] != RV_IRQ_TIMER &&
+		     oirq.args[0] != RV_IRQ_SOFT)) {
+			pr_err("%pOFP: invalid irq %d (hwirq %d)\n",
+			       np, i, oirq.args[0]);
+			return -ENODEV;
+		}
+
+		/* Find parent irq domain and map timer irq */
+		if (!clint_timer_irq &&
+		    oirq.args[0] == RV_IRQ_TIMER &&
+		    irq_find_host(oirq.np))
+			clint_timer_irq = irq_of_parse_and_map(np, i);
+	}
+
+	/* If CLINT timer irq not found then fail */
+	if (!clint_timer_irq) {
+		pr_err("%pOFP: timer irq not found\n", np);
+		return -ENODEV;
+	}
+
+	base = of_iomap(np, 0);
+	if (!base) {
+		pr_err("%pOFP: could not map registers\n", np);
+		return -ENODEV;
+	}
+
+	clint_ipi_base = base + CLINT_IPI_OFF;
+	clint_timer_cmp = base + CLINT_TIMER_CMP_OFF;
+	clint_timer_val = base + CLINT_TIMER_VAL_OFF;
+	clint_timer_freq = riscv_timebase;
+
+	pr_info("%pOFP: timer running at %ld Hz\n", np, clint_timer_freq);
+
+	rc = clocksource_register_hz(&clint_clocksource, clint_timer_freq);
+	if (rc) {
+		iounmap(base);
+		pr_err("%pOFP: clocksource register failed [%d]\n", np, rc);
+		return rc;
+	}
+
+	sched_clock_register(clint_get_cycles64, 64, clint_timer_freq);
+
+	rc = request_percpu_irq(clint_timer_irq, clint_timer_interrupt,
+				 "clint-timer", &clint_clock_event);
+	if (rc) {
+		iounmap(base);
+		pr_err("registering percpu irq failed [%d]\n", rc);
+		return rc;
+	}
+
+	rc = cpuhp_setup_state(CPUHP_AP_CLINT_TIMER_STARTING,
+				"clockevents/clint/timer:starting",
+				clint_timer_starting_cpu,
+				clint_timer_dying_cpu);
+	if (rc) {
+		free_irq(clint_timer_irq, &clint_clock_event);
+		iounmap(base);
+		pr_err("%pOFP: cpuhp setup state failed [%d]\n", np, rc);
+		return rc;
+	}
+
+	riscv_set_ipi_ops(&clint_ipi_ops);
+	clint_clear_ipi();
+
+	return 0;
+}
+
+TIMER_OF_DECLARE(clint_timer, "riscv,clint0", clint_timer_init_dt);
+TIMER_OF_DECLARE(clint_timer1, "sifive,clint0", clint_timer_init_dt);
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 191772d4a4d7..1451f4625833 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -132,6 +132,7 @@ enum cpuhp_state {
 	CPUHP_AP_MIPS_GIC_TIMER_STARTING,
 	CPUHP_AP_ARC_TIMER_STARTING,
 	CPUHP_AP_RISCV_TIMER_STARTING,
+	CPUHP_AP_CLINT_TIMER_STARTING,
 	CPUHP_AP_CSKY_TIMER_STARTING,
 	CPUHP_AP_HYPERV_TIMER_STARTING,
 	CPUHP_AP_KVM_STARTING,
-- 
2.25.1


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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v4 3/4] RISC-V: Remove CLINT related code from timer and arch
  2020-07-17  7:50 [PATCH v4 0/4] Dedicated CLINT timer driver Anup Patel
  2020-07-17  7:50 ` [PATCH v4 1/4] RISC-V: Add mechanism to provide custom IPI operations Anup Patel
  2020-07-17  7:50 ` [PATCH v4 2/4] clocksource/drivers: Add CLINT timer driver Anup Patel
@ 2020-07-17  7:51 ` Anup Patel
  2020-07-17  7:51 ` [PATCH v4 4/4] dt-bindings: timer: Add CLINT bindings Anup Patel
  3 siblings, 0 replies; 19+ messages in thread
From: Anup Patel @ 2020-07-17  7:51 UTC (permalink / raw)
  To: Palmer Dabbelt, Paul Walmsley, Albert Ou, Rob Herring,
	Daniel Lezcano, Thomas Gleixner
  Cc: devicetree, Damien Le Moal, Emil Renner Berhing, Anup Patel,
	Anup Patel, linux-kernel, Atish Patra, Alistair Francis,
	linux-riscv

Right now the RISC-V timer driver is convoluted to support:
1. Linux RISC-V S-mode (with MMU) where it will use TIME CSR for
   clocksource and SBI timer calls for clockevent device.
2. Linux RISC-V M-mode (without MMU) where it will use CLINT MMIO
   counter register for clocksource and CLINT MMIO compare register
   for clockevent device.

We now have a separate CLINT timer driver which also provide CLINT
based IPI operations so let's remove CLINT MMIO related code from
arch/riscv directory and RISC-V timer driver.

Signed-off-by: Anup Patel <anup.patel@wdc.com>
Tested-by: Emil Renner Berhing <kernel@esmil.dk>
---
 arch/riscv/Kconfig                      |  2 +-
 arch/riscv/Kconfig.socs                 |  2 +
 arch/riscv/configs/nommu_virt_defconfig |  7 +--
 arch/riscv/include/asm/clint.h          | 14 ------
 arch/riscv/include/asm/timex.h          | 28 +++--------
 arch/riscv/kernel/Makefile              |  2 +-
 arch/riscv/kernel/clint.c               | 63 -------------------------
 arch/riscv/kernel/setup.c               |  2 -
 arch/riscv/kernel/smp.c                 |  1 -
 arch/riscv/kernel/smpboot.c             |  1 -
 drivers/clocksource/Kconfig             |  2 +-
 drivers/clocksource/timer-riscv.c       | 17 +------
 12 files changed, 16 insertions(+), 125 deletions(-)
 delete mode 100644 arch/riscv/include/asm/clint.h
 delete mode 100644 arch/riscv/kernel/clint.c

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index fedb4a72b29a..57a72ae23d10 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -74,7 +74,7 @@ config RISCV
 	select PCI_DOMAINS_GENERIC if PCI
 	select PCI_MSI if PCI
 	select RISCV_INTC
-	select RISCV_TIMER
+	select RISCV_TIMER if RISCV_SBI
 	select SPARSEMEM_STATIC if 32BIT
 	select SPARSE_IRQ
 	select SYSCTL_EXCEPTION_TRACE
diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
index 6c88148f1b9b..8a55f6156661 100644
--- a/arch/riscv/Kconfig.socs
+++ b/arch/riscv/Kconfig.socs
@@ -12,6 +12,7 @@ config SOC_SIFIVE
 
 config SOC_VIRT
 	bool "QEMU Virt Machine"
+	select CLINT_TIMER if RISCV_M_MODE
 	select POWER_RESET
 	select POWER_RESET_SYSCON
 	select POWER_RESET_SYSCON_POWEROFF
@@ -24,6 +25,7 @@ config SOC_VIRT
 config SOC_KENDRYTE
 	bool "Kendryte K210 SoC"
 	depends on !MMU
+	select CLINT_TIMER if RISCV_M_MODE
 	select SERIAL_SIFIVE if TTY
 	select SERIAL_SIFIVE_CONSOLE if TTY
 	select SIFIVE_PLIC
diff --git a/arch/riscv/configs/nommu_virt_defconfig b/arch/riscv/configs/nommu_virt_defconfig
index cf74e179bf90..cf9388184aa3 100644
--- a/arch/riscv/configs/nommu_virt_defconfig
+++ b/arch/riscv/configs/nommu_virt_defconfig
@@ -26,6 +26,7 @@ CONFIG_EXPERT=y
 CONFIG_SLOB=y
 # CONFIG_SLAB_MERGE_DEFAULT is not set
 # CONFIG_MMU is not set
+CONFIG_SOC_VIRT=y
 CONFIG_MAXPHYSMEM_2GB=y
 CONFIG_SMP=y
 CONFIG_CMDLINE="root=/dev/vda rw earlycon=uart8250,mmio,0x10000000,115200n8 console=ttyS0"
@@ -48,7 +49,6 @@ CONFIG_VIRTIO_BLK=y
 # CONFIG_SERIO is not set
 # CONFIG_LEGACY_PTYS is not set
 # CONFIG_LDISC_AUTOLOAD is not set
-# CONFIG_DEVMEM is not set
 CONFIG_SERIAL_8250=y
 # CONFIG_SERIAL_8250_DEPRECATED_OPTIONS is not set
 CONFIG_SERIAL_8250_CONSOLE=y
@@ -56,16 +56,13 @@ CONFIG_SERIAL_8250_NR_UARTS=1
 CONFIG_SERIAL_8250_RUNTIME_UARTS=1
 CONFIG_SERIAL_OF_PLATFORM=y
 # CONFIG_HW_RANDOM is not set
+# CONFIG_DEVMEM is not set
 # CONFIG_HWMON is not set
-# CONFIG_LCD_CLASS_DEVICE is not set
-# CONFIG_BACKLIGHT_CLASS_DEVICE is not set
 # CONFIG_VGA_CONSOLE is not set
 # CONFIG_HID is not set
 # CONFIG_USB_SUPPORT is not set
 CONFIG_VIRTIO_MMIO=y
 CONFIG_VIRTIO_MMIO_CMDLINE_DEVICES=y
-CONFIG_SIFIVE_PLIC=y
-# CONFIG_VALIDATE_FS_PARSER is not set
 CONFIG_EXT2_FS=y
 # CONFIG_DNOTIFY is not set
 # CONFIG_INOTIFY_USER is not set
diff --git a/arch/riscv/include/asm/clint.h b/arch/riscv/include/asm/clint.h
deleted file mode 100644
index adaba98a7d6c..000000000000
--- a/arch/riscv/include/asm/clint.h
+++ /dev/null
@@ -1,14 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef _ASM_RISCV_CLINT_H
-#define _ASM_RISCV_CLINT_H 1
-
-#include <linux/io.h>
-#include <linux/smp.h>
-
-#ifdef CONFIG_RISCV_M_MODE
-void clint_init_boot_cpu(void);
-#else /* CONFIG_RISCV_M_MODE */
-#define clint_init_boot_cpu()	do { } while (0)
-#endif /* CONFIG_RISCV_M_MODE */
-
-#endif /* _ASM_RISCV_CLINT_H */
diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h
index bad2a7c2cda5..a3fb85d505d4 100644
--- a/arch/riscv/include/asm/timex.h
+++ b/arch/riscv/include/asm/timex.h
@@ -7,41 +7,27 @@
 #define _ASM_RISCV_TIMEX_H
 
 #include <asm/csr.h>
-#include <asm/mmio.h>
 
 typedef unsigned long cycles_t;
 
-extern u64 __iomem *riscv_time_val;
-extern u64 __iomem *riscv_time_cmp;
-
-#ifdef CONFIG_64BIT
-#define mmio_get_cycles()	readq_relaxed(riscv_time_val)
-#else
-#define mmio_get_cycles()	readl_relaxed(riscv_time_val)
-#define mmio_get_cycles_hi()	readl_relaxed(((u32 *)riscv_time_val) + 1)
-#endif
-
 static inline cycles_t get_cycles(void)
 {
-	if (IS_ENABLED(CONFIG_RISCV_SBI))
-		return csr_read(CSR_TIME);
-	return mmio_get_cycles();
+	return csr_read(CSR_TIME);
 }
 #define get_cycles get_cycles
 
+static inline u32 get_cycles_hi(void)
+{
+	return csr_read(CSR_TIMEH);
+}
+#define get_cycles_hi get_cycles_hi
+
 #ifdef CONFIG_64BIT
 static inline u64 get_cycles64(void)
 {
 	return get_cycles();
 }
 #else /* CONFIG_64BIT */
-static inline u32 get_cycles_hi(void)
-{
-	if (IS_ENABLED(CONFIG_RISCV_SBI))
-		return csr_read(CSR_TIMEH);
-	return mmio_get_cycles_hi();
-}
-
 static inline u64 get_cycles64(void)
 {
 	u32 hi, lo;
diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
index b355cf485671..7edf15643146 100644
--- a/arch/riscv/kernel/Makefile
+++ b/arch/riscv/kernel/Makefile
@@ -31,7 +31,7 @@ obj-y	+= cacheinfo.o
 obj-y	+= patch.o
 obj-$(CONFIG_MMU) += vdso.o vdso/
 
-obj-$(CONFIG_RISCV_M_MODE)	+= clint.o traps_misaligned.o
+obj-$(CONFIG_RISCV_M_MODE)	+= traps_misaligned.o
 obj-$(CONFIG_FPU)		+= fpu.o
 obj-$(CONFIG_SMP)		+= smpboot.o
 obj-$(CONFIG_SMP)		+= smp.o
diff --git a/arch/riscv/kernel/clint.c b/arch/riscv/kernel/clint.c
deleted file mode 100644
index a9845ee023e2..000000000000
--- a/arch/riscv/kernel/clint.c
+++ /dev/null
@@ -1,63 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Copyright (c) 2019 Christoph Hellwig.
- */
-
-#include <linux/io.h>
-#include <linux/of_address.h>
-#include <linux/smp.h>
-#include <linux/types.h>
-#include <asm/clint.h>
-#include <asm/csr.h>
-#include <asm/timex.h>
-
-/*
- * This is the layout used by the SiFive clint, which is also shared by the qemu
- * virt platform, and the Kendryte KD210 at least.
- */
-#define CLINT_IPI_OFF		0
-#define CLINT_TIME_CMP_OFF	0x4000
-#define CLINT_TIME_VAL_OFF	0xbff8
-
-u32 __iomem *clint_ipi_base;
-
-static void clint_send_ipi(const struct cpumask *target)
-{
-	unsigned int cpu;
-
-	for_each_cpu(cpu, target)
-		writel(1, clint_ipi_base + cpuid_to_hartid_map(cpu));
-}
-
-static void clint_clear_ipi(void)
-{
-	writel(0, clint_ipi_base + cpuid_to_hartid_map(smp_processor_id()));
-}
-
-static struct riscv_ipi_ops clint_ipi_ops = {
-	.ipi_inject = clint_send_ipi,
-	.ipi_clear = clint_clear_ipi,
-};
-
-void clint_init_boot_cpu(void)
-{
-	struct device_node *np;
-	void __iomem *base;
-
-	np = of_find_compatible_node(NULL, NULL, "riscv,clint0");
-	if (!np) {
-		panic("clint not found");
-		return;
-	}
-
-	base = of_iomap(np, 0);
-	if (!base)
-		panic("could not map CLINT");
-
-	clint_ipi_base = base + CLINT_IPI_OFF;
-	riscv_time_cmp = base + CLINT_TIME_CMP_OFF;
-	riscv_time_val = base + CLINT_TIME_VAL_OFF;
-
-	clint_clear_ipi();
-	riscv_set_ipi_ops(&clint_ipi_ops);
-}
diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index f04373be54a6..2c6dd329312b 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -18,7 +18,6 @@
 #include <linux/swiotlb.h>
 #include <linux/smp.h>
 
-#include <asm/clint.h>
 #include <asm/cpu_ops.h>
 #include <asm/setup.h>
 #include <asm/sections.h>
@@ -79,7 +78,6 @@ void __init setup_arch(char **cmdline_p)
 #else
 	unflatten_device_tree();
 #endif
-	clint_init_boot_cpu();
 
 #ifdef CONFIG_SWIOTLB
 	swiotlb_init(1);
diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
index 8b85683ce203..07626be78c23 100644
--- a/arch/riscv/kernel/smp.c
+++ b/arch/riscv/kernel/smp.c
@@ -17,7 +17,6 @@
 #include <linux/seq_file.h>
 #include <linux/delay.h>
 
-#include <asm/clint.h>
 #include <asm/sbi.h>
 #include <asm/tlbflush.h>
 #include <asm/cacheflush.h>
diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
index 5fe849791bf0..a6cfa9842d4b 100644
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -24,7 +24,6 @@
 #include <linux/of.h>
 #include <linux/sched/task_stack.h>
 #include <linux/sched/mm.h>
-#include <asm/clint.h>
 #include <asm/cpu_ops.h>
 #include <asm/irq.h>
 #include <asm/mmu_context.h>
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index e1ce0d510a03..2e7c4b8387ae 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -649,7 +649,7 @@ config ATCPIT100_TIMER
 
 config RISCV_TIMER
 	bool "Timer for the RISC-V platform"
-	depends on GENERIC_SCHED_CLOCK && RISCV
+	depends on GENERIC_SCHED_CLOCK && RISCV_SBI
 	default y
 	select TIMER_PROBE
 	select TIMER_OF
diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
index 9de1dabfb126..c51c5ed15aa7 100644
--- a/drivers/clocksource/timer-riscv.c
+++ b/drivers/clocksource/timer-riscv.c
@@ -19,26 +19,13 @@
 #include <linux/of_irq.h>
 #include <asm/smp.h>
 #include <asm/sbi.h>
-
-u64 __iomem *riscv_time_cmp;
-u64 __iomem *riscv_time_val;
-
-static inline void mmio_set_timer(u64 val)
-{
-	void __iomem *r;
-
-	r = riscv_time_cmp + cpuid_to_hartid_map(smp_processor_id());
-	writeq_relaxed(val, r);
-}
+#include <asm/timex.h>
 
 static int riscv_clock_next_event(unsigned long delta,
 		struct clock_event_device *ce)
 {
 	csr_set(CSR_IE, IE_TIE);
-	if (IS_ENABLED(CONFIG_RISCV_SBI))
-		sbi_set_timer(get_cycles64() + delta);
-	else
-		mmio_set_timer(get_cycles64() + delta);
+	sbi_set_timer(get_cycles64() + delta);
 	return 0;
 }
 
-- 
2.25.1


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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v4 4/4] dt-bindings: timer: Add CLINT bindings
  2020-07-17  7:50 [PATCH v4 0/4] Dedicated CLINT timer driver Anup Patel
                   ` (2 preceding siblings ...)
  2020-07-17  7:51 ` [PATCH v4 3/4] RISC-V: Remove CLINT related code from timer and arch Anup Patel
@ 2020-07-17  7:51 ` Anup Patel
  2020-07-21  1:15   ` Atish Patra
  3 siblings, 1 reply; 19+ messages in thread
From: Anup Patel @ 2020-07-17  7:51 UTC (permalink / raw)
  To: Palmer Dabbelt, Paul Walmsley, Albert Ou, Rob Herring,
	Daniel Lezcano, Thomas Gleixner
  Cc: devicetree, Damien Le Moal, Palmer Dabbelt, Emil Renner Berhing,
	Anup Patel, Anup Patel, linux-kernel, Atish Patra,
	Alistair Francis, linux-riscv

We add DT bindings documentation for CLINT device.

Signed-off-by: Anup Patel <anup.patel@wdc.com>
Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
Tested-by: Emil Renner Berhing <kernel@esmil.dk>
---
 .../bindings/timer/sifive,clint.yaml          | 58 +++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/sifive,clint.yaml

diff --git a/Documentation/devicetree/bindings/timer/sifive,clint.yaml b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
new file mode 100644
index 000000000000..8ad115611860
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/timer/sifive,clint.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SiFive Core Local Interruptor
+
+maintainers:
+  - Palmer Dabbelt <palmer@dabbelt.com>
+  - Anup Patel <anup.patel@wdc.com>
+
+description:
+  SiFive (and other RISC-V) SOCs include an implementation of the SiFive
+  Core Local Interruptor (CLINT) for M-mode timer and M-mode inter-processor
+  interrupts. It directly connects to the timer and inter-processor interrupt
+  lines of various HARTs (or CPUs) so RISC-V per-HART (or per-CPU) local
+  interrupt controller is the parent interrupt controller for CLINT device.
+  The clock frequency of CLINT is specified via "timebase-frequency" DT
+  property of "/cpus" DT node. The "timebase-frequency" DT property is
+  described in Documentation/devicetree/bindings/riscv/cpus.yaml
+
+properties:
+  compatible:
+    items:
+      - const: sifive,clint0
+      - const: sifive,fu540-c000-clint
+
+    description:
+      Should be "sifive,<chip>-clint" and "sifive,clint<version>".
+      Supported compatible strings are -
+      "sifive,fu540-c000-clint" for the SiFive CLINT v0 as integrated
+      onto the SiFive FU540 chip, and "sifive,clint0" for the SiFive
+      CLINT v0 IP block with no chip integration tweaks.
+      Please refer to sifive-blocks-ip-versioning.txt for details
+
+  reg:
+    maxItems: 1
+
+  interrupts-extended:
+    minItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts-extended
+
+examples:
+  - |
+    clint@2000000 {
+      compatible = "sifive,clint0", "sifive,fu540-c000-clint";
+      interrupts-extended = <&cpu1intc 3 &cpu1intc 7
+                             &cpu2intc 3 &cpu2intc 7
+                             &cpu3intc 3 &cpu3intc 7
+                             &cpu4intc 3 &cpu4intc 7>;
+       reg = <0x2000000 0x4000000>;
+    };
+...
-- 
2.25.1


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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 1/4] RISC-V: Add mechanism to provide custom IPI operations
  2020-07-17  7:50 ` [PATCH v4 1/4] RISC-V: Add mechanism to provide custom IPI operations Anup Patel
@ 2020-07-21  0:46   ` Atish Patra
  0 siblings, 0 replies; 19+ messages in thread
From: Atish Patra @ 2020-07-21  0:46 UTC (permalink / raw)
  To: Anup Patel
  Cc: devicetree, Damien Le Moal, Albert Ou, Emil Renner Berhing,
	Anup Patel, Daniel Lezcano, linux-kernel@vger.kernel.org List,
	Atish Patra, Rob Herring, Palmer Dabbelt, Paul Walmsley,
	Alistair Francis, Thomas Gleixner, linux-riscv

On Fri, Jul 17, 2020 at 12:52 AM Anup Patel <anup.patel@wdc.com> wrote:
>
> We add mechanism to set custom IPI operations so that CLINT driver
> from drivers directory can provide custom IPI operations.
>
> Signed-off-by: Anup Patel <anup.patel@wdc.com>
> Tested-by: Emil Renner Berhing <kernel@esmil.dk>
> ---
>  arch/riscv/include/asm/clint.h | 25 --------------------
>  arch/riscv/include/asm/smp.h   | 19 +++++++++++++++
>  arch/riscv/kernel/clint.c      | 23 ++++++++++++++++--
>  arch/riscv/kernel/sbi.c        | 14 +++++++++++
>  arch/riscv/kernel/smp.c        | 43 +++++++++++++++++++---------------
>  arch/riscv/kernel/smpboot.c    |  3 +--
>  6 files changed, 79 insertions(+), 48 deletions(-)
>
> diff --git a/arch/riscv/include/asm/clint.h b/arch/riscv/include/asm/clint.h
> index a279b17a6aad..adaba98a7d6c 100644
> --- a/arch/riscv/include/asm/clint.h
> +++ b/arch/riscv/include/asm/clint.h
> @@ -6,34 +6,9 @@
>  #include <linux/smp.h>
>
>  #ifdef CONFIG_RISCV_M_MODE
> -extern u32 __iomem *clint_ipi_base;
> -
>  void clint_init_boot_cpu(void);
> -
> -static inline void clint_send_ipi_single(unsigned long hartid)
> -{
> -       writel(1, clint_ipi_base + hartid);
> -}
> -
> -static inline void clint_send_ipi_mask(const struct cpumask *mask)
> -{
> -       int cpu;
> -
> -       for_each_cpu(cpu, mask)
> -               clint_send_ipi_single(cpuid_to_hartid_map(cpu));
> -}
> -
> -static inline void clint_clear_ipi(unsigned long hartid)
> -{
> -       writel(0, clint_ipi_base + hartid);
> -}
>  #else /* CONFIG_RISCV_M_MODE */
>  #define clint_init_boot_cpu()  do { } while (0)
> -
> -/* stubs to for code is only reachable under IS_ENABLED(CONFIG_RISCV_M_MODE): */
> -void clint_send_ipi_single(unsigned long hartid);
> -void clint_send_ipi_mask(const struct cpumask *hartid_mask);
> -void clint_clear_ipi(unsigned long hartid);
>  #endif /* CONFIG_RISCV_M_MODE */
>
>  #endif /* _ASM_RISCV_CLINT_H */
> diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
> index 40bb1c15a731..68de78a8eba6 100644
> --- a/arch/riscv/include/asm/smp.h
> +++ b/arch/riscv/include/asm/smp.h
> @@ -15,6 +15,11 @@
>  struct seq_file;
>  extern unsigned long boot_cpu_hartid;
>
> +struct riscv_ipi_ops {
> +       void (*ipi_inject)(const struct cpumask *target);
> +       void (*ipi_clear)(void);
> +};
> +
>  #ifdef CONFIG_SMP
>  /*
>   * Mapping between linux logical cpu index and hartid.
> @@ -40,6 +45,12 @@ void arch_send_call_function_single_ipi(int cpu);
>  int riscv_hartid_to_cpuid(int hartid);
>  void riscv_cpuid_to_hartid_mask(const struct cpumask *in, struct cpumask *out);
>
> +/* Set custom IPI operations */
> +void riscv_set_ipi_ops(struct riscv_ipi_ops *ops);
> +
> +/* Clear IPI for current CPU */
> +void riscv_clear_ipi(void);
> +
>  /*
>   * Obtains the hart ID of the currently executing task.  This relies on
>   * THREAD_INFO_IN_TASK, but we define that unconditionally.
> @@ -78,6 +89,14 @@ static inline void riscv_cpuid_to_hartid_mask(const struct cpumask *in,
>         cpumask_set_cpu(boot_cpu_hartid, out);
>  }
>
> +static inline void riscv_set_ipi_ops(struct riscv_ipi_ops *ops)
> +{
> +}
> +
> +static inline void riscv_clear_ipi(void)
> +{
> +}
> +
>  #endif /* CONFIG_SMP */
>
>  #if defined(CONFIG_HOTPLUG_CPU) && (CONFIG_SMP)
> diff --git a/arch/riscv/kernel/clint.c b/arch/riscv/kernel/clint.c
> index 3647980d14c3..a9845ee023e2 100644
> --- a/arch/riscv/kernel/clint.c
> +++ b/arch/riscv/kernel/clint.c
> @@ -5,11 +5,11 @@
>
>  #include <linux/io.h>
>  #include <linux/of_address.h>
> +#include <linux/smp.h>
>  #include <linux/types.h>
>  #include <asm/clint.h>
>  #include <asm/csr.h>
>  #include <asm/timex.h>
> -#include <asm/smp.h>
>
>  /*
>   * This is the layout used by the SiFive clint, which is also shared by the qemu
> @@ -21,6 +21,24 @@
>
>  u32 __iomem *clint_ipi_base;
>
> +static void clint_send_ipi(const struct cpumask *target)
> +{
> +       unsigned int cpu;
> +
> +       for_each_cpu(cpu, target)
> +               writel(1, clint_ipi_base + cpuid_to_hartid_map(cpu));
> +}
> +
> +static void clint_clear_ipi(void)
> +{
> +       writel(0, clint_ipi_base + cpuid_to_hartid_map(smp_processor_id()));
> +}
> +
> +static struct riscv_ipi_ops clint_ipi_ops = {
> +       .ipi_inject = clint_send_ipi,
> +       .ipi_clear = clint_clear_ipi,
> +};
> +
>  void clint_init_boot_cpu(void)
>  {
>         struct device_node *np;
> @@ -40,5 +58,6 @@ void clint_init_boot_cpu(void)
>         riscv_time_cmp = base + CLINT_TIME_CMP_OFF;
>         riscv_time_val = base + CLINT_TIME_VAL_OFF;
>
> -       clint_clear_ipi(boot_cpu_hartid);
> +       clint_clear_ipi();
> +       riscv_set_ipi_ops(&clint_ipi_ops);
>  }
> diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> index f383ef5672b2..226ccce0f9e0 100644
> --- a/arch/riscv/kernel/sbi.c
> +++ b/arch/riscv/kernel/sbi.c
> @@ -547,6 +547,18 @@ static inline long sbi_get_firmware_version(void)
>         return __sbi_base_ecall(SBI_EXT_BASE_GET_IMP_VERSION);
>  }
>
> +static void sbi_send_cpumask_ipi(const struct cpumask *target)
> +{
> +       struct cpumask hartid_mask;
> +
> +       riscv_cpuid_to_hartid_mask(target, &hartid_mask);
> +
> +       sbi_send_ipi(cpumask_bits(&hartid_mask));
> +}
> +
> +static struct riscv_ipi_ops sbi_ipi_ops = {
> +       .ipi_inject = sbi_send_cpumask_ipi
> +};
>
>  int __init sbi_init(void)
>  {
> @@ -587,5 +599,7 @@ int __init sbi_init(void)
>                 __sbi_rfence    = __sbi_rfence_v01;
>         }
>
> +       riscv_set_ipi_ops(&sbi_ipi_ops);
> +
>         return 0;
>  }
> diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
> index b1d4f452f843..8b85683ce203 100644
> --- a/arch/riscv/kernel/smp.c
> +++ b/arch/riscv/kernel/smp.c
> @@ -84,9 +84,25 @@ static void ipi_stop(void)
>                 wait_for_interrupt();
>  }
>
> +static struct riscv_ipi_ops *ipi_ops;
> +
> +void riscv_set_ipi_ops(struct riscv_ipi_ops *ops)
> +{
> +       ipi_ops = ops;
> +}
> +EXPORT_SYMBOL_GPL(riscv_set_ipi_ops);
> +
> +void riscv_clear_ipi(void)
> +{
> +       if (ipi_ops && ipi_ops->ipi_clear)
> +               ipi_ops->ipi_clear();
> +
> +       csr_clear(CSR_IP, IE_SIE);
> +}
> +EXPORT_SYMBOL_GPL(riscv_clear_ipi);
> +
>  static void send_ipi_mask(const struct cpumask *mask, enum ipi_message_type op)
>  {
> -       struct cpumask hartid_mask;
>         int cpu;
>
>         smp_mb__before_atomic();
> @@ -94,33 +110,22 @@ static void send_ipi_mask(const struct cpumask *mask, enum ipi_message_type op)
>                 set_bit(op, &ipi_data[cpu].bits);
>         smp_mb__after_atomic();
>
> -       riscv_cpuid_to_hartid_mask(mask, &hartid_mask);
> -       if (IS_ENABLED(CONFIG_RISCV_SBI))
> -               sbi_send_ipi(cpumask_bits(&hartid_mask));
> +       if (ipi_ops && ipi_ops->ipi_inject)
> +               ipi_ops->ipi_inject(mask);
>         else
> -               clint_send_ipi_mask(mask);
> +               pr_warn("SMP: IPI inject method not available\n");
>  }
>
>  static void send_ipi_single(int cpu, enum ipi_message_type op)
>  {
> -       int hartid = cpuid_to_hartid_map(cpu);
> -
>         smp_mb__before_atomic();
>         set_bit(op, &ipi_data[cpu].bits);
>         smp_mb__after_atomic();
>
> -       if (IS_ENABLED(CONFIG_RISCV_SBI))
> -               sbi_send_ipi(cpumask_bits(cpumask_of(hartid)));
> -       else
> -               clint_send_ipi_single(hartid);
> -}
> -
> -static inline void clear_ipi(void)
> -{
> -       if (IS_ENABLED(CONFIG_RISCV_SBI))
> -               csr_clear(CSR_IP, IE_SIE);
> +       if (ipi_ops && ipi_ops->ipi_inject)
> +               ipi_ops->ipi_inject(cpumask_of(cpu));
>         else
> -               clint_clear_ipi(cpuid_to_hartid_map(smp_processor_id()));
> +               pr_warn("SMP: IPI inject method not available\n");
>  }
>
>  void handle_IPI(struct pt_regs *regs)
> @@ -131,7 +136,7 @@ void handle_IPI(struct pt_regs *regs)
>
>         irq_enter();
>
> -       clear_ipi();
> +       riscv_clear_ipi();
>
>         while (true) {
>                 unsigned long ops;
> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
> index 4e9922790f6e..5fe849791bf0 100644
> --- a/arch/riscv/kernel/smpboot.c
> +++ b/arch/riscv/kernel/smpboot.c
> @@ -147,8 +147,7 @@ asmlinkage __visible void smp_callin(void)
>  {
>         struct mm_struct *mm = &init_mm;
>
> -       if (!IS_ENABLED(CONFIG_RISCV_SBI))
> -               clint_clear_ipi(cpuid_to_hartid_map(smp_processor_id()));
> +       riscv_clear_ipi();
>
>         /* All kernel threads share the same mm context.  */
>         mmgrab(mm);
> --
> 2.25.1
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Reviewed-by: Atish Patra <atish.patra@wdc.com>
-- 
Regards,
Atish

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 2/4] clocksource/drivers: Add CLINT timer driver
  2020-07-17  7:50 ` [PATCH v4 2/4] clocksource/drivers: Add CLINT timer driver Anup Patel
@ 2020-07-21  1:11   ` Atish Patra
  2020-07-21 11:43     ` Anup Patel
  2020-07-21 11:02   ` Daniel Lezcano
  1 sibling, 1 reply; 19+ messages in thread
From: Atish Patra @ 2020-07-21  1:11 UTC (permalink / raw)
  To: Anup Patel
  Cc: devicetree, Damien Le Moal, Albert Ou, Emil Renner Berhing,
	Anup Patel, Daniel Lezcano, linux-kernel@vger.kernel.org List,
	Atish Patra, Rob Herring, Palmer Dabbelt, Paul Walmsley,
	Alistair Francis, Thomas Gleixner, linux-riscv

On Fri, Jul 17, 2020 at 12:52 AM Anup Patel <anup.patel@wdc.com> wrote:
>
> We add a separate CLINT timer driver for Linux RISC-V M-mode (i.e.
> RISC-V NoMMU kernel).
>
> The CLINT MMIO device provides three things:
> 1. 64bit free running counter register
> 2. 64bit per-CPU time compare registers
> 3. 32bit per-CPU inter-processor interrupt registers
>
> Unlike other timer devices, CLINT provides IPI registers along with
> timer registers. To use CLINT IPI registers, the CLINT timer driver
> provides IPI related callbacks to arch/riscv.
>
> Signed-off-by: Anup Patel <anup.patel@wdc.com>
> Tested-by: Emil Renner Berhing <kernel@esmil.dk>
> ---
>  drivers/clocksource/Kconfig       |   9 ++
>  drivers/clocksource/Makefile      |   1 +
>  drivers/clocksource/timer-clint.c | 231 ++++++++++++++++++++++++++++++
>  include/linux/cpuhotplug.h        |   1 +
>  4 files changed, 242 insertions(+)
>  create mode 100644 drivers/clocksource/timer-clint.c
>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 91418381fcd4..e1ce0d510a03 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -658,6 +658,15 @@ config RISCV_TIMER
>           is accessed via both the SBI and the rdcycle instruction.  This is
>           required for all RISC-V systems.
>
> +config CLINT_TIMER
> +       bool "Timer for the RISC-V platform"
> +       depends on GENERIC_SCHED_CLOCK && RISCV_M_MODE
> +       select TIMER_PROBE
> +       select TIMER_OF
> +       help
> +         This option enables the CLINT timer for RISC-V systems. The CLINT
> +         driver is usually used for NoMMU RISC-V systems.
> +
>  config CSKY_MP_TIMER
>         bool "SMP Timer for the C-SKY platform" if COMPILE_TEST
>         depends on CSKY
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index bdda1a2e4097..18e700e703a0 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -87,6 +87,7 @@ obj-$(CONFIG_CLKSRC_ST_LPC)           += clksrc_st_lpc.o
>  obj-$(CONFIG_X86_NUMACHIP)             += numachip.o
>  obj-$(CONFIG_ATCPIT100_TIMER)          += timer-atcpit100.o
>  obj-$(CONFIG_RISCV_TIMER)              += timer-riscv.o
> +obj-$(CONFIG_CLINT_TIMER)              += timer-clint.o
>  obj-$(CONFIG_CSKY_MP_TIMER)            += timer-mp-csky.o
>  obj-$(CONFIG_GX6605S_TIMER)            += timer-gx6605s.o
>  obj-$(CONFIG_HYPERV_TIMER)             += hyperv_timer.o
> diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer-clint.c
> new file mode 100644
> index 000000000000..e1698efa73a1
> --- /dev/null
> +++ b/drivers/clocksource/timer-clint.c
> @@ -0,0 +1,231 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2020 Western Digital Corporation or its affiliates.
> + *
> + * Most of the M-mode (i.e. NoMMU) RISC-V systems usually have a
> + * CLINT MMIO timer device.
> + */
> +
> +#define pr_fmt(fmt) "clint: " fmt
> +#include <linux/bitops.h>
> +#include <linux/clocksource.h>
> +#include <linux/clockchips.h>
> +#include <linux/cpu.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/sched_clock.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/interrupt.h>
> +#include <linux/of_irq.h>
> +#include <linux/smp.h>
> +
> +#define CLINT_IPI_OFF          0
> +#define CLINT_TIMER_CMP_OFF    0x4000
> +#define CLINT_TIMER_VAL_OFF    0xbff8
> +
> +/* CLINT manages IPI and Timer for RISC-V M-mode  */
> +static u32 __iomem *clint_ipi_base;
> +static u64 __iomem *clint_timer_cmp;
> +static u64 __iomem *clint_timer_val;
> +static unsigned long clint_timer_freq;
> +static unsigned int clint_timer_irq;
> +
> +static void clint_send_ipi(const struct cpumask *target)
> +{
> +       unsigned int cpu;
> +
> +       for_each_cpu(cpu, target)
> +               writel(1, clint_ipi_base + cpuid_to_hartid_map(cpu));
> +}
> +
> +static void clint_clear_ipi(void)
> +{
> +       writel(0, clint_ipi_base + cpuid_to_hartid_map(smp_processor_id()));
> +}
> +
> +static struct riscv_ipi_ops clint_ipi_ops = {
> +       .ipi_inject = clint_send_ipi,
> +       .ipi_clear = clint_clear_ipi,
> +};
> +
> +#ifdef CONFIG_64BIT
> +#define clint_get_cycles()     readq_relaxed(clint_timer_val)
> +#else
> +#define clint_get_cycles()     readl_relaxed(clint_timer_val)
> +#define clint_get_cycles_hi()  readl_relaxed(((u32 *)clint_timer_val) + 1)
> +#endif
> +
> +#ifdef CONFIG_64BIT
> +static u64 notrace clint_get_cycles64(void)
> +{
> +       return clint_get_cycles();
> +}
> +#else /* CONFIG_64BIT */
> +static u64 notrace clint_get_cycles64(void)
> +{
> +       u32 hi, lo;
> +
> +       do {
> +               hi = clint_get_cycles_hi();
> +               lo = clint_get_cycles();
> +       } while (hi != clint_get_cycles_hi());
> +
> +       return ((u64)hi << 32) | lo;
> +}
> +#endif /* CONFIG_64BIT */
> +
> +static u64 clint_rdtime(struct clocksource *cs)
> +{
> +       return clint_get_cycles64();
> +}
> +
> +static struct clocksource clint_clocksource = {
> +       .name           = "clint_clocksource",
> +       .rating = 300,

nit: Not aligned with other structure members.

> +       .mask           = CLOCKSOURCE_MASK(64),
> +       .flags          = CLOCK_SOURCE_IS_CONTINUOUS,
> +       .read           = clint_rdtime,
> +};
> +
> +static int clint_clock_next_event(unsigned long delta,
> +                                  struct clock_event_device *ce)
> +{
> +       void __iomem *r = clint_timer_cmp +
> +                         cpuid_to_hartid_map(smp_processor_id());
> +
> +       csr_set(CSR_IE, IE_TIE);
> +       writeq_relaxed(clint_get_cycles64() + delta, r);
> +       return 0;
> +}
> +
> +static DEFINE_PER_CPU(struct clock_event_device, clint_clock_event) = {
> +       .name                   = "clint_clockevent",
> +       .features               = CLOCK_EVT_FEAT_ONESHOT,
> +       .rating         = 100,
> +       .set_next_event = clint_clock_next_event,

nit: Not aligned with other structure members.
> +};
> +
> +static DEFINE_PER_CPU(bool, clint_clock_event_registered);
> +
> +static int clint_timer_starting_cpu(unsigned int cpu)
> +{
> +       bool *registered = per_cpu_ptr(&clint_clock_event_registered, cpu);
> +       struct clock_event_device *ce = per_cpu_ptr(&clint_clock_event, cpu);
> +
> +       if (!(*registered)) {
> +               ce->cpumask = cpumask_of(cpu);
> +               clockevents_config_and_register(ce, clint_timer_freq, 200,
> +                                                ULONG_MAX);

Is there a specific reason to choose different values from the timer-riscv ?
The min_delta is set to 100 and max_delta is set to 0x7fffffff in
timer-riscv driver.

> +               *registered = true;
> +       }
> +
> +       enable_percpu_irq(clint_timer_irq,
> +                         irq_get_trigger_type(clint_timer_irq));
> +       return 0;
> +}
> +
> +static int clint_timer_dying_cpu(unsigned int cpu)
> +{
> +       disable_percpu_irq(clint_timer_irq);
> +       return 0;
> +}
> +
> +static irqreturn_t clint_timer_interrupt(int irq, void *dev_id)
> +{
> +       struct clock_event_device *evdev = this_cpu_ptr(&clint_clock_event);
> +
> +       csr_clear(CSR_IE, IE_TIE);
> +       evdev->event_handler(evdev);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static int __init clint_timer_init_dt(struct device_node *np)
> +{
> +       int rc;
> +       u32 i, nr_irqs;
> +       void __iomem *base;
> +       struct of_phandle_args oirq;
> +
> +       /*
> +        * Ensure that CLINT device interrupts are either RV_IRQ_TIMER or
> +        * RV_IRQ_SOFT. If it's anything else then we ignore the device.
> +        */
> +       nr_irqs = of_irq_count(np);
> +       for (i = 0; i < nr_irqs; i++) {
> +               if (of_irq_parse_one(np, i, &oirq)) {
> +                       pr_err("%pOFP: failed to parse irq %d.\n", np, i);
> +                       continue;
> +               }
> +
> +               if ((oirq.args_count != 1) ||
> +                   (oirq.args[0] != RV_IRQ_TIMER &&
> +                    oirq.args[0] != RV_IRQ_SOFT)) {
> +                       pr_err("%pOFP: invalid irq %d (hwirq %d)\n",
> +                              np, i, oirq.args[0]);
> +                       return -ENODEV;
> +               }
> +
> +               /* Find parent irq domain and map timer irq */
> +               if (!clint_timer_irq &&
> +                   oirq.args[0] == RV_IRQ_TIMER &&
> +                   irq_find_host(oirq.np))
> +                       clint_timer_irq = irq_of_parse_and_map(np, i);
> +       }
> +
> +       /* If CLINT timer irq not found then fail */
> +       if (!clint_timer_irq) {
> +               pr_err("%pOFP: timer irq not found\n", np);
> +               return -ENODEV;
> +       }
> +
> +       base = of_iomap(np, 0);
> +       if (!base) {
> +               pr_err("%pOFP: could not map registers\n", np);
> +               return -ENODEV;
> +       }
> +
> +       clint_ipi_base = base + CLINT_IPI_OFF;
> +       clint_timer_cmp = base + CLINT_TIMER_CMP_OFF;
> +       clint_timer_val = base + CLINT_TIMER_VAL_OFF;
> +       clint_timer_freq = riscv_timebase;
> +
> +       pr_info("%pOFP: timer running at %ld Hz\n", np, clint_timer_freq);
> +
> +       rc = clocksource_register_hz(&clint_clocksource, clint_timer_freq);
> +       if (rc) {
> +               iounmap(base);
> +               pr_err("%pOFP: clocksource register failed [%d]\n", np, rc);
> +               return rc;
> +       }
> +
> +       sched_clock_register(clint_get_cycles64, 64, clint_timer_freq);
> +
> +       rc = request_percpu_irq(clint_timer_irq, clint_timer_interrupt,
> +                                "clint-timer", &clint_clock_event);
> +       if (rc) {
> +               iounmap(base);
> +               pr_err("registering percpu irq failed [%d]\n", rc);
> +               return rc;
> +       }
> +
> +       rc = cpuhp_setup_state(CPUHP_AP_CLINT_TIMER_STARTING,
> +                               "clockevents/clint/timer:starting",
> +                               clint_timer_starting_cpu,
> +                               clint_timer_dying_cpu);
> +       if (rc) {
> +               free_irq(clint_timer_irq, &clint_clock_event);
> +               iounmap(base);
> +               pr_err("%pOFP: cpuhp setup state failed [%d]\n", np, rc);
> +               return rc;

All the iounmap & return statements can be moved to a goto at the end
of the function.

> +       }
> +
> +       riscv_set_ipi_ops(&clint_ipi_ops);
> +       clint_clear_ipi();
> +
> +       return 0;
> +}
> +
> +TIMER_OF_DECLARE(clint_timer, "riscv,clint0", clint_timer_init_dt);
> +TIMER_OF_DECLARE(clint_timer1, "sifive,clint0", clint_timer_init_dt);
> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> index 191772d4a4d7..1451f4625833 100644
> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -132,6 +132,7 @@ enum cpuhp_state {
>         CPUHP_AP_MIPS_GIC_TIMER_STARTING,
>         CPUHP_AP_ARC_TIMER_STARTING,
>         CPUHP_AP_RISCV_TIMER_STARTING,
> +       CPUHP_AP_CLINT_TIMER_STARTING,
>         CPUHP_AP_CSKY_TIMER_STARTING,
>         CPUHP_AP_HYPERV_TIMER_STARTING,
>         CPUHP_AP_KVM_STARTING,
> --
> 2.25.1
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv



--
Regards,
Atish

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 4/4] dt-bindings: timer: Add CLINT bindings
  2020-07-17  7:51 ` [PATCH v4 4/4] dt-bindings: timer: Add CLINT bindings Anup Patel
@ 2020-07-21  1:15   ` Atish Patra
  2020-07-21 11:39     ` Anup Patel
  0 siblings, 1 reply; 19+ messages in thread
From: Atish Patra @ 2020-07-21  1:15 UTC (permalink / raw)
  To: Anup Patel
  Cc: devicetree, Damien Le Moal, Albert Ou, Emil Renner Berhing,
	Anup Patel, Daniel Lezcano, linux-kernel@vger.kernel.org List,
	Atish Patra, Rob Herring, Palmer Dabbelt, Paul Walmsley,
	Palmer Dabbelt, Alistair Francis, Thomas Gleixner, linux-riscv

On Fri, Jul 17, 2020 at 12:52 AM Anup Patel <anup.patel@wdc.com> wrote:
>
> We add DT bindings documentation for CLINT device.
>
> Signed-off-by: Anup Patel <anup.patel@wdc.com>
> Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
> Tested-by: Emil Renner Berhing <kernel@esmil.dk>
> ---
>  .../bindings/timer/sifive,clint.yaml          | 58 +++++++++++++++++++
>  1 file changed, 58 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/timer/sifive,clint.yaml
>
> diff --git a/Documentation/devicetree/bindings/timer/sifive,clint.yaml b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> new file mode 100644
> index 000000000000..8ad115611860
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/timer/sifive,clint.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SiFive Core Local Interruptor
> +
> +maintainers:
> +  - Palmer Dabbelt <palmer@dabbelt.com>
> +  - Anup Patel <anup.patel@wdc.com>
> +
> +description:
> +  SiFive (and other RISC-V) SOCs include an implementation of the SiFive
> +  Core Local Interruptor (CLINT) for M-mode timer and M-mode inter-processor
> +  interrupts. It directly connects to the timer and inter-processor interrupt
> +  lines of various HARTs (or CPUs) so RISC-V per-HART (or per-CPU) local
> +  interrupt controller is the parent interrupt controller for CLINT device.
> +  The clock frequency of CLINT is specified via "timebase-frequency" DT
> +  property of "/cpus" DT node. The "timebase-frequency" DT property is
> +  described in Documentation/devicetree/bindings/riscv/cpus.yaml
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: sifive,clint0
> +      - const: sifive,fu540-c000-clint
> +
> +    description:
> +      Should be "sifive,<chip>-clint" and "sifive,clint<version>".
> +      Supported compatible strings are -
> +      "sifive,fu540-c000-clint" for the SiFive CLINT v0 as integrated
> +      onto the SiFive FU540 chip, and "sifive,clint0" for the SiFive
> +      CLINT v0 IP block with no chip integration tweaks.
> +      Please refer to sifive-blocks-ip-versioning.txt for details
> +

As the DT binding suggests that the clint device should be named as "sifive,**",
I think we should change the DT property in kendryte dts as well.

> +  reg:
> +    maxItems: 1
> +
> +  interrupts-extended:
> +    minItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts-extended
> +
> +examples:
> +  - |
> +    clint@2000000 {
> +      compatible = "sifive,clint0", "sifive,fu540-c000-clint";
> +      interrupts-extended = <&cpu1intc 3 &cpu1intc 7
> +                             &cpu2intc 3 &cpu2intc 7
> +                             &cpu3intc 3 &cpu3intc 7
> +                             &cpu4intc 3 &cpu4intc 7>;
> +       reg = <0x2000000 0x4000000>;
> +    };
> +...
> --
> 2.25.1
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Otherwise,

Reviewed-by: Atish Patra <atish.patra@wdc.com>

-- 
Regards,
Atish

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 2/4] clocksource/drivers: Add CLINT timer driver
  2020-07-17  7:50 ` [PATCH v4 2/4] clocksource/drivers: Add CLINT timer driver Anup Patel
  2020-07-21  1:11   ` Atish Patra
@ 2020-07-21 11:02   ` Daniel Lezcano
  2020-07-21 11:49     ` Anup Patel
  1 sibling, 1 reply; 19+ messages in thread
From: Daniel Lezcano @ 2020-07-21 11:02 UTC (permalink / raw)
  To: Anup Patel, Palmer Dabbelt, Paul Walmsley, Albert Ou,
	Rob Herring, Thomas Gleixner
  Cc: devicetree, Damien Le Moal, Emil Renner Berhing, Anup Patel,
	linux-kernel, Atish Patra, Alistair Francis, linux-riscv

On 17/07/2020 09:50, Anup Patel wrote:
> We add a separate CLINT timer driver for Linux RISC-V M-mode (i.e.
> RISC-V NoMMU kernel).
> 
> The CLINT MMIO device provides three things:
> 1. 64bit free running counter register
> 2. 64bit per-CPU time compare registers
> 3. 32bit per-CPU inter-processor interrupt registers
> 
> Unlike other timer devices, CLINT provides IPI registers along with
> timer registers. To use CLINT IPI registers, the CLINT timer driver
> provides IPI related callbacks to arch/riscv.
> 
> Signed-off-by: Anup Patel <anup.patel@wdc.com>
> Tested-by: Emil Renner Berhing <kernel@esmil.dk>
> ---
>  drivers/clocksource/Kconfig       |   9 ++
>  drivers/clocksource/Makefile      |   1 +
>  drivers/clocksource/timer-clint.c | 231 ++++++++++++++++++++++++++++++
>  include/linux/cpuhotplug.h        |   1 +
>  4 files changed, 242 insertions(+)
>  create mode 100644 drivers/clocksource/timer-clint.c
> 
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 91418381fcd4..e1ce0d510a03 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -658,6 +658,15 @@ config RISCV_TIMER
>  	  is accessed via both the SBI and the rdcycle instruction.  This is
>  	  required for all RISC-V systems.
>  
> +config CLINT_TIMER
> +	bool "Timer for the RISC-V platform"
> +	depends on GENERIC_SCHED_CLOCK && RISCV_M_MODE
> +	select TIMER_PROBE
> +	select TIMER_OF
> +	help
> +	  This option enables the CLINT timer for RISC-V systems. The CLINT
> +	  driver is usually used for NoMMU RISC-V systems.

V3 has a comment about fixing the Kconfig option.

[ ... ]

> +{
> +	bool *registered = per_cpu_ptr(&clint_clock_event_registered, cpu);
> +	struct clock_event_device *ce = per_cpu_ptr(&clint_clock_event, cpu);
> +
> +	if (!(*registered)) {
> +		ce->cpumask = cpumask_of(cpu);
> +		clockevents_config_and_register(ce, clint_timer_freq, 200,
> +						 ULONG_MAX);
> +		*registered = true;
> +	}


I was unsure about the clockevents_config_and_register() multiple calls
when doing the comment. It seems like it is fine to call it several
times and that is done in several places like riscv or arch_arm_timer.

It is probably safe to drop the 'registered' code here, sorry for the
confusion.

> +	enable_percpu_irq(clint_timer_irq,
> +			  irq_get_trigger_type(clint_timer_irq));
> +	return 0;
> +}
> +

[ ... ]


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 4/4] dt-bindings: timer: Add CLINT bindings
  2020-07-21  1:15   ` Atish Patra
@ 2020-07-21 11:39     ` Anup Patel
  2020-07-21 12:17       ` Sean Anderson
  0 siblings, 1 reply; 19+ messages in thread
From: Anup Patel @ 2020-07-21 11:39 UTC (permalink / raw)
  To: Atish Patra
  Cc: devicetree, Damien Le Moal, Daniel Lezcano, Emil Renner Berhing,
	Anup Patel, linux-kernel@vger.kernel.org List, Atish Patra,
	Rob Herring, Palmer Dabbelt, Paul Walmsley, Palmer Dabbelt,
	Alistair Francis, Thomas Gleixner, linux-riscv, Albert Ou

On Tue, Jul 21, 2020 at 6:45 AM Atish Patra <atishp@atishpatra.org> wrote:
>
> On Fri, Jul 17, 2020 at 12:52 AM Anup Patel <anup.patel@wdc.com> wrote:
> >
> > We add DT bindings documentation for CLINT device.
> >
> > Signed-off-by: Anup Patel <anup.patel@wdc.com>
> > Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
> > Tested-by: Emil Renner Berhing <kernel@esmil.dk>
> > ---
> >  .../bindings/timer/sifive,clint.yaml          | 58 +++++++++++++++++++
> >  1 file changed, 58 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/timer/sifive,clint.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/timer/sifive,clint.yaml b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> > new file mode 100644
> > index 000000000000..8ad115611860
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> > @@ -0,0 +1,58 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/timer/sifive,clint.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: SiFive Core Local Interruptor
> > +
> > +maintainers:
> > +  - Palmer Dabbelt <palmer@dabbelt.com>
> > +  - Anup Patel <anup.patel@wdc.com>
> > +
> > +description:
> > +  SiFive (and other RISC-V) SOCs include an implementation of the SiFive
> > +  Core Local Interruptor (CLINT) for M-mode timer and M-mode inter-processor
> > +  interrupts. It directly connects to the timer and inter-processor interrupt
> > +  lines of various HARTs (or CPUs) so RISC-V per-HART (or per-CPU) local
> > +  interrupt controller is the parent interrupt controller for CLINT device.
> > +  The clock frequency of CLINT is specified via "timebase-frequency" DT
> > +  property of "/cpus" DT node. The "timebase-frequency" DT property is
> > +  described in Documentation/devicetree/bindings/riscv/cpus.yaml
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: sifive,clint0
> > +      - const: sifive,fu540-c000-clint
> > +
> > +    description:
> > +      Should be "sifive,<chip>-clint" and "sifive,clint<version>".
> > +      Supported compatible strings are -
> > +      "sifive,fu540-c000-clint" for the SiFive CLINT v0 as integrated
> > +      onto the SiFive FU540 chip, and "sifive,clint0" for the SiFive
> > +      CLINT v0 IP block with no chip integration tweaks.
> > +      Please refer to sifive-blocks-ip-versioning.txt for details
> > +
>
> As the DT binding suggests that the clint device should be named as "sifive,**",
> I think we should change the DT property in kendryte dts as well.

Okay, I will do it as a separate patch.

>
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts-extended:
> > +    minItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts-extended
> > +
> > +examples:
> > +  - |
> > +    clint@2000000 {
> > +      compatible = "sifive,clint0", "sifive,fu540-c000-clint";
> > +      interrupts-extended = <&cpu1intc 3 &cpu1intc 7
> > +                             &cpu2intc 3 &cpu2intc 7
> > +                             &cpu3intc 3 &cpu3intc 7
> > +                             &cpu4intc 3 &cpu4intc 7>;
> > +       reg = <0x2000000 0x4000000>;
> > +    };
> > +...
> > --
> > 2.25.1
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>
> Otherwise,
>
> Reviewed-by: Atish Patra <atish.patra@wdc.com>
>
> --
> Regards,
> Atish

Regards,
Anup

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 2/4] clocksource/drivers: Add CLINT timer driver
  2020-07-21  1:11   ` Atish Patra
@ 2020-07-21 11:43     ` Anup Patel
  2020-07-21 19:39       ` Atish Patra
  0 siblings, 1 reply; 19+ messages in thread
From: Anup Patel @ 2020-07-21 11:43 UTC (permalink / raw)
  To: Atish Patra
  Cc: devicetree, Damien Le Moal, Daniel Lezcano, Emil Renner Berhing,
	Anup Patel, linux-kernel@vger.kernel.org List, Atish Patra,
	Rob Herring, Palmer Dabbelt, Paul Walmsley, Alistair Francis,
	Thomas Gleixner, linux-riscv, Albert Ou

On Tue, Jul 21, 2020 at 6:41 AM Atish Patra <atishp@atishpatra.org> wrote:
>
> On Fri, Jul 17, 2020 at 12:52 AM Anup Patel <anup.patel@wdc.com> wrote:
> >
> > We add a separate CLINT timer driver for Linux RISC-V M-mode (i.e.
> > RISC-V NoMMU kernel).
> >
> > The CLINT MMIO device provides three things:
> > 1. 64bit free running counter register
> > 2. 64bit per-CPU time compare registers
> > 3. 32bit per-CPU inter-processor interrupt registers
> >
> > Unlike other timer devices, CLINT provides IPI registers along with
> > timer registers. To use CLINT IPI registers, the CLINT timer driver
> > provides IPI related callbacks to arch/riscv.
> >
> > Signed-off-by: Anup Patel <anup.patel@wdc.com>
> > Tested-by: Emil Renner Berhing <kernel@esmil.dk>
> > ---
> >  drivers/clocksource/Kconfig       |   9 ++
> >  drivers/clocksource/Makefile      |   1 +
> >  drivers/clocksource/timer-clint.c | 231 ++++++++++++++++++++++++++++++
> >  include/linux/cpuhotplug.h        |   1 +
> >  4 files changed, 242 insertions(+)
> >  create mode 100644 drivers/clocksource/timer-clint.c
> >
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index 91418381fcd4..e1ce0d510a03 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -658,6 +658,15 @@ config RISCV_TIMER
> >           is accessed via both the SBI and the rdcycle instruction.  This is
> >           required for all RISC-V systems.
> >
> > +config CLINT_TIMER
> > +       bool "Timer for the RISC-V platform"
> > +       depends on GENERIC_SCHED_CLOCK && RISCV_M_MODE
> > +       select TIMER_PROBE
> > +       select TIMER_OF
> > +       help
> > +         This option enables the CLINT timer for RISC-V systems. The CLINT
> > +         driver is usually used for NoMMU RISC-V systems.
> > +
> >  config CSKY_MP_TIMER
> >         bool "SMP Timer for the C-SKY platform" if COMPILE_TEST
> >         depends on CSKY
> > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> > index bdda1a2e4097..18e700e703a0 100644
> > --- a/drivers/clocksource/Makefile
> > +++ b/drivers/clocksource/Makefile
> > @@ -87,6 +87,7 @@ obj-$(CONFIG_CLKSRC_ST_LPC)           += clksrc_st_lpc.o
> >  obj-$(CONFIG_X86_NUMACHIP)             += numachip.o
> >  obj-$(CONFIG_ATCPIT100_TIMER)          += timer-atcpit100.o
> >  obj-$(CONFIG_RISCV_TIMER)              += timer-riscv.o
> > +obj-$(CONFIG_CLINT_TIMER)              += timer-clint.o
> >  obj-$(CONFIG_CSKY_MP_TIMER)            += timer-mp-csky.o
> >  obj-$(CONFIG_GX6605S_TIMER)            += timer-gx6605s.o
> >  obj-$(CONFIG_HYPERV_TIMER)             += hyperv_timer.o
> > diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer-clint.c
> > new file mode 100644
> > index 000000000000..e1698efa73a1
> > --- /dev/null
> > +++ b/drivers/clocksource/timer-clint.c
> > @@ -0,0 +1,231 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2020 Western Digital Corporation or its affiliates.
> > + *
> > + * Most of the M-mode (i.e. NoMMU) RISC-V systems usually have a
> > + * CLINT MMIO timer device.
> > + */
> > +
> > +#define pr_fmt(fmt) "clint: " fmt
> > +#include <linux/bitops.h>
> > +#include <linux/clocksource.h>
> > +#include <linux/clockchips.h>
> > +#include <linux/cpu.h>
> > +#include <linux/delay.h>
> > +#include <linux/module.h>
> > +#include <linux/of_address.h>
> > +#include <linux/sched_clock.h>
> > +#include <linux/io-64-nonatomic-lo-hi.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/smp.h>
> > +
> > +#define CLINT_IPI_OFF          0
> > +#define CLINT_TIMER_CMP_OFF    0x4000
> > +#define CLINT_TIMER_VAL_OFF    0xbff8
> > +
> > +/* CLINT manages IPI and Timer for RISC-V M-mode  */
> > +static u32 __iomem *clint_ipi_base;
> > +static u64 __iomem *clint_timer_cmp;
> > +static u64 __iomem *clint_timer_val;
> > +static unsigned long clint_timer_freq;
> > +static unsigned int clint_timer_irq;
> > +
> > +static void clint_send_ipi(const struct cpumask *target)
> > +{
> > +       unsigned int cpu;
> > +
> > +       for_each_cpu(cpu, target)
> > +               writel(1, clint_ipi_base + cpuid_to_hartid_map(cpu));
> > +}
> > +
> > +static void clint_clear_ipi(void)
> > +{
> > +       writel(0, clint_ipi_base + cpuid_to_hartid_map(smp_processor_id()));
> > +}
> > +
> > +static struct riscv_ipi_ops clint_ipi_ops = {
> > +       .ipi_inject = clint_send_ipi,
> > +       .ipi_clear = clint_clear_ipi,
> > +};
> > +
> > +#ifdef CONFIG_64BIT
> > +#define clint_get_cycles()     readq_relaxed(clint_timer_val)
> > +#else
> > +#define clint_get_cycles()     readl_relaxed(clint_timer_val)
> > +#define clint_get_cycles_hi()  readl_relaxed(((u32 *)clint_timer_val) + 1)
> > +#endif
> > +
> > +#ifdef CONFIG_64BIT
> > +static u64 notrace clint_get_cycles64(void)
> > +{
> > +       return clint_get_cycles();
> > +}
> > +#else /* CONFIG_64BIT */
> > +static u64 notrace clint_get_cycles64(void)
> > +{
> > +       u32 hi, lo;
> > +
> > +       do {
> > +               hi = clint_get_cycles_hi();
> > +               lo = clint_get_cycles();
> > +       } while (hi != clint_get_cycles_hi());
> > +
> > +       return ((u64)hi << 32) | lo;
> > +}
> > +#endif /* CONFIG_64BIT */
> > +
> > +static u64 clint_rdtime(struct clocksource *cs)
> > +{
> > +       return clint_get_cycles64();
> > +}
> > +
> > +static struct clocksource clint_clocksource = {
> > +       .name           = "clint_clocksource",
> > +       .rating = 300,
>
> nit: Not aligned with other structure members.
>
> > +       .mask           = CLOCKSOURCE_MASK(64),
> > +       .flags          = CLOCK_SOURCE_IS_CONTINUOUS,
> > +       .read           = clint_rdtime,
> > +};
> > +
> > +static int clint_clock_next_event(unsigned long delta,
> > +                                  struct clock_event_device *ce)
> > +{
> > +       void __iomem *r = clint_timer_cmp +
> > +                         cpuid_to_hartid_map(smp_processor_id());
> > +
> > +       csr_set(CSR_IE, IE_TIE);
> > +       writeq_relaxed(clint_get_cycles64() + delta, r);
> > +       return 0;
> > +}
> > +
> > +static DEFINE_PER_CPU(struct clock_event_device, clint_clock_event) = {
> > +       .name                   = "clint_clockevent",
> > +       .features               = CLOCK_EVT_FEAT_ONESHOT,
> > +       .rating         = 100,
> > +       .set_next_event = clint_clock_next_event,
>
> nit: Not aligned with other structure members.

Okay, will update.

> > +};
> > +
> > +static DEFINE_PER_CPU(bool, clint_clock_event_registered);
> > +
> > +static int clint_timer_starting_cpu(unsigned int cpu)
> > +{
> > +       bool *registered = per_cpu_ptr(&clint_clock_event_registered, cpu);
> > +       struct clock_event_device *ce = per_cpu_ptr(&clint_clock_event, cpu);
> > +
> > +       if (!(*registered)) {
> > +               ce->cpumask = cpumask_of(cpu);
> > +               clockevents_config_and_register(ce, clint_timer_freq, 200,
> > +                                                ULONG_MAX);
>
> Is there a specific reason to choose different values from the timer-riscv ?
> The min_delta is set to 100 and max_delta is set to 0x7fffffff in
> timer-riscv driver.

Not really, I will update. I think it's better to use 100 and ULONG_MAX to
have fewer magic values.

>
> > +               *registered = true;
> > +       }
> > +
> > +       enable_percpu_irq(clint_timer_irq,
> > +                         irq_get_trigger_type(clint_timer_irq));
> > +       return 0;
> > +}
> > +
> > +static int clint_timer_dying_cpu(unsigned int cpu)
> > +{
> > +       disable_percpu_irq(clint_timer_irq);
> > +       return 0;
> > +}
> > +
> > +static irqreturn_t clint_timer_interrupt(int irq, void *dev_id)
> > +{
> > +       struct clock_event_device *evdev = this_cpu_ptr(&clint_clock_event);
> > +
> > +       csr_clear(CSR_IE, IE_TIE);
> > +       evdev->event_handler(evdev);
> > +
> > +       return IRQ_HANDLED;
> > +}
> > +
> > +static int __init clint_timer_init_dt(struct device_node *np)
> > +{
> > +       int rc;
> > +       u32 i, nr_irqs;
> > +       void __iomem *base;
> > +       struct of_phandle_args oirq;
> > +
> > +       /*
> > +        * Ensure that CLINT device interrupts are either RV_IRQ_TIMER or
> > +        * RV_IRQ_SOFT. If it's anything else then we ignore the device.
> > +        */
> > +       nr_irqs = of_irq_count(np);
> > +       for (i = 0; i < nr_irqs; i++) {
> > +               if (of_irq_parse_one(np, i, &oirq)) {
> > +                       pr_err("%pOFP: failed to parse irq %d.\n", np, i);
> > +                       continue;
> > +               }
> > +
> > +               if ((oirq.args_count != 1) ||
> > +                   (oirq.args[0] != RV_IRQ_TIMER &&
> > +                    oirq.args[0] != RV_IRQ_SOFT)) {
> > +                       pr_err("%pOFP: invalid irq %d (hwirq %d)\n",
> > +                              np, i, oirq.args[0]);
> > +                       return -ENODEV;
> > +               }
> > +
> > +               /* Find parent irq domain and map timer irq */
> > +               if (!clint_timer_irq &&
> > +                   oirq.args[0] == RV_IRQ_TIMER &&
> > +                   irq_find_host(oirq.np))
> > +                       clint_timer_irq = irq_of_parse_and_map(np, i);
> > +       }
> > +
> > +       /* If CLINT timer irq not found then fail */
> > +       if (!clint_timer_irq) {
> > +               pr_err("%pOFP: timer irq not found\n", np);
> > +               return -ENODEV;
> > +       }
> > +
> > +       base = of_iomap(np, 0);
> > +       if (!base) {
> > +               pr_err("%pOFP: could not map registers\n", np);
> > +               return -ENODEV;
> > +       }
> > +
> > +       clint_ipi_base = base + CLINT_IPI_OFF;
> > +       clint_timer_cmp = base + CLINT_TIMER_CMP_OFF;
> > +       clint_timer_val = base + CLINT_TIMER_VAL_OFF;
> > +       clint_timer_freq = riscv_timebase;
> > +
> > +       pr_info("%pOFP: timer running at %ld Hz\n", np, clint_timer_freq);
> > +
> > +       rc = clocksource_register_hz(&clint_clocksource, clint_timer_freq);
> > +       if (rc) {
> > +               iounmap(base);
> > +               pr_err("%pOFP: clocksource register failed [%d]\n", np, rc);
> > +               return rc;
> > +       }
> > +
> > +       sched_clock_register(clint_get_cycles64, 64, clint_timer_freq);
> > +
> > +       rc = request_percpu_irq(clint_timer_irq, clint_timer_interrupt,
> > +                                "clint-timer", &clint_clock_event);
> > +       if (rc) {
> > +               iounmap(base);
> > +               pr_err("registering percpu irq failed [%d]\n", rc);
> > +               return rc;
> > +       }
> > +
> > +       rc = cpuhp_setup_state(CPUHP_AP_CLINT_TIMER_STARTING,
> > +                               "clockevents/clint/timer:starting",
> > +                               clint_timer_starting_cpu,
> > +                               clint_timer_dying_cpu);
> > +       if (rc) {
> > +               free_irq(clint_timer_irq, &clint_clock_event);
> > +               iounmap(base);
> > +               pr_err("%pOFP: cpuhp setup state failed [%d]\n", np, rc);
> > +               return rc;
>
> All the iounmap & return statements can be moved to a goto at the end
> of the function.

Okay, will update.

>
> > +       }
> > +
> > +       riscv_set_ipi_ops(&clint_ipi_ops);
> > +       clint_clear_ipi();
> > +
> > +       return 0;
> > +}
> > +
> > +TIMER_OF_DECLARE(clint_timer, "riscv,clint0", clint_timer_init_dt);
> > +TIMER_OF_DECLARE(clint_timer1, "sifive,clint0", clint_timer_init_dt);
> > diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> > index 191772d4a4d7..1451f4625833 100644
> > --- a/include/linux/cpuhotplug.h
> > +++ b/include/linux/cpuhotplug.h
> > @@ -132,6 +132,7 @@ enum cpuhp_state {
> >         CPUHP_AP_MIPS_GIC_TIMER_STARTING,
> >         CPUHP_AP_ARC_TIMER_STARTING,
> >         CPUHP_AP_RISCV_TIMER_STARTING,
> > +       CPUHP_AP_CLINT_TIMER_STARTING,
> >         CPUHP_AP_CSKY_TIMER_STARTING,
> >         CPUHP_AP_HYPERV_TIMER_STARTING,
> >         CPUHP_AP_KVM_STARTING,
> > --
> > 2.25.1
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>
>
>
> --
> Regards,
> Atish

Regards,
Anup

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 2/4] clocksource/drivers: Add CLINT timer driver
  2020-07-21 11:02   ` Daniel Lezcano
@ 2020-07-21 11:49     ` Anup Patel
  2020-07-21 12:15       ` Daniel Lezcano
  0 siblings, 1 reply; 19+ messages in thread
From: Anup Patel @ 2020-07-21 11:49 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: devicetree, Damien Le Moal, Albert Ou, Emil Renner Berhing,
	Anup Patel, linux-kernel@vger.kernel.org List, Atish Patra,
	Rob Herring, Palmer Dabbelt, Paul Walmsley, Alistair Francis,
	Thomas Gleixner, linux-riscv

On Tue, Jul 21, 2020 at 4:32 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 17/07/2020 09:50, Anup Patel wrote:
> > We add a separate CLINT timer driver for Linux RISC-V M-mode (i.e.
> > RISC-V NoMMU kernel).
> >
> > The CLINT MMIO device provides three things:
> > 1. 64bit free running counter register
> > 2. 64bit per-CPU time compare registers
> > 3. 32bit per-CPU inter-processor interrupt registers
> >
> > Unlike other timer devices, CLINT provides IPI registers along with
> > timer registers. To use CLINT IPI registers, the CLINT timer driver
> > provides IPI related callbacks to arch/riscv.
> >
> > Signed-off-by: Anup Patel <anup.patel@wdc.com>
> > Tested-by: Emil Renner Berhing <kernel@esmil.dk>
> > ---
> >  drivers/clocksource/Kconfig       |   9 ++
> >  drivers/clocksource/Makefile      |   1 +
> >  drivers/clocksource/timer-clint.c | 231 ++++++++++++++++++++++++++++++
> >  include/linux/cpuhotplug.h        |   1 +
> >  4 files changed, 242 insertions(+)
> >  create mode 100644 drivers/clocksource/timer-clint.c
> >
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index 91418381fcd4..e1ce0d510a03 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -658,6 +658,15 @@ config RISCV_TIMER
> >         is accessed via both the SBI and the rdcycle instruction.  This is
> >         required for all RISC-V systems.
> >
> > +config CLINT_TIMER
> > +     bool "Timer for the RISC-V platform"
> > +     depends on GENERIC_SCHED_CLOCK && RISCV_M_MODE
> > +     select TIMER_PROBE
> > +     select TIMER_OF
> > +     help
> > +       This option enables the CLINT timer for RISC-V systems. The CLINT
> > +       driver is usually used for NoMMU RISC-V systems.
>
> V3 has a comment about fixing the Kconfig option.

I have removed "default y" from the Kconfig option as-per your suggestions.

I looked at other Timer Kconfig options. Most of them have menuconfig name.
Also, we can certainly have different timer MMIO timer drivers in future. Do
you still insist on making this kconfig option totally silent ??

>
> [ ... ]
>
> > +{
> > +     bool *registered = per_cpu_ptr(&clint_clock_event_registered, cpu);
> > +     struct clock_event_device *ce = per_cpu_ptr(&clint_clock_event, cpu);
> > +
> > +     if (!(*registered)) {
> > +             ce->cpumask = cpumask_of(cpu);
> > +             clockevents_config_and_register(ce, clint_timer_freq, 200,
> > +                                              ULONG_MAX);
> > +             *registered = true;
> > +     }
>
>
> I was unsure about the clockevents_config_and_register() multiple calls
> when doing the comment. It seems like it is fine to call it several
> times and that is done in several places like riscv or arch_arm_timer.
>
> It is probably safe to drop the 'registered' code here, sorry for the
> confusion.

Okay, will revert these changes.

>
> > +     enable_percpu_irq(clint_timer_irq,
> > +                       irq_get_trigger_type(clint_timer_irq));
> > +     return 0;
> > +}
> > +
>
> [ ... ]
>
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog

Regards,
Anup

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 2/4] clocksource/drivers: Add CLINT timer driver
  2020-07-21 11:49     ` Anup Patel
@ 2020-07-21 12:15       ` Daniel Lezcano
  2020-07-22  3:36         ` Anup Patel
  0 siblings, 1 reply; 19+ messages in thread
From: Daniel Lezcano @ 2020-07-21 12:15 UTC (permalink / raw)
  To: Anup Patel
  Cc: devicetree, Damien Le Moal, Albert Ou, Emil Renner Berhing,
	Anup Patel, linux-kernel@vger.kernel.org List, Atish Patra,
	Rob Herring, Palmer Dabbelt, Paul Walmsley, Alistair Francis,
	Thomas Gleixner, linux-riscv

On 21/07/2020 13:49, Anup Patel wrote:
> On Tue, Jul 21, 2020 at 4:32 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 17/07/2020 09:50, Anup Patel wrote:
>>> We add a separate CLINT timer driver for Linux RISC-V M-mode (i.e.
>>> RISC-V NoMMU kernel).
>>>
>>> The CLINT MMIO device provides three things:
>>> 1. 64bit free running counter register
>>> 2. 64bit per-CPU time compare registers
>>> 3. 32bit per-CPU inter-processor interrupt registers
>>>
>>> Unlike other timer devices, CLINT provides IPI registers along with
>>> timer registers. To use CLINT IPI registers, the CLINT timer driver
>>> provides IPI related callbacks to arch/riscv.
>>>
>>> Signed-off-by: Anup Patel <anup.patel@wdc.com>
>>> Tested-by: Emil Renner Berhing <kernel@esmil.dk>
>>> ---
>>>  drivers/clocksource/Kconfig       |   9 ++
>>>  drivers/clocksource/Makefile      |   1 +
>>>  drivers/clocksource/timer-clint.c | 231 ++++++++++++++++++++++++++++++
>>>  include/linux/cpuhotplug.h        |   1 +
>>>  4 files changed, 242 insertions(+)
>>>  create mode 100644 drivers/clocksource/timer-clint.c
>>>
>>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>>> index 91418381fcd4..e1ce0d510a03 100644
>>> --- a/drivers/clocksource/Kconfig
>>> +++ b/drivers/clocksource/Kconfig
>>> @@ -658,6 +658,15 @@ config RISCV_TIMER
>>>         is accessed via both the SBI and the rdcycle instruction.  This is
>>>         required for all RISC-V systems.
>>>
>>> +config CLINT_TIMER
>>> +     bool "Timer for the RISC-V platform"
>>> +     depends on GENERIC_SCHED_CLOCK && RISCV_M_MODE
>>> +     select TIMER_PROBE
>>> +     select TIMER_OF
>>> +     help
>>> +       This option enables the CLINT timer for RISC-V systems. The CLINT
>>> +       driver is usually used for NoMMU RISC-V systems.
>>
>> V3 has a comment about fixing the Kconfig option.
> 
> I have removed "default y" from the Kconfig option as-per your suggestions.
> 
> I looked at other Timer Kconfig options. Most of them have menuconfig name.
> Also, we can certainly have different timer MMIO timer drivers in future. Do
> you still insist on making this kconfig option totally silent ??

Yes, and there is an effort to change the entries to be silent as much
as possible.

Just add:

	bool "Timer for the RISC-V platform" if COMPILE_TEST

and remove the RISCV_M_MODE dependency.

Or alternatively:

replace the RISCV_M_MODE dependency with COMPILE_TEST

The goal is to be able to compile the driver on different platforms for
compilation test covering.

Then when more mmio drivers will added we will figure out.

>> [ ... ]
>>
>>> +{
>>> +     bool *registered = per_cpu_ptr(&clint_clock_event_registered, cpu);
>>> +     struct clock_event_device *ce = per_cpu_ptr(&clint_clock_event, cpu);
>>> +
>>> +     if (!(*registered)) {
>>> +             ce->cpumask = cpumask_of(cpu);
>>> +             clockevents_config_and_register(ce, clint_timer_freq, 200,
>>> +                                              ULONG_MAX);
>>> +             *registered = true;
>>> +     }
>>
>>
>> I was unsure about the clockevents_config_and_register() multiple calls
>> when doing the comment. It seems like it is fine to call it several
>> times and that is done in several places like riscv or arch_arm_timer.
>>
>> It is probably safe to drop the 'registered' code here, sorry for the
>> confusion.
> 
> Okay, will revert these changes.
> 
>>
>>> +     enable_percpu_irq(clint_timer_irq,
>>> +                       irq_get_trigger_type(clint_timer_irq));
>>> +     return 0;
>>> +}
>>> +
>>
>> [ ... ]
>>
>>
>> --
>> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>>
>> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
>> <http://twitter.com/#!/linaroorg> Twitter |
>> <http://www.linaro.org/linaro-blog/> Blog
> 
> Regards,
> Anup
> 


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 4/4] dt-bindings: timer: Add CLINT bindings
  2020-07-21 11:39     ` Anup Patel
@ 2020-07-21 12:17       ` Sean Anderson
  2020-07-22  3:55         ` Anup Patel
  0 siblings, 1 reply; 19+ messages in thread
From: Sean Anderson @ 2020-07-21 12:17 UTC (permalink / raw)
  To: Anup Patel, Atish Patra
  Cc: devicetree, Damien Le Moal, Daniel Lezcano, Emil Renner Berhing,
	Anup Patel, linux-kernel@vger.kernel.org List, Atish Patra,
	Rob Herring, Palmer Dabbelt, Paul Walmsley, Palmer Dabbelt,
	Alistair Francis, Thomas Gleixner, linux-riscv, Albert Ou

On 7/20/20 9:15 PM, Atish Patra wrote:
> On Fri, Jul 17, 2020 at 12:52 AM Anup Patel <anup.patel@wdc.com> wrote:
>>
>> We add DT bindings documentation for CLINT device.
>>
>> Signed-off-by: Anup Patel <anup.patel@wdc.com>
>> Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
>> Tested-by: Emil Renner Berhing <kernel@esmil.dk>
>> ---
>>  .../bindings/timer/sifive,clint.yaml          | 58 +++++++++++++++++++
>>  1 file changed, 58 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/timer/sifive,clint.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/timer/sifive,clint.yaml b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
>> new file mode 100644
>> index 000000000000..8ad115611860
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
>> @@ -0,0 +1,58 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/timer/sifive,clint.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: SiFive Core Local Interruptor
>> +
>> +maintainers:
>> +  - Palmer Dabbelt <palmer@dabbelt.com>
>> +  - Anup Patel <anup.patel@wdc.com>
>> +
>> +description:
>> +  SiFive (and other RISC-V) SOCs include an implementation of the SiFive
>> +  Core Local Interruptor (CLINT) for M-mode timer and M-mode inter-processor
>> +  interrupts. It directly connects to the timer and inter-processor interrupt
>> +  lines of various HARTs (or CPUs) so RISC-V per-HART (or per-CPU) local
>> +  interrupt controller is the parent interrupt controller for CLINT device.
>> +  The clock frequency of CLINT is specified via "timebase-frequency" DT
>> +  property of "/cpus" DT node. The "timebase-frequency" DT property is
>> +  described in Documentation/devicetree/bindings/riscv/cpus.yaml
>> +
>> +properties:
>> +  compatible:
>> +    items:
>> +      - const: sifive,clint0
>> +      - const: sifive,fu540-c000-clint
>> +
>> +    description:
>> +      Should be "sifive,<chip>-clint" and "sifive,clint<version>".
>> +      Supported compatible strings are -
>> +      "sifive,fu540-c000-clint" for the SiFive CLINT v0 as integrated
>> +      onto the SiFive FU540 chip, and "sifive,clint0" for the SiFive
>> +      CLINT v0 IP block with no chip integration tweaks.
>> +      Please refer to sifive-blocks-ip-versioning.txt for details
>> +
> 
> As the DT binding suggests that the clint device should be named as "sifive,**",
> I think we should change the DT property in kendryte dts as well.

The kendryte device is based on Rocket Chip, not any SiFive IP/device.
If anything, the general binding should be "chipsalliance,clint" and the
specific bindings should be "sifive,clint" and "kendryte,clint" (or
"canaan,clint").

--Sean

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 2/4] clocksource/drivers: Add CLINT timer driver
  2020-07-21 11:43     ` Anup Patel
@ 2020-07-21 19:39       ` Atish Patra
  0 siblings, 0 replies; 19+ messages in thread
From: Atish Patra @ 2020-07-21 19:39 UTC (permalink / raw)
  To: Anup Patel
  Cc: devicetree, Damien Le Moal, Daniel Lezcano, Emil Renner Berhing,
	Anup Patel, linux-kernel@vger.kernel.org List, Atish Patra,
	Rob Herring, Palmer Dabbelt, Paul Walmsley, Alistair Francis,
	Thomas Gleixner, linux-riscv, Albert Ou

On Tue, Jul 21, 2020 at 4:44 AM Anup Patel <anup@brainfault.org> wrote:
>
> On Tue, Jul 21, 2020 at 6:41 AM Atish Patra <atishp@atishpatra.org> wrote:
> >
> > On Fri, Jul 17, 2020 at 12:52 AM Anup Patel <anup.patel@wdc.com> wrote:
> > >
> > > We add a separate CLINT timer driver for Linux RISC-V M-mode (i.e.
> > > RISC-V NoMMU kernel).
> > >
> > > The CLINT MMIO device provides three things:
> > > 1. 64bit free running counter register
> > > 2. 64bit per-CPU time compare registers
> > > 3. 32bit per-CPU inter-processor interrupt registers
> > >
> > > Unlike other timer devices, CLINT provides IPI registers along with
> > > timer registers. To use CLINT IPI registers, the CLINT timer driver
> > > provides IPI related callbacks to arch/riscv.
> > >
> > > Signed-off-by: Anup Patel <anup.patel@wdc.com>
> > > Tested-by: Emil Renner Berhing <kernel@esmil.dk>
> > > ---
> > >  drivers/clocksource/Kconfig       |   9 ++
> > >  drivers/clocksource/Makefile      |   1 +
> > >  drivers/clocksource/timer-clint.c | 231 ++++++++++++++++++++++++++++++
> > >  include/linux/cpuhotplug.h        |   1 +
> > >  4 files changed, 242 insertions(+)
> > >  create mode 100644 drivers/clocksource/timer-clint.c
> > >
> > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > > index 91418381fcd4..e1ce0d510a03 100644
> > > --- a/drivers/clocksource/Kconfig
> > > +++ b/drivers/clocksource/Kconfig
> > > @@ -658,6 +658,15 @@ config RISCV_TIMER
> > >           is accessed via both the SBI and the rdcycle instruction.  This is
> > >           required for all RISC-V systems.
> > >
> > > +config CLINT_TIMER
> > > +       bool "Timer for the RISC-V platform"
> > > +       depends on GENERIC_SCHED_CLOCK && RISCV_M_MODE
> > > +       select TIMER_PROBE
> > > +       select TIMER_OF
> > > +       help
> > > +         This option enables the CLINT timer for RISC-V systems. The CLINT
> > > +         driver is usually used for NoMMU RISC-V systems.
> > > +
> > >  config CSKY_MP_TIMER
> > >         bool "SMP Timer for the C-SKY platform" if COMPILE_TEST
> > >         depends on CSKY
> > > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> > > index bdda1a2e4097..18e700e703a0 100644
> > > --- a/drivers/clocksource/Makefile
> > > +++ b/drivers/clocksource/Makefile
> > > @@ -87,6 +87,7 @@ obj-$(CONFIG_CLKSRC_ST_LPC)           += clksrc_st_lpc.o
> > >  obj-$(CONFIG_X86_NUMACHIP)             += numachip.o
> > >  obj-$(CONFIG_ATCPIT100_TIMER)          += timer-atcpit100.o
> > >  obj-$(CONFIG_RISCV_TIMER)              += timer-riscv.o
> > > +obj-$(CONFIG_CLINT_TIMER)              += timer-clint.o
> > >  obj-$(CONFIG_CSKY_MP_TIMER)            += timer-mp-csky.o
> > >  obj-$(CONFIG_GX6605S_TIMER)            += timer-gx6605s.o
> > >  obj-$(CONFIG_HYPERV_TIMER)             += hyperv_timer.o
> > > diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer-clint.c
> > > new file mode 100644
> > > index 000000000000..e1698efa73a1
> > > --- /dev/null
> > > +++ b/drivers/clocksource/timer-clint.c
> > > @@ -0,0 +1,231 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) 2020 Western Digital Corporation or its affiliates.
> > > + *
> > > + * Most of the M-mode (i.e. NoMMU) RISC-V systems usually have a
> > > + * CLINT MMIO timer device.
> > > + */
> > > +
> > > +#define pr_fmt(fmt) "clint: " fmt
> > > +#include <linux/bitops.h>
> > > +#include <linux/clocksource.h>
> > > +#include <linux/clockchips.h>
> > > +#include <linux/cpu.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of_address.h>
> > > +#include <linux/sched_clock.h>
> > > +#include <linux/io-64-nonatomic-lo-hi.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/of_irq.h>
> > > +#include <linux/smp.h>
> > > +
> > > +#define CLINT_IPI_OFF          0
> > > +#define CLINT_TIMER_CMP_OFF    0x4000
> > > +#define CLINT_TIMER_VAL_OFF    0xbff8
> > > +
> > > +/* CLINT manages IPI and Timer for RISC-V M-mode  */
> > > +static u32 __iomem *clint_ipi_base;
> > > +static u64 __iomem *clint_timer_cmp;
> > > +static u64 __iomem *clint_timer_val;
> > > +static unsigned long clint_timer_freq;
> > > +static unsigned int clint_timer_irq;
> > > +
> > > +static void clint_send_ipi(const struct cpumask *target)
> > > +{
> > > +       unsigned int cpu;
> > > +
> > > +       for_each_cpu(cpu, target)
> > > +               writel(1, clint_ipi_base + cpuid_to_hartid_map(cpu));
> > > +}
> > > +
> > > +static void clint_clear_ipi(void)
> > > +{
> > > +       writel(0, clint_ipi_base + cpuid_to_hartid_map(smp_processor_id()));
> > > +}
> > > +
> > > +static struct riscv_ipi_ops clint_ipi_ops = {
> > > +       .ipi_inject = clint_send_ipi,
> > > +       .ipi_clear = clint_clear_ipi,
> > > +};
> > > +
> > > +#ifdef CONFIG_64BIT
> > > +#define clint_get_cycles()     readq_relaxed(clint_timer_val)
> > > +#else
> > > +#define clint_get_cycles()     readl_relaxed(clint_timer_val)
> > > +#define clint_get_cycles_hi()  readl_relaxed(((u32 *)clint_timer_val) + 1)
> > > +#endif
> > > +
> > > +#ifdef CONFIG_64BIT
> > > +static u64 notrace clint_get_cycles64(void)
> > > +{
> > > +       return clint_get_cycles();
> > > +}
> > > +#else /* CONFIG_64BIT */
> > > +static u64 notrace clint_get_cycles64(void)
> > > +{
> > > +       u32 hi, lo;
> > > +
> > > +       do {
> > > +               hi = clint_get_cycles_hi();
> > > +               lo = clint_get_cycles();
> > > +       } while (hi != clint_get_cycles_hi());
> > > +
> > > +       return ((u64)hi << 32) | lo;
> > > +}
> > > +#endif /* CONFIG_64BIT */
> > > +
> > > +static u64 clint_rdtime(struct clocksource *cs)
> > > +{
> > > +       return clint_get_cycles64();
> > > +}
> > > +
> > > +static struct clocksource clint_clocksource = {
> > > +       .name           = "clint_clocksource",
> > > +       .rating = 300,
> >
> > nit: Not aligned with other structure members.
> >
> > > +       .mask           = CLOCKSOURCE_MASK(64),
> > > +       .flags          = CLOCK_SOURCE_IS_CONTINUOUS,
> > > +       .read           = clint_rdtime,
> > > +};
> > > +
> > > +static int clint_clock_next_event(unsigned long delta,
> > > +                                  struct clock_event_device *ce)
> > > +{
> > > +       void __iomem *r = clint_timer_cmp +
> > > +                         cpuid_to_hartid_map(smp_processor_id());
> > > +
> > > +       csr_set(CSR_IE, IE_TIE);
> > > +       writeq_relaxed(clint_get_cycles64() + delta, r);
> > > +       return 0;
> > > +}
> > > +
> > > +static DEFINE_PER_CPU(struct clock_event_device, clint_clock_event) = {
> > > +       .name                   = "clint_clockevent",
> > > +       .features               = CLOCK_EVT_FEAT_ONESHOT,
> > > +       .rating         = 100,
> > > +       .set_next_event = clint_clock_next_event,
> >
> > nit: Not aligned with other structure members.
>
> Okay, will update.
>
> > > +};
> > > +
> > > +static DEFINE_PER_CPU(bool, clint_clock_event_registered);
> > > +
> > > +static int clint_timer_starting_cpu(unsigned int cpu)
> > > +{
> > > +       bool *registered = per_cpu_ptr(&clint_clock_event_registered, cpu);
> > > +       struct clock_event_device *ce = per_cpu_ptr(&clint_clock_event, cpu);
> > > +
> > > +       if (!(*registered)) {
> > > +               ce->cpumask = cpumask_of(cpu);
> > > +               clockevents_config_and_register(ce, clint_timer_freq, 200,
> > > +                                                ULONG_MAX);
> >
> > Is there a specific reason to choose different values from the timer-riscv ?
> > The min_delta is set to 100 and max_delta is set to 0x7fffffff in
> > timer-riscv driver.
>
> Not really, I will update. I think it's better to use 100 and ULONG_MAX to
> have fewer magic values.
>

Yeah. Looking at the other timer driver, max_delta is all over the place :).
I guess we should just keep the same max/min_delta between riscv timer
and clint driver.

> >
> > > +               *registered = true;
> > > +       }
> > > +
> > > +       enable_percpu_irq(clint_timer_irq,
> > > +                         irq_get_trigger_type(clint_timer_irq));
> > > +       return 0;
> > > +}
> > > +
> > > +static int clint_timer_dying_cpu(unsigned int cpu)
> > > +{
> > > +       disable_percpu_irq(clint_timer_irq);
> > > +       return 0;
> > > +}
> > > +
> > > +static irqreturn_t clint_timer_interrupt(int irq, void *dev_id)
> > > +{
> > > +       struct clock_event_device *evdev = this_cpu_ptr(&clint_clock_event);
> > > +
> > > +       csr_clear(CSR_IE, IE_TIE);
> > > +       evdev->event_handler(evdev);
> > > +
> > > +       return IRQ_HANDLED;
> > > +}
> > > +
> > > +static int __init clint_timer_init_dt(struct device_node *np)
> > > +{
> > > +       int rc;
> > > +       u32 i, nr_irqs;
> > > +       void __iomem *base;
> > > +       struct of_phandle_args oirq;
> > > +
> > > +       /*
> > > +        * Ensure that CLINT device interrupts are either RV_IRQ_TIMER or
> > > +        * RV_IRQ_SOFT. If it's anything else then we ignore the device.
> > > +        */
> > > +       nr_irqs = of_irq_count(np);
> > > +       for (i = 0; i < nr_irqs; i++) {
> > > +               if (of_irq_parse_one(np, i, &oirq)) {
> > > +                       pr_err("%pOFP: failed to parse irq %d.\n", np, i);
> > > +                       continue;
> > > +               }
> > > +
> > > +               if ((oirq.args_count != 1) ||
> > > +                   (oirq.args[0] != RV_IRQ_TIMER &&
> > > +                    oirq.args[0] != RV_IRQ_SOFT)) {
> > > +                       pr_err("%pOFP: invalid irq %d (hwirq %d)\n",
> > > +                              np, i, oirq.args[0]);
> > > +                       return -ENODEV;
> > > +               }
> > > +
> > > +               /* Find parent irq domain and map timer irq */
> > > +               if (!clint_timer_irq &&
> > > +                   oirq.args[0] == RV_IRQ_TIMER &&
> > > +                   irq_find_host(oirq.np))
> > > +                       clint_timer_irq = irq_of_parse_and_map(np, i);
> > > +       }
> > > +
> > > +       /* If CLINT timer irq not found then fail */
> > > +       if (!clint_timer_irq) {
> > > +               pr_err("%pOFP: timer irq not found\n", np);
> > > +               return -ENODEV;
> > > +       }
> > > +
> > > +       base = of_iomap(np, 0);
> > > +       if (!base) {
> > > +               pr_err("%pOFP: could not map registers\n", np);
> > > +               return -ENODEV;
> > > +       }
> > > +
> > > +       clint_ipi_base = base + CLINT_IPI_OFF;
> > > +       clint_timer_cmp = base + CLINT_TIMER_CMP_OFF;
> > > +       clint_timer_val = base + CLINT_TIMER_VAL_OFF;
> > > +       clint_timer_freq = riscv_timebase;
> > > +
> > > +       pr_info("%pOFP: timer running at %ld Hz\n", np, clint_timer_freq);
> > > +
> > > +       rc = clocksource_register_hz(&clint_clocksource, clint_timer_freq);
> > > +       if (rc) {
> > > +               iounmap(base);
> > > +               pr_err("%pOFP: clocksource register failed [%d]\n", np, rc);
> > > +               return rc;
> > > +       }
> > > +
> > > +       sched_clock_register(clint_get_cycles64, 64, clint_timer_freq);
> > > +
> > > +       rc = request_percpu_irq(clint_timer_irq, clint_timer_interrupt,
> > > +                                "clint-timer", &clint_clock_event);
> > > +       if (rc) {
> > > +               iounmap(base);
> > > +               pr_err("registering percpu irq failed [%d]\n", rc);
> > > +               return rc;
> > > +       }
> > > +
> > > +       rc = cpuhp_setup_state(CPUHP_AP_CLINT_TIMER_STARTING,
> > > +                               "clockevents/clint/timer:starting",
> > > +                               clint_timer_starting_cpu,
> > > +                               clint_timer_dying_cpu);
> > > +       if (rc) {
> > > +               free_irq(clint_timer_irq, &clint_clock_event);
> > > +               iounmap(base);
> > > +               pr_err("%pOFP: cpuhp setup state failed [%d]\n", np, rc);
> > > +               return rc;
> >
> > All the iounmap & return statements can be moved to a goto at the end
> > of the function.
>
> Okay, will update.
>
> >
> > > +       }
> > > +
> > > +       riscv_set_ipi_ops(&clint_ipi_ops);
> > > +       clint_clear_ipi();
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +TIMER_OF_DECLARE(clint_timer, "riscv,clint0", clint_timer_init_dt);
> > > +TIMER_OF_DECLARE(clint_timer1, "sifive,clint0", clint_timer_init_dt);
> > > diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> > > index 191772d4a4d7..1451f4625833 100644
> > > --- a/include/linux/cpuhotplug.h
> > > +++ b/include/linux/cpuhotplug.h
> > > @@ -132,6 +132,7 @@ enum cpuhp_state {
> > >         CPUHP_AP_MIPS_GIC_TIMER_STARTING,
> > >         CPUHP_AP_ARC_TIMER_STARTING,
> > >         CPUHP_AP_RISCV_TIMER_STARTING,
> > > +       CPUHP_AP_CLINT_TIMER_STARTING,
> > >         CPUHP_AP_CSKY_TIMER_STARTING,
> > >         CPUHP_AP_HYPERV_TIMER_STARTING,
> > >         CPUHP_AP_KVM_STARTING,
> > > --
> > > 2.25.1
> > >
> > >
> > > _______________________________________________
> > > linux-riscv mailing list
> > > linux-riscv@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> >
> >
> >
> > --
> > Regards,
> > Atish
>
> Regards,
> Anup



-- 
Regards,
Atish

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 2/4] clocksource/drivers: Add CLINT timer driver
  2020-07-21 12:15       ` Daniel Lezcano
@ 2020-07-22  3:36         ` Anup Patel
  0 siblings, 0 replies; 19+ messages in thread
From: Anup Patel @ 2020-07-22  3:36 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: devicetree, Damien Le Moal, Albert Ou, Emil Renner Berhing,
	Anup Patel, linux-kernel@vger.kernel.org List, Atish Patra,
	Rob Herring, Palmer Dabbelt, Paul Walmsley, Alistair Francis,
	Thomas Gleixner, linux-riscv

On Tue, Jul 21, 2020 at 5:45 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 21/07/2020 13:49, Anup Patel wrote:
> > On Tue, Jul 21, 2020 at 4:32 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 17/07/2020 09:50, Anup Patel wrote:
> >>> We add a separate CLINT timer driver for Linux RISC-V M-mode (i.e.
> >>> RISC-V NoMMU kernel).
> >>>
> >>> The CLINT MMIO device provides three things:
> >>> 1. 64bit free running counter register
> >>> 2. 64bit per-CPU time compare registers
> >>> 3. 32bit per-CPU inter-processor interrupt registers
> >>>
> >>> Unlike other timer devices, CLINT provides IPI registers along with
> >>> timer registers. To use CLINT IPI registers, the CLINT timer driver
> >>> provides IPI related callbacks to arch/riscv.
> >>>
> >>> Signed-off-by: Anup Patel <anup.patel@wdc.com>
> >>> Tested-by: Emil Renner Berhing <kernel@esmil.dk>
> >>> ---
> >>>  drivers/clocksource/Kconfig       |   9 ++
> >>>  drivers/clocksource/Makefile      |   1 +
> >>>  drivers/clocksource/timer-clint.c | 231 ++++++++++++++++++++++++++++++
> >>>  include/linux/cpuhotplug.h        |   1 +
> >>>  4 files changed, 242 insertions(+)
> >>>  create mode 100644 drivers/clocksource/timer-clint.c
> >>>
> >>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> >>> index 91418381fcd4..e1ce0d510a03 100644
> >>> --- a/drivers/clocksource/Kconfig
> >>> +++ b/drivers/clocksource/Kconfig
> >>> @@ -658,6 +658,15 @@ config RISCV_TIMER
> >>>         is accessed via both the SBI and the rdcycle instruction.  This is
> >>>         required for all RISC-V systems.
> >>>
> >>> +config CLINT_TIMER
> >>> +     bool "Timer for the RISC-V platform"
> >>> +     depends on GENERIC_SCHED_CLOCK && RISCV_M_MODE
> >>> +     select TIMER_PROBE
> >>> +     select TIMER_OF
> >>> +     help
> >>> +       This option enables the CLINT timer for RISC-V systems. The CLINT
> >>> +       driver is usually used for NoMMU RISC-V systems.
> >>
> >> V3 has a comment about fixing the Kconfig option.
> >
> > I have removed "default y" from the Kconfig option as-per your suggestions.
> >
> > I looked at other Timer Kconfig options. Most of them have menuconfig name.
> > Also, we can certainly have different timer MMIO timer drivers in future. Do
> > you still insist on making this kconfig option totally silent ??
>
> Yes, and there is an effort to change the entries to be silent as much
> as possible.
>
> Just add:
>
>         bool "Timer for the RISC-V platform" if COMPILE_TEST

Okay, I will update.

>
> and remove the RISCV_M_MODE dependency.

CLINT driver depends on RISC-V specific symbols from asm/smp.h
so we should at least have "depends on RISCV" so that compile
test does not fail.

Agree ??

>
> Or alternatively:
>
> replace the RISCV_M_MODE dependency with COMPILE_TEST
>
> The goal is to be able to compile the driver on different platforms for
> compilation test covering.

Please see the above comment.

>
> Then when more mmio drivers will added we will figure out.
>
> >> [ ... ]
> >>
> >>> +{
> >>> +     bool *registered = per_cpu_ptr(&clint_clock_event_registered, cpu);
> >>> +     struct clock_event_device *ce = per_cpu_ptr(&clint_clock_event, cpu);
> >>> +
> >>> +     if (!(*registered)) {
> >>> +             ce->cpumask = cpumask_of(cpu);
> >>> +             clockevents_config_and_register(ce, clint_timer_freq, 200,
> >>> +                                              ULONG_MAX);
> >>> +             *registered = true;
> >>> +     }
> >>
> >>
> >> I was unsure about the clockevents_config_and_register() multiple calls
> >> when doing the comment. It seems like it is fine to call it several
> >> times and that is done in several places like riscv or arch_arm_timer.
> >>
> >> It is probably safe to drop the 'registered' code here, sorry for the
> >> confusion.
> >
> > Okay, will revert these changes.
> >
> >>
> >>> +     enable_percpu_irq(clint_timer_irq,
> >>> +                       irq_get_trigger_type(clint_timer_irq));
> >>> +     return 0;
> >>> +}
> >>> +
> >>
> >> [ ... ]
> >>
> >>
> >> --
> >> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> >>
> >> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> >> <http://twitter.com/#!/linaroorg> Twitter |
> >> <http://www.linaro.org/linaro-blog/> Blog
> >
> > Regards,
> > Anup
> >
>
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog

Regards,
Anup

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 4/4] dt-bindings: timer: Add CLINT bindings
  2020-07-21 12:17       ` Sean Anderson
@ 2020-07-22  3:55         ` Anup Patel
  2020-07-26 18:37           ` Palmer Dabbelt
  0 siblings, 1 reply; 19+ messages in thread
From: Anup Patel @ 2020-07-22  3:55 UTC (permalink / raw)
  To: Sean Anderson
  Cc: devicetree, Damien Le Moal, Daniel Lezcano, Emil Renner Berhing,
	Anup Patel, linux-kernel@vger.kernel.org List, Atish Patra,
	Rob Herring, Palmer Dabbelt, Paul Walmsley, Atish Patra,
	Palmer Dabbelt, Alistair Francis, Thomas Gleixner, linux-riscv,
	Albert Ou

On Tue, Jul 21, 2020 at 5:48 PM Sean Anderson <seanga2@gmail.com> wrote:
>
> On 7/20/20 9:15 PM, Atish Patra wrote:
> > On Fri, Jul 17, 2020 at 12:52 AM Anup Patel <anup.patel@wdc.com> wrote:
> >>
> >> We add DT bindings documentation for CLINT device.
> >>
> >> Signed-off-by: Anup Patel <anup.patel@wdc.com>
> >> Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
> >> Tested-by: Emil Renner Berhing <kernel@esmil.dk>
> >> ---
> >>  .../bindings/timer/sifive,clint.yaml          | 58 +++++++++++++++++++
> >>  1 file changed, 58 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/timer/sifive,clint.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/timer/sifive,clint.yaml b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> >> new file mode 100644
> >> index 000000000000..8ad115611860
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> >> @@ -0,0 +1,58 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/timer/sifive,clint.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: SiFive Core Local Interruptor
> >> +
> >> +maintainers:
> >> +  - Palmer Dabbelt <palmer@dabbelt.com>
> >> +  - Anup Patel <anup.patel@wdc.com>
> >> +
> >> +description:
> >> +  SiFive (and other RISC-V) SOCs include an implementation of the SiFive
> >> +  Core Local Interruptor (CLINT) for M-mode timer and M-mode inter-processor
> >> +  interrupts. It directly connects to the timer and inter-processor interrupt
> >> +  lines of various HARTs (or CPUs) so RISC-V per-HART (or per-CPU) local
> >> +  interrupt controller is the parent interrupt controller for CLINT device.
> >> +  The clock frequency of CLINT is specified via "timebase-frequency" DT
> >> +  property of "/cpus" DT node. The "timebase-frequency" DT property is
> >> +  described in Documentation/devicetree/bindings/riscv/cpus.yaml
> >> +
> >> +properties:
> >> +  compatible:
> >> +    items:
> >> +      - const: sifive,clint0
> >> +      - const: sifive,fu540-c000-clint
> >> +
> >> +    description:
> >> +      Should be "sifive,<chip>-clint" and "sifive,clint<version>".
> >> +      Supported compatible strings are -
> >> +      "sifive,fu540-c000-clint" for the SiFive CLINT v0 as integrated
> >> +      onto the SiFive FU540 chip, and "sifive,clint0" for the SiFive
> >> +      CLINT v0 IP block with no chip integration tweaks.
> >> +      Please refer to sifive-blocks-ip-versioning.txt for details
> >> +
> >
> > As the DT binding suggests that the clint device should be named as "sifive,**",
> > I think we should change the DT property in kendryte dts as well.
>
> The kendryte device is based on Rocket Chip, not any SiFive IP/device.
> If anything, the general binding should be "chipsalliance,clint" and the
> specific bindings should be "sifive,clint" and "kendryte,clint" (or
> "canaan,clint").

AFAIK, Palmer clearly mentioned in previous discussion that CLINT
spec is still owned by SiFive. No matter who implements CLINT device
in their SOC, we will need one compatible string to represent the
spec version (i.e. "sifive,clint0") and another compatible representing
specific implementation (for kendryte this can be "kendryte,k210-clint").

Regards,
Anup

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 4/4] dt-bindings: timer: Add CLINT bindings
  2020-07-22  3:55         ` Anup Patel
@ 2020-07-26 18:37           ` Palmer Dabbelt
  2020-07-27 11:47             ` Sean Anderson
  0 siblings, 1 reply; 19+ messages in thread
From: Palmer Dabbelt @ 2020-07-26 18:37 UTC (permalink / raw)
  To: atishp, seanga2
  Cc: devicetree, Damien Le Moal, daniel.lezcano, kernel, Anup Patel,
	linux-kernel, Atish Patra, robh+dt, Alistair Francis,
	Paul Walmsley, atishp, tglx, linux-riscv, aou

On Tue, 21 Jul 2020 20:55:31 PDT (-0700), anup@brainfault.org wrote:
> On Tue, Jul 21, 2020 at 5:48 PM Sean Anderson <seanga2@gmail.com> wrote:
>>
>> On 7/20/20 9:15 PM, Atish Patra wrote:
>> > On Fri, Jul 17, 2020 at 12:52 AM Anup Patel <anup.patel@wdc.com> wrote:
>> >>
>> >> We add DT bindings documentation for CLINT device.
>> >>
>> >> Signed-off-by: Anup Patel <anup.patel@wdc.com>
>> >> Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
>> >> Tested-by: Emil Renner Berhing <kernel@esmil.dk>
>> >> ---
>> >>  .../bindings/timer/sifive,clint.yaml          | 58 +++++++++++++++++++
>> >>  1 file changed, 58 insertions(+)
>> >>  create mode 100644 Documentation/devicetree/bindings/timer/sifive,clint.yaml
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/timer/sifive,clint.yaml b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
>> >> new file mode 100644
>> >> index 000000000000..8ad115611860
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
>> >> @@ -0,0 +1,58 @@
>> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> >> +%YAML 1.2
>> >> +---
>> >> +$id: http://devicetree.org/schemas/timer/sifive,clint.yaml#
>> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> >> +
>> >> +title: SiFive Core Local Interruptor
>> >> +
>> >> +maintainers:
>> >> +  - Palmer Dabbelt <palmer@dabbelt.com>
>> >> +  - Anup Patel <anup.patel@wdc.com>
>> >> +
>> >> +description:
>> >> +  SiFive (and other RISC-V) SOCs include an implementation of the SiFive
>> >> +  Core Local Interruptor (CLINT) for M-mode timer and M-mode inter-processor
>> >> +  interrupts. It directly connects to the timer and inter-processor interrupt
>> >> +  lines of various HARTs (or CPUs) so RISC-V per-HART (or per-CPU) local
>> >> +  interrupt controller is the parent interrupt controller for CLINT device.
>> >> +  The clock frequency of CLINT is specified via "timebase-frequency" DT
>> >> +  property of "/cpus" DT node. The "timebase-frequency" DT property is
>> >> +  described in Documentation/devicetree/bindings/riscv/cpus.yaml
>> >> +
>> >> +properties:
>> >> +  compatible:
>> >> +    items:
>> >> +      - const: sifive,clint0
>> >> +      - const: sifive,fu540-c000-clint
>> >> +
>> >> +    description:
>> >> +      Should be "sifive,<chip>-clint" and "sifive,clint<version>".
>> >> +      Supported compatible strings are -
>> >> +      "sifive,fu540-c000-clint" for the SiFive CLINT v0 as integrated
>> >> +      onto the SiFive FU540 chip, and "sifive,clint0" for the SiFive
>> >> +      CLINT v0 IP block with no chip integration tweaks.
>> >> +      Please refer to sifive-blocks-ip-versioning.txt for details
>> >> +
>> >
>> > As the DT binding suggests that the clint device should be named as "sifive,**",
>> > I think we should change the DT property in kendryte dts as well.
>>
>> The kendryte device is based on Rocket Chip, not any SiFive IP/device.
>> If anything, the general binding should be "chipsalliance,clint" and the
>> specific bindings should be "sifive,clint" and "kendryte,clint" (or
>> "canaan,clint").
>
> AFAIK, Palmer clearly mentioned in previous discussion that CLINT
> spec is still owned by SiFive. No matter who implements CLINT device
> in their SOC, we will need one compatible string to represent the
> spec version (i.e. "sifive,clint0") and another compatible representing
> specific implementation (for kendryte this can be "kendryte,k210-clint").

I think "sifive,clint*" is the way to go here.  The Free Chips Foundation owns
Rocket Chip, which contains an implementation of SiFive's CLINT specification,
but as far as I can tell Free Chips itself does not produce a specification for
the CLINT.  The only CLINT specifications I can find are for SiFive's chips and
are copyright SiFive, and we generally prefer sticking to specifications rather
than implementations for these sorts of things.

To be honest, I'm not even sure the Free Chips CLINT is an implementation of
the SiFive specification: we don't have the source code for those chips, and
while I'm fairly sure the Chisel source code for the CLINT itself on SiFive's
chips is very close to what was in Rocket Chip at the time those chips were
built (though we don't have the source, so we don't know for user), device
specifications depend on the broader SOC context they are embedded inside.  For
example: SiFive's CLINT allows us to use simple MMIO reads of mtime to meet the
RISC-V rdtime requirements, but other bus configurations may not meet those
requirement<F6><F5>s.

If Free Chips publishes a specification for a CLINT and it's compatible with
this version of SiFive's CLINT then I don't see any reason why we couldn't add
that as a compatible string, but as it currently stands there is no Free Chips
CLINT specification.  IMO it would be irresponsible for us to define one on
their behalf.

I don't know anything about Kendryte's specifications, as I haven't read them
myself.  Assuming they define the CLINT's behavior in their SOC manual, then
adding something along the lines of "canaan,kendryte-k210-clint" seems
reasonable to me -- don't really care that much about the specific name, but as
we don't define the Kendryte SOCs then calling it anything more general than
this specific SOC's CLINT seems unreasonable.  AFAIK the Kendryte people don't
get involved with Linux development directly, so that's probably as much as we
can define.

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 4/4] dt-bindings: timer: Add CLINT bindings
  2020-07-26 18:37           ` Palmer Dabbelt
@ 2020-07-27 11:47             ` Sean Anderson
  0 siblings, 0 replies; 19+ messages in thread
From: Sean Anderson @ 2020-07-27 11:47 UTC (permalink / raw)
  To: Palmer Dabbelt, atishp
  Cc: devicetree, Damien Le Moal, daniel.lezcano, kernel, Anup Patel,
	linux-kernel, Atish Patra, robh+dt, Alistair Francis,
	Paul Walmsley, tglx, linux-riscv, aou

On 7/26/20 2:37 PM, Palmer Dabbelt wrote:
> On Tue, 21 Jul 2020 20:55:31 PDT (-0700), anup@brainfault.org wrote:
>> On Tue, Jul 21, 2020 at 5:48 PM Sean Anderson <seanga2@gmail.com> wrote:
>>>
>>> On 7/20/20 9:15 PM, Atish Patra wrote:
>>> > On Fri, Jul 17, 2020 at 12:52 AM Anup Patel <anup.patel@wdc.com> wrote:
>>> >>
>>> >> We add DT bindings documentation for CLINT device.
>>> >>
>>> >> Signed-off-by: Anup Patel <anup.patel@wdc.com>
>>> >> Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
>>> >> Tested-by: Emil Renner Berhing <kernel@esmil.dk>
>>> >> ---
>>> >>  .../bindings/timer/sifive,clint.yaml          | 58 +++++++++++++++++++
>>> >>  1 file changed, 58 insertions(+)
>>> >>  create mode 100644 Documentation/devicetree/bindings/timer/sifive,clint.yaml
>>> >>
>>> >> diff --git a/Documentation/devicetree/bindings/timer/sifive,clint.yaml b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
>>> >> new file mode 100644
>>> >> index 000000000000..8ad115611860
>>> >> --- /dev/null
>>> >> +++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
>>> >> @@ -0,0 +1,58 @@
>>> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> >> +%YAML 1.2
>>> >> +---
>>> >> +$id: http://devicetree.org/schemas/timer/sifive,clint.yaml#
>>> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> >> +
>>> >> +title: SiFive Core Local Interruptor
>>> >> +
>>> >> +maintainers:
>>> >> +  - Palmer Dabbelt <palmer@dabbelt.com>
>>> >> +  - Anup Patel <anup.patel@wdc.com>
>>> >> +
>>> >> +description:
>>> >> +  SiFive (and other RISC-V) SOCs include an implementation of the SiFive
>>> >> +  Core Local Interruptor (CLINT) for M-mode timer and M-mode inter-processor
>>> >> +  interrupts. It directly connects to the timer and inter-processor interrupt
>>> >> +  lines of various HARTs (or CPUs) so RISC-V per-HART (or per-CPU) local
>>> >> +  interrupt controller is the parent interrupt controller for CLINT device.
>>> >> +  The clock frequency of CLINT is specified via "timebase-frequency" DT
>>> >> +  property of "/cpus" DT node. The "timebase-frequency" DT property is
>>> >> +  described in Documentation/devicetree/bindings/riscv/cpus.yaml
>>> >> +
>>> >> +properties:
>>> >> +  compatible:
>>> >> +    items:
>>> >> +      - const: sifive,clint0
>>> >> +      - const: sifive,fu540-c000-clint
>>> >> +
>>> >> +    description:
>>> >> +      Should be "sifive,<chip>-clint" and "sifive,clint<version>".
>>> >> +      Supported compatible strings are -
>>> >> +      "sifive,fu540-c000-clint" for the SiFive CLINT v0 as integrated
>>> >> +      onto the SiFive FU540 chip, and "sifive,clint0" for the SiFive
>>> >> +      CLINT v0 IP block with no chip integration tweaks.
>>> >> +      Please refer to sifive-blocks-ip-versioning.txt for details
>>> >> +
>>> >
>>> > As the DT binding suggests that the clint device should be named as "sifive,**",
>>> > I think we should change the DT property in kendryte dts as well.
>>>
>>> The kendryte device is based on Rocket Chip, not any SiFive IP/device.
>>> If anything, the general binding should be "chipsalliance,clint" and the
>>> specific bindings should be "sifive,clint" and "kendryte,clint" (or
>>> "canaan,clint").
>>
>> AFAIK, Palmer clearly mentioned in previous discussion that CLINT
>> spec is still owned by SiFive. No matter who implements CLINT device
>> in their SOC, we will need one compatible string to represent the
>> spec version (i.e. "sifive,clint0") and another compatible representing
>> specific implementation (for kendryte this can be "kendryte,k210-clint").
> 
> I think "sifive,clint*" is the way to go here.  The Free Chips Foundation owns
> Rocket Chip, which contains an implementation of SiFive's CLINT specification,
> but as far as I can tell Free Chips itself does not produce a specification for
> the CLINT.  The only CLINT specifications I can find are for SiFive's chips and
> are copyright SiFive, and we generally prefer sticking to specifications rather
> than implementations for these sorts of things.

Ah, I wasn't aware that compatibility string assignment was based on
published specifications.

> 
> To be honest, I'm not even sure the Free Chips CLINT is an implementation of
> the SiFive specification: we don't have the source code for those chips, and
> while I'm fairly sure the Chisel source code for the CLINT itself on SiFive's
> chips is very close to what was in Rocket Chip at the time those chips were
> built (though we don't have the source, so we don't know for user), device
> specifications depend on the broader SOC context they are embedded inside.  For
> example: SiFive's CLINT allows us to use simple MMIO reads of mtime to meet the
> RISC-V rdtime requirements, but other bus configurations may not meet those
> requirements.
> 
> If Free Chips publishes a specification for a CLINT and it's compatible with
> this version of SiFive's CLINT then I don't see any reason why we couldn't add
> that as a compatible string, but as it currently stands there is no Free Chips
> CLINT specification.  IMO it would be irresponsible for us to define one on
> their behalf.
> 
> I don't know anything about Kendryte's specifications, as I haven't read them
> myself.  Assuming they define the CLINT's behavior in their SOC manual, then

They don't. I emailed some people from Canaan and they said they used
rocketchip as their core. The best thing we have is the documentation in
their SDK [1]. FWIW, these comments almost exactly match the SiFive's
CLINT documentation [2]. I don't know whether that meets Linux's
standards for a "specification" or not.

> adding something along the lines of "canaan,kendryte-k210-clint" seems
> reasonable to me -- don't really care that much about the specific name, but as
> we don't define the Kendryte SOCs then calling it anything more general than
> this specific SOC's CLINT seems unreasonable.  AFAIK the Kendryte people don't
> get involved with Linux development directly, so that's probably as much as we
> can define.

--Sean

[1] https://github.com/kendryte/kendryte-standalone-sdk/blob/develop/lib/drivers/include/clint.h
[2] e.g. chapter 9 of https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf


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

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2020-07-27 11:47 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-17  7:50 [PATCH v4 0/4] Dedicated CLINT timer driver Anup Patel
2020-07-17  7:50 ` [PATCH v4 1/4] RISC-V: Add mechanism to provide custom IPI operations Anup Patel
2020-07-21  0:46   ` Atish Patra
2020-07-17  7:50 ` [PATCH v4 2/4] clocksource/drivers: Add CLINT timer driver Anup Patel
2020-07-21  1:11   ` Atish Patra
2020-07-21 11:43     ` Anup Patel
2020-07-21 19:39       ` Atish Patra
2020-07-21 11:02   ` Daniel Lezcano
2020-07-21 11:49     ` Anup Patel
2020-07-21 12:15       ` Daniel Lezcano
2020-07-22  3:36         ` Anup Patel
2020-07-17  7:51 ` [PATCH v4 3/4] RISC-V: Remove CLINT related code from timer and arch Anup Patel
2020-07-17  7:51 ` [PATCH v4 4/4] dt-bindings: timer: Add CLINT bindings Anup Patel
2020-07-21  1:15   ` Atish Patra
2020-07-21 11:39     ` Anup Patel
2020-07-21 12:17       ` Sean Anderson
2020-07-22  3:55         ` Anup Patel
2020-07-26 18:37           ` Palmer Dabbelt
2020-07-27 11:47             ` Sean Anderson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).